feat(ai): deterministic chat rendering + synchronous send with contextRef - #2072
Merged
Conversation
Closes the structural-gate symptoms from the "Deterministic Chat Rendering
& Send" epic (leaves 1.1-1.9):
- selectMessagesAreaMode: pure decision — skeleton only when loading AND
zero messages AND zero streams; keeps the list/content on screen for
loading-with-content (background refetch, rejoin) instead of swapping to
a skeleton. Wired into ChatMessagesArea (replacing the bare `isLoading`
ternary that also unintentionally hid remote streams and the streaming
indicator during any loading tick) and into SidebarChatTab's in-place
messages-pane indicator.
- resolveInputPosition: pure decision with an explicit, caller-owned latch
— docked position is latched per conversationId so a mid-refetch race
(isLoading/hasMessages both transiently false) can't flash the composer
back to centered for a conversation that plainly has content. A genuinely
fresh, empty conversation (New Chat) still centers correctly since the
latch never carries over to a different conversationId. Wired into
ChatLayout; the messages-panel AnimatePresence is now keyed by
conversationId (clean remount on conversation switch) and gated on the
latched docked decision instead of raw hasMessages/isLoading.
- SidebarChatTab no longer gates its entire subtree (header + input +
messages) on `isMessagesLoading` — only `!isInitialized` does that now.
`isMessagesLoading` becomes an in-place indicator inside the messages
pane, reusing the same selectMessagesAreaMode rule and the same rendering
slot the global-mode fetch indicator already used.
- useSendHandoff.wrapSend: the old synchronous try/catch missed
asynchronous rejections (sendFn is often `async () => { await
contextPromise; sendMessage(...) }`), leaving pendingSend registered
until the 15s safety timeout with no visible error. A new isThenable()
pure guard routes both the synchronous-throw and async-rejection paths
through one settle function that clears pendingSend immediately and
surfaces the error via toast + getAIErrorMessage (never raw
error.message).
Spec -> test mapping:
- 1.2/1.3 selectMessagesAreaMode -> streams/__tests__/selectMessagesAreaMode.test.ts
(100% branch), wired in ChatMessagesArea.tsx + SidebarChatTab.tsx
- 1.4/1.5 resolveInputPosition -> streams/__tests__/resolveInputPosition.test.ts
(100% branch), wired in ChatLayout.tsx
- 1.6 SidebarChatTab gate -> component change, no new pure fn; validated by
typecheck + existing SidebarChatTab.test.tsx pure-fn suite (unaffected)
- 1.7/1.8 useSendHandoff async rejection -> isThenable.test.ts (100% branch)
+ hooks/__tests__/useSendHandoff.test.ts (renderHook, real
useEditingStore, mocked sonner)
Validation: `bun run typecheck` (apps/web) green. Targeted vitest green:
isThenable, selectMessagesAreaMode, resolveInputPosition (all 100% branch),
useSendHandoff, ChatLayout remoteStreams plumbing, SidebarChatTab pure-fn
suite (46 tests, unaffected).
Known-broken in this .pu worktree (environmental, pre-existing, unrelated
to this diff — confirmed via git stash before touching any code):
SidebarMessagesContent.test.tsx fails with "ReferenceError: React is not
defined" on every `render()` call. Not modified by this PR; must be
re-verified on the main checkout/CI per the epic's stated render-test
caveat.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MQpdoGj1j2gRPYE9n9cypX
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 tasks
…2074) PR 2 of 2 from the "Deterministic Chat Rendering & Send" epic (leaves 2.1-2.7), stacked on pu/e1-flash-and-send. Removes the #2060-regression awaited context fetch (resolveLocationContext -> /api/pages/:id + /breadcrumbs, 0.5-3s, deliberately uncached) that delayed the optimistic bubble, status:'submitted', streaming registration, and Stop button on every send from the sidebar and global assistant. - buildContextRef (new pure fn, 100% branch): synchronously parses the current pathname into { routeType, pageId?, driveId?, dmConversationId? } — no network, no await. Reuses parseTabPath (tab-title.ts) rather than re-deriving route parsing. - resolveRequestContext (new, apps/web/src/lib/ai/core): server-side counterpart. Permission-checks the ref at request time via canPrincipalViewPage / isPrincipalDriveMember (principal-permissions.ts, MCP + OAuth + session aware) and DENIES (resolves to null/undefined, never throws) a contextRef pointing at a page/drive the caller cannot view — closing an information-disclosure gap the old client-trusted pageContext/locationContext body fields had no defense against at all. - getPageBreadcrumbTrail (apps/web/src/lib/pages): the recursive-CTE breadcrumb walk factored out of /api/pages/[pageId]/breadcrumbs/route.ts into a shared function — one source of truth for both the breadcrumb bar and the AI resolver, instead of two independent walks. - Both /api/ai/chat and /api/ai/global/[id]/messages accept a top-level contextRef field; when present, it is the sole source of location context (resolved + permission-checked server-side) — the legacy pageContext/locationContext body fields are still honored ONLY when no contextRef is sent, kept 1+ release for any client bundle that hasn't picked up this change yet. - All 3 surfaces (AiChatView, SidebarChatTab, GlobalAssistantView) plus their voice-send and ask-user-answer body builders now build contextRef synchronously (AiChatView: page.id/driveId already known as props, no pathname parsing needed; Sidebar/GlobalAssistantView: buildContextRef from pathname+drives) and send it inline with wrapSend — nothing is awaited between click and sendMessage. GlobalAssistantView's UI-display `locationContext` state/effect (welcome text, mention-picker driveId) is untouched; only its send-time value changed from that effect-derived state to a synchronous contextRef, per the exact pattern documented in resolveLocationContext.ts's own docstring (effect for display, fresh synchronous value for sends). - Deleted the now-fully-dead buildPageContext() (fetchBreadcrumbs-awaiting builder in buildPageContext.ts) and its test file — zero remaining callers after the AiChatView/Sidebar cutover. locationContextToPageContext (still used by chat/route.ts to adapt the resolver's nested output to the flat pageContext shape) is unchanged. Spec -> test mapping: - 2.2/2.3 buildContextRef -> shared/__tests__/buildContextRef.test.ts (100% branch) - 2.4/2.5 resolveRequestContext -> core/__tests__/resolve-request-context.test.ts (13 cases incl. the page-caller-cannot-view and drive-caller-not-member DENY paths; 100% branch on the new module) - 2.6 send-path cutover -> existing SidebarChatTab.test.tsx (46 tests, pure-fn suite, unaffected), existing GlobalAssistantView.test.tsx (22 tests, unaffected), global-chat-request-body.test.ts extended with contextRef passthrough cases (14 tests) - Refactor-only: breadcrumbs route.test.ts (8 tests) verified unchanged behavior after factoring getPageBreadcrumbTrail out Validation: - `bun run typecheck` (apps/web, direct `tsc --noEmit`): green, repeatedly confirmed. The ROOT `bun run typecheck` (turbo) intermittently fails with TS6053 "file not found" against `.next/types/**` generated stubs — this reproduces even on a totally clean `next build` immediately beforehand (confirmed: every failure is TS6053 against Next's generated route-type stubs, zero real semantic errors) and correlates with other worktrees on this machine running concurrent `next build`s. Environmental turbo/.next cache flake, not a defect in this diff. - Targeted `bun vitest run`: all new/changed unit + route tests green (see mapping above); pre-existing chat/global route test suites (mcp-scope, credit-gate, credit-abort, stream-socket-events, sandbox-github-suppression, conversation-id-resolution, resolve-or-create-conversation — 90 tests) re-run and confirmed unaffected by the route wiring changes. - Manual verification ("sidebar send shows bubble+Stop in same tick on cold cache") NOT performed by me — no browser/UI automation tool is available in this environment. Automated coverage (route + pure-fn tests) is green; this click-through needs a reviewer with browser access, same caveat pattern as PR 1's render-path verification. Claude-Session: https://claude.ai/code/session_01MQpdoGj1j2gRPYE9n9cypX Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…tract
ChatMessagesArea.remoteStreams.test.tsx had a pre-existing assertion
("given remoteStreams but isLoading=true, suppresses synthesized renders")
that pinned the OLD behavior selectMessagesAreaMode (leaf 1.2/1.3 of this
epic) deliberately replaces: skeleton only when loading AND zero messages
AND zero streams.
CI caught this as a genuine regression (this render test doesn't hit the
"React is not defined" environment issue that blocks it in .pu worktrees,
so it never ran locally during implementation) — 1 failed, 13306 passed.
The old assertion encoded a stale assumption: that a loading conversation
has nothing worth showing yet. But `remoteStreams` is already
conversation-scoped by every caller before it reaches this component (see
AiChatView's `remoteStreams` selector comment) — a stream present here
genuinely belongs to the conversation being displayed, so hiding it behind
a skeleton during a switch/refresh is exactly the flash this epic exists
to eliminate. Epic goal: "streams stay visible through conversation
switches, refreshes..."; epic-level verification: "switch conversation and
back shows live stream."
Split the one stale assertion into two that match the new, intentional
contract:
- loading + a stream present -> stream still renders (content beats skeleton)
- loading + zero messages + zero streams -> still suppressed (skeleton owns
the area)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MQpdoGj1j2gRPYE9n9cypX
Self-review finding (proactive pass, no reviewer comment yet): both routes computed pageContext/locationContext from contextRef (a permission check + DB round-trip) BEFORE the "messages required" / "chatId required" checks. An invalid request (missing messages or chatId) paid for that DB work before being rejected with 400. Moved both resolutions to right after their respective required-field checks so invalid requests fail fast. No behavior change for valid requests — same resolution, same result, just later in the function. Confirmed via existing route test suites (chat + global messages, 103 tests) all still green, plus a full typecheck pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQpdoGj1j2gRPYE9n9cypX
2witstudios
changed the title
Self-review finding (proactive pass): resolveRequestContext denied a contextRef pointing at a page the caller can't view or a drive they aren't a member of by silently resolving to null — correct for the AI prompt (never leak the resource), but unlike the sibling authz check right next to it in the same route (checkMCPPageScope, which audits 'mcp_page_scope_denied'), the denial left no audit trail at all. Repeated contextRef probing would go completely unobserved. Added an optional onAccessDenied callback to resolveRequestContext, fired only for an actual authorization denial (view/membership check failed) — not for benign misses (routeType 'dm'/'other', missing pageId/driveId, or a page that passed the view check but no longer exists). Both routes wire it to auditRequest with the same eventType/shape convention already used for every other denial in these routes (authz.access.denied, riskScore 0.3 — lower than the outright-blocked mcp_page_scope_denied at 0.5, since this degrades gracefully to "no context" rather than blocking the request). 10 test cases added (20 total, still 100% branch) covering: denial fires for page/drive, does NOT fire for successful resolution, empty trail, missing contextRef, 'other' routeType, or when no callback is passed. Also folds in a small stale-comment tidy in SidebarChatTab.tsx (buildSidebarChatRequestBody's docblock still said "freshly-resolved location" after the contextRef cutover). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MQpdoGj1j2gRPYE9n9cypX
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Both PRs from the "Deterministic Chat Rendering & Send" epic (
pagespace pages read l4w75179xdk5flyfkin184c2) are now combined in this single PR againstmaster. PR #2074 (leaves 2.1-2.7) was originally stacked on this branch and has since been merged into it, so this PR now carries the full epic scope (leaves 1.1-2.7) plus two post-merge fixes from CI/self-review convergence.Part 1 — flash-free chat rendering (leaves 1.1-1.9)
selectMessagesAreaMode(pure fn, 100% branch): skeleton only when loading AND zero messages AND zero streams. Wired intoChatMessagesArea(replacing a bareisLoadingternary that also hid remote streams/the streaming indicator on every loading tick) and intoSidebarChatTab's in-place messages indicator.resolveInputPosition(pure fn, 100% branch): latches the docked composer position perconversationIdso a mid-refetch race can't flash the input back to centered for a conversation that plainly has content. Wired intoChatLayout; the messages-panelAnimatePresenceis keyed byconversationId.isMessagesLoading— only!isInitializeddoes.isMessagesLoadingis an in-place indicator inside the messages pane.useSendHandoff.wrapSend: async rejections (not just sync throws) now clearpendingSendimmediately and surface the error viatoast.error(getAIErrorMessage(...)).Part 2 — synchronous send with server-resolved context (leaves 2.1-2.7)
buildContextRef(pure fn, 100% branch): synchronously parses the current pathname into{ routeType, pageId?, driveId?, dmConversationId? }— no network, no await.resolveRequestContext(server,lib/ai/core): permission-checks the ref at request time viacanPrincipalViewPage/isPrincipalDriveMemberand DENIES (resolves tonull) a contextRef pointing at a page/drive the caller cannot view — closing an information-disclosure gap the old client-trustedpageContext/locationContextfields had no defense against.getPageBreadcrumbTrail: the recursive-CTE breadcrumb walk factored out of the breadcrumbs route into a shared function./api/ai/chatand/api/ai/global/[id]/messagesaccept a top-levelcontextRef; legacypageContext/locationContextfields are honored only when no contextRef is sent (kept 1+ release).contextRefsynchronously — nothing is awaited between click andsendMessage.buildPageContext()builder and its test file.Post-merge convergence fixes (this session)
ChatMessagesArea.remoteStreams.test.tsxhad a pre-existing assertion pinning the OLD "suppress remote streams wheneverisLoading" behavior thatselectMessagesAreaModedeliberately supersedes.remoteStreamsis already conversation-scoped by every caller before reaching this component, so showing an already-scoped stream during a loading tick is correct per the epic's own goal ("streams stay visible through conversation switches, refreshes..."). Split the stale assertion into two that match the new, intentional contract.contextRef(a permission check + DB round-trip) before the messages/chatId required-field validation. Reordered so invalid requests fail fast without the extra DB round-trip.Spec -> test mapping
selectMessagesAreaModestreams/__tests__/selectMessagesAreaMode.test.tsresolveInputPositionstreams/__tests__/resolveInputPosition.test.tsSidebarChatTab.test.tsx(46 tests)useSendHandoffasync rejectionisThenable.test.ts(100% branch) +hooks/__tests__/useSendHandoff.test.tsisThenablebuildContextRefshared/__tests__/buildContextRef.test.tsresolveRequestContextcore/__tests__/resolve-request-context.test.ts(13 cases incl. DENY paths)SidebarChatTab.test.tsx/GlobalAssistantView.test.tsx(unaffected),global-chat-request-body.test.tsextendedD tasks filed
None.
Validation
bun run typecheck(apps/web, directtsc --noEmit): green, repeatedly confirmed.bun vitest run: all new/changed unit + route tests green; pre-existing chat/global route suites (mcp-scope, credit-gate, credit-abort, stream-socket-events, sandbox-github-suppression, conversation-id-resolution, resolve-or-create-conversation — 103 tests) re-confirmed unaffected after the reordering fix.eslinton all touched files: clean, zero warnings.ChatMessagesArea.remoteStreams.test.tsxassertion above, now fixed. Note that render tests which fail locally in this.puworktree withReferenceError: React is not defined(an environment-specific quirk) run and pass fine in real CI — confirmed since only 1/13313 tests failed in the actual CI environment.bun run typecheck(turbo) intermittently fails withTS6053against stale.next/types/**generated stubs, reproducing even after a cleannext build— an environmental turbo/.nextcache flake (correlates with concurrentnext builds from other worktrees on this machine), not a defect in this diff. Directtsc --noEmitis the authoritative signal and is clean.Test plan
🤖 Generated with Claude Code
https://claude.ai/code/session_01MQpdoGj1j2gRPYE9n9cypX