fix(color-model): filter -dark source tokens consistently across Bricks/Gutenberg (PL-013/014/015)

[jackgranatowski]

Summary

First PR from SLASHED-Plugins' technical-debt audit remediation (Wave 0 — ships independently, no dependency on the SLASHED framework repo's own audit PRs). Targets this repo's integration branch claude/pr-469-audit-rebase-ggp0e4, mirroring the same one-branch/multiple-PRs workflow used on the SLASHED side.

• PL-013 / PL-014 (verified bug): classifyVar()/classify_color() exist in three near-identical implementations — Bricks JS, Gutenberg JS, Gutenberg PHP — that had silently drifted. Only Gutenberg's JS correctly filtered both the -light and -dark source-duplicate token suffixes; Bricks' JS and Gutenberg's PHP only filtered -light, so Bricks' color picker rendered a spurious duplicate swatch for every -dark token that Gutenberg correctly hid. Fixed both to match Gutenberg JS's already-correct behavior.
  • Also ported Gutenberg filterModel's missing Array.isArray(group.sections)/Array.isArray(section.swatches) defensive guards into Bricks, which never had them (a second, smaller drift the audit flagged in the same finding).
• PL-015 (test debt): added tests/color-model-cross-impl.test.js — a parametrized regression test running a shared fixture list against all three implementations, so they can't silently re-diverge again. The PHP side is exercised via tests/php-harness/classify-color.php, a small reflection-based harness (classify_color() is private and reads no instance state, so newInstanceWithoutConstructor() avoids needing a full WordPress bootstrap), shelled out to via execFileSync matching this repo's existing lint-php.js convention. Left a TODO to migrate this into the PHPUnit suite once PR-A3 (Brain Monkey scaffold) lands, per the remediation plan.
• Rebuilt SLASHED-for-WP/integrations/bricks/assets/editor-app/app.js to include the source fix, matching this repo's existing built-asset commit convention (confirmed the diff is a minimal 2-line change in the minified bundle, not a full rewrite).

Test plan

