Skip to content

fix(browser-sdk): clean up Preact roots before test teardown - #745

Merged
roncohen merged 1 commit into
mainfrom
fix/browser-test-teardown
Sep 16, 2026
Merged

roncohen merged 1 commit into
mainfrom
fix/browser-test-teardown

Conversation

@roncohen

@roncohen roncohen commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Root cause

Both #739 and #742 were ejected from the merge queue after all 205 browser assertions passed: a delayed Preact toolbar render accessed sessionStorage after jsdom teardown. client.stop() does not unmount the UI; removing DOM nodes alone does not cancel Preact work.

Fix

  • Unmount feedback and toolbar shadow-root trees before removing their hosts in shared afterEach cleanup.
  • Return the toolbar flagsUpdated unsubscribe function from its effect.
  • Remove the earlier usage.test.ts toolbar mock: tests now exercise the real toolbar.
  • Add two deterministic regressions covering delayed effects and already-queued renders after browser globals disappear. Omitting the unmount makes both fail with the exact ReferenceError.

Validation

  • Unmodified main: reproduced the exact sessionStorage rejection in 5 of 40 complete browser-suite runs in a local Linux container (Node 24.13.0, 2 CPUs, CI=true).
  • Fixed code: 0 failures in 40 complete browser-suite runs in the same Linux container, with the usage-test toolbar mock removed. Every run exited 0.
  • Both GitHub Build & Test checks passed.
  • The two deterministic tests pass with unmounting, and both fail with the exact ReferenceError: sessionStorage is not defined when render(null, host.shadowRoot) is omitted (even when the host DOM node is removed).
  • Local reproduction logs: /tmp/reflag-flake-linux/repro-baseline/{1..40}.log and /tmp/reflag-flake-linux/repro-fixed/{1..40}.log; fixed exit codes are in repro-fixed/results.txt.
  • Reproduction environment: node:24.13.0-bookworm, Docker --cpus=2, immutable root install, then repeatedly run CI=true yarn vitest run --reporter=default from packages/browser-sdk. Native macOS runs did not reproduce the exact error, so the before/after counts above are exclusively Linux.
  • Full uncached build and all nine workspace test:ci targets passed locally, including Playwright.
  • Formatting and lint passed (existing Next-generated triple-slash warning only).

Follow-up: once merged, requeue #739 and #742.

@roncohen
roncohen marked this pull request as ready for review September 16, 2026 09:10
Copilot AI lite review requested due to automatic review settings September 16, 2026 09:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@roncohen
roncohen added this pull request to the merge queue Sep 16, 2026
Merged via the queue into main with commit e8a0e10 Sep 16, 2026
7 of 8 checks passed
@roncohen
roncohen deleted the fix/browser-test-teardown branch September 16, 2026 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants