feat(configurator): unified value editor + grouped undo (UX redesign, phase 5) - #695
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (11)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThe PR adds a unified Inherit/Value/Expression token editor, time-based grouped undo, a Changes overview, metadata-driven domain classification, and richer token relationship diagnostics.
Confidence Score: 3/5The PR should not merge until expression mode resynchronizes after external updates and grouped undo preserves separate same-token actions. Persistent manual editor state can defeat the expression-preservation guarantee, while time-only history grouping makes independently committed states unavailable to undo. Files Needing Attention: configurator/src/components/inputs/ValueField.svelte, configurator/src/App.svelte, configurator/src/lib/history.ts, configurator/src/components/panels/ChangesPanel.svelte
|
| Filename | Overview |
|---|---|
| configurator/src/components/inputs/ValueField.svelte | Adds the unified editor, but manual mode persists across external override changes and can misrepresent restored expressions. |
| configurator/src/App.svelte | Integrates grouped undo, but the time/key heuristic merges separate rapid actions on the same token. |
| configurator/src/lib/history.ts | Implements pure coalescing helpers without a gesture or commit-boundary concept. |
| configurator/src/lib/tokenModel.ts | Adds role, dependency, validation-state, change-summary, and scale-shadow modeling with broad unit coverage. |
| configurator/src/lib/domains.ts | Replaces overlapping substring classification with deterministic namespace mapping and exceptions. |
| configurator/src/components/panels/ChangesPanel.svelte | Adds grouped override review and remediation controls, but its invalid-export diagnostic does not match export behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
UI[Token controls] --> Set[setOverrides]
Set --> History{Same key within 600 ms?}
History -->|Yes| Merge[Reuse history entry]
History -->|No| Push[Push previous snapshot]
Set --> Overrides[Active overrides]
Overrides --> Editor[ValueField mode detection]
Overrides --> Changes[Changes classification]
Overrides --> Preview[Live preview and export]
Reviews (1): Last reviewed commit: "feat(configurator): unified value editor..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let manualMode = $state<ValueMode | null>(null); | ||
| let mode = $derived<ValueMode>(manualMode ?? detectMode(overrideValue)); |
There was a problem hiding this comment.
When a user selects a manual mode and the token's override later changes through undo, redo, theme application, import, or another editor surface, manualMode continues to override detectMode. A restored var() or calc() expression therefore remains displayed as Value and can be overwritten without the deliberate mode switch promised by this editor.
| const keys = changedKeys(prev, next); | ||
| const now = Date.now(); | ||
| // Only push a new undo snapshot when this isn't a continuation of the | ||
| // current single-token gesture. | ||
| if (!shouldCoalesce(coalesce, keys, now)) { | ||
| past = [...past.slice(-49), prev]; | ||
| } | ||
| coalesce = { key: keys.length === 1 ? keys[0] : null, time: now }; |
There was a problem hiding this comment.
When two independent edits affect the same token within 600 ms, such as committing a value and immediately selecting Inherit, this code treats the second action as part of the first gesture. No snapshot is recorded for the intermediate state, so Ctrl+Z skips an independently committed edit instead of undoing each action separately.
| accent: string; | ||
| }; | ||
| const GROUPS: GroupDef[] = [ | ||
| { key: "invalid", label: "Invalid", blurb: "Can't be applied safely — will be dropped on export.", icon: AlertTriangle, accent: "text-rose-600 dark:text-rose-400" }, |
There was a problem hiding this comment.
The export path sanitizes structurally unsafe values and still emits their declarations, rather than dropping them. This message therefore gives users an incorrect account of the CSS that export will produce.
| { key: "invalid", label: "Invalid", blurb: "Can't be applied safely — will be dropped on export.", icon: AlertTriangle, accent: "text-rose-600 dark:text-rose-400" }, | |
| { key: "invalid", label: "Invalid", blurb: "Contains CSS-breaking characters — will be sanitized on export.", icon: AlertTriangle, accent: "text-rose-600 dark:text-rose-400" }, |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
30f6e19 to
68e3687
Compare
…tion/Quality/Project) Replace the flat, unlabelled icon rail with a grouped, always-labelled navigation so users reason about areas of the design system instead of decoding icons. The rail is now organised into four named groups: - Foundations: Colors, Typography, Spacing, Shape, Motion - Composition: Layout, Shadows, Effects, Macros, Components, Misc - Quality: Changes, Accessibility - Project: Presets, Install & export, Reference Renames per the plan: Borders→Shape (it already holds radius), WCAG→ Accessibility, Themes→Presets, Install→"Install & export", Classes→Reference. Labels update in App's panel-heading map and HomePanel too, so the sidebar, the panel header and the Home overview all tell the same story (same order, names, descriptions and counts). Responsive: desktop shows the labelled column (w-52) with group headers and a trailing count pill; mobile keeps the compact icon rail (w-14) with corner-dot counts and tooltips — no change to the mobile fold behaviour. Scope: this is the information-architecture + naming half of the redesign's phase 4. Domain/panel ids, token classification and preview mapping are unchanged, so it's non-disruptive. Content-level panel merges (a single Depth panel combining Shadows+Effects, the Misc split, and moving focus-ring/touch into an expanded Accessibility panel) are deliberately deferred to their own focused follow-ups. Updated tests-e2e/shell.spec.js NAV_LABELS to the new names. check, lint, all 257 unit tests and the shell e2e suite pass; desktop + mobile screenshot-verified.
… grouped undo Two universal control fixes from the audit. 1) Grouped undo (src/lib/history.ts). A slider drag fires `input` on every tick, so a single gesture used to push dozens of near-identical snapshots and one Ctrl+Z rewound a single pixel. setOverrides now coalesces a run of consecutive edits to the *same* token within a short window into one history entry; a different edit or a pause starts a fresh group; undo/redo end the open group. Pure helpers (changedKeys, shouldCoalesce) are unit-tested. 2) Unified value editor (ValueField.svelte + src/lib/valueField.ts). An explicit three-mode control — Inherit / Value / Expression — replaces TokenRow's ad-hoc scale-picker/expand/button editor. An expression override (var()/ calc()/clamp()/…) always resolves to Expression mode and is shown verbatim, never parsed down to a fallback number, so the next edit can't silently overwrite it; switching to a fixed Value is a deliberate tab click. Inherit shows the framework default and offers a "relink to a scale step" dropdown (folding in the old sibling-scale picker). detectMode/isExpression/ splitValueUnit are unit-tested. TokenRow keeps all its phase-1 context (role badge, inherits, used-by, Detached/ Invalid warnings) and now edits through ValueField — so All tokens and the Changes panel both gain the unified editor. check + lint clean; 272 unit tests (history 8, valueField 7) and the shell e2e suite pass; screenshot-verified that a calc() override renders in Expression mode verbatim. Scope note: this lands the unified generic editor and grouped undo. Migrating the specialised SliderRow-based panels onto the same three-mode model, and the shared scale control with detach diagnostics for radius/border/motion, are tracked as follow-ups (the detach diagnostics already exist in the Changes panel and tokenModel.scaleShadows).
4230a15 to
27cc308
Compare
2 participants
Phase 5 of the configurator UX redesign — unified controls
Two universal control fixes the audit called out.
1. Grouped undo
A slider drag fires
inputon every tick, so a single gesture used to push dozens of near-identical snapshots — one Ctrl+Z rewound a single pixel.setOverridesnow coalesces a run of consecutive edits to the same token within a short window into one history entry; a different edit or a pause starts a fresh group; undo/redo close the open group. The decision logic (changedKeys,shouldCoalesce) lives insrc/lib/history.tsand is unit-tested (8 tests).2. Unified value editor — Inherit / Value / Expression
ValueField.sveltereplacesTokenRow's ad-hoc scale-picker / expand / button editor with three explicit modes:var()/calc()/clamp()… value shown verbatim.The key guarantee: an expression override always resolves to Expression mode and is never parsed down to a fallback number, so the next edit can't silently overwrite it — turning it into a fixed value is a deliberate tab click.
detectMode/isExpression/splitValueUnitare unit-tested (7 tests).TokenRowkeeps all its phase-1 context (role badge,inherits,used by N, Detached/Invalid warnings) and now edits throughValueField, so All tokens and the Changes panel both gain the unified editor.Verification
npm run check: 0 errors ·npm run lint: clean · build OKnpm run test: 272/272 (history 8, valueField 7)calc(var(--sf-radius-s) * 2)override renders in Expression mode verbatim with the mode tabs.Scope / follow-ups
Lands the unified generic editor + grouped undo. Migrating the specialised
SliderRow-based panels onto the same three-mode model, and the shared scale control with inline detach diagnostics for radius/border/motion, are follow-ups — the detach detection itself already ships (Changes panel +tokenModel.scaleShadows).