feat: add TUI diagnostics center - #197
Merged
Merged
Conversation
Contributor
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. |
WalkthroughThe PR wires an async ChangesTUI /doctor Command Flow
Sequence Diagram(s)sequenceDiagram
participant User
participant TUIModel as TUI model
participant startDoctorCommand
participant doctorOptions as doctorOptions(connectivity)
participant ProbeProviderHealth
participant DoctorRun as doctor.Run
participant doctorCommandOutput
User->>TUIModel: /doctor --connectivity
TUIModel->>startDoctorCommand: command.text
startDoctorCommand-->>TUIModel: tea.Cmd (async), transcript: "checking provider connectivity..."
Note over startDoctorCommand: executes asynchronously
startDoctorCommand->>doctorOptions: connectivity=true
doctorOptions->>ProbeProviderHealth: {Connectivity, UserAgent, Profile}
ProbeProviderHealth-->>doctorOptions: providerhealth.Result
doctorOptions-->>startDoctorCommand: doctor.Options
startDoctorCommand->>DoctorRun: Run(options)
DoctorRun-->>startDoctorCommand: report
startDoctorCommand->>doctorCommandOutput: report + BackendLifecycleSnapshot
doctorCommandOutput-->>startDoctorCommand: commandOutput
startDoctorCommand-->>TUIModel: doctorCommandResultMsg{id, text}
TUIModel->>TUIModel: reduceTranscript with doctor output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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.
🧹 Nitpick comments (3)
internal/tui/command_center.go (1)
48-60: ⚡ Quick winConsider adding test coverage for unknown flag handling.
parseDoctorCommandArgsreturns an error for unknown flags, but there's no test verifying this behavior. Consider adding a test case like:func TestParseDoctorCommandArgsRejectsUnknownFlags(t *testing.T) { _, _, err := parseDoctorCommandArgs("--invalid") if err == nil || !strings.Contains(err.Error(), "unknown doctor flag") { t.Fatalf("expected unknown flag error, got %v", err) } }This ensures the error path is exercised and prevents regressions.
🤖 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_center.go` around lines 48 - 60, The parseDoctorCommandArgs function includes error handling for unknown flags, but this error path is not covered by tests, leaving room for regressions. Add a test function named TestParseDoctorCommandArgsRejectsUnknownFlags that calls parseDoctorCommandArgs with an invalid flag argument (such as "--invalid"), verifies that the function returns a non-nil error, and confirms that the error message contains the text "unknown doctor flag". This will ensure the error handling path is exercised and documented through tests.internal/tui/model.go (1)
1002-1006: ⚡ Quick winClarify the
id == 0fallback condition.The check
msg.id == 0 || msg.id == m.doctorCommandSeqaccepts messages with id 0, but looking atstartDoctorCommand, messages are always created with a non-zero id (m.doctorCommandSeqafter increment). When would a message have id 0? If this is defensive coding, a comment would help explain the intent. If it's unintentional, consider removing it or documenting the scenario.🤖 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/model.go` around lines 1002 - 1006, In the doctorCommandResultMsg case handler where you check `msg.id == 0 || msg.id == m.doctorCommandSeq`, the `msg.id == 0` condition appears unnecessary since messages created by startDoctorCommand always have non-zero ids (assigned from m.doctorCommandSeq after increment). Either remove the `msg.id == 0` check if it serves no purpose, or if this is intentional defensive coding, add a comment explaining the scenario where a message could have id 0 and why it should be accepted.internal/tui/doctor_view.go (1)
146-150: 💤 Low valueConsider conditionally showing backend hints.
The backend hints (
/mcp,/hooks,/plugins) are always added when a backend snapshot is present, regardless of check status. This might clutter the output when diagnostics are healthy.Consider only adding these hints when there are non-pass checks or when explicitly requested (e.g., verbose mode).
🤖 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/doctor_view.go` around lines 146 - 150, The backend hints for `/mcp`, `/hooks`, and `/plugins` are being unconditionally added whenever backend is not nil, which can clutter output when all diagnostics are healthy. Modify the condition in the if backend != nil block to additionally check whether there are any non-pass checks in the diagnostics or if verbose mode is enabled. Only add these hint messages when there are actual issues to diagnose or when verbose output is explicitly requested, rather than always showing them.
🤖 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.
Nitpick comments:
In `@internal/tui/command_center.go`:
- Around line 48-60: The parseDoctorCommandArgs function includes error handling
for unknown flags, but this error path is not covered by tests, leaving room for
regressions. Add a test function named
TestParseDoctorCommandArgsRejectsUnknownFlags that calls parseDoctorCommandArgs
with an invalid flag argument (such as "--invalid"), verifies that the function
returns a non-nil error, and confirms that the error message contains the text
"unknown doctor flag". This will ensure the error handling path is exercised and
documented through tests.
In `@internal/tui/doctor_view.go`:
- Around line 146-150: The backend hints for `/mcp`, `/hooks`, and `/plugins`
are being unconditionally added whenever backend is not nil, which can clutter
output when all diagnostics are healthy. Modify the condition in the if backend
!= nil block to additionally check whether there are any non-pass checks in the
diagnostics or if verbose mode is enabled. Only add these hint messages when
there are actual issues to diagnose or when verbose output is explicitly
requested, rather than always showing them.
In `@internal/tui/model.go`:
- Around line 1002-1006: In the doctorCommandResultMsg case handler where you
check `msg.id == 0 || msg.id == m.doctorCommandSeq`, the `msg.id == 0` condition
appears unnecessary since messages created by startDoctorCommand always have
non-zero ids (assigned from m.doctorCommandSeq after increment). Either remove
the `msg.id == 0` check if it serves no purpose, or if this is intentional
defensive coding, add a comment explaining the scenario where a message could
have id 0 and why it should be accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 82da9a5c-566a-432d-a9e1-98bb2d8406b3
📒 Files selected for processing (13)
internal/cli/app.gointernal/cli/app_test.gointernal/doctor/doctor.gointernal/doctor/doctor_test.gointernal/tui/command_center.gointernal/tui/commands.gointernal/tui/doctor_command_test.gointernal/tui/doctor_view.gointernal/tui/doctor_view_test.gointernal/tui/health_command_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/options.go
gnanam1990
approved these changes
Jun 14, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Approve — well-built, no blockers. I verified the security/correctness-sensitive parts:
Async --connectivity — correct
- Plain
/doctorrenders synchronously and offline (doctorOptions(false)never probes). Only--connectivityprobes. --connectivityreturns atea.Cmd, so the probe runs in bubbletea's goroutine — off the Update loop (no UI freeze). AdoctorCommandSeqid on the producer + thecase doctorCommandResultMsgguard (msg.id == m.doctorCommandSeq) correctly discards superseded probe results.- The probe is time-bounded:
providerhealth.Probewraps the ctx in a 5scontext.WithTimeoutand the HTTP client has its own timeout, so a hung endpoint can't leave the "running…" state stuck.
No secret leak in the new view
doctorCheckRowrenders[status] id - messageonly — it does not rendercheck.Details, where the rawbaseURLlives. Combined with the existingcredentialConfigured(presence-only, never the key), the TUI doctor surfaces no secret material.
Validation change is a strengthening, not a regression
provider.modelnow fails on an empty model (previously unhandled), and the unknown-custom-model path still warns + passes through with a helpful--connectivityhint. No weakening.
Tests / CI
- Good coverage (doctor command/view, health alias, model wiring, provider-model validation); full CI green incl. the Security gate.
One optional, forward-looking note (non-blocking)
The TUI view intentionally omits check.Details (so baseURL isn't shown) — nice. If a future change ever renders Details in the TUI, strip userinfo from baseURL at that point (it can in rare cases embed user:token@host), to match the redaction model elsewhere.
Nice feature — clean async handling and good diagnostics ergonomics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/doctorwith grouped provider/platform checks and actionable hints/healthas a discoverable alias for/doctor/doctor --connectivityin the TUI using the configured provider health probeTests
go test ./internal/tui -run Doctor|Health|Command -count=1go test ./internal/doctor -run ProviderModel -count=1go test ./internal/cli -run TestRunNoArgsLaunchesSetupTUIWithNilProviderWhenNoProviderConfigured|TestRunNoArgsLaunchesTUI -count=1go test ./... -timeout 300sgo vet ./...go build ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokeSummary by CodeRabbit
Release Notes
New Features
/doctorcommand with--connectivityflag to check provider connectivity and health./healthas an alias for the/doctorcommand.Bug Fixes
Tests