fix(design-system): narrow the theme.ts raw-color exemption to APP_THEME_COLORS - #1070
Conversation
…EME_COLORS The exemption added in #1059 used scope "whole-file", so the entire file was blanked before counting and any unrelated raw colour added to theme.ts later would have escaped the contract — in a file whose baseline had just been ratcheted down to 2. An automated reviewer raised this as a P2 on that PR and was right; the fix never landed, so #1059 merged with the hole open. Only the two pre-paint literals are un-tokenisable (read by the inline pre-hydration script and the theme-color meta tag before any CSS, and therefore any token, exists). Scope the exemption to the APP_THEME_COLORS declaration using the bounded-range pattern the printable-paper scopes already use, anchored on "export const APP_THEME_COLORS" so the later APP_THEME_COLORS.dark/.light references inside THEME_BOOTSTRAP_SCRIPT can never be mistaken for the boundary. Fail-closed: a renamed or removed declaration reports a failure (wired to assert(false, ...) -> exit 1) and returns the source UNMASKED, so the two literals are counted and the ratcheted baseline goes red loudly rather than the file being silently exempted. Verified by injecting `export const SNEAKY_ACCENT = "#0f766e";` into the real src/lib/theme.ts: with "whole-file" the gate passes reporting "raw colors 2" and the colour is invisible; with this change it fails with "rawColorLiterals increased from 2 to 3". Clean-tree counts are unchanged (2/1/0), confirming the narrowing masks exactly the two exempted literals. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe raw-color contract utility now exempts only the ChangesTheme color masking
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/design-system-contract-utils.mjs`:
- Around line 185-194: Update the app-theme-colors handling in
balancedBlockRange to locate APP_THEME_COLORS as an exact declaration token,
excluding comments, strings, and suffixed names such as APP_THEME_COLORS_V2, so
renamed declarations trigger reportFailure. Add a regression case covering the
suffixed declaration.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e742ea2-8c62-4abd-9808-d4e60c578d8b
📒 Files selected for processing (2)
scripts/design-system-contract-utils.mjstests/design-system-contract-utils.test.ts
…ect the docs Adversarial review of the previous commit found that the anchoring property its own code comment claims — that the marker is the declaration keyword so a later *reference* cannot be mistaken for the boundary — was caught by none of the new assertions. Mutating the marker to a bare "APP_THEME_COLORS" left the whole file green, and the property is load-bearing: with a bare marker, a doc comment naming the constant above an unrelated raw colour anchors the mask on that mention and silently swallows the colour with no failure reported (masking runs before comments are stripped, so comment-stripping cannot rescue it). Add that exact fixture as a regression test; it goes red under the bare-marker mutation and green as shipped. Also correct docs/design-system.md, which enumerated 6 of the 8 sanctioned raw-colour categories (omitting the globals.css token definitions and the pre-paint theme colours) and credited check-design-system-contract.mjs with owning the path allowlist, which actually lives in RAW_COLOR_EXEMPTIONS in design-system-contract-utils.mjs. Records the bounded-scope-over-whole-file preference this change embodies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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: 75a838f21a
ℹ️ 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".
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ea91aa7b0
ℹ️ 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".
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
Summary
Narrows the
src/lib/theme.tsraw-colour exemption fromwhole-fileto theAPP_THEME_COLORSdeclaration, closing a hole that shipped in #1059.An automated reviewer raised this as a P2 on #1059 and it is correct: the exemption I added there blanked the entire file before counting, so any unrelated raw colour added to
theme.tslater would have escaped the ratcheting contract — in a file whose baseline had just been ratcheted down to 2. Only the two pre-paint literals are genuinely un-tokenisable (they are read by the inline pre-hydration script and thetheme-colormeta tag before any CSS, and therefore any token, exists).This follows the exemption pattern the repo already uses for the two printable-paper scopes: a bounded range plus fail-closed behaviour.
scope: "app-theme-colors"masks only theAPP_THEME_COLORSblock via the existingbalancedBlockRangehelper.export const APP_THEME_COLORS, not the bare identifier, so the laterAPP_THEME_COLORS.dark/.lightreferences insideTHEME_BOOTSTRAP_SCRIPTcan never be mistaken for the boundary.reportFailurefires (wired toassert(false, …)→process.exit(1)) and the source is returned unmasked, so the two literals get counted and the ratcheted baseline goes red loudly rather than the file being silently exempted.Verification
Proof the hole was real and is now closed — appending
export const SNEAKY_ACCENT = "#0f766e";to the realsrc/lib/theme.ts:whole-file(as merged in #1059)raw colors 2, the new colour is invisibleapp-theme-colors(this PR)rawColorLiterals increased from 2 to 3npm run check:design-system-contracton a clean tree — passes with identical counts (raw colors 2; literal shadows 1; legacy tap classes 0), confirming the narrowing masks exactly the two exempted literals and nothing else was being hiddentests/design-system-contract-utils.test.ts— 5/5 pass, including two new cases: an unrelated#0f766eelsewhere in atheme.tsfixture stays countable while#ffffff/#060708are masked, and a renamed constant triggers the fail-closed pathnpm run typecheck,eslint,prettier --check .Risk and rollout
whole-fileand the gate keeps passing.Clinical Governance Preflight
(Build-time design-system guardrail only; no runtime, clinical, or data path is touched.)
Clinical KB Database(sjrfecxgysukkwxsowpy)RAG impact: no retrieval behaviour change — design-system contract script and its unit test only; no retrieval, ranking, or answer-generation surface is touched.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
app-theme-colors.APP_THEME_COLORSdeclaration, without swallowing unrelated raw colors.Tests
Documentation