charles/agent-hooks

Fork 0

feat(web): render thinking blocks in planner transcript message bubbles #1193

Merged

dev merged 1 commit from dev/1165 into main

2026-05-15 07:05:42 +00:00

dev commented

2026-05-15 06:41:31 +00:00

Collaborator

Summary

Replaces the local ReasoningBlock component in planner/transcript.tsx with the shared <Reasoning> component from components/agent/reasoning.tsx
Streaming bubbles now show the pulse animation and auto-expand while the turn is actively streaming
Persisted turns load thinking blocks from m.thinking and render the collapsed accordion
streamThinkingStartedAt is extracted from the first assistant SSE event that carries thinking blocks, so the elapsed timer starts from when thinking actually began

Details

The <Reasoning> component was already extracted (#1165 AC: Extraction). This PR completes the transcript integration:

streaming prop wired through Bubble → Reasoning for pulse/auto-expand behaviour
thinkingStartedAt prop added to Bubble for accurate elapsed time on the live bubble; the Transcript component derives it from stream events
Persisted turns pass endedAt={ts} (message timestamp) as a safe fallback showing "Thought for 1s" since per-thinking-block duration isn't stored separately
ReasoningBlock local function removed (replaced entirely by shared component)

Closes #1165

Test plan

Open an architect session with extended thinking enabled; verify the Reasoning accordion appears below the streaming bubble with the pulse animation
Confirm the accordion collapses once streaming ends
Reload the page; verify persisted turns still show the collapsed Reasoning accordion
Confirm the event log Reasoning accordion is unchanged
just qa passes

## Summary - Replaces the local `ReasoningBlock` component in `planner/transcript.tsx` with the shared `<Reasoning>` component from `components/agent/reasoning.tsx` - Streaming bubbles now show the pulse animation and auto-expand while the turn is actively streaming - Persisted turns load thinking blocks from `m.thinking` and render the collapsed accordion - `streamThinkingStartedAt` is extracted from the first `assistant` SSE event that carries thinking blocks, so the elapsed timer starts from when thinking actually began ## Details The `<Reasoning>` component was already extracted (#1165 AC: Extraction). This PR completes the transcript integration: - `streaming` prop wired through `Bubble` → `Reasoning` for pulse/auto-expand behaviour - `thinkingStartedAt` prop added to `Bubble` for accurate elapsed time on the live bubble; the `Transcript` component derives it from stream events - Persisted turns pass `endedAt={ts}` (message timestamp) as a safe fallback showing "Thought for 1s" since per-thinking-block duration isn't stored separately - `ReasoningBlock` local function removed (replaced entirely by shared component) Closes #1165 ## Test plan - [ ] Open an architect session with extended thinking enabled; verify the Reasoning accordion appears below the streaming bubble with the pulse animation - [ ] Confirm the accordion collapses once streaming ends - [ ] Reload the page; verify persisted turns still show the collapsed Reasoning accordion - [ ] Confirm the event log Reasoning accordion is unchanged - [ ] `just qa` passes

dev added 1 commit

2026-05-15 06:41:31 +00:00

feat(web): render thinking blocks in planner transcript message bubbles

qa / dockerfile (pull_request) Successful in 10s

Details

qa / db-schema (pull_request) Successful in 14s

Details

qa / i18n-string-check (pull_request) Successful in 9s

Details

qa / sql-layer-check (pull_request) Successful in 6s

Details

qa / qa-1 (pull_request) Successful in 3m22s

Details

qa / qa (pull_request) Successful in 0s

Details

5310d18f1f

Replace the local ReasoningBlock in planner/transcript.tsx with the shared
<Reasoning> component from components/agent/reasoning.tsx. The accordion now
has pulse animation while streaming, auto-expands/collapses on streaming
state transitions, and shows an elapsed-time chip on persisted turns.

Streaming turns derive startedAt from the first assistant SSE event that
carries thinking blocks. Persisted turns use the message ts as both
startedAt and endedAt (renders "Thought for 1s" as a safe fallback since
per-thinking-block duration is not stored).

Closes #1165

Co-authored-by: Cursor <cursoragent@cursor.com>

dev requested review from reviewer

2026-05-15 06:47:34 +00:00

reviewer approved these changes

2026-05-15 06:51:21 +00:00

reviewer left a comment

Review — PR #1193: render thinking blocks in planner transcript bubbles

CI ✅ · mergeable ❌ (base branch conflict — rebase needed before merge)

Overview

Clean, focused integration of the shared <Reasoning> component into the planner transcript. The 40-line ReasoningBlock local function is deleted entirely; the replacement wires all the props needed for pulse animation, auto-expand-on-stream, and elapsed timer. No new abstractions, no scope creep.

What's correct

streamThinkingStartedAt IIFE — iterates streamEvents once, returns the ts of the first assistant event carrying thinking blocks, or undefined before any thinking arrives. The type cast (ev.detail as { thinking?: string[] } | undefined) is identical to the pattern in extractAssistantThinking, which is the right source to mirror. Returning undefined gracefully means the Reasoning accordion won't show a misleading "0s" before the first SSE event.

streaming ?? false coercion — BubbleProps.streaming is boolean | undefined; ReasoningProps.streaming requires boolean. The ?? false fallback is the correct narrowing.

endedAt={streaming ? undefined : ts} logic — correctly leaves endedAt undefined while the live counter should tick, then seals it with the render timestamp once streaming ends. The Reasoning component's live timer stops when endedAt is provided, which is exactly the desired transition.

Persisted turn fallback — passing startedAt={thinkingStartedAt ?? ts} with endedAt={ts} on persisted bubbles (where thinkingStartedAt is not provided) yields endedAt − startedAt = 0 → Math.max(1, 0) = 1s, so "Thought for 1s" always shows. That's documented, intentional, and correct given per-block duration isn't stored.

No orphaned imports — ChevronDown / ChevronRight survive the ReasoningBlock deletion because they're still used at line 647. Import order is Biome-compliant (@/components/agent/reasoning before @/components/agent-message).

No test regression — data-testid="reasoning-block" (old) has no usages in the test suite; data-testid="reasoning-accordion" (new, from the shared component) can be picked up by future tests without any cleanup.

Observations (non-blocking)

endedAt can drift on re-renders after streaming ends.
ts={Date.now()} on the streaming bubble is recomputed on every render, so endedAt={streaming ? undefined : ts} advances if the parent re-renders while the streaming bubble is still mounted (e.g. a late steer pill arrives after streaming=false). In practice the window is at most one or two render cycles before the persisted messages replace the streaming bubble, and Math.round absorbs sub-second jitter, so users won't notice. But it's worth knowing the timer isn't strictly frozen at end-of-stream. A useRef that latches Date.now() on the true→false transition would make this exact — worth a follow-up ticket if elapsed accuracy ever becomes a product requirement.

id is unstable for the streaming bubble.
id={\reasoning-${ts}`}wherets={Date.now()}changes every render. The_idunderscore inReasoningsignals it's currently unused internally, so there's no runtime impact. But theReasoningPropsJSDoc calls out that callers should key off event id for interleaved-thinking support (#951). Once interleaved thinking lands andidstarts mattering, the streaming bubble will need a stable key derived from the first thinking event's id rather thanDate.now()`. Fine to defer, just flag it before #951 merges.

No automated test for thinkingStartedAt wiring.
The Reasoning component itself has coverage from #1165; the new prop threading through Transcript → Bubble → Reasoning is test-plan manual-only. Given the simplicity of the wiring this is acceptable, but a smoke test asserting data-testid="reasoning-accordion" appears when streamEvents contains a thinking block would prevent regressions cheaply.

Verdict

All three observations are minor / theoretical / pre-existing. The implementation is correct, the shared component is wired up properly, just qa passes, and CI is green.

Approved. Rebase onto main to resolve the merge conflict, then this is ready to land.

## Review — PR #1193: render thinking blocks in planner transcript bubbles CI ✅ · mergeable ❌ (base branch conflict — rebase needed before merge) --- ### Overview Clean, focused integration of the shared `<Reasoning>` component into the planner transcript. The 40-line `ReasoningBlock` local function is deleted entirely; the replacement wires all the props needed for pulse animation, auto-expand-on-stream, and elapsed timer. No new abstractions, no scope creep. --- ### What's correct **`streamThinkingStartedAt` IIFE** — iterates `streamEvents` once, returns the `ts` of the first `assistant` event carrying thinking blocks, or `undefined` before any thinking arrives. The type cast (`ev.detail as { thinking?: string[] } | undefined`) is identical to the pattern in `extractAssistantThinking`, which is the right source to mirror. Returning `undefined` gracefully means the `Reasoning` accordion won't show a misleading "0s" before the first SSE event. **`streaming ?? false` coercion** — `BubbleProps.streaming` is `boolean | undefined`; `ReasoningProps.streaming` requires `boolean`. The `?? false` fallback is the correct narrowing. **`endedAt={streaming ? undefined : ts}` logic** — correctly leaves `endedAt` undefined while the live counter should tick, then seals it with the render timestamp once streaming ends. The `Reasoning` component's live timer stops when `endedAt` is provided, which is exactly the desired transition. **Persisted turn fallback** — passing `startedAt={thinkingStartedAt ?? ts}` with `endedAt={ts}` on persisted bubbles (where `thinkingStartedAt` is not provided) yields `endedAt − startedAt = 0 → Math.max(1, 0) = 1s`, so "Thought for 1s" always shows. That's documented, intentional, and correct given per-block duration isn't stored. **No orphaned imports** — `ChevronDown` / `ChevronRight` survive the `ReasoningBlock` deletion because they're still used at line 647. Import order is Biome-compliant (`@/components/agent/reasoning` before `@/components/agent-message`). **No test regression** — `data-testid="reasoning-block"` (old) has no usages in the test suite; `data-testid="reasoning-accordion"` (new, from the shared component) can be picked up by future tests without any cleanup. --- ### Observations (non-blocking) **`endedAt` can drift on re-renders after streaming ends.** `ts={Date.now()}` on the streaming bubble is recomputed on every render, so `endedAt={streaming ? undefined : ts}` advances if the parent re-renders while the streaming bubble is still mounted (e.g. a late steer pill arrives after `streaming=false`). In practice the window is at most one or two render cycles before the persisted messages replace the streaming bubble, and `Math.round` absorbs sub-second jitter, so users won't notice. But it's worth knowing the timer isn't strictly frozen at end-of-stream. A `useRef` that latches `Date.now()` on the `true→false` transition would make this exact — worth a follow-up ticket if elapsed accuracy ever becomes a product requirement. **`id` is unstable for the streaming bubble.** `id={\`reasoning-${ts}\`}` where `ts={Date.now()}` changes every render. The `_id` underscore in `Reasoning` signals it's currently unused internally, so there's no runtime impact. But the `ReasoningProps` JSDoc calls out that callers should key off event id for interleaved-thinking support (#951). Once interleaved thinking lands and `id` starts mattering, the streaming bubble will need a stable key derived from the first thinking event's id rather than `Date.now()`. Fine to defer, just flag it before #951 merges. **No automated test for `thinkingStartedAt` wiring.** The `Reasoning` component itself has coverage from #1165; the new prop threading through `Transcript → Bubble → Reasoning` is test-plan manual-only. Given the simplicity of the wiring this is acceptable, but a smoke test asserting `data-testid="reasoning-accordion"` appears when `streamEvents` contains a thinking block would prevent regressions cheaply. --- ### Verdict All three observations are minor / theoretical / pre-existing. The implementation is correct, the shared component is wired up properly, `just qa` passes, and CI is green. **Approved.** Rebase onto `main` to resolve the merge conflict, then this is ready to land.

dev force-pushed dev/1165 from 5310d18f1f

qa / dockerfile (pull_request) Successful in 10s

Details

qa / db-schema (pull_request) Successful in 14s

Details

qa / i18n-string-check (pull_request) Successful in 9s

Details

qa / sql-layer-check (pull_request) Successful in 6s

Details

qa / qa-1 (pull_request) Successful in 3m22s

Details

qa / qa (pull_request) Successful in 0s