• node --test tests/*.test.js — 95/95 passed, including the new 28-case cross-impl suite
• Load-bearing check: confirmed the new test actually catches the original bug — temporarily reverted the Bricks JS fix and re-ran; exactly the 4 expected subtests failed (the two -dark fixture vars × 2 assertions each), then restored the fix
• npm run lint:php (php -l across all 32 PHP files) — clean
• npm run verify (version-metadata consistency) — clean
• npm run build:editor-app — succeeds; rebuilt bundle committed

Not verified here

composer phpstan/vendor/bin/phpcs could not be run in this sandbox — composer install fails on every dev dependency (both dist and source-fallback downloads) with "Could not authenticate against github.com," a pre-existing environment network limitation unrelated to this change. php -l syntax-checked the modified PHP file successfully as a partial substitute.

---

Generated by Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8e8acbc3-aa73-4702-8b92-6fe9f36d24fe

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/plugins-pr-b1-color-model-fix

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Fix Bricks/Gutenberg color-token filtering drift and add cross-impl regression test

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Filter both -light and -dark source-duplicate color tokens across JS/PHP implementations.
• Add defensive guards in Bricks model filtering to match Gutenberg’s robustness.
• Add cross-implementation regression tests (incl. PHP reflection harness) to prevent re-drift.

Diagram

graph TD
  T("cross-impl test") --> B["Bricks color-model.js"] --> U[["Bricks built app.js"]]
  T("cross-impl test") --> G["Gutenberg color-model.js"]
  T("cross-impl test") --> H("PHP classify harness") --> P["Gutenberg class-presets.php"]
  subgraph Legend
    direction LR
    _t("Test / harness") ~~~ _s["Source file"] ~~~ _b[["Built asset"]]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Single shared classifier spec + code generation

• ➕ Eliminates drift by generating JS/PHP implementations from one source of truth
• ➕ Can also generate fixtures and documentation
• ➖ Higher upfront tooling cost (build step, generators, review overhead)
• ➖ May be overkill for a small classifier that rarely changes

2. Extract shared JS classifier and consume from both Bricks & Gutenberg builds

• ➕ Removes JS-to-JS drift entirely by using one module
• ➕ Simplifies future changes (one edit)
• ➖ Repo/module boundary changes; might not fit existing integration layouts
• ➖ Doesn’t address PHP behavior without additional work

3. Move PHP comparison into PHPUnit (no shell-out)

• ➕ Better integration with PHP tooling and reporting
• ➕ Avoids exec + JSON plumbing and environment dependencies
• ➖ Requires a WP test scaffold (not currently available per PR notes)
• ➖ More setup complexity than the current lightweight harness

Recommendation: The PR’s approach is the right pragmatic step for Wave-0 remediation: align behavior by fixing the two drifting implementations, then lock parity with a single cross-impl regression suite. The reflection-based PHP harness is acceptable as an interim solution (given missing PHPUnit/WP scaffolding), but the TODO to migrate into PHPUnit once the test scaffold lands is the right long-term direction.

Files changed (5) +148 / -7

Bug fix (2) +9 / -5

color-model.jsFilter '-dark' source tokens and harden filterModel iteration +5/-3

Filter '-dark' source tokens and harden filterModel iteration

• Extends classifyVar() filtering to exclude both '-light' and '-dark' source-duplicate tokens. Adds Array.isArray guards for group.sections and section.swatches in filterModel() to match Gutenberg’s defensive behavior.

SLASHED-for-WP/integrations/bricks/editor-app/src/lib/color-model.js

class-presets.phpMatch PHP classify_color() filtering with JS implementations +4/-2

Match PHP classify_color() filtering with JS implementations

• Updates classify_color() to treat both '-light' and '-dark' suffixes as internal source tokens and return null for both, preventing UI-level duplicates and aligning with the JS ports.

SLASHED-for-WP/integrations/gutenberg/includes/class-presets.php

Tests (2) +137 / -0

color-model-cross-impl.test.jsAdd cross-implementation color classification regression suite +95/-0

Add cross-implementation color classification regression suite

• Introduces a parametrized Node test that runs a shared fixture list against Bricks JS, Gutenberg JS, and Gutenberg PHP, asserting consistent filtering and consistent family/kind/key output. Uses a PHP harness via execFileSync to evaluate the PHP implementation.

tests/color-model-cross-impl.test.js

classify-color.phpAdd reflection-based PHP harness for classify_color() +42/-0

Add reflection-based PHP harness for classify_color()

• Adds a standalone script that loads the Gutenberg presets class and reflectively invokes the private classify_color() method without a WordPress bootstrap. Reads var names from stdin and returns JSON results for consumption by the Node regression test.

tests/php-harness/classify-color.php

Other (1) +2 / -2

app.jsRebuild Bricks editor bundle to include updated token filtering +2/-2

Rebuild Bricks editor bundle to include updated token filtering

• Updates the committed built/minified Bricks editor asset so runtime behavior matches the source fix. Diff is minimal and corresponds to the classifier logic change.

SLASHED-for-WP/integrations/bricks/assets/editor-app/app.js

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 1 rule

1. Unguarded php call in tests ✓ Resolved 🐞 Bug ☼ Reliability

Description

tests/color-model-cross-impl.test.js invokes execFileSync('php', ...) without checking PHP
availability first, so npm test can fail with a low-signal spawn/ENOENT error in environments
where Node tests run without PHP installed. This repo’s other PHP tooling already performs an
explicit php presence check and emits a clear error, so the test should do the same (or skip with a
clear message).

Code

tests/color-model-cross-impl.test.js[R53-60]

+before(() => {
+  const output = execFileSync('php', [PHP_HARNESS], {
+    input: JSON.stringify(FIXTURE_VARS),
+    encoding: 'utf8',
+  });
+  const phpResults = JSON.parse(output);
+  phpResultsByVar = new Map(phpResults.map((r) => [r.var, r.result]));
+});

Relevance

⭐⭐⭐ High

Repo already checks for missing php and exits clearly in scripts/lint-php.js; team accepts
prerequisite-guarding patterns.

PR-#1
PR-#50

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test suite runs during npm test and unconditionally shells out to php, so missing PHP
will break the default test command; existing repo tooling shows a precedent for proactively
checking PHP availability and emitting a clear message.

tests/color-model-cross-impl.test.js[53-60]
package.json[13-35]
scripts/lint-php.js[29-34]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`tests/color-model-cross-impl.test.js` shells out to `php` in a `before()` hook without verifying `php` exists on `PATH`. When `php` is missing, the test run fails early with an opaque spawn/ENOENT-style error instead of a clear message or an intentional skip.

### Issue Context
- `npm test` runs `node --test tests/*.test.js`, so this new suite is exercised by default.
- The repo already has a convention for checking PHP availability (`scripts/lint-php.js` probes `php --version` and prints a helpful message).

### Fix Focus Areas
- tests/color-model-cross-impl.test.js[53-60]

### Suggested fix
- Add an upfront check similar to `scripts/lint-php.js`:
 - Attempt `execFileSync('php', ['--version'], { stdio: 'ignore' })`.
 - If it fails, either:
   - throw a new `Error('php not found on PATH — install PHP to run tests/color-model-cross-impl.test.js')` (fail with a clear message), **or**
   - skip this describe block (if you want JS-only tests to still run) with an explicit skip reason.
- Optionally add a small `timeout` to the harness exec to avoid hanging the test runner if PHP blocks unexpectedly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---