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/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 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; 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 (
  • - - +
    • ); } 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 () => {