Skip to content

fix(render): keep hardcoded white text readable on a light background - #370

Merged
kshivang merged 2 commits into
masterfrom
fix/light-theme-white-text
Aug 20, 2026
Merged

fix(render): keep hardcoded white text readable on a light background#370
kshivang merged 2 commits into
masterfrom
fix/light-theme-white-text

Conversation

@kshivang

Copy link
Copy Markdown
Owner

The bug

With a light terminal theme active, text from CLIs like Claude Code renders invisible.

Not a theme bug. Captured from a live Claude Code session under a pty:

^[[38;2;255;255;255m   <- pure white, primary text
^[[38;2;153;153;153m   <- #999999
^[[38;2;215;119;87m    <- #D77757
^[[48;2;0;0;0m         <- its own black background block

Claude Code emits truecolor, not palette indices. Pure white on a light floor
(#F5F7FB) is 1.07:1 contrast. And because a 24-bit color goes straight through
ColorUtils.convertTerminalColor, no theme or palette remap can reach it - the only
place to rescue it is per cell at paint time.

claude -p emits no color at all, so print mode proves nothing here. Repro:

(sleep 3; printf 'say hi\r'; sleep 25) | COLORTERM=truecolor script -q out.txt claude
cat -v out.txt | grep -oE '\^\[\[[0-9;]*m' | sort | uniq -c

The fix

ColorUtils.legibleOnLightBackground(fg, bg, minRatio) - the first public contrast util in
the repo; the math was otherwise duplicated privately in UiTheme.Companion and two test
files. Called from the two text-painting sites in TerminalCanvasRenderer: Pass 2
renderText and renderZWJSequence.

A failing color is corrected in two steps:

  1. Mirror its HSL lightness about 0.5, hue and saturation intact.
  2. Clamp toward black for whatever the mirror could not fix.
input result on #F5F7FB
#FFFFFF #000000 20:1, reads as ink
#999999 #666666 5.4:1, still a grey
#D77757 darker orange ~5:1, hue kept
#FFC107 dark amber 4.5:1 via the clamp

Both steps are needed: clamping alone lands white on a mid-grey (~#727272) and collapses
the CLI's own hierarchy, while a fully saturated color already sits near lightness 0.5, so
the mirror barely moves #FFC107.

New setting lightBackgroundMinContrast (default 4.5, 1.0 = off) with a slider in Visual
settings. Purely additive: both loaders use ignoreUnknownKeys, and loadFromFile()
already re-writes when the round-trip differs, so existing settings.json self-migrates.

Decisions worth reviewing

Each of these had a plausible-but-wrong alternative:

  • Measured against the background pass 1 actually painted (if (isInverse) baseFg else baseBg),
    never the theme default. That is what leaves Claude Code's own ESC[48;2;0;0;0m blocks
    alone and still fixes white-on-white INVERSE cells.
  • Backgrounds are never guarded, only glyphs. The other two baseFg/baseBg sites in
    the renderer paint backgrounds and are deliberately untouched.
  • Scoped to light backgrounds, gated on the WCAG 0.1791 pivot rather than a flat 0.5
    so a mid-grey floor still counts. Every dark theme therefore renders byte-identically - a
    symmetric guard would have brightened Claude Code's #505050 separators on the default
    dark theme.
  • Applied after DIM, which falls out right for free: plain white mirrors all the way to
    #000000 while dim white only reaches the floor at ~#727272, so ESC[2m still reads as
    secondary and applyDimColor needs no change.
  • Batching is unaffected: styleMatches already includes batchFgColor == fgColor and
    the guard runs before that comparison, so runs fragment only where colors genuinely differ.

Performance

The guard runs per cell per frame and a contrast check is six pow calls. Results are
memoised behind a single-entry memo (runs of identical style hit it) and an LRU, both keyed
by the active floor. The luminance work sits behind the cache rather than in front: on a
dark theme every cell bails out, so caching the identity result makes that path a field read.

Verification

  • 1082 compose-ui tests, 0 failures.
  • 13 unit tests over the color math (util/ColorUtilsContrastTest).
  • 3 tests that render through the real renderTerminal into an ImageBitmap and read the
    pixels back (rendering/LightBackgroundLegibilityTest). AGENTS.md forbids launching the
    app or screenshotting, and CanvasDrawScope + toPixelMap is the in-repo way to get real
    painted pixels - same approach as rendering/CursorOverlayTest.
  • Reverting the guard fails 7 of the 13 unit tests and 1 of the 3 pixel tests, so they
    are shown to catch the bug rather than assumed to.

Notes

  • Color.hsl() does not exist in Compose Multiplatform desktop ui-graphics (Android-only),
    hence the hand-rolled HSL pair. luminance() does exist and is what UiTheme already uses.
  • Out of scope, still broken: share/TerminalSnapshotEncoder.kt re-encodes cell styles
    to SGR for the browser mirror and has the same truecolor-white problem on a light theme.
  • Also flagged: flushBatch()'s two unreachable fallbacks disagree - :824 uses
    defaultForegroundColor, :846 uses Color.White. Harmless today; the white one would
    paint white if fgColor ever became nullable.
  • A companion fix lands separately in terminal-tab, whose synthesized light palette
    hardcoded white = #D1D5DA (1.27:1 on paper). This guard rescues the painted glyph, but
    the palette is what goes to the share-viewer mirror, so it needs fixing at the source too.

A truecolor CLI hardcodes a dark-terminal palette. Claude Code emits a
literal ESC[38;2;255;255;255m for its primary text, which is 1.07:1 - and
so invisible - against a light theme's paper floor. A 24-bit color bypasses
the theme and palette layers entirely, so the only place to rescue it is
per cell at paint time.

ColorUtils.legibleOnLightBackground adapts a failing glyph color in two
steps: mirror its HSL lightness about 0.5, preserving hue and saturation,
then clamp toward black for whatever the mirror could not fix. White
becomes near-black so primary text reads as ink rather than a washed-out
grey, #999999 stays a grey, and a brand orange stays orange. A fully
saturated color already sits near lightness 0.5, so the mirror barely moves
it; that is the case the clamp exists for.

Scoped to light backgrounds, gated on the WCAG 0.1791 pivot rather than a
flat 0.5 so a mid-grey floor still counts. Every dark theme therefore
renders byte-identically, and a cell carrying its own dark background -
Claude Code's ESC[48;2;0;0;0m blocks - keeps its white text. Measured
against the background pass 1 actually painted, so an INVERSE cell is
corrected against the right floor too.

Applied after DIM on purpose: plain white mirrors all the way to #000000
while dim white only reaches the floor at ~#727272, so ESC[2m still reads
as secondary without applyDimColor needing to change.

The guard runs per cell per frame and a contrast check costs six pow()
calls, so results are memoised behind a single-entry memo and an LRU, both
keyed by the active floor. The luminance work sits behind the cache rather
than in front of it: on a dark theme every cell bails out, and caching the
identity result makes that a field read.

Exposed as lightBackgroundMinContrast (default 4.5, 1.0 = off).

Verified with 13 unit tests over the color math and 3 that render through
renderTerminal and read the bitmap back - reverting the guard fails 7 of
the former and 1 of the latter. Full compose-ui suite: 1082 tests green.
@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review (part 1 of 2). Strong PR. The problem statement is the best part: a captured SGR dump, a real repro, and the observation that a 24-bit color bypasses every layer that could remap it. Measuring against the background Pass 1 actually painted (if (isInverse) baseFg else baseBg) rather than the theme default is the right call, and it is pinned by a test that fails under the plausible-but-wrong alternative. Settings plumbing is complete: TerminalSettings + TerminalSettingsOverride.withOverrides + one UI control is exactly useAntialiasings footprint, and nothing else in the repo enumerates setting names. Pixel tests through the real renderTerminal, plus the mutation check ("reverting fails 7 of 13 and 1 of 3"), are above the usual bar.

I could not run ./gradlew in this environment, so this is a static review.


1. Selection and search backgrounds are painted before the text pass, and the guard does not see them

renderSelectionHighlight draws a solid settings.selectionColorValue rect in Pass 1.5 (TerminalCanvasRenderer.kt:1281, "Use selection color directly"), and Pass 2 has no selection state in its color math. So for a selected cell the guard measures baseBg - the cell own background - while the pixels under the glyph are the selection color.

On a light theme with a dark selection color:

case before after
white truecolor, unselected invisible on paper (the bug) dark ink, fixed
white truecolor, selected white on dark selection, readable near-black on dark selection, invisible

Selecting was the one workaround that made this text readable, and the guard removes it. Solarized Light, the only light builtin, has selection = "0xFFEEE8D5" (light), so builtins are safe - but the theme in the bug report is the embedders synthesized light palette, and light-bg-plus-dark-selection is common (every dark builtin here uses one: 0xFF214283, 0xFF123A7A, 0xFF33467C). Theme.selectionText is documented as unwired (BuiltinThemes.kt:400), so nothing else catches it.

ctx already carries selectionStart/End/Mode and the search state. Resolving the effective background per cell (selection wash > search fill > cell bg) preserves the PRs own principle; skipping the guard for selected cells is also defensible and is a two-line change. Worth a test either way - it is the same class of mistake the "keeps its own dark cell background" test guards against, one pass later.


2. The guard rewrites Solarized Lights own ANSI palette - an unstated behaviour change

The guard runs on every foreground, including indexed colors the theme layer does control. On Solarized Light (bg = #FDF6E3, L about 0.923):

  • black = "0xFFEEE8D5", L about 0.807 - 1.14:1, guard fires, mirrors to L about 0.116 (a dark warm brown)
  • brightBlack = "0xFFFDF6E3" - identical to the background, 1.00:1, guard fires

Those are deliberate: Solarizeds light variant maps ANSI black/brightBlack to base2/base3 so programs setting a black background get the light tone. After this PR, ESC[30m and ESC[90m text on Solarized Light paints dark brown instead of pale. Arguably a fix (1.14:1 is unreadable either way), but it is exactly the case where the theme author did have a say, unlike truecolor, which is the premise of the change.

Either gate on style?.foreground?.isIndexed == false - matches the stated rationale, and collapses most of the hot-path cost since few cells are truecolor - or keep it broad, say so in the KDoc, and add a test pinning the new behaviour on SOLARIZED_LIGHT. every dark builtin theme is unaffected and every light one is corrected samples four fixed foregrounds and would not notice.

@claude

claude Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review (part 2 of 2).

3. Hot path: a global monitor and an allocation per cell on every memo miss

On a miss, legibleOnLightBackground takes synchronized(guardCacheLock), mutates an access-ordered LinkedHashMap (so the read itself is a write and must stay under the lock), then allocates a GuardMemo for the volatile store. Per cell, per frame, on every theme - including the dark ones the PR argues are provably untouched. Two panes rendering concurrently now share that one monitor and one volatile cache line.

The stated reason for putting the luminance check behind the cache does not quite hold:

a bail-out costs six pow() calls, so on a dark theme, where every cell bails, checking first would be a per-cell tax for no benefit

The bail-out does not need a contrast ratio, only bg.luminance() <= LIGHT_BACKGROUND_PIVOT - and the number of distinct backgrounds in a frame is one to three, not one per cell. A single-entry bgArgb -> isLight memo checked in front makes the dark-theme path one int compare plus one float compare, with no lock, no map mutation, no allocation. That is strictly cheaper than what is here for the exact case the comment optimises for. Given AGENTS.mds "99.5% allocation reduction" note for snapshot rendering, a per-cell allocation in the paint loop is against the grain of this file specifically.

4. minRatio is not part of the cache key, so a mismatch clears the whole cache

if (cachedMinRatio != minRatio) {
    guardCache.clear()
    cachedMinRatio = minRatio
}

Two reachable ways to thrash this:

  • Concurrent instances with different values. EmbeddableTerminal and TabbedTerminal both take a per-instance TerminalSettingsOverride, and the new field is in it. Two terminals composed at once with different lightBackgroundMinContrast clear all 512 entries on every alternation.
  • The new slider. valueRange = 1f..7f with no steps yields a distinct float per pointer position, so dragging clears the cache every frame.

Folding minRatio.toRawBits() into the key removes the mode entirely and is simpler than the clear-on-change branch. Related: Float.NaN in cachedMinRatio makes != always true - fine as the initial sentinel, but a NaN setting would clear per call and paint everything black. kotlinx cannot decode NaN from JSON by default so that is theoretical; a coerceIn on read is worth it anyway, since it also handles a hand-edited "lightBackgroundMinContrast": 50.

5. The guard runs for cells that paint nothing

It is computed at :1009, but isHidden, isBlinkVisible, space and NUL are not consulted until canBatch at :1029, and a space always takes the else branch and draws nothing. fgColor is only needed for styleMatches and the two paint calls. Spaces dominate a terminal screen; the single-entry memo hides most of the cost, but the work is waste and moving the call below the skip checks is low risk.

6. The unreachable-floor case is silent

The slider goes to 7:1, but for a background just above the pivot (L about 0.18) even pure black is only about 4.6:1, so computeLegible falls out with out = Color.Black and a still-failing ratio. Reasonable best-effort, but nothing tests it, and the all-builtins test only passes because it hardcodes floor = 4.5. Also for (step in 1 until CLAMP_STEPS) is a 19-step linear scan for something with a closed form - the target linear luminance is (bg.luminance() + 0.05) / minRatio - 0.05 - so one solve would be exact, cheaper, and would remove the magic 20.

7. The de-duplication the PR describes is not finished

the math was otherwise duplicated privately in UiTheme.Companion and two test files

UiTheme.kt:216 still has a byte-identical private contrastRatio, and UiTheme still carries its own mix - and since the new ColorUtils.mix is private, that one is now duplicated twice. Making the util public is the right move; collapsing the existing copy is the payoff. One wrinkle: ColorUtils already imports settings.theme.{Theme, ThemeManager, ColorPaletteManager}, so UiTheme -> ColorUtils closes a package cycle. Legal in Kotlin, but if you would rather not, hoisting contrastRatio into a small colorspace util both can depend on beats leaving the duplicate with no comment.

8. Minor

  • valueDisplay = { if (it <= 1f) "Off" else "%.1f:1".format(it) } renders 1.0:1 for values in (1.0, 1.05], where the guard is enabled but a no-op. A steps value (e.g. 12 for 0.5 stops) fixes the label, makes Off a deliberate stop rather than a range edge, and incidentally fixes the drag-thrash in 4.
  • invalidateColorCache() clearing the guard caches is harmless but not needed for correctness: the key already contains both resolved colors, so a theme change produces different keys anyway. The KDoc claims it is required ("a theme change moves the background those results were solved against"), which will mislead the next reader.
  • contrastRatio ignoring alpha is documented and the tests pin that alpha survives, but a translucent guarded fg still composites to a failing ratio. No path produces one today (applyDimColor scales channels, not alpha), so this is a note rather than a bug.
  • Good call flagging flushBatch()s :824 vs :846 disagreement and TerminalSnapshotEncoder as out of scope. Both deserve follow-up issues so they do not get lost in the PR body.
  • I checked whether the cursor overlay is a fourth site needing the guard: it is not. cursorTextColor comes from theme.cursorTextColor (ProperTerminal.kt:2709), not the cell style, so it is theme-controlled already.

Summary: 1 is the one I would want resolved before merge - it is the same "measure what was actually painted" principle the PR gets right in Pass 1 and misses in Pass 1.5, and it regresses a case that used to work. 2 is a scope/documentation decision that would also make 3 mostly moot. 3 and 4 are worth fixing while the code is fresh; the rest is polish.

The web viewer had the same bug the renderer just fixed: on a light theme, a
CLI that hardcodes truecolor white paints at 1.07:1 and vanishes.

The renderer's guard cannot reach it. Two independent reasons, both checked
rather than assumed:

- The viewer renders through xterm.js, whose glyph pipeline has no per-cell
  foreground hook. registerDecoration is per-marker and registerCharacterJoiner
  carries no color, so there is nowhere to run mirror-and-clamp client side.
- Correcting server side would only fix snapshots. TerminalSnapshotEncoder
  handles PaneSnapshot and screen repaints, but live text is the raw pty string
  forwarded verbatim by MirrorShare's output tap, so the truecolor a CLI emits
  never passes through anything host-side that could rewrite it. Rewriting the
  live stream would mean re-parsing SGR in the mirror path and fighting the
  graphics filter.

What xterm.js does have is its own minimumContrastRatio, which takes a floor
and corrects per cell. So the host sends the floor instead of colors:
ServerMessage.Theme gains minimumContrastRatio, set from the user's
lightBackgroundMinContrast when the terminal floor is light and 1.0 (off)
otherwise, via the new ColorUtils.lightBackgroundGuardRatio so the light-vs-dark
pivot is not restated. Both theme producers send it - MirrorShare for GUI tabs
and DaemonShareServer for headless sessions.

The correction style still differs from the in-app renderer: xterm.js clamps to
the floor rather than mirroring lightness, so white lands on a strong grey in
the viewer where the app paints it as ink. Both are readable, and this keeps one
well-maintained implementation instead of a second copy of the algorithm that
could drift.

The floor is validated in viewer-logic.js rather than trusted: it crosses the
wire, and xterm.js throws on some out-of-range option values, so a stale or
hostile host must not be able to take the viewer down. Clamped to [1, 21] at one
decimal, matching xterm.js's own normalisation, with anything unparseable
meaning off. The option is assigned unconditionally so switching back to a dark
theme disarms the guard instead of latching the previous floor.

Additive on the wire: the field is defaulted and ShareProtocol decodes with
ignoreUnknownKeys, so an older host that never sends it leaves the guard off
rather than failing to decode a theme frame.

Verified by two new scenarios in the fake-browser harness, which run the shipped
viewer.js under Node and assert the option that actually reaches the terminal -
including a pane created after the theme frame. Removing the one-line wiring
fails them. Plus 12 wire-validation cases under Node, a protocol round-trip with
a legacy-decode case, and a test pinning that the viewer's floor and the
renderer's own gate agree for every builtin theme. Full suite: 1084 green.
@kshivang

Copy link
Copy Markdown
Owner Author

Follow-up commit: the share viewer

e3c288a9 extends this PR to the web viewer, which had the same bug.

The renderer's guard could not be reused there, for two reasons I checked rather than assumed:

  • The viewer renders through xterm.js, whose glyph pipeline has no per-cell foreground
    hook - registerDecoration is per-marker and registerCharacterJoiner carries no color.
  • Correcting server side would only fix snapshots. TerminalSnapshotEncoder handles
    PaneSnapshot and screen repaints, but live text is the raw pty string forwarded
    verbatim
    by MirrorShare's output tap, so the truecolor a CLI emits never passes through
    anything host-side that could rewrite it.

What xterm.js does have is its own minimumContrastRatio, which takes a floor and corrects
per cell. So the host sends the floor instead of colors: ServerMessage.Theme gains
minimumContrastRatio, set from lightBackgroundMinContrast when the terminal floor is
light and 1.0 (off) otherwise, through the new ColorUtils.lightBackgroundGuardRatio so
the light-vs-dark pivot is not restated. Both producers send it - MirrorShare for GUI tabs,
DaemonShareServer for headless sessions.

Known difference, called out deliberately

xterm.js clamps to the floor rather than mirroring lightness, so white lands on a strong
grey in the viewer where the app paints it as ink. Both are readable. I chose this over
porting mirror-and-clamp into JS to avoid a second copy of the algorithm that could drift
from the Kotlin one.

Robustness

The floor is validated in viewer-logic.js, not trusted - it crosses the wire and
xterm.js throws on some out-of-range option values, so a stale or hostile host must not be
able to take the viewer down. Clamped to [1, 21] at one decimal, matching xterm.js's own
normalisation; anything unparseable means off. The option is assigned unconditionally, so
switching back to a dark theme disarms the guard instead of latching the previous floor.

Additive on the wire: the field is defaulted and ShareProtocol decodes with
ignoreUnknownKeys, so an older host that omits it leaves the guard off rather than failing
to decode a theme frame.

Verification

  • Two new scenarios in the fake-browser harness, which runs the shipped viewer.js under
    Node and asserts the option that actually reaches the terminal - including a pane created
    after the theme frame. Removing the one-line wiring fails them.
  • 12 wire-validation cases under Node.
  • A protocol round-trip plus a legacy-decode case.
  • A test pinning that the viewer's floor and the renderer's own gate agree for every
    builtin theme, so the two surfaces can never disagree about whether a theme is light.
  • Full suite: 1084 tests, 0 failures.

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review: light-background legibility guard

Strong PR. The diagnosis holds up (truecolor bypasses the palette layer, so per-cell paint time really is the only hook), mirror-then-clamp is well motivated, the guard is measured against the background pass 1 actually painted rather than the theme, and the pixel-level tests through the real renderTerminal are the right call given AGENTS.md forbids launching the app.

I could not run ./gradlew build here, so this is static review only.

1. Duplicated dangling comment in viewer.js

share-viewer/viewer.js:2226-2232 — the three-line comment appears twice, the second copy sitting alone just before the closing } with no code under it. Copy-paste artifact; drop 2230-2232.

2. The Kotlin side doesn't validate the floor the way the JS side does

viewer-logic.js rejects non-finite values and clamps to [1, 21] because "the value crosses the wire". But lightBackgroundMinContrast also crosses a trust boundary — ~/.bossterm/settings.json is hand-editable and TerminalSettingsOverride.lightBackgroundMinContrast is embedder-supplied — and legibleOnLightBackground takes it raw.

  • 100 is degenerate: the clamp loop never reaches the ratio, so every glyph on a light background paints pure black.
  • NaN is worse. if (minRatio <= 1f) return fg is false for NaN, so it proceeds; it.minRatio == minRatio is never true so the memo never hits; and cachedMinRatio != minRatio is always true, so the whole 512-entry LRU is cleared once per cell per frame. (kotlinx rejects NaN literals by default, so the realistic vector is the override lambda, not the file — but the fix is free.)
if (!(minRatio > 1f)) return fg          // also catches NaN
val floor = minRatio.coerceAtMost(21f)   // 21:1 is the max achievable

3. fg == bg concealment and deliberately-subtle UI get slammed to full contrast

SGR 8 is handled (isHidden skips the draw), but the older foreground-equals-background idiom is not — and on a light theme "white text on the paper floor" is exactly what that looks like, so concealed text now renders as ink. Same mechanism catches TUIs that paint low-contrast separators or shadows deliberately: #E5E5E5 on #F5F7FB is 1.15:1 and becomes a hard dark line.

Inherent to any contrast guard, and maybe the right trade — but it is the mirror image of the argument the PR makes for not going symmetric on dark themes (#505050 separators). Worth either an if (fg == bg) return fg carve-out or a sentence in the KDoc.

4. The host guard and the viewer guard are not the same guard

The vendored xterm.js ensureContrastRatio branches on if (n < r) reduceLuminance else increaseLuminance — it brightens as well as darkens. So on a light host theme the viewer will lighten dark glyphs sitting on a cell's own dark background, which is precisely the case LightBackgroundLegibilityTest."text keeps its own dark cell background untouched" pins as must-not-change on the host. Not necessarily wrong, but the share-path comments read as "same decision pushed down", and it isn't. Worth a line in ShareProtocol.Theme's KDoc plus a harness assertion so it's a known divergence.

Related: the "Out of scope, still broken: TerminalSnapshotEncoder" note in the description looks stale — that scrollback is painted by the same xterm.js instance, so arming minimumContrastRatio covers it.

5. The slider is continuous while everything downstream rounds

VisualSettingsSection.kt:111 omits steps but displays %.1f. The user sees 4.4:1 while 4.3789062 is persisted and used by the renderer, and validMinimumContrastRatio rounds the wire copy to 4.4 — so host and viewer apply different floors from the same drag. Every sibling slider here pins granularity (Font Size 15, Background Opacity 17, Blur Radius 8); steps = 11 gives 0.5 increments over 1f..7f.

6. Cache design

  • cachedMinRatio thrash: two TabbedTerminal/EmbeddableTerminal instances in one process with different overrides alternate that field every frame and wipe all 512 entries each time. Folding minRatio into the key removes the failure mode and is strictly cheaper than the clear.
  • This is now a second synchronized LRU on the same per-cell path getOrCreateTruecolor already locks. renderText right above already builds a frame-local textStyleCache HashMap and explains why ("Frame-local so it stays thread-safe on the shared renderer object") — the same shape here would be lock-free and correctly scoped to one minRatio.
  • The guard call sits above the blank/hidden/blink test in renderText, so blank cells pay for it. canBatch already excludes blanks, so it can move below.

7. invalidateColorCache() doesn't need to clear these

The key is (concrete fg ARGB, concrete bg ARGB) and computeLegible is pure in those plus minRatio, so a theme change cannot stale an entry — the new background is just a different key. Harmless, but the comment describes a dependency this cache doesn't have and will mislead the next reader.

8. The advertised de-duplication didn't land

The KDoc says the math is "otherwise duplicated privately in UiTheme.Companion and two test files", and mix's doc says "the same reason UiTheme carries its own mix" — but UiTheme.kt:216 still holds a byte-identical private contrastRatio. Now that a public one exists, delegating is two lines and makes the doc true. (Adjacent: UiTheme.isDark uses luminance() < 0.5f while the guard uses the 0.1791 pivot; two "is this dark?" definitions now coexist.)

9. Minor: share gates on the theme, renderer on the setting

MirrorShare/DaemonShareServer pass theme.backgroundColorValue; the renderer resolves ctx.settings.defaultBackgroundColor. ThemeManager syncs them, but TerminalSettingsOverride.defaultBackground can move one without the other — then the app guards and the viewer doesn't.

Tests

Good coverage; the BuiltinThemes.ALL sweep is a nice way to make "dark themes render byte-identically" a standing invariant. Three gaps:

  • Nothing covers the INVERSE path. if (isInverse) baseFg else baseBg is a headline decision and the one most likely to regress silently. A pixel test with an inverse white-on-white cell would pin it.
  • "every dark builtin theme is unaffected…" hardcodes 0.1791f instead of calling ColorUtils.isLightBackground (the sibling test does). If the pivot moves, that test quietly asserts the wrong partition.
  • The suite reads/writes process-global caches, and "the cache returns the same answer as a cold computation" never starts cold. @BeforeTest { ColorUtils.invalidateColorCache() } would make the name true and the suite order-independent.

Checked and correct

toHsl's hue ties (max == r == g, max == g == b) resolve right; mirror-then-clamp is monotone on a light bg so the guard can never worsen a passing color and idempotency follows; the pre-set out = Color.Black covers the no-step-qualifies case; fg.toArgb().toLong() shl 32 is sign-extension-safe; guardMemo is an immutable holder behind one volatile field so it can't be read torn; encodeDefaults = true means the floor is always on the wire, so "switch back to dark disarms" can't silently omit it; the setting is wired everywhere its sibling useAntialiasing is and nothing else enumerates settings fields; and flushBatch's text + underline and renderCharacter all receive the guarded color, so no text path escapes.

Nothing above is a blocker except possibly #1 and #2, both one-liners.

@kshivang
kshivang merged commit 048d77f into master Aug 20, 2026
5 checks passed
kshivang added a commit to risa-labs-inc/boss-plugin-terminal-tab that referenced this pull request Aug 20, 2026
The synthesized light terminal theme hardcoded white = #D1D5DA, which is 1.27:1
against the Blueprint Light floor: ESC[37m painted nothing readable. It was also
inconsistent with brightWhite two lines below, which already resolved to the
host foreground, so ANSI 7 and 15 disagreed about which direction "white" meant.

ANSI 7/15 invert on a light floor, the way Solarized Light maps them to its two
darkest inks rather than to greys. white now comes from the host's own
TextSecondary token, so it stays correct for Daylight as well as Blueprint Light
instead of pinning another literal.

The new test sweeps all 16 ANSI slots against the floor rather than pinning this
one value, so the next hardcoded light grey cannot slip back in. Held to 3:1
rather than the 4.5:1 text floor because red/green/yellow are brand-carrying
status colors taken from the host tokens.

BossTerm's renderer also rescues this class of color at paint time now
(kshivang/BossTerm#370), but that only covers the painted glyph: this palette is
what gets handed to the share-viewer browser mirror, so it has to be right at
the source too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant