Repository navigation
馃帹 fix: Draw Library Colours From Theme Roles and Record the Exceptions #16482
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { expect, test } from '@playwright/test'; | ||
|
|
||
| /** | ||
| * The SAML sign-in button draws a generic key glyph. It used to be filled black, which vanished | ||
| * on the dark login surface; it now takes the button's own text colour, so it reads in every | ||
| * mode and theme. SAML is switched on by rewriting `/api/config` the way librechat.yaml would. | ||
| */ | ||
| test.use({ storageState: { cookies: [], origins: [] } }); | ||
|
|
||
| test('the SAML sign-in glyph is painted with the button text colour @scenario:saml-login-glyph-follows-button-text', async ({ | ||
| page, | ||
| }) => { | ||
| await page.route( | ||
| (url) => url.pathname === '/api/config', | ||
| async (route) => { | ||
| const response = await route.fetch(); | ||
| const body = await response.json(); | ||
| await route.fulfill({ | ||
| response, | ||
| json: { | ||
| ...body, | ||
| socialLoginEnabled: true, | ||
| samlLoginEnabled: true, | ||
| samlImageUrl: '', | ||
| samlLabel: 'SAML', | ||
| socialLogins: ['saml'], | ||
| }, | ||
| }); | ||
| }, | ||
| ); | ||
|
|
||
| await page.goto('/login', { timeout: 15_000 }); | ||
| const button = page.getByRole('link', { name: 'SAML' }); | ||
| await expect(button).toBeVisible({ timeout: 15_000 }); | ||
|
|
||
| const paint = await button.evaluate((link) => { | ||
| const glyph = link.querySelector('svg g'); | ||
| return { | ||
| text: getComputedStyle(link).color, | ||
| glyph: glyph ? getComputedStyle(glyph).fill : null, | ||
| }; | ||
| }); | ||
| expect(paint.glyph).toBe(paint.text); | ||
| }); |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| import { render, waitFor } from '@testing-library/react'; | ||
| import PixelCard from './PixelCard'; | ||
|
|
||
| /** The canvas draws with whatever the palette resolved to, so the test reads what the card | ||
| * asked the page for: which theme variables, and whether it asks again after a theme change. */ | ||
| describe('PixelCard palette', () => { | ||
| let fillStyles: string[]; | ||
| let channels: Record<string, string>; | ||
|
|
||
| beforeEach(() => { | ||
| fillStyles = []; | ||
| jest.spyOn(HTMLCanvasElement.prototype, 'getContext').mockImplementation(function () { | ||
| return { | ||
| clearRect: jest.fn(), | ||
| fillRect: jest.fn(), | ||
| set fillStyle(value: string) { | ||
| fillStyles.push(value); | ||
| }, | ||
| } as unknown as CanvasRenderingContext2D; | ||
| }); | ||
| jest.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockReturnValue({ | ||
| width: 10, | ||
| height: 10, | ||
| } as DOMRect); | ||
| channels = { | ||
| '--surface-primary-alt': '1 2 3', | ||
| '--surface-tertiary': '1 2 3', | ||
| '--border-medium': '1 2 3', | ||
| }; | ||
| const computed = window.getComputedStyle.bind(window); | ||
| jest.spyOn(window, 'getComputedStyle').mockImplementation((element) => { | ||
| const style = computed(element); | ||
| return { | ||
| ...style, | ||
| getPropertyValue: (name: string) => | ||
| name.startsWith('--') ? (channels[name] ?? '') : style.getPropertyValue(name), | ||
| } as CSSStyleDeclaration; | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.restoreAllMocks(); | ||
| document.documentElement.className = ''; | ||
| }); | ||
|
|
||
| it('draws the default variant from theme variables', async () => { | ||
| render(<PixelCard progress={1} />); | ||
|
|
||
| await waitFor(() => expect(fillStyles).toContain('rgb(1 2 3)')); | ||
| expect(fillStyles.every((style) => style === 'rgb(1 2 3)')).toBe(true); | ||
| }); | ||
|
|
||
| it('draws each default slot from its own theme variable, and currentColor when one is unset', async () => { | ||
| channels = { '--surface-primary-alt': '1 1 1', '--surface-tertiary': '2 2 2' }; | ||
| jest | ||
| .spyOn(HTMLElement.prototype, 'getBoundingClientRect') | ||
| .mockReturnValue({ width: 200, height: 200 } as DOMRect); | ||
| render(<PixelCard progress={1} />); | ||
|
|
||
| await waitFor(() => expect(fillStyles.length).toBeGreaterThan(0)); | ||
| expect(new Set(fillStyles)).toEqual(new Set(['rgb(1 1 1)', 'rgb(2 2 2)', 'currentColor'])); | ||
| }); | ||
|
|
||
| it('keeps an explicit colors prop as given', async () => { | ||
| render(<PixelCard progress={1} colors="#123456" />); | ||
|
|
||
| await waitFor(() => expect(fillStyles).toContain('#123456')); | ||
| expect(fillStyles.every((style) => style === '#123456')).toBe(true); | ||
| }); | ||
|
|
||
| it('re-reads the palette when the theme changes', async () => { | ||
| render(<PixelCard progress={1} />); | ||
| await waitFor(() => expect(fillStyles).toContain('rgb(1 2 3)')); | ||
|
|
||
| fillStyles = []; | ||
| channels = { | ||
| '--surface-primary-alt': '9 9 9', | ||
| '--surface-tertiary': '9 9 9', | ||
| '--border-medium': '9 9 9', | ||
| }; | ||
| document.documentElement.classList.add('dark'); | ||
|
|
||
| await waitFor(() => expect(fillStyles).toContain('rgb(9 9 9)')); | ||
| }); | ||
|
|
||
| it('keeps its pixels when an unrelated root variable changes', async () => { | ||
| render(<PixelCard progress={1} />); | ||
| await waitFor(() => expect(fillStyles).toContain('rgb(1 2 3)')); | ||
| const measure = HTMLElement.prototype.getBoundingClientRect as jest.Mock; | ||
| const layouts = measure.mock.calls.length; | ||
|
|
||
| document.documentElement.style.setProperty('--message-scrollbar-gutter', '8px'); | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
|
|
||
| expect(measure.mock.calls.length).toBe(layouts); | ||
| document.documentElement.style.removeProperty('--message-scrollbar-gutter'); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -139,13 +139,36 @@ const getEffectiveSpeed = (value: number, reducedMotion: boolean) => { | |
|
|
||
| const clamp = (n: number, min = 0, max = 1) => Math.min(Math.max(n, min), max); | ||
|
|
||
| /** The default palette names theme channel variables, read off the card when the pixels are | ||
| * laid out, so the canvas draws the active theme instead of a fixed light palette. */ | ||
| const VARIANTS = { | ||
| default: { | ||
| gap: 5, | ||
| speed: 35, | ||
| colors: '--surface-primary-alt,--surface-tertiary,--border-medium', | ||
| noFocus: false, | ||
| }, | ||
| /** Decorative presets no theme role reproduces; kept as given, like a `colors` prop. */ | ||
| blue: { gap: 10, speed: 25, colors: '#e0f2fe,#7dd3fc,#0ea5e9', noFocus: false }, | ||
| yellow: { gap: 3, speed: 20, colors: '#fef08a,#fde047,#eab308', noFocus: false }, | ||
| pink: { gap: 6, speed: 80, colors: '#fecdd3,#fda4af,#e11d48', noFocus: true }, | ||
| } as const; | ||
|
|
||
| /** Resolves `--token` entries to the element's channel triplet; any other entry is a CSS color | ||
| * the caller supplied and passes through. A token the page does not define falls back to the | ||
| * canvas ink rather than drawing nothing. */ | ||
| const resolvePalette = (palette: string, element: Element): string[] => { | ||
| const style = getComputedStyle(element); | ||
| return palette.split(',').map((entry) => { | ||
| const color = entry.trim(); | ||
| if (!color.startsWith('--')) { | ||
| return color; | ||
| } | ||
| const channels = style.getPropertyValue(color).trim(); | ||
| return channels ? `rgb(${channels})` : 'currentColor'; | ||
| }); | ||
| }; | ||
|
|
||
| interface PixelCardProps { | ||
| variant?: keyof typeof VARIANTS; | ||
| gap?: number; | ||
|
|
@@ -262,7 +285,7 @@ export default function PixelCard({ | |
| canvasRef.current.width = Math.floor(cw); | ||
| canvasRef.current.height = Math.floor(ch); | ||
|
|
||
| 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, AGENTS.md reference: AGENTS.md:L90-L97 Useful? React with 馃憤聽/ 馃憥.
Collaborator
Author
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. |
||
| const px: Pixel[] = []; | ||
|
|
||
| const cx = cw / 2; | ||
|
|
@@ -318,11 +341,34 @@ export default function PixelCard({ | |
| if (containerRef.current) { | ||
| obs.observe(containerRef.current); | ||
| } | ||
| /** A mode switch or an applied theme rewrites the root's class or inline variables, which | ||
| * the resolved palette has already been read from. The root's inline style also carries | ||
| * unrelated variables (scrollbar gutter, font size), so the pixels are laid out again only | ||
| * when the palette itself resolves differently. */ | ||
| let resolved = containerRef.current | ||
| ? resolvePalette(palette, containerRef.current).join(',') | ||
| : ''; | ||
| const themeObs = new MutationObserver(() => { | ||
| if (!containerRef.current) { | ||
| return; | ||
| } | ||
| const next = resolvePalette(palette, containerRef.current).join(','); | ||
| if (next === resolved) { | ||
| return; | ||
| } | ||
| resolved = next; | ||
| initPixels(); | ||
| }); | ||
| themeObs.observe(document.documentElement, { | ||
| attributes: true, | ||
| attributeFilter: ['class', 'style'], | ||
| }); | ||
|
Comment on lines
+362
to
+365
There was a problem hiding this comment.
Observing the entire root AGENTS.md reference: AGENTS.md:L42-L45 Useful? React with 馃憤聽/ 馃憥.
Collaborator
Author
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. |
||
| return () => { | ||
| obs.disconnect(); | ||
| themeObs.disconnect(); | ||
| cancelAnimationFrame(animationRef.current!); | ||
| }; | ||
| }, [initPixels, palette]); | ||
|
|
||
| const hoverIn = () => progressRef.current === undefined && startAnim('appear'); | ||
| const hoverOut = () => progressRef.current === undefined && startAnim('disappear'); | ||
|
|
@@ -355,7 +401,7 @@ export default function PixelCard({ | |
| > | ||
| <div | ||
| className={cn( | ||
| 'border-border-light relative isolate grid place-items-center overflow-hidden rounded-lg border shadow-md transition-colors duration-200 ease-in-out select-none', | ||
| className, | ||
| )} | ||
| style={{ | ||
|
|
||
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # Colours outside the theme | ||
|
|
||
| Every colour in `client/src` and `packages/client/src` that the theme-leakage sweep reached comes from a semantic theme role, except the ones below. Each is a colour that must not follow the theme: a brand mark, artwork, media overlays, 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. | ||
|
|
||
| The sweep skipped files an open pull request was editing; those, with their counts, are listed in berry-13/LibreChat#195, and the avatar colours that still need roles in berry-13/LibreChat#194. Until those land, a file absent from this list is not evidence that it holds no literal colour. | ||
|
|
||
| A literal the design lint can see stays recorded in `eslint-suppressions.json` (inline disables | ||
| of the design rules are rejected by the static checks), so its count there is the exception, not | ||
| debt; the file-scoped allow entry for these paths belongs in `eslint.config.mjs`. Everything else | ||
| is invisible to the lint (CSS, strings passed to a canvas or an iframe), so this list is the record. | ||
|
|
||
| Adding an entry needs the same bar: if a theme author would reasonably want to recolour it, it | ||
| is a role, not an exception. | ||
|
|
||
| ## Theme definitions | ||
|
|
||
| | File | Why | | ||
| | -------------------------------------------------------------- | --------------------------------------------------------------- | | ||
| | `packages/client/src/theme/themes/*.ts`, `themes/clickui.json` | The palettes themselves: these are the values roles resolve to. | | ||
| | `packages/client/src/theme/registry.ts` | Theme resolution and fallbacks written in channel triplets. | | ||
| | `packages/client/src/theme/tokens.css` | Maps each role to `rgb(var(--role))`; no literal colour. | | ||
|
|
||
| ## Brand marks and artwork | ||
|
|
||
| | File | Why | | ||
| | ----------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `packages/client/src/svgs/GoogleIcon.tsx` | Google's multicolour mark, fixed by its brand guidelines. | | ||
| | `packages/client/src/svgs/FacebookIcon.tsx` | Facebook's mark, fixed by its brand guidelines. | | ||
| | `packages/client/src/svgs/DiscordIcon.tsx` | Discord's mark, fixed by its brand guidelines. | | ||
| | `packages/client/src/svgs/GeminiIcon.tsx` | Gemini's gradient mark, fixed by its brand guidelines. | | ||
| | `packages/client/src/svgs/PaLMIcon.tsx` | PaLM's multicolour mark, fixed by its brand guidelines. | | ||
| | `packages/client/src/svgs/BirthdayIcon.tsx` | A multicolour illustration; the palette is artwork, not a UI role. | | ||
| | `packages/client/src/components/PixelCard.tsx` | The `blue`, `yellow` and `pink` presets are decorative palettes a caller opts into, like the `colors` prop; the app renders only the default, which reads theme roles. | | ||
| | `packages/client/src/icons/provider/registry.ts`, `icons/provider/Avatar.tsx` | Provider brand colours, each already behind a `--provider-*` variable a deployment can override; the literal is only the fallback. | | ||
|
|
||
| ## Elevation ink in library stylesheets | ||
|
|
||
| | File | Why | | ||
| | --------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `packages/client/src/components/Dropdown.css` | Popover shadows in black alpha. The app paints `.popover-ui` from its own copy of these rules in `client/src/style.css`, so moving either copy to `--theme-shadow-lg` needs both edited together. | | ||
| | `packages/client/src/components/Tooltip.css` | Tooltip shadows are smaller than any step of the theme shadow scale; black alpha ink, dropped in high contrast where the border takes over. | |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.