feat: adopt an existing browser/page instead of launching a new one#2
feat: adopt an existing browser/page instead of launching a new one#2felixtrz wants to merge 1 commit into
Conversation
Add an opt-in way to run a snippet in a browser/page the caller already owns, so a host (e.g. a dev server with a visible, instrumented tab) can run a vitexec check in that same browser — no second window. Fully backward compatible: with none of the new options, behavior is unchanged. API: new RunVitexecOptions `page`, `adoptPage` (a thunk resolved AFTER the Vite server is listening, so a host that launches a browser in reaction to the server can hand it over), `context`, `browser`, and `keepBrowserOpen`; new export `runVitexecOnPage()`; new CLI flag/env `--keep-browser-open` / VITEXEC_KEEP_BROWSER_OPEN. Internals: split browser acquisition from context/page creation and gate teardown on ownership — vitexec only closes handles it created itself, and always closes its own Vite server. A single idempotent `__vitexecReport` binding per page allows reusing one adopted page across sequential runs. When adopting an already-loaded page the injected runtime is 'armed' so the snippet runs exactly once (on vitexec's navigation, after the report hook is bound), never on the page's prior load. Tests: page/context/browser adoption, sequential reuse, and runVitexecOnPage.
📝 WalkthroughWalkthroughThis PR adds page adoption and keep-browser-open support to vitexec. Callers can now supply existing Playwright pages, contexts, or browsers to vitexec, which tracks ownership and avoids closing adopted resources. The ChangesPage Adoption and Keep-Browser-Open
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da367ca8e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await page.addInitScript((armId) => { | ||
| (globalThis as Record<string, unknown>).__vitexecArm = armId; | ||
| }, id); |
There was a problem hiding this comment.
When the same adopted Page is used for multiple sequential runs, this installs another init script each time and there is no removal before the next navigation. All prior scripts will continue to run on future navigations and each writes a different __vitexecArm; since Playwright does not guarantee ordering for multiple init scripts, an older run's id can end up being the value checked by the injected runtime, causing the current snippet to be skipped and the run to time out. Use a single stable init script that reads the current run id from mutable state instead of appending a per-run script.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vitexec/src/cli.ts (1)
413-423:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the page vitexec creates inside an adopted context.
When
options.contextis provided, vitexec opens a fresh page and marksownsPage = true, but this cleanup path only closes owned contexts/browsers. Reusing one caller-owned context across runs will keep leaking tabs.Possible fix
if (page) { VITEXEC_REPORT_RESOLVERS.delete(page); page.off("console", collectConsoleLog); page.off("pageerror", onPageError); page.off("requestfailed", onRequestFailed); page.off("response", onResponse); page.off("framenavigated", onFrameNavigated); } if (cdp) await cdp.detach().catch(() => undefined); + if (ownsPage && !ownsContext && page && !page.isClosed()) { + await page.close().catch(() => undefined); + } if (ownsContext && context) { await context.close().then( () => { if (options.networkTracePath && !signal.aborted) { log(`[network-trace] ${options.networkTracePath}`);🤖 Prompt for 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. In `@packages/vitexec/src/cli.ts` around lines 413 - 423, The cleanup currently only closes owned contexts and browsers, leaking pages when vitexec created a page in a caller-provided context; add a branch to close the owned page: when ownsPage is true and page exists, await page.close().catch(() => undefined) (before or independent of closing the context), so reference and update the cleanup around ownsContext/context and ownsBrowser/browser/keepOpen to also handle ownsPage/page closure.
🤖 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/vitexec/src/index.ts`:
- Around line 76-93: The armed guard in runtimeScript only checks
globalThis.__vitexecArm (which resets on navigation) so imports can run once per
navigation; change runtimeScript to persist the armed state across reloads
(e.g., use sessionStorage/localStorage keyed by the id) by checking both
globalThis.__vitexecArm and a persistent flag (set the persistent flag before
performing the import and clear/leave it after success/failure), reference
runtimeScript, globalThis.__vitexecArm, globalThis.__vitexecRuns and the
codeRouteForBase(...) import expression when making the check and when setting
the persistent marker so the import truly runs exactly once across reloads.
---
Outside diff comments:
In `@packages/vitexec/src/cli.ts`:
- Around line 413-423: The cleanup currently only closes owned contexts and
browsers, leaking pages when vitexec created a page in a caller-provided
context; add a branch to close the owned page: when ownsPage is true and page
exists, await page.close().catch(() => undefined) (before or independent of
closing the context), so reference and update the cleanup around
ownsContext/context and ownsBrowser/browser/keepOpen to also handle
ownsPage/page closure.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4265871-67f5-4387-a602-7668832a84a5
📒 Files selected for processing (4)
README.mdpackages/vitexec/src/cli.tspackages/vitexec/src/index.tspackages/vitexec/test/cli.test.ts
| function runtimeScript(id: string, base: string, armed: boolean): string { | ||
| const run = `globalThis.__vitexecRuns[${JSON.stringify(id)}] = import(${JSON.stringify(codeRouteForBase(base))} + "/" + ${JSON.stringify(encodeURIComponent(id))}) | ||
| .catch(console.error) | ||
| .finally(() => globalThis.__vitexecReport?.());`; | ||
|
|
||
| if (!armed) { | ||
| return ` | ||
| globalThis.__vitexecRuns ??= {}; | ||
| ${run} | ||
| `; | ||
| } | ||
|
|
||
| return ` | ||
| globalThis.__vitexecRuns ??= {}; | ||
| globalThis.__vitexecRuns[${JSON.stringify(id)}] = import(${JSON.stringify(codeRouteForBase(base))} + "/" + ${JSON.stringify(encodeURIComponent(id))}) | ||
| .catch(console.error) | ||
| .finally(() => globalThis.__vitexecReport?.()); | ||
| if (globalThis.__vitexecArm === ${JSON.stringify(id)}) { | ||
| ${run} | ||
| } | ||
| `; |
There was a problem hiding this comment.
Persist the armed guard across reloads.
On adopted pages this only guards the current document. If the page reloads during the run, globalThis.__vitexecArm is reinitialized and the same snippet imports again, so the "exactly once" contract turns into "once per navigation".
Possible fix
function runtimeScript(id: string, base: string, armed: boolean): string {
const run = `globalThis.__vitexecRuns[${JSON.stringify(id)}] = import(${JSON.stringify(codeRouteForBase(base))} + "/" + ${JSON.stringify(encodeURIComponent(id))})
.catch(console.error)
.finally(() => globalThis.__vitexecReport?.());`;
if (!armed) {
return `
globalThis.__vitexecRuns ??= {};
${run}
`;
}
return `
globalThis.__vitexecRuns ??= {};
-if (globalThis.__vitexecArm === ${JSON.stringify(id)}) {
+if (
+ globalThis.__vitexecArm === ${JSON.stringify(id)} &&
+ sessionStorage.getItem("__vitexecArmedRun") !== ${JSON.stringify(id)}
+) {
+ sessionStorage.setItem("__vitexecArmedRun", ${JSON.stringify(id)});
${run}
}
`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function runtimeScript(id: string, base: string, armed: boolean): string { | |
| const run = `globalThis.__vitexecRuns[${JSON.stringify(id)}] = import(${JSON.stringify(codeRouteForBase(base))} + "/" + ${JSON.stringify(encodeURIComponent(id))}) | |
| .catch(console.error) | |
| .finally(() => globalThis.__vitexecReport?.());`; | |
| if (!armed) { | |
| return ` | |
| globalThis.__vitexecRuns ??= {}; | |
| ${run} | |
| `; | |
| } | |
| return ` | |
| globalThis.__vitexecRuns ??= {}; | |
| globalThis.__vitexecRuns[${JSON.stringify(id)}] = import(${JSON.stringify(codeRouteForBase(base))} + "/" + ${JSON.stringify(encodeURIComponent(id))}) | |
| .catch(console.error) | |
| .finally(() => globalThis.__vitexecReport?.()); | |
| if (globalThis.__vitexecArm === ${JSON.stringify(id)}) { | |
| ${run} | |
| } | |
| `; | |
| function runtimeScript(id: string, base: string, armed: boolean): string { | |
| const run = `globalThis.__vitexecRuns[${JSON.stringify(id)}] = import(${JSON.stringify(codeRouteForBase(base))} + "/" + ${JSON.stringify(encodeURIComponent(id))}) | |
| .catch(console.error) | |
| .finally(() => globalThis.__vitexecReport?.());`; | |
| if (!armed) { | |
| return ` | |
| globalThis.__vitexecRuns ??= {}; | |
| ${run} | |
| `; | |
| } | |
| return ` | |
| globalThis.__vitexecRuns ??= {}; | |
| if ( | |
| globalThis.__vitexecArm === ${JSON.stringify(id)} && | |
| sessionStorage.getItem("__vitexecArmedRun") !== ${JSON.stringify(id)} | |
| ) { | |
| sessionStorage.setItem("__vitexecArmedRun", ${JSON.stringify(id)}); | |
| ${run} | |
| } | |
| `; | |
| } |
🤖 Prompt for 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.
In `@packages/vitexec/src/index.ts` around lines 76 - 93, The armed guard in
runtimeScript only checks globalThis.__vitexecArm (which resets on navigation)
so imports can run once per navigation; change runtimeScript to persist the
armed state across reloads (e.g., use sessionStorage/localStorage keyed by the
id) by checking both globalThis.__vitexecArm and a persistent flag (set the
persistent flag before performing the import and clear/leave it after
success/failure), reference runtimeScript, globalThis.__vitexecArm,
globalThis.__vitexecRuns and the codeRouteForBase(...) import expression when
making the check and when setting the persistent marker so the import truly runs
exactly once across reloads.
1 participant
Add an opt-in way to run a snippet in a browser/page the caller already owns, so a host (e.g. a dev server with a visible, instrumented tab) can run a vitexec check in that same browser — no second window. Fully backward compatible: with none of the new options, behavior is unchanged.
API: new RunVitexecOptions
page,adoptPage(a thunk resolved AFTER the Vite server is listening, so a host that launches a browser in reaction to the server can hand it over),context,browser, andkeepBrowserOpen; new exportrunVitexecOnPage(); new CLI flag/env--keep-browser-open/ VITEXEC_KEEP_BROWSER_OPEN.Internals: split browser acquisition from context/page creation and gate teardown on ownership — vitexec only closes handles it created itself, and always closes its own Vite server. A single idempotent
__vitexecReportbinding per page allows reusing one adopted page across sequential runs. When adopting an already-loaded page the injected runtime is 'armed' so the snippet runs exactly once (on vitexec's navigation, after the report hook is bound), never on the page's prior load.Tests: page/context/browser adoption, sequential reuse, and runVitexecOnPage.
This contribution is funded by Meta Platforms, Inc.
Summary by CodeRabbit
New Features
--keep-browser-openCLI flag andVITEXEC_KEEP_BROWSER_OPENenvironment variable to maintain browser sessions.keepBrowserOpenoption to programmatic API.Documentation
Tests