From ca07ae469e95b51eaa7cab7ee21e17bf19219a1b Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sat, 1 Aug 2026 16:04:21 +0800 Subject: [PATCH] fix(web-shell): deduplicate permission options with same display label (#8250) * fix(web-shell): deduplicate permission options with same display label When the server sends duplicate permission options (e.g. two allow_once entries with identical labels), the Web Shell renders duplicate buttons. Add a deduplicateByLabel pass after ordering to collapse options that resolve to the same i18n key or raw label, keeping the first occurrence. Fixes #8248 * fix(web-shell): deduplicate permission options by id, not i18n category (#8250) * fix(web-shell): prefer server-provided option labels over i18n override (#8250) * fix(web-shell): use data-option-id for ordering test assertions (#8250) * fix(web-shell): document server-label precedence and test empty-label fallback (#8250) * fix(web-shell): reuse memoized display options for initial selection (#8250) * fix(web-shell): localize permission options, using server label only on i18n-key collision (#8250) * test(web-shell): re-add label assertions to option ordering test (#8250) * fix(web-shell): localize plan-mode restore option, never render blank option (#8250) Give restore_previous its own i18n key so plan mode stops colliding with proceed_once and renders fully localized in every language. The collision fallback now degrades an empty server label to the localized string instead of an unlabeled button, and the selection reset stays keyed to request identity so a mid-request option change cannot re-enable a second confirm. * test(web-shell): cover collision guard server-label fallback (#8250) * test(web-shell): guard reset-effect lifecycle and double-submit in ToolApproval (#8250) --------- Co-authored-by: qwen-code-dev-bot Co-authored-by: Qwen Code Bot Co-authored-by: qwen-code-ci-bot --- .../components/messages/ToolApproval.test.ts | 39 +++-- .../components/messages/ToolApproval.test.tsx | 155 +++++++++++++++++- .../components/messages/ToolApproval.tsx | 81 ++++++--- packages/web-shell/client/i18n.tsx | 2 + 4 files changed, 244 insertions(+), 33 deletions(-) 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': '项目内始终允许',