test(configurator): cover previewResolver.svelte.ts and save-error state (SL-023, PR7)

[jackgranatowski]

Summary

Seventh and final themed PR from the SLASHED technical-debt audit (PR #469): the configurator test-coverage buildout (SL-023 continuation). Lands last per the remediation plan's explicit ordering, now that PR4/PR5/PR6 are merged.

Priority 1 (persistence.ts load/save, both standalone and WP-embedded branches, including the malformed-localStorage case) and most of Priority 3 (SliderRow/TokenRow bind → override-set → reset) were already covered by earlier batches. Two real gaps remained:

• previewResolver.svelte.ts had zero test coverage. Added tests/previewResolver.test.js (16 tests) 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.implementation.createHTMLDocument(), since jsdom's getComputedStyle only resolves colors on documents that have a defaultView (a detached document lacks one) — this matches how PreviewPanel.svelte registers a real iframe in production.
  • resolveRgb's canvas-context test documents and locks in the graceful-degradation path: this sandbox has no canvas npm package, so jsdom's getContext('2d') is unimplemented, and resolveRgb correctly degrades to null rather than throwing.

• StudioHeader's 'error' saveState (added by SL-018 in an earlier batch) had no dedicated test coverage. Added a save state describe block to tests-components/header.test.js (6 new tests) 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 correctly disabled. Also corrected the file's baseProps fixture, which was missing hasPendingChanges/saveState/onSave (present in the component's props since SL-018, never added to this test file).

Test plan

• npm run test:unit — 89/89 passed
• npm run test:components — 23/23 passed
• npm run test (combined vitest suite) — 112/112 passed
• npx tsc --noEmit — clean
• npx svelte-check --tsconfig ./tsconfig.json — 0 errors, 0 warnings
• npm run build — succeeds
• Real-browser e2e smoke pass (tests-e2e/shell.spec.js) against a built preview server — 5/6 passing; the 1 failure is the same pre-existing /favicon.ico 404 noted in PR5/PR6, unrelated to this PR (test-only changes, no runtime source touched)

---

Generated by Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4d9fbf90-ddcd-4c97-a68e-61bdae75aa91

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/audit-pr7-test-coverage

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[qodo-code-review]

PR Summary by Qodo

Tests: add coverage for previewResolver cache/invalidation and StudioHeader save-error

🧪 Tests 🕐 40+ Minutes

AI Description

• Add unit tests for previewResolver’s theme resolution, caching, and version-based invalidation.
• Add StudioHeader save button coverage for idle/saving/saved/error states, including
 retry-on-error.
• Fix header test fixtures to include the full save-related prop surface.

Diagram

graph TD
  T1["tests-components/header.test.js"] --> SH["StudioHeader.svelte"]
  T2["tests/previewResolver.test.js"] --> PR["previewResolver.svelte.ts"] --> DOC["Preview iframe doc"] --> API["getComputedStyle / canvas"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Mock getComputedStyle instead of using a real iframe

• ➕ More deterministic and faster unit tests (no iframe/document wiring).
• ➕ Avoids jsdom limitations around defaultView and style computation.
• ➖ Less representative of production behavior (resolver depends on real document semantics).
• ➖ Harder to validate caching/invalidation around actual getComputedStyle calls.

2. Add a canvas polyfill (e.g., node-canvas) to fully exercise resolveRgb

• ➕ Would allow asserting exact RGB triples rather than only the null fallback path.
• ➖ Adds a heavier native dependency surface to the test environment.
• ➖ Current behavior explicitly allows graceful degradation; asserting null is a valid contract test.

Recommendation: Keep the PR’s approach: using a real iframe contentDocument matches production registration and is necessary for jsdom to provide a defaultView-backed getComputedStyle. Testing resolveRgb’s null-on-missing-canvas path is appropriate given the current environment and the function’s documented fallback behavior; only consider adding a canvas dependency if consumers require non-null resolveRgb guarantees in CI.

Files changed (2) +224 / -1

Tests (2) +224 / -1

header.test.jsAdd StudioHeader save-state coverage and complete baseProps fixture +50/-1

Add StudioHeader save-state coverage and complete baseProps fixture

• Extends the StudioHeader component tests with a dedicated save-state suite covering idle/saving/saved/error behavior, including retry clickability after errors and disabled state while saving. Updates the shared baseProps fixture to include hasPendingChanges/saveState/onSave and uses vi.fn for click assertions.

configurator/tests-components/header.test.js

previewResolver.test.jsAdd unit tests for previewResolver theme resolution, caching, and fallbacks +174/-0

Add unit tests for previewResolver theme resolution, caching, and fallbacks

• Introduces a new test suite covering registerPreviewDoc/getActiveTheme, resolveColor/resolveColorForTheme caching semantics, and cache invalidation via bumpPreviewVersion. Uses a real iframe contentDocument to ensure jsdom can resolve computed colors, and validates graceful degradation for resolveRgb when no 2D canvas context is available, plus resolveBackground’s no-doc fallback.

configurator/tests/previewResolver.test.js

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 6 rules

1. Leaky iframe test fixtures ✓ Resolved 🐞 Bug ☼ Reliability

Description

makePreviewDoc() appends a new <iframe> to document.body but never removes it, so the DOM
grows across tests within the file and can pollute/slow the suite. registerPreviewDoc() only
removes resolver probe elements, not the test-created iframes, so teardown needs to explicitly
remove them.

Code

configurator/tests/previewResolver.test.js[R30-36]

+function makePreviewDoc(theme = 'light') {
+  const iframe = document.createElement('iframe');
+  document.body.appendChild(iframe);
+  const doc = iframe.contentDocument;
+  doc.documentElement.setAttribute('data-theme', theme);
+  return doc;
+}

Relevance

⭐⭐⭐ High

Team often accepts cleanup/teardown to prevent leaks (timeouts/debounced saves cleared on teardown
in PR #445, #443).

PR-#445
PR-#443

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test helper appends an iframe to the global document and returns only the document, with no
cleanup in the test file; the resolver cleanup path does not remove iframes, only internal probe DOM
nodes.

configurator/tests/previewResolver.test.js[30-36]
configurator/src/lib/previewResolver.svelte.ts[55-72]

Agent prompt

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

### Issue description
`makePreviewDoc()` appends iframes to `document.body` and never removes them, causing DOM leakage across tests.

### Issue Context
The resolver’s `registerPreviewDoc()` cleans up probe elements only; it does not remove iframes created by the tests.

### Fix Focus Areas
- configurator/tests/previewResolver.test.js[30-36]

### Suggested fix
- Track created iframes in an array (or return `{ doc, iframe }` from `makePreviewDoc`).
- Add `afterEach(() => { registerPreviewDoc(null); iframes.forEach(f => f.remove()); iframes.length = 0; })`.
- Optionally also clear `document.body` if appropriate for this suite, but removing only the created iframes is the least invasive.

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

---

2. Environment-coupled canvas test ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The resolveRgb test asserts resolveRgb('red') returns null based on the current jsdom
environment lacking a working 2D canvas; if a canvas implementation becomes available, resolveRgb
will return an RGB tuple and this test will fail even though the code is correct. The test should
explicitly mock/stub getContext('2d') to force the intended branch.

Code

configurator/tests/previewResolver.test.js[R150-161]

+  // This sandbox has no `canvas` npm package installed, so jsdom's
+  // HTMLCanvasElement.getContext('2d') is unimplemented and logs (but does
+  // not throw) a "not implemented" error — resolveRgb must degrade to null
+  // rather than propagating that. Silencing the expected console.error here
+  // to keep test output clean; the assertion is what matters.
+  test('degrades to null (not a throw) when no 2D canvas context is available', () => {
+    registerPreviewDoc(makePreviewDoc());
+    const spy = vi.spyOn(console, 'error').mockImplementation(() => {});
+    expect(() => resolveRgb('red')).not.toThrow();
+    expect(resolveRgb('red')).toBeNull();
+    spy.mockRestore();
+  });

Relevance

⭐⭐ Medium

No clear historical pattern about mocking jsdom canvas; repo accepted resolveRgb tweaks but not this
test-hardening style.

PR-#429

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

resolveRgb returns [r,g,b] when a 2D context exists, so asserting null without forcing
getContext to fail couples the test to the current environment’s canvas support.

configurator/src/lib/previewResolver.svelte.ts[163-193]
configurator/tests/previewResolver.test.js[150-161]

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 test outcome depends on whether the runtime provides a real 2D canvas context; that’s an environmental detail, not the behavior under test.

### Issue Context
`resolveRgb()` returns a tuple when a 2D context exists and `null` when it doesn’t. The test currently relies on jsdom’s missing implementation rather than explicitly controlling it.

### Fix Focus Areas
- configurator/tests/previewResolver.test.js[150-161]

### Suggested fix
- Replace the console.error-based approach with an explicit stub:
 - `const spy = vi.spyOn(HTMLCanvasElement.prototype, 'getContext').mockReturnValue(null);`
 - Assert `resolveRgb('red') === null`
 - `spy.mockRestore()`
- (Optional) Add a separate unit test that stubs `getContext` to return a minimal fake 2D context and asserts `resolveRgb()` returns a tuple, so both branches are covered without relying on environment features.

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

---

[jackgranatowski]

Both good catches — fixed in 156e421.

1. Leaky iframe fixtures: added an afterEach that removes every <iframe> makePreviewDoc() created and unregisters the preview doc.
2. Environment-coupled canvas test: replaced the "no canvas package installed" assumption with an explicit getContext('2d') stub, plus a new companion test for the positive path (a real 2D context returns an [r,g,b] triple).

One subtlety the literal 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 — confirmed by the new positive-path test actually failing until the spy target was corrected to the iframe's own realm. Both tests now spy on doc.defaultView.HTMLCanvasElement.prototype.

Verified: 90/90 unit tests (up from 89 with the new positive-path test), tsc/svelte-check clean, and a standalone check confirming zero leftover <iframe> elements in document.body after the full suite runs.

---

Generated by Claude Code