chore: technical debt audit remediation — batch 1 (quick wins) - #474
Merged
Merged
Conversation
Rename cryptic 1-2 letter symbols in the persistence/encode/CSS-export module (Wa, Ra, za, Ha, fa, Ga, qa...) to their existing readable export-alias names, and update the four call sites that imported the obfuscated names directly (persistence.ts, App.svelte, PreviewPanel.svelte, ExportPanel.svelte). No behavior change — public export surface (encode/decode/generateCSS/etc.) is unchanged, verified by tsc, svelte-check, and the full codec/css/share unit test suite.
…g (SL-018) Add an 'error' state to the saveState union so a failed REST/network save is distinguishable from "never attempted" — previously handleSave's catch block reset straight back to 'idle'. StudioHeader now renders a red "Save failed" state with a retry-hint tooltip; a further edit clears it back to 'idle' the same way 'saved' already did. Verified live: booted the configurator against a window.slashedApp REST target that 404s, confirmed the button renders "Save failed" after a failed save and clears to "Save" on the next edit. Also tsc, svelte-check, and the full unit suite pass unchanged.
…(SL-023) The audit's top finding: persistence.ts (save/load/error paths, WP-vs-standalone branching) and every panel's token-editing interaction had zero test coverage — only pure codec/data logic was tested. Add tests/persistence.test.js: loadInitialOverrides and saveOverrides across both the standalone (localStorage + URL hash) and WP-embedded (REST) branches, including the malformed-localStorage edge case from SL-019 — pinned to today's real (unguarded) behavior since SL-019's fix itself is out of scope here. Add tests-components/token-editing.test.js: SliderRow and TokenRow, covering bind (initial render), override-set (interacting calls onChange/onSet), and reset (reset affordance calls onReset), plus SliderRow's raw-CSS-value editing mode. 89 unit + component tests pass; tsc/svelte-check remain clean.
…parse.js (SL-007) stripComments/maskComments, readValue, and readFile were each reimplemented 3-4 times across audit.js, check-macro-catalog.js, gen-token-index.js, gen-api-index.js, gen-token-reference.js, and version-sync.js, with no shared module despite gen-api-index.js's header claiming a "canonical parsing contract." Extract byte-identical stripComments/stripStrings/readValue into scripts/lib/parse.js. gen-api-index.js's maskComments/maskStrings are NOT the same contract (they blank comment/string bodies in place to preserve character offsets for banner lookups, rather than removing them) — kept as distinct exports per the audit's explicit warning not to merge them. readFile is split into a bare readFile(rel, root) (no existence check, matching version-sync.js's original behavior exactly) and requireFile(rel, root, message) for the four call sites that throw a custom "missing file" error; each site's original error message text is preserved unchanged. No behavior change: npm run docs output (docs/api-index.json, docs/token-index.json, docs/tokens.md, docs/registry.json, token-registry.json) is byte-identical before/after, verified by diffing a pre-refactor snapshot against a post-refactor regeneration. check:macros, check:registry, check:llm-guide, audit:check, check:version, and the root node --test unit suite all pass. Also: - SL-030: annotate docs/llm-guide.md's optional/utilities.css entry as staged/commented-out, matching the file's own header and README's existing wording — the guide previously listed it as if live. - SL-029: reorder README's a-la-carte <link> example to match core/layers.css's real @layer order (themes.css was listed right after base.css; the real order puts it after states.css), and note that @layer order, not <link> order, controls the cascade.
…027) tests/coverage.spec.js never touched a browser (pure fs/regex work comparing core/*.css selectors against docs/demo.html) but was Playwright-discovered, so it ran redundantly 3x across chromium/firefox/webkit. Rename to coverage.test.js, port from @playwright/test's test/expect to node:test/node:assert to match the tier1-p*.test.js convention, and wire it into pretest/test:unit. Confirmed Playwright's test list no longer includes it and node --test runs it once. Update CONTRIBUTING.md's reference to the new filename.
motion (^12.23.24) had zero imports anywhere in configurator/src. Verified via grep before removing. npm install updates the lockfile; version-sync/check:version confirm nothing drifted. tsc, svelte-check, unit + component tests, and vite build all pass unchanged.
📝 WalkthroughWalkthroughThe PR updates configurator codec, persistence, preview, and save-state handling; adds shared parsing and API-index helpers for scripts; and refreshes docs, core CSS comments, and test infrastructure references. ChangesConfigurator codec, preview, and save-state updates
Estimated code review effort: 5 (Critical) | ~90 minutes Shared script parsing, API index generation, and test harness
Estimated code review effort: 5 (Critical) | ~90 minutes Core docs and repository guidance updates
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
PR Summary by QodoAudit quick wins: codec readability, save error state, scripts dedupe, tests
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
Comment-only changes across core/*.css, no selector/property/value diffs (verified: npm run build's badge-optimal.json is byte-identical). - SL-001: cross-link the duplicated dark/light OKLCH clamp() derivation formula between core/tokens.css:364-373 (inside light-dark()) and core/themes.css's SECTION-LEVEL THEMING block (flat re-declaration for [data-theme] overrides on non-:root elements). Left unmerged per the audit's open question - themes.css genuinely needs flat values outside light-dark(), so this may be intentional; deferred to the maintainer. - SL-002: doc-comment above .sf-clickable-parent warning against applying it to large (100+ node) grids, since its selectors combine multiple :has() branches. - SL-003: strengthen --sf-is-dark's "internal, don't set directly" comment to be unmistakable against the immediately-following --sf-is-active/-current/-pressed/-open block's "public hooks" comment - same --sf-is-* naming pattern, opposite contract. Grepped every var(--sf-is-dark) read site first to confirm nothing external sets it. - SL-004: add short section labels to core/layout.css's 11 previously unlabeled blank-line section dividers, matching sibling files' style. - SL-005: consolidate the "var() not allowed inside @container" note (previously stated at only 1 of 4 @container sites) into one comment near the top of core/layout.css. Adding SL-004's /* Section */ label directly above .sf-section shadowed gen-api-index.js's fallback-to-previous-description logic for the uncommented .sf-section--guttered sitting in the same section (its description regenerated as the literal string "Section" instead of its existing curated text). Fixed by promoting that description into docs/token-annotations.json's classes overlay - the durable, intended mechanism for exactly this - rather than removing the section label. Verified: lint:css, build, test:unit, check:llm-guide, audit:check all pass; docs/api-index.json, docs/api-index.md, and the configurator's generated data files are byte-identical to base after the fix.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/lib/parse.js (1)
70-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for this new shared module.
readFile/requireFile(and the other helpers above) are now the single source of truth for parsing/loading logic reused across 6 generator/audit scripts, and the PR relies on "byte-identical docs output" as its correctness signal. A smallnode:testsuite coveringstripCommentsvsmaskCommentsoffset preservation,readValue's paren-depth handling, and thereadFile/requireFileexistence-check divergence would guard this shared surface against regressions that a manual diff-check might miss.🤖 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 `@scripts/lib/parse.js` around lines 70 - 95, Add unit tests for the shared helpers in parse.js, since they now back multiple generator/audit scripts and the docs diff relies on exact output. Cover the key behaviors in stripComments and maskComments for offset preservation, readValue for parenthesis-depth parsing, and readFile versus requireFile for the missing-file behavior difference. Use the helper names themselves so the tests clearly guard this shared surface against regressions.configurator/src/lib/codec.ts (1)
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
CODEC_VERSION/SHARE_PARAMconstants defined but never used internally.
encode/decodestill hardcode the version byte2(Line 109, Line 120) instead ofCODEC_VERSION, andreadShareFromHash/buildShareUrlhardcode the"c="param (Line 260, Line 272) instead ofSHARE_PARAM. If either constant is ever bumped without updating all the literal occurrences (including thec=check inpersistence.ts), encode/decode or share-link parsing will silently desync.♻️ Proposed fix
- out[0] = 2; + out[0] = CODEC_VERSION; ... - if (rawBytes[0] !== 2) { - console.warn(`[codec] unknown config-code version ${rawBytes[0]} (expected 2); ignoring.`); + if (rawBytes[0] !== CODEC_VERSION) { + console.warn(`[codec] unknown config-code version ${rawBytes[0]} (expected ${CODEC_VERSION}); ignoring.`); return {}; } ... - const match = trimmed.match(/[#&]?c=([^&]+)/); + const match = trimmed.match(new RegExp(`[#&]?${SHARE_PARAM}=([^&]+)`)); ... - url.hash = code ? `c=${code}` : ""; + url.hash = code ? `${SHARE_PARAM}=${code}` : "";(Constants would need to be moved above their first use, or declared via
let/hoistedconstat module top.)Also applies to: 120-123, 260-260, 272-272, 281-282
🤖 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/lib/codec.ts` around lines 108 - 110, The codec helpers are still hardcoding protocol literals instead of using the shared constants, so update the version byte handling in encode/decode to use CODEC_VERSION and replace the `"c="` share-link parameter checks/building in readShareFromHash, buildShareUrl, and the related persistence parsing with SHARE_PARAM. Make sure the constants are defined in a scope accessible to those functions in codec.ts and the corresponding parser in persistence.ts so all version/parameter references stay 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/lib/codec.ts`:
- Around line 108-110: The codec helpers are still hardcoding protocol literals
instead of using the shared constants, so update the version byte handling in
encode/decode to use CODEC_VERSION and replace the `"c="` share-link parameter
checks/building in readShareFromHash, buildShareUrl, and the related persistence
parsing with SHARE_PARAM. Make sure the constants are defined in a scope
accessible to those functions in codec.ts and the corresponding parser in
persistence.ts so all version/parameter references stay in sync.
In `@scripts/lib/parse.js`:
- Around line 70-95: Add unit tests for the shared helpers in parse.js, since
they now back multiple generator/audit scripts and the docs diff relies on exact
output. Cover the key behaviors in stripComments and maskComments for offset
preservation, readValue for parenthesis-depth parsing, and readFile versus
requireFile for the missing-file behavior difference. Use the helper names
themselves so the tests clearly guard this shared surface against regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bfe54e0e-d584-47e0-8211-0dd956efaff3
⛔ Files ignored due to path filters (1)
configurator/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
CONTRIBUTING.mdREADME.mdconfigurator/package.jsonconfigurator/src/App.svelteconfigurator/src/components/panels/ExportPanel.svelteconfigurator/src/components/shell/PreviewPanel.svelteconfigurator/src/components/shell/StudioHeader.svelteconfigurator/src/lib/codec.tsconfigurator/src/lib/persistence.tsconfigurator/tests-components/token-editing.test.jsconfigurator/tests/persistence.test.jsdocs/llm-guide.mdpackage.jsonscripts/audit.jsscripts/check-macro-catalog.jsscripts/gen-api-index.jsscripts/gen-token-index.jsscripts/gen-token-reference.jsscripts/lib/parse.jsscripts/version-sync.jstests/coverage.test.js
- codec.ts: encode/decode now reference CODEC_VERSION instead of a hardcoded 2, and readShareFromHash/buildShareUrl/readShareFromHashIfPresent reference SHARE_PARAM instead of a hardcoded "c=" - previously the exported constants existed but every call site hardcoded the literal, so bumping either constant would have silently desynced encode/decode or share-link parsing. Moved both consts above their first use. - scripts/lib/parse.js: add tests/parse-lib.test.js covering stripComments/maskComments offset preservation, readValue's paren-depth handling (including nested clamp()/oklch() shapes), and the readFile-vs-requireFile missing-file divergence - the shared surface backing 6 generator scripts previously had no direct test, only the "docs output is byte-identical" verification a manual diff could miss. Verified: tsc, svelte-check, configurator unit suite (72 tests) unchanged, root unit suite now 44 tests (was 31), lint:css/build/check:llm-guide/ check:version all pass.
core/themes.css's cross-link to core/tokens.css cited a specific line range (364-373) that will drift as either file is edited independently. Replaced with a stable text anchor (the section's own comment text) to search for instead, matching how the tokens.css side already referenced themes.css by section name (SECTION-LEVEL THEMING) rather than by line number. Caught by Qodo's review on PR 475.
…r commit A stray 'rm -rf badges' before rebuilding removed the tracked badge-optimal.json along with the gitignored bundle files, and the subsequent commit picked up the deletion. Restoring it unchanged - no content difference, it was regenerated identically by npm run build.
…033) - SL-014: add the 12 previously-undocumented npm scripts to CLAUDE.md's Key scripts table, prioritizing the CI-gate scripts (check:macros, check:registry, audit:check, lint:css). - SL-028: add tests/README.md documenting the *.spec.js (Playwright) vs *.test.js (node:test) convention, referencing SL-027 as the motivating example of what happens when it's unenforced. - SL-031: retitle docs/migration.md's stale "0.6.25 -> next" header to "0.6.25 -> 0.6.26" (traced via CHANGELOG.md to the release that actually shipped the base-alias removal it documents), matching the versioned-header style used elsewhere in the same file. - SL-032: document the tier1-p2/p7/p8/p10 numbering gaps (no P1, P3-P6, P9 files) in tests/README.md plus a one-line pointer in each of the 4 files - they trace back to the tier-1-color-fallback feature's original property enumeration; not renumbered. - SL-033: extract tests/render-helpers.js (BUNDLE path + renderWithBundle helper) after diffing all 8 page.setContent call sites individually - 5 files (a11y-patterns, layout, macros, states-full, typography) had near-identical setup() functions differing only in viewport/margin/ extra-stylesheet, now parameterized through one shared function; layers.spec.js/print.spec.js/container-queries.spec.js only shared the BUNDLE path constant (their setContent bodies are genuinely per-test or differently-shaped) and were left otherwise untouched, per the plan's explicit caution not to force uniformity where the originals differ. Verified with real browser execution (not just --list): all 5 refactored setup()-using spec files plus layers/print/container-queries ran under a temporary local executablePath override (this sandbox's pinned Playwright browser build isn't pre-installed) - 150/150 tests pass unchanged. lint:css, build, test:unit (31/31), check:llm-guide, check:version all pass; the temporary config was not committed.
…, 012, 013) - SL-006 (highest-risk item in this wave): split gen-api-index.js's 829 lines along its existing section-comment boundaries into scripts/lib/api-index/extract.js (annotations, previous-index reading, banner/comment parsing, bundle mapping, token/class extraction and merging) and render.js (the Markdown companion). gen-api-index.js is now just path constants, the write-time regression guard, and orchestration in main(). One small behavior-neutral cleanup along the way: replaced the hoisted-closure `bundlesFor_` mutable-variable hack with a normal parameter, since extractClassesFromFile now takes bundlesFor directly. No logic changed otherwise - verified by diffing docs/api-index.json, docs/api-index.md, docs/token-index.json, and token-registry.json byte-for-byte against a pre-refactor snapshot (empty diff on all four, both from a single `node scripts/gen-api-index.js` run and from the full `npm run docs` pipeline). - SL-010 + SL-012: add regression guards before the three fs.writeFileSync sites that downstream tooling trusts (gen-api-index.js, gen-token-registry.js, gen-token-index.js) - a >20% entry-count drop (or, for the append-only token-registry.json, any drop at all) now throws instead of silently writing a truncated artifact. No new dependency; hand-rolled against each file's existing "read the previous version" logic. - SL-008/009/011/013: new scripts/README.md documenting the intentionally-unenforced error-handling convention, the hand-rolled CLI flag parsing, the 3 environment variables the scripts read (SLASHED_ROOT, GITHUB_REF/GITHUB_REF_NAME, GITHUB_BASE_REF), and the execSync(shell:true) pattern in check-artifacts.js/changelog-release.js (config-sourced today, not attacker input, but worth flagging for future artifacts.json entries). Verified: npm run docs before/after diff empty on every generated artifact; check:macros, check:registry, check:llm-guide, audit:check, lint:css, build, test:unit (44/44) all pass.
- check:macros was labeled a CI gate; verified against .github/workflows/ci.yml directly and it isn't wired into any job (the audit's own SL-014 finding had asserted it was — that assumption was wrong). Corrected the label rather than adding it to CI, since wiring in a new gate is a separate decision with its own blast radius. - test:unit's new row didn't mention it needs badges/ built first; hit this myself moments earlier while verifying PR3 (5 spurious test failures from a stale badges/ dir). CI works around it by downloading the badges artifact from the build job before this step; a clean local checkout needs npm run build first.
…ared limit constants SL-017/024: replace `any` at codec.ts's 7 registry/options parameters and its two consumers (App.svelte, CheatsheetPanel.svelte) with new TokenRegistry, ApiIndex, SlashedClass and DecodeOptions interfaces in types.ts, so malformed JSON or option shapes are caught at compile time instead of silently no-op'ing at runtime. SL-026: document why MAX_VALUE_BYTES and MAX_ID share the same numeric value (65535) despite being independent limits, so a future edit doesn't assume they're the same constant in disguise.
…re-link readers readShareFromHash()/readShareFromHashIfPresent() were typed to accept DecodeOptions.sanitize but always forced sanitizeValue internally (a CSS-injection safeguard that must not be caller-overridable) and silently discarded any options.sanitize passed in. Narrow their accepted options to a new ShareOptions type (isKnown only) so the signature matches actual behavior instead of advertising a knob that does nothing.
…e-svelte, document preview singleton
SL-019: reuse savedThemes.ts's isStringRecord() shape guard in
persistence.ts's loadInitialOverrides() instead of trusting
JSON.parse(local) as Record<string,string> unchecked — malformed or
wrong-shaped localStorage now falls back to {} instead of passing through.
SL-021: verified already resolved (no motion dependency or imports remain).
SL-022: migrate lucide-svelte -> @lucide/svelte across all 9 importing
files (import-source only, no icon renames needed — verified via tsc/
svelte-check/build after the full migration).
SL-025: document why previewResolver.svelte.ts's previewVersion is a
deliberate module-level singleton rather than component-scoped state.
SL-018 and SL-015 were investigated but required no changes: SL-018's
'error' saveState and its StudioHeader render branch were already fully
implemented in an earlier batch; SL-015's premise (two competing
`overrides` membership idioms to unify) doesn't hold in this codebase —
only the `in` idiom is used, consistently, across ~120 call sites in ~15
files, well beyond the ~15-site estimate in the remediation plan.
…verride changes SL-020: PreviewPanel.svelte's two $effects that write to the preview iframe(s) and call bumpPreviewVersion() re-run on every overrides tick — e.g. every input event while dragging a slider, often several times per animation frame. Deferred the DOM-writing work (style/attribute mutation, font injection, resolver cache invalidation) inside each effect to a requestAnimationFrame callback that captures the latest values, cancelling any not-yet-fired frame from a superseded run via the effect's cleanup. This collapses a same-frame burst down to one apply using the last state, instead of one apply per keystroke. Reactive dependency tracking is unaffected — overrides/previewTheme/ loadCount/etc. are still read synchronously at the top of each effect, so Svelte's re-run triggering is unchanged; only the actual DOM writes are deferred. previewResolver.svelte.ts's bumpPreviewVersion() itself is untouched, preserving its existing "pure write, no reactive read" invariant. Verified via a manual rapid-drag simulation against a real built preview server (40 input events fired faster than one per frame): zero console/ page errors (no effect_update_depth loop), and the preview iframe's applied CSS exactly matched the last dragged value with no dropped trailing frame.
…it-mode effect when inactive
Qodo review on PR6 flagged two real, pre-existing (not introduced by the
rAF-coalescing itself) inefficiencies in the two effects this PR touches:
1. The single-iframe effect called bumpPreviewVersion() explicitly right
after registerPreviewDoc(doc), but registerPreviewDoc() already bumps
internally on every path (both its activeDoc-unchanged early-return and
its replace-doc branch) — the explicit call double-bumped
previewVersion and double-cleared resolveCache on every apply.
2. The split-mode effect unconditionally scheduled an rAF, computed CSS,
and bumped previewVersion even when splitMode is false and no split
iframes exist in the DOM (they're gated behind {#if splitMode}),
making every apply pure overhead outside split mode.
Fixed both: dropped the explicit bump in the single-iframe effect, added
an early return on !splitMode in the split effect, and made its trailing
bump conditional (only fires when the dark pane updated without the light
pane also updating, since registerPreviewDoc's internal bump already
covers the light-pane case) so consumers are still notified exactly once
whenever a split pane actually changed.
Re-verified via the same rapid-drag simulation as the original PR6
commit, now covering both single- and split-preview modes: zero console/
page errors, and both modes' applied CSS exactly matched the last
dragged value.
docs: CSS source cleanup — comment-only fixes (SL-001..005)
…-hygiene docs: docs & test hygiene remainder (SL-014, 028, 031, 032, 033)
…ardening refactor(scripts): build script hardening (SL-006, 008-013)
refactor(configurator): type codec.ts and its call sites (SL-016/017/024/026)
…ebounce perf(configurator): rAF-coalesce preview iframe writes during rapid drags (SL-020)
PR4 (codec.ts types) and PR6 (preview debounce) merged into the integration branch after this branch diverged, both touching import blocks adjacent to PR5's lucide-svelte migration in App.svelte and CheatsheetPanel.svelte. Resolved by keeping both sides: the @lucide/svelte import source (PR5) plus the ApiIndex/SlashedClass type imports (PR4). PreviewPanel.svelte's lucide import (PR5) and its rAF-coalescing effect bodies (PR6) merged cleanly with no conflict. Verified post-merge: tsc/svelte-check clean, 73/73 unit tests + 17/17 component tests pass, full vite build succeeds.
…tor-ux refactor(configurator): shape-guard localStorage, migrate lucide-svelte (SL-015/018/019/021/022/025)
…s save-error state SL-023 continuation, the last item in the remediation plan's PR7. Two real gaps remained after PR4/PR5/PR6 merged and the earlier batch's persistence.ts/SliderRow/TokenRow coverage: - previewResolver.svelte.ts had zero test coverage. Added tests/previewResolver.test.js covering registerPreviewDoc/getActiveTheme, resolveColor/resolveColorForTheme's per-expression and per-theme caching, resolveRgb/resolveBackground's no-preview-doc fallbacks, and — closing the loop on PR6's SL-020 rAF-coalescing — that bumpPreviewVersion() reliably invalidates the cache and is safe to call repeatedly (a pure counter, no reactive loop). Uses a real <iframe>'s contentDocument rather than a detached document, since jsdom's getComputedStyle only resolves colors on documents with a defaultView — matching how PreviewPanel.svelte registers a real iframe in production. - StudioHeader's 'error' saveState (added by SL-018 in an earlier batch) had no test coverage of its own. Added a save-state describe block to tests-components/header.test.js covering all four states (idle/saving/saved/error), specifically that a failed save surfaces a distinct title/label from a fresh save and stays clickable to retry, while an in-progress retry is still correctly disabled. Corrected the file's baseProps fixture, which was missing hasPendingChanges/saveState/onSave (present since SL-018 but never added to this test file). Verified: npm run test:unit (89/89), npm run test:components (23/23), npm run test (112/112 combined), npx tsc --noEmit, npx svelte-check (0 errors/warnings), npm run build, and a real-browser e2e smoke pass against a built preview server (5/6 — the 1 failure is the same pre-existing /favicon.ico 404 noted in PR5/PR6, unrelated).
…est from environment
Qodo review on PR7 flagged two real issues in tests/previewResolver.test.js:
1. makePreviewDoc() appended a new <iframe> to document.body on every
call with no teardown — registerPreviewDoc() only cleans up the
resolver's internal probe elements, not the test-created iframes
themselves, so they accumulated across the file's 16+ calls. Added
an afterEach that removes every iframe makePreviewDoc() created and
unregisters the preview doc.
2. The canvas-degrades-to-null test asserted resolveRgb('red') returns
null based on this sandbox's jsdom lacking the optional `canvas` npm
package, rather than the documented contract — if canvas support
ever became available the test would start failing for an unrelated
reason. Replaced with an explicit getContext('2d') stub, plus a new
companion test asserting the positive path (a real 2D context
available) returns an [r,g,b] triple.
Fixing this surfaced a real subtlety Qodo's own suggested fix would
have missed: each iframe is a separate jsdom realm with its own
HTMLCanvasElement constructor, so a canvas created via
activeDoc.createElement('canvas') is an instance of
doc.defaultView.HTMLCanvasElement, not the top-level test file's
global HTMLCanvasElement — spying on the wrong one is a silent no-op
(verified: the positive-path test failed until the spy target was
corrected to the iframe's own realm).
Verified: npm run test:unit (90/90, up from 89 with the new positive-
path test), npx tsc --noEmit, npx svelte-check (0 errors/warnings), and
a standalone check confirming zero leftover <iframe> elements in
document.body after the full previewResolver suite runs.
…rage test(configurator): cover previewResolver.svelte.ts and save-error state (SL-023, PR7)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@configurator/tests-components/header.test.js`:
- Around line 73-77: The saved-state test in StudioHeader is overstating what it
verifies because it still looks for the same title as the idle state. Update the
`saved` case in `header.test.js` to either assert a truly different title on the
button rendered by `StudioHeader`, or rename the test so it only claims to check
the label text. Use the existing `saved` and idle tests as a reference and keep
the assertion aligned with the actual behavior of `screen.getByTitle` and the
button text.
🪄 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: 2cd408af-332a-4a75-84de-5bfa400066d7
⛔ Files ignored due to path filters (1)
configurator/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (47)
CLAUDE.mdconfigurator/package.jsonconfigurator/src/App.svelteconfigurator/src/components/DomainPanel.svelteconfigurator/src/components/panels/CheatsheetPanel.svelteconfigurator/src/components/panels/ExportPanel.svelteconfigurator/src/components/panels/HomePanel.svelteconfigurator/src/components/panels/ThemesPanel.svelteconfigurator/src/components/shell/PreviewPanel.svelteconfigurator/src/components/shell/SidebarNav.svelteconfigurator/src/components/shell/StudioHeader.svelteconfigurator/src/lib/codec.tsconfigurator/src/lib/persistence.tsconfigurator/src/lib/previewResolver.svelte.tsconfigurator/src/lib/savedThemes.tsconfigurator/src/types.tsconfigurator/tests-components/header.test.jsconfigurator/tests/persistence.test.jsconfigurator/tests/previewResolver.test.jscore/accessibility.csscore/layout.csscore/themes.csscore/tokens.cssdocs/migration.mddocs/token-annotations.jsonpackage.jsonscripts/README.mdscripts/gen-api-index.jsscripts/gen-token-index.jsscripts/gen-token-registry.jsscripts/lib/api-index/extract.jsscripts/lib/api-index/render.jstests/README.mdtests/a11y-patterns.spec.jstests/container-queries.spec.jstests/layers.spec.jstests/layout.spec.jstests/macros.spec.jstests/parse-lib.test.jstests/print.spec.jstests/render-helpers.jstests/states-full.spec.jstests/tier1-p10-contrast.test.jstests/tier1-p2-coverage.test.jstests/tier1-p7-oldengine.test.jstests/tier1-p8-modern.spec.jstests/typography.spec.js
✅ Files skipped from review due to trivial changes (16)
- configurator/src/components/DomainPanel.svelte
- tests/tier1-p7-oldengine.test.js
- configurator/src/components/panels/HomePanel.svelte
- tests/README.md
- tests/tier1-p8-modern.spec.js
- core/themes.css
- tests/tier1-p2-coverage.test.js
- core/accessibility.css
- configurator/src/lib/previewResolver.svelte.ts
- scripts/README.md
- docs/migration.md
- docs/token-annotations.json
- core/tokens.css
- core/layout.css
- tests/tier1-p10-contrast.test.js
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (7)
- configurator/src/components/panels/ExportPanel.svelte
- configurator/src/App.svelte
- configurator/src/components/shell/StudioHeader.svelte
- configurator/src/lib/persistence.ts
- configurator/tests/persistence.test.js
- package.json
- configurator/src/lib/codec.ts
Comment on lines
+73
to
+77
| test('saved: button reads "Saved" with a distinct title from a fresh, unattempted save', () => { | ||
| render(StudioHeader, { props: { ...baseProps, hasPendingChanges: false, saveState: 'saved' } }); | ||
| const btn = screen.getByTitle('No unsaved changes'); | ||
| expect(btn).toHaveTextContent('Saved'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test name overstates its coverage: "saved" and "idle" titles are actually identical.
This test's description says the "saved" title is "distinct... from a fresh, unattempted save," but it queries getByTitle('No unsaved changes') — the exact same title text used by the idle test (Line 56). No assertion here actually verifies title distinctness between the two states; only text content ("Saved" vs "Save") is distinguished. Either fix the test to check a genuinely distinct title, or rename it to reflect that it only asserts label text.
💡 Suggested fix (if titles should differ) or reword test name
- test('saved: button reads "Saved" with a distinct title from a fresh, unattempted save', () => {
- render(StudioHeader, { props: { ...baseProps, hasPendingChanges: false, saveState: 'saved' } });
- const btn = screen.getByTitle('No unsaved changes');
- expect(btn).toHaveTextContent('Saved');
- });
+ test('saved: button reads "Saved" (title matches the idle "no pending changes" state)', () => {
+ render(StudioHeader, { props: { ...baseProps, hasPendingChanges: false, saveState: 'saved' } });
+ const btn = screen.getByTitle('No unsaved changes');
+ expect(btn).toHaveTextContent('Saved');
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('saved: button reads "Saved" with a distinct title from a fresh, unattempted save', () => { | |
| render(StudioHeader, { props: { ...baseProps, hasPendingChanges: false, saveState: 'saved' } }); | |
| const btn = screen.getByTitle('No unsaved changes'); | |
| expect(btn).toHaveTextContent('Saved'); | |
| }); | |
| test('saved: button reads "Saved" (title matches the idle "no pending changes" state)', () => { | |
| render(StudioHeader, { props: { ...baseProps, hasPendingChanges: false, saveState: 'saved' } }); | |
| const btn = screen.getByTitle('No unsaved changes'); | |
| expect(btn).toHaveTextContent('Saved'); | |
| }); |
🤖 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/tests-components/header.test.js` around lines 73 - 77, The
saved-state test in StudioHeader is overstating what it verifies because it
still looks for the same title as the idle state. Update the `saved` case in
`header.test.js` to either assert a truly different title on the button rendered
by `StudioHeader`, or rename the test so it only claims to check the label text.
Use the existing `saved` and idle tests as a reference and keep the assertion
aligned with the actual behavior of `screen.getByTitle` and the button text.
Merged
1 task
jackgranatowski
added a commit
that referenced
this pull request
Jul 2, 2026
…le-claim test(configurator): fix header.test.js saved-state test claim (CodeRabbit finding on #474)
This was referenced Jul 7, 2026
9 tasks
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
2 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.
First batch from the technical-debt audit (#469): the audit's "Top 5" + "Quick wins" lists, tackled together since they overlap. This batch touches pieces of the plan's PR2 (docs/test hygiene), PR3 (script dedup), PR4 (codec readability), PR5 (config UX/deps), and PR7 (test coverage) — later PRs will finish out each of those themes individually.
Findings addressed
codec.ts's internal names (Wa,Ra,za... → readable), updating the 4 call sites that imported them directly. No behavior change; public export surface unchanged.saveState: 'error'state so a failed save is distinguishable from "never attempted" instead of silently reverting to'idle'. Verified live against a mocked failing REST endpoint.persistence.ts(18 tests, both standalone/WP-embedded branches) andSliderRow/TokenRowcomponents (12 tests).scripts/lib/parse.js; dedupestripComments/readValue/readFilethat were copy-pasted across 6 generator scripts.maskComments/maskStrings(offset-preserving, NOT interchangeable) kept as distinct exports per the audit's explicit warning. Verifiednpm run docsoutput is byte-identical before/after.docs/llm-guide.md'sutilities.cssentry as staged/inactive (it was listed as if live).<link>example to matchcore/layers.css's real@layerorder.coverage.spec.js→coverage.test.js, off Playwright (was never touching a browser) ontonode:test.motiondependency.Verification
tsc --noEmit,svelte-check— cleancheck:version,check:llm-guide,check:macros,check:registry,audit:check— all passnode --testunit suite — 31/31 passnpm run docsoutput diffed byte-for-byte before/after the scripts refactorNot verified here
Full Playwright e2e (
npm test) couldn't be run in this environment — the pinned browser build doesn't match what's pre-installed in this sandbox (pre-existing environment limitation, unrelated to this diff).Generated by Claude Code
Summary by CodeRabbit
@layerorder.