Skip to content

test(frontend): replace JSON structuredClone polyfill with faithful one#1241

Open
cristim wants to merge 2 commits into
mainfrom
test/test-07-fix
Open

test(frontend): replace JSON structuredClone polyfill with faithful one#1241
cristim wants to merge 2 commits into
mainfrom
test/test-07-fix

Conversation

@cristim

@cristim cristim commented Jun 11, 2026

Copy link
Copy Markdown
Member

Problem

TEST-07 (docs/reviews/codebase-review-2026-06-10.md): the Jest setup polyfilled structuredClone for jsdom with JSON.parse(JSON.stringify(val)), which drops undefined-valued properties, stringifies Dates, degrades Map/Set to plain objects, and throws on cycles. Tests guarding the deep-copy-state invariant therefore passed against JSON semantics, not browser semantics, so clone tests could stay green while runtime types drifted in the browser.

Fix

  • frontend/src/__tests__/setup.ts: keep the existing native-first guard (typeof globalThis.structuredClone === 'undefined'), but when polyfilling, delegate to @ungap/structured-clone, a faithful implementation of the HTML structured clone algorithm. It runs inside the jsdom sandbox, so cloned objects keep sandbox-realm prototypes and instanceof assertions keep working (a Node-realm clone, e.g. a worker_threads MessageChannel round-trip, deserializes with Node-realm prototypes and breaks instanceof Date/Map). Unsupported transfer lists now throw instead of being silently ignored.
  • frontend/src/__tests__/ungap-structured-clone.d.ts: typed module declaration (the package ships no types).
  • frontend/src/__tests__/utils.test.ts: new deepClone (TEST-07) regression suite asserting real semantics: undefined preservation, Date instance fidelity, Map/Set fidelity, cyclic references, throw on functions. Also removes the stale comment claiming undefined preservation could not be asserted under jsdom.
  • frontend/package.json / package-lock.json: add @ungap/structured-clone as a devDependency (it was already in the tree as an eslint transitive dep).

Test evidence

  • Pre-fix (JSON polyfill stashed back in): all 5 new regression tests fail (Tests: 5 failed, 86 skipped, 91 total).
  • Post-fix full suite: Test Suites: 77 passed, 77 total; Tests: 1 skipped, 2558 passed, 2559 total (the skip is pre-existing).
  • npm run typecheck clean; npm run build succeeds.

Closes #1160

Summary by CodeRabbit

  • Bug Fixes

    • Improved cloning behavior in the test environment so it matches modern browser semantics more closely, including support for dates, maps, sets, cyclic data, and undefined values.
    • Added safer handling when cloning is used with unsupported transfer options.
  • Tests

    • Expanded unit coverage for deep cloning edge cases to prevent regressions.

@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/internal Team-internal only effort/xs Trivial / one-liner type/chore Maintenance / non-user-visible labels Jun 11, 2026
@cristim

cristim commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f7d424ee-4d1a-405f-8a09-4454123687a6

📥 Commits

Reviewing files that changed from the base of the PR and between 60526ce and df9bd84.

⛔ Files ignored due to path filters (1)
  • frontend/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • frontend/package.json
  • frontend/src/__tests__/setup.ts
  • frontend/src/__tests__/ungap-structured-clone.d.ts
  • frontend/src/__tests__/utils.test.ts

📝 Walkthrough

Walkthrough

This change replaces the JSON round-trip polyfill for globalThis.structuredClone in the Jest test setup with the @ungap/structured-clone library, adds corresponding TypeScript type declarations and a devDependency, and adds regression tests validating real structured-clone semantics for deepClone.

Changes

structuredClone polyfill replacement

Layer / File(s) Summary
Dependency and type declarations
frontend/package.json, frontend/src/__tests__/ungap-structured-clone.d.ts
Adds @ungap/structured-clone as a devDependency and declares StructuredCloneOptions and the structuredClonePolyfill default export type.
structuredClone fallback wiring
frontend/src/__tests__/setup.ts
Imports structuredClonePolyfill and replaces the JSON-based globalThis.structuredClone fallback with a wrapper delegating to the polyfill, throwing when transfer is requested.
Regression tests for real clone semantics
frontend/src/__tests__/utils.test.ts
Updates comments and adds a TEST-07 suite verifying undefined preservation, Date, Map/Set, cyclic references, and function-clone throw behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Test as deepClone test
  participant Global as globalThis.structuredClone
  participant Polyfill as structuredClonePolyfill

  Test->>Global: structuredClone(value, options)
  Global->>Global: check options.transfer
  alt transfer provided
    Global-->>Test: throw Error
  else no transfer
    Global->>Polyfill: structuredClonePolyfill(value)
    Polyfill-->>Global: cloned value
    Global-->>Test: cloned value
  end
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning Issue #1160 is addressed, but the AWS IAM console deep link from #39 is not present in the reviewable changes. Add the AWS IAM console link in the account modal and lock its href, target, and rel attributes with tests.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: replacing the JSON structuredClone polyfill with a faithful implementation.
Out of Scope Changes check ✅ Passed The changes stay focused on the structuredClone polyfill, its typing, and regression tests, with no clear unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/test-07-fix

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

