From 4b0b39f6c460130601cedeab6713aa5e1794e096 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 19:22:24 -0700 Subject: [PATCH 1/8] fix(ui): let Tab reach a dialog's actions, and say the dialog is modal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured in Chromium on **New schedule**: twelve Tab presses cycled Name → Browse → "Repeat every" → the dialog container. Cancel, "Create schedule", the × and the three time selects were never focused, and the dialog carried no `aria-modal`. The trap was not broken. `react-select` unmounts its four `aria-live` spans synchronously while blurring, and Radix's `FocusScope` watches the dialog subtree for removals so it can answer "the focused node was removed and the browser dropped focus on " by focusing the container. Between a `focusout` and its `focusin` that is exactly what the DOM looks like, so the observer's microtask parks focus on the dialog and the in-flight Tab is lost — once per mutation record, four times over. Every control after the first `Select` was unreachable, in every dialog that holds one. `dialogTabRepair` remembers where the browser said the Tab was going and hands focus back if the container takes it instead, deferring the restore past the park loop so the last word is the user's. A blur with no `relatedTarget` — a genuine removal — is left to Radix, which is what its park is for. Evidence: the new spec fails 3/5 on the unfixed primitive. In Playwright against the real dialog the cycle goes from four stops to all ten, forwards and with Shift+Tab. --- ui/desktop/src/components/ui/dialog.tsx | 33 +++++ .../components/ui/dialogTabRepair.test.tsx | 114 ++++++++++++++ .../src/components/ui/dialogTabRepair.ts | 139 ++++++++++++++++++ 3 files changed, 286 insertions(+) create mode 100644 ui/desktop/src/components/ui/dialogTabRepair.test.tsx create mode 100644 ui/desktop/src/components/ui/dialogTabRepair.ts diff --git a/ui/desktop/src/components/ui/dialog.tsx b/ui/desktop/src/components/ui/dialog.tsx index c6c3b94c1..a75d4794b 100644 --- a/ui/desktop/src/components/ui/dialog.tsx +++ b/ui/desktop/src/components/ui/dialog.tsx @@ -5,6 +5,7 @@ import * as DialogPrimitive from '@radix-ui/react-dialog'; import { XIcon } from '../icons/app-icons'; import { cn } from '../../utils'; +import { installDialogTabRepair } from './dialogTabRepair'; function Dialog({ ...props }: React.ComponentProps) { return ; @@ -45,15 +46,47 @@ function DialogContent({ showCloseButton = dismissible, onEscapeKeyDown, onPointerDownOutside, + ref, ...props }: React.ComponentProps & { dismissible?: boolean; showCloseButton?: boolean; }) { + // See `dialogTabRepair`: a `Select` unmounting its live region on blur makes + // Radix's focus scope swallow the Tab that was moving off it, which left the + // New schedule dialog's Cancel and "Create schedule" keyboard-unreachable. + // The repair belongs here, on the one primitive every modal composes, because + // the trigger is any `Select` (or anything else that mutates the dialog's DOM + // while blurring) rather than anything the schedule dialog does. + const attachRepair = React.useCallback( + (node: HTMLDivElement | null) => { + const forward = (value: HTMLDivElement | null) => { + if (typeof ref === 'function') ref(value); + else if (ref) (ref as React.RefObject).current = value; + }; + forward(node); + if (!node) return; + const teardown = installDialogTabRepair(node); + // React 19 calls a ref callback's cleanup INSTEAD of re-invoking it with + // `null`, so the forwarded ref has to be cleared here or a caller holding + // one would keep a detached node. + return () => { + teardown(); + forward(null); + }; + }, + [ref] + ); + return ( ` and a `MutationObserver` microtask + * gets to run) never opens. It was measured with Playwright against the real + * component; what is pinned here is the *pattern* that window leaves behind — a + * `focusout` that named its destination, immediately followed by the dialog + * container taking focus instead — and the two cases the repair must not touch. + */ +function renderDialog() { + render( + + + Schedule + + + + + ); + return { + container: screen.getByRole('dialog'), + first: screen.getByRole('button', { name: 'First' }), + second: screen.getByRole('button', { name: 'Second' }), + }; +} + +/** The restore is deferred past the park loop, so a check has to be too. */ +const settle = () => Promise.resolve(); + +/** + * The three steps the browser really performs, in order: the control blurs, the + * browser reports where the Tab was headed, and — in the window where nothing is + * focused — Radix parks focus on the dialog. `blur()` first is what makes the + * park realistic: it is only because focus is on `` that `container.focus()` + * raises no `focusout` of its own. + */ +function parkFocusOnDialog( + container: HTMLElement, + from: HTMLElement, + headedFor: HTMLElement | null +) { + from.blur(); + fireEvent.focusOut(from, { relatedTarget: headedFor }); + container.focus(); +} + +describe('dialog Tab repair', () => { + it('hands focus back to the control the Tab was going to', async () => { + const { container, first, second } = renderDialog(); + first.focus(); + + parkFocusOnDialog(container, first, second); + await settle(); + + expect(document.activeElement).toBe(second); + }); + + it('answers a park that repeats, which is how the real one arrives', async () => { + const { container, first, second } = renderDialog(); + first.focus(); + + // Radix parks once per mutation record, and react-select's blur produces + // four. A repair that restored inside the first park's `focusin` would be + // overwritten by the rest and the user would still land on the dialog. + parkFocusOnDialog(container, first, second); + container.focus(); + container.focus(); + await settle(); + + expect(document.activeElement).toBe(second); + }); + + it('leaves the park alone when the focused control really was removed', async () => { + const { container, first } = renderDialog(); + first.focus(); + + // A genuine removal drops focus to the document, so the browser has no + // destination to report. Radix's park is the right answer here. + parkFocusOnDialog(container, first, null); + await settle(); + + expect(document.activeElement).toBe(container); + }); + + it('does not chase a destination from an earlier task', async () => { + const { container, first, second } = renderDialog(); + first.focus(); + first.blur(); + fireEvent.focusOut(first, { relatedTarget: second }); + + // The Tab finished long ago; the dialog is focused later for its own + // reasons — a click on its chrome, say. + await new Promise((resolve) => setTimeout(resolve, 0)); + container.focus(); + await settle(); + + expect(document.activeElement).toBe(container); + }); + + it('tells a screen reader the dialog is modal', () => { + const { container } = renderDialog(); + expect(container).toHaveAttribute('aria-modal', 'true'); + }); +}); diff --git a/ui/desktop/src/components/ui/dialogTabRepair.ts b/ui/desktop/src/components/ui/dialogTabRepair.ts new file mode 100644 index 000000000..c6a9d8fbb --- /dev/null +++ b/ui/desktop/src/components/ui/dialogTabRepair.ts @@ -0,0 +1,139 @@ +/** + * Put back a Tab that the dialog's own focus trap cancelled. + * + * ## The bug this exists for + * + * Measured in Chromium on the **New schedule** dialog: twelve Tab presses + * visited only Name → Browse → "Repeat every" → the dialog container, over and + * over. Cancel, "Create schedule", the × and the three time selects were never + * focused. The trap itself was fine — the actions were simply never reached. + * + * The mechanism is an interaction between two libraries, and neither is wrong on + * its own: + * + * 1. `react-select` renders its `aria-live` announcements as children that exist + * only while the control is focused (`isFocused && `). + * Blur unmounts those four ``s **synchronously**, inside the `focusout` + * dispatch, because React flushes discrete events without batching. + * 2. Radix's `FocusScope` watches the dialog subtree with a `MutationObserver` + * to catch "the focused element was removed, so the browser dropped focus on + * ``" — and answers it by focusing the dialog container. + * + * Between a `focusout` and the matching `focusin`, `document.activeElement` is + * ``, and the JS stack empties — so the observer's microtask runs *inside* + * that window, sees removals with focus apparently on ``, and parks focus + * on the container. The Tab the browser was in the middle of delivering is lost, + * and every control after the first `Select` in the dialog becomes unreachable. + * + * This is not specific to the schedule dialog: any dialog holding a `Select` + * (the model picker, the provider modals, lead/worker settings) has it, which is + * why the repair lives on the dialog primitive rather than in `CronPicker`. + * + * ## The repair + * + * The browser told us where the Tab was going — `focusout.relatedTarget`. If the + * dialog container then takes focus itself, that is the park above, and focus is + * handed back to the element the browser had chosen. + * + * Three details are load-bearing, and each was measured rather than reasoned: + * + * - **The restore is deferred to a microtask.** Radix's observer parks focus + * *once per mutation record* — react-select's blur produces four — so a + * restore performed inside the first park's `focusin` is simply overwritten by + * the next three. Deferring puts it after the whole loop, where it sticks; a + * later observer batch then sees a focused control and stands down by itself. + * - **Landing on the intended control does not end the episode**, and neither + * does blurring *towards the container*. Both happen while the park and the + * browser's own transfer interleave, and treating either as "focus moved, we + * are done" makes the repair a no-op for exactly the sequence it exists for. + * - **A genuine removal is left alone.** When the focused element really is + * removed, the blur carries no `relatedTarget` — focus fell to the document — + * so nothing is remembered and Radix's park stands, which is what it is for. + * + * The intent is dropped at the end of the task, so a container focus arriving + * later — a click on dialog chrome, say — can never be answered with a stale + * target. + */ + +/** How many times one Tab may be put back before the repair gives up. */ +const MAX_RESTORES_PER_EPISODE = 3; + +/** + * Watch `container` for the cancelled-Tab pattern and undo it. + * + * Returns the teardown. Safe to call with any element; it touches nothing until + * focus moves. + */ +export function installDialogTabRepair(container: HTMLElement): () => void { + let intended: HTMLElement | null = null; + let forgetTimer: ReturnType | null = null; + let restorePending = false; + let restores = 0; + + const forget = () => { + intended = null; + restores = 0; + if (forgetTimer !== null) { + clearTimeout(forgetTimer); + forgetTimer = null; + } + }; + + const scheduleRestore = () => { + if (!intended || restorePending) return; + restorePending = true; + queueMicrotask(() => { + restorePending = false; + const target = intended; + if (!target) return; + // Something other than the park won the race; whatever it was, it is a + // better answer than a focus we inferred. + if (document.activeElement !== container) return; + if (!target.isConnected || !container.contains(target)) { + forget(); + return; + } + // A cap, not a rhythm: an endless volley is a bug, not something a user + // should have to sit through. + if (restores >= MAX_RESTORES_PER_EPISODE) { + forget(); + return; + } + restores += 1; + target.focus(); + }); + }; + + const handleFocusOut = (event: FocusEvent) => { + const next = event.relatedTarget; + // The park taking focus back mid-episode. Keep the destination: answering it + // is the whole job. + if (next === container && intended) return; + if (!(next instanceof HTMLElement) || next === container || !container.contains(next)) { + forget(); + return; + } + intended = next; + if (forgetTimer === null) forgetTimer = setTimeout(forget, 0); + }; + + const handleFocusIn = (event: FocusEvent) => { + if (event.target !== container) { + if (event.target !== intended) forget(); + return; + } + // The dialog itself has focus. Nothing in the app focuses the container + // deliberately except Radix — on open, when there is no destination + // remembered, and on the park this undoes. + scheduleRestore(); + }; + + container.addEventListener('focusout', handleFocusOut); + container.addEventListener('focusin', handleFocusIn); + + return () => { + forget(); + container.removeEventListener('focusout', handleFocusOut); + container.removeEventListener('focusin', handleFocusIn); + }; +} From e544ccd06e4b7acb9f57c05e163e0c88746602e9 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 19:24:14 -0700 Subject: [PATCH 2/8] fix(settings): give the four Appearance switches an accessible name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured with the accessibility tree: Menu bar icon, Dock icon, Prevent sleep and Cost tracking were `role=switch` with `aria-label` null, `aria-labelledby` null and no text content. A screen reader announced "switch, on" — four times in one panel, each about something different. The subject was already on screen, in the `

` beside each switch; it was simply never connected to the control. The Privacy tiers switch two panels over does connect it, and this copies that: `aria-label` quoting the visible label verbatim, so someone driving the app by voice can say what they read. Two labels move to sentence case in the same breath ("Prevent Sleep", "Cost Tracking"), which is the app's copy rule and what keeps name and label the same string. Nothing else in the repo referenced either. Evidence: the new spec fails all 5 before (no switch can be found by name). --- .../settings/app/AppSettingsSection.test.tsx | 61 +++++++++++++++++++ .../settings/app/AppSettingsSection.tsx | 8 ++- 2 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 ui/desktop/src/components/settings/app/AppSettingsSection.test.tsx diff --git a/ui/desktop/src/components/settings/app/AppSettingsSection.test.tsx b/ui/desktop/src/components/settings/app/AppSettingsSection.test.tsx new file mode 100644 index 000000000..e69c1b158 --- /dev/null +++ b/ui/desktop/src/components/settings/app/AppSettingsSection.test.tsx @@ -0,0 +1,61 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; +import { render, screen } from '@testing-library/react'; + +// The Appearance rows are what this file is about; the rest of the App tab is +// other sections' business and pulls in the API client, the theme registry and +// the usage panel. +vi.mock('./UpdateSection', () => ({ default: () => null })); +vi.mock('../usage/UsageSection', () => ({ default: () => null })); +vi.mock('./ResetPanel', () => ({ default: () => null })); +vi.mock('../../BioRouterSidebar/ThemeSelector', () => ({ default: () => null })); +vi.mock('../../BioRouterSidebar/ThemeFamilySelector', () => ({ default: () => null })); + +import AppSettingsSection from './AppSettingsSection'; + +beforeEach(() => { + Object.assign(window, { + electron: { + platform: 'darwin', + getMenuBarIconState: vi.fn().mockResolvedValue(true), + getDockIconState: vi.fn().mockResolvedValue(true), + getWakelockState: vi.fn().mockResolvedValue(true), + setMenuBarIcon: vi.fn().mockResolvedValue(true), + setDockIcon: vi.fn().mockResolvedValue(true), + setWakelock: vi.fn().mockResolvedValue(true), + openNotificationsSettings: vi.fn(), + }, + appConfig: { get: vi.fn().mockReturnValue(undefined) }, + }); +}); + +/** + * Measured with the accessibility tree: all four Appearance switches came back + * with `aria-label` null, `aria-labelledby` null and no text content, so a + * screen reader announced "switch, on" with no subject — four times in one + * panel, each about something different. + * + * The subject is on screen; it is the `

` beside the switch. It just was not + * connected to the control, which is exactly what the Privacy tiers switch two + * panels over does connect (`aria-label="Privacy tiers"`), and this follows it. + * + * The name is the visible label verbatim, not a paraphrase: someone driving the + * app by voice says what they can read. + */ +describe('Appearance switches', () => { + it.each([['Menu bar icon'], ['Dock icon'], ['Prevent sleep'], ['Cost tracking']])( + 'announces what "%s" is about', + async (name) => { + render(); + expect(await screen.findByRole('switch', { name })).toHaveAccessibleName(name); + } + ); + + it('names every switch in the panel, so none is left to be found by position', async () => { + render(); + await screen.findByRole('switch', { name: 'Menu bar icon' }); + const unnamed = screen + .getAllByRole('switch') + .filter((control) => !control.getAttribute('aria-label')?.trim()); + expect(unnamed).toEqual([]); + }); +}); diff --git a/ui/desktop/src/components/settings/app/AppSettingsSection.tsx b/ui/desktop/src/components/settings/app/AppSettingsSection.tsx index 7981f0e42..aacc9f7d5 100644 --- a/ui/desktop/src/components/settings/app/AppSettingsSection.tsx +++ b/ui/desktop/src/components/settings/app/AppSettingsSection.tsx @@ -179,6 +179,7 @@ export default function AppSettingsSection({ scrollToSection }: AppSettingsSecti checked={menuBarIconEnabled} onCheckedChange={handleMenuBarIconToggle} variant="mono" + aria-label="Menu bar icon" /> @@ -195,13 +196,14 @@ export default function AppSettingsSection({ scrollToSection }: AppSettingsSecti checked={dockIconEnabled} onCheckedChange={handleDockIconToggle} variant="mono" + aria-label="Dock icon" /> )}

-

Prevent Sleep

+

Prevent sleep

Keep your computer awake while Biorouter is running a task (screen can still lock)

@@ -210,13 +212,14 @@ export default function AppSettingsSection({ scrollToSection }: AppSettingsSecti checked={wakelockEnabled} onCheckedChange={handleWakelockToggle} variant="mono" + aria-label="Prevent sleep" />
{COST_TRACKING_ENABLED && (
-

Cost Tracking

+

Cost tracking

Show model pricing and usage costs

@@ -225,6 +228,7 @@ export default function AppSettingsSection({ scrollToSection }: AppSettingsSecti checked={showPricing} onCheckedChange={handleShowPricingToggle} variant="mono" + aria-label="Cost tracking" />
)} From a0f46a98ec59ecb8ed0539053f14837b12306e7f Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 19:25:18 -0700 Subject: [PATCH 3/8] fix(ui): dismiss a tooltip when its target leaves the page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hover a row control, then change route without moving the pointer: the target unmounts, so no `pointerout` is ever delivered and the tooltip is stranded on screen describing an element that no longer exists. The guard for this was `useEffect(() => { if (tooltip && !tooltip.target.isConnected) setTooltip(null) }, [tooltip])` — a check that runs when the tooltip STATE changes, which is never what happens here. Adding `tooltip.target` to the deps would not help either: a node reference does not change when the node is removed. A removal is a DOM event, so it takes a DOM observer, and this watches for one while — and only while — a tooltip is open. Evidence: the new case fails before (the tooltip is still in the document after the target unmounts), passes after; the five existing cases are unchanged. --- .../components/ui/AppTooltipLayer.test.tsx | 28 +++++++++++++++++++ .../src/components/ui/AppTooltipLayer.tsx | 27 ++++++++++++++++-- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/ui/desktop/src/components/ui/AppTooltipLayer.test.tsx b/ui/desktop/src/components/ui/AppTooltipLayer.test.tsx index c1b9ce02e..445223402 100644 --- a/ui/desktop/src/components/ui/AppTooltipLayer.test.tsx +++ b/ui/desktop/src/components/ui/AppTooltipLayer.test.tsx @@ -108,6 +108,34 @@ describe('AppTooltipLayer', () => { expect(tooltip).toHaveClass('w-max', 'max-w-[min(20rem,calc(100vw-16px))]', 'break-words'); }); + /** + * The stranded tooltip. Hover a row control, then change route without moving + * the pointer: the target unmounts, so no `pointerout` is ever delivered and + * the tooltip is left on screen describing an element that no longer exists. + * + * The check for this was `useEffect(…, [tooltip])` — it ran when the tooltip + * STATE changed, which is never the case here. + */ + it('dismisses a tooltip whose target leaves the page', async () => { + const { rerender } = render( + <> + + + + ); + + const target = screen.getByTestId('native-title-target'); + await waitFor(() => expect(target).toHaveAttribute('data-biorouter-tooltip', 'Native action')); + fireEvent.pointerOver(target); + expect(await screen.findByRole('tooltip')).toHaveTextContent('Native action'); + + // The route change. The pointer never moves, so the only signal is the + // removal itself. + rerender(); + + await waitFor(() => expect(screen.queryByRole('tooltip')).toBeNull()); + }); + it('does not open after the pointer leaves during the delay', async () => { render( <> diff --git a/ui/desktop/src/components/ui/AppTooltipLayer.tsx b/ui/desktop/src/components/ui/AppTooltipLayer.tsx index 6626b2191..e1118d04e 100644 --- a/ui/desktop/src/components/ui/AppTooltipLayer.tsx +++ b/ui/desktop/src/components/ui/AppTooltipLayer.tsx @@ -196,9 +196,32 @@ export function AppTooltipLayer() { } }, [horizontalShift, tooltip]); + // A tooltip outlives its target when the target LEAVES rather than when the + // pointer does: hover a row control, change route without moving the pointer, + // and no `pointerout` is ever delivered — the element the pointer was over is + // simply gone, and the tooltip stays on screen describing nothing. + // + // ⚠ This used to be `useEffect(… , [tooltip])`, which is a check that runs + // only when the tooltip STATE changes — never for the one case it was written + // for. Adding `tooltip.target` to the deps does not fix it either: a node + // reference does not change when the node is removed. The removal is a DOM + // event, so it takes a DOM observer. + const tooltipTarget = tooltip?.target ?? null; useEffect(() => { - if (tooltip && !tooltip.target.isConnected) setTooltip(null); - }, [tooltip]); + if (!tooltipTarget) return; + if (!tooltipTarget.isConnected) { + setTooltip(null); + return; + } + const dismissWhenTargetLeaves = () => { + if (!tooltipTarget.isConnected) setTooltip(null); + }; + // Only while a tooltip is open, and only an `isConnected` read per batch — + // the observer is torn down the moment the tooltip closes. + const observer = new MutationObserver(dismissWhenTargetLeaves); + observer.observe(document.body, { childList: true, subtree: true }); + return () => observer.disconnect(); + }, [tooltipTarget]); if (!tooltip) return null; From da27d72e5074c10db366cf3188b2dff94124f435 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 19:28:32 -0700 Subject: [PATCH 4/8] fix(chat): tell the user when copying a message fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both clipboard writes failing ended at two `console.error`s and nothing else: `markCopied()` was never reached, so the button went on saying "Copy" and the only way to learn that nothing had been copied was to paste somewhere and find out. The button now reports the outcome it actually had — "Copy failed", in the warning ink with a warning glyph — and a single error toast says what to do instead, because the pointer is rarely where the eye is. Both signals are the same 2s transient the success path uses, so the control settles back to "Copy" on its own. One transient value with two states rather than two booleans, so the button can never claim both. The success and rich-copy-then-fallback paths are untouched, and both are now pinned. Evidence: the refusal case fails before (the button keeps saying "Copy", no toast); the two success cases pass before and after. --- .../src/components/MessageCopyLink.test.tsx | 83 +++++++++++++++++++ ui/desktop/src/components/MessageCopyLink.tsx | 53 +++++++++--- 2 files changed, 123 insertions(+), 13 deletions(-) create mode 100644 ui/desktop/src/components/MessageCopyLink.test.tsx diff --git a/ui/desktop/src/components/MessageCopyLink.test.tsx b/ui/desktop/src/components/MessageCopyLink.test.tsx new file mode 100644 index 000000000..9df90a2d3 --- /dev/null +++ b/ui/desktop/src/components/MessageCopyLink.test.tsx @@ -0,0 +1,83 @@ +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; + +const mocks = vi.hoisted(() => ({ toastError: vi.fn() })); +vi.mock('../toasts', () => ({ toastError: mocks.toastError })); + +import MessageCopyLink from './MessageCopyLink'; + +/** No rich node, so the component takes the plain-text path. */ +const noContent = { current: null }; + +/** + * ⚠ **Call this AFTER `userEvent.setup()`.** `setup()` installs a clipboard stub + * of its own, so a stub written first is replaced by one whose `writeText` + * resolves — and every failure test quietly measures a success. + */ +function stubClipboard(writeText: () => Promise) { + // `Object.assign` works once and then trips over the prototype getter, which + // makes the second test in the file fail for a reason that has nothing to do + // with what it is testing. + Object.defineProperty(navigator, 'clipboard', { + value: { writeText: vi.fn(writeText), write: vi.fn() }, + configurable: true, + writable: true, + }); +} + +beforeEach(() => { + mocks.toastError.mockClear(); + // The app's own logging stays quiet; these paths log on purpose. + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); + +afterEach(() => vi.restoreAllMocks()); + +/** + * The reported defect: when both clipboard writes fail, the outer catch logged + * `'Failed to copy text: '`, the inner catch logged `'Failed to copy text + * (fallback): '`, and that was the end of it. `markCopied()` was never reached, + * so the button went on saying "Copy" — the user's only way to learn nothing had + * been copied was to paste somewhere and find out. + */ +describe('MessageCopyLink', () => { + it('says so when the clipboard refuses', async () => { + const user = userEvent.setup(); + stubClipboard(() => Promise.reject(new Error('denied'))); + render(); + + await user.click(screen.getByRole('button', { name: 'Copy message' })); + + await waitFor(() => expect(screen.getByRole('button')).toHaveTextContent('Copy failed')); + expect(mocks.toastError).toHaveBeenCalledTimes(1); + expect(mocks.toastError.mock.calls[0][0]).toMatchObject({ title: 'Copy failed' }); + }); + + it('leaves the success path alone', async () => { + const user = userEvent.setup(); + stubClipboard(() => Promise.resolve()); + render(); + + await user.click(screen.getByRole('button', { name: 'Copy message' })); + + await waitFor(() => expect(screen.getByRole('button')).toHaveTextContent('Copied!')); + expect(mocks.toastError).not.toHaveBeenCalled(); + }); + + it('still counts a successful fallback as a copy', async () => { + // The rich write is what fails here — jsdom has no `ClipboardItem`, which is + // the same shape of failure as a browser refusing the `text/html` flavour — + // and the plain-text retry succeeds. + const user = userEvent.setup(); + stubClipboard(() => Promise.resolve()); + const contentRef = { current: document.createElement('div') }; + contentRef.current.textContent = 'hello'; + render(); + + await user.click(screen.getByRole('button', { name: 'Copy message' })); + + await waitFor(() => expect(screen.getByRole('button')).toHaveTextContent('Copied!')); + expect(mocks.toastError).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/desktop/src/components/MessageCopyLink.tsx b/ui/desktop/src/components/MessageCopyLink.tsx index 8d36eb8e1..1bce6f436 100644 --- a/ui/desktop/src/components/MessageCopyLink.tsx +++ b/ui/desktop/src/components/MessageCopyLink.tsx @@ -1,15 +1,21 @@ import React from 'react'; -import { Copy } from './icons/app-icons'; +import { AlertTriangle, Copy } from './icons/app-icons'; import { MessageMetaAction } from './MessageMeta'; -import { useTransientFlag } from '../hooks/useTransientFlag'; +import { useTransientValue } from '../hooks/useTransientFlag'; +import { toastError } from '../toasts'; interface MessageCopyLinkProps { text: string; contentRef: React.RefObject; } +/** What the clipboard write is asked to put on the clipboard. */ +type CopyOutcome = 'copied' | 'failed'; + export default function MessageCopyLink({ text, contentRef }: MessageCopyLinkProps) { - const [copied, markCopied] = useTransientFlag(2000); + // One transient state with two values rather than two booleans that could + // both be true: the button says exactly one thing at a time. + const [outcome, markOutcome] = useTransientValue(2000); const handleCopy = async () => { try { @@ -32,22 +38,43 @@ export default function MessageCopyLink({ text, contentRef }: MessageCopyLinkPro await navigator.clipboard.writeText(text); } - markCopied(); + markOutcome('copied'); + return; } catch (err) { console.error('Failed to copy text: ', err); - // Fallback to plain text if HTML copy fails - try { - await navigator.clipboard.writeText(text); - markCopied(); - } catch (fallbackErr) { - console.error('Failed to copy text (fallback): ', fallbackErr); - } } + + // Fallback to plain text if the rich copy failed. + try { + await navigator.clipboard.writeText(text); + markOutcome('copied'); + return; + } catch (fallbackErr) { + console.error('Failed to copy text (fallback): ', fallbackErr); + } + + // ⚠ Both writes failed, and this is the branch that used to end at a + // `console.error` the user cannot see: `markCopied()` was never reached, so + // the button went on saying "Copy" and the only way to find out nothing had + // been copied was to paste. Say so, in both places the user might be + // looking — on the control they pressed, and once in the corner. + markOutcome('failed'); + toastError({ + title: 'Copy failed', + msg: 'Biorouter could not write to the clipboard. Select the message and copy it with your keyboard.', + }); }; + const failed = outcome === 'failed'; + return ( - } aria-label="Copy message"> - {copied ? 'Copied!' : 'Copy'} + : } + aria-label="Copy message" + className={failed ? 'text-text-warning hover:text-text-warning' : undefined} + > + {outcome === 'copied' ? 'Copied!' : failed ? 'Copy failed' : 'Copy'} ); } From c05d30bf6fa3aed572dcfe748555dd2e0fef9ab5 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 19:35:35 -0700 Subject: [PATCH 5/8] fix(ui): make the checkbox square itself clickable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The primitive hid its input with `peer sr-only` — a 1px clipped box in the corner — so the 22px square everyone can see was a picture. It toggled only when a call site remembered to wrap it in a `