fix(tui): auto-hide thinking blocks when thinking level is off#3314
fix(tui): auto-hide thinking blocks when thinking level is off#3314oldschoola wants to merge 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcdd563d35
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * respects the user's intent when they set thinking to "off" (#626). | ||
| */ | ||
| get effectiveHideThinkingBlock(): boolean { | ||
| return this.hideThinkingBlock || (this.session?.thinkingLevel ?? ThinkingLevel.Off) === ThinkingLevel.Off; |
There was a problem hiding this comment.
In focused-agent mode, viewSession is the subagent transcript attached to the UI while session remains the main session. Because the render paths now consume this getter, a main session with thinking enabled will still show provider-returned thinking blocks for a focused subagent whose thinking level is off, and a main session with thinking off will hide a subagent whose thinking is enabled. Please base the effective flag on the transcript/session being rendered, e.g. viewSession.thinkingLevel for the focused live view.
Useful? React with 👍 / 👎.
| if (this.ctx.effectiveHideThinkingBlock && !this.ctx.hideThinkingBlock) { | ||
| this.ctx.showStatus("Thinking is off — enable thinking to show blocks"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When thinking is off but the persisted hideThinkingBlock preference is already true, this guard does not run because !this.ctx.hideThinkingBlock is false. Pressing Ctrl+T then flips the saved setting to false and applies that raw value to the existing/streaming assistant components, so the off state can immediately reveal provider-returned thinking blocks and corrupt the user's preference. Please check the actual thinking-off state before toggling, or apply the effective value to components.
Useful? React with 👍 / 👎.
roboomp
left a comment
roboomp
left a comment
There was a problem hiding this comment.
Rank: review:p1. Thank you @oldschoola — the read-only getter that combines user pref with session.thinkingLevel === Off is the right shape, the CHANGELOG entry is well-scoped, conventions are clean (no mock.module, no dynamic imports, star exports unaffected), and the toggle test is a real contract test rather than a tautology.
Two should-fix items before merge:
- Ctrl+T guard is half-covered (
input-controller.ts:1529). TheeffectiveHideThinkingBlock && !hideThinkingBlockpredicate is bypassed when the user pref is alreadytrue, so the toggle silently flips and persistshideThinkingBlocktofalsewhile blocks remain hidden — the exact corruption the code comment claims to prevent. Predicate onsession.thinkingLevel === ThinkingLevel.Offdirectly, and useeffectiveHideThinkingBlock(nothideThinkingBlock) at lines 1538 / 1543 to matchselector-controller.ts:321. Add the{ pref: true, thinking: off }regression case. thinking_level_changeddoes not propagate (event-controller.ts:120). Streaming + freshly-built components pick up the new effective value, but existingAssistantMessageComponentchildren inchatContainerdo not — so mid-session transitions tooffleave the prior turn's thinking blocks visible, contradicting the CHANGELOG. Either iterate children +resetDisplay()like thehideThinkingBlocksettings path, or narrow the CHANGELOG wording.
Open maintainer question: is the level-change-doesn't-retro-hide behaviour intentional (preserve content the user has already seen) or a missed propagation path?
| toggleThinkingBlockVisibility(): void { | ||
| // When thinking is "off", thinking blocks are always hidden (some | ||
| // providers return them regardless). The toggle is meaningless in | ||
| // that state — inform the user instead of silently no-op'ing. | ||
| if (this.ctx.effectiveHideThinkingBlock && !this.ctx.hideThinkingBlock) { | ||
| this.ctx.showStatus("Thinking is off — enable thinking to show blocks"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
should-fix. The guard only catches one half of the no-op case. With hideThinkingBlock=true and thinking level off, effectiveHideThinkingBlock && !hideThinkingBlock evaluates to true && false, so the early return is skipped and the toggle:
- flips
this.ctx.hideThinkingBlocktofalseand persists it viasettings.set("hideThinkingBlock", false)(line 1534) — exactly the "corrupting the persisted setting" the comment claims to prevent; - calls
child.setHideThinkingBlock(this.ctx.hideThinkingBlock)(line 1538) andstreamingComponent.setHideThinkingBlock(this.ctx.hideThinkingBlock)(line 1543) with the user pref instead of the effective value — diverging fromselector-controller.ts:321, which now passeseffectiveHideThinkingBlock; - shows
Thinking blocks: visibleeven though new components and streaming created viacreateAssistantMessageComponent/ thegetHideThinkingBlockcallback still hide (becauseeffectiveHideThinkingBlockis stilltrue).
Next press: effective && !hide is now true && true → guard fires and the user is locked out, with the setting silently flipped.
Simpler guard that covers both branches:
if (this.ctx.session?.thinkingLevel === ThinkingLevel.Off) {
this.ctx.showStatus("Thinking is off — enable thinking to show blocks");
return;
}(Or, equivalently, refuse whenever effectiveHideThinkingBlock is true for a reason other than the user pref — same idea.) A regression test should cover { hideThinkingBlock: true, effectiveHideThinkingBlock: true }; the existing new test only exercises { false, true }.
| this.#streamingReveal = new StreamingRevealController({ | ||
| getSmoothStreaming: () => this.ctx.settings.get("display.smoothStreaming"), | ||
| getHideThinkingBlock: () => this.ctx.hideThinkingBlock, | ||
| getHideThinkingBlock: () => this.ctx.effectiveHideThinkingBlock, |
There was a problem hiding this comment.
question / should-fix. Streaming reads effectiveHideThinkingBlock via this getter, and createAssistantMessageComponent consumes it at construction — but thinking_level_changed (handler at line 120-124) only invalidates the status line and editor border. There is no chatContainer.children walk and no rebuildChatFromMessages. Result: when the user transitions from thinking on → off mid-session (Ctrl+T cycle, /thinking, model selector, settings menu at selector-controller.ts:293-298), every already-rendered AssistantMessageComponent keeps its old hideThinkingBlock value and its thinking blocks remain visible — even though the CHANGELOG promises "thinking blocks are now auto-hidden when the thinking level is 'off', regardless of the hideThinkingBlock setting."
selector-controller.ts:317-327 (the user-pref change path) already iterates children + resetDisplay(). Is the divergence intentional (preserve historical content) or an oversight? If intentional, the CHANGELOG line is misleading. If not, the thinking_level_changed handler and the case "thinkingLevel" switch arm should mirror that pattern so the effective state is reflected retroactively.
|
Fixed in latest push. The guard now catches both no-op cases — when thinking is off, any toggle of hideThinkingBlock is a no-op regardless of the current persisted value. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9379d2e98d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // providers return them regardless). The toggle is meaningless in | ||
| // that state regardless of the persisted preference — inform the | ||
| // user instead of silently flipping the persisted value. | ||
| if (this.ctx.effectiveHideThinkingBlock) { |
There was a problem hiding this comment.
When thinking is enabled but the user has already hidden thinking blocks, effectiveHideThinkingBlock is still true because it also includes the saved hideThinkingBlock preference. Pressing Ctrl+T in that normal state now returns with “Thinking is off” and never flips the preference back to visible, so users can get stuck with hidden blocks unless they go through settings. The guard needs to distinguish “thinking level is off” from “blocks are currently hidden by preference.”
Useful? React with 👍 / 👎.
oldschoola
force-pushed
the
fix/thinking-blocks-hidden-when-off-626
branch
from
9379d2e to
5c8318d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c8318dce2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * respects the user's intent when they set thinking to "off" (#626). | ||
| */ | ||
| get effectiveHideThinkingBlock(): boolean { | ||
| return this.hideThinkingBlock || (this.session?.thinkingLevel ?? ThinkingLevel.Off) === ThinkingLevel.Off; |
There was a problem hiding this comment.
When the user changes the session thinking level to off after a message with visible thinking is already rendered, this computed value flips, but existing AssistantMessageComponents keep the hideThinkingBlock flag they were constructed or last toggled with. The thinking_level_changed handler only invalidates the status/border, so the current transcript can continue showing provider thinking until a full rebuild or new component; the off-level auto-hide needs to update/rebuild existing assistant blocks and any streaming component.
Useful? React with 👍 / 👎.
| getMessageRenderer: type => this.ctx.session.extensionRunner?.getMessageRenderer(type), | ||
| cwd: this.ctx.sessionManager.getCwd(), | ||
| hideThinkingBlock: () => this.ctx.hideThinkingBlock, | ||
| hideThinkingBlock: () => this.ctx.effectiveHideThinkingBlock, |
There was a problem hiding this comment.
This callback is passed through to AgentTranscriptViewer/ChatTranscriptBuilder and evaluated while rebuilding the selected agent transcript, but it closes over the main interactive session. In the Agent Hub path, opening an advisor transcript while the main session is off hides advisor reasoning even though advisors default to their own thinking level, and the inverse can reveal provider thinking for a displayed transcript whose own level is off; derive the effective visibility from the transcript/agent being displayed rather than the main session.
Useful? React with 👍 / 👎.
|
Fixed in the latest push. The guard now checks Previous issue: Fix: The guard checks only the thinking level: const thinkingOff = (this.ctx.session?.thinkingLevel ?? ThinkingLevel.Off) === ThinkingLevel.Off;
if (thinkingOff) { ... return; }This means:
Tests updated to set |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30e0ec0200
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // that state — inform the user instead of silently flipping the | ||
| // persisted value. When thinking is on, the toggle works normally | ||
| // even if blocks are already hidden (user may want to show them). | ||
| const thinkingOff = (this.ctx.session?.thinkingLevel ?? ThinkingLevel.Off) === ThinkingLevel.Off; |
There was a problem hiding this comment.
In focused-agent mode, ctx.session remains the main session while ctx.viewSession is the transcript being controlled. This guard therefore blocks Ctrl+T for a focused subagent with thinking enabled whenever the main session is off, and allows the toggle when the focused subagent is off but the main session is on; the latter then applies the raw hideThinkingBlock value to displayed components and can reveal provider thinking that should stay hidden. Base the guard on the viewed session's thinking level instead.
Useful? React with 👍 / 👎.
…viewSession Address 4 P2 review comments on PR can1357#3314: 1. effectiveHideThinkingBlock now uses viewSession.thinkingLevel instead of session.thinkingLevel — in focused-agent mode, the viewed transcript may have a different thinking level than the main session. 2. thinking_level_changed handler now iterates existing AssistantMessageComponent children and calls setHideThinkingBlock with the new effective value, then resetDisplay() to repaint. Previously only new/streaming messages got the updated visibility. 3. Changed AssistantMessageComponent import from type-only to value import for instanceof check. 4. Agent Hub callback already uses effectiveHideThinkingBlock which now reflects viewSession — no separate fix needed.
|
Addressed 4 P2 review comments in latest push (
Changed |
…viewSession Address 4 P2 review comments on PR can1357#3314: 1. effectiveHideThinkingBlock now uses viewSession.thinkingLevel instead of session.thinkingLevel — in focused-agent mode, the viewed transcript may have a different thinking level than the main session. 2. thinking_level_changed handler now iterates existing AssistantMessageComponent children and calls setHideThinkingBlock with the new effective value, then resetDisplay() to repaint. Previously only new/streaming messages got the updated visibility. 3. Changed AssistantMessageComponent import from type-only to value import for instanceof check. 4. Agent Hub callback already uses effectiveHideThinkingBlock which now reflects viewSession — no separate fix needed.
oldschoola
force-pushed
the
fix/thinking-blocks-hidden-when-off-626
branch
from
1471d2e to
7b80931
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b80931e1b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (this.ctx.streamingComponent && this.ctx.streamingMessage) { | ||
| this.ctx.streamingComponent.setHideThinkingBlock(hideThinking); | ||
| this.ctx.streamingComponent.updateContent(this.ctx.streamingMessage); |
There was a problem hiding this comment.
When thinking is changed to off while a smooth-streaming assistant block is mid-reveal, this only mutates the AssistantMessageComponent; StreamingRevealController caches #hideThinkingBlock in begin/setTarget, and its timer's #tick() can keep rendering with the stale value until the next message_update or message_end. That means provider-returned thinking can reappear immediately after this reset despite the off state. Fresh evidence beyond the earlier stale review is that the new handler now updates streamingComponent directly but still does not stop/restart or otherwise resync the reveal controller.
Useful? React with 👍 / 👎.
Some providers (MiniMax, GLM, DeepSeek) return thinking blocks in
their responses even when reasoning is disabled — the model generates
thinking content regardless of the reasoning_effort parameter.
When the user sets thinking level to "off", they expect no thinking
content to be visible. Previously, thinking blocks would still appear
because the hideThinkingBlock setting was independent of the thinking
level and defaulted to false (show).
Fix: add effectiveHideThinkingBlock computed property that returns
true when hideThinkingBlock is true OR the session thinking level is
"off". All render paths (streaming, transcript rebuild, component
construction) now read the effective value instead of the raw setting.
The toggle (Ctrl+T) is guarded: when thinking is off, it shows a
status message ("Thinking is off — enable thinking to show blocks")
instead of silently no-op'ing or corrupting the persisted setting.
Fixes can1357#626
…ingBlock The previous guard used effectiveHideThinkingBlock which combines hideThinkingBlock preference AND thinking-off state. When thinking was enabled but hideThinkingBlock was true (user manually hid blocks), the guard blocked the toggle, preventing users from showing blocks again via Ctrl+T. Fix: check session.thinkingLevel === Off directly, so the guard only fires when thinking is genuinely off. When thinking is on, the toggle works normally regardless of the current hideThinkingBlock preference. Updated tests to set thinkingLevel on the mock context.
…viewSession Address 4 P2 review comments on PR can1357#3314: 1. effectiveHideThinkingBlock now uses viewSession.thinkingLevel instead of session.thinkingLevel — in focused-agent mode, the viewed transcript may have a different thinking level than the main session. 2. thinking_level_changed handler now iterates existing AssistantMessageComponent children and calls setHideThinkingBlock with the new effective value, then resetDisplay() to repaint. Previously only new/streaming messages got the updated visibility. 3. Changed AssistantMessageComponent import from type-only to value import for instanceof check. 4. Agent Hub callback already uses effectiveHideThinkingBlock which now reflects viewSession — no separate fix needed.
oldschoola
force-pushed
the
fix/thinking-blocks-hidden-when-off-626
branch
from
7b80931 to
f8ec048
Compare
Address P2 review: StreamingRevealController caches #hideThinkingBlock in begin()/setTarget(). When thinking level changes mid-stream, the timer's #tick() kept rendering with the stale cached value, causing provider-returned thinking to reappear after the off state. Add resyncVisibility() method to StreamingRevealController that: - Re-reads #hideThinkingBlock and #proseOnlyThinking from getters - Recalculates visible units (hiding thinking reduces total) - Clamps #revealed to new total (prevents over-revealing) - Re-renders with #renderCurrent() using updated values - Resyncs timer via #syncTimer() Call resyncVisibility() in the thinking_level_changed handler instead of streamingComponent.updateContent().
|
Addressed the new P2 review in latest push ( Refresh streaming reveal state after hiding thinking: Added |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0dfa631ce
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.ctx.streamingComponent.setHideThinkingBlock(hideThinking); | ||
| this.#streamingReveal.resyncVisibility(); | ||
| } | ||
| this.ctx.ui.resetDisplay(); |
There was a problem hiding this comment.
When defaultThinkingLevel is auto, AgentSession.#applyAutoThinkingLevel emits thinking_level_changed for normal effort changes such as provisional/high to resolved/medium even though effectiveHideThinkingBlock remains false. This unconditional resetDisplay() turns those routine auto-classification updates into a full terminal replay/scrollback reset on direct terminals, which is disruptive and unnecessary when the hidden/visible state did not actually change; please gate the full reset on an actual effective visibility transition and use a normal render for effort-only changes.
Useful? React with 👍 / 👎.
Address P2 review: auto-classification (e.g. high→medium) emits thinking_level_changed without changing effectiveHideThinkingBlock. The unconditional resetDisplay() turned routine auto-classification updates into a full terminal replay on direct terminals. Track #prevHideThinking and only call resetDisplay() when the effective visibility actually changed. For effort-only changes, use a lightweight requestRender() instead.
|
Addressed new P2 review in latest push ( Avoid resetting display when visibility is unchanged: Added |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 705a2636fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| #lastTtsrNotification: TtsrNotificationComponent | undefined = undefined; | ||
| #streamingReveal: StreamingRevealController; | ||
| #toolArgsReveal: ToolArgsRevealController; | ||
| #prevHideThinking = false; |
There was a problem hiding this comment.
When a session starts or is resumed with thinking level off, existing assistant components are created with effectiveHideThinkingBlock === true, but this new baseline is still initialized to false. If the user then enables thinking, the new effective value is also false, so the thinking_level_changed guard treats it as unchanged and returns without calling setHideThinkingBlock(false) or replaying the transcript; previously rendered thinking stays hidden until a full rebuild. Fresh evidence beyond the earlier stale-propagation comment is that the added #prevHideThinking state is not initialized from the current effective visibility.
Useful? React with 👍 / 👎.
2 participants
Problem
Some providers (MiniMax, GLM, DeepSeek) return thinking blocks in their responses even when reasoning is disabled — the model generates thinking content regardless of the
reasoning_effortparameter. When the user sets thinking level to "off", thinking blocks would still appear in the UI becausehideThinkingBlockwas independent of the thinking level and defaulted tofalse(show).Fixes #626
Changes
effectiveHideThinkingBlockcomputed property onInteractiveMode(and theInteractiveModeContextinterface) that returnstruewhen the user'shideThinkingBlocksetting is on OR the session thinking level is"off".effectiveHideThinkingBlockinstead ofhideThinkingBlock:event-controller.ts— streaming previewinteractive-context-helpers.ts— transcript component constructionselector-controller.ts— agent hub transcript viewer + settings change handlerCtrl+T): when thinking is off, it shows "Thinking is off — enable thinking to show blocks" instead of silently no-op'ing or corrupting the persisted setting.Design
The
hideThinkingBlocksetting stays as the user's preference (not mutated by the thinking level). TheeffectiveHideThinkingBlockgetter combines the two without state mutation, so:Tests
input-controller-thinking-visibility.test.ts: existing toggle test updated witheffectiveHideThinkingBlockmock + new test asserting the toggle refuses and shows a status message when thinking is offbun checkpasses (all 16 packages)