@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/internal Team-internal only effort/xs Trivial / one-liner type/chore Maintenance / non-user-visible labels Jun 11, 2026
@cristim cristim force-pushed the test/test-07-fix branch from b03f7a5 to 1c8e712 Compare June 19, 2026 21:58
@cristim

cristim commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 5 minutes and 26 seconds before sending another message.

cristim added a commit that referenced this pull request Jun 26, 2026
Adversarial review of #1241 found two stale/misleading comments about the
new structuredClone polyfill:

- utils.test.ts said "via Node's MessageChannel serializer" -- left over
  from an earlier draft that used MessageChannel; the actual polyfill is
  @ungap/structured-clone. Update to name the real package.
- setup.ts claimed the polyfill "must run inside the jsdom sandbox" so
  instanceof works. In practice @ungap delegates to Node's native
  structuredClone when present; instanceof works because jsdom-on-jest
  shares Date/Map/Set primordials with Node, not because of any sandbox
  effect. Describe the real mechanism instead.

No behavior change.
@cristim

cristim commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim added 2 commits July 4, 2026 01:24
The Jest setup polyfilled structuredClone for jsdom with a
JSON.parse(JSON.stringify(...)) round-trip, which drops undefined-valued
properties, stringifies Dates, degrades Map/Set to plain objects, and
throws on cycles. Deep-clone tests guarding the deep-copy-state
invariant therefore validated weaker semantics than production
browsers.

Replace the JSON round-trip with @ungap/structured-clone, a faithful
implementation of the HTML structured clone algorithm, applied only
when the environment does not already provide a native
structuredClone. The polyfill runs inside the jsdom sandbox so cloned
objects keep sandbox-realm prototypes and instanceof assertions work
(a Node-realm clone such as worker_threads MessageChannel round-trip
would break them). Unsupported transfer lists now fail loud instead of
being silently ignored.

Add regression tests asserting real structured clone semantics
(undefined preservation, Date/Map/Set fidelity, cyclic references,
throw on functions); all five fail against the previous JSON polyfill
and pass now. Also update the stale test comment that claimed
undefined preservation could not be asserted under jsdom.

Closes #1160
Adversarial review of #1241 found two stale/misleading comments about the
new structuredClone polyfill:

- utils.test.ts said "via Node's MessageChannel serializer" -- left over
  from an earlier draft that used MessageChannel; the actual polyfill is
  @ungap/structured-clone. Update to name the real package.
- setup.ts claimed the polyfill "must run inside the jsdom sandbox" so
  instanceof works. In practice @ungap delegates to Node's native
  structuredClone when present; instanceof works because jsdom-on-jest
  shares Date/Map/Set primordials with Node, not because of any sandbox
  effect. Describe the real mechanism instead.

No behavior change.
@cristim cristim force-pushed the test/test-07-fix branch from c220e0f to df9bd84 Compare July 3, 2026 23:26
@cristim

cristim commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Rebased onto origin/main (31 commits ahead).

Conflicts resolved in frontend/package.json and frontend/package-lock.json:

  • package.json: main had upgraded @typescript-eslint/eslint-plugin and @typescript-eslint/parser to ^8.0.0 (via the ESLint config PR). Resolution keeps main's ^8.0.0 constraint and adds this PR's @ungap/structured-clone: ^1.3.1.
  • package-lock.json: accepted main's lockfile and regenerated via npm install --prefer-offline to incorporate the new dep cleanly.

Gates passed:

  • npm run typecheck clean
  • npm test: Test Suites: 77 passed, 77 total; Tests: 1 skipped, 2558 passed, 2559 total (all 5 new deepClone regression tests pass)

New head: df9bd84

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

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TEST-07: Jest structuredClone polyfill uses JSON round-trip; deep-clone tests validate weaker semantics

1 participant