Migrate TUI to Bubble Tea v2 - #222
Conversation
Move the TUI stack to charm.land Bubble Tea, Bubbles, and Lip Gloss v2, including key, paste, mouse, and view handling changes for the v2 APIs. Add compatibility helpers for the new input event shapes and update TUI tests accordingly. Refresh related Go dependencies and make the PDF truncation fixture deterministic across poppler and pure-Go extraction. Tested: GOCACHE=/tmp/zero-go-cache GOMODCACHE=/tmp/zero-go-mod-cache go test ./internal/tui Tested: GOCACHE=/tmp/zero-go-cache GOMODCACHE=/tmp/zero-go-mod-cache go test ./internal/imageinput -count=1 -v Tested: GOCACHE=/tmp/zero-go-cache GOMODCACHE=/tmp/zero-go-mod-cache go test ./...
|
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 (6)
✅ Files skipped from review due to trivial changes (2)
WalkthroughMigrates the TUI package to ChangesTUI v2 Migration and Input Refactor
Sequence Diagram(s)sequenceDiagram
participant ProviderWizard
participant providerWizardDiscovery
participant providerWizardOAuthCmdFor
participant providerWizardDevicePrepareCmd
participant providerWizardDevicePollCmd
participant applyProviderWizardOAuth
ProviderWizard->>providerWizardDiscovery: beginOAuthAttempt(false)
providerWizardDiscovery->>providerWizardOAuthCmdFor: pass attemptID
providerWizardOAuthCmdFor->>providerWizardDevicePrepareCmd: start device flow with attemptID
providerWizardDevicePrepareCmd->>providerWizardDevicePollCmd: poll with attemptID
providerWizardDevicePollCmd->>applyProviderWizardOAuth: deliver providerID and attemptID
applyProviderWizardOAuth->>ProviderWizard: accept only matching attempt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/input_compat.go`:
- Around line 61-63: The mouseButton function is unused and triggers a
golangci-lint error. Either remove the mouseButton function entirely if it is
not needed, or identify where it should be called and wire it into the codebase
to use it. If the function is intended for future use or public API purposes,
consider adding a comment explaining its purpose to suppress the lint warning,
or ensure it is actually being utilized by callers.
In `@internal/tui/provider_wizard.go`:
- Around line 586-591: The OAuth async login initiated at line 590 lacks an
attempt identifier, allowing stale results from previous wizard attempts to be
incorrectly applied to new ones. Generate and store a unique attempt ID in the
wizard state when starting the OAuth flow (when providerWizardSupportsOAuth is
true), then pass this attempt ID to providerWizardOAuthCmdFor. Update the
applyProviderWizardOAuth and applyProviderWizardDeviceCode functions to validate
three conditions before applying results: wizard.oauthPending is true, the
providerID matches the currentProvider, and the attemptID matches the stored
attempt ID. This ensures async results are only applied to the correct wizard
attempt they originated from.
🪄 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: f821cd23-f20c-4e3d-bb95-5f186836cfee
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (55)
go.modinternal/imageinput/pdf_test.gointernal/tui/ask_user_test.gointernal/tui/assistant_markdown.gointernal/tui/autocomplete_test.gointernal/tui/command_bash.gointernal/tui/command_center.gointernal/tui/command_polish_test.gointernal/tui/command_views.gointernal/tui/commands_test.gointernal/tui/composer.gointernal/tui/composer_test.gointernal/tui/doctor_command_test.gointernal/tui/flush.gointernal/tui/flush_test.gointernal/tui/input_compat.gointernal/tui/input_compat_test.gointernal/tui/mcp_add_wizard.gointernal/tui/mcp_add_wizard_view.gointernal/tui/mcp_manager.gointernal/tui/mcp_view_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/mouse.gointernal/tui/mouse_test.gointernal/tui/onboarding.gointernal/tui/onboarding_test.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/picker_test.gointernal/tui/pr_status.gointernal/tui/provider_onboarding_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_discovery.gointernal/tui/provider_wizard_oauth_test.gointernal/tui/provider_wizard_test.gointernal/tui/queued_message_test.gointernal/tui/rendering.gointernal/tui/rendering_lime_test.gointernal/tui/run.gointernal/tui/scroll_test.gointernal/tui/selectable_list.gointernal/tui/selectable_list_test.gointernal/tui/session_controls.gointernal/tui/session_controls_test.gointernal/tui/session_test.gointernal/tui/spec_mode.gointernal/tui/spec_mode_test.gointernal/tui/startup.gointernal/tui/startup_test.gointernal/tui/theme.gointernal/tui/transcript_selection.gointernal/tui/transcript_view_test.gointernal/tui/view.gointernal/tui/width_tiers_test.go
Remove the unused mouseButton compatibility helper flagged by lint. Correlate provider wizard OAuth and device-code async results with the active provider and attempt id so stale results from abandoned attempts are ignored. Add regression coverage for stale browser OAuth and stale device-code results. Tested: GOCACHE=/tmp/zero-go-cache GOMODCACHE=/tmp/zero-go-mod-cache go test ./internal/tui -run 'ProviderWizard.*OAuth|ProviderWizard.*Device|ApplyProviderWizardOAuth|RenderCredentialStepShowsOAuth' -count=1 -v Tested: GOCACHE=/tmp/zero-go-cache GOMODCACHE=/tmp/zero-go-mod-cache go test ./internal/tui Tested: GOCACHE=/tmp/zero-go-cache GOMODCACHE=/tmp/zero-go-mod-cache go test ./... Tested: git diff --check
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. |
Review — Migrate TUI to Bubble Tea v2Verdict: approach is sound, and both of CodeRabbit's blockers already appear fixed on the branch HEAD — the Strengths
Both flagged findings look resolved
Risks to verify before merge (not blockers)
Merge order#217, #220, and this PR all rewrite the same TUI files, and this one changes the v1→v2 input API itself. Recommend landing this migration first (or last with everyone rebasing onto it) — otherwise #220/#217 have to be re-expressed in the v2 helper API after the fact. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
APPROVE
Read at ae17cf0. Walked through the 28 files (885 +, 197 -), focused on the new behavior:
- in-memory session grants +
sessionMuininternal/sandbox/engine.go - the new
PermissionDecisionAllowForSessionpath ininternal/agent/loop.go:579-587 workspaceWriteAutoAllowed+ the newapply_patchshort-circuit ininternal/sandbox/engine.go- the TUI fallback in
internal/tui/permission_prompt.go:21-53 - the new
TestRunAllowsWorkspaceWriteWithoutPromptWhenSandboxPolicyPermitsand the session-grant test ininternal/agent/loop_test.go - the transcript-row label switch in
internal/tui/session_controls.go
The shape of this is good. Session grants have a real concurrency primitive, AvailableDecisions flows from the engine to the TUI cleanly, and the agent loop's "honor the allow for THIS call regardless" comment on lines 591-598 is the right call. persistSessionPermissionGrant returning a sandbox.Grant instead of an opaque bool keeps the event payload honest.
Three things I want to flag for follow-up, none blocking:
1. apply_patch workspace auto-allow bypasses the engine's path-list validation, not just the user prompt.
workspaceWriteAutoAllowed returns true for apply_patch unconditionally, and the engine's path validation iterates requestPaths(request) (in internal/sandbox/risk.go:223) which does not include the patch key. So with EnforceWorkspace: true and an in-workspace patch header, the engine approves the call without checking DenyWrite/AllowWrite. The apply_patch tool's own validatePatchPaths in internal/tools/apply_patch.go only does workspace confinement (no .., no absolute paths, resolveWorkspaceTargetPath). It does not consult the policy.
The OS-level sandbox (bwrap, sandbox-exec, landlock, Windows ACL) still enforces DenyWrite at the kernel level, so this is defense-in-depth, not a hole. But users running in pure permissions mode (no OS sandbox) get a behavior change: a patch that targets a DenyWrite path used to fail when the user reviewed it in the prompt, now passes silently. Worth either threading the policy into the apply_patch tool or calling the trade-off out in the PR body so it isn't a surprise.
2. No test for apply_patch + DenyWrite.
TestRunAllowsWorkspaceWriteWithoutPromptWhenSandboxPolicyPermits only covers an in-workspace write_file against DefaultPolicy() (which has no DenyWrite). A test that an apply_patch targeting a DenyWrite path is denied would close the loop. If the answer is "the apply_patch tool catches it", a test at that layer would be even more useful.
3. TUI permissionOptions fallback offers "always" even with no persistent store.
In internal/tui/permission_prompt.go:21-30, when AvailableDecisions is empty the fallback hardcodes all four decisions. The TUI is only invoked for prompts that always populate AvailableDecisions, so the fallback is unreachable today. But if a future call site passes a non-prompt event to the TUI, the user can hit "y" with no store and silently get only-this-call behavior. Easy fix: derive the fallback from the presence of a store, same as the engine does in availablePermissionDecisions.
Minor: the persistSessionPermissionGrant error path is swallowed with if ... err == nil (loop.go:583), and that's the right call for "don't deny what the user explicitly allowed", but the asymmetry with the AlwaysAllow branch (which builds the permission event from the persisted grant on success) means the session-grant event's GrantMatched flips to false if persistence fails. Worth a code comment so a future reader doesn't think the event is broken.
Non-blocking follow-ups; happy to see this land.
|
Heads up — wrong review posted here. I just posted a CHANGES_REQUESTED-style review on this PR in error. The body talks about session approvals, the in-memory PR #222 is already MERGED, so the orphan review doesn't change anything in the codebase. I tried to dismiss it but the dismiss endpoint didn't take (likely because #222 is merged). Treating the review as a no-op; the real review will land on #244. Sorry for the noise. |
3 participants
Summary
Tests
Summary by CodeRabbit
Release Notes
Dependencies
Bug Fixes
Refactor & Tests