Repository navigation
🧬 test: Pin Built-In Theme Output to the Cache Epoch - #16875
Conversation
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. |
![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: b3a2acbb6e
ℹ️ 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".
| name: 'bare', | ||
| modes: { light: {}, dark: {} }, | ||
| }); | ||
| const builtIns = [darkTheme, defaultTheme, highContrastDarkTheme, highContrastLightTheme]; |
There was a problem hiding this comment.
When libreChatTheme changes without modifying its two raw color maps—for example, by adding a mode-specific appearance or brand override—the cached output changes but this digest remains unchanged because it hashes defaultTheme and darkTheme rather than the resolved definition. DeploymentTheme.tsx maps interface.theme: librechat to libreChatTheme and writes its buildThemeCache result, so existing entries can replay stale styling while this guard stays green; include the modes produced by buildThemeCache(..., libreChatTheme) in the pin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e8b3fd5: the pin digests the resolved buildThemeCache modes for librechat, clickhouse and an inline definition with role overrides, not the raw palettes.
| name: 'bare', | ||
| modes: { light: {}, dark: {} }, | ||
| }); | ||
| const builtIns = [darkTheme, defaultTheme, highContrastDarkTheme, highContrastLightTheme]; |
There was a problem hiding this comment.
When either high-contrast palette changes, this digest fails and the test directs the contributor to bump the cache epoch even though these palettes are never stored or replayed by the deployment-theme cache: client/index.html skips cache replay whenever contrast is active, and DeploymentTheme.tsx only caches the librechat, clickhouse, or inline deployment definition underneath it. Following the instruction unnecessarily invalidates valid cached themes for regular-mode users and causes a first-reload theme flash, so the pin should cover only definitions whose output can actually enter this cache.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e8b3fd5: the high-contrast palettes are no longer pinned, since the boot script never replays the cache under contrast. Negative control: a high-contrast value change leaves the pin green; a librechat value change fails it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8b3fd5146
ℹ️ 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".
| modes: { | ||
| light: { colors: { 'rgb-text-primary': '10 20 30' } }, | ||
| dark: { colors: { 'rgb-text-primary': '230 220 210' } }, | ||
| }, |
There was a problem hiding this comment.
Although the built-in coverage was corrected, this inline fixture overrides only rgb-text-primary, so it exercises a few text fallbacks but misses most conditional paths in resolveTheme. For example, changing the rgb-surface-code fallback derived from rgb-surface-primary-alt would alter the cached output of a real inline theme using that source role, yet neither this fixture nor the complete bundled palettes would change the digest; the cache epoch could therefore remain unchanged and stale styling would still be replayed at boot. Include representative source-role overrides for every cache-relevant fallback (and appearance/brand override behavior) in the pinned definition.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 25c64e1: fixtures are generated, one definition per color, brand and appearance role overriding only that role, plus librechat and clickhouse, so new roles and every fallback path join by construction.
| expect({ | ||
| epoch: THEME_CACHE_EPOCH, | ||
| digest: digest(JSON.stringify(resolved)), | ||
| }).toEqual({ | ||
| epoch: 1, | ||
| digest: 'f37285', |
There was a problem hiding this comment.
When a color, brand, or appearance role is added, the resolved modes gain another emitted property and this digest changes, so this assertion fails and the preceding instruction tells the contributor to bump THEME_CACHE_EPOCH. However, themeRoleFingerprint() already incorporates the complete role set into every cache entry's version, so that change invalidates old entries automatically; the epoch is specifically documented for output changes that do not change the roles. Track the role fingerprint with this baseline so a role-set change can refresh the digest without consuming an unnecessary epoch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 25c64e1: the pin stores the role hash beside the digest. A role-set change asks for a refresh without an epoch bump; an output change under the same roles demands the bump. Checked both ways (a resolveTheme fallback change, an added color role).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25c64e184b
ℹ️ 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".
| return definitions.map((theme) => ({ | ||
| light: resolveTheme(theme, 'light'), | ||
| dark: resolveTheme(theme, 'dark'), | ||
| })); |
There was a problem hiding this comment.
Fresh evidence after the earlier fix: the generated-fixture rewrite now hashes resolveTheme(...) directly, whereas buildThemeCache persists describeResolvedTheme(resolveTheme(...)) in themeCache.ts. A change confined to serialization—such as a CSS property mapping, emitted attribute, or appearance canonicalization—therefore changes what the boot script stores and replays without changing this digest, allowing stale entries to survive under the same epoch. Build and hash each fixture's actual cached modes instead.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f98aa07: the pin hashes the persisted buildThemeCache modes, stores the full themeRoleFingerprint(), sorts fixtures by key, and generates each role fixture theme-wide, light only and dark only.
| const brands = themeBrandTokens.map((token) => ({ | ||
| ...base, | ||
| name: token, | ||
| brands: { [token]: '#123456' }, | ||
| modes: { light: {}, dark: {} }, |
There was a problem hiding this comment.
Fresh evidence beyond the prior fixture fix: every generated brand definition sets only the theme-wide brands field, while valid inline themes may override brands through modes.light.brands and modes.dark.brands. Neither bundled cacheable theme exercises that mode-specific path, so changing or dropping the final definition?.brands merge in resolveTheme would alter cached inline themes while leaving this digest green; include differing theme-wide and per-mode values so the precedence is pinned.
AGENTS.md reference: AGENTS.md:L147-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f98aa07: the pin hashes the persisted buildThemeCache modes, stores the full themeRoleFingerprint(), sorts fixtures by key, and generates each role fixture theme-wide, light only and dark only.
|
|
||
| describe('resolver output pin', () => { | ||
| const current = () => ({ | ||
| roles: themeRoleFingerprint().split('.').pop() as string, |
There was a problem hiding this comment.
When THEME_VERSION is incremented without changing the role names, the cache is already retired because themeRoleFingerprint() includes that version, but this extraction retains only the final role-hash segment. After updating the fixtures from their hard-coded version, the resolved digest will change while roles remains equal, so pinStatus incorrectly instructs the contributor to bump THEME_CACHE_EPOCH as well. Store and compare the schema-version segment separately, treating its change like a role-set change rather than consuming an unnecessary epoch.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f98aa07: the pin hashes the persisted buildThemeCache modes, stores the full themeRoleFingerprint(), sorts fixtures by key, and generates each role fixture theme-wide, light only and dark only.
| if (missing.length > 0) { | ||
| throw new Error(`Add valid samples to APPEARANCE_CANDIDATES for: ${missing.join(', ')}`); | ||
| } | ||
| return [...colors, ...brands, ...appearance] as ThemeDefinition[]; |
There was a problem hiding this comment.
When any token registry is reordered without adding or removing roles, themeRoleFingerprint() deliberately remains unchanged because it sorts the role set, but these fixture arrays retain registry order and the digest hashes them positionally. The test therefore classifies a semantically irrelevant reorder as an output change and instructs the contributor to bump the epoch, unnecessarily retiring valid stored themes. Sort the generated fixtures by a stable key before hashing so only their resolved contents affect the pin.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f98aa07: the pin hashes the persisted buildThemeCache modes, stores the full themeRoleFingerprint(), sorts fixtures by key, and generates each role fixture theme-wide, light only and dark only.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f98aa07404
ℹ️ 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".
|
|
||
| /** For every role, three definitions overriding only that role: theme-wide, light and dark. */ | ||
| function roleFixtures(): Fixture[] { | ||
| const base = { version: 1 as const }; |
There was a problem hiding this comment.
When THEME_VERSION is incremented, these fixtures remain at version 1, so validateThemeDefinition rejects every appearance fixture and roleFixtures() throws before pinStatus() can recognize the changed fingerprint. This defeats the newly added schema-version handling and produces the misleading Add valid samples error instead of instructing the contributor to refresh the pin; derive this value from the exported current theme version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#298 (test-only pin hardening).
There was a problem hiding this comment.
Fixed in 00cd135: the fixtures are now generated from the registry and the resolver, with a negative control for this category.
| key: `brand:${token}:${scope}`, | ||
| theme: { | ||
| ...base, | ||
| name: token, | ||
| modes: scoped(scope, () => ({ brands: { [token]: '#123456' } })), |
There was a problem hiding this comment.
When a valid inline theme defines the same brand both theme-wide and in a mode, resolveTheme must let the mode-specific value win. Fresh evidence in this head is that the generated mode fixtures still contain no theme-wide brand, while the separate theme-wide fixture contains no mode brand, and all use the same value; therefore reversing the two spreads in resolveTheme would change persisted output for real themes while this digest stayed green. Add a fixture with conflicting theme-wide and per-mode values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#298 (test-only pin hardening).
There was a problem hiding this comment.
Fixed in 00cd135: the fixtures are now generated from the registry and the resolver, with a negative control for this category.
| const colors = themeColorTokens.flatMap((token) => | ||
| SCOPES.map((scope) => ({ | ||
| key: `color:${token}:${scope}`, | ||
| theme: { | ||
| ...base, | ||
| name: token, | ||
| modes: scoped(scope, (mode) => ({ | ||
| colors: { [token]: mode === 'light' ? '1 2 3' : '4 5 6' }, | ||
| })), | ||
| } as ThemeDefinition, |
There was a problem hiding this comment.
Fresh evidence after the one-role fixture rewrite is that no generated color definition can exercise precedence between multiple fallback sources. For example, a dark inline theme with both rgb-link and rgb-text-primary but no rgb-link-prose must prefer rgb-link; reversing that priority in resolveTheme leaves every one-role fixture unchanged, while both bundled themes explicitly define the destination and therefore never exercise the fallback. The digest would stay green although persisted output for that valid inline theme changed, so include combination fixtures for fallback branches with multiple inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#298 (test-only pin hardening).
There was a problem hiding this comment.
Fixed in 00cd135: the fixtures are now generated from the registry and the resolver, with a negative control for this category.
| { key: 'a:clickhouse', theme: clickHouseTheme }, | ||
| ...roleFixtures(), | ||
| ].sort((a, b) => a.key.localeCompare(b.key)); | ||
| return fixtures.map(({ key, theme }) => [key, buildThemeCache(OWNER, key, theme).modes]); |
There was a problem hiding this comment.
Fresh evidence after the fixture-order fix is that only the outer fixture list is sorted; each cached mode still contains the positional properties array emitted in token-registry order. Reordering themeColorTokens or the appearance-property registry without changing any roles or values leaves themeRoleFingerprint() unchanged and produces identical CSS after replay, but changes this digest and incorrectly demands an epoch bump. Sort each mode's property tuples by property name for the pin so semantically irrelevant registry reordering only requires refreshing no cache version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#298 (test-only pin hardening).
There was a problem hiding this comment.
Fixed in 00cd135: the fixtures are now generated from the registry and the resolver, with a negative control for this category.
| const scoped = (scope: Scope, block: (mode: 'light' | 'dark') => object) => ({ | ||
| light: scope === 'dark' ? {} : block('light'), | ||
| dark: scope === 'light' ? {} : block('dark'), | ||
| }); |
There was a problem hiding this comment.
Every fixture produced by scoped() explicitly includes both light and dark, using {} for the inactive mode, and both bundled themes also define both modes. A valid inline theme may instead use modes: {} or omit one mode, which makes resolveTheme receive undefined rather than an empty definition; a future change confined to that branch would alter its cached output without moving this digest. Include an empty-modes fixture so the cache pin covers the valid missing-definition path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#298 (test-only pin hardening).
There was a problem hiding this comment.
Fixed in 00cd135: the fixtures are now generated from the registry and the resolver, with a negative control for this category.
| name: token, | ||
| modes: scoped(scope, () => ({ appearance: { [token]: value } })), | ||
| }) as ThemeDefinition, | ||
| ).find((theme) => validateThemeDefinition(theme).length === 0); |
There was a problem hiding this comment.
For every shadow appearance role, the first valid candidate is a concrete shadow value (or, for elevationSurface, an earlier nonempty value), so no generated fixture supplies the also-valid value none. The bundled themes likewise never set the composable shadow roles to none, leaving withComposableShadows()—which converts that value to 0 0 #0000 before persistence—outside the digest. Removing or changing that conversion would break cached inline themes using none while the pin stayed green; add a special-value fixture for this canonicalization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#298 (test-only pin hardening).
There was a problem hiding this comment.
Fixed in 00cd135: the fixtures are now generated from the registry and the resolver, with a negative control for this category.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00cd135f1e
ℹ️ 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".
| const appearanceOverrides = (tokens: readonly string[]): Record<string, string> => | ||
| Object.fromEntries(tokens.map((token) => [token, SAMPLES[token][0]])); |
There was a problem hiding this comment.
Fresh evidence after the generated-fixture rewrite is that appearanceOverrides() assigns every source its own first accepted sample, so roles sharing a validator receive identical values. For example, fontFamily and displayFontFamily are both discovered as sources for dialogTitleFontFamily, but both become ui-sans-serif, sans-serif; reversing their precedence when both are supplied would therefore leave this digest unchanged even though a valid inline theme with conflicting families would persist different styling. Generate distinct valid values for appearance sources, as colorOverrides() does.
AGENTS.md reference: AGENTS.md:L150-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#299 (test-only pin hardening).
| if (accepted.length === 0) { | ||
| throw new Error(`Add a valid sample for the appearance role ${token} to APPEARANCE_CANDIDATES`); | ||
| } | ||
| return [...new Set([accepted[0], ...accepted.filter((value) => value === 'none')])]; |
There was a problem hiding this comment.
Fresh evidence beyond the shadow-none fix is that this selection retains only the first accepted value and none, so chromeBorderAlpha is exercised with 0.5 while ClickHouse supplies only the already-canonical literal 0. No pinned fixture covers valid values such as .0 or 0.00, meaning a change to canonicalAppearance() that stops converting those values to 0 would alter persisted cache output without changing this digest or requiring the cache epoch bump.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#299 (test-only pin hardening).
| Object.keys(after) | ||
| .filter((key) => key !== source && after[key] !== before[key]) | ||
| .forEach((key) => found[kind].set(key, (found[kind].get(key) ?? new Set()).add(source))); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier combined-source fix is that derivedRoles() can discover only a source whose single-role override changes the target. rgb-text-secondary does not alter rgb-series-8 by itself; the resolver reads it only when at least one of rgb-series-1 through rgb-series-7 is also customized, so it is omitted from the generated source combination. Changing that fallback to use the bundled secondary text would therefore leave this digest unchanged even though a valid inline theme defining both roles would persist a different series-eight color; add probes or a fixture for sources that become relevant only when another role activates the fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#299 (test-only pin hardening).
00cd135 to
a33f1f9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a33f1f9070
ℹ️ 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".
| if (accepted.length === 0) { | ||
| throw new Error(`Add a valid sample for the appearance role ${token} to APPEARANCE_CANDIDATES`); | ||
| } | ||
| return [...new Set([accepted[0], ...accepted.filter((value) => value === 'none')])]; |
There was a problem hiding this comment.
When an appearance validator is broadened so that an earlier APPEARANCE_CANDIDATES value becomes valid, accepted[0] silently changes the fixture input and therefore the digest while themeRoleFingerprint() remains unchanged. pinStatus() then instructs the contributor to bump THEME_CACHE_EPOCH, even though every previously accepted definition still resolves identically and its cache remains valid, causing an unnecessary cache retirement and potential boot-time theme flash. Keep each role's representative input stable, or distinguish fixture-set changes from resolver-output changes.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1f1944f: the pin now carries a separate digest of the generated fixture inputs, so a validator that starts accepting an earlier candidate reports a pin refresh, not an epoch bump.
|
|
||
| /** Property order is positional in the cache entry and means nothing to the page. */ | ||
| const canonical = ({ properties, attributes }: ResolvedThemeStyle) => ({ | ||
| properties: [...properties].sort(([a], [b]) => a.localeCompare(b)), |
There was a problem hiding this comment.
When Jest runs under a locale with different collation rules, these localeCompare sorts produce a different serialized order and therefore a different digest even though the cached theme output is unchanged. For example, under cs-CZ/sk-SK, --dialog-title sorts before --chart-widget-stroke, unlike the ordering used under en-US, so the checked-in pin fails and incorrectly requests an epoch bump. Use a locale-independent code-point comparator for both the property and fixture-key sorts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1f1944f: both sorts use a code-point comparator; the spec passes under en-US and cs_CZ.UTF-8.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f1944f458
ℹ️ 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".
| if (actual.inputs !== PIN.inputs) { | ||
| return `The generated fixture inputs changed (an appearance sample the validator now accepts), not the resolver: set PIN to ${refreshed}.`; |
There was a problem hiding this comment.
When one change alters SAMPLES—for example, an appearance validator starts accepting an earlier candidate—and also changes cached output for an existing definition such as ClickHouse, both inputs and digest differ. This branch returns first and tells the contributor to refresh PIN without bumping the epoch, so existing entries retain the same fingerprint and replay stale styling. Fresh evidence after the separate-input-digest fix is that pinStatus never evaluates the output mismatch when inputs changed; keep an output baseline independent of generated inputs or otherwise detect concurrent output changes.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cedd75a: output is now pinned as two digests, fixtures independent of SAMPLES (bundled themes, colors, brands, modes) and fixtures built from them. The independent digest is checked before the inputs, so a change that moves both still asks for the epoch bump; covered in 'tells a version change from an output change'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cedd75a0e9
ℹ️ 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".
| if (actual.inputs !== PIN.inputs) { | ||
| return `The generated fixture inputs changed (an appearance sample the validator now accepts), not the resolver: set PIN to ${refreshed}.`; |
There was a problem hiding this comment.
When a change makes an earlier appearance candidate valid and simultaneously changes resolution of a custom appearance override not exercised by the fixed fixtures, inputs and sampled both change while fixed remains unchanged. This branch returns first and tells the contributor to refresh PIN without bumping the epoch, so existing cached themes using the affected override can still replay stale output. Fresh evidence after the earlier fix is that the sampled-output mismatch remains checked only after this early return; preserve an output baseline for the previous sampled inputs or otherwise classify concurrent sampled changes as requiring an epoch bump.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c972bfb by dropping the input/output split: the digest now covers every candidate each appearance validator accepts together with the persisted output, and any digest change outside a fingerprint change asks for an epoch bump. Narrowing, widening and concurrent output changes all retire the cache; the cost is an occasional retirement when a validator only widens, which is rare and one-time.
| if (actual.inputs !== PIN.inputs) { | ||
| return `The generated fixture inputs changed (an appearance sample the validator now accepts), not the resolver: set PIN to ${refreshed}.`; |
There was a problem hiding this comment.
When an appearance validator is tightened so the first accepted sample changes while bundled-theme output remains the same, this branch classifies the change as fixture-only and permits refreshing PIN without an epoch bump. A previously valid custom theme using the removed value can still have an entry with the current fingerprint, and the boot path in client/index.html replays that entry before the app revalidates and clears it, producing a flash of styling that the new schema explicitly rejects. Distinguish validator broadening from narrowing, or retire cached entries whenever previously accepted values are removed.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c972bfb by dropping the input/output split: the digest now covers every candidate each appearance validator accepts together with the persisted output, and any digest change outside a fingerprint change asks for an epoch bump. Narrowing, widening and concurrent output changes all retire the cache; the cost is an occasional retirement when a validator only widens, which is rare and one-time.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c972bfb7e1
ℹ️ 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".
| const ACCEPTED: Record<string, string[]> = Object.fromEntries( | ||
| themeAppearanceTokens.map((token) => [ |
There was a problem hiding this comment.
When appearanceValidators is reordered without changing any roles or behavior, themeAppearanceTokens follows that insertion order and this Object.fromEntries preserves it in ACCEPTED. Although the fingerprint sorts the role set and the persisted fixtures are canonicalized, JSON.stringify({ accepted: ACCEPTED, ... }) then produces a different digest and instructs the contributor to bump the epoch, unnecessarily retiring valid stored themes; sort these entries by token before hashing.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#301: the pin is a tripwire for output changes without an epoch bump, and widening its coverage of validator semantics is tracked there.
| /** Locale-independent, so the digest does not depend on the collation Jest runs under. */ | ||
| const byCodePoint = (a: string, b: string): number => (a < b ? -1 : Number(a > b)); | ||
|
|
||
| const APPEARANCE_CANDIDATES = [ |
There was a problem hiding this comment.
Fresh evidence after the accepted-value fix is that this candidate list still omits currently valid literals such as fieldFillStyle: 'transparent' and labelFontWeight: 'inherit'. If either validator is tightened to remove one of those values, ACCEPTED and every generated output remain unchanged, so the pin stays green while an existing cache for a now-invalid custom theme can still be replayed at boot under the old fingerprint; include all finite enum literals in the pinned candidates.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#301: the pin is a tripwire for output changes without an epoch bump, and widening its coverage of validator semantics is tracked there.
| APPEARANCE_CANDIDATES.filter((value) => | ||
| isValid(define(token, () => ({ appearance: { [token]: value } }), 'both')), | ||
| ), |
There was a problem hiding this comment.
Each acceptance probe defines only one appearance token, but collectSwitchIssues validates switchWidth and switchHeight jointly. A tightening that still accepts each sampled value against its default counterpart, yet rejects a formerly valid custom pair, therefore changes neither ACCEPTED nor the persisted fixtures (the bundled ClickHouse pair can also remain valid), allowing that rejected pair's old cache to flash at boot without an epoch bump; add paired validation fixtures to the pin.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#301: the pin is a tripwire for output changes without an epoch bump, and widening its coverage of validator semantics is tracked there.
Summary
The deployment-theme cache version keys on the role set and a hand-bumped
THEME_CACHE_EPOCH, so a release that changes what a cacheable theme resolves to without adding a role leaves old entries valid and the boot script replays stale styling once. Nothing told a contributor to bump the epoch. This adds a pin test over what the cache persists:librechat,clickhouse, and fixtures generated from the registry and the resolver (every role alone, theme-wide and per mode; every role the resolver derives from others, with all sources named together and with the role named; conflicting brand values; absent mode blocks; shadownone), with property order canonicalized. A role-set change asks for a pin refresh, and an output change under the same fingerprint demands the epoch bump.berry-13#290
berry-13#297
berry-13#298
Type of change
Testing
cd client && npx jest src/Providers/__tests__/themeCache.spec.ts --maxWorkers=2: 18 passed.buttonPaddingXin the ClickHouse theme and rebuilding@librechat/clientmade the pin test fail on the digest; reverted.Risk / compatibility
None for runtime; the change is test-only. The pin collides with any open PR that changes resolved theme values, such as #16821 for ClickHouse: whichever merges second bumps
THEME_CACHE_EPOCHand updates the digest.Checklist