diff --git a/packages/web-shell/client/components/messages/ToolApproval.test.ts b/packages/web-shell/client/components/messages/ToolApproval.test.ts index 172b46f34e..8191b57162 100644 --- a/packages/web-shell/client/components/messages/ToolApproval.test.ts +++ b/packages/web-shell/client/components/messages/ToolApproval.test.ts @@ -182,19 +182,32 @@ describe('ToolApproval', () => { ); }); - const text = container.textContent ?? ''; - const rejectIndex = text.indexOf('Reject'); - const userIndex = text.indexOf('Always allow for this user'); - const projectIndex = text.indexOf('Always allow in this project'); - const serverIndex = text.indexOf('Always allow for this server'); - const toolIndex = text.indexOf('Always allow for this tool'); - const onceIndex = text.indexOf('Yes, allow once'); - expect(rejectIndex).toBeGreaterThanOrEqual(0); - expect(userIndex).toBeGreaterThan(rejectIndex); - expect(projectIndex).toBeGreaterThan(userIndex); - expect(serverIndex).toBeGreaterThan(projectIndex); - expect(toolIndex).toBeGreaterThan(serverIndex); - expect(onceIndex).toBeGreaterThan(toolIndex); + const ids = Array.from(container.querySelectorAll('[data-option-id]')).map( + (el) => el.getAttribute('data-option-id'), + ); + expect(ids).toEqual([ + 'cancel', + 'proceed_always_user', + 'proceed_always_project', + 'proceed_always_server', + 'proceed_always_tool', + 'proceed_once', + ]); + + // The fixture's server labels deliberately differ from the i18n strings + // (e.g. 'Always Allow in project' vs 'Always allow in this project'), so + // asserting the rendered text proves localization overrides server labels. + const labels = Array.from( + container.querySelectorAll('[data-web-shell-option-label]'), + ).map((el) => el.textContent); + expect(labels).toEqual([ + 'Reject', + 'Always allow for this user', + 'Always allow in this project', + 'Always allow for this server', + 'Always allow for this tool', + 'Yes, allow once', + ]); act(() => root.unmount()); }); diff --git a/packages/web-shell/client/components/messages/ToolApproval.test.tsx b/packages/web-shell/client/components/messages/ToolApproval.test.tsx index ce8b4ea3ae..ddc0346823 100644 --- a/packages/web-shell/client/components/messages/ToolApproval.test.tsx +++ b/packages/web-shell/client/components/messages/ToolApproval.test.tsx @@ -90,6 +90,12 @@ function optionButtons(): HTMLButtonElement[] { ); } +function optionLabels(): (string | null | undefined)[] { + return optionButtons().map( + (o) => o.querySelector('[data-web-shell-option-label]')?.textContent, + ); +} + function pressKey(target: Element, key: string): void { act(() => { target.dispatchEvent(new KeyboardEvent('keydown', { key, bubbles: true })); @@ -202,7 +208,7 @@ describe('ToolApproval accessibility', () => { language, ); - expect(optionButtons().map((option) => option.textContent)).toEqual([ + expect(optionLabels()).toEqual([ expect.stringContaining(allowOnceLabel), expect.stringContaining(switchLabel), ]); @@ -309,4 +315,151 @@ describe('ToolApproval accessibility', () => { // we assert the handler leaves the event un-cancelled instead.) expect(event.defaultPrevented).toBe(false); }); + + it('deduplicates options with the same id', () => { + const dupRequest: PermissionRequest = { + id: 'req-dup', + content: [], + options: [ + { id: 'proceed_once', label: 'Allow', kind: 'allow_once' }, + { id: 'proceed_once', label: 'Allow', kind: 'allow_once' }, + { id: 'reject', label: 'Reject', kind: 'reject_once' }, + ], + }; + render(undefined, dupRequest); + const opts = optionButtons(); + expect(opts).toHaveLength(2); + expect(opts[0]!.getAttribute('data-option-id')).toBe('reject'); + expect(opts[1]!.getAttribute('data-option-id')).toBe('proceed_once'); + }); + + it.each([ + [ + 'en' as const, + ['Reject', 'Yes, restore previous mode', 'Yes, allow once'], + ], + ['zh-CN' as const, ['拒绝', '是，恢复之前的模式', '是，允许一次']], + ])( + 'renders plan-mode allow_once options as distinct, localized buttons in %s', + (language, expectedLabels) => { + // plan mode emits two allow_once options (restore_previous + + // proceed_once). They must stay distinct AND both localize: before + // restore_previous got its own i18n key, zh-CN leaked the English server + // labels for both. + render( + undefined, + { + id: 'req-plan', + content: [], + options: [ + { + id: 'restore_previous', + label: 'Yes, restore previous mode (default)', + kind: 'allow_once', + }, + { + id: 'proceed_once', + label: 'Yes, and manually approve edits', + kind: 'allow_once', + }, + { id: 'reject', label: 'Reject', kind: 'reject_once' }, + ], + }, + language, + ); + const opts = optionButtons(); + expect(opts).toHaveLength(3); + expect(opts.map((o) => o.getAttribute('data-option-id'))).toEqual([ + 'reject', + 'restore_previous', + 'proceed_once', + ]); + expect(optionLabels()).toEqual(expectedLabels); + }, + ); + + it('falls back to i18n when a standard option has an empty label', () => { + render(undefined, { + id: 'req-empty', + content: [], + options: [ + { id: 'proceed_once', label: '', kind: 'allow_once' }, + { id: 'reject', label: '', kind: 'reject_once' }, + ], + }); + const labels = optionLabels(); + expect(labels).toContain('Yes, allow once'); + expect(labels).toContain('Reject'); + }); + + it('never renders a blank button when colliding options have empty labels', () => { + // Two generic allow_once options share the allowOnce key, so the collision + // guard reaches for their server labels, but both are empty. It must + // degrade to the localized string (duplicated yet readable) rather than + // render an unlabeled button a screen reader cannot announce. + render(undefined, { + id: 'req-collide-empty', + content: [], + options: [ + { id: 'proceed_once', label: '', kind: 'allow_once' }, + { id: 'proceed_once_alt', label: '', kind: 'allow_once' }, + { id: 'reject', label: 'Reject', kind: 'reject_once' }, + ], + }); + expect(optionLabels()).toEqual([ + 'Reject', + 'Yes, allow once', + 'Yes, allow once', + ]); + }); + + it('falls back to distinct server labels when options share an i18n key', () => { + render(undefined, { + id: 'req-collide', + content: [], + options: [ + { id: 'proceed_once', label: 'Allow A', kind: 'allow_once' }, + { id: 'proceed_once_alt', label: 'Allow B', kind: 'allow_once' }, + { id: 'reject', label: 'Reject', kind: 'reject_once' }, + ], + }); + expect(optionLabels()).toEqual(['Reject', 'Allow A', 'Allow B']); + }); + + it('re-enables confirmation when a new request arrives', () => { + render(undefined); + act(() => optionButtons()[1]!.click()); + expect(onConfirm).toHaveBeenCalledWith('req-1', 'proceed'); + + rerender(undefined, { ...request, id: 'req-2' }); + act(() => optionButtons()[1]!.click()); + expect(onConfirm).toHaveBeenCalledTimes(2); + expect(onConfirm).toHaveBeenLastCalledWith('req-2', 'proceed'); + }); + + it('does not re-arm the submit guard when the same request changes options', () => { + render(undefined, { + id: 'same-id', + content: [], + options: [ + { id: 'reject_always', label: 'Never', kind: 'reject_always' }, + { id: 'proceed_once', label: 'Allow', kind: 'allow_once' }, + ], + }); + act(() => optionButtons()[0]!.click()); + expect(onConfirm).toHaveBeenCalledTimes(1); + + // Same request id, but options change so safeDefaultIndex flips 1 -> 0. + // The reset effect must NOT re-run: it is keyed strictly to request.id. + rerender(undefined, { + id: 'same-id', + content: [], + options: [ + { id: 'cancel', label: 'Reject', kind: 'reject_once' }, + { id: 'proceed_once', label: 'Allow', kind: 'allow_once' }, + ], + }); + act(() => optionButtons()[0]!.click()); + expect(onConfirm).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/web-shell/client/components/messages/ToolApproval.tsx b/packages/web-shell/client/components/messages/ToolApproval.tsx index f9338509eb..857eaa8471 100644 --- a/packages/web-shell/client/components/messages/ToolApproval.tsx +++ b/packages/web-shell/client/components/messages/ToolApproval.tsx @@ -145,6 +145,9 @@ function getOptionI18nKey( if (option.id === 'proceed_once_and_switch_to_default') { return 'approval.option.allowOnceAndSwitchToDefault'; } + if (option.id === 'restore_previous') { + return 'approval.option.restorePrevious'; + } if (option.kind === 'allow_once') return 'approval.option.allowOnce'; if (option.kind === 'reject_once') return 'approval.option.rejectOnce'; if (option.kind === 'allow_always') { @@ -161,6 +164,26 @@ function getOptionI18nKey( return undefined; } +// Production producers (toPermissionOptions) emit distinct ids, so this +// rarely fires; it guards the key={option.id} React duplicate-key warning +// if a producer ever repeats an id. +function deduplicateOptions( + options: PermissionRequest['options'], +): PermissionRequest['options'] { + const seen = new Set(); + return options.filter((option) => { + if (seen.has(option.id)) return false; + seen.add(option.id); + return true; + }); +} + +function prepareDisplayOptions( + options: PermissionRequest['options'], +): PermissionRequest['options'] { + return deduplicateOptions(orderPermissionOptions(options)); +} + function getOptionClassName( option: PermissionRequest['options'][number], ): string { @@ -180,30 +203,55 @@ export function ToolApproval({ }: ToolApprovalProps) { const { t } = useI18n(); const displayOptions = useMemo( - () => orderPermissionOptions(request.options), + () => prepareDisplayOptions(request.options), [request.options], ); - const [selected, setSelected] = useState(() => - getSafeDefaultIndex(orderPermissionOptions(request.options)), + const safeDefaultIndex = useMemo( + () => getSafeDefaultIndex(displayOptions), + [displayOptions], ); + // Prefer the localized label. Known producers give every option a distinct + // i18n key (plan mode's restore_previous has its own), so this normally + // localizes everything. The key count is a last-resort guard: if a future + // producer repeats a generic key, those options fall back to the server's + // distinct labels instead of identical buttons. An empty server label still + // degrades to the localized text, never a blank button without an accessible + // name. + const labelForOption = useMemo(() => { + const keyCount = new Map(); + for (const option of displayOptions) { + const key = getOptionI18nKey(option); + if (key) keyCount.set(key, (keyCount.get(key) ?? 0) + 1); + } + return (option: PermissionRequest['options'][number]) => { + const key = getOptionI18nKey(option); + if (key && keyCount.get(key) === 1) return t(key); + return option.label || (key ? t(key) : ''); + }; + }, [displayOptions, t]); + const [selected, setSelected] = useState(safeDefaultIndex); const requestRef = useRef(request); requestRef.current = request; const selectedRef = useRef(selected); selectedRef.current = selected; const submittedRef = useRef(false); + const safeDefaultIndexRef = useRef(safeDefaultIndex); + safeDefaultIndexRef.current = safeDefaultIndex; const optionRefs = useRef<(HTMLButtonElement | null)[]>([]); const headingId = useId(); const questionId = useId(); const descId = useId(); const commandId = useId(); + // Reset only when a NEW request arrives. Reading the safe default through a + // ref keeps this keyed strictly to request identity: if the same request's + // options (and thus its safe default) change mid-flight, re-running the + // effect would clear submittedRef and re-enable a second confirm for a + // request the user already answered. useEffect(() => { - const safeDefaultIndex = getSafeDefaultIndex( - orderPermissionOptions(requestRef.current.options), - ); submittedRef.current = false; - selectedRef.current = safeDefaultIndex; - setSelected(safeDefaultIndex); + selectedRef.current = safeDefaultIndexRef.current; + setSelected(safeDefaultIndexRef.current); }, [request.id]); const parsedTitle = parseTitle(request.title); @@ -250,14 +298,8 @@ export function ToolApproval({ // Fresh request → safe default; same request re-activated (e.g. a covering // panel closed) → restore the option the user had selected rather than // snapping focus back to the default and silently changing their choice. - focusOption( - requestChanged - ? getSafeDefaultIndex( - orderPermissionOptions(requestRef.current.options), - ) - : selectedRef.current, - ); - }, [keyboardActive, request.id, focusOption]); + focusOption(requestChanged ? safeDefaultIndex : selectedRef.current); + }, [keyboardActive, request.id, focusOption, safeDefaultIndex]); const moveSelection = useCallback( (delta: number) => { @@ -388,8 +430,7 @@ export function ToolApproval({
{displayOptions.map((option, i) => { const isSelected = i === selected; - const i18nKey = getOptionI18nKey(option); - const label = i18nKey ? t(i18nKey) : option.label; + const label = labelForOption(option); return ( ); })} diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index e9d14bf36c..74795242c6 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -427,6 +427,7 @@ const EN: Messages = { 'approval.option.allowOnce': 'Yes, allow once', 'approval.option.allowOnceAndSwitchToDefault': 'Allow once and switch to Default mode', + 'approval.option.restorePrevious': 'Yes, restore previous mode', 'approval.option.rejectOnce': 'Reject', 'approval.option.allowAllEdits': 'Allow All Edits', 'approval.option.allowAlwaysProject': 'Always allow in this project', @@ -3012,6 +3013,7 @@ const ZH: Messages = { 'approval.launchAgentQuestion': '启动这个 agent？', 'approval.option.allowOnce': '是，允许一次', 'approval.option.allowOnceAndSwitchToDefault': '允许一次并切换到默认模式', + 'approval.option.restorePrevious': '是，恢复之前的模式', 'approval.option.rejectOnce': '拒绝', 'approval.option.allowAllEdits': '允许所有编辑', 'approval.option.allowAlwaysProject': '项目内始终允许',