Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 144 additions & 0 deletions CODE-AUDIT.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@
> export.js `base β†’ surface` rename). This document reflects the state of the
> codebase at audit time; resolved items are kept for historical context.

> **2026-06-05 WP-plugin re-audit pass** β€” a focused re-review of the
> `plugins/SLASHED-for-WP` tree confirmed most prior High/Medium items were
> already fixed (CSS-generator allowlist validation, `slashed.php` cleanup +
> cron moved to the global layer, color defaults consolidated through
> `Slashed_Token_Defaults`). Two genuine bugs, one PHP duplication, and a set
> of editor-app cleanups were fixed in this pass β€” see
> **"Re-audit fixes (2026-06-05)"** below. A deep read of the entire
> `editor-app` (~5.4k lines) found **no functional bugs** β€” only the small
> cleanups listed. Items that need a product/architecture decision before
> touching are collected under **"Remaining β€” needs a decision"**.

Whole-repo code review across four domains: build scripts, the WordPress PHP
plugin, the Svelte editor/admin apps, and the CSS source. Overall the codebase
is unusually clean and well-documented β€” most "odd" patterns are deliberate and
Expand All @@ -15,6 +26,139 @@ Date: 2026-06-02

---

## βœ… Re-audit fixes (2026-06-05)

Applied on branch `audit/wp-plugin-cleanup`. All PHP files pass `php -l`; the
admin SPA rebuilds cleanly; `node --test` is 64/64 green.

### 1. `surface` β†’ `base` regression fixed in the two remaining stragglers

The framework's only *source* brand token is `--sf-color-base-light`;
`--sf-color-surface` is a derived semantic alias (`= var(--sf-color-base)`) with
no `-light`/`-dark` source. Commit `62b7337` partially reverted a
`base β†’ surface` rename but missed two spots, both of which read the
non-existent `brand_surface` key and emitted a phantom
`--sf-color-surface-light`, silently dropping the user's `brand_base` override:

- `integrations/bricks/admin-app/src/lib/export.js:70` β€” client-side "Export
CSS". **Fixed** `'surface'` β†’ `'base'` and rebuilt the bundle
(`assets/admin-app/app.js`, one-line diff).
- `integrations/bricks/includes/class-inventory.php:241`
(`get_admin_color_overrides()`) β€” its own docblock says it mirrors
`Slashed_CSS_Generator::generate_color_declarations()` (which uses `base`), so
the editor color-swatch preview for the base color was wrong. **Fixed.**

Cross-checked against the source of truth: `class-token-defaults.php`,
`class-css-generator.php`, `ColorTab.svelte`, `color-model.js`,
`LivePreview.svelte` all use `base`, as does `tests/color-model.test.js`.

### 2. Bricks font collection de-duplicated

`Slashed_Token_Page::get_bricks_fonts()` and
`Slashed_Bricks_Fonts_REST::get_fonts()` were ~110-line near-verbatim copies of
the same option-probing + Font-Manager-CPT query + dedup logic (sharing the
`slashed_bricks_cpt_fonts` transient) that had already started to drift. The
collector now lives once in `Slashed_Token_Page::get_bricks_fonts()` (the
always-loaded canonical owner, in both unified and standalone modes); the REST
endpoint is a thin wrapper, and the shared transient key is a single constant
`Slashed_Token_Page::CPT_FONTS_TRANSIENT`. No behavioural change.

### 3. editor-app cleanups (reBEMer + Color System)

A full read of all ~5.4k lines of `editor-app` confirmed the bundle is in very
good shape β€” solid lifecycle teardown (every module wires an `AbortController`
+ `destroy()`), debounced + pre-filtered mutation observers, pure unit-tested
helpers, and an allowlist-based migrate path with snapshot/rollback. The
following small, behaviour-preserving cleanups were applied and the bundle
rebuilt (`assets/editor-app/app.js`):

- **Dead prop `referenceMode` removed.** `ColorPanel.svelte` declared and
`ColorApp.svelte` passed `referenceMode`, but the component derives its mode
purely from whether `onPickValue` is supplied (`pickerMode`). The prop was
never read. Removed from both the prop list and the call site.
- **`ColorPanel.svelte` Brand/Status duplication collapsed.** The two palette
sections were ~90 lines of near-identical markup differing only by the source
group list. Extracted into a single `{#snippet familyScanner(groups)}`
rendered twice (`brandGroups` / `statusGroups`) so they can't drift. Pure
structural refactor β€” `app.css` is byte-identical, confirming no class/markup
change.
- **Redundant branch in `color-swatches.js`.** `injectSFButton()` had an
`else if (varBtn) appendChild` / `else appendChild` pair that both did the
same thing; collapsed to a single `else`.

