fix(tui): stop the permission card's interior clashing on cool themes - #778
Conversation
The focused permission prompt was the only prompt card in the TUI that used
brand/warning colours for its interior, and both read badly on cool themes such
as dracula:
1. The selected option ("Yes, proceed") was filled with zeroTheme.badge — the
brand chip meant for short labels (" 0 ", " ASK ", " SPEC REVIEW "), a
full-brightness accent fill. Every other selectable list in the TUI (model
picker, provider list, / palette) highlights its selected row with onSel/selBg
instead. The permission card was the outlier, painting a saturated accent slab
across the row and skipping the card tint every other line composed onto.
2. The card BODY was tinted with permBg, an amber-family wash intended to signal
a warning surface. It works on the warm default palette but renders as a
brown-yellow slab over cool themes, and it made the permission card the only
prompt whose body carries a fill — ask_user, spec review and plan all use a
transparent body (lipgloss.NewStyle()).
Fix both to match the rest of the TUI: the selected row uses onSel(ink) like
every other picker, and the body drops its permBg fill for a transparent one.
Warning identity is unchanged — the amber PERMISSION badge and the amber-mixed
border still mark it as a permission gate; only the clashing interior fills go.
TestFocusedPermissionSelectedRowUsesSelectionTintNotBrandChip asserts against
palette entries (not hardcoded hex, so it survives a theme retune): the selected
row carries selBg and not the brand accent, the body carries no permBg wash, and
the PERMISSION badge keeps its amber fill so the card still reads as a warning.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe focused permission prompt now highlights the selected option with the selection-row tint, preserves the amber permission badge, and removes the amber card-body background wash. A regression test validates the rendered ANSI background colors. ChangesPermission prompt styling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
Approved. The focused permission card now uses the shared selected-row tint while retaining the permission badge and border as the warning signal. Focused TUI coverage and CI pass.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approve. Tightly scoped fix, and the focused permission card really was the odd one out: it washed its whole body with permBg and filled the selected row with the brand badge, so it clashed on cool themes like dracula. Dropping the body wash and switching the selected row to the standard onSel(ink) tint brings it in line with the other prompt cards and the wizard/palette lists, while the amber PERMISSION badge and border keep the "this is a gate" signal.
Checked on head a1bc1c4:
- Built and ran internal/tui. The new TestFocusedPermissionSelectedRowUsesSelectionTintNotBrandChip passes and is a genuine regression guard: it pins the selected row to selBg (not accent), keeps the amber badge, and asserts the body no longer carries permBg. Reverting either half of the change trips it.
- The ink-on-selBg selected label stays readable on every theme: theme_select_test.go already asserts wcagRatio(ink, selBg) >= 4.5 per palette, so moving off the accent fill costs no contrast.
- go vet and gofmt clean. Heads up if you run internal/tui locally: TestHandleAddDirCommand and TestAltScreenTranscriptScrollKeepsFooterFixed can fail, but they are pre-existing and environment-sensitive (they trip on plain main too) and unrelated to this diff. CI is green on all three OSes.
One non-blocking nit for a later sweep, not this PR: removing the two call sites leaves a few theme symbols with no reader left, namely the onPerm method, the resolved bgPerm color, and the resolved permBg style field. The permBg palette string stays live because the amber-on-permBg WCAG test still reads it, though that assertion is now vestigial since nothing paints on permBg anymore. go vet does not flag unused methods or fields so CI stays green; just flagging it for tidy-up.
LGTM.
3 participants
The problem
The focused permission prompt was the only prompt card in the TUI that used brand/warning colours for its interior, and both read badly on cool themes such as dracula — a warm brown-yellow box over a purple palette:
The selected option was painted with the brand chip.
renderFocusedPermissionPromptfilled the highlighted row ("Yes, proceed") withzeroTheme.badge— the style behind the short brand chips (0,ASK,SPEC REVIEW), a full-brightness accent fill. Every other selectable list in the TUI highlights its selected row withonSel/selBg(provider_wizard.go,provider_manager.go, the/palette). The permission card was the outlier, and it was also the one line in the card that skipped the interiorfill()every other line composed onto.The card body was tinted with
permBg. An amber-family wash meant to signal a warning surface. It suits the warm default palette, but on cool themes it renders as a brown-yellow slab, and it made the permission card the only prompt whose body carries a fill at all —ask_user, spec review and plan all use a transparent body (lipgloss.NewStyle()).The fix
Make the card's interior match the rest of the TUI, and let the warning identity come from the frame rather than the fill:
onSel(ink), the same muted selection tint every other picker uses.ask_user/ spec / plan.PERMISSIONbadge and the amber-mixed border still mark it as a permission gate. Only the two clashing interior fills go.Measured before/after in the dracula palette, at the "Yes, proceed" row and the body:
#bd93f9(bright brand accent)#504482(selBg, muted)#322a1e(permBg, warm slab)#ffb86camber#ffb86camber (unchanged)The same relative change holds on the warm default palette (
#caff3f→#32401bfor the row,#1c1915→ transparent for the body); it is simply less noticeable there because the warm fills already matched that theme.Verification
TestFocusedPermissionSelectedRowUsesSelectionTintNotBrandChipasserts against palette entries, not hardcoded hex, so it survives a theme retune: the selected row must carryselBgand not the brand accent, the body must carry nopermBgwash, and thePERMISSIONbadge must keep its amber fill so the card still reads as a warning surface.Mutation-tested both halves: restoring
zeroTheme.badgefails with "selected row is filled with the brand accent"; restoring thepermBgbody fill fails with "permission card body still tinted with permBg… want a transparent body".gofmtandgo vet ./internal/tui/clean. Fullinternal/tuisuite has the same single failure asmain(TestHandleAddDirCommand, a pre-existing sandbox/TMPDIR artifact) — no new failures.Summary by CodeRabbit