Browser: skip unused screenshot after load failure - #333737
Draft
Alexandru Dima (alexdima) wants to merge 2 commits into
Draft
Browser: skip unused screenshot after load failure#333737Alexandru Dima (alexdima) wants to merge 2 commits into
Alexandru Dima (alexdima) wants to merge 2 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Alexandru Dima (alexdima)
requested review from
Kyle Cutler (kycutler)
and
a balanced review from Copilot
September 1, 2026 09:21
Copilot started reviewing on behalf of
Alexandru Dima (alexdima)
September 1, 2026 09:22
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/vs/workbench/contrib/browserView/electron-browser/features/webContentsViewRendererFeature.ts — This guard only suppresses _refresh's immediate capture. _doScreenshot() also reschedules… |
What changed in this PR
Prevents unnecessary browser placeholder screenshots after load failures or empty-page states.
Changes:
- Gates hidden-state screenshot capture on placeholder availability.
| File | Description |
|---|---|
webContentsViewRendererFeature.ts |
Skips capture when the placeholder is inactive. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.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
Avoid capturing an internal browser placeholder screenshot after a page has entered a load-error state.
Healthy pages still capture a screenshot before the native
WebContentsViewis hidden for an overlapping workbench overlay or editor visibility change. Failed or empty pages skip the capture because their placeholder is not rendered.Evidence
I found this sequence while scanning recent Code - Insiders renderer logs:
The screenshot errors occur immediately after the failed navigation.
Root cause
WebContentsViewRendererFeature._refresh()already determines whether its HTML screenshot placeholder is useful:A failed main-frame load populates
model.error. That makes bothplaceholderActiveand_shouldShowPage()false: the editor transitions to its error UI, and the native browser view should be hidden.Before this change, every transition into the hidden state called
_doScreenshot()unconditionally. This requested a screenshot even though the placeholder was disabled and the result could not be displayed. During failed-navigation teardown, that request reached Electron after the underlying capture target was no longer available and produced thecapturePageerror above.Change
Make
_doScreenshot()clear its pending timer, then return unless the model has a URL, has no load error, and remains visible. This centralizes the rule for refreshes, model attachment, visibility events, and recurring timer callbacks.This preserves the intended screenshot swap for healthy pages while avoiding an unused capture for failed or empty pages. The existing deferred
setVisible(false)behavior is unchanged.Review focus
Kyle Cutler (@kycutler), could you please confirm that skipping the renderer-owned placeholder capture is the intended boundary here? In particular, is there any failed-load state in which the browser error UI still needs the last successful page screenshot, or would you prefer additional hardening in the main-process capture path?
This is a draft because the exact external URL failure has not yet been reproduced against the patched build. The change is based on the logged event ordering and the existing placeholder/error-state contract.
Validation
npm run eslint -- src/vs/workbench/contrib/browserView/electron-browser/features/webContentsViewRendererFeature.tsgit diff --check origin/main...HEAD