fix: emit override CSS unlayered when flat bundle is served - #221
Merged
jackgranatowski merged 5 commits intoJul 28, 2026
Conversation
The generated override CSS was always wrapped in @layer slashed.overrides. That is correct against the layered bundles, where the framework declares every token inside @layer slashed.tokens and reserves slashed.overrides as the last layer — but the flat bundles are the same rules with every @layer stripped, and an unlayered declaration beats any layered one regardless of specificity or source order. With the flat variant enabled, the whole override block was therefore silently inert: every configurator control — colours, gap/gutter, the spacing and typography modular scales, every scale knob — saved fine and changed nothing on the page, while the SPA's own live preview (which injects unlayered :root CSS) kept showing the change. Wrap the block only when the served bundle has layers, and route the same decision through Slashed_CSS_Loader so the Bricks and Gutenberg dark-mode bridges — layered inline CSS with the same defect — follow the bundle too. Verified in a headless browser against the committed dist bundles: before this change all 18 probed control groups came out DEAD on the flat bundle and OK on the layered one; after it, both modes are OK. That measurement is now a committed tool, tests/override-effect-probe.mjs, which asks the real PHP emitter for the CSS a site would serve and diffs every live --sf-* token against the un-overridden page. It carries a must-be-DEAD control case so a noisy measurement can't make the run pass vacuously. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DftXg4tjRkey2c3ybxAWjN
The override-effect probe proves whether the emitted CSS can move the page at all. When it reports every control group OK and a control still does nothing on a real site, the conflict is in that page's CSS environment — which no amount of reading plugin code will find. Add scripts/diagnose-page-tokens.js, a console snippet that lists every stylesheet, every rule declaring a watched token together with the cascade layer it sits in, and the computed source-knob vs derived-output values at :root. An unlayered concrete --sf-space-* declaration beats every @layer, so it shadows the modular-scale knobs while the knob itself reads back correctly — which is exactly what "the control saves but nothing changes" looks like. Verified against a page with that interference injected: the snippet names the shadowing rule. docs/troubleshooting-token-overrides.md turns the snippet's output into a decision table, and records what is already measured-good so it doesn't get re-audited: the configurator writes only live token names, the vendored tree matches the pinned framework ref, and the live preview injects unlayered CSS (which is why a preview/page mismatch indicates a cascade conflict rather than a broken control). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DftXg4tjRkey2c3ybxAWjN
…e knobs The existing sections could show a source knob holding the user's value and still not answer whether that value reaches the ladder. A reported case had --sf-space-base-max at 1.9 (not the framework's 2, so an override was clearly landing at :root) while every --sf-space-* step was inconsistent with ANY setting of the knobs: no interpolation factor fits, best residual 31.7% RMS, and the implied per-step ratio wandered 1.71 → 1.54 → 1.37, which a modular scale cannot produce. The ladder was being declared by something else, so the knobs fed a formula whose result was discarded. That case is invisible to a "did the output tokens stay at their defaults?" test, because the shadowing ladder is not the default one either. Fit the framework's own formula to the measured steps instead: the interpolation factor and the rem size both cancel when steps are taken relative to --sf-space-m, so the shape of the ladder alone decides it. Models clamp()'s lower-bound collapse when the knobs put the mobile end above the desktop end, without which a healthy page reads as a 9.6% mismatch. Verified: 0.0% on healthy pages across the fluid range (320px, 1280px, 2200px), under a 62.5% rem base, and with the reported base-max override; 46.5% against an injected hand-set ladder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DftXg4tjRkey2c3ybxAWjN
…e the map Two defects the first real-world run exposed. Rules were reported as shadowing the scale regardless of whether their selector can match the root element. Bricks ships long `unset` resets for --sf-* custom properties on its own components, so a run on a Bricks site accused `#bricks-settings #tab-custom-code tr.code-note` (from admin.min.css, a selector that cannot match :root) of killing the spacing scale. Test each rule with documentElement.matches() and use that for the verdict; the rows stay in the listing as context. The verdict also pointed at "a rule outside @layer slashed.tokens", implying a third-party stylesheet — but the confirmed cause was a stale per-step ladder stored in the plugin's own override map, emitted into @layer slashed.overrides at :root. Report that case separately and explain it: an explicit per-step value beats the knob that would have generated it by design, so base and ratio read back correctly and do nothing. Points at `wp option get slashed_overrides` and says which keys to drop. Verified against a faithful reproduction of the reported page — the stored clamps plus the Bricks unset noise — which reproduces the measured ladder exactly (4.828 / 11.808 / 20.14 / 30.9 / 107.6px at 872px viewport): the override-map warning fires and the Bricks false positive is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DftXg4tjRkey2c3ybxAWjN
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reached
Next review available in: 43 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 selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds bundle-aware cascade-layer handling for generated overrides and inline theme bridges, tests layered and flat output, and introduces Playwright, PHP, and DevTools diagnostics for verifying rendered token effects. ChangesCascade-Layer Override Handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Configurator
participant PHP_Harness
participant CSS_Bundle
participant Browser
Configurator->>PHP_Harness: Submit token overrides
PHP_Harness->>CSS_Bundle: Generate layered or flat override CSS
PHP_Harness-->>Configurator: Return override CSS
Configurator->>Browser: Inject override CSS
Browser-->>Configurator: Report computed token changes
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/override-effect-probe.mjs (1)
115-115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBrowser/server aren't closed on the early-exit and error paths.
browser(line 115) is launched before the per-bundle loop; the missing-bundle check (lines 140-143) callsprocess.exit(1)without closing it or the HTTP server, and there's notry/finallyaround the loop to guarantee cleanup ifpage.evaluate/measurethrows. For a script that spawns a real Chromium process this can leave it orphaned.Also applies to: 138-186
🤖 Prompt for 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. In `@tests/override-effect-probe.mjs` at line 115, Ensure the browser launched in the main probe flow and the HTTP server are always closed. Wrap the per-bundle processing loop, including page.evaluate and measure calls, in try/finally cleanup, and replace the missing-bundle process.exit path with cleanup before exiting; preserve the existing success behavior while preventing orphaned resources on errors.
🤖 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 `@SLASHED-for-WP/includes/class-css-loader.php`:
- Around line 87-89: Update layers_enabled() and the CSS bundle URL resolution
flow so layer capability is derived from the same resolved bundle URL, including
slashed/css_bundle_url and integration-level URL override paths, rather than
only from get_css_flat(). Carry the resolved layer capability alongside the URL
and ensure mismatched flat/layered overrides emit the corresponding CSS
behavior.
In `@tests/override-effect-probe.mjs`:
- Around line 101-113: Update the probe server created by createServer to pass
the loopback host explicitly when calling listen, while preserving the existing
ephemeral-port behavior and local consumer URLs.
In `@tests/php-harness/emit-override-css.php`:
- Around line 26-48: Configure the PHP harness before loading or executing
Slashed_Token_Store and Slashed_CSS_Generator so display_errors is disabled or
redirected to stderr, keeping stdout exclusively for the json_encode output
consumed by the probe. Ensure notices, warnings, and deprecations cannot corrupt
the JSON stream while preserving their visibility for diagnosing PHP issues.
---
Nitpick comments:
In `@tests/override-effect-probe.mjs`:
- Line 115: Ensure the browser launched in the main probe flow and the HTTP
server are always closed. Wrap the per-bundle processing loop, including
page.evaluate and measure calls, in try/finally cleanup, and replace the
missing-bundle process.exit path with cleanup before exiting; preserve the
existing success behavior while preventing orphaned resources on errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 30afa462-0fc0-4b56-95c5-c5e9dd58bc5c
📒 Files selected for processing (10)
CLAUDE.mdSLASHED-for-WP/includes/class-css-generator.phpSLASHED-for-WP/includes/class-css-loader.phpSLASHED-for-WP/integrations/bricks/includes/class-enqueue.phpSLASHED-for-WP/integrations/gutenberg/includes/class-enqueue.phpdocs/troubleshooting-token-overrides.mdscripts/diagnose-page-tokens.jstests-php/CssGeneratorFlatBundleTest.phptests/override-effect-probe.mjstests/php-harness/emit-override-css.php
Review feedback on #221. layers_enabled() read css_flat, but get_url() lets slashed/css_bundle_url (and the per-integration filters on top of it) serve a bundle that setting does not describe. A flat bundle served while css_flat is false gets layered overrides that can never win — the exact defect this PR exists to fix — and the inverse puts unlayered rules above the framework's @media-scoped rules. Derive the mode from the resolved URL whenever it is recognisably one of SLASHED's own bundles, fall back to the setting for a URL that cannot be read, and add slashed/css_layers_enabled so a host serving an unrecognisable bundle can state it outright. url_layer_mode() is pure so the naming contract is tested directly. That made layers_enabled() reach SLASHED_PATH, which only slashed.php defines — an integration plugin running standalone loads this class without it, so the dark-mode bridges would have fataled on an undefined constant. get_url() now returns the empty-URL path there, as it already does for a missing bundle file. Also from the review, in the probe and its PHP harness: bind the fixture server to loopback instead of every interface (awaiting the listening event, since passing a host makes listen() resolve asynchronously and address() would still be null), always close the browser and server via try/finally so no exit path orphans a Chromium, and send PHP diagnostics to stderr so a stray notice cannot corrupt the JSON stdout the probe parses. Verified: 228 PHPUnit assertions including seven new URL cases, 198 node tests, the probe green in both bundle modes with no orphaned processes, and the missing-bundle bail-out exiting 1 after cleanup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DftXg4tjRkey2c3ybxAWjN
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.
Summary
The flat CSS bundles (with
css_flat = true) have all@layerdeclarations stripped, making any layered CSS inert against them — an unlayered declaration beats every layered one regardless of specificity or source order. Token overrides were being emitted wrapped in@layer slashed.overrides, which is correct for the layered bundles but silently fails against flat bundles, causing all configurator controls to stop working when flat mode is enabled.This change makes the override CSS wrapper follow the bundle actually being served: layered when
Slashed_CSS_Loader::layers_enabled()is true (layered bundle), unlayered when false (flat bundle). The same logic is applied to the Bricks and Gutenberg dark-mode bridges, which had the same vulnerability.Changes
class-css-generator.php: Addeduse_cascade_layer()method that checksSlashed_CSS_Loader::layers_enabled()and emits the override block wrapped in@layer slashed.overridesonly when the served bundle has layers. Against flat bundles, the same declarations are emitted unlayered.class-css-loader.php: Addedlayers_enabled()public method (readsSlashed_Settings::get_css_flat()) andwrap_layer()helper for inline CSS that needs conditional wrapping.wrap_layer()so they follow the same bundle-aware logic.CssGeneratorFlatBundleTestto verify the wrapper switches correctly, andoverride-effect-probe.mjs— a manual QA tool that measures whether each configurator control actually changes anything on the page by diffing live tokens in a headless browser against both layered and flat bundles.troubleshooting-token-overrides.mdanddiagnose-page-tokens.js(browser console snippet) to help diagnose why a control reads back correctly but changes nothing on a specific site.Type
Checklist
feat:,fix:,docs:, …)npm testpassesnpm run lintpasses (stylelint +php -l)npm run verifypasses (version metadata in sync)CHANGELOG.mdupdated under## [Unreleased]editor-app/admin-appsource changed (no SPA changes)Notes
The flat-bundle regression was introduced when flat CSS variants were added to the framework but the plugin's override emission and dark-mode bridges were not updated to match. This fix ensures both code paths (layered and flat) work correctly by making the wrapper choice data-driven: it reads the same
css_flatsetting that determines which bundle is served, so they stay in sync.The
override-effect-probe.mjstool is a manual dev/QA aid (not part ofnpm testor CI) that mechanically verifies every control group can actually move the page. It caught this regression and will prevent it from recurring.https://claude.ai/code/session_01DftXg4tjRkey2c3ybxAWjN
Summary by CodeRabbit
Improvements
Documentation
Testing