Skip to content

fix: correct category-reset domain scoping regression from #127 - #128

Merged
jackgranatowski merged 3 commits into
mainfrom
claude/configurator-reset-ui-fixes-wt1jqs
Jul 1, 2026
Merged

jackgranatowski merged 3 commits into
mainfrom
claude/configurator-reset-ui-fixes-wt1jqs

Conversation

@jackgranatowski

Copy link
Copy Markdown
Contributor

Summary

  • Fixes a bug shipped in fix: stop frontend overlay covering admin bar dropdowns; sync reset UI fixes #127 (already merged): the per-category "Reset N" button scoped its overrides by raw substring matching against the active domain's own pattern list (domainPatterns.some(p => k.includes(p))), which over-matches because DOMAIN_PATTERNS substrings overlap across domains — e.g. layout's "-bg-" pattern also matches color tokens like --sf-color-bg--active. Resetting the Layout category could silently delete unrelated Colors overrides.
  • Fixed upstream in codeslash-dev/SLASHED#468 (caught by automated review there) by scoping via domainOf(k) === domain instead — the same classifier already used for the sidebar's per-domain override badges, so the reset button only ever touches keys that actually belong to the active category.
  • Re-synced admin-app/src/App.svelte from the framework and rebuilt assets/admin-app/app.js to match.

Type

  • fix

Checklist

  • Conventional Commit messages (feat:, fix:, docs:, …)
  • npm test — not run in this session (npm run verify was run and passes)
  • npm run verify passes (version metadata in sync)
  • Generated artifacts not hand-edited (assets/admin-app/app.js regenerated via npm run build:admin-app)
  • CHANGELOG.md updated under ## [Unreleased]
  • Built SPA assets committed (assets/admin-app/app.js)

Notes

Only admin-app/src/App.svelte changed on the source side (vendored from the framework, not hand-edited here) — no plugin-specific files needed changes for this fix.


Generated by Claude Code

Pulls in codeslash-dev/SLASHED#468 (205f9ab): the per-category reset
button merged in #127 scoped its overrides by raw substring matching
against the active domain's own pattern list, which over-matches since
DOMAIN_PATTERNS substrings overlap across domains (e.g. layout's
"-bg-" also appears in color tokens like --sf-color-bg--active).
Resetting Layout could silently wipe out unrelated Colors overrides.
Now scoped via domainOf(k) === domain, the same classifier already
used for the sidebar override badges. Rebuilt assets/admin-app/app.js
to match.
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@jackgranatowski, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bf6d10c7-9933-4828-8e01-abae4ffb93a2

📥 Commits

Reviewing files that changed from the base of the PR and between 1af7002 and 1414bf4.

📒 Files selected for processing (4)
  • SLASHED-for-WP/admin-app/.vendored-manifest.json
  • SLASHED-for-WP/admin-app/src/App.svelte
  • SLASHED-for-WP/admin-app/src/components/DomainPanel.svelte
  • SLASHED-for-WP/assets/admin-app/app.js
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/configurator-reset-ui-fixes-wt1jqs

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

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

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix category reset to use domainOf() scoping (avoid cross-domain override deletes)

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Fix per-category Reset button to only clear overrides classified in the active domain via
 domainOf(k) === domain.
