Skip to content

Conversation

@kycutler
Copy link
Contributor

@kycutler kycutler commented Nov 20, 2025

Resolves #277298, #285151

Copilot AI review requested due to automatic review settings November 20, 2025 23:54
@kycutler kycutler self-assigned this Nov 20, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces an integrated browser feature for VS Code desktop, allowing users to browse web content directly within the editor using Electron's WebContentsView API. The implementation follows VS Code's layered architecture with platform services in the electron-main process communicating with workbench UI components via IPC.

Key changes include:

  • Implementation of a browser editor pane with navigation controls and URL input
  • Platform service for managing browser views in the main process
  • Overlay detection system to hide browser views when VS Code UI elements overlap
  • Theme synchronization between VS Code and web content

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 19 comments.

Show a summary per file
File Description
src/vs/workbench/workbench.desktop.main.ts Registers the browser contribution module
src/vs/workbench/contrib/browser/electron-browser/browser.contribution.ts Registers editor pane, serializer, commands, and service
src/vs/workbench/contrib/browser/electron-browser/browserEditor.ts Main editor implementation with toolbar, navigation, and overlay handling
src/vs/workbench/contrib/browser/electron-browser/browserEditorInput.ts Editor input model tracking URL, title, favicon, and loading state
src/vs/workbench/contrib/browser/electron-browser/browserEditorSerializer.ts Serializer for persisting browser tabs across sessions
src/vs/workbench/contrib/browser/electron-browser/overlayManager.ts Service detecting UI overlays to manage browser view visibility
src/vs/workbench/contrib/browser/electron-browser/media/browser.css Styling for browser UI including toolbar and blur effects
src/vs/platform/browserView/common/browserView.ts Service interface and type definitions for browser view operations
src/vs/platform/browserView/electron-main/browserViewMainService.ts Main process service managing WebContentsView lifecycle and events
src/vs/platform/browserView/electron-main/plugins/themePlugin.ts Plugin synchronizing VS Code theme with web content
src/vs/code/electron-main/app.ts Integrates browser view service and allows navigation in browser views
src/vs/base/common/network.ts Adds vscode-browser URI scheme
src/vs/base/common/keyCodes.ts Adds scan code to event key code mapping for keyboard handling

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 17 comments.

kycutler and others added 4 commits November 26, 2025 16:26
@kycutler kycutler marked this pull request as ready for review December 9, 2025 20:04
@kycutler kycutler requested a review from bpasero December 9, 2025 20:14
Copy link
Collaborator

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM for session, permission handling, browserview lifetime, webPreferences and storage cleanups

try {
this._lastFavicon = await request;
this._onDidChangeFavicon.fire({ favicon: this._lastFavicon });
// On success, leave the promise in the cache and stop looping
Copy link
Collaborator

Choose a reason for hiding this comment

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

why cache the promise instead of the response string ? Maybe I am missing something but _faviconRequestCache is populated and consumed in the same suspended function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's so we don't make duplicate requests if the event fires again before the favicon finishes loading

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I see the cache is only used for a given BrowserView (not shared across BrowserViews), the event will only be emitted when the main page of the BrowserView stopped loading (it won't be emitted for child frame navigations). So in that case wouldn't it be enough to cache the response rather ? Why would the event re-enter before it has finished processing the requests it is waiting on ?

bpasero
bpasero previously approved these changes Dec 19, 2025
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

@kycutler I went over the changes high level and think we can go ahead and merge this in, provided all feedback from @deepak1556 is resolved. I am mainly looking at how well it integrates into our contributions story and it seems to do that well, following how other components do this. This is a non trivial change considering that it also touches on electron-main but I do not see red flags from my basic review 🙏

As you might have seen, EditorInput has some methods (some optional) to drive how it behaves, so maybe check over the API here to see if you want to adopt more things (close handler e.g. is used to bring up a dialog when you close, not sure this ever applies):

export abstract class EditorInput extends AbstractEditorInput {

Here are things to consider next based on my testing:

  • the loading animation for browser editor tab seem somewhat non-standard compared to our usual spinner. maybe you can get help from @mrleemurray on this and see if we could make it consistent: [1]

  • I cannot drag the browser tab over itself to split the editor group into half, something I can do with normal editor tabs. I am not sure how you decide to show the overlay today, but I assume for DND it would need to also be shown: [2]

  • you are duplicating the editor toolbar with specific actions and I wonder if these should move into the editor toolbar for consistency, maybe at least on the right hand side? see also how they are not well aligned: [3]

  • I find the blurry overlay not matching our usual VS Code style, could we just simply not blur it? For a user it might come at a surprise because other editors are not blurry like that and it does not really seem to add much value?: [4]

[1]
https://github.com/user-attachments/assets/3c16e392-d5a1-4f46-8cb4-6682b3688870

[2]
https://github.com/user-attachments/assets/dcce5152-4c6c-404a-ad32-7610460827d4

[3]
image

[4]
image

@kycutler
Copy link
Contributor Author

(close handler e.g. is used to bring up a dialog when you close, not sure this ever applies)

I do intend to support preventing close eventually -- it's just a bit tricky because we have to limit how much say the page has over this to prevent abuse. So for now we just always close the page.

  • I cannot drag the browser tab over itself to split the editor group into half, something I can do with normal editor tabs. I am not sure how you decide to show the overlay today, but I assume for DND it would need to also be shown: [2]
  • I find the blurry overlay not matching our usual VS Code style, could we just simply not blur it? For a user it might come at a surprise because other editors are not blurry like that and it does not really seem to add much value?: [4]

These are tricky since the view is overlayed with the workbench, so we have to do dynamic show/hide.

  • For drag / drop, there is the scenario where user wants to drag some content into (or within) the browser window. We could simply prevent this and hide the overlay on drag over, forcing the user to drop into the workbench. But I worry this will limit valid user scenarios. Sadly I think it comes down to one annoying behavior or another. Perhaps we could add a setting to toggle it.
  • My intent with the blurring is to make it clear that it isn't interactable and may not be fully up-to-date (or very high quality since we use compressed JPEG screenshots to minimize data transfer). I plan to take this to UI sync in January for refinement.

if (eventKeyCode) {
EVENT_KEY_CODE_MAP[eventKeyCode] = keyCode;
}
if (scanCodeStr) {
Copy link
Member

Choose a reason for hiding this comment

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

for such a low level change, it'd be good to have a test.

/**
* Helper for creating and parsing browser view URIs.
*/
export namespace BrowserViewUri {
Copy link
Member

Choose a reason for hiding this comment

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

This also seems like a good class for unit tests

@bpasero
Copy link
Member

bpasero commented Dec 20, 2025

My intent with the blurring is to make it clear that it isn't interactable and may not be fully up-to-date (or very high quality since we use compressed JPEG screenshots to minimize data transfer). I plan to take this to UI sync in January for refinement.

@kycutler I would ship this first without the blur effect and only do it if the feedback is pointing into this direction by many people 🙏 . But a UX sync sounds good on this.

@kycutler kycutler modified the milestones: December 2025, January 2026 Jan 5, 2026
@kycutler kycutler merged commit f85cf0a into main Jan 8, 2026
37 of 54 checks passed
@kycutler kycutler deleted the kycutler/rich-browser branch January 8, 2026 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exploration: rich integrated web browser

8 participants