From 4757cc7b81d8c614daed3b8e1341d6eabc1d300b Mon Sep 17 00:00:00 2001 From: joelrobotham Date: Thu, 22 Oct 2026 03:19:48 +0000 Subject: [PATCH 1/7] fix(threads): read an already-open thread when Activity opens it Opening a thread from sidebar Activity that is already selected changes no tab selection, so nothing handed focus into it and the reading observer never started; the unread dot stayed. A new navigation request for the shown thread now focuses its scroller unless focus is already inside or the opener asked to preserve it. Signed-off-by: joelrobotham --- src/features/messages/ThreadPanel.tsx | 29 ++++++++++++++++ tests/browser/thread-unread.spec.mjs | 50 +++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/src/features/messages/ThreadPanel.tsx b/src/features/messages/ThreadPanel.tsx index 24140ab21a..82fea66850 100644 --- a/src/features/messages/ThreadPanel.tsx +++ b/src/features/messages/ThreadPanel.tsx @@ -751,6 +751,35 @@ function ThreadMessages({ const rootTarget = navigation?.target.kind === "conversation" && navigation.target.threadRootId === messageId; + // Opening a thread that is already shown selects nothing new, so no tab + // hands focus to it. Without focus inside, reading never starts. + // The signal identifies one request; the navigation object is re-bound per + // relay snapshot, so a re-render of the same request must not refocus. + const handledRequest = useRef(navigation?.signal); + // biome-ignore lint/correctness/useExhaustiveDependencies: only a new request moves focus; the others merely qualify it. + useLayoutEffect(() => { + if (handledRequest.current === navigation?.signal) return; + handledRequest.current = navigation?.signal; + const element = scroller.current; + if ( + !navigation || + navigation.signal.aborted || + navigation.focus === "preserve" || + !rootTarget || + !active || + !element + ) + return; + const surface = element.closest("[data-reading-surface]"); + // Focus on this thread's own selected tab already counts as reading it. + const label = surface + ?.closest('[role="tabpanel"]') + ?.getAttribute("aria-labelledby"); + const tab = label ? document.getElementById(label) : null; + const focused = document.activeElement; + if (!surface?.contains(focused) && !tab?.contains(focused)) + element.focus({ preventScroll: true }); + }, [navigation?.signal]); // Ordinary opens follow the latest reply after bounded history finishes. // Keep that first positioning invisible; exact-message navigation reveals itself. const positioning = diff --git a/tests/browser/thread-unread.spec.mjs b/tests/browser/thread-unread.spec.mjs index c988a82a36..236b01ccd0 100644 --- a/tests/browser/thread-unread.spec.mjs +++ b/tests/browser/thread-unread.spec.mjs @@ -47,6 +47,56 @@ test.describe("mentioned reply priority", () => { new Set(["Broadcast descendant", "Unread reply 1"]), ); }); + + test("opening an already-open thread from Activity reads it", async ({ + page, + app, + }) => { + // Reading needs a 300ms dwell (use-reading.ts); hold the clock so the + // thread cannot earn it while it opens. + await page.clock.install(); + await holdReadingFocus(page); + await open(page, app); + const root = app.histories + .get("primary/alpha") + .find((row) => row.content === "Thread root 1"); + const alpha = page.locator('button[data-channel-id="alpha"]'); + const panel = page.getByRole("complementary", { + name: "Thread", + exact: true, + }); + const unread = () => + page.evaluate( + (rootId) => + window.fixtureRelay.snapshot().session.unread.snapshot({ + kind: "thread", + channelId: "alpha", + rootId, + }), + root.id, + ); + await page.clock.pauseAt((await page.evaluate(() => Date.now())) + 1000); + await page + .locator(`[data-channel-timeline] [data-message-id="${root.id}"]`) + .getByRole("button", { name: /^View thread:/ }) + .click(); + await expect(panel).toBeVisible(); + // The reader is looking elsewhere: the open thread stays unread. + await page.evaluate(() => document.activeElement?.blur()); + await page.clock.resume(); + expect((await unread()).observedCount).toBeGreaterThan(0); + await alpha.hover(); + const item = page + .getByRole("dialog", { name: "Activity in Alpha" }) + .getByRole("button", { name: /Open unread thread from/ }) + .filter({ hasText: "Unread reply 1" }); + await expect(item).toBeVisible(); + await releaseReadingFocus(page); + // Activity opens the thread that is already selected. + await item.click(); + await expect(panel).toBeVisible(); + await expect.poll(async () => (await unread()).observedCount).toBe(0); + }); }); test("thread buttons show observed unread independently, clear only after reading, and expose hover/focus affordance", async ({ From 1a338ce522109c70e1e4773ee4edd404709c92e9 Mon Sep 17 00:00:00 2001 From: joelrobotham Date: Thu, 22 Oct 2026 06:21:24 +0000 Subject: [PATCH 2/7] fix(threads): leave focus alone when it is on a tab Opening the already-shown thread moved focus into its scroller even when the person was steering the tab strip (keyboard tab moves), which broke "consecutive native Close requests close each main-pane thread". Only focus left behind by the opener moves in now. Signed-off-by: joelrobotham --- src/features/messages/ThreadPanel.tsx | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/features/messages/ThreadPanel.tsx b/src/features/messages/ThreadPanel.tsx index 82fea66850..927a0ff3c8 100644 --- a/src/features/messages/ThreadPanel.tsx +++ b/src/features/messages/ThreadPanel.tsx @@ -770,14 +770,13 @@ function ThreadMessages({ !element ) return; - const surface = element.closest("[data-reading-surface]"); - // Focus on this thread's own selected tab already counts as reading it. - const label = surface - ?.closest('[role="tabpanel"]') - ?.getAttribute("aria-labelledby"); - const tab = label ? document.getElementById(label) : null; + // Focus already in this surface, or on any tab (the person is steering the + // tab strip), is not stolen; only focus left behind by the opener moves in. const focused = document.activeElement; - if (!surface?.contains(focused) && !tab?.contains(focused)) + if ( + !element.closest("[data-reading-surface]")?.contains(focused) && + !focused?.closest('[role="tab"]') + ) element.focus({ preventScroll: true }); }, [navigation?.signal]); // Ordinary opens follow the latest reply after bounded history finishes. From 0ae40bbe75a21fbae492ad194e7fdbbba4e72b87 Mon Sep 17 00:00:00 2001 From: joelrobotham Date: Thu, 22 Oct 2026 07:14:10 +0000 Subject: [PATCH 3/7] fix(threads): leave a thread's first opening to its tab The focus handoff for an already-open thread also ran when a thread opened for the first time, taking focus from the tab that is meant to receive it and breaking the focus order Back restores. Wait until the panel has been positioned, so only a thread that is already open is refocused. Signed-off-by: joelrobotham --- src/features/messages/ThreadPanel.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/features/messages/ThreadPanel.tsx b/src/features/messages/ThreadPanel.tsx index 927a0ff3c8..07f193faeb 100644 --- a/src/features/messages/ThreadPanel.tsx +++ b/src/features/messages/ThreadPanel.tsx @@ -752,7 +752,8 @@ function ThreadMessages({ navigation?.target.kind === "conversation" && navigation.target.threadRootId === messageId; // Opening a thread that is already shown selects nothing new, so no tab - // hands focus to it. Without focus inside, reading never starts. + // hands focus to it. Without focus inside, reading never starts. A first + // opening (not yet positioned) is focused by its tab and must be left alone. // The signal identifies one request; the navigation object is re-bound per // relay snapshot, so a re-render of the same request must not refocus. const handledRequest = useRef(navigation?.signal); @@ -767,6 +768,7 @@ function ThreadMessages({ navigation.focus === "preserve" || !rootTarget || !active || + !positioned.current || !element ) return; From 1bec78512fac3f9f0573eccfdf328a05b043b793 Mon Sep 17 00:00:00 2001 From: joelrobotham Date: Thu, 22 Oct 2026 09:00:36 +0000 Subject: [PATCH 4/7] test(threads): make the already-open Activity test say why it failed The thread is already on screen when Activity opens it, so waiting for it to be visible proved nothing. Wait for the click's own navigation to open, then report where focus was if the thread is still unread. Signed-off-by: joelrobotham --- tests/browser/thread-unread.spec.mjs | 32 ++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/tests/browser/thread-unread.spec.mjs b/tests/browser/thread-unread.spec.mjs index 236b01ccd0..3adfe03f10 100644 --- a/tests/browser/thread-unread.spec.mjs +++ b/tests/browser/thread-unread.spec.mjs @@ -92,10 +92,38 @@ test.describe("mentioned reply priority", () => { .filter({ hasText: "Unread reply 1" }); await expect(item).toBeVisible(); await releaseReadingFocus(page); + const previousAttempt = await page.evaluate( + () => window.fixtureNavigation.snapshot().attempt.id, + ); // Activity opens the thread that is already selected. await item.click(); - await expect(panel).toBeVisible(); - await expect.poll(async () => (await unread()).observedCount).toBe(0); + // The thread is already on screen, so its visibility proves nothing. Wait + // for the click's own navigation, so a failure below means the thread was + // opened and still not read, never that the click missed. + await expect + .poll(() => + page.evaluate((previous) => { + const { status, attempt } = window.fixtureNavigation.snapshot(); + return { status, freshAttempt: attempt.id !== previous }; + }, previousAttempt), + ) + .toEqual({ status: "opened", freshAttempt: true }); + try { + await expect.poll(async () => (await unread()).observedCount).toBe(0); + } catch (error) { + // Reading needs focus inside the thread; say where it actually was. + const focus = await page.evaluate(() => ({ + active: + document.activeElement?.getAttribute("aria-label") ?? + document.activeElement?.tagName, + inThread: !!document.activeElement?.closest("[data-reading-surface]"), + windowFocused: document.hasFocus(), + })); + throw new Error( + `The thread opened but stayed unread. Focus: ${JSON.stringify(focus)}`, + { cause: error }, + ); + } }); }); From 58095305d254123cad8de1eee88fad31cdf5f589 Mon Sep 17 00:00:00 2001 From: Joel Robotham Date: Fri, 23 Oct 2026 09:02:14 +1100 Subject: [PATCH 5/7] fix(threads): keep focus in a thread opened from Activity Two races let the already-open-thread focus handoff misbehave under load. The Activity popover returned focus to its sidebar trigger in a Base UI microtask after closing. When the popover unmounted in the same commit as the navigation, that microtask ran after the thread took focus and stole it, so the thread stayed unread. A thread row now hands focus off instead, as ProfileButton does for Settings. On a first opening the panel can position before its routed request arrives. The handoff then mistook that request for a reopening and took focus from a control the person had focused since. It now moves focus only when nothing holds it, which is what the opener leaves behind. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Joel Robotham --- src/bundled/channels/ChannelActivityPopover.tsx | 11 ++++++++++- src/features/messages/ThreadPanel.tsx | 10 ++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/bundled/channels/ChannelActivityPopover.tsx b/src/bundled/channels/ChannelActivityPopover.tsx index 687c68844a..c8272fae0a 100644 --- a/src/bundled/channels/ChannelActivityPopover.tsx +++ b/src/bundled/channels/ChannelActivityPopover.tsx @@ -201,6 +201,10 @@ export function ChannelActivityPopover({ NonNullable["actionsRef"]>["current"] >(null); const pressedOpen = useRef(false); + // An opened thread takes focus itself. Base UI returns focus to the trigger + // in a microtask after closing, which can land after, and steal it from, the + // thread. + const handoff = useRef(false); // Hover opens a transient preview. Selecting its row may still take focus. const [preview, setPreview] = useState(false); const [now, setNow] = useState(() => Date.now()); @@ -275,7 +279,10 @@ export function ChannelActivityPopover({ pressedOpen.current = next && details.reason === "trigger-press"; setPreview(next && details.reason === "trigger-hover"); setOpen(next); - if (next) setNow(Date.now()); + if (next) { + handoff.current = false; + setNow(Date.now()); + } if (next && items.length) void session.profiles .ensure( @@ -310,6 +317,7 @@ export function ChannelActivityPopover({ size="wide" padding="list" aria-label={`Activity in ${channelName}`} + finalFocus={() => !handoff.current} > {open && (
@@ -366,6 +374,7 @@ export function ChannelActivityPopover({ session={session} agents={activeAgents} onOpen={(selected) => { + handoff.current = true; close(); onOpenThread(selected); }} diff --git a/src/features/messages/ThreadPanel.tsx b/src/features/messages/ThreadPanel.tsx index 07f193faeb..de2c77a19e 100644 --- a/src/features/messages/ThreadPanel.tsx +++ b/src/features/messages/ThreadPanel.tsx @@ -772,13 +772,11 @@ function ThreadMessages({ !element ) return; - // Focus already in this surface, or on any tab (the person is steering the - // tab strip), is not stolen; only focus left behind by the opener moves in. + // Only focus the opener left behind (its control unmounted) moves in. + // Focus the person has since placed anywhere, even a moment after a first + // opening whose request arrived late, is not stolen. const focused = document.activeElement; - if ( - !element.closest("[data-reading-surface]")?.contains(focused) && - !focused?.closest('[role="tab"]') - ) + if (!focused || focused === document.body) element.focus({ preventScroll: true }); }, [navigation?.signal]); // Ordinary opens follow the latest reply after bounded history finishes. From 89bf94c5ee98e27c50b17ba4f5d98f27a5743132 Mon Sep 17 00:00:00 2001 From: Joel Robotham Date: Fri, 23 Oct 2026 09:13:08 +1100 Subject: [PATCH 6/7] test(threads): keep focus moved during an Activity reopening Review on #1524 found that focus the person moved while an already-open thread retargets could be pulled back into the thread. Hold the reads, type in the channel composer meanwhile, and check the composer keeps focus and text after the reopening completes. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Joel Robotham --- tests/browser/thread-unread.spec.mjs | 77 ++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/tests/browser/thread-unread.spec.mjs b/tests/browser/thread-unread.spec.mjs index 3adfe03f10..d79d1d7551 100644 --- a/tests/browser/thread-unread.spec.mjs +++ b/tests/browser/thread-unread.spec.mjs @@ -125,6 +125,83 @@ test.describe("mentioned reply priority", () => { ); } }); + + test("an Activity reopening leaves focus the person moved meanwhile", async ({ + page, + app, + }) => { + // As above, hold the clock so the thread cannot be read while it opens. + await page.clock.install(); + await holdReadingFocus(page); + await open(page, app); + const root = app.histories + .get("primary/alpha") + .find((row) => row.content === "Thread root 1"); + const panel = page.getByRole("complementary", { + name: "Thread", + exact: true, + }); + const composer = page.getByRole("textbox", { + name: "Message #Alpha", + exact: true, + }); + await page.clock.pauseAt((await page.evaluate(() => Date.now())) + 1000); + await page + .locator(`[data-channel-timeline] [data-message-id="${root.id}"]`) + .getByRole("button", { name: /^View thread:/ }) + .click(); + await expect(panel).toBeVisible(); + await page.evaluate(() => document.activeElement?.blur()); + await page.clock.resume(); + await page.locator('button[data-channel-id="alpha"]').hover(); + const item = page + .getByRole("dialog", { name: "Activity in Alpha" }) + .getByRole("button", { name: /Open unread thread from/ }) + .filter({ hasText: "Unread reply 1" }); + await expect(item).toBeVisible(); + await releaseReadingFocus(page); + const previousAttempt = await page.evaluate( + () => window.fixtureNavigation.snapshot().attempt.id, + ); + // Hold the reopening's reads so the person can move focus before the + // thread receives its request. + let held = false; + let release; + let sawRead; + const gate = new Promise((resolve) => { + release = resolve; + }); + const read = new Promise((resolve) => { + sawRead = resolve; + }); + await page.route("**/api/relay/**/query", async (route) => { + if (held) { + sawRead(); + await gate; + } + await route.continue().catch(() => {}); + }); + held = true; + try { + await item.click(); + await read; + await composer.click(); + await page.keyboard.type("Still typing"); + } finally { + held = false; + release(); + } + await expect + .poll(() => + page.evaluate((previous) => { + const { status, attempt } = window.fixtureNavigation.snapshot(); + return { status, freshAttempt: attempt.id !== previous }; + }, previousAttempt), + ) + .toEqual({ status: "opened", freshAttempt: true }); + await expect(composer).toBeFocused(); + await expect(composer).toHaveText("Still typing"); + }); }); test("thread buttons show observed unread independently, clear only after reading, and expose hover/focus affordance", async ({ From d458d7d9e20a150d43ab38eebd1258c6d948c907 Mon Sep 17 00:00:00 2001 From: Joel Robotham Date: Fri, 23 Oct 2026 10:54:17 +1100 Subject: [PATCH 7/7] test(github): let each PR video finish instead of racing its Pause The fixture clip lasts two seconds. On CI's Linux WebKit the Pause button took about two seconds to settle, so the clip ended first, the button became Play, and the click restarted it. The next lookup by position then found no second Play button. Each video now uses its own player's controls and plays to the end. Pause clicks remain covered by the video player tests and messages.spec. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Joel Robotham --- tests/browser/github-body.spec.mjs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/browser/github-body.spec.mjs b/tests/browser/github-body.spec.mjs index f109af56bc..636cd3db7f 100644 --- a/tests/browser/github-body.spec.mjs +++ b/tests/browser/github-body.spec.mjs @@ -126,9 +126,11 @@ test("PR media plays under the packaged media policy in a narrow GitHub panel", ).toHaveCSS("list-style-type", "disc"); for (let index = 0; index < 2; index++) { const video = videos.nth(index); - await panel + // Each player owns its controls. The two-second clip can end before a + // Pause click lands, flipping that button to Play, so let it finish. + const player = video.locator(".."); + await player .getByRole("button", { name: "Play video", exact: true }) - .nth(index) .click(); await expect .poll(() => video.evaluate((element) => element.currentTime)) @@ -136,9 +138,12 @@ test("PR media plays under the packaged media policy in a narrow GitHub panel", await expect .poll(() => video.evaluate((element) => element.videoWidth)) .toBeGreaterThan(0); - await panel - .getByRole("button", { name: "Pause video", exact: true }) - .click(); + await expect + .poll(() => video.evaluate((element) => element.ended)) + .toBe(true); + await expect( + player.getByRole("button", { name: "Play video", exact: true }), + ).toBeVisible(); } for (const source of [before, after]) await expect(panel.locator(`a[href="${source}"]`)).toHaveCount(0);