test(embed): move integration tests onto @playwright/test under tests/e2e — the runner owns page lifecycle (#389) - #415
Merged
Conversation
…/e2e — the runner owns page lifecycle (#389) Failed assertions in the old hand-rolled `chromium.launch()`/`browser.newPage()` vitest suite leaked live Chromium pages: nothing guaranteed a close on a failure path, and those leaks pressured CI shard-4 and stranded sibling vitest browser sessions. Kill the leak class by construction: @playwright/test owns the browser/context/page lifecycle end to end via its built-in fixtures, so a failed assertion can no longer skip a close. Splits packages/embed's test suite by kind: - tests/e2e/ — the 22 real browser-driven integration tests (*.it.test.ts), now run by `playwright test`. Kit boots stay per-file via test.beforeAll/test.afterAll; per-test pages come from the page/context fixtures; multi-page tests use context.newPage(); the two reload-simulation tests in embed.it.test.ts keep their intentional mid-body page.close(). - tests/unit/ — the 4 build/bundle assertion tests plus proxy-upgrade (an integration test against a real HTTP/WS server that never opens a browser page), still run by vitest. - tests/helpers/ — boot.ts, host.ts, proxy.ts: shared, non-playwright helpers used by both tiers. - tests/e2e/helpers/ — the playwright-only helpers (suite.ts and probe-suite.ts no longer own a browser at all). - tests/fixtures/, tests/dist/, tests/globals.d.ts moved alongside. Supersedes the closed #411 try/finally hand-wrapping attempt and epic #409. Rebased onto #410 (port-drift navigation.set deadline fix) after it merged. Also fixes a latent gap in the banned-vocabulary allowlist: the renamed native-bridge.ts helper was never covered under its old path either, so add the new path to NATIVE_AND_REACT_BRIDGE_PATHS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe embed package now uses Playwright Test for E2E execution. Tests and fixtures moved from ChangesEmbed Playwright migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates embed browser integration tests from hand-managed Chromium sessions to Playwright Test fixtures, preventing page leaks after failed assertions.
Changes:
- Splits browser E2E and Vitest unit suites.
- Adds Playwright configuration and fixture-owned browser lifecycle.
- Updates test paths, fixtures, dependencies, and lint allowlists.
Reviewed changes
Copilot reviewed 45 out of 54 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
.oxlintrc.json |
Updates embed test lint path. |
pnpm-lock.yaml |
Replaces Playwright dependency entry. |
packages/embed/package.json |
Runs both unit and E2E suites. |
packages/embed/playwright.config.ts |
Configures serialized Chromium tests and reporting. |
packages/embed/tsconfig.json |
Includes relocated tests and Playwright config. |
packages/embed/vitest.config.ts |
Restricts Vitest to unit tests. |
packages/embed/vite.global.config.ts |
Updates global fixture path. |
packages/embed/vite.handle.config.ts |
Updates handle fixture/output paths. |
packages/embed/vite.ws-probe.config.ts |
Updates probe fixture/output paths. |
packages/oxlint-plugin/src/banned-vocabulary.js |
Exempts relocated native bridge helper. |
packages/embed/tests/unit/proxy-upgrade.test.ts |
Updates shared helper imports. |
packages/embed/tests/unit/navigation-storage.test.ts |
Updates source import path. |
packages/embed/tests/unit/native-bundle.test.ts |
Updates bundle path. |
packages/embed/tests/unit/mount-node.test.ts |
Updates source import path. |
packages/embed/tests/unit/mount-externals.test.ts |
Updates bundle path. |
packages/embed/tests/helpers/proxy.ts |
Relocates proxy helper. |
packages/embed/tests/helpers/host.ts |
Relocates host/server helper. |
packages/embed/tests/helpers/boot.ts |
Relocates embed kit helper. |
packages/embed/tests/globals.d.ts |
Relocates browser declarations. |
packages/embed/tests/fixtures/ws-probe.ts |
Relocates WebSocket probe fixture. |
packages/embed/tests/fixtures/handle-entry.tsx |
Relocates handle fixture. |
packages/embed/tests/fixtures/global-entry.ts |
Relocates global fixture. |
packages/embed/tests/e2e/transport-selection.it.test.ts |
Uses Playwright page fixture. |
packages/embed/tests/e2e/rpc-observer.it.test.ts |
Migrates RPC observer tests. |
packages/embed/tests/e2e/rpc-fault.it.test.ts |
Migrates fault-injection test. |
packages/embed/tests/e2e/reload-continuity.it.test.ts |
Migrates reload test. |
packages/embed/tests/e2e/recording-attachment.it.test.ts |
Migrates recording test. |
packages/embed/tests/e2e/rebind.it.test.ts |
Migrates rebind tests and deadlines. |
packages/embed/tests/e2e/panel-focus.it.test.ts |
Migrates focus tests. |
packages/embed/tests/e2e/page-plane.it.test.ts |
Migrates page-plane tests. |
packages/embed/tests/e2e/page-dispatch-parity.it.test.ts |
Migrates shared-page parity tests. |
packages/embed/tests/e2e/page-dispatch-boot.it.test.ts |
Migrates boot dispatcher tests. |
packages/embed/tests/e2e/navigation-hold.it.test.ts |
Migrates navigation ordering tests. |
packages/embed/tests/e2e/native-widget.it.test.ts |
Migrates native widget tests. |
packages/embed/tests/e2e/native-live-region.it.test.ts |
Migrates native region tests. |
packages/embed/tests/e2e/model-selector.it.test.ts |
Migrates model error-path test. |
packages/embed/tests/e2e/forced-drop.it.test.ts |
Migrates connection-drop test. |
packages/embed/tests/e2e/embed.it.test.ts |
Migrates main widget suite. |
packages/embed/tests/e2e/element-capture.it.test.ts |
Migrates capture tests. |
packages/embed/tests/e2e/draft-selection.it.test.ts |
Migrates draft persistence test. |
packages/embed/tests/e2e/create-conciv.it.test.ts |
Migrates lifecycle tests. |
packages/embed/tests/e2e/connection-pool.it.test.ts |
Uses fixture-owned shared context. |
packages/embed/tests/e2e/composer-trigger-menu.it.test.ts |
Migrates trigger-menu tests. |
packages/embed/tests/e2e/composer-rich-input.it.test.ts |
Migrates rich-input tests. |
packages/embed/tests/e2e/helpers/suite.ts |
Removes manual browser ownership. |
packages/embed/tests/e2e/helpers/probe-suite.ts |
Removes manual probe browser ownership. |
packages/embed/tests/e2e/helpers/probe-server.ts |
Relocates probe server helper. |
packages/embed/tests/e2e/helpers/panel.ts |
Uses Playwright Test assertions. |
packages/embed/tests/e2e/helpers/page-plane-host.ts |
Accepts fixture-owned pages. |
packages/embed/tests/e2e/helpers/navigation.ts |
Updates Playwright and helper imports. |
packages/embed/tests/e2e/helpers/native-bridge.ts |
Updates Playwright type import. |
packages/embed/tests/e2e/helpers/handle.ts |
Updates Playwright type import. |
packages/embed/tests/e2e/helpers/chat.ts |
Uses Playwright Test assertions. |
packages/embed/test/panel-focus.it.test.ts |
Removes the old test location. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sum the sequential wait budgets (expect timeouts, observer completed timeouts, and the shared helper waits they call into) for every packages/embed/tests/e2e/*.it.test.ts test and set test.setTimeout on any test whose sum clears the ~45s default, so slow CI runs don't trip the global 60s playwright timeout. Move teardown robustness into the shared helpers instead of wrapping every test.afterAll in try/finally at the call site: bootEmbedKit's returned cleanup, serveHost's returned close, proxyTo's returned close, and startProbeServer's returned close now catch and log rather than throw, so a failure in one step never skips the rest of a multi-step afterAll. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@packages/embed/tests/e2e/embed.it.test.ts`:
- Around line 334-363: Update both tests in the affected describe block to use
the phone-suite fixtures consistently: navigate with phoneHost instead of host,
and control the harness through phoneKit instead of kit. Preserve the existing
test flow and assertions while ensuring these cases exercise the describe
block’s phone-specific setup.
In `@packages/embed/tests/e2e/panel-focus.it.test.ts`:
- Around line 54-83: Update each test using openPanelOverFocusedHostButton so
the assertions and interactions execute inside a try block, with await
host.close() moved into a finally block. Apply this to the tests for FAB
closing, header closing, and resize-handle collapsing, ensuring the dedicated
host is closed even when an assertion fails.
In `@packages/embed/tests/e2e/rebind.it.test.ts`:
- Line 15: Remove the duplicate host declarations so each lexical scope retains
exactly one let host declaration in
packages/embed/tests/e2e/rebind.it.test.ts:15-15 and
packages/embed/tests/e2e/recording-attachment.it.test.ts:10-10. Preserve the
existing host type and usage in both files.
In `@packages/embed/tests/helpers/host.ts`:
- Around line 71-77: Update the listenLocal close flow around the close callback
used by the host close handler so callback errors, including
ERR_SERVER_NOT_RUNNING, reject the Promise returned by server.close instead of
resolving it. Preserve successful closure behavior so serveHost.close can
continue logging propagated cleanup failures.
🪄 Autofix
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: 68c445e1-89cb-42ac-9d8c-ba9c397d685c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
.oxlintrc.jsonpackages/embed/package.jsonpackages/embed/playwright.config.tspackages/embed/test/panel-focus.it.test.tspackages/embed/tests/e2e/composer-rich-input.it.test.tspackages/embed/tests/e2e/composer-trigger-menu.it.test.tspackages/embed/tests/e2e/connection-pool.it.test.tspackages/embed/tests/e2e/create-conciv.it.test.tspackages/embed/tests/e2e/draft-selection.it.test.tspackages/embed/tests/e2e/element-capture.it.test.tspackages/embed/tests/e2e/embed.it.test.tspackages/embed/tests/e2e/forced-drop.it.test.tspackages/embed/tests/e2e/helpers/chat.tspackages/embed/tests/e2e/helpers/handle.tspackages/embed/tests/e2e/helpers/native-bridge.tspackages/embed/tests/e2e/helpers/navigation.tspackages/embed/tests/e2e/helpers/page-plane-host.tspackages/embed/tests/e2e/helpers/panel.tspackages/embed/tests/e2e/helpers/probe-server.tspackages/embed/tests/e2e/helpers/probe-suite.tspackages/embed/tests/e2e/helpers/suite.tspackages/embed/tests/e2e/model-selector.it.test.tspackages/embed/tests/e2e/native-live-region.it.test.tspackages/embed/tests/e2e/native-widget.it.test.tspackages/embed/tests/e2e/navigation-hold.it.test.tspackages/embed/tests/e2e/page-dispatch-boot.it.test.tspackages/embed/tests/e2e/page-dispatch-parity.it.test.tspackages/embed/tests/e2e/page-plane.it.test.tspackages/embed/tests/e2e/panel-focus.it.test.tspackages/embed/tests/e2e/rebind.it.test.tspackages/embed/tests/e2e/recording-attachment.it.test.tspackages/embed/tests/e2e/reload-continuity.it.test.tspackages/embed/tests/e2e/rpc-fault.it.test.tspackages/embed/tests/e2e/rpc-observer.it.test.tspackages/embed/tests/e2e/transport-selection.it.test.tspackages/embed/tests/fixtures/global-entry.tspackages/embed/tests/fixtures/handle-entry.tsxpackages/embed/tests/fixtures/ws-probe.tspackages/embed/tests/globals.d.tspackages/embed/tests/helpers/boot.tspackages/embed/tests/helpers/host.tspackages/embed/tests/helpers/proxy.tspackages/embed/tests/unit/mount-externals.test.tspackages/embed/tests/unit/mount-node.test.tspackages/embed/tests/unit/native-bundle.test.tspackages/embed/tests/unit/navigation-storage.test.tspackages/embed/tests/unit/proxy-upgrade.test.tspackages/embed/tsconfig.jsonpackages/embed/vite.global.config.tspackages/embed/vite.handle.config.tspackages/embed/vite.ws-probe.config.tspackages/embed/vitest.config.tspackages/oxlint-plugin/src/banned-vocabulary.js
💤 Files with no reviewable changes (1)
- packages/embed/test/panel-focus.it.test.ts
…close errors panel-focus.it.test.ts left dedicated hosts open on a failed assertion since close() only ran on the happy path; move cleanup to a module-level afterEach that owns every host openPanelOverFocusedHostButton creates. Also make listenLocal's close callback reject on a server.close error instead of silently discarding it, so serveHost.close's existing catch/log wrapper can actually log cleanup failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
omridevk
added a commit
that referenced
this pull request
Aug 10, 2026
…wright Test Main moved the embed integration suite off vitest onto @playwright/test under tests/e2e (#415) since this branch opened. Port the pending test from the old packages/embed/test/panel-focus.it.test.ts onto the new suite: native expect()/test.describe idiom, hostPage/serveHost from tests/helpers, the page.route hold of the first /rpc/sessions/list carried over unchanged.
omridevk
added a commit
that referenced
this pull request
Aug 11, 2026
…te boundary never detaches the composer (#346) (#391) * fix(app): isolate pane query reads behind suspense islands so the route boundary never detaches the composer (#346) solid-router wraps every route Match in a Suspense with an undefined fallback. Solid registers a suspension when a solid-query `.data` read runs inside a non-user computation (a memo or a JSX render effect) while the query is pending with no cached data, and the nearest enclosing Suspense catches it. With no inner boundary that was the route Match, so the whole subtree detached: blank shell, composer unmounted, focus lost. Per the user's direction this uses the platform rather than guarding the reads: every query-reading island now sits in its own Suspense with a real loading state (session pill, context usage, view tabs, composer actions, thread) and the reads stay plain. The root FAB's working state became a real ring element inside its own boundary, so the button, its ref and the mascot rig never suspend. The composer input shares a boundary with no query read at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(test): wait for the live page plane, not the FAB, before tanstack page tools (#346) waitForWidget treated "the FAB is visible" as "the widget is connected". That held only by accident: the FAB's class read sessions.data, so the whole button suspended until that query round trip finished, which was always after the page plane's page.queries subscription reached the server. #346 moved the working() read into its own Suspense island, so the FAB now paints immediately and the accidental synchronisation is gone. The first adapter.client.detect() then hit an empty page bus, got NO_PAGE_CLIENT, and the adapter's catch turned it into null. Gate the helper on the real readiness signal instead: the rpc observer's completed() for the page.queries subscription. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(embed): port the session-list-loading focus regression onto Playwright Test Main moved the embed integration suite off vitest onto @playwright/test under tests/e2e (#415) since this branch opened. Port the pending test from the old packages/embed/test/panel-focus.it.test.ts onto the new suite: native expect()/test.describe idiom, hostPage/serveHost from tests/helpers, the page.route hold of the first /rpc/sessions/list carried over unchanged. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
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.
Summary
chromium.launch()/browser.newPage()vitest suite leaked live Chromium pages: nothing guaranteed a close on a failure path, and those leaks pressured CI shard-4 and stranded sibling vitest browser sessions (Flaky: grab-reference.browser.test.tsx dies with 'Browser connection was closed' + extension teardown unhandled (CI shard-4) #389, epic Zero flakes: consolidate and burn down every known CI flake #409). This PR kills the leak class by construction:@playwright/testowns the browser/context/page lifecycle end to end via its built-in fixtures, so a failed assertion can no longer skip a close.packages/embed's test suite by kind:tests/e2e/(22 real browser-driven integration tests, run byplaywright test) vstests/unit/(5 vitest tests that never open a browser page — the 4 build/bundle assertion tests plusproxy-upgrade.test.ts, an HTTP/WS integration test with no browser involved).navigation.setdeadline fix) after it merged to main;rebind.it.test.ts's port-drift test carries that fix's content.Layout
tests/e2e/— the 22*.it.test.tsfiles, converted totest/test.describe/test.beforeAll/test.afterAll. Per-test pages come from the built-inpage/contextfixtures; multi-page tests usecontext.newPage(); the two reload-simulation tests inembed.it.test.tskeep their intentional mid-bodypage.close()(that's the "reload" trigger, not a leak).expectLocator(theplaywright/testalias) is gone — oneexpectfrom@playwright/testcovers locator and value assertions everywhere.tests/unit/— vitest,environment: 'node', unchanged behavior.tests/helpers/—boot.ts,host.ts,proxy.ts: shared, non-playwright helpers used by both tiers (proxy-upgrade.test.tsintests/unit/needsboot.ts/proxy.tstoo, so they can't live undertests/e2e/helpers/).tests/e2e/helpers/— the playwright-only helpers.suite.ts/probe-suite.tsno longer launch or own aBrowserat all — kit + host still boot once per file viatest.beforeAll/test.afterAll, but browser lifecycle is entirely fixture-owned now.tests/fixtures/,tests/dist/(build output for the handle/ws-probe test bundles),tests/globals.d.tsmoved alongside;vite.handle.config.ts,vite.ws-probe.config.ts,vite.global.config.ts,tsconfig.json,.oxlintrc.jsonupdated to match.Config choices
*.it.test.tsnaming convention (didn't rename to playwright's*.spec.ts) — it's a repo-wide "browser integration test" marker (e.g.apps/conciv/test/transport-standalone.it.test.tskeeps it under vitest too), and fragmenting the convention per-runner would cost more than it saves.playwright.config.ts:workers: 1,fullyParallel: false— mirrors embed's vitestfileParallelism: false(tests share real Chromium + real HTTP/WS servers and were never meant to run concurrently).timeout: 60_000/expect: {timeout: 30_000}default, matching vitest's oldtestTimeout: 60_000; per-testtest.setTimeout(90_000|120_000)overrides on the handful of tests that chain 3+ sequential 30s-ceiling waits (rebind.it.test.ts's port-drift test chains ~10), so a degraded CI run gets the real per-step timeout message instead of a premature whole-test abort.ciTest()reporters fortests/unitas-is, and added a playwright reporter fortests/e2emirroringe2e-utils' exact pattern (linelocally,line+json → test-results.jsonin CI). Since thetestscript runsvitest run && playwright testin sequence, playwright's later write naturally becomes the package's canonicaltest-results.jsonfor@conciv/embed's summary row — no new merge machinery.conciv-ci-shards'parseReportalready auto-detects playwright's{suites: [...]}shape (seeparsePlaywrightReportinpackages/vitest-config/src/summary.ts), so zero changes needed there. Trade-off, stated explicitly: unit-suite failures still fail thetestscript (and therefore the CI job) via exit code, they just won't get their own per-case rows in the job summary table for this one package — acceptable giventests/unitis 5 small, stable tests.Also fixed
packages/oxlint-plugin/src/banned-vocabulary.js'sNATIVE_AND_REACT_BRIDGE_PATHSallowlist never coverednative-bridge.tsunder its old path either (verified:oxlintfails identically on the pre-migration file content) — added the new path so the legitimate iOS-bridge naming stays exempted from the deleted-chat-rewrite-domain check.Test plan
pnpm turbo run build --filter=@conciv/embed(prebuilt widget bundle for the e2e tests)tsc -p packages/embed/tsconfig.json --noEmit— cleanpnpm testinpackages/embed(vite handle + ws-probe builds → vitest → playwright): 20 unit tests + 105 e2e tests, all green, zero skips (confirmed clean on 3 separate runs; 2 other runs on this same run flaked/misattributed under heavy concurrent-Chromium load from other sessions on the machine — see PR discussion)oxlint+oxfmt --checkclean on touched filesfallow audit --changed-since mainverdictpass, zero introduced findings🤖 Generated with Claude Code
Summary by CodeRabbit