Skip to content

Themes: Fix washed-out diff accents and invisible word highlighting - #825

Open
masonmcelvain wants to merge 3 commits into
modem-dev:mainfrom
masonmcelvain:fix/theme-guard-washout
Open

Themes: Fix washed-out diff accents and invisible word highlighting#825
masonmcelvain wants to merge 3 commits into
modem-dev:mainfrom
masonmcelvain:fix/theme-guard-washout

Conversation

@masonmcelvain

Copy link
Copy Markdown

The two contrast guards in src/ui/themes.ts protected readability but destroyed theme identity in the process. This fixes both while keeping every existing contrast invariant.

Changes

  • readableDiffSign now rescues low-contrast accents with the smallest black/white blend that clears the 3:1 floor, stepping up from 2%, instead of a fixed 45% wash.
    • Saturation loss is proportional to how far out of range the accent actually is. catppuccin-latte's green (contrast 2.96, missing the floor by 0.04) now gets a ~2% nudge (#40a02b#3f9d2a) instead of being crushed to #235818.
  • Word-emphasis tints (addedContentBg/removedContentBg) now derive first at full readable strength, and the row tints step further down until the pair clears a 12 channel-distance separation floor (readableSeparatedRowBackground).
    • This fixes invisible word-level highlighting on everforest-light, one-dark-pro, and material-theme-palenight (row and content had collapsed to identical colors) and near-invisible highlighting on plastic (distance 6).
    • Pushing content higher would break the readability guarantee that caused the collapse. This is a deliberate deviation from the issue's suggestion.

Tests

  • Table-driven regression tests across all 66 bundled themes: accents that already meet the floor pass through unrescued, rescued accents stay within 2° of the source hue and clear the floor, and row-vs-content separation stays ≥ 12.

QA

  • Spot-check in a real terminal: --theme catppuccin-latte (sign colors), --theme one-dark-pro or --theme everforest-light (word-level highlighting on changed rows).
bun run src/main.tsx -- diff --theme catppuccin-latte
Theme 0.19.0 Feature
catpuccin-latte image image
one-dark-pro image image
everforest-light image image

masonmcelvain and others added 3 commits August 19, 2026 14:44
Low-contrast diff accents were rescued with a fixed 45% black/white
blend, washing genuine palette colors into pastel or mud — e.g.
catppuccin-latte's green (contrast 2.96, missing the floor by 0.04) was
crushed from #40a02b to #235818 even though a 2% nudge suffices. Since
sign colors seed the derived row tints and badges, the wash propagated
through the whole theme. Step the blend up from 2% until the contrast
floor passes instead, which preserves hue by construction and keeps
saturation loss proportional to how far out of range the accent is.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contrast stepping in readableTintedBackground could converge a theme's
row tint (0.12/0.2) and word-emphasis tint (0.18/0.28) onto the same
color, making word-level diff highlighting invisible — everforest-light,
one-dark-pro, and material-theme-palenight all collapsed to identical
pairs, and plastic sat 6 channel units apart. Derive the emphasis tints
first at full readable strength, then step the row tint further down
until the pair clears a minimum channel-distance floor.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 19, 2026

Copy link
Copy Markdown

@masonmcelvain is attempting to deploy a commit to the Modem Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps

greptile-apps Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

PR author is not in the allowed authors list.

@benvinegar

Copy link
Copy Markdown
Member

Pulled this onto fresh main and dug through it. The incremental sign-color rescue looks right, but I don’t think the PR is ready yet (feel free to push back on any of this).

The word-emphasis part seems to solve the same problem twice. diffRows.ts already checks whether a word-level highlight is distinct enough from its row background and strengthens it when needed, using a distance threshold of 28. This PR adds an earlier check in themes.ts with a threshold of 12. A color can therefore pass the new theme test and still be changed again by the renderer—77 of 130 current theme pairs do exactly that. Could we keep this policy in one place and test the final rendered highlight instead?

The new sign-color tests also don’t catch the old washout behavior. I replayed the old 45% blend against all 36 rescued catalog accents, and every one still passes the new contrast and hue assertions—blending toward black or white mostly preserves hue. A test for the first passing step, or at least a bound on how far the result can move from the source, would cover the regression better.

One smaller edge case: the rescue chooses white whenever background luminance is at most 0.45, but white can’t reach 3:1 once the background is above 0.30. For example, #aaaaaa ends up at about 2.32:1 against white, while black is over 9:1. None of the current bundled themes hit that range, but the helper should still uphold the contrast floor it promises.

I’d keep the incremental sign rescue, tighten its tests, and fold the emphasis fix into the existing renderer guard.

This comment was generated by Pi using GPT-5.6 Sol

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.

Theme derivation guards wash out diff colors and cause invisible highlighting

2 participants