### 4. PHP: duplicated `require_once` block extracted

`slashed-bricks.php` loaded the same parser β†’ resolver β†’ inventory trio in both
`slashed_bricks_data_init()` (plugins_loaded) and `slashed_bricks_init()`
(after_setup_theme). The two paths gate on different signals and must each load
their deps independently, so the calls can't simply be removed β€” instead the
shared trio is now `slashed_bricks_require_data_classes()`, called from both.
Idempotent via `require_once`; no behavioural change.

### 5. PR #230 review feedback addressed (CodeRabbit)

- **CPT font-cache invalidation moved to an always-loaded path (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 (non-REST) admin save of a custom-font post would
therefore not invalidate the `slashed_bricks_cpt_fonts` transient, leaving a
stale font list for up to an hour. The invalidation now lives on
`Slashed_Token_Page::flush_bricks_fonts_cache()` (the always-loaded canonical
owner of the transient) and is hooked from `slashed_bricks_data_init()`
(`plugins_loaded`, all request types). The REST class no longer registers the
hook. Pre-existing bug, surfaced by the de-dup refactor.
- **Inline `base`/`surface` comment added in `class-inventory.php`** to mirror
the note in `export.js`, so the brand-family list documents why `base` is
included and `surface` (a derived alias) is not.

### Verified already-resolved since the 2026-06-02 snapshot

CSS-generator allowlist validation (`valid_color`/`valid_dimension`/
`valid_font_family` + `is_css_safe` + `balanced_parens`); `slashed.php` no-op
activation hook removed and the version-check cron clean-up moved to the global
layer; `class-color-resolver.php` default colors derived from
`Slashed_Token_Defaults`; the "kept for tests" `get_colors()`/`get_classes()`
wrappers and the empty `slashed_bricks_activation_check()` are gone. In
`editor-app`: the speculative "shape 3 (hypothetical)" history probe in
`bricks-api.js` is gone (only two version-verified shapes remain), and
`color-swatches.js` now pre-filters mutations via `touchesPicker()` + a 50 ms
debounce rather than running `.closest()` on every mutation.

### Remaining β€” needs a decision (not changed this pass)

Each of these is either intentional-by-design or carries enough behavioural risk
that it should be a deliberate choice rather than a drive-by edit:

1. **Triplicated `DIST_SHA` constant** β€” `slashed.php:41`,
`slashed-bricks.php:32`, `slashed-gutenberg.php:32` hand-sync the same SHA.
The inline comment says this is **deliberate** so each integration file stays
self-contained and distributable standalone; the version-sync workflow keeps
them aligned. Decision needed: accept the documented duplication, or add a
shared canonical constant with a standalone fallback. *Left as-is.*
2. **Duplicated `slashed_inject_token_overrides()`** β€” defined in both
`slashed.php` and (guarded by `! defined('SLASHED_VERSION')`) in
`slashed-bricks.php`. This is the **standalone-mode bootstrap**: the Bricks
plugin must provide the function when the unified plugin isn't present.
Removing it would break standalone activation. Decision needed: keep, or
factor the shared bootstrap into a common file loaded by both. *Left as-is.*
3. **`apply.js` migrate edge case** β€” two siblings migrating into the *same new*
class name. Re-reading the full file, the live `getGlobalClasses()` reference
+ the additive merge block + auto-numbering's uniqueness guarantee appear to
cover this, but confirming it needs a live Bricks editor. Decision needed:
write an integration test harness, or leave the current (apparently-correct)
behaviour documented. *Left as-is β€” no blind change.*
4. **`class-css-parser.php` `[^}]*` regex** β€” breaks on a `}` inside an
`@property` initial-value. Non-fatal (cached, editor-only) and CSS parsing
via regex is inherently brittle. Decision needed: accept, or move to a small
tokeniser. *Left as-is.*
5. **`migrate-keys.js` allowlist carries both naming conventions** β€” e.g.
`_widthMin`/`_widthMax` *and* `_minWidth`/`_maxWidth`. Harmless (unknown keys
are simply never present), but worth confirming which Bricks actually uses
and pruning the dead half. Decision needed: verify against a live Bricks
build before trimming. *Left as-is.*

---

## πŸ”΄ High priority

### 1. `export.js` drops the `surface` brand color (confirmed regression)
Expand Down
55 changes: 47 additions & 8 deletions plugins/SLASHED-for-WP/includes/class-token-page.php
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,53 @@ public static function get_class_hints() {
}

/**
* Collect Bricks-registered fonts for the SPA bootstrap.
* Transient key caching the Bricks Font-Manager CPT font list.
*
* Mirrors the logic in Slashed_Bricks_Fonts_REST::get_fonts() without
* the REST request overhead. Returns an empty array when Bricks is not
* active or the integration is disabled.
* Shared with Slashed_Bricks_Fonts_REST, which busts it on
* save_post_{BRICKS_DB_CUSTOM_FONTS}. Kept here too because this class
* is the canonical owner of the collector and is always loaded, whereas
* the REST class is only required during REST dispatch.
*/
const CPT_FONTS_TRANSIENT = 'slashed_bricks_cpt_fonts';

/**
* Flush the cached Bricks Font-Manager CPT font list.
*
* Registered on save_post_{BRICKS_DB_CUSTOM_FONTS} from an always-loaded
* bootstrap path (see slashed-bricks.php) rather than from REST route
* registration, so the cache is invalidated on every custom-font save β€”
* including normal admin saves, not only during REST requests.
*/
public static function flush_bricks_fonts_cache() {
delete_transient( self::CPT_FONTS_TRANSIENT );
}

/**
* Collect every font Bricks already knows how to serve.
*
* Canonical implementation shared by the admin SPA bootstrap (here) and
* the REST endpoint (Slashed_Bricks_Fonts_REST::get_fonts(), which is a
* thin wrapper around this method). SLASHED never loads fonts itself β€”
* Bricks owns that pipeline β€” so this only enumerates names for the
* typography "Bricks fonts" dropdown.
*
* Bricks does not expose a PHP API for its font registry, so we probe the
* WP options it is known to use across versions and skip any unrecognised
* shapes gracefully (the SPA falls back to a manual text input):
* - bricks_custom_fonts: font_family | family | title | name
* - bricks_google_fonts: family | font_family | name | title
* - bricks_adobe_fonts: fonts[].font_family | family
* - Font Manager CPT (BRICKS_DB_CUSTOM_FONTS): the post title is the
* family name. Fonts created via the builder UI may stay in 'draft'
* even after the files upload, so both 'publish' and 'draft' are
* included. We read titles directly rather than calling
* Bricks\Custom_Fonts::get_custom_fonts() (static cache + @font-face
* side-effects + publish-only β€” all unnecessary for a name lookup),
* and cache the result in a 1-hour transient busted on CPT save.
*
* Returns an empty array when the Bricks integration is disabled.
*
* @return array[]
* @return array<int, array{family: string, label: string, source: string}>
*/
public static function get_bricks_fonts() {
if ( class_exists( 'Slashed_Settings' ) && ! Slashed_Settings::is_enabled( 'bricks' ) ) {
Expand Down Expand Up @@ -237,8 +277,7 @@ public static function get_bricks_fonts() {

// Fonts uploaded via Bricks Font Manager CPT (includes 'draft' status).
if ( defined( 'BRICKS_DB_CUSTOM_FONTS' ) ) {
$cache_key = 'slashed_bricks_cpt_fonts';
$cpt_cached = get_transient( $cache_key );
$cpt_cached = get_transient( self::CPT_FONTS_TRANSIENT );

if ( false !== $cpt_cached && is_array( $cpt_cached ) ) {
$fonts = array_merge( $fonts, $cpt_cached );
Expand All @@ -264,7 +303,7 @@ public static function get_bricks_fonts() {
'source' => 'custom',
);
}
set_transient( $cache_key, $cpt_fonts, HOUR_IN_SECONDS );
set_transient( self::CPT_FONTS_TRANSIENT, $cpt_fonts, HOUR_IN_SECONDS );
$fonts = array_merge( $fonts, $cpt_fonts );
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,10 @@ function generateColorDeclarations(settings) {
const declarations = [];

// Brand colors (light): brand_primary -> --sf-color-primary-light.
const brandColors = ['primary', 'secondary', 'tertiary', 'action', 'neutral', 'surface'];
// The final family is `base` (source token --sf-color-base-light), NOT
// `surface` β€” surface is a derived token with no -light source. Must stay
// in sync with class-css-generator.php::generate_color_declarations().
const brandColors = ['primary', 'secondary', 'tertiary', 'action', 'neutral', 'base'];
for (const color of brandColors) {
const key = `brand_${color}`;
if (hasValue(settings[key])) {
Expand Down
Loading