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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 70 additions & 37 deletions app/assistant.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import { Button } from "@/components/ui/button";
import { threadListAdapter } from "@/lib/threads/adapter";
import { R2AttachmentAdapter } from "@/lib/attachments/r2-adapter";
import { cn } from "@/lib/utils";
import { LOCAL_THREAD_PREFIX, ACTIVE_THREAD_ID } from "@/lib/constants";
import { createLangGraphStream } from "@/lib/langgraph/create-stream";

// Provider-scoped values (api, mainThreadId) bridged into a ref so the
Expand Down Expand Up @@ -216,6 +215,12 @@ export function Assistant() {
unstable_allowCancellation: true,
unstable_enableMessageQueue: true,
unstable_threadListAdapter: threadListAdapter,
// ponytail: aUI's built-in thread-change hook fires once per
// _mainThreadRemoteId transition, deduped internally — see
// RemoteThreadListThreadListRuntimeCore._notifyThreadIdChange.
// We push the URL via plain history.pushState (no Next router), so
// thread nav doesn't RSC-refetch the page.
onThreadIdChange: writeUrlForThread,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

stream,
eventHandlers,
...(attachments ? { adapters: { attachments } } : {}),
Expand Down Expand Up @@ -307,7 +312,7 @@ export function Assistant() {
return (
<AssistantRuntimeProvider aui={aui} runtime={runtime}>
<AuiRefCapture bridgeRef={bridgeRef} />
<ThreadPersistence />
<ThreadUrlShadow />
<div className="bg-muted/30 flex h-dvh w-full">
<Sidebar collapsed={sidebarCollapsed} />
<div className="flex min-w-0 flex-1 flex-col overflow-hidden p-2 md:pl-0">
Expand Down Expand Up @@ -335,45 +340,73 @@ const AuiRefCapture: FC<{ bridgeRef: RefObject<RuntimeBridge> }> = ({ bridgeRef
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// echo from re-pushing.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lastSyncedIdRef is only written on the no-op branch — comment vs. implementation mismatch

The leading comment on ThreadUrlShadow says:

lastSyncedIdRef prevents 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 popstate firing twice on a single back gesture"), or
  • Actually populate the ref after switchToThread resolves so the ref documents "the last thread we successfully switched to from a popstate," which then gates popstate-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.

Suggested change
// 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.

// ponytail: trust the URL. /chat → aUI's default (placeholder, new
// thread welcome). /chat/<id> → "the active thread is this id".
// React to URL changes (mount, back/forward) by switching aUI; react
// to aUI's `onThreadIdChange` by writing the URL back. No clever
// precheck, no parallel placeholder effect, no lastSyncedIdRef —
// `switchToThread` is idempotent and `onThreadIdChange` already dedupes
// internally. The only failure path is aUI's Promise reject on a
// genuinely-bogus id (404 / cross-user), which we redirect to /chat.
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);

// Runs before the write effect on first commit so hasHydratedRef
// suppresses the placeholder write below.
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);
},
);
}
hasHydratedRef.current = true;
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
// Read thread id off the current path. /chat/<id> → id. /chat (or

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// anything else) → null. window.location (NOT useParams/usePathname)
// because Next.js's router cache doesn't observe our pushState writes.
const readUrlThreadId = (): string | null => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;
}
};

if (typeof window === "undefined") return null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 m = window.location.pathname.match(/^\/chat\/([^/]+)$/);
return m ? decodeURIComponent(m[1]) : null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cubic-dev-ai cubic-dev-ai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with cubic

};
Comment on lines +366 to +373

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 "What" comments violate Rule 5

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!

Fix in Claude Code


// URL → aUI: on mount + on browser back/forward. `switchToThread`
// is idempotent (aUI returns early when already on the target id)
// and resolves to a Promise that rejects for unknown / cross-user
// ids — that's our "this URL was bogus" signal, no inference needed.
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]);
const syncFromUrl = () => {
const urlThreadId = readUrlThreadId();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!urlThreadId) return;
void Promise.resolve(
api.threads().switchToThread(urlThreadId) as unknown as Promise<void>,
).catch(() => {
if (typeof window === "undefined" || window.location.pathname === "/chat") return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

window.history.replaceState({ threadId: null }, "", "/chat");
});
};
syncFromUrl();
window.addEventListener("popstate", syncFromUrl);
return () => window.removeEventListener("popstate", syncFromUrl);
}, [api]);

