Skip to content

fix(mcp): await init page before screenshot#41646

Merged
yury-s merged 4 commits into
microsoft:mainfrom
harshmathurx:fix-1671
Jul 8, 2026
Merged

fix(mcp): await init page before screenshot#41646
yury-s merged 4 commits into
microsoft:mainfrom
harshmathurx:fix-1671

Conversation

@harshmathurx

@harshmathurx harshmathurx commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • wait for MCP tab initialization before taking page screenshots
  • add a regression test for first-call browser_take_screenshot with --init-page

Fixes #41650

@harshmathurx harshmathurx mentioned this pull request Jul 6, 2026
Closed
pavelfeldman
pavelfeldman reviewed Jul 6, 2026
View reviewed changes
Comment thread packages/playwright-core/src/tools/backend/screenshot.ts Outdated
const screenshotTargetLabel = params.target ? params.element || 'element' : (params.fullPage ? 'full page' : 'viewport');
const target = params.target ? await tab.targetLocator({ element: params.element, target: params.target }) : null;
if (!target)
await tab.waitForInitialized();

@pavelfeldman pavelfeldman Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it should be earlier in the pipeline, most likely in the context.ensureTab

@harshmathurx harshmathurx Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call — ensureTab is the right place. That way all tools (snapshot, evaluate, screenshot, etc.) get the guarantee automatically instead of each one having to remember to await init. I will move the waitForInitialized() call into context.ensureTab() and keep the method on Tab for direct use when needed.

@harshmathurx
fix(mcp): move init-page wait from screenshot to ensureTab
60bd1c8 
Per review feedback, await tab.waitForInitialized() in
context.ensureTab() so all tools benefit from waiting for
init-page scripts to complete, not just screenshot.

Keep waitForInitialized() on Tab for direct use when needed.
@harshmathurx

harshmathurx commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Moved waitForInitialized() from screenshot.ts into context.ensureTab() — now all tools (snapshot, evaluate, etc.) automatically wait for init-page scripts, not just screenshot. Kept the waitForInitialized() method on Tab for any direct use.

@harshmathurx

harshmathurx commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@github-actions

This comment has been minimized.

Sign in to view
@harshmathurx

harshmathurx commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@yury-s CI follow-up: the previous red checks looked unrelated to this patch (Lint snippets hit npm ci / ECONNRESET, and MCP had a single macOS annotate failure). I don't have repo permissions to re-run failed jobs directly, so I pushed a no-op chore: retrigger ci commit to refresh the checks. GitHub Actions now appears to be waiting for maintainer approval on the forked commit — could you approve the workflow run when you get a chance?

@github-actions

This comment has been minimized.

Sign in to view
@harshmathurx

harshmathurx commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

The remaining red MCP job looks unrelated to this PR (tests/mcp/annotate.spec.ts timed out on macOS Chrome while waiting for the dashboard endpoint). I tried to rerun the failed job, but GitHub requires admin rights on the repo. I pushed an empty chore: retrigger ci commit, and the new workflow runs are now waiting for maintainer approval because this is a forked PR.

Could someone approve/rerun the workflows when convenient?

@harshmathurx

harshmathurx commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@yury-s sorry for the extra ping — the refreshed MCP workflow is running now, but one unrelated shard has gone red again (ubuntu-latest - webkit) while the rest are still in progress. Could you rerun the failed MCP shard once this run settles?

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Test results for "MCP"

4 failed
❌ [chrome] › mcp/annotate.spec.ts:230 › should capture annotations via show --annotate @mcp-windows-latest-chrome
❌ [chrome] › mcp/cli-navigation.spec.ts:56 › goto chrome:// page that closes the tab does not crash the response @mcp-windows-latest-chrome
❌ [firefox] › mcp/annotate.spec.ts:270 › should start dashboard and annotate when no dashboard is running @mcp-windows-latest-firefox
❌ [webkit] › mcp/cli-killall.spec.ts:42 › kill-all kills filtered dashboard pid @mcp-ubuntu-latest-webkit

7731 passed, 1248 skipped

Merge workflow run.

@yury-s yury-s merged commit a9ae10f into microsoft:main Jul 8, 2026
15 of 18 checks passed
This was referenced Jul 9, 2026
Merged
Merged
Merged
@JustasMonkev JustasMonkev mentioned this pull request Jul 10, 2026
Open
@github-actions github-actions Bot mentioned this pull request Jul 10, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pavelfeldman pavelfeldman pavelfeldman approved these changes
@yury-s yury-s yury-s approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: --init-page hook races the first tool call -- Tab _initialize() is fire-and-forget and browser_take_screenshot does not await it

3 participants

@harshmathurx @pavelfeldman @yury-s