fix(tui): reassign Dune to dark Claude Code colorblind palette - #713
fix(tui): reassign Dune to dark Claude Code colorblind palette#713euxaristia wants to merge 7 commits into
Conversation
WalkthroughThe Dune TUI theme changes from a warm light-style palette to a near-black, colorblind-friendly dark palette. Registry metadata, documentation, and ANSI256 contrast tests are updated, and a daemon drain test now waits for active worker tracking. ChangesDune theme
Daemon drain test
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Small, clean change: Dune is reassigned from the warm light-sand palette to a dark, daltonized palette with IsDark correctly flipped to true, and the wiring test is updated to match. The full tui suite passes, including the truecolor and xterm-256-quantized WCAG-AA contrast guards covering Dune's selected-row, diff-band, and status-color pairs, so the new colors hold up on 256-color terminals too. No objections — approving.
4631efa to
da6cb17
Compare
Status
Ready for human review/approval. |
2389075
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve Dune's diff identity on ANSI-256 terminals
internal/tui/theme_palettes.go:455
In a 256-color terminal, the repository's ownxterm256Hexalgorithm maps bothaddBg(#0a1f14) anddelBg(#240a0e) to#121212; it likewise maps both word-span backgrounds to#303030. Added and removed diff bands therefore become identical (and the base bands blend into the panel) for Dune users, contrary to the claimed colorblind/xterm behavior. Choose backgrounds that quantize to distinct green/red entries and add Dune equivalents of the existing Neon ANSI-256 diff assertions. -
[P2] Update the public Dune description
docs/THEMES.md:12
The guide still says Dune is a warm sand-and-cream palette with charcoal ink and a soft amber accent. This change makes the same user-selectable theme near-black with white/orange and daltonized blue/red signals, and registers it as dark. Update the documentation with the new appearance so--theme dune,ZERO_THEME=dune, and the picker are described consistently.
|
Pushed a fixup addressing both open findings:
Verified go build ./..., go vet ./..., and internal/tui tests (theme-specific tests all pass; there's one unrelated pre-existing failure, TestAltScreenTranscriptScrollKeepsFooterFixed, that also fails on the unmodified branch since it needs a TTY). Checked gofmt cleanliness with git diff --check against upstream/main on the changed files, no issues. |
Dune was rendering as a light/sandy theme with a 55/55/55 grey surface, which did not match Claude Code's dark colorblind mode. Rebase the palette on a near-black surface (#0e0e10) with the brand-orange accent from openclaude's dark-daltonized theme, and move the warm grey to the user-prompt bubble only. Brighten the muted/faint/faintest gray ramp and darken selBg/addBg/delBg so all WCAG-AA and xterm-256 contrast invariants hold. Updates the stale test that still marked Dune as light. Refs Gitlawb#709
Empty commit to re-run CI. Prior Windows smoke failed on TestRunExecOptimizedSessionUnderGate (0 prewarm HEAD probes), which is unrelated to the Dune palette change.
waitFor polled QueueDepth(), which flips as soon as a slot is leased, before the worker is registered in p.active by track(). Drain() only sees stragglers in p.active, so a wide enough scheduling window let it observe an empty active set and return without killing anything. Windows CI's coarser goroutine scheduling was wide enough to hit this reliably; wait on WorkerStats() instead, which reflects p.active.
Dune's addBg/delBg (#0a1f14/#240a0e) and addBgWord/delBgWord (#1f4d33/#4d1620) each quantized to the same xterm-256 gray (#121212 and #303030), making added and removed diff lines indistinguishable on terminals without truecolor support. Rebase the four diff band colors on stronger green/red channel separation so they quantize to distinct xterm cube colors, following the pattern Neon already uses. Adds the Dune equivalent of Neon's ANSI-256 diff-band assertions in TestExtendedThemeANSI256Contrast. Also updates the Dune entry in docs/THEMES.md, which still described the old warm sand/cream/amber palette instead of the dark colorblind-safe one this PR introduced.
4efe601 to
c8ee46c
Compare
|
Rebased onto current upstream main (was 17 commits behind). Clean replay, no conflicts. Local: theme/palette-focused |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Preserve readable added diffs on ANSI-256 terminals
internal/tui/theme_palettes.go:459
buildThemerenders the changed palette as actual add-diff foreground/background pairs, but ANSI-256 quantization makes them unreadable:greenonaddBgbecomes#5f87ffon#005f00(2.43:1) for the add sign,faintestonaddBgbecomes#9e9e9eon#005f00(2.97:1) for the line-number gutter, andaddInkonaddBgWordbecomes#afffd7on#008700(4.06:1) for changed-word text. Thus--theme dunefails AA/readability for core added-diff content on 256-color terminals. The new Dune assertions check only green/red band identity, whereas the existing Neon section tests these rendered text pairs. Please choose quantization-safe Dune colors and add the equivalent Dune assertions.
The prior fix made Dune's add and del diff bands quantize to distinct xterm-256 colors, but the foreground/background pairs rendered on top of them were not checked and fail AA once quantized: green on addBg drops to 2.43:1, faintest on addBg to 2.97:1, and addInk on addBgWord to 4.06:1. Brighten green, faintest, and addInk so each pairing clears 4.5:1 after xterm-256 quantization, matching the bar Neon already holds. Add the equivalent Dune assertions to TestExtendedThemeANSI256Contrast, mirroring the existing Neon checks for these rendered text pairs.
|
Addressed the latest finding: the add-diff bands were distinct after xterm-256 quantization, but the text rendered on top of them wasn't checked and failed AA. Changed in Dune:
faintest and green both needed a hue shift, not just a brightness bump: at the same brightness a neutral gray/blue can't clear 4.5:1 against xterm's quantized green background (the math works out the same way Neon's own faintest is tinted green rather than staying pure gray). Del-side pairs (red/faintest/delInk on delBg) were already fine and are untouched. Added the equivalent Dune assertions to
|
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Keep the selected row visibly distinct after ANSI-256 quantization
internal/tui/theme_palettes.go:464
On an ANSI-256 terminal, the repository'sxterm256Hexmapping turns the new Dune panel (#0e0e10) into#121212andselBg(#191c1f) into#1c1c1c. Their contrast is only 1.099:1, just below the 1.10 selected-row visibility threshold enforced byTestSelectedRowBandIsVisibleAndReadable.onSeluses this background for the selected permission choice and every selectable list, so the active row remains effectively indistinguishable in the terminal mode this PR explicitly claims to support. Choose a selection color that quantizes farther from the panel and add the panel-vs-selection ANSI-256 assertion. -
[P2] Split the unrelated daemon test repair out of this palette PR
internal/daemon/pool_test.go:227
This is a separate current scope-policy blocker, not a claim that the test repair is incorrect: it repairs a Windows scheduling race inTestPoolDrainKillsStraggler, but the PR description, its Dune-theme claim, and its cited reference contain no approved daemon scope. The repository policy requires each community PR to stay focused on its approved issue. Move this change to its own approved daemon issue/PR (with its race validation) so the Dune palette review remains auditable.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/theme_select_test.go (1)
449-455: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck hue identity after quantization.
greenishandreddishinspect the original hex values, while the test claims to validate xterm-256 output. A future palette could quantize to a neutral or wrong-hue color and still pass these assertions.Proposed fix
- if q(dune.addBg) == q(dune.delBg) || !greenish(q(dune.addBg)) || !reddish(q(dune.delBg)) { + if q(dune.addBg) == q(dune.delBg) || !greenish(q(dune.addBg)) || !reddish(q(dune.delBg)) { ... - if q(dune.addBgWord) == q(dune.delBgWord) || !greenish(q(dune.addBgWord)) || !reddish(q(dune.delBgWord)) { + if q(dune.addBgWord) == q(dune.delBgWord) || !greenish(q(dune.addBgWord)) || !reddish(q(dune.delBgWord)) {🤖 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/theme_select_test.go` around lines 449 - 455, Update the quantized palette assertions in the dune checks to evaluate hue identity on the quantized xterm-256 values, not the original hex colors. Ensure both add/del row bands and word-span bands remain distinct and green/red respectively after quantization, using the existing q, greenish, and reddish helpers.
🤖 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.
Outside diff comments:
In `@internal/tui/theme_select_test.go`:
- Around line 449-455: Update the quantized palette assertions in the dune
checks to evaluate hue identity on the quantized xterm-256 values, not the
original hex colors. Ensure both add/del row bands and word-span bands remain
distinct and green/red respectively after quantization, using the existing q,
greenish, and reddish helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 917ede12-fa90-44e0-ae81-b4fd02cec874
📒 Files selected for processing (2)
internal/tui/theme_palettes.gointernal/tui/theme_select_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/theme_palettes.go
|
Addressed both open findings from @jatmn's latest review:
All theme & palette tests pass (go test ./internal/tui/ -run 'Theme|Palette|Dune|Contrast|ANSI'). Ready for re-review! |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving, same as my earlier pass before it was dismissed by the push.
The coverage question I wanted answered is answered: this is not just an IsDark == true flip. The registry-wide suites pick the new palette up automatically and the numbers are healthy (ink 16.35, muted 12.06, accent 8.86 on panel, ink-on-selBg 12.83, selBg-vs-panel 1.27, plus the quantized xterm-256 pairs). I also confirmed the unrelated internal/daemon/pool_test.go change from the earlier revision is NOT in this head, only the three intended files.
Two colour nits, neither blocking:
faintest(#55c6cd) ended up close enough togreen(#5eccfa) that the two are hard to tell apart, and they render side by side in diff line numbers. Worth nudging one of them.gitAdd(#7db87a) andgitDel(#b87a7a) are carried over byte-identical from the Lime dark palette, so the one green/red pair that daltonization exists to fix is the pair that did not get daltonized. Given the palette is described as colorblind-safe, that is the pair I would most want adjusted.
Verification note: the go toolchain is blocked on my machine by Smart App Control right now, so this is a source review; CI is green.
LGTM.
|
@kevincodex1 merge pls big bro |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
miss marked my last review
Updated
LGTM
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Changes requested — on the process gate and on scope, not on the engineering. The contrast work in here is real, I reproduced the defect it fixes, and the new assertions are the most valuable part of the patch. The problem is that a measured accessibility bug and an unapproved re-theming are riding in the same change.
Reviewed at a57cf8a, base a50574f (origin/main at 097c265).
What I verified
-
The underlying defect is genuine, and I measured it. Checking out the base
internal/tui/theme_palettes.gounder this PR's tests reproduces it exactly:--- FAIL: TestExtendedThemeANSI256Contrast dune: add/del row bands lose their green/red identity after quantization: addBg #dcecd0 -> #dadada, delBg #f5dbd5 -> #ffd7d7Dune's added-line band quantizes to a neutral grey on 256-colour terminals, so added and removed lines stop reading as green and red. That is a legitimate accessibility bug and it is worth fixing.
-
The new assertions are load-bearing, which is the check I care most about. Reverting only
theme_palettes.goand keeping the tests fails bothTestExtendedThemeANSI256ContrastandTestNewThemePresetsWired— they are testing the palette, not describing it. -
go test ./internal/tui/ -run 'Theme|Contrast|Wired' -count=1passes on the branch. Ran on darwin/arm64 from a non-/tmpcheckout.
Blocking
-
No parent issue carrying
issue-approved. — PR descriptionThe description says
Refs #709. #709 is not an issue; it is a merged pull request (fix(tui): fix rendering corruption over multipass + Windows Terminal) —gh api repos/Gitlawb/zero/issues/709returns apull_requestfield. I searched the tracker for an issue covering the Dune palette and found none; the closest, #633, is about adding new theme presets, is unlabelled, and does not cover changing an existing one.CONTRIBUTING.mdmakes the label the gate for community PRs, ahead of code quality, so this needs an issue before it can be judged on merit. I would rather say that now than have it closed later on process after the work is already done. This is a paperwork outcome, not a comment on the contribution. -
The patch is substantially larger than the defect it demonstrates. —
internal/tui/theme_palettes.goWhat the reproduction above shows is that
addBg/delBg(and the word-span bands) do not survive xterm-256 quantization. Fixing that needs those bands adjusted. What this PR does instead is rebase the entire theme onto a near-black surface, invertIsDark, and rewrite Dune's description indocs/THEMES.mdfrom "a warm sand-and-cream palette" to "a dark, colorblind-safe palette".Two consequences that need a maintainer's decision rather than a reviewer's:
- Dune is a shipped theme with a published identity. Users who selected it chose a light warm theme; after this they get a near-black one under the same name. That is a product change, and
CONTRIBUTING.mdputs unapproved feature work outside what a PR may carry. - It reduces light-theme coverage from three to two. After the change
themeRegistry(internal/tui/theme_palettes.go:488-501) holds twelve dark entries and two light ones —lightandsolarized-light— and Dune was the only warm light option. Whether the catalogue should keep a warm light theme is exactly the kind of call that belongs in an issue.
The clean split, if the maintainers agree the dark variant is wanted: fix Dune's diff bands in place so it stays light and passes the new quantization assertions, and add the daltonized dark palette as a new registry entry under its own name. That keeps both audiences, keeps every assertion in this PR, and needs no product decision to be reversed later.
- Dune is a shipped theme with a published identity. Users who selected it chose a light warm theme; after this they get a near-black one under the same name. That is a product change, and
Non-blocking
internal/tui/theme_select_test.go:422-483is the part of this PR I would most want kept regardless of how the theme question is settled. It promotes checks that previously guarded only Neon — band distinctness after quantization,addBgWorddistinguishable fromaddBg,faintestlegible on both bands,addInk/delInkon their word spans — into invariants Dune must also satisfy. Those assertions are the durable value here, and they apply to whatever palette Dune ends up with.internal/tui/theme_select_test.go:278-280flipsTestNewThemePresetsWiredfrom asserting Dune is light to asserting it is dark. That is correct given the change, but it is worth naming explicitly: an existing test was rewritten to match new behaviour rather than a new one added, so the light-theme guarantee it used to hold is simply gone rather than relocated. If a light Dune is retained, this line reverts.- The
greenish/reddishhelpers were moved above theduneblock so both palettes can use them (theme_select_test.go:422-429). Sensible, no issue.
Not covered by this PR
- The quantization assertions now cover Dune and Neon. The other twelve registry entries have no equivalent guard, so a palette with the same latent defect would ship unnoticed. Worth a separate issue proposing the check be applied across the whole registry rather than per-theme — that is the class, and it is a better use of this work than either theme alone.
Not verified
- Rendering by eye on a real 256-colour terminal. The PR describes a manual
/theme dunecheck, which I did not repeat; my verification is the contrast suite only. - Any platform other than darwin/arm64.
To be clear about the order of things: the engineering is not what is holding this up. Open an issue for the Dune change, get it labelled, and the technical content here is close to ready.
4 participants
Summary
Dune was rendering as a light/sandy theme with a creamy white background, which did not match Claude Code's dark colorblind (daltonized) mode. This reassigns Dune to a genuinely dark theme based on openclaude's dark-daltonized palette.
Changes
Test plan
Refs #709
Summary by CodeRabbit
Summary