Repository navigation
feat(agent): collapsible Reasoning block composed into agent-turn (CL-8482) - #47
Conversation
ReviewVerdict: request changes. ReasoningBlock collapsed by default; tests cover empty/toggle/streaming. No CI. No prior reviews. Blocking: hardcoded regionId = reasoning-block-content. Two AgentTurns with reasoning duplicate the id; both buttons aria-controls the first region. Use useId(). A uniqueness fix is in flight on this branch. |
Re-reviewVerdict: approve. SHA 1da98af. ReasoningBlock uses useId() for region id / aria-controls. Two-instance test added. bun test 340 pass, typecheck/lint/build 0. |
Review (Draper + Emil)Verdict: approve with copy nits. Collapsed-by-default including while streaming is the right call. Library strings: streaming “Thinking…”, idle “Thought about this”, aria “Reasoning.” Three names; idle fallback is generic-AI. Copy-only follow-up if we touch this PR. |
TheGreatAxios
left a comment
TheGreatAxios
left a comment
There was a problem hiding this comment.
Pushback on the shape of this one. ReasoningBlock is a near-duplicate of ActivityBlock — same chevron disclosure, same collapsed-while-streaming rationale, same bordered body — and this PR orphans ActivityBlock (still exported, zero consumers once agent-turn switches).
The doc comment's justification for dropping <details> doesn't match the implementation: it says the duration label "needs to sit next to the summary text without becoming part of it," but the label IS the summary text inside the button — <details><summary>Thought for 4s</summary> renders that identically.
The real delta is controlled open/onOpenChange — which ActivityBlock could gain additively (uncontrolled-by-default, the exact pattern the CL-5633 pass applied to ToolBlock, ToolNarrative, and friends). The button+region rewrite also loses what the codebase praised <details> for: find-in-page expansion and free disclosure semantics — {open ? <p> : null} means closed reasoning isn't in the DOM at all, so browser find can't match it.
Two honest options: fold durationLabel + controlled state into ActivityBlock (keeps <details>), or make this a clean rename that deletes ActivityBlock. Shipping both leaves two public components doing the same job.
Re-reviewIdle fallback is “Thinking”. SHA 9e4b776. bun test 341 pass. ActivityBlock still has the old phrase (out of this PR). |
…-8482)
Adds src/ui/reasoning-block.tsx, a keyboard-accessible disclosure for the
agent's thinking text: collapsed by default, an optional duration label
("Thought for 4s"), and a streaming state. AgentTurn now renders it in place
of the reasoning summary, fed by an optional reasoningDurationLabel prop —
rendering is unchanged when a turn has no reasoning text.
Gates run locally: dep-guard, typecheck, lint, build (incl. contrast gate),
bun test.
Hardcoded reasoning-block-content duplicated aria-controls when two AgentTurns mounted. Generate a per-instance id with useId and assert two blocks have distinct region ids matching their toggles.
Drop the "Thought about this" fallback so the term matches TypingIndicator. Duration label stays caller-supplied; streaming still uses Thinking…; aria region stays Reasoning.
9e4b776 to
cf0da24
Compare
Review pass — rebased onto post-prune
|
Summary
src/ui/reasoning-block.tsx: a collapsible "thought" block for the agent's reasoning text — collapsed by default, keyboard-accessible toggle (button +aria-expanded+ a labelled content region), an optional duration label ("Thought for 4s"), and a streaming state ("Thinking…").AgentTurnin place of the old reasoning summary, fed by the existingreasoningText()accessor, via a new optionalreasoningDurationLabelprop. Rendering is unchanged for a turn with no reasoning text.stories/primitives/reasoning-block.stories.tsx(collapsed, expanded, streaming, no-duration) and a red/green test for the toggle.Test plan
bun run dep-guardbun run typecheckbun run lintbun run build(incl. contrast gate)bun test