fix(bricks): emit brand base token in CSS export + color overrides; de-dupe font collector - #230
Merged
Merged
Conversation
Two spots missed by the partial base->surface rename revert (62b7337) still read the non-existent `brand_surface` key and emitted a phantom `--sf-color-surface-light`, silently dropping a user's `brand_base` override: - admin-app export.js (client-side "Export CSS") — rebuilt the bundle - class-inventory.php get_admin_color_overrides() — wrong editor swatch preview (its own docblock says it mirrors the CSS generator, which uses `base`) Also de-duplicate the ~110-line Bricks font collector: the canonical implementation now lives once in Slashed_Token_Page::get_bricks_fonts() and the REST endpoint is a thin wrapper; the shared transient key is a single constant. No behavioural change. php -l clean; node --test 64/64; admin SPA rebuilds cleanly. Co-authored-by: Jack Granatowski <contact@codeslash.net>
|
Warning Review limit reached
More reviews will be available in 4 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR consolidates Bricks font collection into a shared transient-cached method in ChangesRe-audit: Font collection and color token fixes
Sequence DiagramsequenceDiagram
participant REST as Bricks REST Handler
participant Collector as Slashed_Token_Page
participant Cache as Transient Cache
REST->>Collector: get_bricks_fonts()
Collector->>Cache: get_transient(CPT_FONTS_TRANSIENT)
Cache-->>Collector: cached CPT fonts or null
Collector->>Collector: enumerate CPT posts & deduplicate
Collector->>Cache: set_transient(CPT_FONTS_TRANSIENT)
Collector-->>REST: merged font list
REST-->>REST: return fonts payload
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/SLASHED-for-WP/integrations/bricks/includes/class-fonts-rest.php (1)
45-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRegister CPT cache invalidation outside REST route registration.
save_post_{BRICKS_DB_CUSTOM_FONTS}is currently attached insideregister_routes(), which runs viarest_api_init. On requests where REST isn’t initialized, the invalidation hook won’t be bound, so font cache can remain stale until TTL expiry. Move this hook registration to an always-loaded bootstrap path.🤖 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 `@plugins/SLASHED-for-WP/integrations/bricks/includes/class-fonts-rest.php` around lines 45 - 48, The save_post_{BRICKS_DB_CUSTOM_FONTS} hook is being registered inside register_routes() (called via rest_api_init) so it won't be bound on non-REST requests; move the add_action( 'save_post_' . BRICKS_DB_CUSTOM_FONTS, array( $this, 'bust_cpt_cache' ) ) registration out of register_routes() into an always-run bootstrap location (for example the class constructor or an init hook that runs on every request) so bust_cpt_cache() is always attached and CPT font cache invalidation runs regardless of REST initialization.
🧹 Nitpick comments (1)
plugins/SLASHED-for-WP/integrations/bricks/includes/class-inventory.php (1)
241-241: ⚡ Quick winConsider adding an inline comment explaining the
baseinclusion andsurfaceexclusion.The corresponding change in
export.jsincludes a helpful comment clarifying thatsurfaceis a derived token without a-lightsource. Adding a similar note here would prevent future confusion and align the documentation quality across the client and server implementations.📝 Suggested inline comment
- $brand_colors = array( 'primary', 'secondary', 'tertiary', 'action', 'neutral', 'base' ); + // Brand families: 'base' is the source token (--sf-color-base-light); + // 'surface' is derived with no -light source, so it's excluded. + $brand_colors = array( 'primary', 'secondary', 'tertiary', 'action', 'neutral', 'base' ); $status_colors = array( 'success', 'warning', 'error', 'info', 'danger' );🤖 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 `@plugins/SLASHED-for-WP/integrations/bricks/includes/class-inventory.php` at line 241, Add an inline comment above the $brand_colors = array( 'primary', 'secondary', 'tertiary', 'action', 'neutral', 'base' ); declaration in class-inventory.php that explains why 'base' is included and why 'surface' is intentionally excluded (e.g., note that 'surface' is a derived token without a -light source and thus not exported/handled here), mirroring the explanatory note in export.js to prevent future confusion when reading the $brand_colors list.
🤖 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.
Outside diff comments:
In `@plugins/SLASHED-for-WP/integrations/bricks/includes/class-fonts-rest.php`:
- Around line 45-48: The save_post_{BRICKS_DB_CUSTOM_FONTS} hook is being
registered inside register_routes() (called via rest_api_init) so it won't be
bound on non-REST requests; move the add_action( 'save_post_' .
BRICKS_DB_CUSTOM_FONTS, array( $this, 'bust_cpt_cache' ) ) registration out of
register_routes() into an always-run bootstrap location (for example the class
constructor or an init hook that runs on every request) so bust_cpt_cache() is
always attached and CPT font cache invalidation runs regardless of REST
initialization.
---
Nitpick comments:
In `@plugins/SLASHED-for-WP/integrations/bricks/includes/class-inventory.php`:
- Line 241: Add an inline comment above the $brand_colors = array( 'primary',
'secondary', 'tertiary', 'action', 'neutral', 'base' ); declaration in
class-inventory.php that explains why 'base' is included and why 'surface' is
intentionally excluded (e.g., note that 'surface' is a derived token without a
-light source and thus not exported/handled here), mirroring the explanatory
note in export.js to prevent future confusion when reading the $brand_colors
list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a7b8698-099c-4986-883c-042f878adde6
📒 Files selected for processing (6)
CODE-AUDIT.mdplugins/SLASHED-for-WP/includes/class-token-page.phpplugins/SLASHED-for-WP/integrations/bricks/admin-app/src/lib/export.jsplugins/SLASHED-for-WP/integrations/bricks/assets/admin-app/app.jsplugins/SLASHED-for-WP/integrations/bricks/includes/class-fonts-rest.phpplugins/SLASHED-for-WP/integrations/bricks/includes/class-inventory.php
Behaviour-preserving cleanups found during a full read of editor-app
(~5.4k lines — no functional bugs found, only these nits):
- ColorPanel: remove dead `referenceMode` prop (mode is derived from
`onPickValue`); drop the matching arg in ColorApp.
- ColorPanel: collapse the ~90-line Brand/Status palette duplication into
one `{#snippet familyScanner}` rendered twice. app.css is byte-identical,
confirming a pure structural refactor. Bundle rebuilt.
- color-swatches.js: collapse redundant else-if/else (both appendChild).
- slashed-bricks.php: extract the parser/resolver/inventory require_once
trio shared by both bootstrap paths into slashed_bricks_require_data_classes().
Also expand CODE-AUDIT.md: document these fixes, the verified-already-resolved
items, and a "Remaining — needs a decision" list (triplicated DIST_SHA,
duplicated standalone bootstrap, apply.js migrate edge case, css-parser regex,
migrate-keys dual naming) — each intentional or needing a live-Bricks decision.
php -l clean; node --test 64/64; editor-app rebuilds cleanly.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Addresses CodeRabbit review on PR #230: - Real fix: the save_post_{BRICKS_DB_CUSTOM_FONTS} cache-bust hook was registered inside Slashed_Bricks_Fonts_REST::register_routes(), which only runs on rest_api_init. A normal admin save of a custom-font post therefore left the slashed_bricks_cpt_fonts transient stale for up to an hour. Moved invalidation to Slashed_Token_Page::flush_bricks_fonts_cache() (the always-loaded canonical owner of the transient) and hooked it from slashed_bricks_data_init() (plugins_loaded, all request types). REST class no longer registers the hook. - Nitpick: added the base/surface explanatory comment in class-inventory.php to mirror the note in export.js. php -l clean on all changed files; PHP-only change, no bundle rebuild needed.
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.
This pull request was created by @kiro-agent on behalf of @jackgranatowski 👻
Comment with /kiro fix to address specific feedback or /kiro all to address everything.
Learn about Kiro Web
WP plugin code audit — fixes
Focused re-audit of
plugins/SLASHED-for-WP. Most prior High/Medium audit items were already resolved (CSS-generator allowlist validation,slashed.phpcleanup + cron moved to the global layer, color defaults consolidated throughSlashed_Token_Defaults). This PR fixes the two genuine bugs that remained, plus one duplication.1.
surface→baseregression (functional bug)The framework's only source brand token is
--sf-color-base-light;--sf-color-surfaceis a derived semantic alias (= var(--sf-color-base)) with no-light/-darksource. Commit62b7337partially reverted abase → surfacerename but missed two spots that read the non-existentbrand_surfacekey and emitted a phantom--sf-color-surface-light, silently dropping the user'sbrand_baseoverride:admin-app/src/lib/export.js— client-side "Export CSS" (bundle rebuilt)includes/class-inventory.phpget_admin_color_overrides()— wrong editor color-swatch preview; its own docblock says it mirrors the CSS generator, which usesbaseCross-checked against the source of truth (
class-token-defaults.php,class-css-generator.php,ColorTab.svelte,color-model.js,LivePreview.svelte, andtests/color-model.test.js).2. Bricks font collector de-duplicated (maintainability)
Slashed_Token_Page::get_bricks_fonts()andSlashed_Bricks_Fonts_REST::get_fonts()were ~110-line near-verbatim copies (sharedslashed_bricks_cpt_fontstransient) that had started to drift. The collector now lives once inSlashed_Token_Page::get_bricks_fonts()(the always-loaded canonical owner, in both unified and standalone modes); the REST endpoint is a thin wrapper. Shared transient key is now a single constantSlashed_Token_Page::CPT_FONTS_TRANSIENT. No behavioural change.Changed files
integrations/bricks/admin-app/src/lib/export.js(+ rebuiltassets/admin-app/app.js, 1-line bundle diff)integrations/bricks/includes/class-inventory.phpincludes/class-token-page.php(canonical font collector + shared transient constant)integrations/bricks/includes/class-fonts-rest.php(now a thin wrapper)CODE-AUDIT.md(documents this pass + remaining open items)Testing
php -lclean on all changed PHP filesnode --test→ 64/64 passapp.jsnow emits...neutral","baseand no longer...neutral","surfaceKnown limitations / left for follow-up
editor-app/src/lib/apply.jsmigrate path still reads a pre-batchglobalClassessnapshot for the two-siblings-into-one-new-class edge case. It lives in the compiled editor bundle and needs a live Bricks editor to verify safely, so it was intentionally left unchanged here.class-css-parser.php[^}]*regex is brittle on a}inside an@propertyinitial-value (non-fatal: cached, editor-only).Summary by CodeRabbit
Bug Fixes
Documentation
Refactor