🎨 fix: Draw Library Colours From Theme Roles and Record the Exceptions - #16482
Conversation
PixelCard resolves its variant palettes from theme channel variables when it lays out pixels, so the image generation placeholder follows the active theme in dark mode instead of drawing a fixed light slate palette. The SAML glyph and Codey mark take the text colour. The new theme/allowlist.md records every colour that deliberately sits outside the theme (brand marks, artwork, theme definitions, library elevation ink) and why.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
berry-13
changed the title
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
![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: b9ef2ccf95
ℹ️ 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".
| blue: { | ||
| gap: 10, | ||
| speed: 25, | ||
| colors: '--status-info-subtle,--status-info-border,--status-info', |
There was a problem hiding this comment.
For any consumer rendering the exported blue, yellow, or pink variants under the stock light theme, these substitutions visibly redesign the component rather than only making it theme-aware. For example, blue changes from the sky palette #e0f2fe/#7dd3fc/#0ea5e9 to the substantially different status-info values #eff6ff/#93c5fd/#2563eb; the warning and error roles similarly replace the yellow and pink palettes. Preserve the existing colors as the default values—such as through dedicated semantic roles—while allowing other themes to override them.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bbcdb29: the blue, yellow and pink presets keep their original palettes (no role reproduces them) and are recorded in theme/allowlist.md; only the default palette, the one the app renders, reads theme roles.
| canvasRef.current.height = Math.floor(ch); | ||
|
|
||
| const cols = palette.split(','); | ||
| const cols = resolvePalette(palette, containerRef.current); |
There was a problem hiding this comment.
When a user switches light/dark mode or applies another runtime theme while an image-generation card remains mounted, ThemeProvider changes root classes and custom properties without resizing this element. Because this line resolves the variables into fixed RGB strings only during initPixels, every existing Pixel.color retains the previous theme until the card resizes or remounts, leaving an active canvas painted with stale colors. Reinitialize or re-resolve the palette when the theme variables change.
AGENTS.md reference: AGENTS.md:L90-L97
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d327847: the card lays its pixels out again when the root's class or inline variables change. PixelCard.spec.tsx 're-reads the palette when the theme changes' fails without the observer and passes with it.
The palette is read from theme variables when the pixels are laid out, so a mode switch or an applied theme left the previous theme's pixels on the canvas. The card now lays its pixels out again when the root's class or inline variables change.
The blue, yellow and pink presets have no theme role that reproduces them, so they keep their original palettes and are recorded in the allowlist; only the default palette, the one the app renders, reads theme roles.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d52628ec2
ℹ️ 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".
| Every colour in `client/src` and `packages/client/src` comes from a semantic theme role, except | ||
| the ones below. Each is a colour that must not follow the theme: a brand mark, artwork, a third | ||
| party's surface, a document that leaves the app, or the theme definitions themselves. An entry | ||
| names the file, what the literal paints and why the theme does not own it. |
There was a problem hiding this comment.
This new document claims to record every non-semantic color in both source trees, but existing exceptions are absent—for example, client/src/mobile.css still contains fixed light/dark link colors at lines 179–194, and packages/client/src/components/Avatar.tsx contains fixed RGB/RGBA values at lines 54–57. Because the document is presented as the authoritative record for literals that lint cannot see, subsequent audits will incorrectly treat these omissions as covered; either enumerate all current exceptions with reasons or explicitly scope the document as partial.
AGENTS.md reference: AGENTS.md:L92-L97
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c64ccda: the document now states it covers what the sweep reached and points at berry-13#195 (files an open PR held, with counts, including client/src/mobile.css) and berry-13#194 (the avatar colours in Avatar.tsx).
| themeObs.observe(document.documentElement, { | ||
| attributes: true, | ||
| attributeFilter: ['class', 'style'], | ||
| }); |
There was a problem hiding this comment.
Observing the entire root style attribute makes non-theme updates repaint and reset an in-progress card: for example, useScrollbarGutter writes --message-scrollbar-gutter to document.documentElement.style when the message column changes, and the font-size utility writes --markdown-font-size there as well. Either mutation calls initPixels, which clears the canvas by assigning its dimensions, replaces every pixel and restarts the delayed progress animation, producing a blink or replay while an image is generating even though none of the three palette channels changed. Compare the resolved palette before reinitializing or subscribe to a theme-specific signal instead.
AGENTS.md reference: AGENTS.md:L42-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c64ccda: the root observer now compares the resolved palette and lays the pixels out again only when it changed, so scrollbar-gutter and font-size writes no longer blink the card. PixelCard.spec.tsx 'keeps its pixels when an unrelated root variable changes' fails without the comparison.
The root's inline style also carries unrelated variables such as the scrollbar gutter and markdown font size, and relaying the pixels on every write blinked and replayed an in-progress card. The allowlist now states what it covers and where the skipped files are tracked.
1 participant
Summary
Part of the theme-leakage stack (merge from the bottom up). This link covers the component library in
packages/client/src. The image generation placeholder (PixelCard) drew a fixed light slate palette onto its canvas, so in dark mode, and in any theme, it showed pale pixels over a dark surface. Its default palette now names theme channel variables (--surface-primary-alt,--surface-tertiary,--border-medium), resolved off the card when the pixels are laid out and again when the root's class or inline variables change, so a mode or theme switch repaints a mounted card. The opt-in blue, yellow and pink presets keep their palettes (no role reproduces them; recorded in the allowlist), and a caller's explicitcolorsstill passes through. The SAML login glyph painted black on the dark login button and now takes the button's text colour; the unused Codey mark takesfill-text-primaryinstead ofdark:fill-white.It also adds
packages/client/src/theme/allowlist.md, the record of every colour that deliberately stays outside the theme (brand marks, artwork, theme definitions, library elevation ink, and, in the later links, media scrims and exported documents), one reason per entry. Brand marks stay recorded ineslint-suppressions.json: the static checks reject inline design-rule disables, and the config-level allow entry belongs ineslint.config.mjs, which #16248 currently edits.Baseline at
origin/canary1d5062c (TS/TSX outside specs, and CSS underclient/srcandpackages/client/src), before to after this link: raw palette utilities 57 to 56,dark:colour utilities 33 to 32, hex/rgb/hsl literals in TS/TSX 796 to 793, raw CSS literals 89 (unchanged), palette variables in CSS 150 (unchanged, all inclient/src/style.css). Suppressions,client/src/packages/client/src: no-raw-colors 47/47 to 47/46, no-unknown-classes 23/7, no-restyle 2054/0, no-inline-styles 211/39, no-arbitrary-values 162/0, require-static-classes 67/0 (unchanged). Only the SamlIcon entry changes ineslint-suppressions.json.Type of change
Testing
Tested environments/configuration:
interface.themeon/api/config, the way the e2e specs do), light and dark.fill-text-primaryresolves to 33,33,33 (default light), 236,236,236 (default dark), 22,21,23 and 255,255,255 (ClickHouse light and dark).Automated tests:
PixelCard.spec.tsx(new): the default palette draws theme variables, an explicitcolorsprop draws as given, and a theme change re-reads the palette (this case fails without the root observer).reviewctl precheckagainstorigin/canary: static checks andjest-packages-client(5 suites, 67 tests) pass.@scenario:saml-login-glyph-follows-button-text(new,e2e/specs/mock/scenarios/saml-glyph.spec.ts): the SAML glyph's computed fill equals its button's text colour.npx tsc --noEmit(incremental) forpackages/client: clean.npm run static-checks -- --against origin/canary: all affected checks pass, including design-rule suppressions.Screenshots / recordings
No capture of the image generation placeholder: it only renders mid-generation of an OpenAI image tool call, which this environment has no provider for. The palette change is verified by computed style above.
Risk / compatibility
PixelCard's exportedvariantnames are unchanged; a consumer that loads the component without the app stylesheet (no theme variables) getscurrentColorpixels instead of nothing.Checklist