fix(configurator): add per-category reset button, fix invisible reset icons - #468
Conversation
… icons Wire up the panel header with a "Reset N" button that clears just the active category's overrides (the matching logic already existed unused in DomainPanel.svelte). Also fix per-token reset controls that relied on bare `opacity-0 group-hover:opacity-100`, which never becomes visible on touch devices or via keyboard focus — apply the same opacity-100 below `sm:`, group-focus-within/focus fallback already used correctly in ThemesPanel.svelte across TokenRow, SliderRow, PowerKnobRow, and the inline reset buttons in MiscPanel/TypographyPanel/MotionPanel.
PR Summary by QodoConfigurator: per-category reset button + accessible reset controls
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
|
Warning Review limit reached
Next review available in: 15 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 (1)
📝 WalkthroughWalkthroughDomain-scoped override reset logic now lives in ChangesDomain Reset Relocation
Reset Button Visibility Styling
Estimated code review effort: 2 (Simple) | ~12 minutes 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 |
Code Review by Qodo
Context used✅ Compliance rules (platform):
6 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…n overlap domainPatterns.some(p => k.includes(p)) matched against the active domain's own pattern list in isolation, but DOMAIN_PATTERNS substrings overlap across domains (e.g. layout's "-bg-" also appears in color tokens like --sf-color-bg--active). Resetting Layout could therefore wipe out unrelated Colors overrides. Use domainOf(k) === domain instead, the same classifier already used for the sidebar override badges, so the reset button only ever touches keys that actually belong to the active category. (caught by automated review on #468)
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.
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)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
configurator/src/App.svelte (1)
61-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: dedupe the domain-key filter.
domainOverridesCountandhandleResetDomainboth recomputeObject.keys(overrides).filter(k => domainOf(k) === domain)independently. Extracting a single derived array of domain-scoped keys would avoid the duplicated predicate and keep both consumers guaranteed in sync as the logic evolves.♻️ Suggested consolidation
- let domainOverridesCount = $derived( - Object.keys(overrides).filter((k) => domainOf(k) === domain).length - ); + let domainOverrideKeys = $derived( + Object.keys(overrides).filter((k) => domainOf(k) === domain) + ); + let domainOverridesCount = $derived(domainOverrideKeys.length);function handleResetDomain() { const patch: Record<string, null> = {}; - for (const k of Object.keys(overrides)) { - if (domainOf(k) === domain) patch[k] = null; - } + for (const k of domainOverrideKeys) patch[k] = null; handleBulkChange(patch); }Also applies to: 155-162
🤖 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 `@configurator/src/App.svelte` around lines 61 - 68, The domain-scoped override filtering is duplicated between domainOverridesCount and handleResetDomain, so extract the shared Object.keys(overrides).filter(...) logic into one derived list or helper and reuse it in both places. Update App.svelte so the active-domain key selection is defined once and both the count and reset behavior consume the same source of truth, keeping domainOf() and domain in sync.
🤖 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.
Nitpick comments:
In `@configurator/src/App.svelte`:
- Around line 61-68: The domain-scoped override filtering is duplicated between
domainOverridesCount and handleResetDomain, so extract the shared
Object.keys(overrides).filter(...) logic into one derived list or helper and
reuse it in both places. Update App.svelte so the active-domain key selection is
defined once and both the count and reset behavior consume the same source of
truth, keeping domainOf() and domain in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6942551-5de0-42d9-a16b-41ba8e246ffd
📒 Files selected for processing (2)
configurator/src/App.svelteconfigurator/src/components/DomainPanel.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- configurator/src/components/DomainPanel.svelte
domainOverridesCount and handleResetDomain each recomputed Object.keys(overrides).filter(k => domainOf(k) === domain) independently. Extract the shared domainOverrideKeys derived so both consumers stay guaranteed in sync as the logic evolves. (nitpick from automated review on #468)
…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)
|
Applied CodeRabbit's dedup suggestion in 081e9d8: extracted a shared Generated by Claude Code |
2 participants
Wire up the panel header with a "Reset N" button that clears just the
active category's overrides (the matching logic already existed unused
in DomainPanel.svelte). Also fix per-token reset controls that relied on
bare
opacity-0 group-hover:opacity-100, which never becomes visible ontouch devices or via keyboard focus — apply the same opacity-100 below
sm:, group-focus-within/focus fallback already used correctly inThemesPanel.svelte across TokenRow, SliderRow, PowerKnobRow, and the
inline reset buttons in MiscPanel/TypographyPanel/MotionPanel.
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements