From 49f0a30df2c5210b41ea85c163bb1b22fb6f96a0 Mon Sep 17 00:00:00 2001 From: Stefan Walcz Date: Wed, 21 Oct 2026 15:31:15 +0200 Subject: [PATCH] fix(ui): keep focus in settings sheets when the page re-renders The chat settings sheet, the Talk session sheet and the agent and task preview sheets set up their focus handling in an effect that depended on onClose. Every parent passes onClose as an inline arrow, so each re-render of the page gave it a new identity: the effect cleanup sent focus back to the opener and the effect ran again and focused the Close button. Any edit in a sheet re-renders the page, so focus left the control being used. In the chat sheet, typing in the system prompt moved focus to Close after the first key, and a following space activated Close and dismissed the sheet; an open native dropdown in the sheet closed by itself. Keep the latest onClose in a ref and run the focus effect once per open. Escape still calls the current onClose; initial focus on Close, the Tab trap and restoring focus to the opener are unchanged. The e2e specs type key by key (or switch preview tabs) and assert that focus stays put; they fail without this change. Docs-Not-Needed: bug fix, no user-facing behaviour change beyond the fix Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Stefan Walcz (cherry picked from commit 773cae7259cbd422f3fc368e5ecfd6ecebf899bf) --- .../ui/src/components/talk/SessionSheet.jsx | 9 ++++-- .../http/react-ui/e2e/agent-jobs-task.spec.js | 2 ++ core/http/react-ui/e2e/agents-edit.spec.js | 2 ++ core/http/react-ui/e2e/chat-settings.spec.js | 30 +++++++++++++++++++ core/http/react-ui/e2e/talk.spec.js | 17 +++++++++++ .../src/components/chat/ChatSettingsSheet.jsx | 9 ++++-- core/http/react-ui/src/pages/AgentCreate.jsx | 9 ++++-- .../react-ui/src/pages/AgentTaskDetails.jsx | 9 ++++-- 8 files changed, 79 insertions(+), 8 deletions(-) diff --git a/addons/talk/ui/src/components/talk/SessionSheet.jsx b/addons/talk/ui/src/components/talk/SessionSheet.jsx index 97f31f7b1..b714b428e 100644 --- a/addons/talk/ui/src/components/talk/SessionSheet.jsx +++ b/addons/talk/ui/src/components/talk/SessionSheet.jsx @@ -19,12 +19,17 @@ export default function SessionSheet({ const sheetRef = useRef(null) const closeRef = useRef(null) const openerRef = useRef(null) + // The parent passes a fresh onClose on every render. Read it through a ref + // so the effect below runs once per open: re-running it would move focus to + // the opener and back to Close, pulling it out of the control in use. + const onCloseRef = useRef(onClose) + useEffect(() => { onCloseRef.current = onClose }) useEffect(() => { openerRef.current = document.activeElement closeRef.current?.focus() const onKey = (e) => { - if (e.key === 'Escape') { e.preventDefault(); e.stopPropagation(); onClose(); return } + if (e.key === 'Escape') { e.preventDefault(); e.stopPropagation(); onCloseRef.current(); return } if (e.key !== 'Tab' || !sheetRef.current) return const focusable = Array.from(sheetRef.current.querySelectorAll('button:not([disabled]), input:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])')) if (focusable.length === 0) return @@ -39,7 +44,7 @@ export default function SessionSheet({ const el = openerRef.current if (el && document.contains(el)) el.focus?.() } - }, [onClose]) + }, []) const slots = pipeline && !pipeline.self_contained ? [ diff --git a/core/http/react-ui/e2e/agent-jobs-task.spec.js b/core/http/react-ui/e2e/agent-jobs-task.spec.js index 6fb9793f1..dba88c189 100644 --- a/core/http/react-ui/e2e/agent-jobs-task.spec.js +++ b/core/http/react-ui/e2e/agent-jobs-task.spec.js @@ -161,7 +161,9 @@ test.describe('Edit a task', () => { await page.getByTestId('task-preview-open').click() const sheet = page.getByTestId('task-preview') await expect(sheet.getByTestId('task-preview-prompt')).toContainText('Summarise overnight news about local inference in bullet points format. Keep it to 8 items.') + // Switching tabs re-renders the page; focus stays on the tab. await sheet.getByRole('tab', { name: /Changes/ }).click() + await expect(sheet.getByRole('tab', { name: /Changes/ })).toBeFocused() const changes = sheet.getByTestId('task-preview-changes') await expect(changes.locator('[data-key="cron"]')).toContainText('0 7 * * *') await expect(changes.locator('[data-key="cron"]')).toContainText('0 8 * * *') diff --git a/core/http/react-ui/e2e/agents-edit.spec.js b/core/http/react-ui/e2e/agents-edit.spec.js index 60192d454..47c1058d7 100644 --- a/core/http/react-ui/e2e/agents-edit.spec.js +++ b/core/http/react-ui/e2e/agents-edit.spec.js @@ -60,7 +60,9 @@ test.describe('Create and edit an agent', () => { expect(cfg).not.toContain('sk-secret-value') expect(cfg).not.toContain('tok-123') + // Switching tabs re-renders the page; focus stays on the tab. await sheet.getByRole('tab', { name: /Changes/ }).click() + await expect(sheet.getByRole('tab', { name: /Changes/ })).toBeFocused() const changes = sheet.getByTestId('agent-preview-changes') await expect(changes.locator('[data-key]')).toHaveCount(1) await expect(changes.locator('[data-key="system_prompt"]')).toContainText('You are a careful research assistant') diff --git a/core/http/react-ui/e2e/chat-settings.spec.js b/core/http/react-ui/e2e/chat-settings.spec.js index fd9a78a46..900c0f583 100644 --- a/core/http/react-ui/e2e/chat-settings.spec.js +++ b/core/http/react-ui/e2e/chat-settings.spec.js @@ -80,6 +80,36 @@ test.describe('Chat settings sheet', () => { expect(answer.request().messages[0]).toEqual({ role: 'system', content: 'Answer in one line.' }) }) + // Every edit in the sheet re-renders the chat page, which hands the sheet a + // new onClose. Focus must stay where the user put it: before the fix each + // re-render sent focus to the opener and then to the Close button, so a + // second keystroke went nowhere and an open native dropdown closed itself. + test('keeps focus on the control being edited while the page re-renders', async ({ page }) => { + await openChat(page, [chatOf('c1', 'Settings', 'qwen3-8b', pair('hi', 'hello'))]) + await open(page) + + const prompt = sheet(page).getByTestId('chat-system-prompt') + await prompt.click() + await page.keyboard.type('Be brief.', { delay: 20 }) + await expect(prompt).toBeFocused() + await expect(prompt).toHaveValue('Be brief.') + await expect.poll(async () => (await storedChats(page)).chats[0].systemPrompt).toBe('Be brief.') + + const topK = sheet(page).locator('.cx-field', { hasText: 'Top K' }).getByRole('slider') + await topK.focus() + await page.keyboard.press('ArrowRight') + await page.keyboard.press('ArrowRight') + await expect(topK).toBeFocused() + await expect.poll(async () => (await storedChats(page)).chats[0].topK).toBe(42) + + // Escape still reaches the current onClose, and focus goes back to the + // button that opened the sheet. + await expect(sheet(page)).toBeVisible() + await page.keyboard.press('Escape') + await expect(sheet(page)).toHaveCount(0) + await expect(page.getByTestId('chat-settings-button')).toBeFocused() + }) + test('the context size has quick sizes, drives the header meter and is not sent', async ({ page }) => { const answer = await answerWith(page, 'ok') await openChat(page, [chatOf('c1', 'Settings', 'qwen3-8b', pair('hi', 'hello'), { tokenUsage: { prompt: 1000, completion: 96, total: 1096 } })]) diff --git a/core/http/react-ui/e2e/talk.spec.js b/core/http/react-ui/e2e/talk.spec.js index 540bbb743..073eaddc3 100644 --- a/core/http/react-ui/e2e/talk.spec.js +++ b/core/http/react-ui/e2e/talk.spec.js @@ -263,6 +263,23 @@ test.describe('Talk session settings', () => { expect(update.session.audio.input.transcription.language).toBe('it') }) + // Each keystroke re-renders the Talk page, which hands the sheet a new + // onClose. Focus has to stay in the field rather than jump to Close. + test('typing in the sheet keeps focus in the field while the page re-renders', async ({ page }) => { + await fakeRealtime(page) + await page.goto('/app/talk') + await page.getByTestId('talk-settings-button').click() + const sheet = page.getByTestId('talk-settings') + const instructions = sheet.getByRole('textbox', { name: 'Instructions' }) + await instructions.fill('') + await page.keyboard.type('Be brief.', { delay: 20 }) + await expect(instructions).toBeFocused() + await expect(instructions).toHaveValue('Be brief.') + await page.keyboard.press('Escape') + await expect(sheet).toHaveCount(0) + await expect(page.getByTestId('talk-settings-button')).toBeFocused() + }) + test('Manage mode is fixed while a session is open', async ({ page }) => { await fakeRealtime(page) await startSession(page) diff --git a/core/http/react-ui/src/components/chat/ChatSettingsSheet.jsx b/core/http/react-ui/src/components/chat/ChatSettingsSheet.jsx index e39b2099f..fc76600e2 100644 --- a/core/http/react-ui/src/components/chat/ChatSettingsSheet.jsx +++ b/core/http/react-ui/src/components/chat/ChatSettingsSheet.jsx @@ -64,12 +64,17 @@ export default function ChatSettingsSheet({ const sheetRef = useRef(null) const closeRef = useRef(null) const openerRef = useRef(null) + // The page passes a fresh onClose on every render. Read it through a ref so + // the effect below runs once per open: re-running it would move focus to + // the opener and back to Close, pulling it out of the control being edited. + const onCloseRef = useRef(onClose) + useEffect(() => { onCloseRef.current = onClose }) useEffect(() => { openerRef.current = document.activeElement closeRef.current?.focus() const onKey = (e) => { - if (e.key === 'Escape') { e.preventDefault(); e.stopPropagation(); onClose(); return } + if (e.key === 'Escape') { e.preventDefault(); e.stopPropagation(); onCloseRef.current(); return } if (e.key !== 'Tab' || !sheetRef.current) return const focusable = Array.from(sheetRef.current.querySelectorAll('button:not([disabled]), input:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])')) if (focusable.length === 0) return @@ -84,7 +89,7 @@ export default function ChatSettingsSheet({ const el = openerRef.current if (el && document.contains(el)) el.focus?.() } - }, [onClose]) + }, []) const set = (patch) => onUpdate(patch) const used = chat.tokenUsage?.total || 0 diff --git a/core/http/react-ui/src/pages/AgentCreate.jsx b/core/http/react-ui/src/pages/AgentCreate.jsx index 617b1d6fc..f962f1bcb 100644 --- a/core/http/react-ui/src/pages/AgentCreate.jsx +++ b/core/http/react-ui/src/pages/AgentCreate.jsx @@ -1128,12 +1128,17 @@ function PreviewSheet({ tab, onTab, payload, diff, isEdit, saving, onSave, onClo const sheetRef = useRef(null) const closeRef = useRef(null) const openerRef = useRef(null) + // The parent passes a fresh onClose on every render. Read it through a ref + // so the effect below runs once per open: re-running it would move focus to + // the opener and back to Close, pulling it out of the control in use. + const onCloseRef = useRef(onClose) + useEffect(() => { onCloseRef.current = onClose }) useEffect(() => { openerRef.current = document.activeElement closeRef.current?.focus() const onKey = (e) => { - if (e.key === 'Escape') { e.preventDefault(); e.stopPropagation(); onClose(); return } + if (e.key === 'Escape') { e.preventDefault(); e.stopPropagation(); onCloseRef.current(); return } if (e.key !== 'Tab' || !sheetRef.current) return const focusable = Array.from(sheetRef.current.querySelectorAll('button:not([disabled]), [tabindex]:not([tabindex="-1"])')) if (focusable.length === 0) return @@ -1148,7 +1153,7 @@ function PreviewSheet({ tab, onTab, payload, diff, isEdit, saving, onSave, onClo const el = openerRef.current if (el && document.contains(el)) el.focus?.() } - }, [onClose]) + }, []) const shown = useCallback((v) => displayValue(maskSecrets(v)), []) const json = useMemo(() => JSON.stringify(maskSecrets(payload), null, 2), [payload]) diff --git a/core/http/react-ui/src/pages/AgentTaskDetails.jsx b/core/http/react-ui/src/pages/AgentTaskDetails.jsx index 01545d815..9053f633c 100644 --- a/core/http/react-ui/src/pages/AgentTaskDetails.jsx +++ b/core/http/react-ui/src/pages/AgentTaskDetails.jsx @@ -723,12 +723,17 @@ function PreviewSheet({ tab, onTab, task, params, diff, isNew, saving, onSave, o const { t } = useTranslation('agents') const sheetRef = useRef(null) const closeRef = useRef(null) + // The parent passes a fresh onClose on every render. Read it through a ref + // so the effect below runs once per open: re-running it would move focus to + // the opener and back to Close, pulling it out of the control in use. + const onCloseRef = useRef(onClose) + useEffect(() => { onCloseRef.current = onClose }) useEffect(() => { const opener = document.activeElement closeRef.current?.focus() const onKey = (e) => { - if (e.key === 'Escape') { e.preventDefault(); e.stopPropagation(); onClose(); return } + if (e.key === 'Escape') { e.preventDefault(); e.stopPropagation(); onCloseRef.current(); return } if (e.key !== 'Tab' || !sheetRef.current) return const focusable = Array.from(sheetRef.current.querySelectorAll('button:not([disabled]), [tabindex]:not([tabindex="-1"])')) if (focusable.length === 0) return @@ -742,7 +747,7 @@ function PreviewSheet({ tab, onTab, task, params, diff, isNew, saving, onSave, o window.removeEventListener('keydown', onKey, true) if (opener && document.contains(opener)) opener.focus?.() } - }, [onClose]) + }, []) return createPortal(