chore: technical debt audit remediation — full batch - #146
Merged
Merged
Conversation
…ks/Gutenberg PL-013/PL-014: 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 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. PL-015: added tests/color-model-cross-impl.test.js, a parametrized regression test running a shared fixture list against all three implementations to guarantee they can't silently re-diverge. 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 WordPress bootstrap) shelled out to via execFileSync, matching the plugin'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. Verified: node --test tests/*.test.js (95/95, including the new 28-case cross-impl suite — confirmed it catches the original bug by temporarily reverting the Bricks fix and observing exactly the 4 expected subtest failures), php -l via npm run lint:php, npm run verify, and a full build:editor-app rebuild. composer phpstan/phpcs could not be run in this sandbox — composer install fails on all dev dependencies with "Could not authenticate against github.com" for both dist and source fallback downloads, a pre-existing environment network limitation unrelated to this change.
Matches scripts/lint-php.js's existing convention so a missing php binary fails with a clear message instead of an opaque ENOENT from execFileSync. Also caps the harness call with a timeout so a stuck php process can't hang the test runner.
…names SLASHED's SL-016 (codec.ts's `fa` → `generateCSS`) and SL-022 (lucide-svelte → @lucide/svelte) landed in main via #474. AppOverlay.svelte is plugin-specific and not vendored, so it kept referencing the old names and broke `vite build` as soon as CI's prebuild sync pulled the renamed exports from slashed@main — failing on every open plugin PR, not just the one that triggered it. Swaps the import/call site to generateCSS and updates package.json/ package-lock.json to depend on @lucide/svelte instead of the deprecated lucide-svelte package.
…ator PL-025: sync-core.mjs now supports --check/--dry-run, reporting missing/ stale/orphaned vendored files against the framework source without ever writing to disk. All writes are routed through a single writeFile() helper so the never-write invariant only needs to hold in one place. Adds scripts/check.js (npm run check) to run the class-hints, variables- hints, and admin-app sync checks together and aggregate their exit status. Also: PL-036 fixes CLAUDE.md's npm run sync location (it's defined in admin-app/package.json, not the root), PL-033 trims .syncignore's stale historical PR reference, and PL-037 adds the missing assets/, data/, tests/, and docs/ directories to CLAUDE.md's structure diagram.
PR-C1's branch forked before #135 landed, so it inherited the same stale fa/lucide-svelte references breaking `vite build`.
npm run check (svelte-check) had no equivalent of dev/build's predev/prebuild sync hook, so it type-checked whatever was already on disk. On a fresh checkout with the committed vendored src/ still predating the lucide-svelte rename, that meant a stale module-resolution error instead of an accurate check against the current framework source.
- copyLocalDir: open the fd once and reuse it for both the isDirectory check and the file read (CodeQL: potential file system race condition) instead of a separate statSync + readFileSync on the path. - GitHub-mode syncignore preservation: use O_NOFOLLOW like the local-mode block already does, instead of a plain 'r' open that would follow symlinks (Qodo). - reportOrphans(): also scan framework-css/core/ for files outside the fixed CHROME_LAYERS set, not just src/ (Qodo). - Document the accepted trust boundary for the GitHub-fetch-to-disk write path (CodeQL: network data written to file) — hardcoded repo/ ref, HTTPS transport, and destination paths already confined to SRC regardless of fetched content.
This branch predates the SL-016/SL-022 rename fix, so it inherited the same stale fa/lucide-svelte references breaking `vite build` in CI.
…review effect PL-029/030: window.slashedApp is a plain global any script on the page can clobber before plugin-main.ts runs. Add a same-origin check before using cssUrl as a stylesheet href, and replace the (window as any) cast with a typed local accessor (mirroring persistence.ts's own window.slashedApp read) instead of widening to any. PL-031: AppOverlay.svelte's live-preview effect (injectLivePreview + registerPreviewDoc) re-ran on every override change with no coalescing, unlike PreviewPanel.svelte's SL-020 fix. A fast-changing control (dragging a slider) could re-run it many times per frame even though the underlying <style> rewrite only needs to happen once per paint. Wrapped it in the same rAF-coalescing pattern.
PL-034: the script hardcoded an absolute path to one specific sandbox's
global playwright install (/opt/node22/...), assumed a test-admin.html
fixture that isn't committed anywhere in the repo, assumed a dev server
already running on port 9999, and had no pass/fail assertions (a run
with console errors still exited 0). None of that is fixable without
committing a fixture, starting a server in CI, and rewriting it as real
assertions — out of scope here.
Instead: replace the hardcoded path with a normal `import('playwright')`
that fails with clear install instructions instead of a cryptic
ERR_MODULE_NOT_FOUND, and document in the file header and CLAUDE.md that
this is a manual local tool, not part of `npm test`/CI.
…iew effect Svelte 5's \$effect only tracks reactive reads that happen during its own synchronous execution. The rAF-coalescing added for PL-031 read overrides only inside the (later-firing) requestAnimationFrame callback, so the effect would never re-run after the first paint -- the live preview would silently stop reacting to override changes. PreviewPanel.svelte's SL-020 fix (the pattern this was meant to mirror) already captures its reactive dependencies synchronously before entering the rAF callback for exactly this reason -- this file just missed it.
…-model-fix fix(color-model): filter -dark source tokens consistently across Bricks/Gutenberg (PL-013/014/015)
…n-check-script feat: add sync drift checking (PL-025/033/036/037)
…end-hardening fix: harden frontend overlay's cssUrl injection and preview effect (PL-029/030/031)
…right-docs docs: document playwright-admin.js as manual-only (PL-034)
Wave 1 (PR-SYNC): catches admin-app/src/ and framework-css/core/ up to the SLASHED framework audit fixes merged to main via #474 (SL-001..034) -- the committed vendored copy had predated all of them since before this remediation effort started, which is exactly the drift PR-C1's --check mode has been reporting all session. Ran `npm run sync` against the current local SLASHED checkout (verified byte-identical to origin/main for configurator/src and badges/), then rebuilt assets/admin-app/ per this repo's committed-build-output convention. Verified: `sync-core.mjs --check` now reports zero drift, `npm run check` has no new errors (the one remaining error, plugin-main.ts's .ts import extension, is pre-existing and unrelated), `npm run build` succeeds, `npm test` passes (95/95), `npm run lint` is clean.
…chup chore: re-sync vendored configurator core from SLASHED main (PR-SYNC)
…n classes PR-A2 (architecture cleanup): - Slashed_Color_Resolver mixed generic OKLab/sRGB color-space math (oklch_to_hex, hex_to_oklch, mix_rgb, etc.) with SLASHED-specific semantic-token resolution logic. Extracted the math into a new dependency-free Slashed_Color_Math class; the resolver now delegates to it instead of duplicating both concerns in one 876-line file. - Slashed_Inventory bundled CSS-bundle resolution/caching with a ~110-entry hardcoded category-display-name map -- presentation data living inside a data-resolution class. Extracted it into a new Slashed_Category_Map class; categorize_variable()/category_order() now delegate to it. Both extractions verified behavior-preserving: ran the pre-refactor and post-refactor versions of each class standalone (via a throwaway harness, not committed) against representative inputs covering every branch -- resolve()/resolve_dark() across a full color set, and categorize_variable() across every category-map segment plus unknown segments (Misc fallback) -- output was byte-identical in both cases. Also updated the two require_once chains that load class-inventory.php directly (bypassing the plugin's central load order) -- the Bricks Slashed_Bricks_Color_Resolver compat shim and both integrations' Inventory subclasses -- to pull in the new classes too.
…otstrap slashed_bricks_require_data_classes() required the new shared class from SLASHED_BRICKS_PATH . 'includes/class-color-math.php', which resolves inside the Bricks integration's own includes/ (no such file there) instead of the shared top-level includes/ where Slashed_Color_Math actually lives. Match Gutenberg's already-correct '../../includes/class-color-math.php'.
Adds phpunit/phpunit (^9.6, PHP 7.4-compatible) as a composer dev dependency, a phpunit.xml pointed at a new tests-php/ suite, and a bootstrap that defines ABSPATH and stubs sanitize_key() — the only WordPress function any of the covered classes touch — so the suite never needs a WP bootstrap or a mocking framework. Covers three pure/near-pure targets identified in the audit: Slashed_CSS_Parser::parse(), Slashed_CSS_Generator::validate_override_value() (plus its is_css_safe()/balanced_parens() defence-in-depth guards), and Slashed_REST_Controller::sanitize_rebemer_element_map(). Every assertion was cross-checked against the real implementation via a standalone php -r harness before being committed, since composer install can't reach github.com in this sandbox (pre-existing network limitation) to actually run the suite here — CI's quality job, which already runs a real composer install, is the first place this executes for real. Wires composer phpunit into CI's quality job and documents the new tests-php/ convention in CLAUDE.md.
composer.lock was generated on this sandbox's local PHP 8.4, so `composer require phpunit/phpunit` resolved doctrine/instantiator 2.1.0 (requires PHP ^8.4) as a transitive dependency. CI runs PHP 8.2 and failed composer install with a platform-requirement conflict. Pinning config.platform.php to 7.4 (composer.json's own declared floor) makes dependency resolution deterministic regardless of which PHP version composer happens to run on, and re-locks phpunit/phpunit's tree onto doctrine/instantiator 1.5.0, which supports the full 7.4-8.4 range.
Only define ABSPATH when not already set, matching the defined() guard every plugin file uses, so running this suite in a context where ABSPATH is already defined doesn't emit a redefinition warning.
apply.js (buildPlan, applyAutoNumbering, applyToSubtree) had zero test
coverage despite being the core mutation path for the reBEMer panel —
680 lines handling five apply modes, sibling auto-numbering collision
resolution, migrate-mode key lifting/conflict detection, and an
error-triggered rollback across a whole subtree apply.
buildPlan()/computeBlockAssignment() are pure and tested directly.
applyToSubtree() only reaches live Bricks state through bricks-api.js,
which itself needs just one DOM call
(document.querySelector('[data-v-app]') → a Vue-app-shaped object
exposing $_state) to resolve — so these tests stub that single call
with an in-memory fake state tree and let the real bricks-api.js run
against it, rather than mocking the api module. This exercises the
actual integration (findElement, upsertGlobalClass, setElementClasses,
batchMutations) including a real induced-failure rollback test, not a
synthetic mock of the API surface.
Covers: mode validation, auto-numbering (collision resolution,
authoritative-provenance protection, post-numbering integrity check),
modifier slug resolution order, sub-block scoping, all five apply
modes' happy paths, migrate-mode's conflict-refusal and additive-merge
paths, and full rollback on a mid-apply error.
Three gaps in the GitHub-API fallback path of the vendoring sync (used when no local sibling framework checkout is found — e.g. a fresh CI checkout): - ghFetch() made a single fetch attempt with no retry, so a transient network blip or a momentary GitHub rate-limit hit failed the whole sync outright. Added fetchWithRetry(): exponential backoff on network errors, 5xx responses, and a rate-limit- exhausted 403 (x-ratelimit-remaining: 0) specifically — a plain 403/404 still fails immediately so the existing "keep the vendored copy" fallback in syncGhFile()/vendorChromeRemote() isn't delayed by pointless retries on genuine not-found/permission errors. - syncGhDir()'s recursive Promise.all() fan-out was unbounded: a wide/deep configurator/src tree could issue hundreds of concurrent GitHub API requests, which is exactly what trips secondary rate limits. Added createLimiter(), a small bounded-concurrency job queue shared across the whole recursive walk (not just per directory), capping total in-flight requests to 6. - Added tests/sync-core.test.js covering fetchWithRetry and createLimiter with injected fetch/sleep — the first test coverage sync-core.mjs itself has had (existing tests/sync.test.js only covers the separate verify-sync.js). main() still does real fs writes and real network calls, so it isn't imported/exercised directly; it's now gated behind an isMainModule check so importing the module for these unit tests doesn't trigger a real sync as an import side effect. Verified with `node scripts/sync-core.mjs --check` (local-source path, unaffected by this PR) and the full node --test suite — no regressions.
…imiter Qodo caught a real, likely-live bug: syncGhDir() wrapped its whole recursive syncGhDir()/syncGhFile() call in ghLimiter, so a directory job held a limiter slot for its entire subtree's duration while its own children queued behind the same limiter for a slot — a reentrant lock-holding pattern that starves or deadlocks once enough directory jobs are simultaneously in flight. This is a very plausible explanation for a CI "Build admin-app" failure on this same PR: the GitHub-path sync fetched only 7 of ~50 files and then silently stopped with no error, consistent with the walk getting stuck once enough concurrent directory jobs occupied the limiter's slots. Fix: move the limiter to wrap only the atomic HTTP request itself (inside ghFetch(), via fetchWithRetry), not the recursive directory traversal. syncGhDir()'s fan-out is unbounded again structurally, but every actual GitHub API call anywhere in the whole recursive tree funnels through the same ghFetch() -> ghLimiter, so total concurrent network requests are still capped at 6 tree-wide — with no self-referential wait possible, since a slot is now held only for the duration of one HTTP request rather than a whole subtree. Also fixes two related createLimiter() robustness bugs Qodo flagged in the same review: - A job that throws synchronously (rather than returning a rejected promise) used to skip the .finally() cleanup entirely, permanently leaking its slot and stalling the queue forever. Now invoked via Promise.resolve().then(fn) so a sync throw is caught the same way as an async rejection. - createLimiter(limit) silently built a permanently-stalled limiter for limit <= 0 (active >= limit is true from the start, so next() never runs anything). Now throws a TypeError immediately for any non-positive or non-integer limit. Added tests for both: a synchronously-throwing job still frees its slot for subsequent jobs, and invalid limit values throw at construction. Re-verified the whole GitHub-path walk against the real codeslash-dev/slashed repo with a throwaway script exercising just the exported helpers (49 files, 7 dirs, completed cleanly) before this fix; local --check mode and the full test suite (110 tests) still pass after it.
SLASHED-for-WP/assets/admin-app/, integrations/bricks/assets/editor-app/, and dist/ are Vite/framework build output (verified against the actual outDir config in both admin-app/vite.config.js and integrations/bricks/editor-app/vite.config.js, and dist/'s own CLAUDE.md description) — never hand-edited, and previously showed up as noisy full-file diffs and skewed the repo's language stats on every rebuild. Deliberately excludes SLASHED-for-WP/assets/admin/ (settings.css, bricks-settings.css): those are hand-written admin-page styles, not Vite output, confirmed by checking there's no build script targeting that directory. Also suppresses line diffs on *.min.css/*.min.js/*.js.map/*.css.map specifically, since a "changed" diff on minified/source-map content is never something a reviewer can meaningfully read.
…rchitecture refactor: extract color-math and category-map from oversized PHP classes
…it-scaffold test: add PHPUnit scaffold for pure/near-pure PHP logic
…-tests test: add coverage for the Bricks reBEMer apply.js engine
…hardening fix: harden sync-core.mjs's GitHub API sync path
…tributes chore: mark built SPA/CSS output as linguist-generated
|
Warning Review limit reached
Next review available in: 42 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 (2)
📝 WalkthroughWalkthroughThis PR refactors codec/color-math logic, adds check-mode drift detection to the framework sync script, extracts shared PHP color math and category-mapping classes used across Bricks/Gutenberg integrations, updates Svelte icon imports and save-state handling, adds PHPUnit test infrastructure, and updates CI, config, and docs. ChangesFrontend codec, persistence, and UI wiring
Estimated code review effort: 4 (Complex) | ~60 minutes PHP color math and category map extraction
Estimated code review effort: 3 (Moderate) | ~30 minutes Sync-core drift detection, check pipeline, and PHPUnit/CI infra
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as sync-core.mjs main()
participant WriteFile as writeFile()
participant GH as GitHub API (ghFetch)
participant Limiter as createLimiter
CLI->>GH: syncGhFile()/vendorChromeRemote()
GH->>Limiter: acquire slot
Limiter->>GH: fetchWithRetry(url)
GH-->>CLI: fetched content
CLI->>WriteFile: writeFile(path, content)
WriteFile-->>CLI: compare vs disk (check mode) or write (sync mode)
CLI->>CLI: reportOrphans() / finishCheck()
sequenceDiagram
participant Effect as Svelte $effect
participant RAF as requestAnimationFrame
participant Iframe as Preview iframe
Effect->>Effect: capture overrides synchronously
Effect->>RAF: schedule callback
RAF->>Iframe: write generateCSS(overrides) to style tag
RAF->>Iframe: set data-theme/data-lumlocker
RAF->>Iframe: registerPreviewDoc(doc)
Effect->>RAF: cancel on cleanup
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
PR Summary by Qodochore: technical debt audit remediation — full batch
AI Description
Diagram
High-Level Assessment
Files changed (53)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
1 rule 1. Vendored App.svelte modified
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SLASHED-for-WP/admin-app/src/AppOverlay.svelte (1)
73-73: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAppOverlay.svelte is missing the
saveState: 'error'handling added to App.svelte.
App.svelte's identical save-state logic was updated in this PR to add an'error'variant (type union, reset-on-error, and catch-block assignment), but this duplicate implementation inAppOverlay.sveltestill resets to'idle'on save failure — so a failed save via the frontend overlay gives the user no visible feedback (only aconsole.warn).🐛 Proposed fix to mirror App.svelte's error handling
- let saveState = $state<'idle' | 'saving' | 'saved'>('idle'); + let saveState = $state<'idle' | 'saving' | 'saved' | 'error'>('idle');- if (saveState === 'saved') saveState = 'idle'; + if (saveState === 'saved' || saveState === 'error') saveState = 'idle';} catch (err) { console.warn('slashed: save failed', err); - saveState = 'idle'; + saveState = 'error'; }Also applies to: 155-155, 177-180
🤖 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 `@SLASHED-for-WP/admin-app/src/AppOverlay.svelte` at line 73, AppOverlay.svelte’s duplicated save flow is missing the new error state, so mirror the App.svelte save-state updates in the AppOverlay logic. Update the saveState union to include 'error', change the save-failure path in the save handler to set saveState to 'error' instead of resetting to 'idle', and ensure the reset-on-error behavior matches the corresponding App.svelte implementation in the save button/update flow.
🧹 Nitpick comments (4)
tests-php/RestControllerSanitizeRebemerElementMapTest.php (1)
116-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCap test only verifies count, not retained-entry identity.
Confirms
REBEMER_MAP_CAPtruncation count but not that the first N entries (per insertion order) are the ones kept.♻️ Optional: assert first-N entries retained
$result = Slashed_REST_Controller::sanitize_rebemer_element_map( $raw ); $this->assertCount( Slashed_REST_Controller::REBEMER_MAP_CAP, $result ); + $this->assertArrayHasKey( 'type-0', $result ); + $this->assertArrayNotHasKey( 'type-' . Slashed_REST_Controller::REBEMER_MAP_CAP, $result );🤖 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-php/RestControllerSanitizeRebemerElementMapTest.php` around lines 116 - 125, The cap test only checks the final count and should also verify which entries are preserved after truncation. Update test_caps_the_entry_count_at_rebemer_map_cap in RestControllerSanitizeRebemerElementMapTest to assert that sanitize_rebemer_element_map keeps the first REBEMER_MAP_CAP insertion-order entries from the raw array, using Slashed_REST_Controller::REBEMER_MAP_CAP and the existing $raw/$result variables.scripts/check.js (1)
30-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSwallowed spawn errors give no diagnostic when a check script itself fails to launch.
If
execFileSyncthrows for a reason other than the subprocess's own reported failure (e.g., missing file, spawn error), the barecatch { failed = true; }drops the error entirely with no message printed, sincestdio: 'inherit'only surfaces the subprocess's own output. Loggingerr.messageon catch would aid debugging such spawn-level failures.♻️ Proposed fix
try { execFileSync(process.execPath, [file, '--check'], { cwd, stdio: 'inherit' }); - } catch { + } catch (err) { + console.error(`[check] ${label} failed to run: ${err.message}`); failed = true; }🤖 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 `@scripts/check.js` around lines 30 - 37, The CHECKS loop in check.js swallows launch-time failures from execFileSync, so missing files or spawn errors produce no diagnostics. Update the try/catch around execFileSync in the CHECKS runner to accept the thrown error object and log its message before setting failed = true, so the script reports which check failed to start while preserving the existing failed flag behavior.SLASHED-for-WP/admin-app/scripts/sync-core.mjs (1)
174-186: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
listRelFilesfollows symlinks viastatSync, unlike the rest of the file.Every other traversal in this file (copyLocalDir, the preserve-across-wipe reads) deliberately avoids following symlinks via
O_NOFOLLOW.listRelFilesusesstatSync, which dereferences symlinks, so a symlink cycle undersrc/(or a symlinked directory) would cause unbounded recursion / a stack overflow during--check. Low real-world likelihood sincesrc/is normally populated only by this script's own writes, but worth aligning with the rest of the file's symlink-safety posture (e.g.,lstatSync+ skip symlinks).🤖 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 `@SLASHED-for-WP/admin-app/scripts/sync-core.mjs` around lines 174 - 186, `listRelFiles` currently dereferences symlinks with `statSync`, which can recurse indefinitely on symlinked directories or cycles during `--check`. Update this helper to match the symlink-safety used elsewhere in `sync-core.mjs` (for example, `copyLocalDir` and the preserve-across-wipe reads) by using `lstatSync` and skipping symlink entries instead of descending into them. Keep the recursive traversal in `listRelFiles` returning only real files relative to `base`, and use the existing helpers like `readdirSync`, `relative`, and `toPosix` unchanged where possible.SLASHED-for-WP/admin-app/src/lib/savedThemes.ts (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPossible duplicate
isStringRecordimplementation.Cross-file context shows
persistence.tsdefines an identicalisStringRecordfunction (same body, same line range) and calls it internally inloadInitialOverrideswithout importing it from this module. If both files independently define this guard, any future change to validation logic will need to be kept in sync manually. Consider having one module own the canonical implementation and have the other import it.🤖 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 `@SLASHED-for-WP/admin-app/src/lib/savedThemes.ts` around lines 13 - 18, The `isStringRecord` type guard in `savedThemes.ts` is duplicated elsewhere with the same logic, so there should be a single canonical implementation. Move or keep `isStringRecord` in one owning module and update the other module (including `loadInitialOverrides` in `persistence.ts`) to import and reuse that shared function instead of maintaining an identical copy.
🤖 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/admin-app/package.json`:
- Around line 13-14: The package.json lifecycle hook is causing `check` to run
the full `sync-core.mjs` sync implicitly, which can mutate files and fetch data
during a simple validation command. Update the `precheck`/`check` setup so `npm
run check` only verifies drift, using the non-mutating `--check` or `--dry-run`
path in `scripts/sync-core.mjs` (or remove the `precheck` hook entirely if that
better matches the intended workflow). Keep the `precheck` and `check` entries
aligned with the script’s `--check` support so type-checking stays side-effect
free.
In `@SLASHED-for-WP/admin-app/src/types.ts`:
- Around line 22-83: Move the type definitions change out of the generated app
copy and into the upstream framework source, since src/types.ts in this repo is
not protected by .syncignore and will be overwritten on the next resync. Update
the corresponding types file in the framework repo, keeping the new interfaces
DecodeOptions, ShareOptions, ApiIndex, ClassIndex, TokenRegistry, ApiIndexToken,
SlashedClass, and TokenRegistryEntry aligned with codec.ts and the generated
JSON shapes.
In `@SLASHED-for-WP/includes/class-color-math.php`:
- Around line 32-38: The parse_oklch() parser in class-color-math.php only
recognizes unitless L C H values with optional deg, so update it to also accept
CSS percentage lightness and an optional "/ alpha" component. Adjust the regex
and returned parsing in parse_oklch() so inputs like oklch(45% 0.2 264 / 0.5)
are accepted, while preserving the existing behavior for current OKLCH formats.
---
Outside diff comments:
In `@SLASHED-for-WP/admin-app/src/AppOverlay.svelte`:
- Line 73: AppOverlay.svelte’s duplicated save flow is missing the new error
state, so mirror the App.svelte save-state updates in the AppOverlay logic.
Update the saveState union to include 'error', change the save-failure path in
the save handler to set saveState to 'error' instead of resetting to 'idle', and
ensure the reset-on-error behavior matches the corresponding App.svelte
implementation in the save button/update flow.
---
Nitpick comments:
In `@scripts/check.js`:
- Around line 30-37: The CHECKS loop in check.js swallows launch-time failures
from execFileSync, so missing files or spawn errors produce no diagnostics.
Update the try/catch around execFileSync in the CHECKS runner to accept the
thrown error object and log its message before setting failed = true, so the
script reports which check failed to start while preserving the existing failed
flag behavior.
In `@SLASHED-for-WP/admin-app/scripts/sync-core.mjs`:
- Around line 174-186: `listRelFiles` currently dereferences symlinks with
`statSync`, which can recurse indefinitely on symlinked directories or cycles
during `--check`. Update this helper to match the symlink-safety used elsewhere
in `sync-core.mjs` (for example, `copyLocalDir` and the preserve-across-wipe
reads) by using `lstatSync` and skipping symlink entries instead of descending
into them. Keep the recursive traversal in `listRelFiles` returning only real
files relative to `base`, and use the existing helpers like `readdirSync`,
`relative`, and `toPosix` unchanged where possible.
In `@SLASHED-for-WP/admin-app/src/lib/savedThemes.ts`:
- Around line 13-18: The `isStringRecord` type guard in `savedThemes.ts` is
duplicated elsewhere with the same logic, so there should be a single canonical
implementation. Move or keep `isStringRecord` in one owning module and update
the other module (including `loadInitialOverrides` in `persistence.ts`) to
import and reuse that shared function instead of maintaining an identical copy.
In `@tests-php/RestControllerSanitizeRebemerElementMapTest.php`:
- Around line 116-125: The cap test only checks the final count and should also
verify which entries are preserved after truncation. Update
test_caps_the_entry_count_at_rebemer_map_cap in
RestControllerSanitizeRebemerElementMapTest to assert that
sanitize_rebemer_element_map keeps the first REBEMER_MAP_CAP insertion-order
entries from the raw array, using Slashed_REST_Controller::REBEMER_MAP_CAP and
the existing $raw/$result variables.
🪄 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: 18e19f91-9531-4e31-aaf6-af77f2fb7427
⛔ Files ignored due to path filters (2)
SLASHED-for-WP/admin-app/package-lock.jsonis excluded by!**/package-lock.jsoncomposer.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
.gitattributes.github/workflows/ci.yml.gitignoreCLAUDE.mdSLASHED-for-WP/admin-app/.syncignoreSLASHED-for-WP/admin-app/.vendored-manifest.jsonSLASHED-for-WP/admin-app/framework-css/core/layout.cssSLASHED-for-WP/admin-app/framework-css/core/themes.cssSLASHED-for-WP/admin-app/framework-css/core/tokens.cssSLASHED-for-WP/admin-app/package.jsonSLASHED-for-WP/admin-app/scripts/sync-core.mjsSLASHED-for-WP/admin-app/src/App.svelteSLASHED-for-WP/admin-app/src/AppOverlay.svelteSLASHED-for-WP/admin-app/src/components/DomainPanel.svelteSLASHED-for-WP/admin-app/src/components/panels/CheatsheetPanel.svelteSLASHED-for-WP/admin-app/src/components/panels/ExportPanel.svelteSLASHED-for-WP/admin-app/src/components/panels/HomePanel.svelteSLASHED-for-WP/admin-app/src/components/panels/ThemesPanel.svelteSLASHED-for-WP/admin-app/src/components/shell/PreviewPanel.svelteSLASHED-for-WP/admin-app/src/components/shell/SidebarNav.svelteSLASHED-for-WP/admin-app/src/components/shell/StudioHeader.svelteSLASHED-for-WP/admin-app/src/lib/codec.tsSLASHED-for-WP/admin-app/src/lib/persistence.tsSLASHED-for-WP/admin-app/src/lib/previewResolver.svelte.tsSLASHED-for-WP/admin-app/src/lib/savedThemes.tsSLASHED-for-WP/admin-app/src/plugin-main.tsSLASHED-for-WP/admin-app/src/types.tsSLASHED-for-WP/assets/admin-app/app.cssSLASHED-for-WP/assets/admin-app/app.jsSLASHED-for-WP/includes/class-category-map.phpSLASHED-for-WP/includes/class-color-math.phpSLASHED-for-WP/includes/class-color-resolver.phpSLASHED-for-WP/includes/class-inventory.phpSLASHED-for-WP/integrations/bricks/assets/editor-app/app.jsSLASHED-for-WP/integrations/bricks/editor-app/src/lib/color-model.jsSLASHED-for-WP/integrations/bricks/includes/class-color-resolver.phpSLASHED-for-WP/integrations/bricks/includes/class-inventory.phpSLASHED-for-WP/integrations/bricks/slashed-bricks.phpSLASHED-for-WP/integrations/gutenberg/includes/class-inventory.phpSLASHED-for-WP/integrations/gutenberg/includes/class-presets.phpSLASHED-for-WP/integrations/gutenberg/slashed-gutenberg.phpcomposer.jsonpackage.jsonphpunit.xmlscripts/check.jstests-php/CssGeneratorValidateOverrideValueTest.phptests-php/CssParserTest.phptests-php/RestControllerSanitizeRebemerElementMapTest.phptests-php/bootstrap.phptests/apply.test.jstests/color-model-cross-impl.test.jstests/php-harness/classify-color.phptests/playwright-admin.jstests/sync-core.test.js
💤 Files with no reviewable changes (1)
- SLASHED-for-WP/admin-app/.syncignore
Comment on lines
+22
to
+83
| // SL-017/024: types for the two consumed-generated-JSON shapes and codec.ts's | ||
| // options objects, replacing `any` at their 7 call sites. | ||
|
|
||
| /** A token entry as it appears in data/api-index.generated.json's `tokens` array. */ | ||
| export interface ApiIndexToken extends SlashedToken { | ||
| fallbackOnly?: boolean; | ||
| optional?: boolean; | ||
| layer?: string | null; | ||
| bundles?: string[]; | ||
| } | ||
|
|
||
| /** Shape of data/api-index.generated.json (configurator/scripts/sync-api.mjs's output). */ | ||
| export interface ApiIndex { | ||
| _sync?: Record<string, unknown>; | ||
| tokens: ApiIndexToken[]; | ||
| } | ||
|
|
||
| /** A class entry as it appears in data/classes.generated.json's `classes` array. */ | ||
| export interface SlashedClass { | ||
| name: string; | ||
| selector: string; | ||
| kind: string; | ||
| category: string; | ||
| group?: string; | ||
| description?: string; | ||
| optional?: boolean; | ||
| layer?: string | null; | ||
| } | ||
|
|
||
| /** Shape of data/classes.generated.json (configurator/scripts/sync-api.mjs's output). */ | ||
| export interface ClassIndex { | ||
| _sync?: Record<string, unknown>; | ||
| classes: SlashedClass[]; | ||
| } | ||
|
|
||
| /** A registry entry as it appears in data/token-registry.generated.json's `tokens` array. */ | ||
| export interface TokenRegistryEntry { | ||
| id: number; | ||
| name: string; | ||
| removed?: boolean; | ||
| } | ||
|
|
||
| /** Shape of data/token-registry.generated.json, consumed by codec.ts's encode/decode. */ | ||
| export interface TokenRegistry { | ||
| _meta?: Record<string, unknown>; | ||
| tokens: TokenRegistryEntry[]; | ||
| } | ||
|
|
||
| /** Options accepted by codec.ts's decode(). */ | ||
| export interface DecodeOptions { | ||
| sanitize?: (value: string) => string; | ||
| isKnown?: (tokenName: string) => boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Options accepted by codec.ts's readShareFromHash() / readShareFromHashIfPresent(). | ||
| * Deliberately omits `sanitize` (unlike DecodeOptions): these are the public | ||
| * share-link entry points and always force sanitizeValue as a CSS-injection | ||
| * safeguard, so a caller-supplied sanitize is never honoured. | ||
| */ | ||
| export type ShareOptions = Pick<DecodeOptions, "isKnown">; | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate and print .syncignore, then check whether the edited src files are protected.
fd -H -t f '^\.syncignore$' SLASHED-for-WP/admin-app --exec sh -c 'echo "== {} =="; cat "{}"'
echo "---- membership check ----"
fd -H -t f '^\.syncignore$' SLASHED-for-WP/admin-app --exec grep -nE 'types\.ts|codec\.ts|lib/' {} \; || echo "no matching entries found"Repository: codeslash-dev/SLASHED-Plugins
Length of output: 583
Move this change upstream
SLASHED-for-WP/admin-app/.syncignore only exempts src/plugin-main.ts and src/AppOverlay.svelte; src/types.ts is not protected here, so this edit will be overwritten on the next core resync unless it lives in the framework repo.
🤖 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 `@SLASHED-for-WP/admin-app/src/types.ts` around lines 22 - 83, Move the type
definitions change out of the generated app copy and into the upstream framework
source, since src/types.ts in this repo is not protected by .syncignore and will
be overwritten on the next resync. Update the corresponding types file in the
framework repo, keeping the new interfaces DecodeOptions, ShareOptions,
ApiIndex, ClassIndex, TokenRegistry, ApiIndexToken, SlashedClass, and
TokenRegistryEntry aligned with codec.ts and the generated JSON shapes.
Source: Path instructions
Comment on lines
+32
to
+38
| public static function parse_oklch( $str ) { | ||
| $str = trim( $str ); | ||
| if ( ! preg_match( '/oklch\(\s*([0-9.]+)\s+([0-9.]+)\s+([0-9.]+)(?:deg)?\s*\)/i', $str, $m ) ) { | ||
| return null; | ||
| } | ||
| return array( (float) $m[1], (float) $m[2], (float) $m[3] ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where color-*-dark overrides / oklch strings originate to confirm accepted formats
rg -nP -C3 "color-[a-z]+-dark|oklch\(" --type=php --type=ts --type=jsRepository: codeslash-dev/SLASHED-Plugins
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the parser plus its callers/inputs.
git ls-files 'SLASHED-for-WP/includes/**' | rg 'class-color-math\.php|class-color-resolver\.php|color'
printf '\n--- outline: class-color-math.php ---\n'
ast-grep outline SLASHED-for-WP/includes/class-color-math.php --view expanded || true
printf '\n--- outline: class-color-resolver.php ---\n'
ast-grep outline SLASHED-for-WP/includes/class-color-resolver.php --view expanded || true
printf '\n--- parse_oklch + callers ---\n'
rg -n -C 4 'parse_oklch|hex_to_oklch|derive_dark_sources|oklch\(' SLASHED-for-WP/includes/class-color-math.php SLASHED-for-WP/includes/class-color-resolver.phpRepository: codeslash-dev/SLASHED-Plugins
Length of output: 14153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace where `color_values` comes from and which formats are documented/produced.
rg -n -C 4 'color_values|--sf-color-.*-dark|initial-value|oklch\(' SLASHED-for-WP/includes SLASHED-for-WP -g '!*.min.*'
printf '\n--- class-color-resolver.php (relevant ranges) ---\n'
sed -n '1,120p' SLASHED-for-WP/includes/class-color-resolver.php
printf '\n--- class-color-resolver.php (resolve_sources / derive_dark_sources) ---\n'
sed -n '280,360p' SLASHED-for-WP/includes/class-color-resolver.php
printf '\n--- class-color-math.php (parse_oklch docs + regex) ---\n'
sed -n '20,45p' SLASHED-for-WP/includes/class-color-math.phpRepository: codeslash-dev/SLASHED-Plugins
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact inputs flowing into the resolver and the token formats that reach it.
rg -n -C 3 'class-color-resolver|color_values|--sf-color-.*-dark|--sf-color-.*-light|parse_oklch|hex_to_oklch' \
SLASHED-for-WP/includes \
SLASHED-for-WP/admin-app/src \
--glob '!**/*.map' --glob '!**/node_modules/**' --glob '!**/*.min.*' \
| sed -n '1,260p'Repository: codeslash-dev/SLASHED-Plugins
Length of output: 22098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '60,130p' SLASHED-for-WP/includes/class-css-parser.php
printf '\n---\n'
sed -n '1,120p' SLASHED-for-WP/includes/class-token-store.phpRepository: codeslash-dev/SLASHED-Plugins
Length of output: 6992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how override values are validated before they can reach the resolver.
rg -n -C 4 'sanitize.*color|oklch\(|source-dark|source-light|update_overrides|get_overrides|tokens/overrides|rest' \
SLASHED-for-WP/includes \
SLASHED-for-WP/admin-app/src \
--glob '!**/*.map' --glob '!**/*.min.*' \
| sed -n '1,240p'Repository: codeslash-dev/SLASHED-Plugins
Length of output: 20418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the override validation/emission rules to see what values can reach `parse_oklch`.
sed -n '1,220p' SLASHED-for-WP/includes/class-css-generator.php
printf '\n---\n'
rg -n -C 3 'validate_override_value|is_css_safe|emit|override' SLASHED-for-WP/includes/class-css-generator.phpRepository: codeslash-dev/SLASHED-Plugins
Length of output: 14590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the color validation branch that feeds stored overrides.
sed -n '320,384p' SLASHED-for-WP/includes/class-css-generator.phpRepository: codeslash-dev/SLASHED-Plugins
Length of output: 2393
Expand parse_oklch() to handle CSS oklch() percentages and / alpha
The override save path already accepts oklch() syntax, but this parser only handles unitless L C H plus optional deg. Values like oklch(45% 0.2 264 / 0.5) fall back to the auto-derived dark source in the PHP preview.
🤖 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 `@SLASHED-for-WP/includes/class-color-math.php` around lines 32 - 38, The
parse_oklch() parser in class-color-math.php only recognizes unitless L C H
values with optional deg, so update it to also accept CSS percentage lightness
and an optional "/ alpha" component. Adjust the regex and returned parsing in
parse_oklch() so inputs like oklch(45% 0.2 264 / 0.5) are accepted, while
preserving the existing behavior for current OKLCH formats.
…tate CodeRabbit review on #146: - admin-app's "precheck" hook ran a real sync-core.mjs sync (writes, GitHub fetches) as an npm pre-script side effect of "npm run check" (svelte-check), the opposite of what --check/--dry-run mode exists for. Add --check so precheck verifies drift and fails loudly on staleness instead of silently mutating files before every type-check. - AppOverlay.svelte's save flow is a plugin-owned (non-vendored) duplicate of App.svelte's, and never got the 'error' saveState App.svelte gained — a failed save from the frontend overlay silently reset to 'idle' with only a console.warn, unlike the main admin panel which now shows a clear error state. Mirrors App.svelte/StudioHeader's saveState union, reset-on-error, and catch-block assignment, plus a matching AlertTriangle/red-tinted icon state in the overlay's compact save button. Verified: svelte-check passes with no new errors (the one pre-existing plugin-main.ts import-extension warning predates this PR), and the full node --test suite (142 tests) passes.
2 tasks
This was referenced Jul 7, 2026
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
Merges the technical-debt audit's staging branch into
main. This is the missing counterpart tocodeslash-dev/SLASHED#474— the equivalent step never happened for this repo, so none of the audit work below has been in any plugin release up to and including v0.4.20.main's current tip is the exact common ancestor with this branch (verified viagit merge-base), so this merges cleanly with zero conflicts, and both branches already agree on the0.4.20version number — no version-file drift to resolve.What's included
-darksource tokens consistently across Bricks/Gutenberg (PL-013/014/015) — a real classification-drift bug between the Bricks JS, Gutenberg JS, and Gutenberg PHP color implementations, plus a 3-way regression test locking the fix innpm run checkorchestrator +--check/--dry-runmode for the vendored-source sync script, plus several doc fixescssUrlinjection and preview effect (PL-029/030/031) — same-origin validation, a typed window global, and a Svelte 5 reactivity bug fixplaywright-admin.jsas manual-only (PL-034)main(the vendored copy had drifted genuinely out of date)Slashed_Color_Math/Slashed_Category_Mapout of oversized PHP classes — includes a real bug fix: the Bricks bootstrap loaded the new shared class from the wrong path and would have fataled on first load in a fresh Bricks-only installapply.jsengine, which had zero coverage despite being the core mutation path for that featuresync-core.mjs's GitHub API sync path (retry/backoff, concurrency cap) — includes a real bug fix: my first concurrency-limiter design had a reentrant-lock deadlock risk, caught by review and confirmed against the live GitHub API before the fix shippedlinguist-generatedto quiet noisy diffsVerification
git merge-base origin/main origin/claude/pr-469-audit-rebase-ggp0e4confirmsmain's tip is the common ancestor — this is a clean merge, not a rebase/conflict-resolution situation.Test plan
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes