From 083ad6c6b39e5718609d3d7a3e126a7fb8655fe4 Mon Sep 17 00:00:00 2001 From: jackwener Date: Tue, 4 Aug 2026 19:09:05 +0800 Subject: [PATCH 1/2] feat(settings): let a settled value stay a row until you ask to edit it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The display name is read every time Settings opens and changed once, if ever. It was a permanently open text input — the interface spending its space on the rare act and asking the user to fill in something already filled in — and it saved on blur, so there was no way to back out of a change once made. It is a row now: the current value with one affordance to change it, which swaps in place for the editor plus Save / Cancel. The collapsed line carries more than the old input did — 「未设置，Maka 会称呼你"你"」 says what happens when it is empty, which a blank field cannot. Editing costs one click and buys an explicit save and a real Cancel. The shape is Astryx's own settings-sidebar ExpandableRow. The template's implementation is not, because it has three problems the new SettingsExpandableRow exists not to have: - its trigger is `` with preventDefault, which announces itself as a link, ignores Space, and points nowhere. Opening a form is a button. - it has no focus management at all. Expanding moves focus into the editor and Save / Cancel return it to the trigger, verified against the running app rather than asserted: FOCUS_AFTER_EXPAND reports INPUT and FOCUS_AFTER_CANCEL reports 设置. A `wasEditingRef` guard keeps that to user-driven transitions, so a caller mounting already-expanded does not steal focus on first paint. - its Cancel only closes, because its fields edit live. Ours reverts the draft, which is what makes Cancel mean anything. Built on SettingsRow rather than a bare HStack so the collapsed row keeps the Item vocabulary — the settingsRowEnd width ceiling and rows.css container queries apply here too — and the editor sits in a SettingsField, the same full-width block a permanently open input would use. The label survives the swap: collapsed it names the value, expanded it would otherwise be an unlabelled box, so the control's own label is hidden and the name is stated once. Scope is the identity row only. The spec also named the data page's workspace path; design withdrew it on review — that row is a read-only value with actions, not an editable field, so the pattern does not fit. Verified: build, typecheck, lint, format:check, check-dead-css, test:checks. --- .../locales/settings-preferences-copy.ts | 7 +- .../settings/appearance-settings-page.tsx | 39 +++++- .../settings/settings-expandable-row.tsx | 124 ++++++++++++++++++ .../src/renderer/styles/settings/rows.css | 9 ++ 4 files changed, 170 insertions(+), 9 deletions(-) create mode 100644 apps/desktop/src/renderer/settings/settings-expandable-row.tsx diff --git a/apps/desktop/src/renderer/locales/settings-preferences-copy.ts b/apps/desktop/src/renderer/locales/settings-preferences-copy.ts index 2066f2c7c4..fca50fb4de 100644 --- a/apps/desktop/src/renderer/locales/settings-preferences-copy.ts +++ b/apps/desktop/src/renderer/locales/settings-preferences-copy.ts @@ -14,6 +14,9 @@ export type SettingsPreferencesCopy = { displayName: string; displayNameHelp: string; displayNamePlaceholder: string; + displayNameUnset: string; + displayNameChange: string; + displayNameSet: string; interfaceLanguage: string; interfaceLanguageHelp: string; localeOptions: ReadonlyArray; @@ -128,7 +131,7 @@ export type SettingsPreferencesCopy = { const SETTINGS_PREFERENCES_COPY_BY_LOCALE = { zh: { personalization: { - saveFailed: '保存失败', displayName: '显示名称', displayNameHelp: 'Maka 在聊天里会以这个名字称呼你。留空就用默认的“你”。', displayNamePlaceholder: '例如：JK', + saveFailed: '保存失败', displayName: '显示名称', displayNameHelp: 'Maka 在聊天里会以这个名字称呼你。留空就用默认的“你”。', displayNamePlaceholder: '例如：JK', displayNameUnset: '未设置，Maka 会称呼你“你”', displayNameChange: '更改', displayNameSet: '设置', interfaceLanguage: '界面语言', interfaceLanguageHelp: '选择 Maka 界面的显示语言。切换后立即生效，重启后保持。', localeOptions: [['auto', '跟随系统'], ['zh', '中文'], ['en', 'English']], assistantTone: '助手语气偏好', assistantToneHelp: '最多 500 字，只影响回答的语气和风格。权限确认与安全规则不受影响；改动会自动保存。', assistantTonePlaceholder: '例如：技术严谨、偏简洁、不要 emoji。', }, @@ -159,7 +162,7 @@ const SETTINGS_PREFERENCES_COPY_BY_LOCALE = { }, en: { personalization: { - saveFailed: 'Could not save', displayName: 'Display name', displayNameHelp: 'Maka uses this name when addressing you. Leave it blank to use “you”.', displayNamePlaceholder: 'For example: JK', interfaceLanguage: 'Interface language', interfaceLanguageHelp: 'Choose the language used by Maka. Changes apply immediately and persist after restart.', localeOptions: [['auto', 'Follow system'], ['zh', '中文'], ['en', 'English']], assistantTone: 'Assistant tone', assistantToneHelp: 'Up to 500 characters. This changes response style only; permission and safety rules still apply. Changes save automatically.', assistantTonePlaceholder: 'For example: technically rigorous, concise, and no emoji.', + saveFailed: 'Could not save', displayName: 'Display name', displayNameHelp: 'Maka uses this name when addressing you. Leave it blank to use “you”.', displayNamePlaceholder: 'For example: JK', displayNameUnset: 'Not set — Maka will say “you”', displayNameChange: 'Change', displayNameSet: 'Set', interfaceLanguage: 'Interface language', interfaceLanguageHelp: 'Choose the language used by Maka. Changes apply immediately and persist after restart.', localeOptions: [['auto', 'Follow system'], ['zh', '中文'], ['en', 'English']], assistantTone: 'Assistant tone', assistantToneHelp: 'Up to 500 characters. This changes response style only; permission and safety rules still apply. Changes save automatically.', assistantTonePlaceholder: 'For example: technically rigorous, concise, and no emoji.', }, sections: { identity: 'Identity', identityHelp: 'How Maka addresses you, plus interface language and response tone.', diff --git a/apps/desktop/src/renderer/settings/appearance-settings-page.tsx b/apps/desktop/src/renderer/settings/appearance-settings-page.tsx index d6d5fecee6..7f9c090e38 100644 --- a/apps/desktop/src/renderer/settings/appearance-settings-page.tsx +++ b/apps/desktop/src/renderer/settings/appearance-settings-page.tsx @@ -9,6 +9,8 @@ import { VStack, } from '@astryxdesign/core'; import { SettingsField, SettingsPage, SettingsRow, SettingsSection } from './settings-section'; +import { SettingsExpandableRow } from './settings-expandable-row'; +import { getSettingsSharedCopy } from '../locales/settings-shared-copy'; import type { AppSettings, PersonalizationSettings, @@ -69,6 +71,9 @@ export function PersonalizationSettingsPage(props: { const locale = useUiLocale(); const copy = getSettingsPreferencesCopy(locale).personalization; const sections = getSettingsPreferencesCopy(locale).sections; + const sharedCopy = getSettingsSharedCopy(locale); + // At most one row in the group is open — the template's own rule. + const [expandedRow, setExpandedRow] = useState(null); // Persist the tone textarea this long after the user stops typing; blur // flushes immediately regardless. const TONE_AUTOSAVE_DEBOUNCE_MS = 800; @@ -143,10 +148,6 @@ export function PersonalizationSettingsPage(props: { } } - function flushDisplayName(nextValue: string) { - void persistPersonalization({ displayName: nextValue.trim().slice(0, 60) }); - } - function persistLocale(next: UiLocalePreference) { setUiLocale(next); void persistPersonalization({ uiLocale: next }); @@ -177,18 +178,42 @@ export function PersonalizationSettingsPage(props: { neighboring preferences; the full-width tone field uses the vertical row variant. */} - + {/* A name you set once and then read. A permanently-open input asked + the user to fill in something already filled in, and its blur-save + gave them no way to back out of a change. The row reports the + settled value and opens on demand; Cancel puts the draft back. */} + { + setDisplayName(value.displayName); + setExpandedRow('displayName'); + }} + onCancel={() => { + setDisplayName(value.displayName); + setExpandedRow(null); + }} + onSave={async () => { + await persistPersonalization({ displayName: displayName.trim().slice(0, 60) }); + setExpandedRow(null); + }} + > setDisplayName(value.slice(0, 60))} - onBlur={() => flushDisplayName(displayName)} placeholder={copy.displayNamePlaceholder} label={copy.displayName} description={copy.displayNameHelp} + isLabelHidden width="100%" /> - + {/* PR-LANG-PREF-0 (WAWQAQ msg `edc9cb41` + kenji `7e532892` diff --git a/apps/desktop/src/renderer/settings/settings-expandable-row.tsx b/apps/desktop/src/renderer/settings/settings-expandable-row.tsx new file mode 100644 index 0000000000..18c17b5c2f --- /dev/null +++ b/apps/desktop/src/renderer/settings/settings-expandable-row.tsx @@ -0,0 +1,124 @@ +import { useEffect, useId, useRef, type ReactNode } from 'react'; +import { Button, HStack, Text } from '@astryxdesign/core'; +import { SettingsField, SettingsRow } from './settings-section'; + +/** + * A settled value that becomes a form only when the user asks it to. + * + * The shape is Astryx's own — the `settings-sidebar` template's ExpandableRow + * (`@astryxdesign/cli/templates/pages/settings-sidebar/page.tsx:140-193`): a + * row reports its label and current value with one affordance to change it, + * and swaps in place for the editor plus Save / Cancel. At most one row in a + * group is open at a time, which is the caller's `expandedRow` state. + * + * The shape is copied; the template's implementation is not, because it has + * three problems this component exists to not have: + * + * 1. The template's trigger is `` with `preventDefault`. That + * announces itself as a link, ignores Space, and points nowhere. Opening a + * form is a button. + * + * 2. The template has no focus management at all. Expanding moves focus into + * the editor here, and Save / Cancel return it to the trigger the user came + * from — otherwise collapsing drops focus to the top of the document and a + * keyboard user loses their place in the list. + * + * 3. The template's Cancel only closes; its fields edit live, so there is + * nothing to discard. Ours is a real discard: the caller reverts its draft + * in `onCancel`, which is what makes Cancel mean anything. + * + * Built on `SettingsRow` rather than a bare HStack so the collapsed row keeps + * the Item vocabulary the rest of the surface uses — the `settingsRowEnd` + * width ceiling and the container queries in rows.css apply here too. The + * expanded editor sits in a `SettingsField`, the same full-width block a + * permanently-open input would use. + */ +export function SettingsExpandableRow(props: { + label: string; + /** The settled value, shown while collapsed. */ + value: ReactNode; + /** Label for the affordance that opens the editor (更改 / 设置 / 编辑). */ + actionLabel: string; + isEditing: boolean; + isDisabled?: boolean; + /** Save stays disabled until the draft actually differs from the value. */ + canSave?: boolean; + saveLabel: string; + cancelLabel: string; + onEdit(): void; + onCancel(): void; + onSave(): void | Promise; + children: ReactNode; +}) { + const editorId = useId(); + const triggerRef = useRef(null); + const editorRef = useRef(null); + // Only pull focus for a transition the user drove. Without this the row + // would grab focus on first paint whenever a caller mounts already-expanded. + const wasEditingRef = useRef(props.isEditing); + + useEffect(() => { + const opened = props.isEditing && !wasEditingRef.current; + const closed = !props.isEditing && wasEditingRef.current; + wasEditingRef.current = props.isEditing; + if (opened) { + // The first control the editor renders, whatever the caller put there. + const first = editorRef.current?.querySelector( + 'input, textarea, select, button, [contenteditable="true"], [tabindex]:not([tabindex="-1"])', + ); + first?.focus(); + return; + } + if (closed) triggerRef.current?.focus(); + }, [props.isEditing]); + + if (!props.isEditing) { + return ( + + )} + /> + ); + } + + return ( + +
+ {/* The label survives the swap. Collapsed, the row's own label names + the value; expanded, the editor would otherwise be an unlabelled + box — the user pressed 更改 on something and must still be able to + see what. The caller hides the control's own label instead, so the + name is stated once. */} + {props.label} + {props.children} + +
+ + ); +} diff --git a/apps/desktop/src/renderer/styles/settings/rows.css b/apps/desktop/src/renderer/styles/settings/rows.css index f9478f179f..9a0d91aa21 100644 --- a/apps/desktop/src/renderer/styles/settings/rows.css +++ b/apps/desktop/src/renderer/styles/settings/rows.css @@ -141,3 +141,12 @@ min-height: 2rem; } } + +/* The expanded half of SettingsExpandableRow: the editor the caller supplies, + then its Save / Cancel pair. A grid rather than margins so the gap is the + row group's own rhythm and the editor keeps the field block's full width. */ +.settingsExpandableEditor { + display: grid; + grid-template-columns: minmax(0, 1fr); + gap: var(--space-3); +} From 99a247fcecdff85ae584ba519b09f267a3ba37c0 Mon Sep 17 00:00:00 2001 From: jackwener Date: Tue, 4 Aug 2026 19:18:47 +0800 Subject: [PATCH 2/2] fix(settings): keep the row open when the save did not land MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on SettingsExpandableRow. A failed save collapsed the row anyway. `persistPersonalization` catches its error to raise a toast and does not rethrow, so `await` always resolved and `setExpandedRow(null)` always ran: the row shut, showed the old value, and the draft was gone with only a toast to say why. That is exactly the promise this refactor makes — an explicit save means the change either lands or is still sitting in the editor for you to retry — so it is the one failure it could not afford. It returns whether the write landed now, and the row closes only on true. The autosaving fields keep calling it with `void`; they have nowhere to put the answer and are unaffected. An unmounted page after a successful write still reports success, because the write did land — there is just no longer anywhere to reflect it. The trigger also dropped `aria-expanded={false}` and `aria-controls`. Those describe a disclosure, where a trigger stays put while a region opens beside it. This is a mode swap: the trigger unmounts when the editor replaces it, so aria-expanded could never reach true and aria-controls named a node that does not exist while collapsed. Declaring a contract the DOM never honours is worse than declaring none, and the focus move already carries the state change — the part that was measured rather than assumed. Verified: typecheck, lint, format:check, check-dead-css, test:checks, and settings.spec e2e 5/5. The failure path was checked by reading it end to end rather than by test: updateSettings rethrows, the catch returns false, onSave gates on it. An e2e attempt to force the failure could not work — contextBridge freezes window.maka, so patching settings.update from the renderer silently no-ops and the save simply succeeded. Proving this one needs main-process fault injection. --- .../settings/appearance-settings-page.tsx | 21 ++++++++++++++----- .../settings/settings-expandable-row.tsx | 15 ++++++++----- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/apps/desktop/src/renderer/settings/appearance-settings-page.tsx b/apps/desktop/src/renderer/settings/appearance-settings-page.tsx index 7f9c090e38..add1dd12a7 100644 --- a/apps/desktop/src/renderer/settings/appearance-settings-page.tsx +++ b/apps/desktop/src/renderer/settings/appearance-settings-page.tsx @@ -125,24 +125,31 @@ export function PersonalizationSettingsPage(props: { // Shared persist path for every personalization field. Locale has its own // last-write-wins lane so unrelated saves cannot steal rollback ownership. - async function persistPersonalization(patch: Partial) { + // Returns whether the write landed. The autosaving fields ignore it — they + // have nowhere to put the answer — but a row that closes on save has to + // know, or a failed write collapses the row onto the old value and drops + // the draft with only a toast to show for it. + async function persistPersonalization(patch: Partial): Promise { const ticket = ++persistTicketRef.current; const localeTicket = patch.uiLocale === undefined ? null : ++localePersistTicketRef.current; persistPendingCountRef.current += 1; try { const result = await props.onUpdate({ personalization: patch }); - if (!personalizationMountedRef.current) return; + // Saved. An unmounted page just has nowhere left to reflect it. + if (!personalizationMountedRef.current) return true; if (localeTicket !== null && localeTicket === localePersistTicketRef.current) { setUiLocale(result.settings.personalization.uiLocale); } + return true; } catch (error) { - if (!personalizationMountedRef.current) return; + if (!personalizationMountedRef.current) return false; if (localeTicket !== null && localeTicket === localePersistTicketRef.current) { setUiLocale(value.uiLocale); } if (ticket === persistTicketRef.current) { toast.error(copy.saveFailed, settingsActionErrorMessage(error, locale)); } + return false; } finally { persistPendingCountRef.current = Math.max(0, persistPendingCountRef.current - 1); } @@ -199,8 +206,12 @@ export function PersonalizationSettingsPage(props: { setExpandedRow(null); }} onSave={async () => { - await persistPersonalization({ displayName: displayName.trim().slice(0, 60) }); - setExpandedRow(null); + // Only close on a write that landed: a failed save leaves the row + // open with the draft intact, which is the promise explicit saving + // makes and blur-autosave could not keep. + if (await persistPersonalization({ displayName: displayName.trim().slice(0, 60) })) { + setExpandedRow(null); + } }} > ; children: ReactNode; }) { - const editorId = useId(); const triggerRef = useRef(null); const editorRef = useRef(null); // Only pull focus for a transition the user drove. Without this the row @@ -72,6 +71,14 @@ export function SettingsExpandableRow(props: { if (closed) triggerRef.current?.focus(); }, [props.isEditing]); + // The collapsed row carries no aria-expanded / aria-controls on purpose. + // Those describe a disclosure — a trigger that stays put while a region + // beside it opens — and this is a mode swap: the trigger unmounts when the + // editor replaces it, so aria-expanded could never reach true and + // aria-controls would name a node that does not exist while collapsed. + // Declaring a contract the DOM never honours is worse than declaring none; + // the focus move is what tells assistive tech the mode changed, and that + // part is measured rather than assumed. if (!props.isEditing) { return ( )} @@ -96,7 +101,7 @@ export function SettingsExpandableRow(props: { return ( -
+
{/* The label survives the swap. Collapsed, the row's own label names the value; expanded, the editor would otherwise be an unlabelled box — the user pressed 更改 on something and must still be able to