Revert "Merge pull request #565" (re-merge as fresh PR) - #566
Merged
Conversation
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoRevert merged #565 to re-land as a fresh PR
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
12 rules 1. Ungated color-mix hover
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
113
to
117
| .sf-btn--secondary:hover:not(:disabled, .sf-is-disabled, .sf-is-loading) { | ||
| background: color-mix(in oklab, var(--sf-btn-color) 8%, transparent); | ||
| border-color: var(--sf-btn-color); | ||
| } | ||
|
|
There was a problem hiding this comment.
1. Ungated color-mix hover 🐞 Bug ≡ Correctness
optional/components.css now uses color-mix() directly in hover backgrounds without an @supports gate, so engines without color-mix() will ignore the hover background tint for .sf-btn--secondary/.sf-btn--ghost. The updated P2/P7 tests no longer cover non-token declarations, so this regression is unlikely to be caught by CI.
Agent Prompt
### Issue description
`optional/components.css` contains `background: color-mix(...)` in normal rules (not inside `@supports`). Browsers lacking `color-mix()` support will drop the `background` hover tint, and the current P2/P7 tests won't detect this because they no longer scan ordinary declarations.
### Issue Context
This used to be gated via `@supports (background: color-mix(in oklab, red, red)) { ... }` so older engines would skip only the modern tint while retaining other hover cues.
### Fix Focus Areas
- optional/components.css[113-125]
- tests/tier1-p2-coverage.test.js[1-43]
- tests/tier1-p7-oldengine.test.js[50-56]
### Proposed fix
1. Wrap the `background: color-mix(...)` hover declarations for `.sf-btn--secondary:hover...` and `.sf-btn--ghost:hover...` in an `@supports (background: color-mix(in oklab, red, red)) { ... }` block.
2. Re-expand the gating tests to include component/utility rules again (either by reinstating the previous whole-source scan or adding a targeted scan for non-custom-property declarations in `optional/components.css`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+23
to
+48
| /** | ||
| * Simulate an engine with no @supports by stripping all @supports blocks. | ||
| * Returns only declarations that survive (ungated ones). | ||
| */ | ||
| function stripSupports(css) { | ||
| let result = ''; | ||
| let depth = 0; | ||
| let inSupports = false; | ||
| let supportsDepth = 0; | ||
| for (const line of css.split('\n')) { | ||
| const trimmed = line.trim(); | ||
| if (trimmed.startsWith('@supports')) { | ||
| inSupports = true; | ||
| supportsDepth = depth; | ||
| } | ||
| if (!inSupports) result += line + '\n'; | ||
| for (const ch of line) { | ||
| if (ch === '{') depth++; | ||
| else if (ch === '}') { | ||
| depth--; | ||
| if (inSupports && depth <= supportsDepth) inSupports = false; | ||
| } | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
2. Broken @supports stripper 🐞 Bug ☼ Reliability
The new stripSupports() in tests/tier1-p7-oldengine.test.js counts braces without understanding
@supports preludes that themselves contain braces (e.g. `@supports (@property --x { ... }) { ...
}`), so it can stop stripping too early and leak gated declarations into the “ungated” corpus. This
makes the P7 old-engine invariant check unreliable and can produce false failures or missed
regressions.
Agent Prompt
### Issue description
`tests/tier1-p7-oldengine.test.js` replaced the prior robust `@supports` removal logic with a brace counter that doesn't handle braces inside `@supports` preludes (notably the `@property` feature-query form). This can terminate `inSupports` before the actual supports-body begins.
### Issue Context
The codebase contains exactly this pattern in `core/motion.css`:
`@supports (@property --x { ... }) { ... }`
Inside that gated block there are `--sf-*` custom-property declarations using `oklch(from ...)`, so a broken stripper will incorrectly treat those as ungated.
### Fix Focus Areas
- tests/tier1-p7-oldengine.test.js[23-48]
- core/motion.css[127-139]
### Proposed fix
1. Restore the previous proven implementation (the one that was in `tests/supports-helpers.js`) or re-implement it in-place:
- strip comments first
- skip over string literals (honour backslash escapes)
- when encountering `@supports` at a token boundary, find the body-opening `{` at paren depth 0
- then walk to the matching closing `}` (nested at-rules allowed)
2. Add back unit tests for tricky cases (braces in prelude, braces/parens in strings) so regressions are caught immediately.
3. (Optional) If you keep a local implementation, document why it must be prelude/paren-aware.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
13
to
18
| "token-registry.json", | ||
| "dist/css-custom-data.json", | ||
| "configurator/src/data/api-index.generated.json", | ||
| "configurator/src/data/classes.generated.json", | ||
| "configurator/src/data/bundles.generated.json", | ||
| "configurator/src/data/token-registry.generated.json" | ||
| ] |
There was a problem hiding this comment.
3. Artifacts gate misses classes 🐞 Bug ☼ Reliability
scripts/artifacts.json no longer declares configurator/src/data/classes.generated.json as an output artifact even though the configurator imports it, so scripts/check-artifacts.js --check will not fail CI when it drifts. This can ship a configurator whose class cheatsheet is out of sync with the framework sources/docs pipeline.
Agent Prompt
### Issue description
The artifact freshness gate only checks files listed in `scripts/artifacts.json`. This PR removes `configurator/src/data/classes.generated.json` from those outputs, but the configurator still imports and relies on it.
### Issue Context
- `npm run docs` runs `npm run configurator:sync`, which generates `classes.generated.json`.
- The configurator UI imports `classes.generated.json` directly.
- `scripts/check-artifacts.js` only runs `git diff --exit-code` on declared `outputs`, so omitted generated files can drift silently.
### Fix Focus Areas
- scripts/artifacts.json[1-29]
- scripts/check-artifacts.js[51-69]
- configurator/scripts/sync-api.mjs[47-51]
- configurator/src/components/panels/CheatsheetPanel.svelte[5-13]
### Proposed fix
1. Add `configurator/src/data/classes.generated.json` back into the `outputs` array(s) for the relevant artifact entries (at least the ones that run `npm run docs` / `configurator:sync`).
2. (Recommended) Restore the check-mode guard that fails if an artifact output is not git-tracked, to prevent un-checkable outputs from being added in the future.
3. (Optional) Consider reintroducing a lightweight contract test for generated configurator data if downstream consumers are sensitive to shape changes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+124
to
125
| <p style="color:var(--sf-color-text--muted)">v0.7.0 · optimal-components bundle from jsDelivr CDN · 324 classes · 729 tokens (238 configurable)</p> | ||
| <div class="notice">Baseline render with default tokens. Use the toolbar to switch theme, toggle the ultimate override live, or replay motion. The always-on override variant is <a href="full-api-demo-with-overrides.html">full-api-demo-with-overrides.html</a>.</div> |
There was a problem hiding this comment.
4. Demo version string stale 🐞 Bug ⚙ Maintainability
The full-api demo pages now display v0.7.0 while package.json declares version 0.7.3, so the demo UI reports an incorrect framework version. This undermines the demos as a smoke-test surface and can mislead release verification.
Agent Prompt
### Issue description
Demo assets hard-code a version string that no longer matches `package.json`.
### Issue Context
`package.json` is the version source-of-truth for most repo artifacts. The demo banner should align with it (ideally generated, not hand-edited).
### Fix Focus Areas
- demos/full-api-demo.html[121-125]
- package.json[1-6]
- scripts/artifacts.json[1-29]
### Proposed fix
1. Update/regenerate the demo assets so the displayed version matches `package.json` (e.g., rerun the demo generator if available).
2. If these demos are intended to be kept in sync automatically, re-add them to `scripts/artifacts.json` so `check-artifacts` enforces freshness.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Temporarily reverts the changes from #565 out of
main.Background: #565's web merge succeeded at the git level (merge commit
40d352f) but the request errored before GitHub could update the PR status, leaving #565 stuck as "Closed" — GitHub refuses to reopen it ("commits are already merged") and won't show a "Merged" badge.This revert removes #565's changes so they can be re-introduced through a fresh, cleanly-mergeable PR that lands in history with a proper "Merged" badge. Net effect on the codebase is zero once the follow-up re-add PR is merged.
Generated by Claude Code