Skip to content

Simplify configurator studio domain layout - #405

Closed
jackgranatowski wants to merge 1 commit into
mainfrom
codex/refactor-domainpanel-component-layout
Closed

Simplify configurator studio domain layout#405
jackgranatowski wants to merge 1 commit into
mainfrom
codex/refactor-domainpanel-component-layout

Conversation

@jackgranatowski

Copy link
Copy Markdown
Contributor

Motivation

  • Enforce a consistent three-part layout for Studio-backed domains: CategoryHeader → domain StudioAll variables.
  • Remove duplicated quick controls / presets / curated sections from the global panel and let domain-specific Studio components own those controls.
  • Preserve the legacy/settings fallback for domains that do not yet have a Studio (e.g. misc).

Description

  • Update configurator/src/components/DomainPanel.svelte to render CategoryHeader then the domain Studio when present, and only render legacy quick controls/presets/curated sections when the domain lacks a Studio (introduces hasLegacySettings).
  • Move high-impact controls into the Studio components by adding ScaleGenerator / QuickKnobs and knobs wiring inside the relevant editors: configurator/src/components/editors/ColorStudio.svelte, TypographyStudio.svelte, SpacingStudio.svelte, ShapeStudio.svelte, ShadowStudio.svelte, and MotionStudio.svelte.
  • Move style preset rows into the appropriate Studios (ShapeStudio and ShadowStudio) and remove now-unused global rendering of those sections; also remove an unused import in DomainPanel.svelte.
  • Keep the full token catalogue (All variables) available beneath the Studio or as an inline catalogue for domains without a curated Settings surface.

Testing

  • Ran npm --prefix configurator run check which completed with no errors and one pre-existing Svelte warning in ControlSection.svelte about a state reference.
  • Ran npm --prefix configurator run build (Vite production build) which succeeded; build emitted the same non-blocking warnings for ControlSection.svelte and chunk-size warnings.
  • Attempted to run Playwright browser install / screenshot for E2E verification but npx --prefix configurator playwright install chromium failed due to the Playwright CDN returning HTTP 403, so no screenshot was captured.

Codex Task

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Warning

Review limit reached

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

More reviews will be available in 29 minutes. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

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 credits.

🚦 How do rate 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 see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5db7dd13-a741-4c70-9380-a735daad3dd6

📥 Commits

Reviewing files that changed from the base of the PR and between f391ec2 and e7cf4ab.

📒 Files selected for processing (7)
  • configurator/src/components/DomainPanel.svelte
  • configurator/src/components/editors/ColorStudio.svelte
  • configurator/src/components/editors/MotionStudio.svelte
  • configurator/src/components/editors/ShadowStudio.svelte
  • configurator/src/components/editors/ShapeStudio.svelte
  • configurator/src/components/editors/SpacingStudio.svelte
  • configurator/src/components/editors/TypographyStudio.svelte
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/refactor-domainpanel-component-layout

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

Simplify configurator Studio domain panel layout
✨ Enhancement 🕐 40+ Minutes

Grey Divider

Description

• Enforce CategoryHeader → Studio → All variables layout for Studio-backed domains.
• Move scaling knobs, generators, and style presets from DomainPanel into domain Studios.
• Keep legacy settings/curated controls only for domains without a Studio component.
Diagram

graph TD
  A["DomainPanel.svelte"] --> B{"Studio exists?"}
  B -->|Yes| C["Domain Studio (e.g., ColorStudio)"] --> F["All variables catalogue"]
  B -->|No| D["Legacy settings sections"] --> F
  C --> E["In-Studio controls (knobs/generators/presets)"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Shared StudioLayout wrapper component
  • ➕ Centralizes the 'Header → Studio → All variables' scaffold in one place
  • ➕ Reduces repeated QuickKnobs/ScaleGenerator/preset placement patterns across Studios
  • ➕ Makes future layout tweaks less error-prone
  • ➖ Adds another abstraction layer around Studios, which may reduce editor flexibility
  • ➖ Still requires per-domain decisions about which controls belong in-Studio
2. Studio provides optional slots/exports for 'high-impact controls'
  • ➕ Keeps DomainPanel as the consistent shell while Studios only declare what they need
  • ➕ Avoids importing KNOBS_BY_DOMAIN/STYLE_PRESETS in every Studio file
  • ➖ Couples Studio implementations to DomainPanel API/slots
  • ➖ Harder to reason about when controls render if logic is split across components

Recommendation: The PR’s approach (Studios fully owning their controls, with a legacy fallback) is sound for long-term consistency and avoids duplicated global panel sections. If control wiring continues to expand across Studios, consider a lightweight shared wrapper (StudioLayout) to standardize repeated patterns (e.g., preset row + scaling section placement) without moving ownership back into DomainPanel.

Files changed (7) +116 / -78

Enhancement (6) +55 / -0
ColorStudio.svelteMove scaling knobs into Color Studio +6/-0

Move scaling knobs into Color Studio

• Adds QuickKnobs to ColorStudio and wires it to KNOBS_BY_DOMAIN.colors. This shifts scaling/tuning controls into the Studio so DomainPanel can remain a simple shell for Studio domains.

configurator/src/components/editors/ColorStudio.svelte

MotionStudio.svelteEmbed global multiplier knobs in Motion Studio +7/-0

Embed global multiplier knobs in Motion Studio

• Imports QuickKnobs and connects it to KNOBS_BY_DOMAIN.motion, rendering the scaling section inside the Studio. Keeps Motion’s workflow and controls while relocating the high-impact multipliers from the global panel.

configurator/src/components/editors/MotionStudio.svelte

ShadowStudio.svelteAdd presets and scaling knobs to Shadow Studio +14/-0

Add presets and scaling knobs to Shadow Studio

• Moves shadow style presets into ShadowStudio via StylePresetRow and STYLE_PRESETS_BY_DOMAIN.shadows. Also adds QuickKnobs wired to KNOBS_BY_DOMAIN.shadows so the Studio owns both presets and scaling controls.

configurator/src/components/editors/ShadowStudio.svelte

ShapeStudio.svelteAdd presets and scaling knobs to Shape Studio +14/-0

Add presets and scaling knobs to Shape Studio

• Moves border/radius style presets into ShapeStudio via StylePresetRow and STYLE_PRESETS_BY_DOMAIN.borders. Adds QuickKnobs wired to KNOBS_BY_DOMAIN.borders to keep scaling controls within the Studio surface.

configurator/src/components/editors/ShapeStudio.svelte

SpacingStudio.svelteRelocate spacing generator and knobs into Spacing Studio +7/-0

Relocate spacing generator and knobs into Spacing Studio

• Adds ScaleGenerator for the space scale and renders QuickKnobs wired to KNOBS_BY_DOMAIN.spacing directly inside SpacingStudio. This aligns spacing with the Studio-first layout by removing reliance on DomainPanel for generator/knob placement.

configurator/src/components/editors/SpacingStudio.svelte

TypographyStudio.svelteRelocate typography scale generator and knobs into Typography Studio +7/-0

Relocate typography scale generator and knobs into Typography Studio

• Adds ScaleGenerator for type/display and renders QuickKnobs wired to KNOBS_BY_DOMAIN.typography within TypographyStudio. Keeps the panel structure consistent by having Typography Studio own global text scaling controls.

configurator/src/components/editors/TypographyStudio.svelte

Refactor (1) +61 / -78
DomainPanel.svelteMake Studio-first layout with legacy-only settings fallback +61/-78

Make Studio-first layout with legacy-only settings fallback

• Reworks the panel to always render CategoryHeader → domain Studio (when present) → All variables. Introduces a legacy-only settings predicate (hasLegacySettings) so generators/knobs/presets/curated sections render only when no Studio exists, and tightens docs link rendering to legacy domains.

configurator/src/components/DomainPanel.svelte

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 5 rules

Grey Divider


Remediation recommended

1. Docs link suppressed 🐞 Bug ≡ Correctness
Description
DomainPanel.svelte now renders the framework docs footer link only when !usesVisualStudio, so
Studio-backed domains (e.g., Colors/Typography/Spacing) no longer show any docs link even though
they still define docsPath.
Code

configurator/src/components/DomainPanel.svelte[250]

+      {#if !usesVisualStudio && domain.docsPath}
Relevance

⭐⭐⭐ High

DomainPanel historically rendered docs link whenever domain.docsPath (PR #367); new
!usesVisualStudio gate likely unintended regression.

PR-#367

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The footer link is explicitly gated by !usesVisualStudio, and docsPath is intended to be linked
from the panel footer per the domain schema docs; multiple domains still define docsPath, so this
change removes that documented UI affordance for Studio domains.

configurator/src/components/DomainPanel.svelte[250-257]
configurator/src/lib/domains.js[103-116]

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

## Issue description
The per-domain docs footer link is currently gated by `!usesVisualStudio && domain.docsPath`, which hides documentation links for all Studio-backed domains.

## Issue Context
`docsPath` is documented as being “linked from the panel footer” in `lib/domains.js`, and Studio-backed domains still define `docsPath` values.

## Fix Focus Areas
- configurator/src/components/DomainPanel.svelte[250-257]
- configurator/src/lib/domains.js[103-116]

## Suggested fix
- Restore the footer condition to `{#if domain.docsPath}` so Studio and non-Studio domains both show the docs link; or
- If the strict 3-part layout must remain, move the link into `CategoryHeader` or each Studio component so Studio domains still expose the docs link somewhere consistent.

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


Grey Divider

Qodo Logo

@qodo-code-review

Copy link
Copy Markdown

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: Configurator tests

Failed stage: Run npm run test:e2e [❌]

Failed test name: display tab: ratio selects are read-only (shared with type)

Failure summary:

The GitHub Action failed because a Playwright E2E test assertion did not match the UI state.
-
Failed test: tests-e2e/generator.spec.js:27:1 (display tab: ratio selects are read-only (shared with
type)) on chromium.
- Failure details: the test expected display.locator('select[disabled]') to have
count 2, but it found 0 elements within the 5000ms timeout (toHaveCount(2) failed).
- Location:
tests-e2e/generator.spec.js:31:53 (await
expect(display.locator('select[disabled]')).toHaveCount(2);).
- The test retried once and failed
again with the same result, causing the run to exit with code 1.

Relevant error logs:
1:  ##[group]Runner Image Provisioner
2:  Hosted Compute Agent
...

1091:  type: 'test'
1092:  ...
1093:  1..3
1094:  ok 28 - bestContrastVsBW
1095:  ---
1096:  duration_ms: 1.111885
1097:  type: 'suite'
1098:  ...
1099:  # Subtest: wcagLevel
1100:  # Subtest: classifies known thresholds
1101:  ok 1 - classifies known thresholds
1102:  ---
1103:  duration_ms: 0.394553
1104:  type: 'test'
1105:  ...
1106:  # Subtest: non-finite ratios fail
1107:  ok 2 - non-finite ratios fail
1108:  ---
...

3960:  ok 7 - storage key is versioned
3961:  ---
3962:  duration_ms: 0.324742
3963:  type: 'test'
3964:  ...
3965:  1..7
3966:  ok 88 - sanitiseUiState
3967:  ---
3968:  duration_ms: 5.93237
3969:  type: 'suite'
3970:  ...
3971:  1..88
3972:  # tests 525
3973:  # suites 88
3974:  # pass 525
3975:  # fail 0
3976:  # cancelled 0
...

4013:  ##[group]Run npm run check
4014:  �[36;1mnpm run check�[0m
4015:  shell: /usr/bin/bash -e {0}
4016:  ##[endgroup]
4017:  > slashed-configurator@0.6.15 check
4018:  > svelte-check --tsconfig ./jsconfig.json
4019:  Loading svelte-check in workspace: /home/runner/work/SLASHED/SLASHED/configurator
4020:  Getting Svelte diagnostics...
4021:  /home/runner/work/SLASHED/SLASHED/configurator/�[32msrc/components/ControlSection.svelte�[39m:3:48
4022:  �[33mWarn�[39m: This reference only captures the initial value of `defaultOpen`. Did you mean to reference it inside a derived instead?
4023:  https://svelte.dev/e/state_referenced_locally (svelte)
4024:  �[36m  let { title, hint = '', modifiedCount = 0, defaultOpen = true, children } = $props();
4025:  let open = $state(window.innerWidth >= 768 ? �[35mdefaultOpen�[36m : false);
4026:  </script>�[39m
4027:  ====================================
4028:  �[33msvelte-check found 0 errors and 1 warning in 1 file
4029:  �[39m
...

5314:  �[1A�[2K[3/101] [chromium] › tests-e2e/a11y.spec.js:46:1 › every button exposes an accessible name
5315:  �[1A�[2K[4/101] [chromium] › tests-e2e/a11y.spec.js:59:1 › segmented controls expose aria-pressed; info buttons aria-expanded
5316:  �[1A�[2K[5/101] [chromium] › tests-e2e/a11y.spec.js:75:1 › search filters the catalogue and shows an empty state for no match
5317:  �[1A�[2K[6/101] [chromium] › tests-e2e/axe.spec.js:32:3 › no structural a11y violations on the main routes (dark)
5318:  �[1A�[2K[7/101] [chromium] › tests-e2e/axe.spec.js:32:3 › no structural a11y violations on the main routes (light)
5319:  �[1A�[2K[8/101] [chromium] › tests-e2e/dogfood.spec.js:26:1 › chrome aliases are sourced from framework tokens
5320:  �[1A�[2K[9/101] [chromium] › tests-e2e/dogfood.spec.js:39:1 › overrides restyle the preview stage but never the chrome
5321:  �[1A�[2K[10/101] [chromium] › tests-e2e/dogfood.spec.js:57:1 › chrome theme toggle flips the framework [data-theme] and repaints
5322:  �[1A�[2K[11/101] [chromium] › tests-e2e/domain-preview.spec.js:32:1 › preview hub follows every sidebar domain, including tool domains
5323:  �[1A�[2K[12/101] [chromium] › tests-e2e/domain-preview.spec.js:44:1 › Layout panel renders proportional ContainerBars instead of the old inline preview card
5324:  �[1A�[2K[13/101] [chromium] › tests-e2e/domain-preview.spec.js:62:1 › the preview hub reflects a live override
5325:  �[1A�[2K[14/101] [chromium] › tests-e2e/generator.spec.js:14:1 › type generator writes engine scalars only
5326:  �[1A�[2K[15/101] [chromium] › tests-e2e/generator.spec.js:27:1 › display tab: ratio selects are read-only (shared with type)
5327:  �[1A�[2K[16/101] (retries) [chromium] › tests-e2e/generator.spec.js:27:1 › display tab: ratio selects are read-only (shared with type) (retry #1)
5328:  �[1A�[2K  1) [chromium] › tests-e2e/generator.spec.js:27:1 › display tab: ratio selects are read-only (shared with type) 
5329:  Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoHaveCount�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed
5330:  Locator:  locator('.gen').nth(1).locator('select[disabled]')
...

5332:  Received: �[31m0�[39m
5333:  Timeout:  5000ms
5334:  Call log:
5335:  �[2m  - Expect "toHaveCount" with timeout 5000ms�[22m
5336:  �[2m  - waiting for locator('.gen').nth(1).locator('select[disabled]')�[22m
5337:  �[2m    14 × locator resolved to 0 elements�[22m
5338:  �[2m       - unexpected value "0"�[22m
5339:  29 |   await sideItem(page, 'Typography').click();
5340:  30 |   const display = page.locator('.gen').nth(1);
5341:  > 31 |   await expect(display.locator('select[disabled]')).toHaveCount(2);
5342:  |                                                     ^
5343:  32 |   await expect(display.locator('.gen__hint').first()).toContainText('reuses the type ratios');
5344:  33 | });
5345:  34 |
5346:  at /home/runner/work/SLASHED/SLASHED/configurator/tests-e2e/generator.spec.js:31:53
5347:  Error Context: test-results/generator-display-tab-rati-18399-read-only-shared-with-type--chromium/error-context.md
5348:  Retry #1 ───────────────────────────────────────────────────────────────────────────────────────
5349:  Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoHaveCount�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed
5350:  Locator:  locator('.gen').nth(1).locator('select[disabled]')
...

5352:  Received: �[31m0�[39m
5353:  Timeout:  5000ms
5354:  Call log:
5355:  �[2m  - Expect "toHaveCount" with timeout 5000ms�[22m
5356:  �[2m  - waiting for locator('.gen').nth(1).locator('select[disabled]')�[22m
5357:  �[2m    14 × locator resolved to 0 elements�[22m
5358:  �[2m       - unexpected value "0"�[22m
5359:  29 |   await sideItem(page, 'Typography').click();
5360:  30 |   const display = page.locator('.gen').nth(1);
5361:  > 31 |   await expect(display.locator('select[disabled]')).toHaveCount(2);
5362:  |                                                     ^
5363:  32 |   await expect(display.locator('.gen__hint').first()).toContainText('reuses the type ratios');
5364:  33 | });
5365:  34 |
5366:  at /home/runner/work/SLASHED/SLASHED/configurator/tests-e2e/generator.spec.js:31:53
5367:  Error Context: test-results/generator-display-tab-rati-18399-read-only-shared-with-type--chromium-retry1/error-context.md
5368:  �[1A�[2K[17/101] [chromium] › tests-e2e/generator.spec.js:35:1 › viewport range is shared: space sets it, type seeds it, reset clears it
...

5439:  �[1A�[2K[88/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Borders @ 1280px
5440:  �[1A�[2K[89/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Borders @ 768px
5441:  �[1A�[2K[90/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Borders @ 390px
5442:  �[1A�[2K[91/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Shadows @ 1280px
5443:  �[1A�[2K[92/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Shadows @ 768px
5444:  �[1A�[2K[93/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Shadows @ 390px
5445:  �[1A�[2K[94/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Motion @ 1280px
5446:  �[1A�[2K[95/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Motion @ 768px
5447:  �[1A�[2K[96/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Motion @ 390px
5448:  �[1A�[2K[97/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Effects @ 1280px
5449:  �[1A�[2K[98/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Effects @ 768px
5450:  �[1A�[2K[99/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Effects @ 390px
5451:  �[1A�[2K[100/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Misc @ 1280px
5452:  �[1A�[2K[101/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Misc @ 768px
5453:  �[1A�[2K[102/101] [screenshots] › tests-e2e/screenshots.spec.js:20:7 › Studio screenshot QA › Misc @ 390px
5454:  �[1A�[2K  1 failed
5455:  [chromium] › tests-e2e/generator.spec.js:27:1 › display tab: ratio selects are read-only (shared with type) 
5456:  2 skipped
5457:  98 passed (2.5m)
5458:  ##[error]Process completed with exit code 1.
5459:  Post job cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant