feat: add collapsible thinking blocks with configurable display mode#24635
feat: add collapsible thinking blocks with configurable display mode#24635DanielleMaywood merged 19 commits intomainfrom
Conversation
DanielleMaywood
changed the title
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review (Netero). This is a mechanical scan only; the full review panel has not yet reviewed this PR.
The thinking-block UX is well-designed: the shimmer animation, chevron toggle, and mode-derived auto-expand/collapse layering are clean. Using the existing user_configs table and Collapsible primitive avoids unnecessary new infrastructure.
Findings: 1 P1, 1 P2, 1 P3, 1 Nit. The P1 and P2 are coverage gaps that should be addressed before the panel spends parallel review time.
P3 [DEREM-3] ConversationTimeline.stories.tsx: No stories exercise the new thinkingDisplayMode prop. ReasoningDisclosure gained four distinct behaviors (auto, preview, always_expanded, always_collapsed) including auto-scroll, height constraints, and collapse transitions. The existing stories test the placeholder text change but not the collapsible mode logic. (Netero)
"The handler uses correct HTTP status codes: 400 for invalid mode, 500 for DB errors. Errors are wrapped with descriptive messages." (Netero, noting the error handling is solid)
🤖 This review was automatically generated with Coder Agents.
Re: [DEREM-3] — Added |
…tail of the stream
… placeholder and disclosure
Adds a user preference for controlling how thinking blocks are displayed in chat. The preference is stored in the user_configs database table and exposed via the existing preferences API. Four modes are supported: - auto: expand during streaming, collapse when done (default) - preview: expand with height constraint during streaming - always_expanded: always show full thinking content - always_collapsed: always keep collapsed
…ok, use Radix Collapsible
- Skip thinking_display_mode DB write when field is empty, preventing older clients from silently resetting the stored preference - Lift useQuery from ReasoningDisclosure to BlockList, passing mode as a prop to avoid per-instance query subscriptions - Guard ThinkingDisplaySettings mutation behind query.data check - Use spread pattern consistently in TasksPage preference mutation - Switch auto-scroll from useEffect to useLayoutEffect to avoid visible frame lag in preview mode
- Add TestThinkingDisplayMode with 4 subtests: default auto, round-trip, invalid mode 400, empty mode preserves stored value - Add ThinkingBlockAlwaysExpanded and ThinkingBlockAlwaysCollapsed stories that mock the preference query to test display modes - Rename destructured prop from thinkingDisplayMode to mode directly
DanielleMaywood
force-pushed
the
dm/collapsible-thinking-blocks
branch
from
b1db438 to
ea55a95
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (12 reviewers). R1 findings DEREM-1, DEREM-3, DEREM-4 addressed. DEREM-2 (missing story) contested and closed by panel vote (5/5 accept the author's defense: 11 of 12 settings components in the directory have no story).
The collapsible thinking blocks are well-designed. The four-mode switch is clean, the preview height constraint with auto-scroll is a nice touch, and the SQL queries follow the established user_configs UPSERT pattern. The streaming placeholder shimmer is a clear UX improvement.
New findings: 1 P2, 4 P3, 1 P4, 1 Nit. The P2 is an API contract asymmetry that will bite new callers.
P3 [DEREM-8] ConversationTimeline.stories.tsx:1670: No stories for auto or preview modes. These are the two streaming-dependent modes with the most complex behavior: expand/collapse transitions, manual toggle reset, height constraints, and auto-scroll. The two added stories (ThinkingBlockAlwaysExpanded, ThinkingBlockAlwaysCollapsed) cover the static modes; the dynamic modes have zero assertion coverage. A bug where auto returned true unconditionally would pass every existing story. (Bisky P3, Nami P3, Meruem P3)
"Any new consumer of BlockList inherits the preference API dependency without asking for it. Silent correctness is the most fragile kind." (Hisoka)
DEREMs dropped: DEREM-10 (no transaction for two DB writes) follows the existing putUserAppearanceSettings pattern (theme + font, same structure). Not worth changing without a broader preference handler refactor.
🤖 This review was automatically generated with Coder Agents.
Re: DEREM-8 (no stories for auto/preview modes) Added |
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review, round 3 (4 reviewers: Mafuuu, Meruem, Pariston, Knov).
All R2 findings addressed or closed. DEREM-6 (P2, asymmetric PUT/PATCH semantics) contested and closed by panel vote (4/4 accept). The asymmetry is real, but: (a) all frontend callers correctly spread the full object, (b) the endpoint has no known external consumers, (c) the consequence (alert reappears) is an annoyance, not data loss, and (d) fixing it would change the API contract disproportionately to the risk. Pariston noted the defense's factual basis is imprecise (the single-field endpoint could not be asymmetric before this PR), but the practical consequence is tolerable.
No new findings from any reviewer. The R2->R3 fixes are all verified correct: mode-gated manualToggle reset (DEREM-7), exhaustive never check (DEREM-12), render-time setState pattern (DEREM-13), inline error feedback (DEREM-9), four mode stories (DEREM-8), dbauthz tests (DEREM-5).
"The
manualToggle ?? autoExpandedlayering cleanly separates user intent from mode-derived behavior." (Meruem)
14 findings across 3 rounds. 9 fixed, 2 contested and panel-closed, 1 author-accepted, 1 dropped, 1 note.
🤖 This review was automatically generated with Coder Agents.
…t silent zero-value resets
There was a problem hiding this comment.
Round 4 (Netero + 3 panel reviewers). Clean.
The author voluntarily fixed DEREM-6 (the *bool asymmetry the panel had already accepted in R3). TaskNotificationAlertDismissed is now *bool with omitempty, so nil means "not provided" and skips the DB write. Both preference fields now have symmetric skip-on-zero semantics. The change is correctly propagated across Go structs, generated TypeScript types, swagger docs, test call sites, and all frontend callers.
"Go's JSON decoder maps absent fields to nil and explicit false to *false, which gives the handler the signal it needs to distinguish 'not sent' from 'set to false.'" (Pariston)
14 findings across 4 rounds. 10 fixed, 2 panel-closed (contested), 1 author-accepted, 1 dropped. 0 open.
🤖 This review was automatically generated with Coder Agents.
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
A couple of comments below, but nothing blocking.
…corrupt DB values
3 participants
Note
🤖 This PR was written by Coder Agent on behalf of Danielle Maywood
Thinking blocks are now collapsible and support a user-configurable display mode stored in the database.
Changes
Collapsible thinking blocks (ConversationTimeline.tsx):
Streaming placeholder (StreamingOutput.tsx):
Configurable display modes (new
thinking_display_modeuser preference):auto(default): fully expands during streaming, auto-collapses when donepreview: auto-expands with max-height constraint during streaming, collapses when donealways_expanded: always shows full thinking contentalways_collapsed: always keeps collapsedBackend: New
GetUserThinkingDisplayMode/UpdateUserThinkingDisplayModequeries inuser_configstable, wired through the existing/api/v2/users/{user}/preferencesendpoint with validation.Settings UI: New "Thinking Display" dropdown on the Agent Settings > General page.
Implementation plan
See PLAN-731c3c5a-efcc-4b98-a241-e0ab49d77379.md in the workspace for the full plan.
Key design decisions:
user_configsDB table (not localStorage) for cross-device syncReasoningDisclosureusesmanualTogglestate (nullable) layered over mode-derivedautoExpanded, withuseEffectto reset manual override on streaming transitionsmax-h-24 overflow-hiddenonly when streaming and user hasn't manually toggledUpdateUserPreferenceSettingsRequestspread existing preferences to preserve the new field