Fix/natural sort color palette - #131
Merged
Merged
Conversation
Implements approved design refinements from PR #129 review. The design doc moves from 'pre-implementation' to a per-item status table (§0). #1 Element-aware row pre-fill - New lib/element-types.js maps Bricks element.name → BEM label (heading → 'heading', image → 'image', layout containers fall through to 'item'). - BemPanel onMount uses suggestElementName() when the structure- panel label slugifies to nothing, so opening the panel on a fresh subtree doesn't require typing every name. - Provenance tracked on each row via suggestedFrom; the panel renders 'suggested' rows with italic muted text and a chip so the user knows what to override. #2 Sibling auto-numbering - New applyAutoNumbering() pre-pass in apply.js groups ops by finalClassName and appends -1, -2, ... in document order. - User-typed names (suggestedFrom: 'user') are authoritative and never auto-numbered. Two user-typed collisions hard-error rather than silently mangling. - Skipped rows (include: false) are excluded from the tally. - Modifier-mode collisions error out (block__title--lg-2 reads poorly; better to ask the user to disambiguate). #3 Per-row skip toggle - Already present in skeleton as the include checkbox; now formally aligned with §6.2 'skip' semantics — checkbox unchecks but the row stays visible for transparency, contributes nothing to apply or to collision detection. Documented in Row.svelte. #4 'Use existing' recommendation (client-side half of §11.3) - Row.svelte derives candidateClassName + checks against the globalClasses snapshot captured at panel open. When a class with the same name already exists, the row shows a green 'will attach existing class' hint. - This is the §11.3 recommendedAction: 'attach' path executed purely from in-memory state. The 'rename' / 'replace' paths need a server preflight (cross-page reference count) and ship in a follow-up PR. The §0 status table makes the split explicit. #5 Migrate ID styles mode - New 'migrate' value in the operation dropdown. - lib/migrate-keys.js holds the allowlist (padding, color, typography, border, layout, position, ~80 keys). Defense-in-depth: apply.js re-checks the allowlist before lifting any key, so a tampered row.migrateKeys can't smuggle non-style settings into a class. - Per-row chip strip (§6.3 preview) lists the keys that will lift, plus a 'N skipped' badge for keys present-but-not-allowlisted (which stay on the element by design). - Panel-level summary strip totals migrated/skipped across the included rows so the user can sanity-check before pressing Apply. - Lifted keys are deep-cloned into the new class' settings, then deleted from the element. Other classes preserved (additive). #10 Unused-class read-only report - New GET /wp-json/slashed-bricks/v1/rebemer/unused endpoint in new class-rebemer-rest.php. Capability gate matches the editor enqueue gate (bricks_full_access || manage_options). - count_class_references() helper walks postmeta for Bricks page content keys (_bricks_page_content_2 + legacy + header/footer), capped at REFERENCE_SCAN_CAP=5000 rows. Iterative-with-stack walk avoids deep recursion. Response includes truncated:true when the cap is hit so the UI can warn. - Returns naturally-sorted unused class list, total counts, and scan stats. Read-only — never touches bricks_global_classes. Goal #6 (no destructive writes) stays intact; deletion remains Bricks' Global Class Manager job. - Helper signature designed for reuse by the future preflight endpoint. Dev harness updates - index.html now seeds a mock __vue_app__.$_state and bricks_global_classes so the badge injector + recommendation hint + migrate chips are exercised in standalone dev. - Mock element types match real Bricks (heading, image, button, block, text-basic) so element-types.js is hit. Build artifacts - assets/editor-app/app.js + app.css regenerated. Bundle grew from skeleton size to ~60kB JS (+22.8kB gzip), ~7kB CSS (+1.7kB gzip). Filemtime cache-busting in class-rebemer-enqueue.php handles the version bump automatically. What does NOT ship in this PR (still spec-only, see §0 table) - Cross-page reference count preflight (§11) - Snapshot/rollback transactional apply (§10) - In-panel undo ring buffer (§15) - i18n string table (§16) - SLASHED-utility reserved-name guard wired from inventory (§13) These are on the roadmap; each is its own PR.
… and actions
Badge was being injected at the start of .actions (when found) or at
the start of the <li> as a fallback — neither matches Bricks' actual
structure-panel DOM, which is:
<li data-id="…">
<div class="structure-item">
<div class="title">…label…</div>
<ul class="actions">…icon buttons…</ul>
</div>
</li>
The fallback path was firing for users on current Bricks builds and
producing a chip overlaying the row. Fix:
- Selector: prefer :scope > .structure-item, locate ul.actions
inside it, insert the badge BEFORE actions (so it lands between
.title and .actions per design §6.1). Defensive fallbacks
preserve behavior for legacy/forked Bricks variants.
- Visual: drop the yellow pill in favor of small inline 'reBEM'
text — muted by default, accent-colored on hover, with a subtle
background tint. Reads as a discreet inline word, not a chip.
- .rebemer-badge-host: inline-flex, flex: 0 0 auto, 6px horizontal
margin so it slots cleanly into Bricks' row flex layout without
competing with the title for space.
- Badge text 'BEM' → 'reBEM' to match the product name.
Dev harness updated to reflect the real Bricks DOM (div.structure-item
> div.title + ul.actions), so the new selector logic is exercised
under npm run dev.
Resolves the 10 concerns surfaced in the design-level review of PR #130 (stored at semantic-review/2026-05-27-233854-pr-130.md, gitignored). #1 Migrate-mode silent data loss on existing-class collision - Added validateMigrate() pre-validation pass that runs BEFORE any mutation. Detects three cases: a) Target class doesn't exist → safe (creates with seed). b) Target class exists, no key overlap → safe (additive merge). c) Target class exists, same key with different value → CONFLICT. Hard-errors before any mutation, with a message naming the conflicting keys and suggesting Add mode or a different name. - Per-op merge logic: when target exists with no conflicts, missing seed keys are added to its settings before upsert. upsertGlobalClass still never overwrites — we only ADD keys it didn't have. The 'never overwrite' policy is preserved (overwrite = replacing existing values; this is purely additive). - End result: migrate is now safe in all three cases. removeMigratedKeys is only called after every key is guaranteed to be provided by the (new or existing) class with the same value. #2 Modifier-mode auto-numbering rejected legitimate multi-row applies - Two siblings each producing card__image--lg is the canonical 'attach this modifier to all of them' case, NOT a collision. Replaced the blanket name.includes('--') reject with a per-mode check: in modifier mode, intra-plan duplicates are intentional, no error and no numbering. upsertGlobalClass dedupes by name so all rows share the single class. #3 + #10 'label' provenance not honored as authoritative - Added AUTHORITATIVE_PROVENANCE = new Set(['user', 'label']) and use it consistently in applyAutoNumbering. Both user-typed and structure-panel-label-derived names are treated as authoritative and never auto-renumbered. Two authoritative rows colliding is a hard error (mentions row count, not provenance details, since the user shouldn't need to know about reBEMer's internal vocabulary). - Updated apply.js JSDoc typedef to enumerate all five provenance values: 'user' | 'label' | 'element-type' | 'fallback' | 'auto-number'. - §0 status table no longer overstates this row. #4 Auto-numbering didn't re-check post-numbered names - Added a final post-numbering integrity pass: in non-modifier modes, every op must have a unique finalClass after numbering. Catches the case where a user-typed card__image-1 collides with an auto-numbered card__image-1 from a different group. Modifier mode is exempt (duplicates are by design). #5 Badge dataset flag never cleared on host disconnect - Removed the ATTACHED_FLAG dataset entirely. refreshBadges() now uses badgeInstances map state as the source of truth, with two explicit passes: reap stale (host disconnected), then mount any <li> without a live host inside it. The li.contains(existing.host) check handles the common Bricks pattern of rerendering inner .structure-item without removing the outer <li>. - Eliminates the permanent-skip bug where a row that was rendered once and then had its inner subtree rebuilt by Bricks would lose its badge forever. #6 'Use existing' hint not migrate-aware - Row.svelte recommendation copy now branches on mode. Migrate-mode copy reflects the new validateMigrate semantics: 'On Apply, missing style keys will be merged into it. Conflicting values block the migration — pick a different name or use Add.' - Non-migrate modes keep the original 'attach the existing class instead of creating a duplicate' copy. #7 Auto-numbered names didn't refresh the 'use existing' hint - Refactored apply.js to expose a pure buildPlan({ rootId, rows, mode }) → { ok, ops, error }. Returns the ops with their POST-numbering finalClass values. - BemPanel.svelte computes a previewClassNames Map that runs buildPlan and indexes the result by row id. Reactive — re-runs when rows or mode change. - Row.svelte now accepts finalClassName as a prop (from the panel's preview map) instead of computing candidateClassName locally. The 'use existing' hint matches what apply.js will actually produce, even after auto-numbering. - Same buildPlan is used by applyToSubtree, so preview and apply can never disagree. #8 Unused-classes scan was non-deterministic past the cap - Added ORDER BY meta_id ASC to the postmeta query in class-rebemer-rest.php. The cap+1 truncation detection now pairs with reproducible content: two truncated runs on the same site return the same first 5000 rows. #9 Apply has no mid-apply rollback (acknowledged spec-only) - Snapshot/rollback transactional apply (§10) remains spec-only, but the partial-failure path now (a) console.warn-logs the failure with the count of completed mutations, and (b) surfaces a partial-success toast: 'Applied to N elements before halting: <error>. State is partially applied — undo via Bricks (Cmd-Z) before retrying.' The user has actionable info instead of a bare error. Also addresses cleanup - Added semantic-review/ to .gitignore so future review artifacts from the sub-agent don't accidentally land in commits. Build artifacts regenerated. Bundle grew from 60.20 kB to 62.25 kB JS (+0.6 kB gzip) — the new buildPlan refactor + validateMigrate pre-pass + post-numbering check. Acceptable for the correctness gain. What's still spec-only after this PR Items #4 and #6 in §0 status table are now ✅ shipped where they were partial; the remaining ❌ rows are unchanged (cross-page preflight, snapshot/rollback, undo ring buffer, i18n, reserved-name guard wired from inventory).
…tions feat(rebemer): implement element-aware suggestions, auto-numbering, migrate mode, unused-class endpoint
|
Warning Review limit reached
More reviews will be available in 48 minutes and 45 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 (3)
📝 WalkthroughWalkthroughThis PR adds a "Migrate ID styles" operation mode to reBEMer, allowing designers to lift element-specific styling into reusable global classes with validated conflict detection, preview tables, and warning summaries. It also introduces a WordPress REST API endpoint to scan and report unused global classes across Bricks pages. ChangesMigrate Mode Feature and Class Detection
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
integrations/bricks/editor-app/src/lib/apply.js (1)
140-145: ⚡ Quick winDefault missing provenance to a non-authoritative value.
Line 144 currently treats absent
suggestedFromas'user', which can trigger false duplicate errors instead of auto-numbering for callers that omit this optional field.Proposed patch
ops.push({ row, isRoot, finalClass, - suggestedFrom: row.suggestedFrom || 'user', + suggestedFrom: row.suggestedFrom || 'fallback', });🤖 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 `@integrations/bricks/editor-app/src/lib/apply.js` around lines 140 - 145, The code defaults missing provenance to 'user' when building ops (see ops.push and suggestedFrom), which makes absent provenance appear non-authoritative and breaks auto-numbering; change the behavior to not default suggestedFrom to 'user' — either omit the suggestedFrom property when row.suggestedFrom is undefined/null or explicitly set it to undefined/null (rather than 'user') so callers that omit provenance are treated as authoritative and will trigger auto-numbering instead of duplicate 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 `@integrations/bricks/editor-app/index.html`:
- Line 53: Update the harness copy that currently reads "Click the
<code>BEM</code> badge on any structure-panel row." to match the UI label by
changing it to "Click the <code>reBEM</code> badge on any structure-panel row."
so the instruction string in integrations/bricks/editor-app/index.html matches
the rendered badge label.
In `@integrations/bricks/includes/class-rebemer-rest.php`:
- Around line 121-124: The reported total (`$totalGlobalClasses` / response
totals) currently uses count($classes) even though malformed entries are
skipped, causing drift; update the loop that iterates over `$classes` (the
foreach handling `$cls`) to maintain an explicit processed counter (e.g.,
`$processedClasses++`) only for valid records (or decrement a running total when
you `continue`) and use that processed counter when building the response
instead of `count($classes)`; apply the same change to the other loop/location
referenced (the block around lines handling `$unused` and the similar code at
the later 153-154 area) so totals reflect only actually processed/accepted class
records.
---
Nitpick comments:
In `@integrations/bricks/editor-app/src/lib/apply.js`:
- Around line 140-145: The code defaults missing provenance to 'user' when
building ops (see ops.push and suggestedFrom), which makes absent provenance
appear non-authoritative and breaks auto-numbering; change the behavior to not
default suggestedFrom to 'user' — either omit the suggestedFrom property when
row.suggestedFrom is undefined/null or explicitly set it to undefined/null
(rather than 'user') so callers that omit provenance are treated as
authoritative and will trigger auto-numbering instead of duplicate 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: 70369ba1-2e66-4ce4-9b5c-5cf508dc8363
📒 Files selected for processing (16)
.gitignoredocs/rebemer.mdintegrations/bricks/assets/editor-app/app.cssintegrations/bricks/assets/editor-app/app.jsintegrations/bricks/editor-app/index.htmlintegrations/bricks/editor-app/src/components/BemBadge.svelteintegrations/bricks/editor-app/src/components/BemPanel.svelteintegrations/bricks/editor-app/src/components/Row.svelteintegrations/bricks/editor-app/src/lib/apply.jsintegrations/bricks/editor-app/src/lib/bricks-api.jsintegrations/bricks/editor-app/src/lib/element-types.jsintegrations/bricks/editor-app/src/lib/migrate-keys.jsintegrations/bricks/editor-app/src/main.jsintegrations/bricks/editor-app/src/styles/panel.cssintegrations/bricks/includes/class-rebemer-rest.phpintegrations/bricks/slashed-bricks.php
- apply.js: default suggestedFrom to 'fallback' instead of 'user' so callers that omit provenance trigger auto-numbering rather than false duplicate errors - class-rebemer-rest.php: use an explicit processed-classes counter so totalGlobalClasses reflects only valid entries, not malformed ones - index.html: update dev-harness copy from 'BEM' to 'reBEM' to match the rendered badge label
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
3 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 by CodeRabbit
Release Notes
New Features
Documentation
Style