[Maya-Kid]

Review follow-up (2026-09-23)

Current head: 2d7e35b6af71ebf3356e702f9ff63ab0c3b370ba. Comment-only follow-up to #854 review: document the new-conversation route/ownership/live-entry handoff, correct the old early-return comment, and provide a complete optional Playwright installation and browser-test invocation at the top of chat-view.test.mjs. These browser probes are explicitly separate from Vitest and CI; no new CI scope is introduced. The changes are carried through the #854 → #856 → #855 stack.

The documented command passed on #854 (11 subtests, 12 including the parent); the final stack passed all 27 browser subtests (29 including parents). Runtime code and assertions are unchanged. Prior validation below refers to its stated historical heads.

---

Post-merge synchronization (2026-09-22)

Rebased onto dev@7cfeedfeb11c3a1c42d5091c5941a51afaaeb161 after #848, #851, #849, #850 and #852 merged. Current head: bd11c1d9ea61f5015d55ed8bc0f656c1bb2e2717.

The UI dependency order remains #854 → #856 → #855. Retained upstream stream-interruption strings alongside the history/pagination keys when resolving i18n conflicts. The complete UI tree is byte-for-byte identical to the previous validated integration tree. Maintainer visual review remains outstanding.

Validation on combined tree d06f4ad0199a382635a6bbf94210b9421cbeba66 (675 tracked Linux inputs hash-matched):

• Rust fmt, strict workspace/all-target Clippy and workspace build passed. Workspace tests: 1018 passed, 0 failed, 5 ignored. The two delivery/process-kill parent tests and busy-deferral test additionally passed on their own exclusive database; crash_child is invoked by its parent. The existing public-network RSS test remains ignored.
• The exact unified Chat CI command listed and executed 47 passing tests, including all 37 introduced tests from Hand off exhausted tool runs to an evidence-only final answer #845/Keep older chat handles from unregistering newer generations #848/Publish citation sources added by document reads #849/Keep fallback retrieval failures distinct from empty results #850/Preserve exact chat outcomes across stream closure and reattachment #851/Do not report fallback chat completion when saving fails #852/Assert a chat turn ends in exactly one earned terminal #858. All four terminal-table rows ran. Seven deliberately omitted groups each produced a green Cargo result but were rejected by the independent executed-test checker (exit 1); the complete run passed (exit 0).
• Frontend: 129 module tests, 27 browser subtests (29 including parent tests) and build/typecheck/guard passed.
• The five actual-backend browser scenarios from the previous validation were not repeated this time: the complete frontend and frozen finalization/LLM implementation are unchanged. This pass reruns the deterministic browser and real-PostgreSQL Rust regressions. No new paid-model accuracy claim is made.

Earlier evidence below refers to its stated historical heads.

---

When conversation A loads slowly and the user selects B, A's late success or failure can replace B's transcript or navigate away. A late identity response from a new chat can likewise pull the user back after they leave.

This gives each view request an identity, including separate visits in A → B → A. History, navigation and initial reattachment callbacks must still own that view before changing it. Existing generation handles continue in the background. Temporary history failures keep the selected URL and offer a retry; missing/inaccessible conversations retain their existing redirect behavior. Route changes also hide the previous transcript before paint.

Validation on dev@ea0557b, fix 0df6d2a:

• The first four browser regressions fail on the original production component, then pass with the fix. All 11 component/browser cases pass: late success/error, repeated visits, late identity, new chat, cross-KB navigation, retry, missing history, StrictMode, late snapshots, and selecting an already-loading conversation. Tests import the real Chat, router, query client and live-answer store; HTTP responses are controlled.
• Frontend 116 module tests and the production build/typecheck/guard pass.
• A separate Chrome run against the actual Linux server and PostgreSQL delayed a real history response while selecting another conversation; the current URL and transcript stayed correct. No history payloads were fabricated in that run.
• Integration tree f7bcd0878bf490d6efb122b71d6f13d379df4709 combines this with Hand off exhausted tool runs to an evidence-only final answer #845 and the other scoped Chat fixes: Rust fmt, strict workspace Clippy, 1,013 tests (one pre-existing external-HTTPS RSS test ignored), workspace build, 129 frontend module tests, 27 browser subtests and frontend build pass. This integration result is distinct from the standalone branch checks.

The browser harness uses optional Playwright/Chromium, following the repository's existing browser-test pattern; it adds no package dependency. Run node --test tests/chat-view.test.mjs from web, with CHAT_PLAYWRIGHT_PATH and CHAT_CHROMIUM_PATH if needed. This does not introduce server-side cancellation, retries of a generation, or changes to #845's model policy.

[WaylandYang]

