Repository navigation
feat(narrative): text-vs-thoughts classification + Whisper redesign - #484
Merged
Merged
Conversation
… per message Adopt Anthropic's message-level `stop_reason` as the authoritative discriminator between thoughts and final response text in the Claude provider, replacing the legacy `hasFiredToolThisTurn && pendingToolUses==0` heuristic that failed on tool-free turns and on the /goal-hook pattern (answer streamed as preamble before a verification tool call). New `AssistantMessageBoundary` AgentEvent fires at the end of each assistant SDK message with `isFinalResponse` derived from `stop_reason`: end_turn / stop_sequence / max_tokens / refusal → final response, anything else (tool_use, pause_turn) → preamble kept inline. The server agent-service drops the open thought when the boundary is final and closes it as preamble otherwise; the web threadStore mirrors the same logic for the live timeline. Also stores the model identifier on each assistant message row so the UI can surface which model produced the response (the message body already shows it in the footer). Adds a nullable `model` column to the `messages` table via drizzle migration 0007.
A self-contained HTML simulator for iterating on assistant-turn rendering without spinning up the full app. Pick a scenario (direct answer, goal-feature pattern, research flow, parallel sub-agents, extended thinking), pick a visual style (production-close, Whisper, Storyboard, Letter), and watch the SDK event stream produce a live narrative while the wire-format log records the underlying Anthropic content_block_delta / message_delta events. Used as the design reference for the Whisper narrative redesign on this branch.
Reshape the assistant turn around prose-first reading with a quiet "actions molecule" of structured rows. The legacy `THOUGHT` chip is gone — all `text`-typed content blocks now render as body prose with a streaming cursor while active. Genuine internal reasoning (Anthropic `thinking`-typed blocks) is the only thing that would still earn a labelled thought treatment in future. Layout changes: - `ThoughtBlock` delegates to `DeltaBlock` so live and persisted text share one renderer; the typewriter reveal and end-of-text cursor now work for every streaming text segment, not just the final response. - `NarrativeFlow` and `PersistedNarrative` drop the outer rail and row dots — tools/hooks/sub-agents carry their own visual markers and stack tightly as one action group above the prose. - `TurnFooter` renders AFTER the response body in both the live flow (inside `NarrativeFlow`) and the persisted flow (new `PersistedTurnFooter` virtual item slotted between the message bubble and `turn-changes`). The misleading "thoughts" tally is dropped from the footer. - `SubagentRow` expanded children get a quiet mini-rail (1px hairline under the parent's layers icon), and the parent layers icon animates while the sub-agent is running. The composer-level `NarrativeIndicator` swaps its pulsing dot for the same animated layers glyph whenever sub-agents are active. Provenance: - `MessageBubble` removes the `▸ ASSISTANT` head. The foot meta line now reads `model · tokens · cost · time`, with each piece omitted gracefully if absent. New `formatModelLabel` helper turns raw provider IDs (`claude-opus-4-7`) into friendly labels (`Claude Opus 4.7`) for the footer. - Foot meta is right-anchored (`ml-auto`) and `flex-wrap`s on narrow panels. Polish: - `Composer` gradient veil reduced from h-5/opaque to h-3/70% so it doesn't bury the turn footer when the chat is scrolled to its tail. - `DeltaBlock` cursor element only mounts while streaming (the `typing-cursor` CSS animation was overriding inline opacity and blinking in the top-left corner of completed segments). - `useTypewriter` initialises `displayed = ""` when streaming so the rAF catch-up actually animates the first batch of deltas, instead of popping the full text in and only typewriting subsequent characters.
📝 WalkthroughWalkthroughThis PR adds model identifier tracking to assistant messages through database persistence and event classification, paired with a major refactoring of client-side narrative streaming UI. A new ChangesModel tracking and narrative streaming refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Adds `model` to the `AgentEvent.Message` contract and has agent-service inject it from the just-persisted row alongside the existing `messageId`. The threadStore's locally-built `Message` reads `params.model` so the foot meta line shows the model name as soon as the bubble appears — previously the model only appeared after a thread refresh re-fetched the persisted row from the DB.
…l-item slot The streaming response previously rendered inside NarrativeFlow's content tree (the `delta` item at the bottom of the timeline) while the persisted MessageBubble rendered as a separate virtual item below — different DOM containers, different fade timings. On session.message the DeltaBlock unmounted at one position and the MessageBubble mounted at another, which read as a jumpy duplication. Extracts the streaming text into its own virtual-item slot (`streaming-response`) positioned where the persisted MessageBubble will land. New `StreamingResponseRow` component renders the live text with the same prose chrome as MessageBubble's content area, so the swap on persist is a content replacement at the same virtual-list index. New `computeLiveStreamingText` helper (exported from build-narrative) mirrors the existing delta-promotion logic so the timeline rows and the streaming-response slot stay in sync: when the latest thought segment is open and no tool is running, that segment fills the streaming slot; otherwise the suffix of streamingText past the closed-thought tape does. NarrativeFlow stops rendering the `delta` items from buildNarrativeItems — the items remain on the array for compatibility with `counts` and existing tests but the rendering moves to the new slot in MessageList.
…ow is shown After session.message fires, volatile state still has the timeline rows for the just-persisted message, so NarrativeFlow renders its own TurnFooter below the timeline (the `!isAgentRunning && !deltaItem` branch). The new persisted-turn-footer virtual item was rendering alongside it — two footers visible until the next turnStarted cleared volatile state, with slightly different durations because one reads event timestamps and the other reads DB-recorded completed_at. Extends the existing persisted-narrative suppression filter in buildVirtualItems to drop the persisted-turn-footer for the message that has live narrative-flow above it. The live footer is canonical while volatile state is in-memory; the persisted one takes over once the next sendMessage clears the volatile slot. Updates two virtual-items tests to reflect the new suppression.
…ticky cursor
Three small adjustments so the streaming reveal reads as deliberate
typing rather than jank:
1. Typewriter step is now clamped to [6, 28] chars per frame
(≈360-1680 chars/sec) instead of `max(3, behind/10)`. The floor
keeps tiny deltas visibly typing for at least one frame; the
ceiling prevents huge coalesced batches from popping the full text
in within a single frame. Mid-size buffers still use the
exponential `behind/8` catch-up so the response feels snappy.
2. The cursor's layout effect no longer sets `opacity: 0` when the
markdown DOM is momentarily empty (Suspense in flight, fallback ↔
resolved transition). Hiding on every empty frame produced visible
flicker during fast streaming. Once the cursor has been positioned,
it stays at its last position until the next text node is found.
The mount-time path still keeps the cursor invisible until the first
positioning runs, so the "blink-in-the-top-left" bug from when
`{isStreaming && <cursor>}` was unconditional doesn't regress.
…w bubble Two ordering bugs in the assistant-turn layout: - The "X steps · N subagents · phase…" indicator rendered at the bottom of NarrativeFlow, but that container sits ABOVE the streaming-response slot in the virtual list. Result: indicator floated above the writing animation. Moved to its own narrative-indicator virtual-item placed after streaming-response so the typewriter reads as the primary surface and the meta status sits beneath. - The TurnFooter rendered inside NarrativeFlow's content, which also sits before the assistant MessageBubble. The summary therefore appeared between the actions and the response instead of closing the turn. Removed the in-flow footer; the persisted-turn-footer slot (already positioned after the bubble) is now the sole owner of that summary, no longer suppressed while a live narrative-flow is on screen. Three new tests in virtual-items.test.ts lock the order: narrative-flow → streaming-response → narrative-indicator → message bubble → persisted-turn-footer.
…, no stale caret Three coordinated changes to the per-text-block animation rules: - useTypewriter previously initialised displayed="" whenever isStreaming was true, including on remount. Switching threads back into an in-flight stream made the rAF loop drain hundreds of buffered chars at the catch-up cap — a visible "dump" before reaching the typing edge. The lazy initialiser now distinguishes a fresh first-flush (target ≤ 96 chars → type from empty) from a remount into an existing stream (target > 96 chars → snap to target - 24 so only the leading edge types in). - build-narrative.ts set isActive on a thought only when it was the latest, open, and no tool was running. But that exact case is already promoted to a delta item one branch up and filtered out of the timeline, so every ThoughtBlock in the live flow always rendered with isActive=false and skipped the typewriter. Now isActive tracks the agent-running state so closed thoughts animate when they snap up to the timeline. - A blinking caret was parking at the end of every ThoughtBlock because isStreaming gated both the typewriter reveal AND the cursor. Split into two props: isStreaming (reveal) and showCursor (caret). ThoughtBlock now passes showCursor=false so closed segments animate in without leaving a cursor behind.
chuks-qua
marked this pull request as ready for review
May 20, 2026 18:35
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/components/chat/narrative/PersistedNarrative.tsx (1)
86-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOutdated docstring: TurnFooter reference is no longer accurate.
The component docstring at lines 95-96 states "Always renders
TurnFooterwhen there's at least one row", butTurnFooterwas removed from this component. The footer is now rendered via the separatepersisted-turn-footervirtual item.📝 Suggested docstring update
/** * Render the persisted narrative timeline for a completed assistant message. * * Lazy-loads records via `loadNarrativeForMessage` on first mount (eager * prefetch in the store covers the recent-message window; this effect catches * the older-message lazy path). Returns `null` until records arrive so the * layout doesn't jump. * * Persisted mode differences from live `NarrativeFlow`: * - No `NarrativeIndicator` (the turn is over) - * - Always renders `TurnFooter` when there's at least one row + * - `TurnFooter` is rendered via the separate `persisted-turn-footer` + * virtual item slot, not inside this component * - Sub-agents render via the same `SubagentRow` but lack the "active" * visual treatment (no pulse, no primary tint) */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/chat/narrative/PersistedNarrative.tsx` around lines 86 - 99, The docstring in PersistedNarrative.tsx incorrectly states that the component "Always renders TurnFooter when there's at least one row"; update the comment to remove the TurnFooter reference and instead state that the footer is rendered via the separate 'persisted-turn-footer' virtual item (and keep the rest of the persisted-mode differences intact), so anyone reading the docstring sees the current rendering behavior and the correct symbol name.
🧹 Nitpick comments (1)
apps/server/src/__tests__/message-repo.test.ts (1)
22-24: ⚡ Quick winAdd a focused round-trip test for
modelpersistence.Line 22 and Line 67 update schema/query coverage, but there’s still no assertion that
create(..., model)is persisted and returned via repo reads.Proposed test addition
describe("create with reply fields", () => { + it("round-trips model for assistant messages", () => { + const created = repo.create( + "thread-1", + "assistant", + "hello", + 1, + undefined, + undefined, + undefined, + "claude-opus-4-7", + ); + + expect(created.model).toBe("claude-opus-4-7"); + + const found = repo.findByIdInThread("thread-1", created.id); + expect(found).not.toBeNull(); + expect(found!.model).toBe("claude-opus-4-7"); + }); + it("persists replyToMessageId and quotedText", () => {Also applies to: 66-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/__tests__/message-repo.test.ts` around lines 22 - 24, Add a focused round‑trip test in apps/server/src/__tests__/message-repo.test.ts that creates a message with a non‑trivial model value and then reads it back from the repository to assert the same model is persisted; call the repo create method you already use (e.g., MessageRepo.create or createMessage) with a distinct model payload, then fetch the saved record via the repo read method used in other tests (e.g., MessageRepo.getById, findById, or list/filter) and assert the returned record.model strictly equals the original model value; keep the test small and isolated so it only verifies persistence of the model field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/chat/MessageBubble.tsx`:
- Around line 576-582: This change affects responsive layout in
MessageBubble.tsx (the metadata area around ReplyButton, BranchButton,
CopyButton and the span rendering
message.model/tokens_used/cost_usd/formattedTime) so add or update Playwright
e2e tests to cover the responsive behaviors: verify that at small, medium, and
large viewports the metadata wraps as expected (flex-wrap and gap utilities) and
that buttons and textContent remain accessible; run the full Playwright suite
locally or CI and include the numeric pass/fail counts for the relevant tests in
the PR description (or CI job output) so reviewers can confirm the responsive
layout tests passed.
In `@apps/web/src/lib/format-model-label.ts`:
- Line 16: Add a TSDoc/JSDoc comment block directly above the exported function
formatModelLabel describing its purpose in one concise sentence (minimum
one-line summary) and optionally include `@param` modelId and `@returns` tags;
ensure the docblock is attached immediately above the "export function
formatModelLabel(modelId: string): string" declaration so the function is
properly documented per coding guidelines.
In `@apps/web/src/stores/threadStore.ts`:
- Around line 1843-1845: When reconciling/replacing the last assistant message
in the dual-channel dedupe path you update id/tokens/timestamp but fail to
propagate the message model, so the footer can remain stale; update the object
mutation that replaces the assistant row to also set model to (params.model as
string | null | undefined) ?? null (i.e., assign params.model normalized to
null) and apply the same change in the other similar reconciliation site noted
around lines 1913-1918 so both code paths propagate model consistently.
---
Outside diff comments:
In `@apps/web/src/components/chat/narrative/PersistedNarrative.tsx`:
- Around line 86-99: The docstring in PersistedNarrative.tsx incorrectly states
that the component "Always renders TurnFooter when there's at least one row";
update the comment to remove the TurnFooter reference and instead state that the
footer is rendered via the separate 'persisted-turn-footer' virtual item (and
keep the rest of the persisted-mode differences intact), so anyone reading the
docstring sees the current rendering behavior and the correct symbol name.
---
Nitpick comments:
In `@apps/server/src/__tests__/message-repo.test.ts`:
- Around line 22-24: Add a focused round‑trip test in
apps/server/src/__tests__/message-repo.test.ts that creates a message with a
non‑trivial model value and then reads it back from the repository to assert the
same model is persisted; call the repo create method you already use (e.g.,
MessageRepo.create or createMessage) with a distinct model payload, then fetch
the saved record via the repo read method used in other tests (e.g.,
MessageRepo.getById, findById, or list/filter) and assert the returned
record.model strictly equals the original model value; keep the test small and
isolated so it only verifies persistence of the model field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5de8be02-f4be-4365-b56e-9a6989cfdd07
📒 Files selected for processing (30)
apps/server/drizzle/0007_lowly_moondragon.sqlapps/server/drizzle/meta/0007_snapshot.jsonapps/server/drizzle/meta/_journal.jsonapps/server/src/__tests__/message-repo.test.tsapps/server/src/providers/claude/claude-provider.tsapps/server/src/repositories/message-repo.tsapps/server/src/services/agent-service.tsapps/server/src/store/schema.tsapps/web/src/__tests__/threadStore-assistant-message-boundary.test.tsapps/web/src/__tests__/virtual-items.test.tsapps/web/src/components/chat/Composer.tsxapps/web/src/components/chat/MessageBubble.tsxapps/web/src/components/chat/MessageList.tsxapps/web/src/components/chat/narrative/DeltaBlock.tsxapps/web/src/components/chat/narrative/NarrativeFlow.tsxapps/web/src/components/chat/narrative/NarrativeIndicator.tsxapps/web/src/components/chat/narrative/PersistedNarrative.tsxapps/web/src/components/chat/narrative/PersistedTurnFooter.tsxapps/web/src/components/chat/narrative/StreamingResponseRow.tsxapps/web/src/components/chat/narrative/SubagentRow.tsxapps/web/src/components/chat/narrative/ThoughtBlock.tsxapps/web/src/components/chat/narrative/TurnFooter.tsxapps/web/src/components/chat/narrative/build-narrative.tsapps/web/src/components/chat/virtual-items.tsapps/web/src/lib/__tests__/format-model-label.test.tsapps/web/src/lib/format-model-label.tsapps/web/src/stores/threadStore.tsdocs/mockups/narrative-rendering.htmlpackages/contracts/src/events/agent-event.tspackages/contracts/src/models/message.ts
Comment on lines
+576
to
582
| <div className="flex flex-wrap items-center gap-x-3 gap-y-1 px-1"> | ||
| {onReply && <ReplyButton onClick={() => onReply(message.id, message.content, "assistant")} />} | ||
| {onBranch && <BranchButton onClick={() => onBranch(message.id)} />} | ||
| <CopyButton content={textContent} /> | ||
| {(message.tokens_used != null || message.cost_usd != null || formattedTime) && ( | ||
| <span className="font-mono text-[10px] tabular-nums text-muted-foreground/55 transition-colors group-hover/msg:text-muted-foreground/80"> | ||
| {(message.model || message.tokens_used != null || message.cost_usd != null || formattedTime) && ( | ||
| <span className="ml-auto font-mono text-[10px] tabular-nums text-muted-foreground/55 transition-colors group-hover/msg:text-muted-foreground/80"> | ||
| {[ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cd apps/web && bun run e2eRepository: Mzeey-Empire/mcode
Length of output: 106
🏁 Script executed:
cat -n apps/web/src/components/chat/MessageBubble.tsx | sed -n '570,600p'Repository: Mzeey-Empire/mcode
Length of output: 1394
🏁 Script executed:
fd -t f "package.json" apps/webRepository: Mzeey-Empire/mcode
Length of output: 84
🏁 Script executed:
fd -t f "playwright.config" apps/webRepository: Mzeey-Empire/mcode
Length of output: 92
🏁 Script executed:
cat -n apps/web/package.json | head -50Repository: Mzeey-Empire/mcode
Length of output: 1963
Run Playwright e2e tests and report pass counts for this responsive layout change.
This modifies responsive message metadata layout with flex-wrap and gap utilities. Per coding guidelines, Playwright test pass counts must be reported when making responsive layout changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/components/chat/MessageBubble.tsx` around lines 576 - 582, This
change affects responsive layout in MessageBubble.tsx (the metadata area around
ReplyButton, BranchButton, CopyButton and the span rendering
message.model/tokens_used/cost_usd/formattedTime) so add or update Playwright
e2e tests to cover the responsive behaviors: verify that at small, medium, and
large viewports the metadata wraps as expected (flex-wrap and gap utilities) and
that buttons and textContent remain accessible; run the full Playwright suite
locally or CI and include the numeric pass/fail counts for the relevant tests in
the PR description (or CI job output) so reviewers can confirm the responsive
layout tests passed.
| */ | ||
| const CLAUDE_PATTERN = /^claude-(opus|sonnet|haiku)-(\d+)-(\d+)(?:-.+)?$/; | ||
|
|
||
| export function formatModelLabel(modelId: string): string { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a TSDoc block directly on the exported function.
formatModelLabel is exported but does not have a directly attached TSDoc/JSDoc comment.
Proposed fix
+/**
+ * Formats a raw model identifier into a short, human-friendly display label.
+ */
export function formatModelLabel(modelId: string): string {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function formatModelLabel(modelId: string): string { | |
| /** | |
| * Formats a raw model identifier into a short, human-friendly display label. | |
| */ | |
| export function formatModelLabel(modelId: string): string { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/lib/format-model-label.ts` at line 16, Add a TSDoc/JSDoc comment
block directly above the exported function formatModelLabel describing its
purpose in one concise sentence (minimum one-line summary) and optionally
include `@param` modelId and `@returns` tags; ensure the docblock is attached
immediately above the "export function formatModelLabel(modelId: string):
string" declaration so the function is properly documented per coding
guidelines.
Comment on lines
+1843
to
+1845
| // Server injects the model after persisting; defaults to null when | ||
| // unknown (legacy clients, non-Claude providers without model info). | ||
| model: (params.model as string | null | undefined) ?? null, |
There was a problem hiding this comment.
Propagate model when reconciling duplicate assistant messages.
When the Electron dual-channel dedupe path replaces the last assistant row, it updates id/tokens/timestamp but not model, so the footer can remain null/old even after receiving params.model.
Suggested fix
const replaced = state.messages.slice(0, -1).concat({
...last,
id: message.id,
+ model: message.model ?? null,
tokens_used: message.tokens_used,
timestamp: message.timestamp,
});Also applies to: 1913-1918
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/stores/threadStore.ts` around lines 1843 - 1845, When
reconciling/replacing the last assistant message in the dual-channel dedupe path
you update id/tokens/timestamp but fail to propagate the message model, so the
footer can remain stale; update the object mutation that replaces the assistant
row to also set model to (params.model as string | null | undefined) ?? null
(i.e., assign params.model normalized to null) and apply the same change in the
other similar reconciliation site noted around lines 1913-1918 so both code
paths propagate model consistently.
Remove speculative refusal from the final-response set and add provider and agent-service tests for AssistantMessageBoundary persistence paths. Co-authored-by: Cursor <cursoragent@cursor.com>
Match step and subagent counts to PersistedTurnFooter semantics, key the persisted narrative memo on messageContent, and exercise the production flat boundary event shape in tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop narrative console.debug calls, unreachable cursor code, the narrative HTML simulator, and document the thought vs response classification flow in the pipeline guide. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/components/chat/narrative/build-persisted-narrative.ts (1)
91-103: ⚡ Quick winReattach TSDoc directly above the exported function.
Line 102 exports
buildPersistedNarrativeItems, but its function docblock is no longer directly attached after the memo-cache block insertion.Suggested fix
+/** Build a chronological `NarrativeItem[]` from persisted DB records. */ export function buildPersistedNarrativeItems( inputs: PersistedNarrativeInputs, ): NarrativeItem[] {As per coding guidelines,
Always add JSDoc/TSDoc docstrings to all exported functions, components, types, and interfaces, including at minimum a one-line summary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/chat/narrative/build-persisted-narrative.ts` around lines 91 - 103, The TSDoc for the exported function buildPersistedNarrativeItems was left separated by the _memoCache block; move the existing docblock so it sits immediately above the export of buildPersistedNarrativeItems (directly attached to the function declaration) or add a one-line TSDoc summary if missing, ensuring the comment documents the exported function per guidelines and remains adjacent to buildPersistedNarrativeItems for tooling and linting to pick up.apps/server/src/__tests__/claude-provider-assistant-boundary.test.ts (1)
92-93: ⚡ Quick winReplace fixed sleeps with event-based synchronization.
The
setTimeout(10)waits are timing-sensitive and can flake in CI. Prefer awaiting a deterministic terminal signal (e.g.,AgentEventType.Endedfor the thread) before assertions.Suggested pattern
+function waitForEnded(provider: ClaudeProvider, threadId: string): Promise<void> { + return new Promise((resolve) => { + const onEvent = (e: { type: string; threadId?: string }) => { + if (e.type === AgentEventType.Ended && e.threadId === threadId) { + provider.off("event", onEvent); + resolve(); + } + }; + provider.on("event", onEvent); + }); +} ... - await new Promise((r) => setTimeout(r, 10)); + await waitForEnded(provider, "thread-boundary-final");Also applies to: 133-134, 173-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/__tests__/claude-provider-assistant-boundary.test.ts` around lines 92 - 93, Replace the timing-based sleeps (await new Promise((r) => setTimeout(r, 10))) with a deterministic event wait: instead of sleeping, await the thread/agent to emit the terminal event (AgentEventType.Ended) before proceeding with assertions; e.g., create or reuse a helper like waitForEvent(agentOrThread, AgentEventType.Ended) or convert the event callback into a Promise that resolves when AgentEventType.Ended fires and await that in the tests (apply the change for each occurrence including the ones around lines 133-134 and 173-174).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/guides/narrative-pipeline.md`:
- Line 72: Replace the sentence fragment "May be absent on tool-free turns;
boundary event wins." with a complete sentence that adds the missing
subject—e.g., "It may be absent on tool-free turns; boundary event wins."—so the
clause is unambiguous; update the text that currently reads "fired this turn.
May be absent on tool-free turns; boundary event wins." to use the corrected
phrasing.
---
Nitpick comments:
In `@apps/server/src/__tests__/claude-provider-assistant-boundary.test.ts`:
- Around line 92-93: Replace the timing-based sleeps (await new Promise((r) =>
setTimeout(r, 10))) with a deterministic event wait: instead of sleeping, await
the thread/agent to emit the terminal event (AgentEventType.Ended) before
proceeding with assertions; e.g., create or reuse a helper like
waitForEvent(agentOrThread, AgentEventType.Ended) or convert the event callback
into a Promise that resolves when AgentEventType.Ended fires and await that in
the tests (apply the change for each occurrence including the ones around lines
133-134 and 173-174).
In `@apps/web/src/components/chat/narrative/build-persisted-narrative.ts`:
- Around line 91-103: The TSDoc for the exported function
buildPersistedNarrativeItems was left separated by the _memoCache block; move
the existing docblock so it sits immediately above the export of
buildPersistedNarrativeItems (directly attached to the function declaration) or
add a one-line TSDoc summary if missing, ensuring the comment documents the
exported function per guidelines and remains adjacent to
buildPersistedNarrativeItems for tooling and linting to pick up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9127b9f-f422-490c-a99b-494c6b3c80d2
📒 Files selected for processing (12)
apps/server/src/__tests__/claude-provider-assistant-boundary.test.tsapps/server/src/providers/claude/claude-provider.tsapps/server/src/services/__tests__/agent-service-narrative-persist.test.tsapps/web/src/__tests__/threadStore-assistant-message-boundary.test.tsapps/web/src/__tests__/virtual-items.test.tsapps/web/src/components/chat/narrative/DeltaBlock.tsxapps/web/src/components/chat/narrative/NarrativeIndicator.tsxapps/web/src/components/chat/narrative/build-persisted-narrative.test.tsapps/web/src/components/chat/narrative/build-persisted-narrative.tsapps/web/src/components/chat/virtual-items.tsapps/web/src/stores/threadStore.tsdocs/guides/narrative-pipeline.md
💤 Files with no reviewable changes (2)
- apps/web/src/stores/threadStore.ts
- apps/web/src/components/chat/narrative/DeltaBlock.tsx
2 of 4 tasks
2 of 4 tasks
Merged
This was referenced Jun 11, 2026
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.
What
Rework the assistant-turn pipeline so user-facing text always reads as user-facing text. Replaces the legacy
THOUGHTchip / italic dimmed treatment fortextcontent blocks, fixes the/goalstop-hook flow where the actual answer was hidden as a preamble, and redesigns the narrative rendering around a quiet "actions molecule" sitting above prose.Also records the model identifier on every assistant message so the foot meta line can show which model produced the response, even after the user switches model mid-thread.
Why
The Claude provider relied on a heuristic (
hasFiredToolThisTurn && pendingToolUses == 0) to decide whether a text delta belonged to a thought or to the final response. The heuristic failed on:hasFiredToolThisTurnwasfalseand the entire response landed in the thought bucket/goalstop-hook pattern — the model writes the real answer first, runs a verification tool, then says "/goal clear when ready"; the answer ships withstop_reason: tool_useand was buried as preamble while the boilerplate at the end was treated as the "final response"Anthropic's own message-level
stop_reasonis the authoritative signal —end_turn/stop_sequence/max_tokens/refusalmark final responses,tool_usemarks preamble. This branch wires that signal through the provider and emits anAssistantMessageBoundaryAgentEvent that the agent-service (server) and threadStore (web) both consume to drop or close the open thought correctly.Pairs the data-layer fix with a UI redesign so all
textblocks render as body prose regardless ofstop_reason. The rendering distinction between preamble and final response goes away — both are agent voice, just at different chronological positions. Tools, hooks, and sub-agents form one tight "actions molecule" above the prose; the turn footer (step / sub-agent counts + duration) closes the turn AFTER the response body, not before.Key changes
Data layer (commit `34d201c`)
Mockup (commit `5ad3a72`)
UI redesign (commit `e17ffc9`)
Configuration changes
Test plan
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Improvements