From 2ca2f1342c187024780fadb8ce3bb59215726211 Mon Sep 17 00:00:00 2001 From: Vedran Burojevic Date: Mon, 24 Aug 2026 09:15:36 +0200 Subject: [PATCH 1/4] Navigate app routes at transition priority with a pending signal Every route tap ran navigateRef.current() bare inside the click's discrete event. Wrap it in useTransition's startTransition inside RouteNavigationProvider so the tap's urgent commit paints first, and expose isPending through a separate RouteNavigationPendingContext (the navigate context identity stays stable, so navigate consumers still never re-render per navigation). Replace the raw react-router in RootComposeMobileRecents with RouteAnchor so the mobile recents rows take the same path. New test proves ordering: the tap's commit shows pending with the old route still mounted, and the destination lands in a later transition commit. Co-Authored-By: Claude Fable 5 --- .../components/ui/app-route-anchor.test.tsx | 65 +++++++++++++++++++ .../src/components/ui/app-route-anchor.tsx | 47 +++++++++++--- .../src/views/RootComposeMobileRecents.tsx | 11 ++-- 3 files changed, 111 insertions(+), 12 deletions(-) create mode 100644 apps/app/src/components/ui/app-route-anchor.test.tsx diff --git a/apps/app/src/components/ui/app-route-anchor.test.tsx b/apps/app/src/components/ui/app-route-anchor.test.tsx new file mode 100644 index 0000000000..f346393344 --- /dev/null +++ b/apps/app/src/components/ui/app-route-anchor.test.tsx @@ -0,0 +1,65 @@ +// @vitest-environment jsdom + +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { MemoryRouter, useLocation } from "react-router-dom"; +import { afterEach, describe, expect, it } from "vitest"; +import { + RouteAnchor, + RouteNavigationProvider, + useIsRouteNavigationPending, +} from "./app-route-anchor"; + +afterEach(() => { + cleanup(); +}); + +interface NavigationSample { + isPending: boolean; + pathname: string; +} + +const samples: NavigationSample[] = []; + +/** + * Records every committed (isPending, pathname) pair. The pairs prove commit + * ordering: a `{ isPending: true, pathname: }` sample means the pending + * flag painted while the previous route was still on screen, i.e. the tap's + * event did not flush the destination route synchronously. + */ +function NavigationSampler() { + const isPending = useIsRouteNavigationPending(); + const { pathname } = useLocation(); + samples.push({ isPending, pathname }); + return null; +} + +describe("RouteAnchor transition navigation", () => { + it("swaps the route in a later commit than the tap and signals pending in between", () => { + samples.length = 0; + render( + + + + open thr-new + + , + ); + expect(samples).toEqual([ + { isPending: false, pathname: "/threads/thr-old" }, + ]); + + fireEvent.click(screen.getByRole("link", { name: "open thr-new" })); + + // The tap's urgent commit shows the pending affordance with the old route + // still mounted; the destination route lands in a follow-up transition + // commit, which also clears the pending flag. + expect(samples).toContainEqual({ + isPending: true, + pathname: "/threads/thr-old", + }); + expect(samples.at(-1)).toEqual({ + isPending: false, + pathname: "/threads/thr-new", + }); + }); +}); diff --git a/apps/app/src/components/ui/app-route-anchor.tsx b/apps/app/src/components/ui/app-route-anchor.tsx index 341a02db2f..72658a943d 100644 --- a/apps/app/src/components/ui/app-route-anchor.tsx +++ b/apps/app/src/components/ui/app-route-anchor.tsx @@ -6,6 +6,7 @@ import { useLayoutEffect, useMemo, useRef, + useTransition, type ComponentPropsWithoutRef, type MouseEvent as ReactMouseEvent, type ReactNode, @@ -36,6 +37,23 @@ type RouteNavigate = (path: string, options?: RouteNavigateOptions) => void; const RouteNavigationContext = createContext(null); +// Separate from RouteNavigationContext on purpose: the pending bit flips on +// every navigation, and folding it into the navigate context would re-render +// every navigate consumer (sidebar rows, thread actions) per navigation — +// the exact churn RouteNavigationContext exists to avoid. +const RouteNavigationPendingContext = createContext(false); + +/** + * True while a navigation started through {@link useRouteNavigate} or + * {@link RouteAnchor} is still rendering the destination route. Navigation + * runs at transition priority, so the previous route stays on screen for a + * beat; surfaces read this to show a lightweight pending affordance (e.g. + * keeping the tapped row's active state) instead of appearing unresponsive. + */ +export function useIsRouteNavigationPending(): boolean { + return useContext(RouteNavigationPendingContext); +} + /** * A `navigate` whose identity never changes and whose caller does not * subscribe to the router's location. @@ -94,13 +112,24 @@ export function RouteNavigationProvider({ useLayoutEffect(() => { navigateRef.current = navigate; }, [navigate]); - const navigateRoute = useCallback((path, options) => { - if (options === undefined) { - navigateRef.current(path); - return; - } - navigateRef.current(path, options); - }, []); + // Navigate at transition priority: a tap's urgent commit (active states, + // isNavigationPending) paints first, and the destination route renders in an + // interruptible follow-up commit instead of blocking the tap's frame. + // `startNavigationTransition` has a stable identity, so `navigateRoute` + // keeps the never-changing identity its consumers depend on. + const [isNavigationPending, startNavigationTransition] = useTransition(); + const navigateRoute = useCallback( + (path, options) => { + startNavigationTransition(() => { + if (options === undefined) { + navigateRef.current(path); + return; + } + navigateRef.current(path, options); + }); + }, + [startNavigationTransition], + ); useEffect(() => { const browserApi = getDesktopBrowserApi(); if (browserApi === null) { @@ -116,7 +145,9 @@ export function RouteNavigationProvider({ return ( - {children} + + {children} + ); } diff --git a/apps/app/src/views/RootComposeMobileRecents.tsx b/apps/app/src/views/RootComposeMobileRecents.tsx index 1ad8475780..881c9c53f9 100644 --- a/apps/app/src/views/RootComposeMobileRecents.tsx +++ b/apps/app/src/views/RootComposeMobileRecents.tsx @@ -1,6 +1,6 @@ import { useMemo } from "react"; -import { Link } from "react-router-dom"; import type { ThreadListEntry } from "@bb/domain"; +import { RouteAnchor } from "@/components/ui/app-route-anchor"; import { ThreadStatusGlyph } from "@/components/sidebar/ThreadRow"; import { SIDEBAR_WORKING_STATUS_COLOR_CLASS } from "@/components/sidebar/sidebarRowClasses"; import { CHROME_SECTION_LABEL_CLASS } from "@bb/shared-ui/chrome-style-tokens"; @@ -117,8 +117,11 @@ function MobileRecentThreadRow({ ); return (
  • - - +
    • ); } From 61b765bbed47b322e1672794ca07684d82cc80be Mon Sep 17 00:00:00 2001 From: Vedran Burojevic Date: Mon, 24 Aug 2026 13:45:02 +0200 Subject: [PATCH 2/4] Defer expander bodies out of the toggle's click commit Expanding a collapsed ExpandablePanel materialized the whole body subtree inside the click's discrete commit (button.inline-flex stalls in the hang ledger). Drive the expandedBody memo from useDeferredValue(isExpanded) so the caret/header flip paints in the tap's first frame and the body mounts in a follow-up interruptible commit. Header state, the closing-body ref retention, and the layout-animation signal stay on the urgent value; rows that mount already expanded still render their body immediately (useDeferredValue returns the live value on first render). New test fails before this change: with flushSync standing in for the tap's urgent flush, the body used to be mounted in that same commit. Co-Authored-By: Claude Fable 5 --- .../app/src/components/ui/disclosure.test.tsx | 66 ++++++++++++++++++- apps/app/src/components/ui/disclosure.tsx | 13 +++- 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/apps/app/src/components/ui/disclosure.test.tsx b/apps/app/src/components/ui/disclosure.test.tsx index 64df40de0d..69844dd0fb 100644 --- a/apps/app/src/components/ui/disclosure.test.tsx +++ b/apps/app/src/components/ui/disclosure.test.tsx @@ -1,6 +1,8 @@ // @vitest-environment jsdom -import { act, cleanup, render } from "@testing-library/react"; +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { useState } from "react"; +import { flushSync } from "react-dom"; import { afterEach, describe, expect, it, vi } from "vitest"; import { ExpandablePanel } from "./disclosure"; @@ -21,6 +23,7 @@ afterEach(() => { cleanup(); vi.unstubAllGlobals(); vi.restoreAllMocks(); + vi.useRealTimers(); }); function renderPanel(isExpanded: boolean) { @@ -85,3 +88,64 @@ describe("ExpandablePanel body height", () => { expect(region.style.transitionDuration).toBe("0s"); }); }); + +/** A toggleable panel driven by its own header, like a timeline tool row. */ +function TogglablePanel() { + const [isExpanded, setIsExpanded] = useState(false); + return ( + setIsExpanded((expanded) => !expanded)} + > + Expanded body + + ); +} + +describe("ExpandablePanel deferred body realization", () => { + it("flips the caret in the tap's commit and mounts the body in a deferred one", () => { + render(); + const header = screen.getByRole("button", { name: "Tool call" }); + + let bodyMountedInToggleCommit: boolean | null = null; + let headerExpandedInToggleCommit: string | null = null; + act(() => { + // flushSync stands in for the tap's discrete event: it flushes only the + // urgent lane, so the deferred body re-render is still pending when the + // samples are taken and lands when act exits. + flushSync(() => { + header.click(); + }); + bodyMountedInToggleCommit = screen.queryByText("Expanded body") !== null; + headerExpandedInToggleCommit = header.getAttribute("aria-expanded"); + }); + + // The tap's synchronous commit flips the caret without paying for the + // body subtree; the body lands in the follow-up interruptible commit. + expect(headerExpandedInToggleCommit).toBe("true"); + expect(bodyMountedInToggleCommit).toBe(false); + expect(screen.getByText("Expanded body")).toBeTruthy(); + }); + + it("keeps the closing body mounted through the collapse animation", () => { + vi.useFakeTimers(); + render(); + const header = screen.getByRole("button", { name: "Tool call" }); + fireEvent.click(header); + expect(screen.getByText("Expanded body")).toBeTruthy(); + + fireEvent.click(header); + + // The collapse animates from the still-rendered subtree: the body must + // stay mounted for the 200ms transition, then unmount. + expect(header.getAttribute("aria-expanded")).toBe("false"); + expect(screen.getByText("Expanded body")).toBeTruthy(); + + act(() => { + vi.advanceTimersByTime(200); + }); + expect(screen.queryByText("Expanded body")).toBeNull(); + }); +}); diff --git a/apps/app/src/components/ui/disclosure.tsx b/apps/app/src/components/ui/disclosure.tsx index e052238e02..1de25ef77a 100644 --- a/apps/app/src/components/ui/disclosure.tsx +++ b/apps/app/src/components/ui/disclosure.tsx @@ -1,5 +1,6 @@ import { useSetAtom } from "jotai"; import { + useDeferredValue, useEffect, useLayoutEffect, useMemo, @@ -223,12 +224,20 @@ export function ExpandablePanel({ const headerRootClassName = cn("px-2 py-1", headerClassName); const [isClosing, setIsClosing] = useState(false); const renderedBodyRef = useRef(null); + // The header/chevron flip stays urgent (it reads `isExpanded` directly), + // but the body realizes off the deferred value: an expand tap's discrete + // commit paints the caret in the first frame, and the body subtree — the + // expensive part of a large tool section — mounts in a follow-up + // interruptible commit. On first render the deferred value equals + // `isExpanded`, so rows that mount already expanded render their body + // immediately. + const deferredIsExpanded = useDeferredValue(isExpanded); const expandedBody = useMemo(() => { - if (!isExpanded) { + if (!deferredIsExpanded) { return null; } return renderBody ? renderBody() : children; - }, [children, isExpanded, renderBody]); + }, [children, deferredIsExpanded, renderBody]); // Signal to AutoHeightContainer / HeightTransition wrappers that a // CSS-driven layout animation is in flight, so they snap their wrapper to From 7b22677671829cf0002a4698474e4935eb98c5ad Mon Sep 17 00:00:00 2001 From: Vedran Burojevic Date: Mon, 24 Aug 2026 13:47:16 +0200 Subject: [PATCH 3/4] Realize the mobile sidebar at transition priority When boot's idle pre-realization has not run yet, the sidebar trigger's click flushed the whole ProjectList/ThreadRow subtree synchronously before the slide's first frame could composite. Wrap realizeMobileSidebar() in React.startTransition so the tap's flush only writes the inline drag styles (the slide starts immediately) and the subtree mounts interruptibly during the settle window. The drag-style write order is unchanged, and the settle commit's render-phase latch still realizes the subtree synchronously if it somehow lands first. The other flushSync sites in this file run after the settle window (deferred open/close commits and the swipe settle paths), not in the tap's critical path, and are deliberately untouched. New test fails before this change: with flushSync standing in for the tap's urgent flush, the subtree used to be realized in that same flush. Co-Authored-By: Claude Fable 5 --- apps/app/src/components/ui/sidebar.test.tsx | 32 +++++++++++++++++++++ apps/app/src/components/ui/sidebar.tsx | 11 +++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/apps/app/src/components/ui/sidebar.test.tsx b/apps/app/src/components/ui/sidebar.test.tsx index 86f26aff67..0c2932f1d5 100644 --- a/apps/app/src/components/ui/sidebar.test.tsx +++ b/apps/app/src/components/ui/sidebar.test.tsx @@ -8,6 +8,7 @@ import { screen, } from "@testing-library/react"; import { memo } from "react"; +import { flushSync } from "react-dom"; import { renderToString } from "react-dom/server"; import { afterEach, describe, expect, it, vi } from "vitest"; import { CompactViewportOverrideProvider } from "@bb/shared-ui/hooks/use-compact-viewport"; @@ -331,6 +332,37 @@ describe("mobile sidebar deferred realization", () => { expect(getMobilePanel()?.textContent).toContain("Sidebar content"); }); + it("keeps the realize commit out of the open tap's synchronous flush", () => { + vi.useFakeTimers(); + renderCompactSidebarHarness(); + const trigger = screen.getByRole("button", { name: "Toggle Sidebar" }); + + let panelStyledForSlideInTap = false; + let realizedInTapFlush = true; + act(() => { + // flushSync stands in for the tap's discrete event: it flushes only the + // urgent lane, so the transition-priority realize commit is still + // pending when the samples are taken and lands when act exits. + flushSync(() => { + trigger.click(); + }); + const panel = getMobilePanel(); + panelStyledForSlideInTap = panel?.style.translate === "0%"; + realizedInTapFlush = + panel?.textContent?.includes("Sidebar content") ?? false; + }); + + // The tap's own flush only starts the slide (inline drag styles); the + // subtree mounts in the interruptible commit that follows, so the first + // frame of the slide never waits on the realize commit. + expect(panelStyledForSlideInTap).toBe(true); + expect(realizedInTapFlush).toBe(false); + expect(getMobilePanel()?.textContent).toContain("Sidebar content"); + + settleMobileToggle(); + expect(getMobilePanel()?.dataset.state).toBe("open"); + }); + // The width is an inherited custom property unless registered otherwise // (theme.css registers it non-inherited). Either way it must be written on // the elements that read it and never on the provider wrapper: the wrapper diff --git a/apps/app/src/components/ui/sidebar.tsx b/apps/app/src/components/ui/sidebar.tsx index acd4df98b3..1e36dacf98 100644 --- a/apps/app/src/components/ui/sidebar.tsx +++ b/apps/app/src/components/ui/sidebar.tsx @@ -636,8 +636,15 @@ const SidebarProvider = React.forwardRef< } // Mount the subtree now if boot has not realized it yet, so it commits - // during the slide instead of after the settle. - realizeMobileSidebar(); + // during the slide instead of after the settle. Transition priority: + // the realize commit (ProjectList/ThreadRow, thousands of lines on a + // cold route) must not block this tap's frame — the drag-style write + // below composites the slide first and the subtree mounts + // interruptibly during the settle window. If the settle commit beats + // the transition, the render-phase latch below realizes it there. + React.startTransition(() => { + realizeMobileSidebar(); + }); applySidebarMobileDragStyles({ progress: 1, settling: true }); mobileSettleTimeoutRef.current = window.setTimeout(() => { mobileSettleTimeoutRef.current = null; From 28f6f596a64624dfc4ee9eb5af1c2de3331f6270 Mon Sep 17 00:00:00 2001 From: Vedran Burojevic Date: Mon, 24 Aug 2026 13:48:59 +0200 Subject: [PATCH 4/4] Early-exit the maximize restore loop The maximize/restore scroll-preservation loop unconditionally rewrote scrollLeft/scrollTop on every tracked element for 30 animation frames, forcing layout each frame for half a second after every toggle. Make restore() compare before writing and report whether anything needed correction, stop the rAF loop after the first frame with zero corrections, and cap the loop at 5 frames. The pre-paint initial restore() stays. New test fails before this change: the settled case saw 31 scroll writes (pre-paint + 30 frames); now it sees none, and an adversarial scroller that keeps normalizing to zero is corrected at most 6 times. Co-Authored-By: Claude Fable 5 --- .../thread-detail/SplitThreadArea.test.tsx | 45 +++++++++++++++++++ .../views/thread-detail/SplitThreadArea.tsx | 28 +++++++++--- 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/apps/app/src/views/thread-detail/SplitThreadArea.test.tsx b/apps/app/src/views/thread-detail/SplitThreadArea.test.tsx index 96314a8f48..0e87c509a8 100644 --- a/apps/app/src/views/thread-detail/SplitThreadArea.test.tsx +++ b/apps/app/src/views/thread-detail/SplitThreadArea.test.tsx @@ -783,6 +783,51 @@ describe("SplitThreadArea", () => { await waitFor(() => expect(hiddenScroller.scrollTop).toBe(0)); }); + it("stops the restore loop once positions settle instead of burning 30 frames", async () => { + renderSplitArea({ + path: threadPath("thr-a"), + layout: twoPaneLayout("pane-1"), + }); + const hiddenScroller = screen.getByTestId("scroll-thr-b"); + hiddenScroller.scrollTop = 12; + fireEvent.scroll(hiddenScroller); + + let scrollTopValue = 12; + const writes: number[] = []; + Object.defineProperty(hiddenScroller, "scrollTop", { + configurable: true, + get: () => scrollTopValue, + set: (value: number) => { + writes.push(value); + scrollTopValue = value; + }, + }); + + // Maximize: the tracked element already sits at its saved offset, so the + // pre-paint restore and the first frame find nothing to correct and the + // loop must end without a single scroll write (each write would force + // layout every frame for half a second). + fireEvent.click(screen.getByTestId("maximize-thr-a")); + await new Promise((resolve) => setTimeout(resolve, 600)); + expect(writes).toHaveLength(0); + + // Restore, with the scroller reporting 0 on every read — an adversary + // that keeps normalizing the position. The loop corrects before paint and + // on each frame, but gives up at the frame cap instead of running all 30. + Object.defineProperty(hiddenScroller, "scrollTop", { + configurable: true, + get: () => 0, + set: (value: number) => { + writes.push(value); + }, + }); + fireEvent.click(screen.getByTestId("maximize-thr-a")); + await new Promise((resolve) => setTimeout(resolve, 600)); + expect(writes.length).toBeGreaterThan(0); + // Pre-paint restore + at most 5 frames. + expect(writes.length).toBeLessThanOrEqual(6); + }); + it("toggles the focused pane through the discoverable app command", async () => { const store = renderSplitArea({ path: threadPath("thr-b"), diff --git a/apps/app/src/views/thread-detail/SplitThreadArea.tsx b/apps/app/src/views/thread-detail/SplitThreadArea.tsx index 8d0c53b721..e6e122ca8d 100644 --- a/apps/app/src/views/thread-detail/SplitThreadArea.tsx +++ b/apps/app/src/views/thread-detail/SplitThreadArea.tsx @@ -215,30 +215,44 @@ function usePreservedSplitScrollPositions(maximizedPaneId: string | null) { } previousMaximizedPaneIdRef.current = maximizedPaneId; - const restore = () => { + /** Reapplies saved positions; true when any element needed correction. */ + const restore = (): boolean => { const workspace = workspaceRef.current; + let corrected = false; for (const [element, position] of positionsRef.current) { if (workspace === null || !workspace.contains(element)) { positionsRef.current.delete(element); continue; } + if ( + element.scrollLeft === position.left && + element.scrollTop === position.top + ) { + continue; + } element.scrollLeft = position.left; element.scrollTop = position.top; + corrected = true; } + return corrected; }; // Restore before paint, then briefly across animation frames so passive // timeline effects, virtualization, and browser scroll anchoring cannot - // overwrite the saved position while pane visibility settles. + // overwrite the saved position while pane visibility settles. Each frame + // forces layout on every tracked scroller, so the loop ends after the + // first frame with nothing to correct; the frame cap bounds the + // pathological case where something keeps fighting the restore. restore(); let frame: number | null = null; - let framesRemaining = 30; + let framesRemaining = 5; const restoreUntilSettled = () => { - restore(); + const corrected = restore(); framesRemaining -= 1; - if (framesRemaining > 0) { - frame = window.requestAnimationFrame(restoreUntilSettled); - } + frame = + corrected && framesRemaining > 0 + ? window.requestAnimationFrame(restoreUntilSettled) + : null; }; frame = window.requestAnimationFrame(restoreUntilSettled); return () => {