return null;
};
Comment on lines 340 to 396

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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)

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.

Fix in Claude Code


// aUI → URL: write side. Module-scope closure passed to
// `useLangGraphRuntime`'s `onThreadIdChange`; the runtime already
// dedupes internally (_notifyThreadIdChange keeps a lastNotifiedThreadId
// ref), so we just sync the URL on every notify.
//
// `remoteId === undefined` → push /chat (placeholder / cleared state,
// e.g. immediately after +New before adapter.initialize resolves;
// we treat that as "user wants /chat for now"). Real uuid → push
// /chat/<uuid>. No preconditions, no reads of aUI's state.
const writeUrlForThread = (remoteId: string | undefined): void => {
if (typeof window === "undefined") return;
const target = remoteId ? `/chat/${remoteId}` : "/chat";
if (window.location.pathname === target) return;
window.history.pushState(remoteId ? { threadId: remoteId } : { threadId: null }, "", target);
};
28 changes: 28 additions & 0 deletions app/chat/[threadId]/error.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"use client";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Suggested change
"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;
}

import { useEffect } from "react";
import { useRouter } from "next/navigation";

// ponytail: fall back to /chat whenever this segment can't render.
// Two failure modes hit this in practice:
// 1. Turbopack "Entry not available in the store" — a Next 16 dev-mode
// cache miss on a dynamic param we haven't built yet. Replacing
// with the root URL lands us in the well-trodden /chat path
// where the runtime's constructor creates a fresh placeholder.
// 2. Any other segment-level render error (auth check throw, RSC
// serialize hiccup, etc.). Same fallback.
// `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");

@cubic-dev-ai cubic-dev-ai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with cubic

}, [router]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return null;
}
15 changes: 15 additions & 0 deletions app/chat/[threadId]/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { redirect } from "next/navigation";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cubic-dev-ai cubic-dev-ai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with cubic

import { Assistant } from "@/app/assistant";
import { getSessionFromHeaders } from "@/lib/auth/queries";

// ponytail: the dynamic route exists ONLY so refresh / share-link on
// /chat/<id> lands on a real page. Thread state is read off
// `window.location.pathname` client-side by `ThreadUrlShadow`, NOT off
// 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() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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 /login redirect, surface a specific error code) only lands once.

  2. Drop /chat/page.tsx and turn [threadId] into an optional catch-all ([[...threadId]]/page.tsx). The client-side ThreadUrlShadow already 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.

const session = await getSessionFromHeaders();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

if (!session) redirect("/login");

@cubic-dev-ai cubic-dev-ai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with cubic

return <Assistant />;

@cubic-dev-ai cubic-dev-ai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with cubic

}
13 changes: 5 additions & 8 deletions lib/constants.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
// Placeholder mainThreadId assigned to a freshly-created "new thread" that
// hasn't been persisted yet. Filter it out before writing to localStorage —
// we don't want to "remember" a thread id that has no backing record on
// the server, otherwise the next page load would try to switchToThread a
// ghost and hit 404.
// 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.)
Comment on lines +5 to +6

@cubic-dev-ai cubic-dev-ai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
// load. (Used to gate localStorage; the localStorage write itself was
// removed in issue #27 once URL became the source of truth.)
// load.
Fix with cubic

export const LOCAL_THREAD_PREFIX = "__LOCALID_";

// localStorage key for the active thread id. Must match the string the
// runtime reads on hydration.
export const ACTIVE_THREAD_ID = "ACTIVE_THREAD_ID";

// User-facing product name. Don't hardcode "LangGraph" / "assistant-ui"
// strings elsewhere; import this so the brand is one-line to change.
export const APP_NAME = "LangGraph App";
Expand Down
Loading