feat(tui): Ctrl+T cycle + divider display for reasoning effort - #229
Conversation
Reasoning effort was already fully wired (per-model supported lists, DefaultReasoningEffort, the EffectiveReasoningEffort clamp, /effort and /mode), but it was invisible in the UI and reachable only by typing a command. Add the opencode-style one-key fast path: - Ctrl+T cycles the active model's effort ring (auto -> low -> medium -> high -> auto), gated by the same modal condition shift+tab uses and a silent no-op on models with no effort controls. - The composer divider shows the active effort in the brand lime when set; the segment is omitted on "auto", so its presence/absence is itself the auto-state feedback. Zero per-frame registry lookups: the cycle runs only on the rare keypress and the divider branches purely on m.reasoningEffort != "" (DefaultRegistry rebuilds the whole catalog on every call, so it must never be touched from the render path). Tests cover every cycle branch (auto->first, mid-ring advance, last->auto wrap, unknown->auto, no-op on unsupported model) and the divider show/omit paths. Co-Authored-By: Claude <noreply@anthropic.com>
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds ChangesReasoning Effort Cycling and Composer Description Hints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/tui/session_controls_test.go (1)
102-107: ⚡ Quick winUse a truly unknown effort token in the unknown-reset test.
ReasoningEffortMinimalis currently unsupported for this model, but that can change with registry updates and make this test fail for the wrong reason. Use an invalid sentinel so this path always validates “unplaceable effort resets to auto.”Suggested test hardening
func TestCycleReasoningEffortUnknownResetsToAuto(t *testing.T) { m := newModel(context.Background(), Options{ModelName: "claude-sonnet-4.5"}) - // minimal is a valid ReasoningEffort but not in claude-sonnet-4.5's supported - // set, so the ring can't place it — cycle falls back to auto rather than guess. - m.reasoningEffort = modelregistry.ReasoningEffortMinimal + // Use a sentinel that is guaranteed to be unplaceable in any supported ring. + m.reasoningEffort = modelregistry.ReasoningEffort("__unknown_effort__") next, _ := m.cycleReasoningEffort() if next.reasoningEffort != "" { t.Fatalf("expected unknown effort to reset to auto, got %q", next.reasoningEffort) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/session_controls_test.go` around lines 102 - 107, In the TestCycleReasoningEffortUnknownResetsToAuto test, replace the assignment of m.reasoningEffort with modelregistry.ReasoningEffortMinimal with a truly invalid or unknown sentinel value instead. This ensures the test reliably validates that unplaceable effort tokens reset to auto, independent of future registry updates that might add support for ReasoningEffortMinimal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/model.go`:
- Around line 743-752: The Ctrl+T handler gating condition (line 743) is missing
a check for m.transcriptDetailed that the Shift+Tab handler uses (line 730-733),
causing effort cycling to be allowed in transcript mode when it should be
blocked. Add a check for m.transcriptDetailed == nil to the conditional gate
protecting the cycleReasoningEffort() call in the Ctrl+T case, aligning it with
the same gating pattern used by Shift+Tab to prevent state changes while in
transcript detailed mode.
---
Nitpick comments:
In `@internal/tui/session_controls_test.go`:
- Around line 102-107: In the TestCycleReasoningEffortUnknownResetsToAuto test,
replace the assignment of m.reasoningEffort with
modelregistry.ReasoningEffortMinimal with a truly invalid or unknown sentinel
value instead. This ensures the test reliably validates that unplaceable effort
tokens reset to auto, independent of future registry updates that might add
support for ReasoningEffortMinimal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 507b1868-c894-40c0-aaeb-44275322e481
📒 Files selected for processing (5)
internal/tui/model.gointernal/tui/rendering_lime_test.gointernal/tui/session_controls.gointernal/tui/session_controls_test.gointernal/tui/view.go
- Block Ctrl+T effort cycling inside the detailed transcript (matches the shift+tab gate) so state changes don't happen while the user is reading a frozen view. - Use a guaranteed-unplaceable sentinel in the unknown-effort reset test instead of ReasoningEffortMinimal, so the test stays correct if Minimal becomes a supported ring level later. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review — Ctrl+T reasoning-effort cycle + divider
Nice, self-contained feature. It adds a Ctrl+T shortcut that walks the active model's supported reasoning-effort ring (auto → low → medium → high → auto) and surfaces the current value in the composer divider. Builds clean and the new tests pass locally (go test ./internal/tui/ -run 'CycleReasoningEffort|ComposerDivider').
Strengths
cycleReasoningEffortis a tidy ring-walk and the test table covers every branch — auto-start, mid-ring advance, last-slot wrap, an unknown/unsupported effort resetting to auto, and a true no-op on a model with no effort controls.- Good awareness of the render hot path: both
cycleReasoningEffortand the divider call out thatDefaultRegistry()must not run per-frame, and the divider correctly does no registry lookup. - The divider segment is width-safe — it rides on
meta, so it drops out together under the narrow-width fallback (view.go:185) rather than overflowing. - Hiding the segment on auto (
reasoningEffort == "") makes its appearance/disappearance the state feedback. Clean.
Suggestion (the one thing I'd change)
The 7-term modal gate is now duplicated verbatim between the existing shift+tab case and the new Ctrl+T case:
if m.pendingPermission == nil && m.pendingAskUser == nil && m.pendingSpecReview == nil &&
m.providerWizard == nil && m.mcpAddWizard == nil && m.mcpManager == nil && m.picker == nil {The Ctrl+T comment even says "the same gate shift+tab uses above." Two copies will drift the day a new modal is added to one and not the other. Worth extracting a single predicate and using it in both places, e.g.:
// noBlockingModal reports that no modal surface (permission prompt, ask_user,
// spec review, provider/MCP wizard, MCP manager, or picker) is up, so a global
// shortcut may act instead of falling through to a modal's own handler.
func (m model) noBlockingModal() bool {
return m.pendingPermission == nil && m.pendingAskUser == nil && m.pendingSpecReview == nil &&
m.providerWizard == nil && m.mcpAddWizard == nil && m.mcpManager == nil && m.picker == nil
}Then both cases read if m.noBlockingModal() {. Pure refactor, no behavior change.
Minor / optional
cycleReasoningEffortreturns(model, tea.Cmd)but the cmd is alwaysnil. Fine for thereturn m.cycleReasoningEffort()call site (it matches the(tea.Model, tea.Cmd)shape), just noting it never produces a command.- Discoverability: the binding isn't in any help/keymap hint. The divider shows the state but not that Ctrl+T sets it — consider adding it to the shortcuts list so users can find it.
Verdict
Solid and low-risk, with genuinely thorough tests. Only the duplicated modal gate is worth folding into a shared helper before merge; everything else is optional polish.
The 7-term modal gate was duplicated verbatim between the shift+tab and Ctrl+T cases. Fold it into a single noBlockingModal() helper so the two shortcuts can't drift the day a new modal is added to one and not the other. Pure refactor, no behavior change. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The divider showed whatever m.reasoningEffort was set to, even when the active model had no effort controls (e.g. gpt-4.1). Switching from claude-sonnet-4.5 (low/medium/high) to gpt-4.1 would leave a stale "high" floating in the divider even though it has no effect. Make the segment selective: only render it when the active model exposes availableReasoningEfforts, matching the same appearing/disappearing state feedback the divider already uses for the auto case. The lookup hits a hard-coded static catalog, not a per-frame rebuild, so it's safe on the render path. handleModelCommand already resets an unsupported effort preference on model switch, so no changes are needed there. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…eady selective The previous commit added `&& len(m.availableReasoningEfforts()) > 0` to the divider's effort-segment gate so a stale value couldn't be shown after switching to a model without effort controls. Reverting that change: the underlying state mutation is already gated elsewhere (handleModelCommand clears an unsupported preference on switch, and the /effort picker only opens for models that actually expose effort controls), so the divider doesn't need its own check. The /effort picker (newEffortPicker, pickerEffort kind) is already fully wired and tested — /effort with no args on a supported model opens the picker; on a model with no effort controls, the text path returns "Active model does not expose reasoning effort controls." Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…trols Previously newEffortPicker returned nil for any model not in the hard-coded catalog (e.g. ollama-cloud providers serving glm-5.1 or similar). The case commandEffort dispatch fell through to handleEffortCommand, which rendered a grey "Effort / status: warning / available: none for active model" status card -- the very static panel users were reporting. Now newEffortPicker always returns a picker: a single "auto" option when the active model exposes no effort controls, or "auto" plus the model's known efforts otherwise. handleEffortCommand already returns the "Active model does not expose reasoning effort controls" message if any non-auto value is later set, so no other gate is needed. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Claude-code-style: when the user types a slash command that resolves to exactly one command in the autocomplete palette, surface that command's description on a muted line just below the composer box. Multiple matches keep the existing dropdown as the right affordance; once the user starts typing arguments the hint clears; the @file palette keeps its own rows and the hint stays scoped to slash commands. The inline argument hint ([low|medium|high|auto]) still renders inside the composer box after the slash, so both UIs coexist. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/tui/picker_test.go (1)
682-686: ⚡ Quick winUse a stable fake model ID for “unsupported effort” tests.
Line 685 and Line 704 rely on
glm-5.1staying outside the registry; if that changes, these tests will fail for the wrong reason. Prefer a sentinel test-only model ID.Suggested refactor
+const unsupportedEffortModel = "test-unsupported-effort-model" func TestEffortPickerOpensForModelWithoutEffortControls(t *testing.T) { @@ - m := newModel(context.Background(), Options{ModelName: "glm-5.1"}) + m := newModel(context.Background(), Options{ModelName: unsupportedEffortModel}) @@ } func TestEffortPickerAutoSelectionKeepsEffortUnset(t *testing.T) { @@ - m := newModel(context.Background(), Options{ModelName: "glm-5.1"}) + m := newModel(context.Background(), Options{ModelName: unsupportedEffortModel})Also applies to: 704-705
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/picker_test.go` around lines 682 - 686, The tests use the hardcoded model ID "glm-5.1" to test unsupported effort scenarios, but if this model is added to the registry in the future, these tests will fail for the wrong reason. Replace the "glm-5.1" model ID in both test cases (at the newModel calls on line 685 and line 704 where ModelName is set) with a stable sentinel test-only model ID that will never exist in the registry. This ensures the tests continue to verify the correct behavior regardless of future registry changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/model.go`:
- Around line 1965-1967: The validation logic for slash-token input is checking
for whitespace characters after trimming the input with strings.TrimSpace(),
which means trailing whitespace is never detected. Move the whitespace
validation check to occur before trimming the input value, or apply the
strings.ContainsAny check directly to the raw m.input.Value() instead of the
trimmed value. This ensures that inputs with trailing whitespace like /effort
are properly rejected by the condition on line 1966 before any trimming occurs.
---
Nitpick comments:
In `@internal/tui/picker_test.go`:
- Around line 682-686: The tests use the hardcoded model ID "glm-5.1" to test
unsupported effort scenarios, but if this model is added to the registry in the
future, these tests will fail for the wrong reason. Replace the "glm-5.1" model
ID in both test cases (at the newModel calls on line 685 and line 704 where
ModelName is set) with a stable sentinel test-only model ID that will never
exist in the registry. This ensures the tests continue to verify the correct
behavior regardless of future registry changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 195219a1-2591-402e-8a63-dea4441a8c95
📒 Files selected for processing (4)
internal/tui/model.gointernal/tui/picker.gointernal/tui/picker_test.gointernal/tui/rendering_lime_test.go
The /effort command used to push its status text through renderCommandOutput,
which renders as a thin grey panel (faint text on panel background, neutral
border). Replace that path with renderCommandCardTranscript so the result
renders the same way as /tools, /mcp, /permissions, etc.: a lime-bordered
titled card with a key/value "State" section and a footer of next-step actions.
Three call sites updated:
- handleEffortCommand("auto") -> sets effort to auto, shows confirmation card
- handleEffortCommand(<value>) -> sets effort, shows confirmation card
- handleEffortCommand(<bad>) -> unknown / unsupported / not exposed -> shows
the same card with the relevant detail line
effortText() (the no-arg / list / picker-dismiss path) follows the same
pattern. The "status: warning" line and grey panel are gone; the warning
case instead keeps the lime accent and surfaces "no reasoning controls on
this model" in the summary, matching the rest of the TUI's visual language.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
@coderabbitai review |
|
@coderabbitai approve |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review — reasoning-effort UX (Ctrl+T, divider, /effort card, hint)
This grew from the single Ctrl+T shortcut into a cohesive reasoning-effort experience, and it directly addressed my earlier note. Builds clean and the targeted tests pass locally (-run 'Effort|CycleReasoning|ComposerDescriptionHint|ComposerDivider').
Strengths
noBlockingModal()is extracted and shared by both shift+tab and Ctrl+T — exactly the refactor I asked for. The 7-term gate now lives in one place, so the two shortcuts can't drift.- The divider's unconditional effort render is actually safe. Every path that sets
reasoningEffortgates or clears against the active model's supported set —handleEffortCommand,cycleReasoningEffort, the mode command, and crucially the model switch atcommand_center.go:434(clears the effort if the new model doesn't allow it). So a stale effort can't appear, and the comment matches the code. Nice reasoning for dropping the model-aware gate. cycleReasoningEffort/reasoningEffortIndexare clean, with a good caveat about not calling the registry lookup from the render path.- The
/effortcommand-card migration is consistent, and a no-control model still gets a real card with the "none for active model" hint rather than a grey status panel. - Genuinely thorough tests: every cycle branch, the picker-on-unsupported-model, divider show/omit, and the description hint across single / ambiguous / args / @file-palette cases.
Suggestions
- (Confirms CodeRabbit's open thread —
model.gocomposerDescriptionHint.)value := strings.TrimSpace(m.input.Value())runs before the whitespace check, so/effort(trailing space — argument entry has begun) still renders the description. Test the raw value for the space (e.g.strings.ContainsAny(m.input.Value(), " \t\n"), orstrings.HasSuffix(m.input.Value(), " ")) so the hint clears the instant an argument starts — matching the intent ofTestComposerDescriptionHintStaysEmptyAfterArgs. newEffortPicker: theif m.reasoningEffort == "" { selected = 0 }is a no-op (selectedis already0). Drop it.- Optional: a one-item
[auto]picker on a no-control model is a slightly unusual affordance (a popup with nothing to pick but auto). It's a reasonable consistency choice — just flagging it in case the inline status reads better there.
Verdict
Solid and well-tested, and it folded in the earlier feedback. Only the trailing-whitespace nit (#1) is worth fixing before merge; the rest is optional polish.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approve ✅
Solid, well-tested, and it folded in my earlier noBlockingModal() feedback — the shared modal gate is exactly right. I verified it builds and the targeted tests pass, and confirmed the divider's unconditional effort render is safe (every effort-setting path gates/clears against the active model, including the model switch at command_center.go:434, so no stale effort can show).
Non-blocking follow-up: the trailing-whitespace nit in composerDescriptionHint (also flagged by CodeRabbit) — checking the raw input so /effort clears the hint — is worth a quick fix, but it's cosmetic and shouldn't hold the PR. The other two items in my review comment are optional polish.
Nice work.
✅ Action performedReview finished.
|
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review: Ctrl+T effort cycle + divider display
Solid, idiomatic Bubble Tea v2 work. Verified locally: gofmt -l clean, go vet clean, and the new tests pass under -race. No key collision (Ctrl+T is unused elsewhere and reuses the same noBlockingModal() gate as Shift+Tab), the effort cycle is correct on every branch (empty→first, mid→next, last/unknown→reset to auto), and the divider/hint gating is sound. Good test coverage on the cycle and rendering.
Two non-blocking nits:
internal/tui/picker.go(newEffortPicker) —selected := 0immediately followed byif m.reasoningEffort == "" { selected = 0 }is a no-op (it's already0). Drop theif.internal/tui/model.go:2294—newEffortPickeralways returns a non-nil*commandPicker, so theif picker := m.newEffortPicker(); picker != nilguard is always true and the nil branch is dead. Either simplify the guard, or returnnilfor models with no effort controls if that was the intent.
Optional: one Update-level test asserting Ctrl+T actually mutates effort (and is a no-op while a modal is open) would close the last small gap — current tests call cycleReasoningEffort directly.
Approving — the nits are cosmetic.
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
2 participants
Summary by CodeRabbit
Release Notes