[Feat]: Sync URL with active thread (issue #27)#34
Conversation
Bypass next/navigation for thread navigation: router.push re-fires the dynamic route's RSC, which double-fires useLangGraphRuntime's mount-only load() plus the adapter fetch() for the same id (issue #27's "previous attempt looped"). Instead: - useLangGraphRuntime's onThreadIdChange pushes the URL via window.history.pushState — no RSC roundtrip. - ThreadUrlShadow listens for popstate so browser back/forward hydrates the active thread from window.location.pathname. - The new app/chat/[threadId]/page.tsx renders <Assistant /> without a threadId prop so a re-render never carries the thread id back through React props — that flow was the trigger for the double-load. Verified end-to-end via Chrome DevTools MCP: click existing thread -> 1x state?subgraphs=true (no RSC, no duplicate fetch); click +New -> URL goes to /chat; reload on /chat/<id> -> no empty-thread flash. See issue #27 for the full before/after.
PR review — focus on correctness, race conditions, and CLAUDE.md complianceOverall the CLAUDE.md rule violations
Correctness — race conditions and history pollution
Error handling
Minor
Nit: comment density (rule 5)Most "ponytail:" headers are legit "why" comments documenting the non-obvious workaround for the double-load, which rule 5 endorses. A couple cross the line:
A |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const readUrlThreadId = (): string | null => { | ||
| if (typeof window === "undefined") return null; | ||
| const m = window.location.pathname.match(/^\/chat\/([^/]+)$/); | ||
| return m ? decodeURIComponent(m[1]) : null; |
There was a problem hiding this comment.
decodeURIComponent will throw URIError: URI malformed on inputs like /chat/%E0%A4%A (incomplete percent sequence). The regex /^\/chat\/([^/]+)$/ happily matches such paths because % is one character and there's no /. The synchronous throw escapes the effect, and on the next mount the listener could fail to register until the malformed URL is replaced. Wrap the decode in try/catch and fall back to raw m[1] or to null.
| return m ? decodeURIComponent(m[1]) : null; | |
| let m: RegExpExecArray | null; | |
| if (typeof window === "undefined" || !(m = /^\/chat\/([^/]+)$/.exec(window.location.pathname))) { | |
| return null; | |
| } | |
| try { | |
| return decodeURIComponent(m[1]); | |
| } catch { | |
| return null; | |
| } |
| // here so the bar reflects "new chat" instantly; once the backend | ||
| // thread materializes, `onThreadIdChange` → writeUrlForThread pushes | ||
| // the real `/chat/<uuid>`. | ||
| useEffect(() => { |
There was a problem hiding this comment.
This is missing the hasHydratedRef guard that the prior ThreadPersistence had for the analogous effect (see the deleted block in the diff). On initial mount at /chat/<X>, before switchToThread(urlThreadId) has resolved, activeExternalId is undefined (or the __LOCALID_* placeholder); the first two conditions don't early-return, and since pathname !== /chat the effect fires pushState("/chat"). writeUrlForThread then re-pushes /chat/<X> once the runtime settles, polluting the history stack to [..., /chat/<X>, /chat, /chat/<X>] for one logical reload. Gate the first commit with didHydrateRef (already declared on line 363) and only push on later transitions. Also note: the // writeUrlForThread handled it comment on line 439 restates the code — rule 5 says default to no comment.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Read thread id off the current path. /chat/<id> → id. /chat (or | ||
| // anything else) → null. We use window.location (NOT useParams/usePathname) | ||
| // because Next.js's router cache doesn't observe our pushState writes. | ||
| const readUrlThreadId = (): string | null => { | ||
| if (typeof window === "undefined") return null; | ||
| const m = window.location.pathname.match(/^\/chat\/([^/]+)$/); | ||
| return m ? decodeURIComponent(m[1]) : null; | ||
| }; |
There was a problem hiding this comment.
The first line of the readUrlThreadId comment ("Read thread id off the current path. /chat/ → id. /chat (or anything else) → null.") restates what the regex immediately below makes obvious. Per CLAUDE.md Rule 5, comments should explain why, not narrate what — this sentence should be dropped and only the reasoning about window.location vs useParams retained. The same pattern appears on line 374 ("URL → aUI: initial mount + browser back/forward.") and line 416 ("localStorage write-through so a stale tab still re-opens correctly.").
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/assistant.tsx
Line: 365-372
Comment:
**"What" comments violate Rule 5**
The first line of the `readUrlThreadId` comment ("Read thread id off the current path. /chat/<id> → id. /chat (or anything else) → null.") restates what the regex immediately below makes obvious. Per CLAUDE.md Rule 5, comments should explain *why*, not narrate *what* — this sentence should be dropped and only the reasoning about `window.location` vs `useParams` retained. The same pattern appears on line 374 ("URL → aUI: initial mount + browser back/forward.") and line 416 ("localStorage write-through so a stale tab still re-opens correctly.").
**Context Used:** CLAUDE.md ([source](https://app.greptile.com/firetable/github/FireTable/langgraph-app/-/custom-context?memory=629c10ea-a5a4-4d67-ba55-56060d5b9284))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| return null; | ||
| }; | ||
|
|
||
| // Persists the active thread id to localStorage so the chat reopens on the | ||
| // same thread after a refresh. We write the runtime's externalId (our | ||
| // Postgres uuid) rather than mainThreadId, because the runtime keeps | ||
| // _mainThreadId on the placeholder __LOCALID_* until the user | ||
| // explicitly switches threads — see assistant-ui #2577 and PR #3855 | ||
| // for the related ExternalStore fix; RemoteThreadList is still affected. | ||
| const ThreadPersistence: FC = () => { | ||
| // ponytail: the URL is a shadow of the active aUI thread (issue #27). | ||
| // We deliberately bypass `next/router` for thread nav: `router.push` on | ||
| // this page re-fires the dynamic route's RSC, which double-fires `load()` | ||
| // and the adapter `fetch()` — aUI's runtime assumes ONE mount per active | ||
| // thread, so the second call hits the network again. Instead we: | ||
| // - push URL via `history.pushState` (URL bar updates; no RSC roundtrip) | ||
| // - hook the runtime's `onThreadIdChange` (passed in `useLangGraphRuntime` | ||
| // below) so user-driven thread switches write the URL automatically. | ||
| // - listen to `popstate` so browser back/forward syncs aUI without a | ||
| // Next.js route change. | ||
| // `lastSyncedIdRef` prevents the popstate → switchToThread → onThreadIdChange | ||
| // echo from re-pushing. | ||
| const ThreadUrlShadow: FC = () => { | ||
| const api = useAui(); | ||
| const mainThreadId = useAuiState((s) => s.threads.mainThreadId); | ||
| const activeExternalId = useAuiState( | ||
| (s) => s.threads.threadItems.find((t) => t.id === s.threads.mainThreadId)?.externalId, | ||
| ); | ||
| const hasHydratedRef = useRef(false); | ||
| const lastSyncedIdRef = useRef<string | null>(null); | ||
| const didHydrateRef = useRef(false); | ||
|
|
||
| // Runs before the write effect on first commit so hasHydratedRef | ||
| // suppresses the placeholder write below. | ||
| // Read thread id off the current path. /chat/<id> → id. /chat (or | ||
| // anything else) → null. We use window.location (NOT useParams/usePathname) | ||
| // because Next.js's router cache doesn't observe our pushState writes. | ||
| const readUrlThreadId = (): string | null => { | ||
| if (typeof window === "undefined") return null; | ||
| const m = window.location.pathname.match(/^\/chat\/([^/]+)$/); | ||
| return m ? decodeURIComponent(m[1]) : null; | ||
| }; | ||
|
|
||
| // URL → aUI: initial mount + browser back/forward. After our pushState | ||
| // popstate does NOT fire — that's the whole point of bypassing the router. | ||
| useEffect(() => { | ||
| const savedId = localStorage.getItem(ACTIVE_THREAD_ID); | ||
| if (savedId) { | ||
| // switchToThread is typed void but returns a Promise at runtime. | ||
| void Promise.resolve(api.threads().switchToThread(savedId) as unknown as Promise<void>).catch( | ||
| () => { | ||
| localStorage.removeItem(ACTIVE_THREAD_ID); | ||
| }, | ||
| ); | ||
| const syncFromUrl = () => { | ||
| const urlThreadId = readUrlThreadId(); | ||
| const currentExternalId = typeof activeExternalId === "string" ? activeExternalId : null; | ||
| if ( | ||
| urlThreadId && | ||
| urlThreadId !== currentExternalId && | ||
| urlThreadId !== lastSyncedIdRef.current | ||
| ) { | ||
| // switchToThread is typed void but returns a Promise at runtime. | ||
| void Promise.resolve( | ||
| api.threads().switchToThread(urlThreadId) as unknown as Promise<void>, | ||
| ).catch(() => {}); | ||
| } else if (urlThreadId) { | ||
| lastSyncedIdRef.current = urlThreadId; | ||
| } | ||
| }; | ||
| if (!didHydrateRef.current) { | ||
| didHydrateRef.current = true; | ||
| const urlThreadId = readUrlThreadId(); | ||
| if (!urlThreadId) { | ||
| // /chat root: seed from localStorage so a stale tab re-opens on | ||
| // its last thread. The seeded thread will then trigger | ||
| // onThreadIdChange → pushState. | ||
| const savedId = localStorage.getItem(ACTIVE_THREAD_ID); | ||
| if (savedId) { | ||
| void Promise.resolve( | ||
| api.threads().switchToThread(savedId) as unknown as Promise<void>, | ||
| ).catch(() => { | ||
| localStorage.removeItem(ACTIVE_THREAD_ID); | ||
| }); | ||
| } | ||
| } | ||
| syncFromUrl(); | ||
| } | ||
| hasHydratedRef.current = true; | ||
| window.addEventListener("popstate", syncFromUrl); | ||
| return () => window.removeEventListener("popstate", syncFromUrl); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
|
|
||
| // localStorage write-through so a stale tab still re-opens correctly. | ||
| // mainThreadId is also tracked (placeholder phase) — see #27 history | ||
| // on the placeholder id fallback (assistant-ui #2577, PR #3855). | ||
| useEffect(() => { | ||
| if (!hasHydratedRef.current) return; | ||
| const id = activeExternalId ?? mainThreadId; | ||
| if (!id || id.startsWith(LOCAL_THREAD_PREFIX)) { | ||
| if (!id || (typeof id === "string" && id.startsWith(LOCAL_THREAD_PREFIX))) { | ||
| localStorage.removeItem(ACTIVE_THREAD_ID); | ||
| return; | ||
| } | ||
| localStorage.setItem(ACTIVE_THREAD_ID, id); | ||
| }, [activeExternalId, mainThreadId]); | ||
|
|
||
| // ponytail: parallel URL-write for the NEW-thread placeholder case. | ||
| // `onThreadIdChange` only fires on `_mainThreadRemoteId` transitions, | ||
| // which stays `undefined` for `__LOCALID_*` until the first message | ||
| // hits the server and `adapter.initialize()` resolves. So when the | ||
| // user clicks +New and `activeExternalId` clears, the canonical hook | ||
| // is silent and the URL stays on the previous thread. We push /chat | ||
| // here so the bar reflects "new chat" instantly; once the backend | ||
| // thread materializes, `onThreadIdChange` → writeUrlForThread pushes | ||
| // the real `/chat/<uuid>`. | ||
| useEffect(() => { | ||
| if (typeof window === "undefined") return; | ||
| if (activeExternalId) return; // writeUrlForThread handled it | ||
| if (window.location.pathname === "/chat") return; | ||
| window.history.pushState({ threadId: null }, "", "/chat"); | ||
| }, [activeExternalId]); | ||
|
|
||
| return null; | ||
| }; |
There was a problem hiding this comment.
ThreadUrlShadow URL-state logic (Rule 2)
ThreadUrlShadow contains meaningful branching: popstate → switchToThread, localStorage seed vs URL seed, new-thread placeholder URL push, and the lastSyncedIdRef dedup path. CLAUDE.md Rule 2 requires a failing test first for non-declarative code. None is included in this PR. The component can be tested with @testing-library/react + jsdom's window.history and a mocked useAui/useAuiState, covering at minimum: initial URL seed, browser-back nav, and the +New /chat redirect.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/assistant.tsx
Line: 341-445
Comment:
**No tests for `ThreadUrlShadow` URL-state logic (Rule 2)**
`ThreadUrlShadow` contains meaningful branching: popstate → `switchToThread`, localStorage seed vs URL seed, new-thread placeholder URL push, and the `lastSyncedIdRef` dedup path. CLAUDE.md Rule 2 requires a failing test first for non-declarative code. None is included in this PR. The component can be tested with `@testing-library/react` + `jsdom`'s `window.history` and a mocked `useAui`/`useAuiState`, covering at minimum: initial URL seed, browser-back nav, and the `+New` `/chat` redirect.
**Context Used:** CLAUDE.md ([source](https://app.greptile.com/firetable/github/FireTable/langgraph-app/-/custom-context?memory=629c10ea-a5a4-4d67-ba55-56060d5b9284))
How can I resolve this? If you propose a fix, please make it concise.
| // switchToThread is typed void but returns a Promise at runtime. | ||
| void Promise.resolve( | ||
| api.threads().switchToThread(urlThreadId) as unknown as Promise<void>, | ||
| ).catch(() => {}); |
There was a problem hiding this comment.
Silent .catch(() => {}) is too quiet for switchToThread(urlThreadId) failures — this is the path users land on when they share /chat/<other-user-thread-id> (cross-user 404), follow a deleted thread, or hit a network blip. The catch on line 405 cleans up localStorage, but the URL bar keeps the bad id forever, the sidebar shows "New Chat", and the user has no signal anything is wrong. At minimum, redirect to /chat on catch (mirroring what the third effect does for activeExternalId === null) and consider a toast for deleted/inaccessible threads. As written, an attacker who learns any uuid format can leave /chat/ URLs lying around that look like they ought to work.
| // localStorage write-through so a stale tab still re-opens correctly. | ||
| // mainThreadId is also tracked (placeholder phase) — see #27 history | ||
| // on the placeholder id fallback (assistant-ui #2577, PR #3855). | ||
| useEffect(() => { |
There was a problem hiding this comment.
This effect lost the hasHydratedRef early-return that the old ThreadPersistence had on lines 335–338 of the previous version. On initial mount at /chat/<X>, the first commit sees activeExternalId === undefined, mainThreadId = __LOCALID_* — id = "__LOCALID_*", hits the LOCAL_THREAD_PREFIX branch, and removeItem(ACTIVE_THREAD_ID). The first effect (line 376) kicks off switchToThread(urlThreadId) asynchronously, but localStorage is already empty for the duration of that round-trip. If the user closes the tab during the gap, their saved thread id is lost. The original hasHydratedRef was there for this exact reason — please re-add it (or a didHydrateRef) here too.
| // this `params.threadId` prop — passing it down would trip a runtime | ||
| // remount on every route push (issue #27 history: that double-fires | ||
| // `load()` and the adapter `fetch()`). | ||
| export default async function ChatThreadPage() { |
There was a problem hiding this comment.
This page is byte-for-byte equivalent to app/chat/page.tsx (modulo the doc comment). Both: import the same auth helper, redirect on missing session, render <Assistant />. Two ways to consolidate:
-
Pull the auth check into a shared helper (
lib/auth/require-session-or-redirect.ts) so the auth contract lives in one place. Cheapest fix, smallest diff. Bonus: any future change (e.g., add a?next=param to the/loginredirect, surface a specific error code) only lands once. -
Drop
/chat/page.tsxand turn[threadId]into an optional catch-all ([[...threadId]]/page.tsx). The client-sideThreadUrlShadowalready handles the no-id case (/chat↔ "new chat"), so the server doesn't actually need a non-dynamic sibling. This also lets the route file own the doc comment about the dynamic-route workaround, instead of duplicating it.
Either fix prevents the "I changed the redirect URL on one page and forgot the other" failure mode.
…now (#27) localStorage ACTIVE_THREAD_ID was a write-through mirror that hydrated `/chat` (no thread id) on next load — useful before the URL became the source of truth, redundant once it is. What changes for users: - /chat (root) used to seed from a persisted uuid when one existed; now the runtime constructor's own switchToNewThread() runs and the user lands on a fresh placeholder (welcome view + suggestions). - /chat/<id> behavior unchanged. The dynamic page still hydrates from window.location.pathname; reload / share-link / back-forward land on the right thread. Removal: - ThreadUrlShadow: the `didHydrateRef` block + the write-through effect (also dropped the `mainThreadId` useAuiState, no longer needed). The mount useEffect collapses to "listen for popstate, also re-sync on every render" — URL is the only state. - lib/constants.ts: ACTIVE_THREAD_ID gone; LOCAL_THREAD_PREFIX is kept (still defined for any future placeholder-id filter, but no current consumer in assistant.tsx — the observability components use their own `__LOCAL_` prefix and that bit-rot is out of scope for this PR). Net diff: -29 lines across 2 files. No behavior change for /chat/<id> flow; /chat root changes from "resume last" to "fresh chat" (matches mainstream chat apps).
| urlThreadId && | ||
| urlThreadId !== currentExternalId && | ||
| urlThreadId !== lastSyncedIdRef.current | ||
| ) { |
There was a problem hiding this comment.
lastSyncedIdRef.current is set in the wrong branch — it is updated only when switchToThread is not called (the else if path), so it never guards against the case it is documented to prevent. If syncFromUrl fires twice rapidly for the same urlThreadId (e.g. two quick Back presses before the first switchToThread promise settles), the condition is re-evaluated with lastSyncedIdRef.current still null, and switchToThread is invoked a second time — a double load() + double adapter.fetch(), which is the exact regression this PR exists to prevent.
Move the assignment to the call site so the ref is written before the switch:
| ) { | |
| if ( | |
| urlThreadId && | |
| urlThreadId !== currentExternalId && | |
| urlThreadId !== lastSyncedIdRef.current | |
| ) { | |
| lastSyncedIdRef.current = urlThreadId; | |
| // switchToThread is typed void but returns a Promise at runtime. | |
| void Promise.resolve( | |
| api.threads().switchToThread(urlThreadId) as unknown as Promise<void>, | |
| ).catch(() => {}); | |
| } else if (urlThreadId) { | |
| lastSyncedIdRef.current = urlThreadId; | |
| } |
| // here so the bar reflects "new chat" instantly; once the backend | ||
| // thread materializes, `onThreadIdChange` → writeUrlForThread pushes | ||
| // the real `/chat/<uuid>`. | ||
| useEffect(() => { |
There was a problem hiding this comment.
Missing hydration guard — this effect was designed for the +New transition (placeholder after a real id), not the initial-mount case. On direct navigation to /chat/<X>, effect order means syncFromUrl runs first (correctly calls switchToThread(X)), but then this effect fires with activeExternalId === undefined and pathname === "/chat/<X>" (not /chat), so it unconditionally pushes /chat. Once the runtime resolves switchToThread, onThreadIdChange → writeUrlForThread(X) pushes /chat/<X> again.
Resulting history stack on a single reload: [..., /chat/<X>, /chat, /chat/<X>] — three entries for one logical reload, plus a brief paint of /chat in the URL bar. A user pressing Back from the final /chat/<X> lands on /chat, reads as a "new chat" route, but the runtime is still on thread X. syncFromUrl won't switchToThread because urlThreadId is null. URL stays at /chat until the next action. Visible bug.
Fix: gate on a didHydrateRef (set true after the first syncFromUrl settles), or fold this into syncFromUrl so the placeholder push only fires when the previous external id was a real one — not the initial undefined.
| const readUrlThreadId = (): string | null => { | ||
| if (typeof window === "undefined") return null; | ||
| const m = window.location.pathname.match(/^\/chat\/([^/]+)$/); | ||
| return m ? decodeURIComponent(m[1]) : null; |
There was a problem hiding this comment.
decodeURIComponent throws URIError: URI malformed on incomplete percent sequences. A path like /chat/%E0%A4%A matches the regex but blows up here, escaping the effect synchronously and aborting addEventListener("popstate", ...) — back/forward then silently stops syncing for the lifetime of the mount.
Wrap in try/catch and fall back to raw m[1] (or null) on failure. The same throw propagates from popstate, so a malformed URL encountered via Back corrupts the listener permanently.
| const hasHydratedRef = useRef(false); | ||
| const lastSyncedIdRef = useRef<string | null>(null); | ||
|
|
||
| // Read thread id off the current path. /chat/<id> → id. /chat (or |
There was a problem hiding this comment.
Rule 5: the first sentence ("Read thread id off the current path. /chat/ → id. /chat (or anything else) → null.") restates what the regex immediately below makes obvious. Drop it; keep only the window.location vs useParams/usePathname rationale (the actual why — Next.js's router cache doesn't observe our pushState writes, so a hook reading from the router would be stale).
| localStorage.setItem(ACTIVE_THREAD_ID, id); | ||
| }, [activeExternalId, mainThreadId]); | ||
| if (typeof window === "undefined") return; | ||
| if (activeExternalId) return; // writeUrlForThread handled it |
There was a problem hiding this comment.
Rule 5: // writeUrlForThread handled it restates the next line (if (activeExternalId) return;). Either drop it or replace with the actual why — e.g. that activeExternalId becoming truthy means the canonical onThreadIdChange → writeUrlForThread path will fire with the real id, so this fallback effect should stay quiet to avoid a redundant push.
| @@ -0,0 +1,15 @@ | |||
| import { redirect } from "next/navigation"; | |||
There was a problem hiding this comment.
Rule 2 (TDD for new code): no test for this route. Minimum coverage: an authenticated GET returns <Assistant />; an unauthenticated GET redirects to /login; the rendered tree does not pass params.threadId down as a prop (regression guard for the RSC-double-load from issue #27). The tests/api/auth mock pattern (next/headers + @/lib/auth/config) is the template — see tests/api/alchemy/status.test.ts.
|
Independent follow-up. Adds concrete bugs not surfaced by inline comments yet. Echoes / extends the existing top-level review. Concrete bugs to fix1. lastSyncedIdRef is updated in the wrong branchsyncFromUrl sets lastSyncedIdRef.current only on the else-if branch (the URL-already-matches-aUI case). The branch that DOES call switchToThread leaves the ref at its previous value. The stated contract (prevents the popstate -> switchToThread -> onThreadIdChange echo from re-pushing) is the one place the ref is needed most, and it is the one place it is not being written. Trigger: rapid Back presses before the first switchToThread settles. syncFromUrl fires twice for the same urlThreadId, the condition re-evaluates with lastSyncedIdRef.current still null, and switchToThread is invoked twice. That is a double load() + double adapter.fetch(), the exact regression this PR exists to prevent. 2. Placeholder effect missing hydration guard - history pollution on reloadEffect order on mount at /chat/X: the syncFromUrl useEffect runs first, calls switchToThread(X). Then the placeholder useEffect fires with activeExternalId = undefined and pathname = /chat/X (not /chat), so it unconditionally pushes /chat. Once switchToThread resolves, onThreadIdChange -> writeUrlForThread(X) pushes /chat/X again. Resulting history stack on a reload: [..., /chat/X, /chat, /chat/X] - three entries for one logical reload, and the URL bar briefly paints /chat. A user pressing Back from the final /chat/X lands on /chat, reads as a "new chat" route, but the runtime is still on thread X. syncFromUrl will not switchToThread because urlThreadId is null. URL stays at /chat until the next action. Visible bug. 3. decodeURIComponent throws on malformed percent-encodingA path like /chat/%E0%A4%A matches the regex but decodeURIComponent throws URIError: URI malformed. The throw escapes the effect synchronously and aborts addEventListener("popstate", ...), so back/forward silently stops syncing for |
|
Continuation of the top-level review above. 3. decodeURIComponent throws on malformed percent-encodingA path like /chat/%E0%A4%A matches the regex but decodeURIComponent throws URIError: URI malformed. The throw escapes the effect synchronously and aborts addEventListener("popstate", ...), so back/forward silently stops syncing for the lifetime of the mount. Wrap in try/catch and fall back to raw m[1] (or null) on failure. Correction to the prior top-level review"The localStorage write effect lost its hydration guard" - there is no localStorage write effect in this diff. ThreadPersistence's second effect was deleted along with the component. The first effect's localStorage READ was also deleted. The hydration concern translates to "the placeholder effect (item 2 above) needs a hydration guard" - same shape, different code path. Rule 2 (tests)No coverage for ~115 lines of new client state. Suggested cases (templates: tests/api/threads.test.ts, tests/threads/adapter.test.ts):
A short history-stack assertion on reload would catch item 2 without needing full jsdom wiring for assistant-ui. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/constants.ts">
<violation number="1" location="lib/constants.ts:5">
P3: The historical localStorage note adds stale implementation detail after localStorage was removed and obscures the useful reason for rejecting placeholder IDs. Keeping the comment focused on the missing server record would match the repository’s comment guideline.</violation>
</file>
<file name="app/assistant.tsx">
<violation number="1" location="app/assistant.tsx:396">
P2: Direct development loads of `/chat/<id>` now call `switchToThread` twice because Strict Mode replays this unguarded effect while `lastSyncedIdRef` remains unset in the switching branch. Recording `urlThreadId` immediately before `switchToThread` (or retaining a one-time hydration guard) would prevent the duplicate load and race.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| lastSyncedIdRef.current = urlThreadId; | ||
| } | ||
| }; | ||
| syncFromUrl(); |
There was a problem hiding this comment.
P2: Direct development loads of /chat/<id> now call switchToThread twice because Strict Mode replays this unguarded effect while lastSyncedIdRef remains unset in the switching branch. Recording urlThreadId immediately before switchToThread (or retaining a one-time hydration guard) would prevent the duplicate load and race.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/assistant.tsx, line 396:
<comment>Direct development loads of `/chat/<id>` now call `switchToThread` twice because Strict Mode replays this unguarded effect while `lastSyncedIdRef` remains unset in the switching branch. Recording `urlThreadId` immediately before `switchToThread` (or retaining a one-time hydration guard) would prevent the duplicate load and race.</comment>
<file context>
@@ -390,41 +393,12 @@ const ThreadUrlShadow: FC = () => {
- }
- syncFromUrl();
- }
+ syncFromUrl();
window.addEventListener("popstate", syncFromUrl);
return () => window.removeEventListener("popstate", syncFromUrl);
</file context>
| // load. (Used to gate localStorage; the localStorage write itself was | ||
| // removed in issue #27 once URL became the source of truth.) |
There was a problem hiding this comment.
P3: The historical localStorage note adds stale implementation detail after localStorage was removed and obscures the useful reason for rejecting placeholder IDs. Keeping the comment focused on the missing server record would match the repository’s comment guideline.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/constants.ts, line 5:
<comment>The historical localStorage note adds stale implementation detail after localStorage was removed and obscures the useful reason for rejecting placeholder IDs. Keeping the comment focused on the missing server record would match the repository’s comment guideline.</comment>
<file context>
@@ -1,14 +1,11 @@
+// hasn't been persisted yet. Filter it out before writing to anything
+// (URL, telemetry, etc.) — the placeholder has no backing record on the
+// server, so anything that points back at it would 404 on the next page
+// load. (Used to gate localStorage; the localStorage write itself was
+// removed in issue #27 once URL became the source of truth.)
export const LOCAL_THREAD_PREFIX = "__LOCALID_";
</file context>
| // load. (Used to gate localStorage; the localStorage write itself was | |
| // removed in issue #27 once URL became the source of truth.) | |
| // load. |
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/chat/[threadId]/page.tsx">
<violation number="1" location="app/chat/[threadId]/page.tsx:1">
P3: This page duplicates the complete authenticated chat-page implementation, so future auth or rendering changes can diverge between `/chat` and `/chat/<threadId>`. Re-exporting or sharing the existing page keeps both routes on one implementation.</violation>
<violation number="2" location="app/chat/[threadId]/page.tsx:13">
P2: Logged-out users opening `/chat/<threadId>` lose the deep link and land on `/chat` after sign-in. Preserve the requested chat path through the authentication flow and redirect back to it.</violation>
<violation number="3" location="app/chat/[threadId]/page.tsx:14">
P2: A hard reload can still paint the New Chat state before the URL thread is selected after hydration. Gate the visible thread UI until the initial URL sync completes, or seed the runtime before the first render.</violation>
</file>
<file name="app/assistant.tsx">
<violation number="1" location="app/assistant.tsx:371">
P2: A malformed encoded thread id can throw and break URL sync logic. Wrapping decode in a safe parse path keeps navigation handling resilient to bad URLs.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| const readUrlThreadId = (): string | null => { | ||
| if (typeof window === "undefined") return null; | ||
| const m = window.location.pathname.match(/^\/chat\/([^/]+)$/); | ||
| return m ? decodeURIComponent(m[1]) : null; |
There was a problem hiding this comment.
P2: A malformed encoded thread id can throw and break URL sync logic. Wrapping decode in a safe parse path keeps navigation handling resilient to bad URLs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/assistant.tsx, line 371:
<comment>A malformed encoded thread id can throw and break URL sync logic. Wrapping decode in a safe parse path keeps navigation handling resilient to bad URLs.</comment>
<file context>
@@ -335,45 +341,117 @@ const AuiRefCapture: FC<{ bridgeRef: RefObject<RuntimeBridge> }> = ({ bridgeRef
+ const readUrlThreadId = (): string | null => {
+ if (typeof window === "undefined") return null;
+ const m = window.location.pathname.match(/^\/chat\/([^/]+)$/);
+ return m ? decodeURIComponent(m[1]) : null;
+ };
+
</file context>
| export default async function ChatThreadPage() { | ||
| const session = await getSessionFromHeaders(); | ||
| if (!session) redirect("/login"); | ||
| return <Assistant />; |
There was a problem hiding this comment.
P2: A hard reload can still paint the New Chat state before the URL thread is selected after hydration. Gate the visible thread UI until the initial URL sync completes, or seed the runtime before the first render.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/chat/[threadId]/page.tsx, line 14:
<comment>A hard reload can still paint the New Chat state before the URL thread is selected after hydration. Gate the visible thread UI until the initial URL sync completes, or seed the runtime before the first render.</comment>
<file context>
@@ -0,0 +1,15 @@
+export default async function ChatThreadPage() {
+ const session = await getSessionFromHeaders();
+ if (!session) redirect("/login");
+ return <Assistant />;
+}
</file context>
| // `load()` and the adapter `fetch()`). | ||
| export default async function ChatThreadPage() { | ||
| const session = await getSessionFromHeaders(); | ||
| if (!session) redirect("/login"); |
There was a problem hiding this comment.
P2: Logged-out users opening /chat/<threadId> lose the deep link and land on /chat after sign-in. Preserve the requested chat path through the authentication flow and redirect back to it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/chat/[threadId]/page.tsx, line 13:
<comment>Logged-out users opening `/chat/<threadId>` lose the deep link and land on `/chat` after sign-in. Preserve the requested chat path through the authentication flow and redirect back to it.</comment>
<file context>
@@ -0,0 +1,15 @@
+// `load()` and the adapter `fetch()`).
+export default async function ChatThreadPage() {
+ const session = await getSessionFromHeaders();
+ if (!session) redirect("/login");
+ return <Assistant />;
+}
</file context>
| @@ -0,0 +1,15 @@ | |||
| import { redirect } from "next/navigation"; | |||
There was a problem hiding this comment.
P3: This page duplicates the complete authenticated chat-page implementation, so future auth or rendering changes can diverge between /chat and /chat/<threadId>. Re-exporting or sharing the existing page keeps both routes on one implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/chat/[threadId]/page.tsx, line 1:
<comment>This page duplicates the complete authenticated chat-page implementation, so future auth or rendering changes can diverge between `/chat` and `/chat/<threadId>`. Re-exporting or sharing the existing page keeps both routes on one implementation.</comment>
<file context>
@@ -0,0 +1,15 @@
+import { redirect } from "next/navigation";
+import { Assistant } from "@/app/assistant";
+import { getSessionFromHeaders } from "@/lib/auth/queries";
</file context>
Two failure modes met at /chat/<bogus>:
1. adapter.fetch 404 — the silent .catch(() => {}) left the sidebar
pinned at 5× ThreadListSkeleton.
2. Turbopack "Entry not available in the store" on a dynamic param —
blank page until manual reload.
Fix layering:
- Pre-validate against threadListAdapter.list() before calling
api.threads().switchToThread(urlThreadId). aUI's switchToThread
internally re-fetches the item; if THAT 404s the function rejects
but the runtime's _state.isLoading flag stays stuck mid-transition
no matter how the URL is rewritten (router.replace, window.history,
window.location.assign all leave the sidebar pinned at the
skeleton). Gating the call on a list() lookup keeps the runtime
state clean in the first place.
- Keep the silent .catch as a fallback for the cross-user race
(list() returned a stale yes between the precheck and the per-thread
fetch).
- Add app/chat/[threadId]/error.tsx as a server-side catch for
Turbopack/streaming errors that bypass the client-side catch.
Drive-by: popstate handler was registered as `() => syncFromUrl()`
at addEventListener and removed as a fresh `() => syncFromUrl()` at
cleanup — oxlint no-invalid-remove-event-listener caught the
reference mismatch. Single onPopState reference now.
| useEffect(() => { | ||
| if (!hasHydratedRef.current) return; | ||
| const id = activeExternalId ?? mainThreadId; | ||
| if (!id || id.startsWith(LOCAL_THREAD_PREFIX)) { | ||
| localStorage.removeItem(ACTIVE_THREAD_ID); | ||
| return; | ||
| } | ||
| localStorage.setItem(ACTIVE_THREAD_ID, id); | ||
| }, [activeExternalId, mainThreadId]); | ||
| if (typeof window === "undefined") return; | ||
| if (activeExternalId) return; // writeUrlForThread handled it | ||
| if (window.location.pathname === "/chat") return; | ||
| window.history.pushState({ threadId: null }, "", "/chat"); | ||
| }, [activeExternalId]); |
There was a problem hiding this comment.
/chat history entry on share-link/reload to /chat/<id>
On initial mount the aUI runtime creates a placeholder thread (__LOCALID_*) before the thread list loads, so activeExternalId is undefined. This effect fires immediately — before syncFromUrl (async, awaiting threadListAdapter.list()) has had a chance to call switchToThread — and pushes /chat to history while the URL is still /chat/<id>. Later, when switchToThread settles and onThreadIdChange fires, writeUrlForThread pushes /chat/<id> back. The net result is an extra /chat entry in the history stack: pressing Back from the restored /chat/<id> lands on the spurious /chat entry rather than the user's actual previous page.
The two scenarios this effect must distinguish are identical from the effect's perspective: a user who just clicked "+New" and a page that just loaded on /chat/<id> both present activeExternalId = undefined with mainThreadId as a __LOCALID_* placeholder. Suppressing the push until syncFromUrl has finished its first run (tracked via a ref) would separate the cases — the "new thread" push only needs to happen when initialization has completed and there is still no external id to push.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/assistant.tsx
Line: 445-450
Comment:
**Spurious `/chat` history entry on share-link/reload to `/chat/<id>`**
On initial mount the aUI runtime creates a placeholder thread (`__LOCALID_*`) before the thread list loads, so `activeExternalId` is `undefined`. This effect fires immediately — before `syncFromUrl` (async, awaiting `threadListAdapter.list()`) has had a chance to call `switchToThread` — and pushes `/chat` to history while the URL is still `/chat/<id>`. Later, when `switchToThread` settles and `onThreadIdChange` fires, `writeUrlForThread` pushes `/chat/<id>` back. The net result is an extra `/chat` entry in the history stack: pressing Back from the restored `/chat/<id>` lands on the spurious `/chat` entry rather than the user's actual previous page.
The two scenarios this effect must distinguish are identical from the effect's perspective: a user who just clicked "+New" and a page that just loaded on `/chat/<id>` both present `activeExternalId = undefined` with `mainThreadId` as a `__LOCALID_*` placeholder. Suppressing the push until `syncFromUrl` has finished its first run (tracked via a ref) would separate the cases — the "new thread" push only needs to happen when initialization has completed and there is still no external id to push.
How can I resolve this? If you propose a fix, please make it concise.
| // here so the bar reflects "new chat" instantly; once the backend | ||
| // thread materializes, `onThreadIdChange` → writeUrlForThread pushes | ||
| // the real `/chat/<uuid>`. | ||
| useEffect(() => { |
There was a problem hiding this comment.
URL flash on direct /chat/<id> visit
On initial mount with activeExternalId === undefined, this effect pushes /chat synchronously — before the first effect's syncFromUrl has a chance to await threadListAdapter.list() and hand off to switchToThread. The result:
- URL:
/chat/<id>→ mount → effects run - Effect 2 (this one) fires:
activeExternalIdisundefined, pathname is/chat/<id>(not/chat), sopushState({ threadId: null }, "", "/chat")fires. Bar now shows/chat. syncFromUrl'sawait threadListAdapter.list()resolves,switchToThread(<id>)succeeds.onThreadIdChange(<id>)→writeUrlForThreadpushes/chat/<id>back.
The user sees the address bar briefly flip to /chat — exactly the flash this PR claims to eliminate (and exactly the symptom Greptile flagged in the sequence diagram).
The "user clicks +New" intent this effect needs to serve is post-mount, not on first render. Gate it with a mounted ref (e.g. const mountedRef = useRef(false) set to true after the first useEffect tick and the URL has been observed via readUrlThreadId), or skip the push when the initial render already had a __LOCALID_* placeholder AND the URL has a threadId segment (it can't be a "new chat" intent at that moment — mainThreadId would have been [threadId segment] if the runtime had hydrated).
Minimal repro: copy the PR's verified table row "Reload on /chat/<A>" and add "watch the address bar between page-paint and the first state?subgraphs=true response." Even a single paint frame of /chat is a flash the user will notice if they typed the URL or refreshed.
| useEffect(() => { | |
| const mountedRef = useRef(false); | |
| useEffect(() => { | |
| // Skip the very first synchronous push after hydration — the parallel | |
| // mount effect (syncFromUrl) is still resolving at this tick and the | |
| // URL may legitimately carry a threadId we're about to hydrate. After | |
| // the first tick, "activeExternalId cleared" always means the user | |
| // clicked +New and we should reset to /chat. | |
| mountedRef.current = true; | |
| }, []); | |
| useEffect(() => { | |
| if (typeof window === "undefined") return; | |
| if (!mountedRef.current) return; | |
| if (activeExternalId) return; // writeUrlForThread handled it | |
| if (window.location.pathname === "/chat") return; | |
| window.history.pushState({ threadId: null }, "", "/chat"); | |
| }, [activeExternalId]); |
| // - listen to `popstate` so browser back/forward syncs aUI without a | ||
| // Next.js route change. | ||
| // `lastSyncedIdRef` prevents the popstate → switchToThread → onThreadIdChange | ||
| // echo from re-pushing. |
There was a problem hiding this comment.
lastSyncedIdRef is only written on the no-op branch — comment vs. implementation mismatch
The leading comment on ThreadUrlShadow says:
lastSyncedIdRefprevents the popstate → switchToThread → onThreadIdChange echo from re-pushing.
But the only assignment is lastSyncedIdRef.current = urlThreadId inside the urlThreadId === currentExternalId early return — i.e. the case where we did no work. After a successful switchToThread(urlThreadId) the ref still holds whatever it held before (null on first popstate). So the urlThreadId === lastSyncedIdRef.current check at line 393 only ever short-circuits the same popstate event firing twice — that's a real possibility (some browsers do fire popstate twice for a single back gesture under back/forward cache restores), so the ref isn't useless, just nowhere near what the comment claims.
Either:
- Drop the leading comment and document the real semantic ("memoizes the last url threadId we observed, regardless of whether we switched to it — protects against
popstatefiring twice on a single back gesture"), or - Actually populate the ref after
switchToThreadresolves so the ref documents "the last thread we successfully switched to from a popstate," which then gatespopstate-driven re-entry to already-switched ids.
Right now the inline comment is misleading enough that a future reader will think the dedup is stronger than it is.
| // echo from re-pushing. | |
| // Memoizes the last url threadId we observed, regardless of whether | |
| // switchToThread ran — protects against `popstate` firing twice on a | |
| // single back gesture (some browsers do, in particular under bfcache | |
| // restores). The value is intentionally not updated after a successful | |
| // switchToThread; see `writeUrlForThread` for the URL write path. |
| // Read thread id off the current path. /chat/<id> → id. /chat (or | ||
| // anything else) → null. We use window.location (NOT useParams/usePathname) | ||
| // because Next.js's router cache doesn't observe our pushState writes. | ||
| const readUrlThreadId = (): string | null => { |
There was a problem hiding this comment.
decodeURIComponent can throw URIError on a malformed path segment
window.location.pathname is normally sane (the browser percent-encodes anything that isn't a valid path byte) but a hand-crafted <a href> targeting this app — or a Next.js redirect that round-trips user input — can land a malformed %-sequence on /chat/<bad%>. decodeURIComponent raises synchronously; the throw bubbles out of syncFromUrl's call site (useEffect callback), and React will surface an uncaught error rather than the redirect we want.
Wrap the decode, or skip to the no-thread path on failure.
let id: string | null = null;
try {
id = decodeURIComponent(m[1]);
} catch {
// fall through, leave id null
}
return id;Currently the effect's catch on switchToThread doesn't cover this and the throw escapes.
| const readUrlThreadId = (): string | null => { | |
| const readUrlThreadId = (): string | null => { | |
| if (typeof window === "undefined") return null; | |
| const m = window.location.pathname.match(/^\/chat\/([^/]+)$/); | |
| if (!m) return null; | |
| try { | |
| return decodeURIComponent(m[1]); | |
| } catch { | |
| return null; | |
| } | |
| }; |
| @@ -0,0 +1,28 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
Pre-flight the redirect inside the server pass (no client flash)
router.replace in useEffect (line 25) means: the user lands on /chat/<bogus>, the segment tries to render, fails, error.tsx mounts, then the URL bar jumps to /chat on the next tick. The error UI renders return null so the flash is empty → black; the user briefly sees a blank /chat/ → /chat churn.
Since this is the failure-mode fallback for "the page couldn't render," the cleanest fix is to do the redirect server-side in app/chat/[threadId]/page.tsx (e.g. when params.threadId fails an obvious sanity check, redirect("/chat")), so the user never sees the bogus URL at all. The Turbopack "Entry not available in the store" case is harder to filter server-side — that one still needs the client fallback — but a pure-static redirect for obviously-bad ids would eliminate most of this.
(Alternative: at minimum, document which of the two failure modes actually requires the client fallback, since the comment lists both as the same path.)
| "use client"; | |
| "use client"; | |
| import { useEffect } from "react"; | |
| import { useRouter } from "next/navigation"; | |
| // Client fallback for render-time errors only — primarily the | |
| // Turbopack "Entry not available in the store" cache miss on a | |
| // dynamic param we haven't built yet. Static id validation (length, | |
| // charset, uuid shape) is handled server-side in | |
| // `app/chat/[threadId]/page.tsx` via `redirect("/chat")`, so by | |
| // the time this mounts the URL is already past the cheap filters | |
| // and we're in genuine error-recovery territory. | |
| // `error`/`reset` are Next-mandated props; we don't surface the | |
| // error to the user — by the time this mounts we've already | |
| // decided the URL is bogus, so just navigate. | |
| export default function ChatThreadError({ | |
| error: _error, | |
| reset: _reset, | |
| }: { | |
| error: Error; | |
| reset: () => void; | |
| }) { | |
| const router = useRouter(); | |
| useEffect(() => { | |
| router.replace("/chat"); | |
| }, [router]); | |
| return null; | |
| } |
Review summaryDirection is sound: bypassing But there are a few issues that need to land before merge. I've posted four inline comments — they're the blocking set; this body is the supporting context. Inline comments posted
CLAUDE.md compliance
Things that look fine
Cross-reference: I didn't open a Discussion on assistant-ui/assistant-ui to verify the runtime's "new options reference → re-load" sensitivity claim. The PR's "we're not flagging a bug, just surfacing a design question" framing is the right tone. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/chat/[threadId]/error.tsx">
<violation number="1" location="app/chat/[threadId]/error.tsx:25">
P1: Valid thread URLs are discarded whenever this segment hits an unrelated render, auth, or transient RSC error, so users are silently moved to a new chat and the original failure is masked. This boundary should preserve the URL and expose `reset`/an error state; only a confirmed invalid-thread response should replace the URL with `/chat`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| }) { | ||
| const router = useRouter(); | ||
| useEffect(() => { | ||
| router.replace("/chat"); |
There was a problem hiding this comment.
P1: Valid thread URLs are discarded whenever this segment hits an unrelated render, auth, or transient RSC error, so users are silently moved to a new chat and the original failure is masked. This boundary should preserve the URL and expose reset/an error state; only a confirmed invalid-thread response should replace the URL with /chat.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/chat/[threadId]/error.tsx, line 25:
<comment>Valid thread URLs are discarded whenever this segment hits an unrelated render, auth, or transient RSC error, so users are silently moved to a new chat and the original failure is masked. This boundary should preserve the URL and expose `reset`/an error state; only a confirmed invalid-thread response should replace the URL with `/chat`.</comment>
<file context>
@@ -0,0 +1,28 @@
+}) {
+ const router = useRouter();
+ useEffect(() => {
+ router.replace("/chat");
+ }, [router]);
+ return null;
</file context>
URL was flickering /chat/<x> → /chat → /chat/<x> because ThreadUrlShadow had
three URL-write paths stacked on top of each other (writeUrlForThread
on onThreadIdChange, a parallel "placeholder effect" for +New, and a
precheck that re-routed to /chat when aUI's `s.threads.threadIds`
didn't contain the URL id). Each rule had its own escape valve, and
they ended up tripping each other under timing variance.
Reset to a single principle: /chat means "trust aUI to pick a
thread"; /chat/<id> means "switch aUI to that id, then back off".
aUI already knows how to reject unknown ids via switchToThread's
Promise — that's the only signal we need.
Removed:
- activeExternalId / threadIds / threadsIsLoading useAuiState reads
- lastSyncedIdRef dedup (switchToThread is idempotent in aUI)
- parallel "placeholder effect" (writeUrlForThread now pushes /chat
directly when remoteId is undefined)
- precheck-then-switch two-step on URL→aUI (single switchToThread +
catch replaces it)
Kept (one path each):
- URL → aUI: switchToThread on mount + popstate, catch on reject
replaces /chat (the only "smart" path left; one wasted 404 round-
trip on bogus ids is the trade)
- aUI → URL: writeUrlForThread in onThreadIdChange, one pushState
per remoteId transition (aUI dedupes internally)
Verified scenarios (Chrome DevTools):
- /chat/<real> direct load → loads, no flicker, 1× /api/threads
- /chat/<bogus> direct load → /chat via reject catch
- click sidebar thread → URL = /chat/<new> immediately, no
intermediate /chat
- /chat (root) → Welcome, no fetch
|
Thanks for the careful write-up — the bypass- 1. The double-push race is real, despite the comment claiming it's not. On a share-link/reload to The author's "no clever precheck … Fix sketch: gate 2. Stale docstring above 3. Back-button to 4. Dead 5. PR description claims "pre-validate ids with 6. Error boundary swallows the error. 7. TDD (CLAUDE.md rule #2). The new Happy to expand any of these into inline comments or PR code suggestions. |
| // below) so user-driven thread switches write the URL automatically. | ||
| // - listen to `popstate` so browser back/forward syncs aUI without a | ||
| // Next.js route change. | ||
| // `lastSyncedIdRef` prevents the popstate → switchToThread → onThreadIdChange |
There was a problem hiding this comment.
Stale docstring — this paragraph describes a lastSyncedIdRef that the actual ThreadUrlShadow implementation never defines; the implementation now uses no such ref. Either reintroduce the ref (which would actually fix the race in the comment 30 lines above this) or trim this line so the docstring matches the code. CLAUDE.md rule #5 also asks for short, why-not-what comments — this block is ~20 lines of prose explaining the bypass that could collapse to a 2-3 line note.
| // RemoteThreadListThreadListRuntimeCore._notifyThreadIdChange. | ||
| // We push the URL via plain history.pushState (no Next router), so | ||
| // thread nav doesn't RSC-refetch the page. | ||
| onThreadIdChange: writeUrlForThread, |
There was a problem hiding this comment.
Race: on mount of /chat/<id>, the runtime's create: factory initializes a placeholder (__LOCALID_*) and fires onThreadIdChange(undefined) → writeUrlForThread(undefined) → pushState("/chat"). Then syncFromUrl's switchToThread(<id>) resolves and aUI fires onThreadIdChange(<id>) → another pushState("/chat/<id>"). Net effect: history gains an extra /chat entry between the user's previous page and the restored thread, breaking Back on share-link/reload (the exact scenario this PR targets).
The pathname equality check is not enough here — undefined → "/chat" proceeds because the URL is /chat/<id> at the time, not /chat. Dedup happens per remoteId inside aUI, but undefined IS a distinct notify the runtime fires on first mount.
Fix: silence the placeholder notify (e.g. flip a hasSeenFirstRemoteIdRef once a real uuid arrives, or treat undefined → /chat as replaceState instead of pushState so it doesn't grow the stack).
| localStorage.setItem(ACTIVE_THREAD_ID, id); | ||
| }, [activeExternalId, mainThreadId]); | ||
| const syncFromUrl = () => { | ||
| const urlThreadId = readUrlThreadId(); |
There was a problem hiding this comment.
When the user navigates back/forward to the bare /chat URL, syncFromUrl reads null and returns early — it never tells aUI to switch off the current thread. So popstate to /chat leaves the chat UI still rendering thread <id> while the URL bar says /chat. Inconsistent state.
Either invoke api.threads().switchToThread(undefined) (or re-run the create: path that boots a fresh placeholder) on the !urlThreadId branch, or have +New push /chat and rely on the runtime already being in placeholder mode at that point. Right now back-button behavior is documented as "works" in the PR description but the popstate handler does nothing for /chat.
| void Promise.resolve( | ||
| api.threads().switchToThread(urlThreadId) as unknown as Promise<void>, | ||
| ).catch(() => { | ||
| if (typeof window === "undefined" || window.location.pathname === "/chat") return; |
There was a problem hiding this comment.
Dead branch: typeof window === "undefined" inside readUrlThreadId, which is only ever called from syncFromUrl inside a useEffect body that already called addEventListener("popstate", ...). window is necessarily defined by the time we get here. The check makes the real branch (pathname !== /chat) easier to miss in review and adds SSR paranoia to a path that can never run during SSR. Drop it.
| // anything else) → null. window.location (NOT useParams/usePathname) | ||
| // because Next.js's router cache doesn't observe our pushState writes. | ||
| const readUrlThreadId = (): string | null => { | ||
| if (typeof window === "undefined") return null; |
There was a problem hiding this comment.
Dead branch: typeof window === "undefined" inside .catch(), which runs as a microtask after switchToThread was invoked from syncFromUrl (itself inside the same effect that already bound popstate). The window global is necessarily defined. The pathname guard is the actual logic; the typeof check is noise.
| const router = useRouter(); | ||
| useEffect(() => { | ||
| router.replace("/chat"); | ||
| }, [router]); |
There was a problem hiding this comment.
Error is silently dropped. The comment names "auth check throw, RSC serialize hiccup" as expected fallthrough cases — those are real production bugs that should be visible in dev / staging console logs. Calling console.error(error) before router.replace keeps the redirect UX intact while preserving signal for debugging. Today, the only way an engineer sees those errors is by reproducing the URL — which is fine for the bogus-id case (error.message isn't interesting) but not for the generic catch-all.
| // remount on every route push (issue #27 history: that double-fires | ||
| // `load()` and the adapter `fetch()`). | ||
| export default async function ChatThreadPage() { | ||
| const session = await getSessionFromHeaders(); |
There was a problem hiding this comment.
Missing tests (CLAUDE.md rule #2). The PR introduces non-trivial new client behavior — ThreadUrlShadow, writeUrlForThread, this dynamic route page, and the error boundary — and the verification in the PR description is a Chrome DevTools MCP walkthrough, not a CI-runnable test. At minimum, a Vitest+jsdom test for writeUrlForThread (idempotency, placeholder handling, no spurious history) and a Playwright test that exercises share-link reload + back/forward would catch the race + the /chat popstate gap I'd flag inline. No test files under tests/frontend/ or tests/e2e/ reference ThreadUrlShadow / chat/[threadId].
1 participant
Closes #27.
What
Clicking a thread in the sidebar now updates the URL to
/chat/<threadId>. Reload and share-link on/chat/<threadId>land on the right thread without the empty-thread flash. Browser back/forward walk thread history.Why the prior attempt looped (and how this avoids it)
The previous attempt routed through
next/navigation'srouter.push, which on a dynamic route triggers an RSC payload refetch and re-passesparams.threadIdas a React prop. That re-pass is what tripped the assistant-ui runtime's "new options reference → re-init the load effect" path — firingstate?subgraphs=trueandadapter.fetch(remoteId)twice for the same id. Issue #27's Risk section flagged this exact cycle as the reason a previous attempt was abandoned.Fix is to bypass
next/navigationentirely for thread navigation:useLangGraphRuntime({ onThreadIdChange: writeUrlForThread })— aUI's built-in deduped callback (seeRemoteThreadListThreadListRuntimeCore._notifyThreadIdChange) writes the URL viawindow.history.pushState. No RSC roundtrip.ThreadUrlShadowlistens forpopstateso browser back/forward sync aUI without a Next.js route change.app/chat/[threadId]/page.tsxexists for share-link / refresh, but renders<Assistant />with nothreadIdprop — thread state is read offwindow.location.pathnameclient-side. The dynamic-param-as-prop flow was the trigger for the double-load; removing it cuts the cycle.+New(placeholder__LOCALID_*has no remoteId, soonThreadIdChangeis silent until first message initializes the backend thread); the placeholder pushes/chat, the real uuid then takes over.Verified
End-to-end via Chrome DevTools MCP:
/chat/<A>/chat/<B>via pushState; 1×state?subgraphs=true, 0 RSC, 0 duplicateadapter.fetch/chat/<A>/chat; 0 state fetch (placeholder, lazy init)/chat/<A>/chat/<A>→/chat/<B>switchToThread, aUI state resumes from cacheFollow-up
A Discussion is being filed on assistant-ui/assistant-ui asking whether the runtime's "new options reference → re-load" sensitivity should be relaxed (or documented as a contract). The above fix is the recommended workaround in either reading; the Discussion is to surface the design question for maintainers rather than flag a bug.
Summary by cubic
Sync the chat URL with the active thread and fix the navigation loop from issue #27. Threads now have stable, shareable URLs; back/forward is smooth;
/chatopens a fresh chat; invalid thread URLs fall back to/chat.New Features
history.pushState; bypassnext/navigationand simplifyThreadUrlShadowto a single URL↔aUI path./chat/[threadId]page for refresh/share; thread id is read fromwindow.location.pathnameto avoid prop-driven remounts.ACTIVE_THREAD_IDlocalStorage;/chatnow starts a new chat.Bug Fixes
router.pushon dynamic routes./chatflicker on reload/share; no spurious history entries./chatviaswitchToThread().catch(...); keep/chat/[threadId]/error.tsxfor segment errors.popstatelistener cleanup to avoid leaked handlers.Written for commit 519a5ad. Summary will update on new commits.
Greptile Summary
This PR closes issue #27 by replacing the
localStorage-basedThreadPersistencecomponent with ahistory.pushState-basedThreadUrlShadow, making thread URLs stable, shareable, and back/forward-navigable without triggering RSC refetches or double-loading the adapter.onThreadIdChange: A module-scopewriteUrlForThreadcallback is passed touseLangGraphRuntime; it updates the URL on every aUI-driven thread change and guards against echoes with apathname === targetcheck.popstateevents sync the reverse direction (browser → aUI)./chat/[threadId]route: The page renders<Assistant />without forwardingparams.threadIdas a prop — thread state is read offwindow.location.pathnameclient-side byThreadUrlShadow, avoiding the prop-change remount that caused the double-load()in the prior attempt.ACTIVE_THREAD_IDlocalStorage constant removed;error.tsxboundary added to redirect stale/invalid segment URLs back to/chat.Confidence Score: 5/5
Safe to merge — all changed paths are additive or replace equivalent functionality with a more correct design.
The core URL-sync logic is sound: the pathname === target guard in writeUrlForThread correctly prevents the popstate echo without needing a separate ref, switchToThread is idempotent so rapid back-presses are safe, and router.replace in the error boundary avoids spurious history entries. The only finding is a stale comment that contradicts itself about a non-existent lastSyncedIdRef.
The block comment at the top of ThreadUrlShadow in app/assistant.tsx (lines 353–361) is internally contradictory and should be cleaned up, but it has no runtime impact.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Browser participant ThreadUrlShadow participant aUI as aUI Runtime participant writeUrlForThread Note over Browser,writeUrlForThread: Thread click (sidebar) Browser->>aUI: user clicks thread item aUI->>writeUrlForThread: onThreadIdChange(remoteId) writeUrlForThread->>Browser: history.pushState(/chat/remoteId) Note over Browser,writeUrlForThread: Browser Back / Forward Browser->>ThreadUrlShadow: popstate event ThreadUrlShadow->>ThreadUrlShadow: readUrlThreadId() ThreadUrlShadow->>aUI: switchToThread(urlThreadId) aUI->>writeUrlForThread: onThreadIdChange(remoteId) writeUrlForThread->>writeUrlForThread: "pathname === target? no-op" Note over Browser,writeUrlForThread: Refresh / Share-link on /chat/id Browser->>ThreadUrlShadow: mount (syncFromUrl) ThreadUrlShadow->>ThreadUrlShadow: readUrlThreadId() → id ThreadUrlShadow->>aUI: switchToThread(id) aUI-->>ThreadUrlShadow: reject (bogus id) ThreadUrlShadow->>Browser: history.replaceState(/chat)%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Browser participant ThreadUrlShadow participant aUI as aUI Runtime participant writeUrlForThread Note over Browser,writeUrlForThread: Thread click (sidebar) Browser->>aUI: user clicks thread item aUI->>writeUrlForThread: onThreadIdChange(remoteId) writeUrlForThread->>Browser: history.pushState(/chat/remoteId) Note over Browser,writeUrlForThread: Browser Back / Forward Browser->>ThreadUrlShadow: popstate event ThreadUrlShadow->>ThreadUrlShadow: readUrlThreadId() ThreadUrlShadow->>aUI: switchToThread(urlThreadId) aUI->>writeUrlForThread: onThreadIdChange(remoteId) writeUrlForThread->>writeUrlForThread: "pathname === target? no-op" Note over Browser,writeUrlForThread: Refresh / Share-link on /chat/id Browser->>ThreadUrlShadow: mount (syncFromUrl) ThreadUrlShadow->>ThreadUrlShadow: readUrlThreadId() → id ThreadUrlShadow->>aUI: switchToThread(id) aUI-->>ThreadUrlShadow: reject (bogus id) ThreadUrlShadow->>Browser: history.replaceState(/chat)Reviews (4): Last reviewed commit: "refactor(chat): trust URL threadId; simp..." | Re-trigger Greptile
Context used: