Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions apps/app/src/components/ui/app-route-anchor.test.tsx
Original file line number Diff line number Diff line change
@@ -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: <old> }` 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(
<MemoryRouter initialEntries={["/threads/thr-old"]}>
<RouteNavigationProvider>
<NavigationSampler />
<RouteAnchor href="/threads/thr-new">open thr-new</RouteAnchor>
</RouteNavigationProvider>
</MemoryRouter>,
);
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",
});
});
});
47 changes: 39 additions & 8 deletions apps/app/src/components/ui/app-route-anchor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
useLayoutEffect,
useMemo,
useRef,
useTransition,
type ComponentPropsWithoutRef,
type MouseEvent as ReactMouseEvent,
type ReactNode,
Expand Down Expand Up @@ -36,6 +37,23 @@ type RouteNavigate = (path: string, options?: RouteNavigateOptions) => void;

const RouteNavigationContext = createContext<RouteNavigate | null>(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.
Expand Down Expand Up @@ -94,13 +112,24 @@ export function RouteNavigationProvider({
useLayoutEffect(() => {
navigateRef.current = navigate;
}, [navigate]);
const navigateRoute = useCallback<RouteNavigate>((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<RouteNavigate>(
(path, options) => {
startNavigationTransition(() => {
if (options === undefined) {
navigateRef.current(path);
return;
}
navigateRef.current(path, options);
});
},
[startNavigationTransition],
);
useEffect(() => {
const browserApi = getDesktopBrowserApi();
if (browserApi === null) {
Expand All @@ -116,7 +145,9 @@ export function RouteNavigationProvider({

return (
<RouteNavigationContext.Provider value={navigateRoute}>
{children}
<RouteNavigationPendingContext.Provider value={isNavigationPending}>
{children}
</RouteNavigationPendingContext.Provider>
</RouteNavigationContext.Provider>
);
}
Expand Down
66 changes: 65 additions & 1 deletion apps/app/src/components/ui/disclosure.test.tsx
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -21,6 +23,7 @@ afterEach(() => {
cleanup();
vi.unstubAllGlobals();
vi.restoreAllMocks();
vi.useRealTimers();
});

function renderPanel(isExpanded: boolean) {
Expand Down Expand Up @@ -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 (
<ExpandablePanel
isExpanded={isExpanded}
summaryContent="Tool call"
headerToneClass="text-foreground"
onToggle={() => setIsExpanded((expanded) => !expanded)}
>
<span>Expanded body</span>
</ExpandablePanel>
);
}

describe("ExpandablePanel deferred body realization", () => {
it("flips the caret in the tap's commit and mounts the body in a deferred one", () => {
render(<TogglablePanel />);
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(<TogglablePanel />);
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();
});
});
13 changes: 11 additions & 2 deletions apps/app/src/components/ui/disclosure.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { useSetAtom } from "jotai";
import {
useDeferredValue,
useEffect,
useLayoutEffect,
useMemo,
Expand Down Expand Up @@ -223,12 +224,20 @@ export function ExpandablePanel({
const headerRootClassName = cn("px-2 py-1", headerClassName);
const [isClosing, setIsClosing] = useState(false);
const renderedBodyRef = useRef<ReactNode>(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
Expand Down
32 changes: 32 additions & 0 deletions apps/app/src/components/ui/sidebar.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down
11 changes: 9 additions & 2 deletions apps/app/src/components/ui/sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
11 changes: 7 additions & 4 deletions apps/app/src/views/RootComposeMobileRecents.tsx
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -117,8 +117,11 @@ function MobileRecentThreadRow({
);
return (
<li>
<Link
to={getThreadRoutePath({
{/* RouteAnchor, not react-router's Link: it navigates through the
stable transition-priority navigate, so the tap paints before the
thread view's commit instead of stalling on it. */}
<RouteAnchor
href={getThreadRoutePath({
projectId: thread.projectId,
threadId: thread.id,
})}
Expand All @@ -139,7 +142,7 @@ function MobileRecentThreadRow({
<span className="flex size-6 shrink-0 items-center justify-center">
<ThreadStatusGlyph {...indicatorState} />
</span>
</Link>
</RouteAnchor>
</li>
);
}
Expand Down
Loading
Loading