fix(tui): route chrome colours through the palette - #520
Conversation
77 hardcoded Color:: literals in the modern TUI bypassed the theme. That is not cosmetic: the theme adapts itself to the terminal's colour depth in adapt_for_emit, so a literal Color::DarkGray opts that cell out of the adaptation entirely. Under NO_COLOR — which asks for no colour, not less colour — those cells stayed coloured. Each literal now maps to the palette slot for its role: DarkGray and Gray to the two dim tiers (muted, inactive), and Green/Red/Yellow/White/Blue to success/error/warning/text/accent. the_frame_uses_only_theme_colours pins it. One-dark's slots are all RGB, so any named colour left in a rendered frame is a bypass; the test fails with a list of them. Before this change it reported three from a minimal frame, and the frame it renders now also covers errors, warnings, thinking and the tasks pane — the chrome most tempting to hand-colour. The golden snapshots caught the change, which is what they are for. The regenerated files differ only in their legends and style grids: the glyph sections are byte-identical across all four, so nothing moved on screen — DarkGray became Rgb(92, 99, 112) and Gray became Rgb(124, 131, 144), one-dark's muted and inactive.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bead73318
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| use ratatui::style::{Color, Modifier, Style}; | ||
|
|
||
| use super::colors::palette; |
There was a problem hiding this comment.
The reviewed commit records both its author and committer as Codex <codex@openai.com>, whereas the repository's AGENTS.md requires commits to use only the human author and prohibits Codex attribution. Ensure the final commit carries the human contributor's identity instead.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
| for x in 0..buf.area.width { | ||
| let cell = &buf[(x, y)]; | ||
| for (what, c) in [("fg", cell.fg), ("bg", cell.bg)] { | ||
| let named = !matches!(c, Color::Rgb(..) | Color::Reset); |
There was a problem hiding this comment.
With inline or fenced Markdown, this guard passes while NO_COLOR remains violated: it accepts every Color::Rgb, including the hardcoded foreground/backgrounds at markdown.rs:186-187 and the code-block background at markdown.rs:366; the rendered setup also never reaches remaining named-color paths such as the Color::Black permission badge at render.rs:1183. Since tests pin the emit mode to truecolor, this does not exercise Mono at all. Validate colors against the adapted palette or a controllable Mono render, and route the remaining literals through that adaptation.
Useful? React with 👍 / 👎.
The previous commit routed the chrome's named literals through the palette but left three kinds of colour behind, and the test that was meant to pin the invariant could not see any of them: it asserted only that no *named* colour reached the screen, which every hardcoded Color::Rgb passes trivially, and cfg(test) pins the emit mode to truecolor so Mono was never exercised at all. Behind that gap: the inline-code chip and code-block background (Color::Rgb literals in markdown.rs), syntect's own highlight palette, which never passes through a theme slot and so cannot reach adapt_for_emit, and three filled badges painting Color::Black text on an accent or warning fill. The last one is a real defect rather than a purity violation — under NO_COLOR the fill collapses to the terminal default while the black text stays black, so "action required", the mouse selection and the current search match all go invisible on a dark terminal; on any light theme they were low-contrast already. Three new slots carry them: on_accent (the theme's own background, the colour those fills were chosen to contrast against), code_fg and code_bg. Each is derived in theme_from_palette and adapted in adapt_for_emit_with alongside every other slot. code_bg is written mix(bg, fg, 0.08) rather than the other way round because mix returns its first argument unchanged for named colours — on the ANSI-16 themes that difference is code-on-background versus code-on-foreground. syntax_color gives the highlighter the same adaptation hop. Tests now pin the invariant in the three shapes a bypass can take, each verified to fail when a literal is put back: - one-dark truecolor — no named colour on screen (as before); - dark-ansi at Ansi16 — no Color::Rgb survives quantization; - one-dark at Mono — no cell carries colour at all, fg or bg. The last two need a mode a test could not previously reach, so color_emit grows a thread-local pin_mode override; thread-local so a test pinning Mono cannot bleed into a frame another test renders in parallel. The probe frame asserts its own subject is on screen first — an earlier draft of the Ansi16 test passed only because the code block had scrolled out of the viewport. Slot coverage runs over every built-in palette, so the colour-blind and ANSI-16 accessibility themes are checked for legible badge and code pairs rather than assumed. Golden snapshots regenerated deliberately and inspected: exactly one row changed across the four files, permission_modal legend entry `i`, fg=Black to fg=Rgb(40, 44, 52) — one-dark's background, i.e. the "action required" badge now taking its foreground from on_accent. The glyph grids, the style grids and every other legend row are unchanged, so nothing moved on screen and no other cell changed style.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76068df474
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| static EMIT_MODE: OnceLock<EmitMode> = OnceLock::new(); | ||
|
|
||
| // Test-only emit-mode override. Thread-local rather than global: the |
There was a problem hiding this comment.
The reviewed commit records both its author and committer as Codex <codex@openai.com>, but this repository explicitly prohibits Codex attribution in commits. Recreate the commit with the human author's metadata before merging.
AGENTS.md reference: AGENTS.md:L200-L202
Useful? React with 👍 / 👎.
| let muted = palette().muted; | ||
| let gutter = Style::default().fg(muted); | ||
| let body_bg = Color::Rgb(24, 24, 32); | ||
| let body_bg = palette().code_bg; |
There was a problem hiding this comment.
When a light theme such as solarized-light is active, this changes fenced blocks to a near-white code_bg, while HighlightLines below still uses the fixed base16-ocean.dark syntax theme. Its light foreground colors were designed for a dark background, so syntax-highlighted code can become very low-contrast or unreadable; select a matching light syntect theme or keep the background coupled to the selected syntax theme.
Useful? React with 👍 / 👎.
| tool: p.cyan, | ||
| plan: p.purple, | ||
| text: p.fg, | ||
| on_accent: p.bg, |
There was a problem hiding this comment.
For light themes this assigns the nearly white page background as the foreground for every accent and warning fill. In solarized-light, for example, #fdf6e3 on the warning fill #b58900 has only about 2.98:1 contrast, versus about 6.54:1 for the previous black foreground, making the action-required badge and warning search highlight substantially harder to read. Derive separate foregrounds from the actual accent and warning fills rather than reusing the theme background.
Useful? React with 👍 / 👎.
Routing the code background through the palette fixed the NO_COLOR bypass but broke light themes two ways, both of which review caught. The badge foreground was a single slot holding the theme background. That is right on dark themes and wrong on light ones: solarized-light paints cream on its #b58900 warning fill, about 3:1, where the black it replaced managed 6.5:1. The theme's text slot is no better — a mid-grey against the same fill is worse than the cream. The slot was the wrong shape, because one value cannot serve fills that sit at different luminances within a single theme. on_fill picks per fill instead: the better contrasting of the theme's background and text, and when neither clears 4.5:1, the achromatic pole that always does. Theme colours win wherever they are legible, so one-dark's badge is unchanged; the floor only engages where the palette cannot meet it. The fallback takes the same emit-adaptation hop as everything else, so NO_COLOR still strips it — the property the previous commit added and this must not undo. Second, the highlighter kept a fixed base16-ocean.dark palette while the code background it draws on had become theme-derived. On a light theme that is pale syntax colours on near-white. code_theme now follows the theme's polarity, preferring InspiredGitHub for light: the base16 light palettes are deliberately low-contrast and their keyword purple reaches only 2.4:1 on a cream background, which the new test rejects. Both fixes are pinned by contrast assertions rather than by naming expected colours, so they keep holding as palettes change: every built-in theme's fills must clear 4.5:1 for their badge text, and every highlighted run must clear 3:1 against the background it is painted on. Each was verified to fail when reverted. on_fill reads named ANSI colours through canonical xterm values, so the ANSI-16 accessibility themes get a real contrast decision rather than a fallback. Golden snapshots re-checked, not regenerated: the diff against the branch point is still the single permission_modal legend row, fg=Black to fg=Rgb(40, 44, 52). One-dark's background clears the floor on the warning fill, so the contrast pick returns exactly what the previous commit's fixed slot did and no further style row moved.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 375964d2e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| plan: p.purple, | ||
| text: p.fg, | ||
| bg: p.bg, | ||
| code_fg: p.yellow, |
There was a problem hiding this comment.
With the built-in solarized-light theme, this maps inline-code text to #b58900 while code_bg becomes approximately #f1ecdb; that pairing is only about 2.7:1 contrast, making snippets such as run() difficult to read. The new test only checks that the slots differ, so it misses this regression; derive code_fg against code_bg or enforce a meaningful contrast floor for every built-in palette.
Useful? React with 👍 / 👎.
| let Some(fill_l) = luminance(fill) else { | ||
| return p.bg; | ||
| }; |
There was a problem hiding this comment.
When AGENT_CODE_COLOR_MODE=ansi256 is used, or auto-detection selects ANSI-256 for Apple Terminal/tmux/screen, every RGB fill is an AnsiValue, so this branch bypasses the contrast calculation and always returns the theme background. For solarized-light's warning fill, the quantized cream-on-olive pairing is only about 3.3:1, reintroducing low-contrast text in the action required badge and search highlight even though the truecolor path deliberately falls back to black. Decode indexed colors to RGB (or make the black/white choice before quantization) instead of treating all indexed fills as unknowable.
Useful? React with 👍 / 👎.
Merging main brought the v0.28.0 version bump, and the header line the golden frames capture carries the version — so all four snapshots went stale on the merge commit itself, before any change in this branch. Verified by regenerating on the merge commit alone, with none of this branch's later work applied, and diffing that against regenerating here: byte-identical. The change is one glyph line per file, 0.27.0 to 0.28.0. No style grid, legend row or other glyph moved, which is what distinguishes a version restamp from a visual regression riding along with one.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de06e1312e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| }; | ||
| if expected != *actual { | ||
| if expected != actual { |
There was a problem hiding this comment.
When the next release bumps crates/cli/Cargo.toml, the rendered header will contain the new package version while all four golden files remain pinned to 0.28.0; comparing actual directly here therefore makes the snapshot suite fail despite no UI change. Keep normalizing the package version to a stable placeholder before writing or comparing snapshots so routine release bumps do not block the required CI gate.
AGENTS.md reference: AGENTS.md:L174-L181
Useful? React with 👍 / 👎.
My previous commit reverted #525. Regenerating the goldens against a working tree that still held the pre-merge snapshot.rs, then staging everything, deleted mask_version and re-pinned all four headers to a concrete 0.28.0 — reintroducing precisely the breakage #525 landed to end, where a release bump turns every frame snapshot red. Review flagged it as a P1 and was right. snapshot.rs is taken verbatim from main; this branch never had a reason to touch that file. The goldens are regenerated on top of it, so they now carry both the x.y.z placeholder and this branch's palette routing. Verified the masking rather than assuming it: with crates/cli set to 0.29.0 the snapshot suite still passes, and every header reads agent-code x.y.z. Every style row that differs from main is accounted for by the colour routing, and no glyph moved in any of the four frames: - DarkGray becomes Rgb(92, 99, 112), one-dark's muted; - Gray becomes Rgb(124, 131, 144), one-dark's inactive; - White becomes Rgb(171, 178, 191), the text slot; - Green becomes Rgb(152, 195, 121), the success slot; - Black on the warning fill becomes Rgb(40, 44, 52), which is what on_fill picks for one-dark. transcript_basic also loses a legend entry, and its style row shifts from i to e: bold Green and bold success are now the same style, so the two entries dedupe into one. Same pixels, one fewer legend line. transcript_light resolves against solarized-light instead — muted #93a1a1 and inactive #819295 — which is the point of routing through the palette rather than naming colours. Taking main's copy of any file this PR has no intent to change is the rule I should have applied when resolving the merge in the first place.
|
@codex review — restored the version masking that the merge dropped, regenerated goldens with palette changes |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
One conflict, in render.rs, where #520 routed the tasks pane's chrome through the palette and this branch had reworked the same overflow block. Kept this branch's structure (the footer only renders when it has something to report) with main's palette().muted. Followed #520 through the rest of the checklist rendering, which main could not see: the plan heading and the "+n more" row now take palette().muted and pending items palette().inactive, matching what main did to the sibling group headings and task headlines. Added a checklist to colour_probe_app so the_frame_uses_only_theme_colours covers those rows -- it had a task but no todos, so the new surface was outside the guard. One item, because the strip this frame affords windows a longer checklist down to its heading; `done` so the success colour is in the frame. Frame goldens are unchanged from main: regenerating on this branch reproduces them byte-for-byte, mask_version is intact, and the header still reads x.y.z.
Resolves the /clear conflict by keeping both sides: this branch factored the visual clear into clear_transcript_view (so a /clear deferred behind a resume can re-run it after the restore repaints), and main added the checklist reset from #519. ctx_meter is nulled inside clear_transcript_view, so main's separate assignment is redundant. Wires the session picker into #519's history-rewriting seam. A resume replaces the conversation exactly as /rewind and /snip do, so it now bumps conversation_epoch and rebuilds the pane from the restored messages via App::adopt_restored_todos; without it the checklist kept describing the session the user just left. The deferred /clear does the same, because a resume applied in between will have rebuilt the pane from a conversation that clear is about to empty. Also moves the session picker's own chrome onto the palette (#520): its rows used hardcoded Color::Gray/DarkGray.
… collapse work #519 (model checklist) and #520 (palette routing) landed on the same pane. Four conflict sites, resolved as: - app.rs: additive on both sides — `collapsed_groups` alongside `todos` and `conversation_epoch`. - pane sizing: `pane_rows_with_todos` now also takes the collapsed groups, so one computation accounts for both the checklist and folded rows. Keeps main's saturating `u16::try_from` — a model-authored checklist has no length limit and `as u16` would wrap a huge plan to a one-row request. - group heading: keeps the fold marker, the `(n)` count and the selection caret, with main's `palette().muted` instead of the hardcoded colour #520 removed. - overflow accounting: keeps main's footer-earns-its-row logic and reads the row anchors, so a surviving folded heading still accounts for its whole group instead of double-reporting those rows in "+n more". Checklist rows keep their own separate count.
1 participant
Summary
Closes the B8 / D11-09 gap. 77 hardcoded
Color::literals in the modern TUI bypassed the theme:This is not cosmetic. The theme adapts itself to the terminal's colour depth in
adapt_for_emit— Truecolor → Ansi256 → Ansi16 → Mono. A literalColor::DarkGrayin the chrome never passes through it, so underNO_COLORthose cells stayed coloured. no-color.org asks for no colour, not less of it.The test came first
the_frame_uses_only_theme_coloursrenders a frame underone-dark, whose slots are all RGB, and asserts no named colour survives — any named colour is by definition something the palette did not produce. Before the fix it failed with:…from a minimal frame. The frame it renders now also includes an error, a warning, a thinking block, an errored tool card and the tasks pane — the chrome most tempting to hand-colour.
Mapping
DarkGraymutedGrayinactiveGreen/Red/Yellowsuccess/error/warningWhitetextBlueaccentTest modules are untouched — several deliberately assert on literal colours.
The golden snapshots earned their keep
The snapshots from #508 failed on this change, which is exactly what they exist for. I reviewed the diff rather than blind-regenerating:
== glyphs ==section before and after), so nothing moved on screen.DarkGray → Rgb(92, 99, 112)andGray → Rgb(124, 131, 144)— one-dark'smutedandinactive.Verification
645 bin tests pass.
cargo test --workspace --all-targetsgreen apart from the 3bwrap_*tests, which fail on this host withsetting up uid map: Permission deniedand pass in CI.clippy --all-targets -- -D warningsandfmt --checkclean.Note
Color::BlackandColor::Resetare left alone:Resetis the "use the terminal default" signal that Mono mode collapses to, and the remainingBlackuses are explicit contrast backgrounds rather than themeable chrome. The guard test permitsResetfor that reason and would catch a newBlackin themeable chrome.