feat(tui): polish command output#98
Conversation
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. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
-
Unified
commandOutputframework reduces duplication. Every TUI command output is now acommandOutputstruct (Title,Status,Sections,Hints) rendered byformatCommandOutput. Adding a new command is just filling in the struct — no morestrings.Join([]string{...}, "\n")sprawl acrossmodel.go,session_controls.go,command_center.go, and friends. -
commandStatusenum is precise.ok/warning/blocked/infocovers the real semantic states.normalizeCommandStatusdefaults unknown toinfoso a future status addition doesn't break rendering. The data is in place for a future enhancement to theme the status line per value. -
Secret redaction is centralized and tested.
\bsk-[A-Za-z0-9._-]+\bincommandOutputSecretPatternscatchessk-...tokens. Applied incompactCommandOutputTextwhich is called for every text segment (title, field key/value, line, row, hint). The test feeds"sk-sensitive approved shell"and asserts it becomes"[REDACTED] approved shell". Defense-in-depth: theinternal/redactionpackage is the primary layer; this is a fallback for tokens that escaped. -
Whitespace compaction and empty-skipping are consistent.
strings.Join(strings.Fields(text), " ")collapses all whitespace. The test" use /doctor for details\r\nsoon "→"use /doctor for details soon"locks the behavior. Empty/whitespace-only fields, lines, and rows are dropped — so a nil sections list renders as just the title + status line, not a tree of blanks. -
Grouping
/helpoutput is a real UX win. The 5 groups (Model,Session,Runtime,Tools,Meta) make the help scannable.commandGroupOrder()is the single source of truth for the group order.TestFormatCommandHelpLinesGroupsCommandsByStableOrderasserts the order is preserved end-to-end, andTestHelpCommandRendersGroupedSectionsasserts the new/helpshape (with the "submit plain text" hint replacing the old "Submit text to ask the assistant"). -
startupOrdermetadata replaces hard-coded list. The previous[]string{"/plan", "/debug", "/tools", "/model", "/provider"}is now driven bycommandDefinition.startupOrder. Adding a new startup chip is one field.TestCommandDefinitionsExposeStartupChipsInStableOrderuses reflection to verify the chip order matches the metadata, catching future field renames. -
Cross-PR integration with
zerocommandsis clean./provideruseszerocommands.ProviderSnapshotFromProfile(PR #82/#90), so URL userinfo andapi_key=query param redaction happens at the snapshot layer./permissionsstill useszerocommands.SandboxGrantSnapshots(PR #89), so grant reason redaction is inherited.TestProviderCommandRedactsCredentialBearingBaseURLconfirmshttps://user:super-secret@proxy.local/v1?api_key=query-secret→https://proxy.local/v1?api_key=[REDACTED]. Good layering — the TUI doesn't re-implement snapshot logic. -
Removing "shell-only" placeholder text is a real product-quality improvement. The old
"X is registered in the Go TUI shell but is not wired yet"was honest but unhelpful. The new"This control is available in the TUI but does not have a backend setting yet"is still honest, more specific, and less jarring.TestCompactCommandAvoidsShellOnlyPlaceholderandTestCompactCommandRecordsShellRequestboth lock the new strings and assert the old placeholders are gone. -
compactTextnow describes product state. Old output:"Compact\nstatus: ...\nbackend: not wired yet"or"request recorded for future compaction backend.". New output: structuredState+Backendsections,"summary: ...","requested: yes/no","state: pending integration", and a hint"compaction request is tracked for this TUI session". TheboolTexthelper gives a cleanrequested: yesrather than a free-form phrase. -
Redaction is applied uniformly via
compactCommandOutputText. Every text segment (title, field, line, row, hint) goes through the same compaction + redaction path. A future secret format addition is one regex entry, and the test pattern (TestFormatCommandOutputRedactsTokenLikeText) generalizes to any token pattern. -
transcriptHasMarkedModelEntrytest fix is correct. The previous checkstrings.HasPrefix(line, "* ")would fail on the new 2-space indented output. The fix trims first. Defensive test pattern. -
Test coverage is comprehensive. New
command_output_test.go(5 tests),commands_test.go(2 tests with reflection), andcommand_polish_test.go(5 tests for product-state output). ExistingTestCompactCommandRecordsShellRequestupdated to lock the new strings.
Observations (non-blocking)
-
Redaction pattern is narrow. Only
sk-...is matched. Other formats (AWS, GitHubghp_..., JWTs, etc.) are not caught. Theinternal/redactionpackage has broader patterns; a future enhancement could call it directly fromcompactCommandOutputTextor expand the regex list. -
Status is not visually distinguished in the rendered output.
formatCommandOutputemitsstatus: <value>as a plain text line. A future enhancement could theme the status line (red forblocked, amber forwarning, green forok). The data is there; the styling is not. -
Redundant lines in some command outputs.
/permissionshas both"Permission mode:"and"mode:"./providerhas both"active:"and"provider:"./modelhas both"Active model:"and"model:"./contexthas both"tools:"and"registered tools:". A cleanup pass could drop the duplicate. Minor. -
A tool name or command starting with
sk-would be mis-redacted. The\bword boundary helps, butsk-looker-integrationwould also be redacted. Unlikely in practice, but worth noting for future edge cases. -
shellOnlyCommandTextis no longer "shell-only". The function name is now misleading. A rename toplaceholderCommandTextorpendingCommandTextwould be clearer. -
No test for nil sections / nil fields slices in
formatCommandOutput. The existing tests cover empty/whitespace values but not nil slices. A future test could lock the nil-handling behavior. -
No benchmark for
formatCommandOutput. The regex is compiled at package init (compile-once), so the hot path is fast. A small benchmark would catch future regressions where someone adds an O(n) string allocation per call. -
/compactstill says "state: pending integration" — itself a placeholder. The test nameTestCompactCommandAvoidsShellOnlyPlaceholderis accurate (it asserts the old text is gone), but the new text is still a placeholder. A future slice could implement compaction and replace it with real state. -
No test for
startupOrderties.startupCommandNamesusessort.SliceStablewith a secondary sort onnameto break ties. The test only exercises all-unique orders. A test with two commands sharingstartupOrderwould lock the tie-breaker. -
formatGroupedCommandHelpLinesis only used in one place (as the body offormatCommandHelpLines). The two functions are redundant —formatCommandHelpLinescould be deleted and callers updated. Minor. -
commands_test.goreflection test is a meta-test. It verifies thestartupOrderfield exists and isint. A simpler test that asserts the chip order would suffice, but the reflection version catches field renames. Defensive. -
No test for
commandHelpLinesForGroupwith an empty group. If a group has no commands, the lines are not emitted (handled by thelen(groupLines) == 0check). A test would lock the behavior. -
The
\bword boundary in the redaction regex requires the token to be preceded and followed by a non-word character. That works forsk-foo barbut not forprefixsk-foo. The current usage is fine (tokens are usually standalone), but a future edge case to consider. -
commandGroupTitlereturnsstring(group)as the default. For an unknown group, the rendered title would be"commandGroup99"-style. The test doesn't exercise the unknown-group case. A future test could assert the default behavior. -
/plandescription is "Show planning mode status." Accurate but doesn't explain what planning mode is. A hint like "use /plan to view or toggle planning mode" would help discoverability. -
No test for the
formatCommandSectionempty sections case. A section with no fields, lines, rows, or hints renders as just the title. A test would lock the behavior. -
The
formatCommandOutputalways emits astatus:line, even ifstatusis empty (whichnormalizeCommandStatusconverts toinfo). A future optimization could omit the line if status isinfoand no sections are present. Not a concern at the current scale.
Approving — the slice is well-executed. The unified commandOutput framework reduces duplication, the redaction layer is a useful safety net, the help grouping is a real UX win, the startup chip metadata replaces a hard-coded list, the cross-PR integration with zerocommands is clean, and the "shell-only" placeholder text is replaced with softer language. The tests are comprehensive. The follow-ups are minor (narrow redaction pattern, status theming, redundant lines in some command outputs, function rename for shellOnlyCommandText).
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. 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 (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughIntroduce a structured commandOutput model and formatter, migrate TUI view methods and command help/startup metadata to use it, and add unit/integration tests for formatting, redaction, help grouping, and stable startup ordering. ChangesTUI Command Output Refactoring
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)
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: 4
🧹 Nitpick comments (3)
internal/tui/command_views.go (1)
127-141: 💤 Low valueMinor redundancy in field label.
Line 134 outputs "Active model:" within a section already titled "Active", creating redundancy ("Active\n Active model: ..."). Consider shortening the field label to just "model:" for consistency with other views.
♻️ Optional simplification
Title: "Active", Lines: []string{ - "Active model: " + displayValue(m.modelName, "none"), + "model: " + displayValue(m.modelName, "none"), "provider: " + displayValue(m.providerName, "none"), "effort: " + m.effortDisplay(), },🤖 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/command_views.go` around lines 127 - 141, The "Active model: " label in the modelText function is redundant given the enclosing section title "Active"; update the field label in modelText (inside commandOutput -> Sections -> Lines) to a shorter form such as "model: " (matching casing used elsewhere) so the section reads "Active" with a line "model: <value>" instead of "Active model: <value>" for consistency with other views.internal/tui/command_output.go (2)
8-10: ⚡ Quick winSecret pattern may not cover all provider key formats.
The regex
\bsk-[A-Za-z0-9._-]+\bonly matches OpenAI-style keys starting withsk-. Other providers use different prefixes (e.g., Anthropic keys may use different formats). If the goal is comprehensive credential redaction across all providers, consider expanding the pattern list.🔐 Suggested pattern additions
var commandOutputSecretPatterns = []*regexp.Regexp{ regexp.MustCompile(`\bsk-[A-Za-z0-9._-]+\b`), + regexp.MustCompile(`\bant-api-[A-Za-z0-9_-]+\b`), + regexp.MustCompile(`\bBearer\s+[A-Za-z0-9._-]+\b`), }🤖 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/command_output.go` around lines 8 - 10, The redaction list in commandOutputSecretPatterns only covers the OpenAI-style sk- token format, so expand the pattern set to include other provider key prefixes/formats used in command output redaction. Update the regexp slice in command_output.go and keep the matching centralized there so command output sanitization in commandOutputSecretPatterns covers all supported credentials.
123-125: Unused helper: remove or justifycommandFieldLine
internal/tui/command_output.godefinescommandFieldLine(lines 123-125), and no other references/call sites were found, so it’s dead code unless planned for future use. Remove it, or add a TODO/use it in the current TUI rendering path.🤖 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/command_output.go` around lines 123 - 125, The function commandFieldLine is unused and should be removed or justified: either delete the commandFieldLine function entirely to eliminate dead code, or if it’s intended for future use, add a clear TODO comment above it and actually integrate it into the TUI rendering path (replace current manual key/value formatting with commandFieldLine in the component that builds command output lines, using compactCommandOutputText and displayValue), ensuring there are call sites referencing commandFieldLine so it’s not orphaned.
🤖 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/command_views.go`:
- Around line 100-104: The profileLines slice currently contains a duplicate use
of m.providerName for both "active:" and "provider:"; update the slice so each
entry is unique by removing or replacing the duplicate—e.g., keep only one line
referencing m.providerName ("provider: " + displayValue(m.providerName, "none"))
and ensure "active:" is either removed or replaced with the correct field (if
there is an active profile field, use that instead). Locate the profileLines
declaration and adjust the elements accordingly, keeping displayValue(...) and
m.modelName unchanged.
- Around line 170-176: The "Tools" view emits the same toolCount twice (Lines
contains both fmt.Sprintf("tools: %d", toolCount) and fmt.Sprintf("registered
tools: %d", toolCount)); remove the duplicate entry and leave a single
descriptive line (e.g., keep only fmt.Sprintf("registered tools: %d", toolCount)
or "tools") inside the Lines slice for the struct literal so the tool count is
shown once; reference the Lines slice and the toolCount variable in the block
that builds the "Tools" Title.
- Around line 47-50: The stateLines slice in internal/tui/command_views.go
contains a duplicated permission mode entry; remove one of the two entries so
only a single line representing m.permissionMode remains (e.g., keep "Permission
mode: "+string(m.permissionMode) and delete the other "mode:
"+string(m.permissionMode)) to eliminate the unintended duplicate output in the
UI.
In `@internal/tui/session_controls.go`:
- Around line 167-173: The compactText method currently ignores the requested
parameter because it sets status := commandStatusInfo then immediately
overwrites status to commandStatusWarning in both branches; update compactText
so that when requested is true it assigns commandStatusWarning and when false it
assigns commandStatusInfo (or if the intended behavior is always warning, remove
the conditional and set status := commandStatusWarning), referencing the
compactText function and the commandStatusInfo/commandStatusWarning constants to
fix the logic so requested actually changes the status.
---
Nitpick comments:
In `@internal/tui/command_output.go`:
- Around line 8-10: The redaction list in commandOutputSecretPatterns only
covers the OpenAI-style sk- token format, so expand the pattern set to include
other provider key prefixes/formats used in command output redaction. Update the
regexp slice in command_output.go and keep the matching centralized there so
command output sanitization in commandOutputSecretPatterns covers all supported
credentials.
- Around line 123-125: The function commandFieldLine is unused and should be
removed or justified: either delete the commandFieldLine function entirely to
eliminate dead code, or if it’s intended for future use, add a clear TODO
comment above it and actually integrate it into the TUI rendering path (replace
current manual key/value formatting with commandFieldLine in the component that
builds command output lines, using compactCommandOutputText and displayValue),
ensuring there are call sites referencing commandFieldLine so it’s not orphaned.
In `@internal/tui/command_views.go`:
- Around line 127-141: The "Active model: " label in the modelText function is
redundant given the enclosing section title "Active"; update the field label in
modelText (inside commandOutput -> Sections -> Lines) to a shorter form such as
"model: " (matching casing used elsewhere) so the section reads "Active" with a
line "model: <value>" instead of "Active model: <value>" for consistency with
other views.
🪄 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: e35a5887-0899-4b80-a610-81aaae2b5aaf
📒 Files selected for processing (15)
internal/tui/command_center.gointernal/tui/command_output.gointernal/tui/command_output_test.gointernal/tui/command_polish_test.gointernal/tui/command_views.gointernal/tui/commands.gointernal/tui/commands_test.gointernal/tui/model.gointernal/tui/model_catalog.gointernal/tui/model_test.gointernal/tui/rendering.gointernal/tui/session.gointernal/tui/session_controls.gointernal/tui/session_controls_test.gointernal/tui/startup.go
💤 Files with no reviewable changes (1)
- internal/tui/model.go
2 participants
Summary
/model,/provider,/config,/context,/permissions,/resume,/compact,/style,/effort, and debug/theme shell-state outputs.model.gointo focused command view helpers.Tests
go test ./internal/tuigo test ./...go build ./cmd/zeroReview Notes
/provideruses redacted provider snapshots so credential-bearing base URLs are not leaked.Summary by CodeRabbit
New Features
Tests