Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/playwright-core/src/tools/backend/navigate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,11 @@ const navigate = defineTool({

handle: async (context, params, response) => {
const tab = await context.ensureTab();
const url = await tab.checkUrlAndNavigate(params.url);

// Always include the snapshot, even when navigation fails: navigating to a
// page that closes the last tab (e.g. chrome://extensions/) rejects goto,
// and we still want to render the resulting tab state gracefully.
response.setIncludeSnapshot();
const url = await tab.checkUrlAndNavigate(params.url);
Comment on lines +35 to +39

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.

if checkUrlAndNavigate rejects, handle throws and browserBackend.callTool returns the raw error via formatError without ever calling response.serialize(), so the snapshot flag never gets read

i think the other change is what's actually fixing the flake, so id either drop this or fix the comment

response.addCode(`await page.goto('${url}');`);
},
});
Expand Down
9 changes: 7 additions & 2 deletions packages/playwright-core/src/tools/backend/response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -270,8 +270,13 @@ export class Response {
addSection('Ran Playwright code', this._code, 'js');

// Render tab titles upon changes or when more than one tab.
const tabSnapshot = this._context.currentTab() ? await this._context.currentTabOrDie().captureSnapshot(this._includeSnapshotRoot, this._includeSnapshotDepth, this._includeSnapshotBoxes, this._clientWorkspace) : undefined;
const tabHeaders = await Promise.all(this._context.tabs().map(tab => tab.headerSnapshot()));
const currentTab = this._context.currentTab();
const tabSnapshot = currentTab && !currentTab.page.isClosed() ? await currentTab.captureSnapshot(this._includeSnapshotRoot, this._includeSnapshotDepth, this._includeSnapshotBoxes, this._clientWorkspace) : undefined;

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.

NIT: isClosed() closes most of the window, but the page can still close mid-captureSnapshot/headerSnapshot and reject, so i think a .catch() would be good here

// A navigation may have closed the last tab (e.g. chrome://extensions/); the
// 'close' event that removes it from the tab list is async, so ignore tabs
// whose page has already closed to render a stable tab state.
const openTabs = this._context.tabs().filter(tab => !tab.page.isClosed());
const tabHeaders = await Promise.all(openTabs.map(tab => tab.headerSnapshot()));
if (this._includeSnapshot !== 'none' || tabHeaders.some(header => header.changed)) {
if (tabHeaders.length !== 1)
addSection('Open tabs', renderTabsMarkdown(tabHeaders));
Expand Down
Loading