Skip to content

Commit 1fabc20

Browse files
authored
Fix stale thread route recovery (#170)
* fix(web): recover stale thread routes * fix(web): keep stale routes out of sidebar state * fix(web): keep bootstrap from racing route recovery * test(web): serialize shared event router fixtures * test(web): disable concurrent stable browser cases * fix(web): recover deleted routes from shell events * test(web): serialize stable browser state * test(web): run stateful event case before fixture mutations
1 parent b8854f6 commit 1fabc20

12 files changed

Lines changed: 349 additions & 26 deletions

apps/web/src/components/EventRouter.browser.tsx

Lines changed: 139 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -351,13 +351,16 @@ const worker = setupWorker(
351351
);
352352

353353
async function mountApp(options?: {
354+
initialEntry?: string;
354355
routeThreadId?: ThreadId;
355356
waitForThreadId?: ThreadId | null;
356-
}): Promise<{ cleanup: () => Promise<void> }> {
357+
}): Promise<{ cleanup: () => Promise<void>; router: ReturnType<typeof getRouter> }> {
357358
const host = createFullscreenTestHost();
358359

359360
const routeThreadId = options?.routeThreadId ?? THREAD_ID;
360-
const router = getRouter(createMemoryHistory({ initialEntries: [`/${routeThreadId}`] }));
361+
const router = getRouter(
362+
createMemoryHistory({ initialEntries: [options?.initialEntry ?? `/${routeThreadId}`] }),
363+
);
361364
const screen = await render(<RouterProvider router={router} />, { container: host });
362365

363366
try {
@@ -391,6 +394,7 @@ async function mountApp(options?: {
391394
let cleanedUp = false;
392395

393396
return {
397+
router,
394398
cleanup: async () => {
395399
if (cleanedUp) return;
396400
cleanedUp = true;
@@ -444,7 +448,10 @@ function sendShellEventPush(event: OrchestrationShellStreamItem) {
444448
sendEffectRpcChunk(shellStreamClient, shellStreamRequestId, event);
445449
}
446450

447-
describe("EventRouter scoped orchestration sync", () => {
451+
// This file drives one browser app, WebSocket mock, and projection fixture. Keep
452+
// the cases serialized: several intentionally advance the fixture while their
453+
// route/stream assertions are pending, which is not safe to overlap.
454+
describe.sequential("EventRouter scoped orchestration sync", () => {
448455
beforeAll(async () => {
449456
fixture = buildFixture();
450457
await worker.start({
@@ -522,6 +529,133 @@ describe("EventRouter scoped orchestration sync", () => {
522529
}
523530
});
524531

532+
it("keeps a live assistant intro when a lagging thread snapshot arrives right after it", async () => {
533+
await assertLiveAssistantIntroSurvivesLaggingSnapshot();
534+
});
535+
536+
it("recovers a stale remembered route without opening a missing detail subscription", async () => {
537+
const staleThreadId = ThreadId.makeUnsafe("thread-stale-route");
538+
localStorage.setItem(
539+
"synara:sidebar-ui:v1",
540+
JSON.stringify({
541+
chatSectionExpanded: true,
542+
chatThreadListExtraPages: 2,
543+
projectThreadListExtraPagesByCwd: { "/repo/project": 1 },
544+
dismissedThreadStatusKeyByThreadId: {},
545+
lastThreadRoute: { threadId: staleThreadId },
546+
activityViewEnabled: false,
547+
}),
548+
);
549+
const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
550+
const mounted = await mountApp({ routeThreadId: staleThreadId, waitForThreadId: null });
551+
552+
try {
553+
await vi.waitFor(
554+
() => {
555+
expect(mounted.router.state.location.pathname).not.toBe(`/${staleThreadId}`);
556+
expect(
557+
JSON.parse(localStorage.getItem("synara:sidebar-ui:v1") ?? "null")?.lastThreadRoute,
558+
).toBeNull();
559+
},
560+
{ timeout: 4_000, interval: 16 },
561+
);
562+
await new Promise((resolve) => window.setTimeout(resolve, 120));
563+
expect(subscribeThreadRequestCountById.get(staleThreadId)).toBeUndefined();
564+
expect(consoleError).not.toHaveBeenCalled();
565+
} finally {
566+
consoleError.mockRestore();
567+
await mounted.cleanup();
568+
}
569+
});
570+
571+
it("restores a valid remembered thread from the root route", async () => {
572+
localStorage.setItem(
573+
"synara:sidebar-ui:v1",
574+
JSON.stringify({
575+
lastThreadRoute: { threadId: THREAD_ID },
576+
}),
577+
);
578+
const mounted = await mountApp({ initialEntry: "/" });
579+
580+
try {
581+
await vi.waitFor(
582+
() => {
583+
expect(mounted.router.state.location.pathname).toBe(`/${THREAD_ID}`);
584+
expect(subscribeThreadRequestCountById.get(THREAD_ID)).toBeGreaterThanOrEqual(1);
585+
},
586+
{ timeout: 4_000, interval: 16 },
587+
);
588+
} finally {
589+
await mounted.cleanup();
590+
}
591+
});
592+
593+
it("clears a missing remembered thread while resolving the root route", async () => {
594+
const staleThreadId = ThreadId.makeUnsafe("thread-stale-root-restore");
595+
localStorage.setItem(
596+
"synara:sidebar-ui:v1",
597+
JSON.stringify({
598+
chatSectionExpanded: true,
599+
lastThreadRoute: { threadId: staleThreadId },
600+
}),
601+
);
602+
const mounted = await mountApp({ initialEntry: "/", waitForThreadId: null });
603+
604+
try {
605+
await vi.waitFor(
606+
() => {
607+
expect(
608+
JSON.parse(localStorage.getItem("synara:sidebar-ui:v1") ?? "null")?.lastThreadRoute,
609+
).toBeNull();
610+
expect(mounted.router.state.location.pathname).not.toBe(`/${staleThreadId}`);
611+
},
612+
{ timeout: 4_000, interval: 16 },
613+
);
614+
expect(subscribeThreadRequestCountById.get(staleThreadId)).toBeUndefined();
615+
} finally {
616+
await mounted.cleanup();
617+
}
618+
});
619+
620+
it("stops leasing a deleted route and clears its remembered location after recovery", async () => {
621+
localStorage.setItem(
622+
"synara:sidebar-ui:v1",
623+
JSON.stringify({
624+
lastThreadRoute: { threadId: THREAD_ID },
625+
}),
626+
);
627+
const mounted = await mountApp();
628+
629+
try {
630+
const subscribeCountBeforeDelete = subscribeThreadRequestCountById.get(THREAD_ID) ?? 0;
631+
fixture.snapshot = {
632+
...fixture.snapshot,
633+
snapshotSequence: 2,
634+
threads: [],
635+
};
636+
sendShellEventPush({
637+
kind: "thread-removed",
638+
sequence: 2,
639+
threadId: THREAD_ID,
640+
});
641+
642+
await vi.waitFor(
643+
() => {
644+
expect(mounted.router.state.location.pathname).not.toBe(`/${THREAD_ID}`);
645+
expect(
646+
JSON.parse(localStorage.getItem("synara:sidebar-ui:v1") ?? "null")?.lastThreadRoute,
647+
).toBeNull();
648+
},
649+
{ timeout: 5_000, interval: 16 },
650+
);
651+
await new Promise((resolve) => window.setTimeout(resolve, 120));
652+
expect(subscribeThreadRequestCountById.get(THREAD_ID)).toBe(subscribeCountBeforeDelete);
653+
} finally {
654+
fixture = buildFixture();
655+
await mounted.cleanup();
656+
}
657+
});
658+
525659
it("drops duplicate thread events after the thread snapshot sequence advances", async () => {
526660
const mounted = await mountApp();
527661

@@ -1919,7 +2053,7 @@ describe("EventRouter scoped orchestration sync", () => {
19192053
}
19202054
});
19212055

1922-
it("keeps a live assistant intro when a lagging thread snapshot arrives right after it", async () => {
2056+
async function assertLiveAssistantIntroSurvivesLaggingSnapshot() {
19232057
const mounted = await mountApp();
19242058

19252059
try {
@@ -1997,5 +2131,5 @@ describe("EventRouter scoped orchestration sync", () => {
19972131
fixture = buildFixture();
19982132
await mounted.cleanup();
19992133
}
2000-
});
2134+
}
20012135
});

apps/web/src/components/RestoreOrCreateChatRoute.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,15 @@ export type RestoreRouteResolver = (input: RestoreRouteResolverInput) => LastThr
4040
export function RestoreOrCreateChatRoute({
4141
resolveRestoreRoute,
4242
createFreshChat,
43+
onUnrestorableRememberedRoute,
4344
}: {
4445
// Surface-specific policy for picking the thread route to restore (e.g. the last-visited route
4546
// for home chats, the latest Studio thread or draft for Studio). The remembered-route recovery
4647
// below still keys off the total thread count, which is shared across surfaces.
4748
readonly resolveRestoreRoute: RestoreRouteResolver;
4849
readonly createFreshChat: () => Promise<StartContainerChatResult>;
50+
/** Called only after hydration/recovery has made fallback authoritative. */
51+
readonly onUnrestorableRememberedRoute?: (route: LastThreadRoute) => void;
4952
}) {
5053
const navigate = useNavigate();
5154
const threadsHydrated = useStore((store) => store.threadsHydrated);
@@ -150,6 +153,9 @@ export function RestoreOrCreateChatRoute({
150153
if (cancelled || createFreshChatInFlightRef.current) {
151154
return;
152155
}
156+
if (lastThreadRoute) {
157+
onUnrestorableRememberedRoute?.(lastThreadRoute);
158+
}
153159
createFreshChatInFlightRef.current = true;
154160
// .finally instead of try/finally: React Compiler does not yet support
155161
// try/finally and would skip optimizing this whole component.
@@ -170,6 +176,7 @@ export function RestoreOrCreateChatRoute({
170176
createFreshChat,
171177
emptyRestoreRecoveryState,
172178
navigate,
179+
onUnrestorableRememberedRoute,
173180
resolveRestoreRoute,
174181
splitViewIds,
175182
splitViewsHydrated,

apps/web/src/components/Sidebar.tsx

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1342,6 +1342,7 @@ export default function Sidebar(props: {
13421342
);
13431343
const activeSpaceId = resolveActiveSpaceId(storedActiveSpaceId, spaces, pendingActiveSpaceId);
13441344
const threadsHydrated = useStore((store) => store.threadsHydrated);
1345+
const serverThreadIds = useStore((store) => store.threadIds);
13451346
const sidebarThreadSummaryById = useStore((store) => store.sidebarThreadSummaryById);
13461347
const syncServerShellSnapshot = useStore((store) => store.syncServerShellSnapshot);
13471348
const markThreadVisited = useStore((store) => store.markThreadVisited);
@@ -1656,6 +1657,15 @@ export default function Sidebar(props: {
16561657
const routeActiveSidebarThreadId = routeThreadId;
16571658
const activeSidebarThreadId = optimisticActiveThreadId ?? routeActiveSidebarThreadId;
16581659
const visualActiveSidebarThreadId = optimisticActiveThreadId ?? routeThreadId;
1660+
// The router exposes params before shell hydration has proven that they refer
1661+
// to a thread. Persisting every param lets a stale URL win the recovery race:
1662+
// the route clears its remembered target, then the sidebar writes that same
1663+
// invalid target back on its deferred route-sync pass. Local drafts are valid
1664+
// before their first shell row, so they remain eligible restore targets.
1665+
const routeThreadIsKnown =
1666+
routeThreadId !== null &&
1667+
(serverThreadIds?.includes(routeThreadId) === true ||
1668+
draftThreadsByThreadId[routeThreadId] !== undefined);
16591669
const selectSidebarThreads = useMemo(() => createSidebarThreadSummariesSelector(), []);
16601670
const hideAutomationRunThreads = !appSettings.showAutomationRunThreads;
16611671
const selectSidebarTreeThreads = useMemo(
@@ -4025,7 +4035,7 @@ export default function Sidebar(props: {
40254035
]);
40264036

40274037
useEffect(() => {
4028-
if (isOnSettings || routeThreadId === null) {
4038+
if (isOnSettings || routeThreadId === null || !routeThreadIsKnown) {
40294039
return;
40304040
}
40314041

@@ -4045,7 +4055,7 @@ export default function Sidebar(props: {
40454055
});
40464056
}, 0);
40474057
return () => window.clearTimeout(settle);
4048-
}, [isOnSettings, routeSearch.splitViewId, routeThreadId]);
4058+
}, [isOnSettings, routeSearch.splitViewId, routeThreadId, routeThreadIsKnown]);
40494059

40504060
const handleThreadClick = useCallback(
40514061
(event: MouseEvent, threadId: ThreadId, orderedProjectThreadIds: readonly ThreadId[]) => {

apps/web/src/components/Sidebar.uiState.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
import { afterEach, beforeEach, describe, expect, it } from "vitest";
22

33
import {
4+
clearLastThreadRouteIfMatches,
45
normalizeSidebarProjectThreadListCwd,
56
persistSidebarUiState,
67
readSidebarUiState,
8+
subscribeSidebarUiState,
79
} from "./Sidebar.uiState";
810

911
describe("Sidebar.uiState", () => {
@@ -14,6 +16,8 @@ describe("Sidebar.uiState", () => {
1416
Object.defineProperty(globalThis, "window", {
1517
configurable: true,
1618
value: {
19+
addEventListener: () => {},
20+
removeEventListener: () => {},
1721
localStorage: {
1822
clear: () => {
1923
storage.clear();
@@ -163,4 +167,48 @@ describe("Sidebar.uiState", () => {
163167
activityViewEnabled: false,
164168
});
165169
});
170+
171+
it("atomically clears only a matching remembered route and preserves unrelated state", () => {
172+
persistSidebarUiState({
173+
chatSectionExpanded: true,
174+
chatThreadListExtraPages: 3,
175+
projectThreadListExtraPagesByCwd: { "/repo": 2 },
176+
dismissedThreadStatusKeyByThreadId: { "thread-other": "ready:turn-1" },
177+
lastThreadRoute: { threadId: "thread-stale", splitViewId: "split-stale" },
178+
activityViewEnabled: true,
179+
});
180+
181+
expect(clearLastThreadRouteIfMatches("thread-other")).toBe(false);
182+
expect(readSidebarUiState().lastThreadRoute?.threadId).toBe("thread-stale");
183+
expect(clearLastThreadRouteIfMatches("thread-stale")).toBe(true);
184+
expect(readSidebarUiState()).toEqual({
185+
chatSectionExpanded: true,
186+
chatThreadListExtraPages: 3,
187+
projectThreadListExtraPagesByCwd: {
188+
[normalizeSidebarProjectThreadListCwd("/repo")]: 2,
189+
},
190+
dismissedThreadStatusKeyByThreadId: { "thread-other": "ready:turn-1" },
191+
lastThreadRoute: null,
192+
activityViewEnabled: true,
193+
});
194+
});
195+
196+
it("notifies same-window sidebar consumers after a matching clear", () => {
197+
persistSidebarUiState({
198+
chatSectionExpanded: false,
199+
chatThreadListExtraPages: 0,
200+
projectThreadListExtraPagesByCwd: {},
201+
dismissedThreadStatusKeyByThreadId: {},
202+
lastThreadRoute: { threadId: "thread-stale" },
203+
activityViewEnabled: false,
204+
});
205+
const observed: Array<ReturnType<typeof readSidebarUiState>> = [];
206+
const unsubscribe = subscribeSidebarUiState((state) => observed.push(state));
207+
208+
expect(clearLastThreadRouteIfMatches("thread-stale")).toBe(true);
209+
expect(observed).toHaveLength(1);
210+
expect(observed[0]?.lastThreadRoute).toBeNull();
211+
212+
unsubscribe();
213+
});
166214
});

apps/web/src/components/Sidebar.uiState.ts

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ export type SidebarUiState = {
1818
activityViewEnabled: boolean;
1919
};
2020

21+
const sameWindowListeners = new Set<(state: SidebarUiState) => void>();
22+
2123
const DEFAULT_SIDEBAR_UI_STATE: SidebarUiState = {
2224
chatSectionExpanded: false,
2325
chatThreadListExtraPages: 0,
@@ -153,13 +155,17 @@ export function subscribeSidebarUiState(listener: (state: SidebarUiState) => voi
153155
if (event.key !== SIDEBAR_UI_STATE_STORAGE_KEY) return;
154156
listener(readSidebarUiState());
155157
};
158+
sameWindowListeners.add(listener);
156159
window.addEventListener("storage", handleStorage);
157-
return () => window.removeEventListener("storage", handleStorage);
160+
return () => {
161+
sameWindowListeners.delete(listener);
162+
window.removeEventListener("storage", handleStorage);
163+
};
158164
}
159165

160-
export function persistSidebarUiState(input: SidebarUiState): void {
166+
function writeSidebarUiState(input: SidebarUiState): boolean {
161167
if (typeof window === "undefined") {
162-
return;
168+
return false;
163169
}
164170

165171
try {
@@ -187,7 +193,34 @@ export function persistSidebarUiState(input: SidebarUiState): void {
187193
activityViewEnabled: input.activityViewEnabled,
188194
}),
189195
);
196+
return true;
190197
} catch {
191198
// Ignore storage errors so sidebar rendering keeps working when persistence is unavailable.
199+
return false;
200+
}
201+
}
202+
203+
export function persistSidebarUiState(input: SidebarUiState): void {
204+
writeSidebarUiState(input);
205+
}
206+
207+
/**
208+
* Atomically clears a stale remembered route without overwriting unrelated sidebar state.
209+
* Storage events do not fire in the window that performed the write, so same-window consumers
210+
* receive an explicit notification after the persisted compare-and-clear succeeds.
211+
*/
212+
export function clearLastThreadRouteIfMatches(threadId: string): boolean {
213+
const current = readSidebarUiState();
214+
if (current.lastThreadRoute?.threadId !== threadId) {
215+
return false;
216+
}
217+
218+
const next: SidebarUiState = { ...current, lastThreadRoute: null };
219+
if (!writeSidebarUiState(next)) {
220+
return false;
221+
}
222+
for (const listener of sameWindowListeners) {
223+
listener(next);
192224
}
225+
return true;
193226
}

0 commit comments

Comments
 (0)