fix(theme): tokenize the remaining chrome surfaces and make the exemptions explicit - #438
Merged
Merged
Conversation
…tions explicit Second batch of issue #341 (#341): the prose reading surface, code-card chrome, scrims, Mermaid canvas, tool output, disabled send chip, and composer placeholders were still pinned by `:root[data-theme="…"]` literals that raised specificity above the base rule *and* skipped the variable, so no contributed theme could reach them. - 11 new tokens in tokens.css, each defaulting to the literal (or literal mix) the sheet painted before, so the built-in light/dark paint is unchanged. - Prose and transcript ink mixes now read `--ds-text-primary`, whose light value was the hardcoded `#1a1c1f`; four light overrides became pure duplicates and are gone, and the keycap ink moved into `--ds-prose-kbd-fg`. - The Shiki `one-dark-pro` / `one-light` plate and its ink stay with the Shiki theme (they are emitted as inline colours on the highlighted markup), so they are exempted by value with the reason recorded instead of being rewritten. - scripts/style-surface-tokens.mjs now holds every colour declaration inside a `:root[data-theme]` rule plus the base rules of the migrated families to the token contract, and enumerates its exemptions (Shiki palette, translucent shadow values, the ⌘K veil) so no gap is invisible. - Kept the light-qualified `.tool-row-content` rule: at (0,3,0) it is what keeps light from tinting error output and giving plain tool blocks a fill. Removing it would have been a silent paint change in light only. - §6.4 (en/zh) records the new surface-layer rows, the convention, and the exemptions; E2E-078's probe now pins 19 surfaces, including a plain/error tool-row cascade pair, against paint sampled from the pre-change app. Test assertions updated because they encoded the old literals or the deleted overrides: surface-polish (light `.overlay` scrim), markdown-prose-style (light link underline, inline-code chip, blockquote ink), plugins-page-style (light plugin backdrop), mermaid-rendering (light diagram canvas) — each now asserts the token and its light value, so the intent survives and the coverage is stronger.
There was a problem hiding this comment.
🟡 Changes recommended
Four moderate findings remain: missing surface assertions and two guard-matcher gaps.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Completes the remaining renderer surface-token migration, documents intentional exemptions, and expands static and E2E validation.
Changes:
- Adds tokens for prose, code, overlays, tool rows, disabled controls, and placeholders.
- Extends style-token checks, fixtures, and regression tests.
- Updates English and Chinese design-system and E2E documentation.
File summaries
| File | Reviewed changes and findings |
|---|---|
scripts/style-surface-tokens.mjs |
Expanded guard and exemptions. Moderate findings: color() values are not matched (2 votes); opaque shadow alpha is accepted (1 vote). |
scripts/e2e/theme-surfaces.js |
Added surface sampling and custom-theme checks. Moderate findings: missing hover/disabled-foreground coverage (3 votes); baseline keycap ink is not checked (1 vote). |
scripts/e2e-theme-surfaces.mjs |
Extended the Electron fixture. |
docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md |
Updated localized E2E coverage. |
docs/zh-CN/spec/04-ux/07-ui-design-system.md |
Documented localized token guidance. |
docs/spec/06-delivery/04-e2e-test-plan.md |
Updated E2E scenario expectations. |
docs/spec/04-ux/07-ui-design-system.md |
Documented tokens and exemptions. |
apps/desktop/test/theme-surface-tokens.test.mjs |
Expanded guard regression tests. |
apps/desktop/test/surface-polish.test.mjs |
Updated scrim assertions. |
apps/desktop/test/plugins-page-style.test.mjs |
Verified modal-veil tokenization. |
apps/desktop/test/mermaid-rendering.test.mjs |
Verified Mermaid tokenization. |
apps/desktop/test/markdown-prose-style.test.mjs |
Updated prose-token assertions. |
apps/desktop/src/styles/ui-kit.css |
Uses the scrim token. |
apps/desktop/src/styles/tokens.css |
Defines new light and dark surface tokens. |
apps/desktop/src/styles/theme-overrides.css |
Uses placeholder and tool-row tokens. |
apps/desktop/src/styles/prose.css |
Tokenizes prose and code-card surfaces. |
apps/desktop/src/styles/plugins.css |
Uses the modal-veil token. |
apps/desktop/src/styles/overlays.css |
Documents the search-overlay exemption. |
apps/desktop/src/styles/messages.css |
Uses the tool-row token. |
apps/desktop/src/styles/composer.css |
Tokenizes disabled-send styling. |
apps/desktop/src/styles/composer-menus.css |
Uses the placeholder token. |
apps/desktop/src/styles/chat-shell.css |
Removes a redundant literal override. |
Review details
Suppressed comments (4)
scripts/e2e/theme-surfaces.js:77
inkRgbais only used in the custom-theme branch, while the built-in-paint loop still comparesvalue.rgbafor every surface. For the newkbdentry this verifies the keycap background, not--ds-prose-kbd-fg, so a wrong default keycap ink could pass despite the claim that the pre-fix paint is pinned. Compare the channel-specific sample in the non-custom baseline as well.
ink: style.color,
inkRgba: rgba(style.color),
scripts/style-surface-tokens.mjs:65
SHIKI_PALETTEis stripped from every declaration before the selector is considered, so any migrated family can use one of these colours and evade the guard. For example,.mermaid-block-body { background: #fafafa; }is treated as clean even though that is not a Shiki plate. Restrict this exemption to the Shiki-owned selectors (and add a regression test for the same literal on a non-Shiki family).
/** one-dark-pro / one-light plate and ink (see the file header). */
const SHIKI_PALETTE = /#(?:282c34|abb2bf|383a42|fafafa|d7dae0)\b/gi;
scripts/style-surface-tokens.mjs:78
- The prefix regex means
allowed()also exempts descendants such as.search-overlay .search-dialogfrom the overlay's literal-background check. This exemption is intended only for the.search-overlaybase rule; otherwise a new descendant can introduce a raw background without a guard failure.
selector: /\.search-overlay/,
scripts/style-surface-tokens.mjs:62
BOX_SHADOW_ALPHAaccepts any numeric alpha, including1and100%, so an opaque shadow such asbox-shadow: 0 0 0 1px rgba(0, 0, 0, 1)is stripped and passes the guard. That contradicts the documented exemption that only translucent black/white shadows are allowed; restrict the matcher to alpha values below 1 and add an opaque-rgba regression case.
const BOX_SHADOW_ALPHA =
/#(?:0{6}|f{6})(?!f{2})[\da-f]{2}\b|\b(?:rgba|rgb)\(\s*0\s*[, ]\s*0\s*[, ]\s*0\s*[,/]\s*[\d.]+%?\s*\)|\b(?:rgba|rgb)\(\s*255\s*[, ]\s*255\s*[, ]\s*255\s*[,/]\s*[\d.]+%?\s*\)/gi;
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+30
to
+34
| codeHead: [".code-block-head", "--ds-code-head-bg"], | ||
| mermaidCanvas: [".mermaid-block-body", "--ds-mermaid-canvas"], | ||
| thinkingCode: [".thinking-prose code", "--ds-thinking-code-bg"], | ||
| sendDisabled: [".send-btn:disabled", "--ds-send-disabled-bg"], | ||
| kbd: [".prose-chat kbd", "--ds-prose-kbd-fg", "ink"], |
| const THEME_ROOT = /^:root(?:\[data-theme=["'][^"']*["']\])?$/; | ||
|
|
||
| /** Colour literals: hex, colour functions, and the `white` / `black` keywords. */ | ||
| const LITERAL = /#[\da-f]{3,8}\b|\b(?:rgba?|hsla?|hwb|lab|lch|oklab|oklch)\(|\b(?:white|black)\b/i; |
…ng-surface-tokens
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes out #341. The issue reported that
:root[data-theme="..."]overrides wrote literal surface colours, which both raise specificity to (0,2,0) and skip every--ds-*token, so no contributed plugin theme can reach those surfaces. Thesettings.csschrome batch (settings rail, search, nav item, switch knob, capability/plugins search, composer shell) was already migrated onmain—scripts/style-surface-tokens.mjsguards it andpnpm check-style-tokensis green, so this PR does not redo it.This PR covers what was left, and makes the deliberate exemptions explicit rather than leaving them as invisible gaps.
What was tokenized
New tokens in
apps/desktop/src/styles/tokens.css, each defaulting to exactly the value the sheet painted before (the light values were the literals the overrides hardcoded):--ds-prose-kbd-fg#303030var(--ds-text-secondary)--ds-thinking-code-bg#f0f0f0color-mix(in oklab, var(--ds-text-primary) 4.5%, transparent)--ds-mermaid-canvas#ffffffcolor-mix(in oklab, var(--ds-bg-primary) 94%, var(--ds-text-primary))--ds-code-head-bgcolor-mix(in oklab, #1a1c1f 3.5%, transparent)color-mix(in oklab, #fff 4%, transparent)--ds-code-hover-bgcolor-mix(in oklab, #1a1c1f 6%, transparent)color-mix(in oklab, #fff 8%, transparent)--ds-scrimcolor-mix(in oklab, #1a1c1f 28%, transparent)color-mix(in oklab, #000 45%, transparent)--ds-modal-veilcolor-mix(in oklab, #1a1c1f 32%, transparent)color-mix(in oklab, var(--ds-bg-primary) 78%, transparent)--ds-tool-row-bgcolor-mix(in oklab, #1a1c1f 2%, transparent)var(--ds-tile)--ds-send-disabled-bg/--ds-send-disabled-fg#8e8e90/#ffffffcolor-mix(..., 18%)/color-mix(var(--gray-900) 70%, transparent)--ds-placeholder-ink#4a4c4fcolor-mix(in oklab, var(--gray-0) 42%, transparent)The light-prose ink mixes now read
--ds-text-primary, whose light value is the#1a1c1fthey used to hardcode — paint-identical, but a theme can move the whole light ink tier at once.What was deliberately NOT tokenized
one-dark-pro/one-lightbackgrounds and ink in.code-block/.prose-chat pre). The stylesheet states outright that Shiki owns the code-block background; converting it would change rendering and contradict a design decision. It is now named inscripts/style-surface-tokens.mjswith that reason instead of being an unlisted gap..tool-block.is-plain .tool-row-content,.tool-row-content.is-error), where light keeps its own lighter tile over the plain and error variants. Recorded aspinnedin the cascade fixture so the decision is visible and still has to be revisited deliberately.The guard was extended from a handful of chrome selectors to the full migrated set plus explicit exemptions, and it now says what it does and does not cover.
docs/spec/04-ux/07-ui-design-system.md§6.4 lists every reachable token and states the convention that:root[data-theme]overrides must not write literal surface colours; thezh-CNmirror is updated too.Paint-identity is measured, not asserted
pnpm test:e2e:theme-surfacesboots real Electron, links the built stylesheet, and samples the computed RGB of each surface in light, light+custom theme, dark and dark+custom theme. The new surfaces were added to that fixture with their pre-fix RGBA pinned (scrim [25,29,32,71],modalVeil [25,28,31,82],codeHead [28,28,28,9],mermaidCanvas [255,255,255,255],thinkingCode [240,240,240,255],sendDisabled [142,142,144,255],kbd [26,26,26,20], …), and each token is also asserted to actually move when a custom theme sets it — so this proves both "the built-in look is unchanged" and "the surface is now reachable".Validation
On the integrated local
main(merge commitee5b00a1, after merging this branch):node scripts/check-style-tokens.mjsstyle tokens OKpnpm build:jspnpm --filter @pi-desktop/desktop typecheckpnpm lint:biomepnpm -r --if-present testpnpm docs:checkpnpm test:e2e:theme-surfacesok: true,failures: []pnpm test:e2e:layoutpnpm test:e2e:bootpnpm test:e2e:transcriptTest assertions updated
Four existing files pinned the old literals. Each was moved from "the override writes this literal" to "the token exists twice (both palettes) and the override is gone", which strengthens the intent rather than weakening it:
surface-polish.test.mjs—.overlaylight veil is now--ds-scrim; addeddoesNotMatchfor the removed override.markdown-prose-style.test.mjs— light ink mixes now assertvar(--ds-text-primary).plugins-page-style.test.mjs—.plugins-modal-backdropis--ds-modal-veil, asserted to be defined exactly twice.mermaid-rendering.test.mjs—.mermaid-block-bodyis--ds-mermaid-canvas, same two-definition assertion.Remaining gap
The guard cannot catch a cascade problem (a selector out-specifying a token, which is how #339 arose) — that still needs review, and the convention is now written down in the design-system doc so reviewers have a rule to apply.
Fixes #341