feat(tui): add thinking effort, mcp status, and tab agent hints - #39
Conversation
|
Before and after screenshots look super gud btw, helps a lot with the review. Did you automate it or just manual. If automate, did u use something like before-and-after + cua-driver? PR Review — #39Branch: Merge confidence: 3/5Solid, scoped TUI parity work with helper tests and a clean merge. Confidence is capped because the default-reasoning change also feeds live LLM requests, GitHub CI does not run tests, and GitHub commentHome parity looks good: default reasoning effort, per-model overrides, display names, MCP counts, and the One thing to confirm before merge:
No migrations. I ran What changed+711 / −29 across 7 files. No persistence, config schema, or aisdk changes.
RegressionsNo obvious display regressions. Tab already cycles agents ( Possible behavior change (not just UI): // before
let requested = self.reasoning_effort_override_for_model(...)?;
let resolved = capability.resolve(Some(requested))?;
// after
let requested = self.reasoning_effort_override_for_model(...);
let resolved = capability.resolve(requested)?;
MCP status edge cases (not a main-path regression if MCP is present at startup):
Existing MigrationsNo. Nothing in Merge checklist
Confidence bumps
The reasoning-request confirmation is the highest-leverage item. Everything else is polish / edge-case coverage. Checks I ran (non-mutating)Worktree at
Not run: full suite, clippy, interactive TUI smoke. |
|
Thanks for the thorough review! The before/after screenshots were captured and arranged manually — I did not use The default reasoning behavior is intentional: when there is no explicit override, the model catalog/config default should be used by both the UI and the actual request path. This keeps the displayed variant consistent with what we send to the provider. Good call on the missing coverage; I’ll add an app-level regression test for no override → capability default. The MCP findings are also valid. I’ll follow up before merge to:
Thanks as well for running the focused checks and documenting the remaining confidence gaps so clearly. |
|
Thanks again for the detailed review. I pushed
Validation completed:
Would appreciate a re-review when you have time. |
PR Review — #39 (re-review)Branch: Merge confidence: 5/5 (was 4/5 → 3/5 originally)
Follow-up addresses the review. Thanks.
I re-ran fmt, diff-check, merge-tree, and the focused tests (including the new ones) — all green. No migrations. LGTM. What landed since last review+70 / −12 in 3 files on top of the original TUI work.
RegressionsNone from the follow-up. Original display/request change remains intentional. Nits only (not blockers):
MigrationsNo. Still prefs-only; no SQLite / auth / cache schema. Merge checklist
Checks I ran (non-mutating)@
Not run: full suite, interactive TUI smoke. |
Dialog::handle_key_event returns true for Up/Down/Esc/search, so /variants was persisting the adjacent item as soon as you navigated. Match other pickers: arrows move the highlight, Enter (or item click) confirms, Esc/outside-click dismisses without saving.
Make /variants a compact picker that confirms on Enter, wire variants and status through process_command_input so Ctrl-P actually opens them, and align palette labels/search with OpenCode-style variant wording.
Hide the unused search box on /variants and give the list five visible rows so Default plus low/medium/high/xhigh all fit.
|
lgtm! Thanks as alwys @yan-ad ! |
Summary
tab agentshome hintValidation
cargo fmt --allcargo checkgit diff --checkNotes