From 960dc492d3eb0cad820cb5ac2d9e85e323224a3e Mon Sep 17 00:00:00 2001 From: Sheng Kun Chang Date: Sun, 14 Jun 2026 16:40:09 +0800 Subject: [PATCH] fix: stop sidebar reload from losing saved width and stranding new-chat tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent client-side restore races on reload, both in the kit. The agent server and the save path are sound; this is entirely about RESTORE. Bug 1 — width (deterministic loss on reload when the sidebar was left open). SidebarProvider mounts with defaultWidth 0, so sidebarWidth and lastOpenWidth start at 0. The instant the host's persisted open:true hydrates, AppShell's controlled-open effect synchronously seeds a PLACEHOLDER width (reopenWidth() returns the half-viewport fallback because lastOpenWidth is still 0 mid-restore; in web-applications AgentSessionBridge separately seeds the 350 floor). The async width GET resolves LATER and only seeded a default-0 width (`cur <= 0 ? stored : cur`), so the non-zero placeholder won the race and the user's saved drag width was discarded. Fix: a widthHydratedRef makes the fetched stored width OVERRIDE a placeholder seed exactly once on first hydrate; a genuine user drag (handleSetWidth, width > minOpenWidth) marks the width hydrated so it still wins ("first REAL writer wins"). AppShell now seeds the placeholder through a new `seedWidth` that does NOT mark hydrated (so the GET can override it) and never persists. Bug 2 — tabs/active (intermittent "new chat wins" on reload). The mount hydrate restores remote tabs+activeTabId only when keepDrafts is false, but keepDrafts was `mode === 'refetch' || touchedRef`. A focus/visibilitychange firing reconcile('refetch'), or a host mutation setting touchedRef, DURING the in-flight hydrate made keepDrafts true (or bumped the shared fetchSeq so the hydrate GET early-returned), so the fresh placeholder draft survived and stayed active — the restored conversation was stranded. Fix: a hasHydratedRef forces the first successful load authoritative (keepDrafts=false regardless of mode/touchedRef) and early-returns focus/visibility refetches until the first hydrate settles. Tests: SidebarProvider gains a seed-vs-fetch race test (a non-zero placeholder set before get() resolves to 700; asserts the rendered width becomes 700) plus its inverse (a real pre-fetch drag still wins). useAgentTabs gains two race tests (a host mutation, and a focus/visibility event, each during the in-flight hydrate) asserting the remote tabs+active are restored and the draft is dropped. All four fail on the pre-fix code. No version bump — rides the next kit rollup. web-applications can optionally drop its redundant AgentSessionBridge width seed in a follow-up; not required now since the kit override makes the stored width win the race regardless. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/components/layout/app-shell/AppShell.tsx | 23 ++-- src/context/sidebar/SidebarProvider.test.tsx | 104 +++++++++++++++++++ src/context/sidebar/SidebarProvider.tsx | 71 +++++++++++-- src/hooks/agent-chat/useAgentTabs.test.ts | 76 ++++++++++++++ src/hooks/agent-chat/useAgentTabs.ts | 42 +++++++- 5 files changed, 297 insertions(+), 19 deletions(-) diff --git a/src/components/layout/app-shell/AppShell.tsx b/src/components/layout/app-shell/AppShell.tsx index 3a1a700d3..d43938655 100644 --- a/src/components/layout/app-shell/AppShell.tsx +++ b/src/components/layout/app-shell/AppShell.tsx @@ -293,7 +293,8 @@ function AppShellInner({ agentInputPlaceholder, agentInputLabels, }: AppShellProps) { - const { sidebarWidth, setSidebarWidth, lastOpenWidth } = useSidebarWidth(); + const { sidebarWidth, setSidebarWidth, seedWidth, lastOpenWidth } = + useSidebarWidth(); const [isResizing, setIsResizing] = useState(false); const [windowWidth, setWindowWidth] = useState(() => typeof window === "undefined" ? 0 : window.innerWidth, @@ -377,14 +378,22 @@ function AppShellInner({ // When `open` is controlled, keep the rendered width in lockstep with it: // open === true while the width is still 0 (fresh open, or a reload that - // restored open:true before the async width fetch landed) seeds the width; - // open === false while open collapses it. The width fetch's own reconcile - // (SidebarProvider) only touches a default-0 width, so the two don't fight: - // the first one to land wins and the other no-ops. Uncontrolled - // (open === undefined) skips this entirely. + // restored open:true before the async width fetch landed) SEEDS the width; + // open === false while open collapses it. Uncontrolled (open === undefined) + // skips this entirely. + // + // On reload this seed is a PLACEHOLDER: the persisted open:true hydrates + // synchronously while the async width GET (SidebarProvider) is still in + // flight, and lastOpenWidth is still 0 at that instant, so reopenWidth() + // returns the half-viewport fallback — not the user's saved drag. We seed + // through `seedWidth`, NOT `setSidebarWidth`, so this placeholder does NOT + // mark the width hydrated; the fetched stored width then overrides it exactly + // once when the GET lands (SidebarProvider). A genuine user drag still goes + // through setSidebarWidth and wins. Closing (open=false) is a real intent, so + // it uses setSidebarWidth. useEffect(() => { if (open === undefined) return; - if (open && sidebarWidth === 0) setSidebarWidth(reopenWidth()); + if (open && sidebarWidth === 0) seedWidth(reopenWidth()); else if (!open && sidebarWidth > 0) setSidebarWidth(0); // reopenWidth is recreated each render; we intentionally depend only on // [open, sidebarWidth], the inputs that gate this decision. diff --git a/src/context/sidebar/SidebarProvider.test.tsx b/src/context/sidebar/SidebarProvider.test.tsx index 87b3f4b94..39754b0b5 100644 --- a/src/context/sidebar/SidebarProvider.test.tsx +++ b/src/context/sidebar/SidebarProvider.test.tsx @@ -386,4 +386,108 @@ describe("SidebarProvider injected server persistence (docs 12.4 + 13b)", () => }); expect(screen.getByTestId("last-open").textContent).toBe("0"); }); + + // ---- Bug 1 regression: the seed-vs-fetch width race on reload ---- + // + // On reload SidebarProvider mounts with defaultWidth 0, so sidebarWidth and + // lastOpenWidth are both 0. A NON-ZERO placeholder writer then runs BEFORE the + // async width GET resolves — either AppShell's controlled-open half-viewport + // seed, or (in web-applications) AgentSessionBridge's setSidebarWidth(350) + // clobber. The old reconcile only seeded a default-0 width + // (`cur <= 0 ? stored : cur`), so any non-zero placeholder won the race and + // the saved width was discarded — reload reopened at the placeholder, not the + // user's drag size. The fix overrides the placeholder with the fetched width + // exactly once on first hydrate. + it("overrides a non-zero placeholder seed with the fetched width on first hydrate (Bug 1)", async () => { + // A controllable get() that resolves only when we say so — this is the + // async round-trip that lands AFTER the synchronous placeholder seed. + let resolveGet!: (width: number) => void; + const get = vi.fn( + () => + new Promise((resolve) => { + resolveGet = resolve; + }), + ); + render( + + + , + ); + + // The mount effect calls get() inside a microtask — let it run so the + // deferred resolver is wired, while the GET itself stays in flight. + await act(async () => { + await Promise.resolve(); + }); + expect(get).toHaveBeenCalledTimes(1); + + // Reproduce the racing placeholder writer (e.g. AgentSessionBridge's 350 + // clobber): a non-zero width is set while the GET is still in flight. 350 + // === minOpenWidth, so it is NOT a "real" open width — it must not be + // allowed to defeat the saved width. + act(() => { + fireEvent.click(screen.getByText("Collapse")); + }); + expect(screen.getByTestId("width").textContent).toBe("350"); + + // Now the GET resolves to the user's saved drag width. + await act(async () => { + resolveGet(700); + await Promise.resolve(); + }); + + // The fetched width wins: it overrides the non-zero placeholder seed once. + // (Old behavior: 350 stayed because `cur <= 0` was false.) + expect(screen.getByTestId("width").textContent).toBe("700"); + expect(screen.getByTestId("last-open").textContent).toBe("700"); + }); + + // A genuine user drag landing before the fetch is a REAL width and must win + // over the late GET — "first REAL writer wins". The placeholder seed above + // never reaches this path (it stays at/below the floor), so only an actual + // drag claims hydration. + it("a real pre-fetch user drag wins over the late fetched width (Bug 1, inverse)", async () => { + let resolveGet!: (width: number) => void; + const get = vi.fn( + () => + new Promise((resolve) => { + resolveGet = resolve; + }), + ); + render( + + + , + ); + + // Let the mount effect wire the deferred resolver (GET still in flight). + await act(async () => { + await Promise.resolve(); + }); + expect(get).toHaveBeenCalledTimes(1); + + // A real drag (500 > minOpenWidth) lands before the GET resolves. + act(() => { + fireEvent.click(screen.getByText("Resize")); + }); + expect(screen.getByTestId("width").textContent).toBe("500"); + + await act(async () => { + resolveGet(700); + await Promise.resolve(); + }); + + // The user's live drag is authoritative — the stale GET does not clobber it. + expect(screen.getByTestId("width").textContent).toBe("500"); + }); }); diff --git a/src/context/sidebar/SidebarProvider.tsx b/src/context/sidebar/SidebarProvider.tsx index 362ce8e4c..a148b3fe8 100644 --- a/src/context/sidebar/SidebarProvider.tsx +++ b/src/context/sidebar/SidebarProvider.tsx @@ -47,6 +47,14 @@ export interface SidebarWidthPersistence { export interface SidebarContextType { sidebarWidth: number; setSidebarWidth: (width: number) => void; + /** Set a PLACEHOLDER width that must yield to the persisted width once it is + * fetched. Use for the open-flag-driven seed (AppShell's controlled-open + * effect, which on reload derives a half-viewport / floor value because the + * real width hasn't hydrated yet) — NOT for a user gesture. Unlike + * `setSidebarWidth`, it never marks the width hydrated, so the async-fetched + * stored width overrides it exactly once on first hydrate (first REAL writer + * wins; a placeholder is not a real writer). It also never persists. */ + seedWidth: (width: number) => void; /** Last width the sidebar was opened to — survives close AND reload (when a * persistence surface is wired). Reopen logic should restore this, not the * default, so the user's chosen size sticks. Falls back to `defaultWidth` @@ -134,6 +142,16 @@ export function SidebarProvider({ const persistenceRef = useRef(widthPersistence); persistenceRef.current = widthPersistence; + // True once a REAL width has hydrated the rendered state — either the + // async-fetched stored width landed (apply() below) OR a genuine user drag + // recorded an open width before the fetch resolved. Until then, an open-flag + // PLACEHOLDER seed (AppShell's controlled-open effect, which derives a + // half-viewport / floor width because lastOpenWidth is still 0 mid-restore) + // does NOT count as hydrated, so the fetched stored width may overwrite it + // exactly once. This makes the persisted width authoritative for the hydrate + // window regardless of which writer seeded a non-zero placeholder first. + const widthHydratedRef = useRef(false); + // Rehydrate after mount only — never during render/SSR — so the server pass // and first client render both use `defaultWidth` (no hydration mismatch). // The persisted width applies one tick (or one round-trip) later. The @@ -155,13 +173,28 @@ export function SidebarProvider({ // reopen memory (`lastOpenWidth`). Without this the fetched width is // stranded — it only surfaces on the NEXT open gesture (reopenWidth reads // lastOpenWidth), which is why close-then-reopen restored it but a reload - // did not. The functional updater seeds only from the default floor - // (`cur <= 0`), so a user open gesture (or AppShell's controlled-open - // effect) that landed a real width before this async fetch resolved is - // never clobbered — first writer of a real width wins. No render gate: - // width intentionally paints default-then-hydrate to avoid an SSR - // mismatch. - setSidebarWidth((cur) => (cur <= 0 ? stored : cur)); + // did not. + // + // The first time we apply, the fetched stored width overrides whatever is + // rendered — even a non-zero PLACEHOLDER seed. On reload AppShell's + // controlled-open effect synchronously seeds a half-viewport / floor width + // the instant the host's persisted open:true hydrates (lastOpenWidth is + // still 0 at that point, so reopenWidth() returns a placeholder, not the + // real saved width). That seed lands BEFORE this async GET resolves, so a + // plain `cur <= 0` guard would discard the real stored width. Overriding + // once on first hydrate makes the persisted width win that race. A genuine + // user drag landing before the fetch sets widthHydratedRef in + // handleSetWidth, so it is treated as already-hydrated and is NOT + // clobbered — first REAL writer wins. After the one-time hydrate, the + // functional updater seeds only from the default floor (`cur <= 0`). No + // render gate: width intentionally paints default-then-hydrate to avoid an + // SSR mismatch. + if (!widthHydratedRef.current) { + widthHydratedRef.current = true; + setSidebarWidth(stored); + } else { + setSidebarWidth((cur) => (cur <= 0 ? stored : cur)); + } }; const persistence = persistenceRef.current; @@ -225,6 +258,11 @@ export function SidebarProvider({ // reopen fall back to the default. Only widths strictly above the floor // count as the remembered open width. if (width > minOpenWidth) { + // A real user drag landing before the async fetch resolves is the + // authoritative width — mark hydrated so the late-arriving stored width + // does NOT overwrite it (first REAL writer wins). A placeholder seed at + // or below the floor never reaches here, so it can't claim hydration. + widthHydratedRef.current = true; setLastOpenWidth(width); persist(width); } @@ -232,9 +270,24 @@ export function SidebarProvider({ [minOpenWidth, persist], ); + // A placeholder seed (the open-flag-driven width on reload) paints the + // sidebar open immediately, but it is NOT the user's real width — it must not + // mark the width hydrated, or the async-fetched stored width would be + // discarded (the bug). It also never persists. The fetched width overrides it + // exactly once when the GET lands. Seeding 0 (or the floor) is a no-op for the + // hydration race; only a positive placeholder needs the special handling. + const seedWidth = useCallback((width: number) => { + setSidebarWidth(width); + }, []); + const value = useMemo( - () => ({ sidebarWidth, setSidebarWidth: handleSetWidth, lastOpenWidth }), - [sidebarWidth, handleSetWidth, lastOpenWidth], + () => ({ + sidebarWidth, + setSidebarWidth: handleSetWidth, + seedWidth, + lastOpenWidth, + }), + [sidebarWidth, handleSetWidth, seedWidth, lastOpenWidth], ); return ( diff --git a/src/hooks/agent-chat/useAgentTabs.test.ts b/src/hooks/agent-chat/useAgentTabs.test.ts index 614a33680..df2a46f3d 100644 --- a/src/hooks/agent-chat/useAgentTabs.test.ts +++ b/src/hooks/agent-chat/useAgentTabs.test.ts @@ -323,6 +323,82 @@ describe("useAgentTabs", () => { expect(put).toHaveBeenCalledTimes(1); expect(put).toHaveBeenCalledWith(state({ activeTabId: "c-1" })); }); + + // ---- Bug 2 regression: an interleaving event during the in-flight hydrate + // could strand a fresh placeholder draft over the restored conversation ---- + // + // The first (hydrate) load is authoritative: it must restore the remote tabs + // and active tab even if a host mutation or a focus/visibility refetch races + // it. Old behavior kept drafts (keepDrafts = mode==='refetch' || touchedRef) + // whenever the in-flight hydrate overlapped such an event, so the new-chat + // placeholder won and the user's restored conversation was lost. + + it("a host mutation during the in-flight hydrate does not strand the placeholder draft (Bug 2)", async () => { + // A controllable get() that resolves only when we release it — the hydrate + // round-trip is in flight while the mutation lands. + let release!: (s: AgentSidebarState) => void; + const get = vi.fn( + () => + new Promise((resolve) => { + release = resolve; + }), + ); + const put = vi.fn().mockResolvedValue(undefined); + const { result } = renderHook(() => useAgentTabs({ get, put })); + + // Hydrate GET is in flight (not yet resolved). A route-change / host + // mutation creates and focuses a fresh draft — this sets touchedRef, which + // old code honored as keepDrafts on the still-pending hydrate. + act(() => result.current.newTab()); + const draftId = result.current.activeTabId; + expect(isDraftTab(draftId)).toBe(true); + + // Now the hydrate response lands with the user's restored conversation. + await act(async () => { + release(state({ tabs: tabsOf("c-1", "c-2"), activeTabId: "c-2" })); + await Promise.resolve(); + }); + + // First load is authoritative: remote tabs + active restored, the racing + // placeholder draft did NOT win. (Old behavior: draft kept + still active.) + expect(result.current.tabs).toEqual(["c-1", "c-2"]); + expect(result.current.activeTabId).toBe("c-2"); + expect(result.current.hydrated).toBe(true); + }); + + it("a focus/visibility event during the in-flight hydrate cannot pre-empt it (Bug 2)", async () => { + let release!: (s: AgentSidebarState) => void; + const get = vi.fn( + () => + new Promise((resolve) => { + release = resolve; + }), + ); + const put = vi.fn().mockResolvedValue(undefined); + const { result } = renderHook(() => useAgentTabs({ get, put })); + + // Hydrate GET in flight. A focus + visibilitychange fire before it settles. + // Old code ran reconcile('refetch'), bumping the shared fetchSeq so the + // hydrate GET early-returned (stale seq) — the restore was dropped. The fix + // skips refetches until the first hydrate settles. + act(() => { + window.dispatchEvent(new Event("focus")); + document.dispatchEvent(new Event("visibilitychange")); + }); + // Only the single in-flight hydrate GET exists — no refetch was issued. + expect(get).toHaveBeenCalledTimes(1); + + await act(async () => { + release(state({ tabs: tabsOf("c-1", "c-2"), activeTabId: "c-2" })); + await Promise.resolve(); + }); + + // The hydrate completed and restored the remote strip + active tab. + expect(get).toHaveBeenCalledTimes(1); + expect(result.current.tabs).toEqual(["c-1", "c-2"]); + expect(result.current.activeTabId).toBe("c-2"); + expect(result.current.hydrated).toBe(true); + }); }); describe("deriveTabTitles", () => { diff --git a/src/hooks/agent-chat/useAgentTabs.ts b/src/hooks/agent-chat/useAgentTabs.ts index 6307859be..cd0b4718f 100644 --- a/src/hooks/agent-chat/useAgentTabs.ts +++ b/src/hooks/agent-chat/useAgentTabs.ts @@ -373,6 +373,14 @@ export function useAgentTabs( const pendingRef = useRef(false); // Guards stale GET responses racing a newer fetch. const fetchSeq = useRef(0); + // True once the FIRST successful load (hydrate OR a focus refetch that beat + // the hydrate) has settled. The first load is authoritative: it forces + // keepDrafts=false regardless of mode/touchedRef so an interleaving refetch + // or host mutation (route-change newTab/selectTab, AgentSessionBridge setOpen) + // can't strand a fresh placeholder draft over the restored conversation/active + // tab. Until it flips true, focus/visibility refetches are skipped so they + // can't pre-empt the in-flight hydrate via the shared fetchSeq. + const hasHydratedRef = useRef(false); const schedulePut = useCallback(() => { if (!persistenceRef.current || !enabledRef.current) return; @@ -424,10 +432,20 @@ export function useAgentTabs( if (mode === "refetch" && pendingRef.current) return; const p = persistenceRef.current; if (!p) { - if (mode === "hydrate") setHydrated(true); + if (mode === "hydrate") { + hasHydratedRef.current = true; + setHydrated(true); + } return; } const seq = ++fetchSeq.current; + // The first successful load is authoritative whether it arrives via the + // hydrate effect or a focus refetch that beat it: force keepDrafts=false so + // the restored remote tabs + active tab replace the initial placeholder + // draft outright. An interleaving refetch/mutation (which would otherwise + // set touchedRef or pass mode==='refetch') can no longer keep — and strand — + // a fresh placeholder draft over the user's restored conversation. + const isFirstLoad = !hasHydratedRef.current; try { const remote = await p.get(); if (fetchSeq.current !== seq || pendingRef.current) return; @@ -444,7 +462,12 @@ export function useAgentTabs( if (fetchSeq.current !== seq || pendingRef.current) return; } const prev = stripRef.current; - const next = applyRemote(prev, remote, ids, mode === "refetch" || touchedRef.current); + const next = applyRemote( + prev, + remote, + ids, + !isFirstLoad && (mode === "refetch" || touchedRef.current), + ); // Healed drops stay silent (no write-back) — the next real mutation // PUTs the full healed state anyway. lastPersistedRef.current = toPersisted(next); @@ -455,7 +478,14 @@ export function useAgentTabs( } catch (err) { console.error("agent: sidebar state get failed", err); } finally { - if (mode === "hydrate" && fetchSeq.current === seq) setHydrated(true); + // Mark hydrated whenever a load WINS its seq — not just mode==='hydrate'. + // A focus refetch that completes before the hydrate effect's GET settles + // is the first authoritative load; it must flip the flag (and surface + // `hydrated`) so the next load takes the normal keep-drafts path. + if (fetchSeq.current === seq) { + hasHydratedRef.current = true; + setHydrated(true); + } } }, []); @@ -469,6 +499,12 @@ export function useAgentTabs( useEffect(() => { if (!enabled) return; const onVisible = () => { + // Until the first hydrate settles, a focus/visibility refetch must NOT + // fire: it shares fetchSeq with the in-flight hydrate, so a refetch that + // bumps the seq would make the hydrate GET early-return (stale seq) and a + // fresh placeholder draft could win. Once hydrated, refetches converge + // concurrent hosts as normal. + if (!hasHydratedRef.current) return; if (document.visibilityState === "visible") void reconcile("refetch"); }; window.addEventListener("focus", onVisible);