• Prevent accidental deletion of unrelated overrides caused by overlapping DOMAIN_PATTERNS
 substrings (e.g. layout's "-bg-" matching color tokens).
• Re-sync vendored App.svelte from upstream framework fix and rebuild committed SPA bundle.
Diagram

graph TD
    A["User: Reset N button"] --> B["handleResetDomain()\nApp.svelte"]
    B --> C["domainOf(tokenKey)\nlib/domains.ts"]
    C --> D{"domain matches\nactive category?"}
    D -- yes --> E["Add key to patch"]
    D -- no --> F["Skip key"]
    E --> G["handleBulkChange(patch)"]
    G --> H["Overrides state\nupdated safely"]

    subgraph Legend
      direction LR
      _file["App.svelte"] ~~~ _mod(["lib/domains.ts"]) ~~~ _state{{"Overrides state"}}
    end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Make DOMAIN_PATTERNS non-overlapping
  • ➕ Would make substring matching safe everywhere, not just for reset
  • ➖ Hard to guarantee long-term as the token set grows
  • ➖ Requires coordinated data changes in the framework; much larger scope than needed
2. Tag tokens with domain at generation time (API index)
  • ➕ O(1) lookup with no substring ambiguity
  • ➕ Removes all reliance on pattern ordering
  • ➖ Requires changes to the token generation pipeline and API index schema
  • ➖ Backwards-incompatible; far more invasive than this regression warrants

Recommendation: Using domainOf(k) === domain is the correct fix: it reuses the same classifier already trusted for sidebar badge counts, so reset and badge behavior are guaranteed to agree. The alternatives below were considered but are more invasive for what is a targeted regression fix.

Files changed (3) +10 / -9

Bug fix (1) +8 / -7
App.svelteScope category reset via domainOf() instead of DOMAIN_PATTERNS substring matching +8/-7

Scope category reset via domainOf() instead of DOMAIN_PATTERNS substring matching

• Removes the DOMAIN_PATTERNS import and the derived domainPatterns reactive variable. Both domainOverridesCount and handleResetDomain() now use domainOf(k) === domain to classify keys, matching the same logic used by the sidebar's per-domain override badges. This prevents cross-domain override deletion when pattern substrings overlap across categories.

SLASHED-for-WP/admin-app/src/App.svelte

Other (2) +2 / -2
.vendored-manifest.jsonBump vendored sync timestamp after re-sync +1/-1

Bump vendored sync timestamp after re-sync

• Updates the syncedAt timestamp in the vendored manifest to record when the latest framework re-sync occurred. No functional change.

SLASHED-for-WP/admin-app/.vendored-manifest.json

app.jsRegenerate compiled SPA bundle with reset scoping fix +1/-1

Regenerate compiled SPA bundle with reset scoping fix

• Rebuilt and committed the minified admin app bundle to include the updated domain-scoping logic from App.svelte. This is a generated artifact and was not hand-edited.

SLASHED-for-WP/assets/admin-app/app.js

@qodo-code-review

qodo-code-review Bot commented Jul 1, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 1 rule

Grey Divider


Informational

1. Vendored App.svelte modified 📘 Rule violation § Compliance
Description
SLASHED-for-WP/admin-app/src/App.svelte is listed in .vendored-manifest.json but is modified in
this PR. Per policy, vendored files must not be changed in-repo; changes should be made upstream and
re-vendored via the approved mechanism.
Code

SLASHED-for-WP/admin-app/src/App.svelte[R155-159]

  function handleResetDomain() {
    const patch: Record<string, null> = {};
    for (const k of Object.keys(overrides)) {
-      if (domainPatterns.some((p) => k.includes(p))) patch[k] = null;
+      if (domainOf(k) === domain) patch[k] = null;
    }
Relevance

⭐ Low

Team's approved mechanism IS re-syncing vendored files (PRs #124, #125, #127 all do this). PR
description explicitly states this is a re-sync from upstream, not a hand-edit.

PR-#127
PR-#125
PR-#124

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The manifest lists src/App.svelte as a vendored file, and the PR modifies that same file (e.g.,
handleResetDomain() now uses domainOf(k) === domain). This matches the rule's failure criteria:
a path present in .vendored-manifest.json appears as changed in the PR.

Rule 1514148: Do not modify vendored files listed in .vendored-manifest.json
SLASHED-for-WP/admin-app/.vendored-manifest.json[14-17]
SLASHED-for-WP/admin-app/src/App.svelte[155-159]

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

## Issue description
A file marked as vendored (`SLASHED-for-WP/admin-app/src/App.svelte`) was modified in this PR, which violates the rule prohibiting changes to files listed in `.vendored-manifest.json`.

## Issue Context
`.vendored-manifest.json` explicitly lists `src/App.svelte` as a vendored file. The PR includes functional edits in that file (e.g., reset scoping logic), so the change set includes modifications to a vendored path.

## Fix Focus Areas
- SLASHED-for-WP/admin-app/src/App.svelte[155-159]
- SLASHED-for-WP/admin-app/.vendored-manifest.json[14-17]

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


2. Reset count vs badge count mismatch ✓ Resolved 🐞 Bug ≡ Correctness
Description
App.svelte's domainOverridesCount (used for the 'Reset N' button) now uses `domainOf(k) ===
domain, while DomainPanel.svelte's domainOverridesInTokenTab` badge still uses
DOMAIN_PATTERNS[domain] substring matching. For tokens where patterns overlap across domains (e.g.
--sf-color-bg--active matches layout's -bg- pattern but domainOf() resolves it to colors),
the two counts diverge, showing different numbers for what the user expects to be the same scope.
Code

SLASHED-for-WP/admin-app/src/App.svelte[R61-68]

+  // Scope the active category's reset to exactly the keys domainOf() would
+  // badge under this domain — matching against the domain's own pattern list
+  // directly would over-match, since patterns overlap across domains (e.g.
+  // layout's "-bg-" also appears in color tokens like --sf-color-bg--active,
+  // which domainOf() resolves to "colors" by checking that domain first).
  let domainOverridesCount = $derived(
-    Object.keys(overrides).filter((k) => domainPatterns.some((p) => k.includes(p))).length
+    Object.keys(overrides).filter((k) => domainOf(k) === domain).length
  );
Relevance

⭐ Low

No historical evidence of this mismatch being flagged. Team re-syncs vendored files from upstream;
DomainPanel fix would come via next upstream sync.

PR-#127
PR-#125

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
App.svelte lines 66-68 now filter overrides with domainOf(k) === domain, while DomainPanel.svelte
lines 47-53 still use DOMAIN_PATTERNS[domain] ?? [domain] substring matching for its
domainOverridesInTokenTab badge. The token --sf-color-bg--active (confirmed in
api-index.generated.json) contains -bg-, which is in layout's DOMAIN_PATTERNS entry, so
DomainPanel would count it under layout. But domainOf('--sf-color-bg--active') returns colors
(colors domain is checked first in domains.ts and matches color), so App.svelte would not count it
under layout. This is exactly the overlap scenario the PR description calls out, but the fix was
only applied to App.svelte and not to DomainPanel.svelte.

SLASHED-for-WP/admin-app/src/components/DomainPanel.svelte[47-53]
SLASHED-for-WP/admin-app/src/lib/domains.ts[7-12]
SLASHED-for-WP/admin-app/src/data/domain-patterns.json[1-20]

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

## Issue description
After this PR, `App.svelte` counts domain overrides using `domainOf(k) === domain` (exact domain resolution), but `DomainPanel.svelte` still counts them using `DOMAIN_PATTERNS[domain]` substring matching. For tokens where patterns overlap (e.g. `--sf-color-bg--active` matches layout's `-bg-` pattern but `domainOf()` resolves it to `colors`), the 'Reset N' button count and the 'All tokens' tab badge count will show different numbers.

## Issue Context
The PR fixed the reset button and the header count in `App.svelte` to use `domainOf(k) === domain`, but did not update the equivalent count in `DomainPanel.svelte`.

## Fix Focus Areas
- SLASHED-for-WP/admin-app/src/components/DomainPanel.svelte[47-53]

Replace the `domainOverridesInTokenTab` derived value to use `domainOf(t.name) === domain` instead of `patterns.some((p) => t.name.includes(p))`. Import `domainOf` from `'../lib/domains'` if not already imported. The `patterns` variable can be kept for the `AllTokensTab` component which still receives it as a prop for its own filtering.

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


Grey Divider

Qodo Logo

jackgranatowski pushed a commit to codeslash-dev/SLASHED that referenced this pull request Jul 1, 2026
domainOverridesInTokenTab still filtered by raw DOMAIN_PATTERNS
substring matching after 205f9ab switched the "Reset N" count to
domainOf(k) === domain, so the two could disagree for overlapping
tokens (e.g. --sf-color-bg--active matches layout's "-bg-" pattern but
domainOf() resolves it to colors). Use the same domainOf() predicate
here so both counts always agree.

(caught by automated review on codeslash-dev/SLASHED-Plugins#128)
Pulls in codeslash-dev/SLASHED#468 (ccdcbe0): DomainPanel.svelte's
domainOverridesInTokenTab badge still filtered by raw DOMAIN_PATTERNS
substring matching after the "Reset N" count switched to
domainOf(k) === domain, so the two counts could disagree for
overlapping tokens (e.g. --sf-color-bg--active). Now both use the same
domainOf() predicate. Rebuilt assets/admin-app/app.js to match.

(caught by automated review on #128)

Copy link
Copy Markdown
Contributor Author

Re Qodo's two findings:

  1. Vendored App.svelte modified — as noted in this finding itself, this is the approved re-sync mechanism (see fix: re-sync mobile domain panel overflow fix from framework #124, fix: re-sync single-row mobile preview toolbar from framework #125, fix: stop frontend overlay covering admin bar dropdowns; sync reset UI fixes #127), not a hand-edit. No action needed.
  2. Reset count vs badge count mismatch — confirmed, real bug: DomainPanel.svelte's domainOverridesInTokenTab still filtered by raw DOMAIN_PATTERNS substring matching after App.svelte switched to domainOf(k) === domain. Fixed upstream in codeslash-dev/SLASHED#468 (commit ccdcbe0) and re-synced here in 0430255 — both counts now use domainOf().

Generated by Claude Code

…ework

Pulls in codeslash-dev/SLASHED#468 (081e9d8): domainOverridesCount and
handleResetDomain each recomputed the same domainOf() filter
independently; extracted the shared domainOverrideKeys derived so both
stay in sync. Rebuilt assets/admin-app/app.js to match.

(nitpick from automated review on #128)
@jackgranatowski
jackgranatowski merged commit d0f9cad into main Jul 1, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants