refactor(design-system): burn down raw-color and legacy-tap contract debt (DSD-02/03) - #1059
Conversation
…debt (DSD-02/03) Lower two ratcheting design-system-contract baselines with zero visual change. Legacy tap classes 28 → 0: replace the `-11` (44px) magic-number tap tokens with the semantic `-tap` tokens (`h-tap`/`w-tap`/`min-h-tap`) across 14 files. These are value-identical — `--spacing-tap` is 2.75rem (44px), the exact size the DS tokens were created to replace — so computed sizes are unchanged. Raw color literals 9 → 2: - Move the settings sign-in provider marks (Microsoft's four-colour grid, Apple/Google lettered tiles) out of settings-dialog into the already brand-exempt provider-brand-icons.tsx as `ProviderBrandMark`. Byte-identical rendering; the fixed brand colours now live with the other brand artwork. - Exempt src/lib/theme.ts: its two colours are pre-paint / meta theme-color values consumed before any CSS (and therefore any token) exists. The two remaining literals are the medication default-accent fallback and are left as-is (data layer). Verified: check:design-system-contract green at the new floor (raw colors 2, literal shadows 1, legacy tap classes 0); typecheck, lint, format:check clean; full unit + jsdom suite 352 files / 3141 passed / 0 failed. 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 (18)
📝 WalkthroughWalkthroughThis PR updates design-system contract counts and exemptions, introduces shared SSO provider branding, and standardizes numerous UI icon, placeholder, chip, and notification dimensions on tap-target sizing utilities. ChangesDesign-system and UI alignment
Estimated code review effort: 2 (Simple) | ~15 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 |
![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: ecdc6c7af2
ℹ️ 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".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecdc6c7af2
ℹ️ 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".
…EME_COLORS (#1070) * fix(design-system): narrow the theme.ts raw-color exemption to APP_THEME_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> * test(design-system): pin the theme-color boundary anchoring, and correct 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> * fix: apply CodeRabbit auto-fixes Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
Summary
Burns down two ratcheting
check:design-system-contractbaselines (findings DSD-02 / DSD-03) with zero visual change.Legacy tap classes 28 → 0. Replace the
-11(44px) magic-number tap utilities with the semantic-taptokens (h-tap/w-tap/min-h-tap) across 14 files. These are value-identical:--spacing-tapis2.75rem= 44px — the exact size the DS tokens were created to replace ("this is a touch target", not "44px") — so every computed size is unchanged. Adjacent non-tap utilities (w-20,h-10,sm:h-12) and variant prefixes (sm:) are preserved.Raw color literals 9 → 2.
settings-dialog.tsxinto the already brand-exemptprovider-brand-icons.tsxasProviderBrandMark. Byte-identical rendering (the exact markup moved) — the fixed brand colours now sit with the other OAuth brand artwork where they belong.src/lib/theme.ts: its two colours are pre-paint /<meta name="theme-color">values consumed by the inline pre-hydration script before any CSS (and therefore any token) exists, so they cannot be tokenised.#0f766e) in the data layer, left as-is.The baseline JSON is lowered to
{ rawColorLiterals: 2, literalShadowClasses: 1, legacyTapClasses: 0 }.Verification
npm run check:design-system-contract— green at the new floor (raw colors 2, literal shadows 1, legacy tap classes 0)npm run typecheck,npm run lint,npm run format:checkcleannpm run test— full unit + jsdom suite: 352 files / 3141 passed / 0 failedh-11→h-tapetc. are value-identical (44px→44px) and the provider-mark move is byte-identical markup; the full Chromiumverify:uilane runs in CI.Risk and rollout
Clinical Governance Preflight
(Touched
auth-panel.tsx— a value-identicalh-11→h-tapicon-tile rename, no behaviour change.)Clinical KB Database(sjrfecxgysukkwxsowpy)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes