fix(tui): decode ANSI alpha-channel encoding in syntax themes#13382
fix(tui): decode ANSI alpha-channel encoding in syntax themes#13382joshka-oai merged 4 commits intoopenai:mainfrom
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR fixes ANSI-family syntax themes in the TUI by decoding syntect’s alpha-channel ANSI palette encoding (as used by bat), so ansi, base16, and base16-256 themes emit terminal palette colors instead of near-black hardcoded RGB values.
Changes:
- Added syntect color decoding that maps alpha-encoded ANSI indices to ratatui named/indexed colors, preserving “terminal default” semantics.
- Refactored the highlighter core to accept an explicit theme reference to support theme-specific tests without mutating global state.
- Added contract validation + warnings for ANSI-family themes and introduced snapshot/regression tests for palette outputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
codex-rs/tui/src/render/highlight.rs |
Implements alpha-based syntect→ratatui color decoding, contract validation/warnings, refactors highlighting for testability, and adds extensive tests. |
codex-rs/tui/src/render/snapshots/codex_tui__render__highlight__tests__ansi_family_foreground_palette.snap |
Adds snapshot output to lock in expected ANSI-family foreground palette behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ 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". |
Replace the warn-once bitmap in `tui/src/render/highlight.rs` with a simple warning path that logs every unexpected alpha marker. This removes global atomic state and keeps the alpha decoding logic straightforward while preserving RGB fallback behavior for unknown values.
Keep `convert_syntect_color` resilient by treating unexpected alpha values as plain RGB without emitting runtime warnings. Built-in themes can legitimately contain these values, so warning spam adds noise without improving correctness or user actionability.
Update style-conversion tests in `tui/src/render/highlight.rs` to use `matches!` pattern assertions instead of constructing `RtColor::Indexed` and `RtColor::Rgb` values directly. This keeps the assertions equivalent while satisfying the crate-level `clippy::disallowed_methods` policy in test code.
02f3d07 to
a90aee6
Compare
etraut-openai
left a comment
There was a problem hiding this comment.
Looks pretty good. I left one comment — not sure the warning is needed here. Seems to be adding a lot of code for little benefit.
joshka-oai
left a comment
There was a problem hiding this comment.
When a module grows a large percentage of code like this, it's worth considering whether the code can be split into a new more focused module (e.g. highlight/syntect_theme.rs (or whatever makes sense - theme.rs?). This helps keep the tests, and docs about why and how this code works pretty well self-contained / local / collocated and makes the additive and changed parts of a change like this more obvious. A bunch of the mental model docs in the PR probably belong in the code too.
Stamped - mainly because this improves how this looks significantly, so code nits are more about making it read nicely.
Reword user-facing theme warnings to be clearer and less jargon-heavy. Remove runtime ANSI-family contract diagnostic (caught by tests, not actionable by users). Downgrade duplicate-override and resolve-fallback messages to debug level since they are developer-only breadcrumbs.
Problem
The
ansi,base16, andbase16-256syntax themes are designed to emit ANSI palette colors so that highlighted code respects the user's terminal color scheme. Syntect encodes this intent in the alpha channel of itsColorstruct — a convention shared withbat— butconvert_stylewas ignoring it entirely, treating every foreground color as raw RGB. This caused ANSI-family themes to produce hard-coded RGB values (e.g.Rgb(0x02, 0, 0)instead ofGreen), defeating their purpose and rendering them as near-invisible dark colors on most terminals.Reported in #12890.
Mental model
Syntect themes use a compact encoding in their
Colorstruct:alphar0x00RtColor::Black…Grayfor 0–7,Indexed(n)for 8–2550x01None— inherit terminal default fg/bg0xFFRtColor::Rgb(r, g, b)RtColor::Rgb(r, g, b)(silent fallback)This encoding is a bat convention that three bundled themes rely on. The new
convert_syntect_colorfunction decodes it;ansi_palette_colormaps indices 0–7 to ratatui's named ANSI variants.Non-goals
convert_styledoes not apply it..tmThemesupport for ANSI encoding — only the bundled themes use this convention.Tradeoffs
Black,Red, …,Gray) rather thanIndexed(0)…Indexed(7). This lets terminals apply bold/bright semantics to named colors, which is the expected behavior for ANSI themes, but means the two representations are not perfectly round-trippable.Architecture
All changes are in
codex-rs/tui/src/render/highlight.rs, within the style-conversion layer between syntect and ratatui:convert_styledelegates foreground mapping toconvert_syntect_colorinstead of inlining theRgb(r,g,b)conversion. The core highlighter is refactored intohighlight_to_line_spans_with_theme(accepts an explicit theme reference) so tests can highlight against specific themes without mutating process-global state.ANSI-family theme contract
The ANSI-family themes (
ansi,base16,base16-256) rely on upstream alpha-channel encoding from two_face/syntect. We intentionally do not validate this contract at runtime — if the upstream format changes, theansi_themes_use_only_ansi_palette_colorstest catches it at build time, long before it reaches users. A runtime warning would be unactionable noise.Warning copy cleanup
User-facing warning messages were rewritten for clarity:
warntodebugObservability
ansi_themes_use_only_ansi_palette_colorstest enforces the ANSI-family contract at build time..tmThemefiles.Tests
alpha=0x00with low index (named color),alpha=0x00with high index (Indexed),alpha=0x01(terminal default), unexpected alpha (falls back to RGB), ANSI white → Gray mapping.ansi_family_themes_use_terminal_palette_colors_not_rgb— highlights a Rust snippet with each ANSI-family theme and asserts zeroRgbforeground colors appear.ansi_family_foreground_palette_snapshot— records the exact set of unique foreground colors each ANSI-family theme produces, guarding against regressions..tmThemefiles, and bundled theme resolution.Test plan
cargo test -p codex-tuipasses all new and existing testsansi,base16, orbase16-256theme and verify code blocks render with terminal palette colors (not near-black RGB)dracula) and verify no regression in color output