Reviewed the mechanism closely and it holds up. Three things I expected to be
problems and checked — all fine, recording them so the next reader doesn't repeat
the work:

• Removing the prevKbRef effect is safe, not a behaviour loss. setKb
  already navigates, and samePageInKb already drops everything after the section
  segment, so /kb/A/chat/<conv> becomes /kb/B/chat — which is what the deleted
  effect was doing by hand. It was redundant since #261.
• if (handle) return in onSnapshot is a no-op, not a dropped update. The
  server yields exactly one snapshot frame at attach (api/chat.rs:995) and then
  streams deltas, so there is never a second snapshot to ignore.
• The transcript does not vanish when a stream ends. finish() only flips
  streaming, it does not delete the entry, so liveHere stays truthy and
  shown keeps rendering even though loadedKey is still null for a
  freshly-created conversation.

Two things worth a look before this lands:

1. The new-conversation path works by coincidence of ordering. When send()
creates a conversation, onConversation navigates, which fires the new
useLayoutEffect; that effect calls claimView(newId) — invalidating the owner
token send() is still holding — and sets activeIdRef.current = null. The null
then defeats the routeConvId === activeIdRef.current early return in the
route-sync effect, so loadConversation is called mid-stream. It only stays
correct because loadConversation happens to return early on
liveAnswer.entry(...). That's three separate mechanisms having to agree. A short
comment saying so, or skipping the reset when routeConvId === activeIdRef.current,
would make it robust to the next edit.

2. The 448 test lines never run. vite.config.ts has
include: ["src/**/*.test.ts"], so web/tests/*.test.mjs is outside vitest
entirely — pnpm vitest list reports 0 cases from chat-view, and pnpm test is
129 tests on this branch, exactly as on dev. Nothing in CI or in any package
script references web/tests/.

That matches the existing rss-default.test.mjs convention, so it isn't a
departure — but it does mean this fix ships with no executed regression guard, for
precisely the class of async-ownership bug that regresses silently. At minimum,
please copy the rest of that convention: rss-default.test.mjs documents its exact
npm install --prefix ... playwright-core + env-var + node --test command at the
top, and this file only says "uses the optional browser setup from
rss-default.test.mjs". Without the command, the two cases are unrunnable for
anyone who doesn't go reconstruct it.

(Also worth noting for the maintainers: web/tests/rss-default.test.mjs on dev
has the same property. If these browser probes are meant to be runnable on demand,
a pnpm test:browser script would make them discoverable; if they're meant to be
CI-gated, the include pattern needs widening. Separate issue either way.)

[WaylandYang]

Heads-up for reviewers: these three are a stack, not three independent PRs. All
three target dev, so GitHub shows each as mergeable on its own, but:

$ git merge-base --is-ancestor refs/pr/854 refs/pr/856   # true
$ git merge-base --is-ancestor refs/pr/856 refs/pr/855   # true

#854 ⊂ #856 ⊂ #855 — commits relative to dev are 1 / 3 / 4, sharing bd11c1d as
the first commit. They need to merge in the order #854 → #856 → #855.

It also means the diff sizes on the PR list overstate the review work, because each
contains the previous one:

PR	shown	actually new
#854	+543	+543
#856	+774	+231
#855	+1338	+564

Nothing wrong with the stack itself — just worth stating so reviewers don't read
Chat.tsx three times or merge them out of order.

[Maya-Kid]

Addressed both review points with comments only:

• Documented why new-conversation navigation invalidates the send owner and resets activeIdRef, and why loadConversation must first reclaim the already identified liveAnswer entry. Also corrected the old comment that incorrectly attributed this path to the route-sync early return.
• Added the full optional playwright-core installation, CHAT_PLAYWRIGHT_PATH / CHAT_CHROMIUM_PATH environment variables, and node --test invocation at the top of chat-view.test.mjs. It now explicitly says these on-demand probes are outside pnpm test and CI.

I ran the documented command using the stated playwright-core version: #854 passes 11 browser subtests (12 including its parent). Propagated the comment-only change through #856 and #855; the complete stack passes 27 browser subtests (29 including parents). Runtime code and test assertions are unchanged. A shared browser script or CI integration remains a separate scope, as suggested. Updated the three PR descriptions with their current heads; the merge order remains #854 → #856 → #855.

[WaylandYang]

Both points addressed as asked, and the two new comments describe the mechanism
accurately: navigation on a new conversation invalidates the send owner and clears
activeIdRef, so route-sync does call loadConversation, and it is
loadConversation's own liveAnswer.entry check that reclaims the already-identified
handle rather than the early return I had first credited. The test file now carries
the same install/env/run block rss-default.test.mjs does. Re-approving; the earlier
approval was dismissed by the push, not by a change of mind.