Skip to content

fix(tui): thinking fold toggles relative to the expanded baseline - #5857

Open
Hmbown wants to merge 1 commit into
mainfrom
fix/thinking-expand-20260902
Open

fix(tui): thinking fold toggles relative to the expanded baseline#5857
Hmbown wants to merge 1 commit into
mainfrom
fix/thinking-expand-20260902

Conversation

@Hmbown

@Hmbown Hmbown commented Sep 2, 2026

Copy link
Copy Markdown
Owner

No-Issue: half of #5847 (one-cell truth-table fix + 8-cell test); the keyboard inline-expand half follows separately.

Half of #5847: one-cell truth-table fix plus 8-cell test; old + new fold tests pass. Keyboard inline expand is the other half and follows separately. Full matrix via CI.

The collapse rule was a triple-XOR (folded ^ !verbose ^ default_expanded)
whose own doc comment promised verbose-on means expanded, yet verbose-on
plus default-expanded rendered collapsed. Rule is now
folded ^ !(verbose || default_expanded): Space inverts the expanded
baseline, changing exactly one truth-table cell. Adds an 8-cell test.

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Copilot AI lite review requested due to automatic review settings September 2, 2026 20:55
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@cursor

cursor Bot commented Sep 2, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_e18cdf55-2877-4544-be79-18ebd5b8eab5)

@claude

claude Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 2m 40s —— View job


Code review — thinking fold toggle fix

  • Gather context (PR diff, changed files)
  • Understand the request (code review)
  • Review crates/tui/src/tui/history.rs changes
  • Review crates/tui/src/tui/history/tests.rs changes
  • Post final review

Summary

The core fix is correct. history.rs:436 replaces the triple-XOR
folded ^ !options.verbose ^ options.thinking_default_expanded with
folded ^ !(options.verbose || options.thinking_default_expanded). The old
formula canceled out when both verbose and thinking_default_expanded were
true (two trues XOR to false), silently collapsing a cell that should
render expanded — exactly the bug #5847 describes. I hand-verified all 8
truth-table rows against the new formula and they check out, matching the new
test's expectations.

The added test thinking_fold_toggle_is_relative_to_the_expanded_baseline
(history/tests.rs:545) exercises the full 2×2×2 matrix (folded, verbose,
thinking_default_expanded) directly against lines_with_options_folded,
including the previously-broken verbose=true, thinking_default_expanded=true
cell. It's a real regression test, not one that passes either way, and it
doesn't duplicate the existing reasoning_folds_in_live_and_the_fold_is_reversible
test, which never varies verbose.

Bug found — not fixed by this PR

crates/tui/src/tui/ui/event_loop.rs:97 computes the inverse of the
collapsed formula to decide what folded value to store in
app.folded_thinking when the user presses Space:

let folded = (!options.verbose ^ options.thinking_default_expanded)
    ^ (target.action == ReasoningAction::Collapse);

This still uses the old !verbose ^ thinking_default_expanded baseline.
Before this PR both the render side and this inverse used the same (buggy)
formula, so they were self-consistent even though both were wrong. Now that
history.rs uses !(verbose || thinking_default_expanded), the two sides
disagree exactly in the verbose=true, thinking_default_expanded=true case —
the very case this PR set out to fix.

Concretely: with verbose=true, thinking_default_expanded=true, and a cell
currently expanded (idx not in folded_thinking), pressing Space to
collapse it computes target.action == Collapse, giving
folded = (!true ^ true) ^ true = true ^ true = false. Since folded is
false, app.folded_thinking.remove(&idx) is a no-op and nothing gets
inserted — Space silently does nothing in this configuration.
app.folded_thinking is fed straight into lines_with_options_folded as
folded_cells (crates/tui/src/tui/widgets/mod.rs:363), so this is
reachable, not theoretical, and none of the existing handle_transcript_space
tests in crates/tui/src/tui/ui/tests.rs vary verbose, so it isn't caught
today.

This file isn't part of the diff, so GitHub won't let me leave an inline
comment on it — flagging here instead. Suggested fix, mirroring the new
render formula:

let expanded_baseline = options.verbose || options.thinking_default_expanded;
let folded = !expanded_baseline ^ (target.action == ReasoningAction::Collapse);

Fix this →

Other notes

  • Provider/model/route safety: not applicable — no provider/model routing
    code touched.
  • Reuse/simplification: no concerns; the change is minimal and doesn't
    introduce new abstractions.
  • Security: not applicable.
  • I was not able to run cargo check/cargo test in this environment (tool
    approval required for cargo invocations), so this review is based on manual
    code reading and truth-table verification rather than compiled/executed
    evidence. Worth confirming CI is green on the full matrix as the PR
    description states.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The Space-toggle path still computes the per-cell fold override using the old XOR baseline in handle_transcript_space, so the UI toggle can remain incorrect in the exact (verbose=true, default_expanded=true) case this PR aims to fix.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes the “thinking/reasoning” fold toggle semantics so that per-cell folding (Space) inverts a single expanded baseline rather than XOR-ing verbose and thinking_default_expanded independently, and adds a truth-table unit test to lock the intended behavior.

Changes:

  • Update thinking-cell collapsed/expanded computation to use baseline_expanded = verbose || thinking_default_expanded.
  • Update renderer documentation to describe the baseline behavior.
  • Add an 8-case unit test covering (folded, verbose, default_expanded) -> expanded.
File summaries
File Description
crates/tui/src/tui/history.rs Switch thinking collapse logic to invert a single “expanded baseline” (`verbose
crates/tui/src/tui/history/tests.rs Add an 8-case truth-table test asserting expanded/collapsed outcomes for folded toggle vs baseline flags.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

duration_secs,
} => {
let collapsed = folded ^ !options.verbose ^ options.thinking_default_expanded;
let collapsed = folded ^ !(options.verbose || options.thinking_default_expanded);

@codewhale-agent codewhale-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codewhale review

The PR fixes the thinking-cell fold baseline calculation by replacing the triple XOR with a single XOR against a combined baseline of verbose OR thinking_default_expanded. A new 8-case test covers the full truth table for folded, verbose, and default_expanded.

Assessment

The change is correct and well-tested. No blocking or non-blocking issues found.


Advisory review by Codewhale (codewhale review --pr 5857 --post, head bbd30ea667ea4f050136da1363383fa2b0c20118). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants