From 3e0b48d0f71a3226996a5d180fb697b77362ebbe Mon Sep 17 00:00:00 2001 From: mtoohey Date: Wed, 4 Nov 2026 19:58:19 +0000 Subject: [PATCH] fix(channels): keep a panel reopened before its close's return lands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Two WebKit journeys lost a Profile that was reopened right after its tab closed. Both failed on attempt 1 on unrelated branches, and the same SHA passed on rerun. - `profiles.spec.mjs:321` "profile plumbing: exact avatar/mention targets, thread enrichment, lifecycle and recovery", branch peon/participant-collator, run 41752442750 (webkit 13/14), https://github.com/block/buzz-app/actions/runs/41752442750/job/126320885261 The test closed the Mic tab and pressed Space on the focused mention. The Profile never reopened, and the test failed at :479, `expect(key).toHaveText(npubs.mic)`. - `solo-workspace.spec.mjs:159` "Solo chat Activity opens the agent's profile locally, restores per conversation and retires on plugin disable", branch peon/dnd-stable-transform, run 41757728323 attempt 1 (webkit 5/14), https://github.com/block/buzz-app/actions/runs/41757728323/job/126338025674 The test closed the agent's Profile and pressed Enter on the focused Activity trigger. "Profile details" never appeared. Since #1729 on main, Enter opens the "Working now" popover, and its "View … activity" row opens the Profile through the same `openLink` path. ## Cause Closing a routed panel tab removes it locally. `dismissContent` then calls `navigator.dismiss`. The browser driver retires the visit and calls `history.go(-1)`, and that return lands on a later popstate. Until it lands, the navigator still shows the Profile visit that is being retired. `afterClose` focuses the opener synchronously, so the opener can be activated within that window. The reopen runs `open()` and then `visitPanel`. `visitPanel` sees the navigator already on that Profile's detail and returns without navigating, so the panel opens only locally. When the return lands, the route effect enters the channel visit. `enterVisit` restores the channel visit's saved selection, and that hides the reopened panel. A reopen of a different panel fares no better. Its visit is pushed on top of the retiring one, where the late return removes it. Both traces show this sequence. In the profiles run the close click finished at 80128 ms. Space ran at 80148-80193 while the URL still held the Mic detail. By 80383 the URL was the channel and the dock was closing again. In the Solo run the close finished at 251783. Enter ran at 251808-251872 while the URL still held the agent's activity detail. By 251999 the URL was the channel and the dock was hidden. The dock is not the cause. `PanelDock` keeps a reopened subtree through its exit (`PanelDock.test.tsx`), and `inert` covers only the dock, not the openers. Its exit motion just overlaps the pending return. ## Fix A reopen before the return lands now supersedes the close: - `dismissContent` records a pending return when its dismissal retires the current visit and the navigator hasn't moved yet. A synchronous driver or a replacing dismissal moves it at once, so nothing is recorded for them. - While that return is pending, `visitPanel` doesn't navigate. It records the panel as the reopen. - When the return lands on the dismissal's fallback, the route effect checks whether that panel is still selected. If it is, the effect reselects it over the restored selection and opens it, which gives it its own visit. If it has closed again, or the return landed somewhere else, nothing is revived. - A later close before the return lands, such as closing the thread beneath a closed Profile, retires more visits and moves where the return lands. The record follows the latest fallback and keeps the reopen. - While its return is pending, the route effect no longer reapplies the retiring visit's detail. A re-render in that window would otherwise reopen the panel that was just closed, and the deferral would then keep it. `open` changes identity on a close whenever a panel sits in the main pane, so such a re-render can happen. Thread and other explicit navigation during a pending return are not deferred. #2113 lists that as a remaining limit. #2113's close-focus handoff doesn't overlap these hunks, and its head merges cleanly with this change. Two cases in `profiles.spec.mjs` hold `history.go` and pause the dock's exit while a Profile closes. One reopens Mic by click and by Space. The other closes a thread's Profile and then the thread, and opens Mic. Each case asserts that: - the Profile shows while the return is held - the navigator stays on the closed visit and exactly one return is held - a fresh Mic visit lands after the release - Back returns to the channel and closes the panel The journeys are unchanged, and no assertions are loosened. ## Validation Local Linux workstation, Playwright 1.63.0, `--no-deps --retries=0`: - The new cases without the fix: 4/4 failed. The reopen case failed at :406 (no fresh visit). The thread case failed at :442: the click stacked a visit on the closed one while the return was held. With the fix but without the latest-fallback step, the thread case failed 2/2 at :452. With the fix: all pass. - Adversarial ordering, using temporary, uncommitted changes: both journeys delayed `history.go` calls from click or key handlers by 250 ms, 6 repeats per engine. Without the fix: 9/24 failed. Seven were the CI signatures: profiles at the shifted :479 (3), and Solo's Profile never visible (4). Two were at the thread mention, where a thread opened while the delayed return was still pending, which is the non-panel limit above. With the fix: 24/24 passed, and 24/24 again on the final code. - Natural contention: both journeys pinned to 4 CPUs shared with 4 busy loops, 2 workers, 10 repeats per engine. With the fix: 40/40 passed. The unfixed code wasn't run under natural contention. - Full `profiles.spec.mjs` and `solo-workspace.spec.mjs`, both engines: 48/48 passed. - The 38 browser specs that close panel tabs, both engines, 12 workers: 495 passed, 1 skipped, 42 failed. The failures were mostly `page.goto` and click timeouts while the test server had stopped serving. All 42 passed when rerun with 6 workers. - Vitest for channels, panels, navigation and channel-navigation: 87 files, 1539 tests. Five files timed out while sharing the machine with that browser run, and all five (248 tests) passed when rerun alone. - `tsc --noEmit`, `biome check` and `git diff --check` are clean. A green run doesn't prove the flake is gone. Hosted CI covers broad validation. Co-Authored-By: Claude Opus 5.5 Signed-off-by: mtoohey (cherry picked from commit 503f82aba732070adbc133398f5c6f83735d9a53) --- src/bundled/channels/ChannelsPage.tsx | 50 ++++++++- tests/browser/profiles.spec.mjs | 139 ++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 2 deletions(-) diff --git a/src/bundled/channels/ChannelsPage.tsx b/src/bundled/channels/ChannelsPage.tsx index 1649efe221..26679b9fc8 100644 --- a/src/bundled/channels/ChannelsPage.tsx +++ b/src/bundled/channels/ChannelsPage.tsx @@ -987,10 +987,23 @@ function ChannelWorkspace({ const returnFocus = useRef< { trigger: HTMLElement; channelId: string | undefined } | undefined >(undefined); + // A close's native return lands a task or more after its dismissal. Until + // then the navigator still shows the retiring visit. + const pendingReturn = useRef<{ + from: string; + target: string; + reopen?: PanelOpening; + }>(undefined); const visitPanel = useCallback( (entry: PanelOpening) => { - const target = navigator?.snapshot().entry.target; + const { id, target } = navigator?.snapshot().entry ?? {}; if (!navigator || target?.kind !== "conversation") return; + // Opening now would revisit or stack on the retiring visit. The panel + // takes its own visit once the return lands. + if (pendingReturn.current?.from === id) { + pendingReturn.current.reopen = entry; + return; + } if ( target.channelId === entry.channelId && target.detail?.target === entry.target && @@ -1092,6 +1105,21 @@ function ChannelWorkspace({ }; } navigator.dismiss(visits, fallback); + // A later close can retire more visits before the return lands, moving + // where it lands. + const pending = + pendingReturn.current?.from === current.id + ? pendingReturn.current + : undefined; + if ( + (pending || visits.includes(current.id)) && + navigator.snapshot().entry.id === current.id + ) + pendingReturn.current = { + ...pending, + from: current.id, + target: targetKey(fallback), + }; }; const secondary = useConversationTabs({ state: tabState, @@ -1336,6 +1364,20 @@ function ChannelWorkspace({ (kept.pane === "main" ? tabState.getMainSelected() : tabState.getSelected()) === kept.id; + // A panel opened before the close's return landed supersedes that + // close, unless it has since closed or the return went elsewhere. + const returned = pendingReturn.current; + const reopen = + returned?.reopen && + navigation.entryId !== returned.from && + targetKey(navigation.target) === returned.target && + [tabState.getSelected(), tabState.getMainSelected()].includes( + panelTabId(returned.reopen), + ) + ? returned.reopen + : undefined; + if (returned && navigation.entryId !== returned.from) + pendingReturn.current = undefined; tabState.enterVisit( `${navigation.entryId}:${detail ? "detail" : routedThread ? "thread" : "channel"}`, detail ? undefined : routedThread, @@ -1344,7 +1386,9 @@ function ChannelWorkspace({ ? { channelId: currentId, target: detail.target } : undefined, ); - if (detail && currentId) { + // A re-render while the return is pending must not reapply the detail + // that was just closed. + if (detail && currentId && navigation.entryId !== returned?.from) { const entry = panelOpening( panels, currentId, @@ -1354,6 +1398,8 @@ function ChannelWorkspace({ if (entry && historyPanel(entry)) open(entry); else navigation.complete({ status: "failed", reason: "unavailable" }); } + // Reselect it over the restored selection and give it its own visit. + if (reopen) open(reopen); if (kept && navigation.entryId !== kept.from) { keptTabReturn.current = undefined; if (retained && targetKey(navigation.target) === kept.target) { diff --git a/tests/browser/profiles.spec.mjs b/tests/browser/profiles.spec.mjs index 5398bbd037..3212b310f5 100644 --- a/tests/browser/profiles.spec.mjs +++ b/tests/browser/profiles.spec.mjs @@ -318,6 +318,145 @@ test("closing a profile beside a main thread preserves the thread destination", .toBe(true); }); +// Close returns through native history, which lands a task or more later, and +// the dock keeps its exit motion. Hold both so a reopen arrives before them. +async function holdCloseReturn(page) { + await page.evaluate(() => { + const go = history.go.bind(history); + const held = []; + history.go = (delta) => held.push(delta); + // Pause the exit while it is still pending, so it cannot finish first. + const exit = new MutationObserver(() => { + const dock = document.querySelector("[data-panel-dock][data-closing]"); + for (const animation of [ + ...(dock?.getAnimations() ?? []), + ...(dock?.parentElement?.getAnimations() ?? []), + ]) + animation.pause(); + }); + exit.observe(document.body, { + subtree: true, + attributeFilter: ["data-closing"], + }); + window.releaseReturn = () => { + exit.disconnect(); + history.go = go; + for (const delta of held) go(delta); + return held.length; + }; + }); + return () => page.evaluate(() => window.releaseReturn()); +} + +async function openReturnFixture(page) { + await page.goto("/tests/fixtures/profiles.html"); + await page.waitForFunction(() => !!window.profilesFixture); + const read = async () => + (await page.evaluate(() => window.profilesFixture.navigation.snapshot())) + .entry; + return { + read, + ...(await page.evaluate(() => ({ + npubs: window.profilesFixture.npubs, + targets: window.profilesFixture.targets, + }))), + channel: (await read()).id, + panel: page.getByRole("complementary", { name: "Profile", exact: true }), + mention: page.getByRole("button", { + name: "View Mic profile", + exact: true, + }), + closing: page.locator("[data-panel-dock][data-closing]:not([hidden])"), + }; +} + +test("a Profile reopened before its close returns stays open with a fresh visit", async ({ + page, +}) => { + const errors = watchPageErrors(page); + const { read, npubs, targets, channel, panel, mention, closing } = + await openReturnFixture(page); + for (const activation of ["click", "Space"]) { + await mention.click(); + await expect(panel.locator("code")).toHaveText(npubs.mic); + const closed = (await read()).id; + const release = await holdCloseReturn(page); + let held; + try { + await page + .getByRole("tablist", { name: "Panel tabs" }) + .getByRole("button", { name: "Close Mic tab", exact: true }) + .click(); + await expect(panel).toHaveCount(0); + await expect(closing).toHaveCount(1); + if (activation === "click") await mention.click(); + else await mention.press(activation); + await expect(panel.locator("code")).toHaveText(npubs.mic); + expect((await read()).id).toBe(closed); + } finally { + held = await release(); + } + expect(held).toBe(1); + // The return lands; the reopened Profile then needs its own visit. + await expect + .poll(async () => { + const entry = await read(); + return entry.id !== closed && entry.target.detail?.target; + }) + .toBe(targets.mic); + await expect(panel.locator("code")).toHaveText(npubs.mic); + await page.goBack(); + await expect.poll(async () => (await read()).id).toBe(channel); + await expect(panel).toHaveCount(0); + } + expect(errors.unexplained()).toEqual([]); +}); + +test("a Profile opened after closing a thread and its Profile stays open with a fresh visit", async ({ + page, +}) => { + const errors = watchPageErrors(page); + const { read, npubs, targets, channel, panel, mention, closing } = + await openReturnFixture(page); + await page + .getByRole("button", { name: "View thread: 1 reply", exact: true }) + .click(); + await page + .getByRole("button", { name: "View Pinky profile", exact: true }) + .click(); + await expect(panel.locator("code")).toHaveText(npubs.pinky); + const closed = (await read()).id; + const tabs = page.getByRole("tablist", { name: "Panel tabs" }); + const release = await holdCloseReturn(page); + let held; + try { + await tabs + .getByRole("button", { name: "Close Pinky tab", exact: true }) + .click(); + // The second close retires the thread under the pending return, which + // then lands on the channel instead. + await tabs.getByRole("button", { name: /^Close .* tab$/ }).click(); + await expect(closing).toHaveCount(1); + await mention.click(); + await expect(panel.locator("code")).toHaveText(npubs.mic); + expect((await read()).id).toBe(closed); + } finally { + held = await release(); + } + expect(held).toBe(1); + await expect + .poll(async () => { + const entry = await read(); + return entry.id !== closed && entry.target.detail?.target; + }) + .toBe(targets.mic); + await expect(panel.locator("code")).toHaveText(npubs.mic); + await page.goBack(); + await expect.poll(async () => (await read()).id).toBe(channel); + await expect(panel).toHaveCount(0); + expect(errors.unexplained()).toEqual([]); +}); + test("profile plumbing: exact avatar/mention targets, thread enrichment, lifecycle and recovery", async ({ page, }, testInfo) => {