fix(extension-testkit): name-bound the dispose steps and stop rpc-observer timers outliving their observer (#361) - #418
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe extension testkit now supports named, timed teardown steps. RPC observers retain waiter promises, project settlement results, suppress abandoned-wait rejections, and dispose automatically when their page closes. ChangesExtension testkit lifecycle handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Page
participant RPCObserver
participant PendingWait
Page->>RPCObserver: close event
RPCObserver->>PendingWait: reject with disposal error
RPCObserver->>RPCObserver: attach rejection handlers
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens extension-testkit teardown by bounding named cleanup steps and disposing RPC observers with their pages.
Changes:
- Adds per-step teardown timeouts and diagnostics.
- Cancels pending RPC observer waits on disposal/page closure.
- Adds regression coverage and the
p-timeoutdependency.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Locks p-timeout. |
packages/extension-testkit/package.json |
Adds the teardown dependency. |
packages/extension-testkit/src/settle-teardown.ts |
Bounds and names cleanup steps. |
packages/extension-testkit/src/rpc-observer.ts |
Rejects pending waits during disposal. |
packages/extension-testkit/src/launch.ts |
Disposes observers when pages close. |
packages/extension-testkit/src/get-extension-test-api.ts |
Names teardown operations. |
packages/extension-testkit/test/settle-teardown.test.ts |
Tests bounded, independent teardown. |
packages/extension-testkit/test/rpc-observer-dispose.it.test.ts |
Tests observer disposal behavior. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/extension-testkit/src/settle-teardown.ts`:
- Around line 15-18: Update the teardown step mapping around step.run() to
invoke it inside a promise callback before passing it to pTimeout, converting
synchronous throws into rejected promises so steps.map() continues and later
teardown steps settle. Add coverage for a synchronously throwing run function
and verify subsequent steps still complete.
🪄 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: 0be7ba79-60da-4db6-b126-38a22e6a0e43
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/extension-testkit/package.jsonpackages/extension-testkit/src/get-extension-test-api.tspackages/extension-testkit/src/launch.tspackages/extension-testkit/src/rpc-observer.tspackages/extension-testkit/src/settle-teardown.tspackages/extension-testkit/test/rpc-observer-dispose.it.test.tspackages/extension-testkit/test/settle-teardown.test.ts
…erver timers outliving their observer (#361) settleTeardown now names each step and wraps it in p-timeout (20s default), so a wedged closeBrowser/close/stop names itself in the error instead of blowing vitest's generic 30s hookTimeout, and the sibling steps still settle inside the hook budget. rpc-observer's dispose() cleared its waiters Set but never cancelled the setTimeout each buffered() call schedules, so a pending wait outlived its own observer and page; under @playwright/test's single-worker-per-file run, that timer could fire during a later, unrelated test and misattribute a timeout. dispose() now fails every pending waiter (which clears its timer) before clearing the set, and openObservedPage wires dispose to the page's own 'close' event so observers never outlive the page that owns them. Refs #361, epic #409.
…c-observer waits CI caught a regression from the prior dispose fix: embed's composer-rich-input.it.test.ts failed with "Error: rpc observer disposed while awaiting page.queries" leaking as an unhandled rejection from an abandoned wait (a wait a test never awaited, or moved on from before the page closed). Root cause: completed()/firstEvent() were async wrapper functions, so the promise a caller actually holds is a *different* object from the internal buffered() promise — attaching a no-op .catch() to only the internal one (the previous fix) didn't reach the outer promise callers can abandon. Collapsed the wrapper: buffered()/settle() now take the result projection directly and return the exact promise handed to callers, so Waiter.promise IS that promise. dispose() attaches a no-op .catch() to it before failing it, so an abandoned wait's disposal rejection is always handled (no leak) while an awaited wait still receives the rejection (multiple handlers each get it). Refs #361, epic #409.
- settle-teardown.ts: wrap step.run() in Promise.resolve().then() before pTimeout so a step that throws synchronously (not just one that returns a rejected promise) still lets sibling steps start and complete, instead of throwing out of steps.map() before any step began. - rpc-observer-dispose.it.test.ts: drop the wall-clock Date.now()-delta assertion; a descheduled CI worker can fail it for reasons unrelated to the code under test. rejects.toThrow(/disposed/) already proves disposal beat the natural timeout semantically. - rpc-observer-dispose.it.test.ts: migrate the two raw chromium.launch() calls onto @conciv/browser-fixture's file-scoped, timeout-bounded browser fixture (already a direct dependency), each test opening and closing its own page. The third test (closing the launched page...) is unchanged: it exercises launch.ts's own browser lifecycle, not a bare chromium.launch() call. Refs #361, epic #409.
omridevk
force-pushed
the
fix/361-dispose-bounds
branch
from
August 10, 2026 23:29
240527b to
c81ee05
Compare
… rpc-observer test CI (run 31442555924, shard-1) failed rpc-observer.it.test.ts:186 with "rpc observer disposed while awaiting page.queries" — not composer-rich-input as first suspected. The test intentionally leaves a page.queries wait pending (proving an extension-namespaced call never falsely matches the core path) and disposes while it's still pending, which #361's dispose contract correctly rejects. The test derived a second promise via `.then()` with no `.catch()`; dispose's own `waiter.promise.catch(() => {})` only guards the promise it holds, not sibling branches callers derive from it, so the derived chain surfaced as an unhandled rejection. Attaching `.catch()` to that derived chain is enough — no rpc-observer.ts change needed, the dispose contract in rpc-observer-dispose.it.test.ts is correct and 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
settleTeardownsteps are now named ({name, run, timeoutMs?}) and each wrapped inp-timeout(20s default), so a wedgedcloseBrowser/close/stopnames itself in the rejection instead of surfacing as vitest's generic 30shookTimeoutfailure, and the sibling steps still settle inside the hook budget (Promise.allSettledalready made them failure-independent since fix(test-teardown): harden playwright suite teardown against CI leaks #412; this bounds and names each one).rpcObserver.dispose()cleared itswaitersSet but never cancelled the pendingsetTimeouteachbuffered()call schedules. Under@playwright/test's single-worker-per-file execution (post test(embed): move integration tests onto @playwright/test under tests/e2e — the runner owns page lifecycle (#389) #415), that timer could outlive its page/observer and fire during a later, unrelated test, misattributing a timeout to the wrong test — the root cause behind the original Flaky: recorder useRecorderTestApi afterAll dispose exceeds 30s hookTimeout, fails the file while all tests pass #361 symptom surfacing again from the recorder migration.dispose()now fails every pending waiter (which internallyclearTimeouts) before clearing the set.openObservedPage(used by bothlaunch()'s primary page andsecondClient()) now wiresobserver.dispose()to the page's own'close'event, so observers created for testkit pages are always disposed by their natural owner instead of only being disposed manually by ad-hoc call sites likeconnect-handshake.ts.p-timeout(already used elsewhere in the workspace, e.g.@conciv/browser-fixture) as a direct dependency of@conciv/extension-testkit.Test plan
packages/extension-testkit/test/settle-teardown.test.ts— unit test proving a wedged, named step rejects withtestkit <name> exceeded <ms>mswhile sibling steps still complete.packages/extension-testkit/test/rpc-observer-dispose.it.test.ts— real-browser test provingdispose()fails a pending waiter immediately (well under its own natural timeout) instead of leaving the timer to fire later, and that closing alaunch()-opened page disposes its observer and cancels pending waits.@conciv/extension-testkittypecheck (tsc -p tsconfig.json --noEmitvia turbo) — green.@conciv/extension-testkitfull test suite (TURBO_CONCURRENCY=1 VITEST_MAX_FORKS=1 turbo run test --concurrency=1) — 9 files / 31 tests green.@conciv/extension-recordertypecheck + lint (its test suite is CI-only, not run locally) — green.pnpm lint&&pnpm format:check— green (one pre-existing, unrelatedoxfmtfinding indocs/assets/page-session-card-spike/fake-form.htmlfrom main, not touched by this PR).pnpm exec fallow audit --changed-since origin/main --format json --quiet— verdictpass, 0 introduced findings (9 pre-existing/inherited unused-dependency findings, unrelated to this diff).Refs #361, epic #409.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests