Skip to content

Refactor browser element selection to be event-based#315362

Merged
kycutler merged 2 commits into
mainfrom
kycutler/selectevents
May 8, 2026
Merged

Refactor browser element selection to be event-based#315362
kycutler merged 2 commits into
mainfrom
kycutler/selectevents

Conversation

@kycutler
Copy link
Copy Markdown
Contributor

@kycutler kycutler commented May 8, 2026

No description provided.

Copilot AI review requested due to automatic review settings May 8, 2026 20:42
@kycutler kycutler self-assigned this May 8, 2026
Copy link
Copy Markdown
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

Refactors Integrated Browser element selection from a request/cancellation-token flow into a model/service event-based flow, so element picks are delivered via events and selection is toggled explicitly.

Changes:

  • Replace getElementData / getFocusedElementData RPCs with toggleElementSelection plus onDidSelectElement / onDidChangeElementSelectionActive events.
  • Wire the workbench chat integration to listen for element-selection events and attach picked elements to chat.
  • Add main-process handling to pick the focused element via Ctrl/Cmd+Enter while element selection is active, and persist selection-active state in view state.
Show a summary per file
File Description
src/vs/workbench/contrib/browserView/electron-browser/features/browserEditorChatFeatures.ts Switches chat attachment to react to element-selection events; toggles selection via model API.
src/vs/workbench/contrib/browserView/common/browserView.ts Updates IBrowserViewModel/BrowserViewModel to expose selection state and selection events, and to toggle selection via the service.
src/vs/platform/browserView/electron-main/browserViewMainService.ts Removes cancellable element-data RPCs and exposes dynamic events + toggleElementSelection to renderer via IPC.
src/vs/platform/browserView/electron-main/browserViewElementInspector.ts Introduces selection-active state and emits events for selected elements and selection-active changes; adds focused-element pick API.
src/vs/platform/browserView/electron-main/browserView.ts Intercepts Ctrl/Cmd+Enter during selection to pick the focused element; exposes inspector publicly and includes selection state in getState().
src/vs/platform/browserView/common/browserView.ts Extends service/state contracts with selection-active state, selection events, and toggleElementSelection.

Copilot's findings

Comments suppressed due to low confidence (3)

src/vs/platform/browserView/electron-main/browserViewElementInspector.ts:149

  • toggleElementSelection sets up _selectionStore and an onEvent listener before calling Overlay.setInspectMode, but if sendCommand throws (e.g. CDP connection drops) the method will reject without disposing the store or resetting state. This can leak the event listener and leave _selectionStore populated while isElementSelectionActive remains false. Consider wrapping the sendCommand + state flip in a try/catch that disposes _selectionStore (and resets it to undefined) on failure, or moving the sendCommand into a try/finally cleanup path.
		// Start selection
		const connection = await this._connectionPromise;

		// Clean up any prior selection state
		this._selectionStore?.dispose();
		const store = this._selectionStore = new DisposableStore();

		store.add(connection.onEvent(async (event) => {
			if (event.method !== 'Overlay.inspectNodeRequested') {
				return;
			}

			const params = event.params as { backendNodeId: number };
			if (!params?.backendNodeId) {
				return;
			}

			try {
				const nodeData = await extractNodeData(connection, { backendNodeId: params.backendNodeId });
				this._onDidSelectElement.fire({
					...nodeData,
					url: this.browser.getURL()
				});
				await this._stopElementSelection();
			} catch {
				// Best effort - selection continues
			}
		}));

		await connection.sendCommand('Overlay.setInspectMode', {
			mode: 'searchForNode',
			highlightConfig: inspectHighlightConfig,
		});

		this._elementSelectionActive = true;
		this._onDidChangeElementSelectionActive.fire(true);
	}

src/vs/platform/browserView/electron-main/browserViewElementInspector.ts:199

  • pickFocusedElement fires onDidSelectElement but does not disable element selection afterwards, unlike the click-pick path which calls _stopElementSelection(). This leaves the inspect overlay (and context key) active after Ctrl/Cmd+Enter, which is inconsistent with the “auto disabled after the first pick” behavior and the previous flow where accepting ended selection. Consider stopping selection after firing the event (and also ensuring errors from CDP calls don’t surface as unhandled rejections).
	/**
	 * Fire a selection event for the currently focused element.
	 * Only effective when element selection is active.
	 */
	async pickFocusedElement(): Promise<void> {
		if (!this._elementSelectionActive) {
			return;
		}

		const connection = await this._connectionPromise;

		await connection.sendCommand('Runtime.enable');
		const { result } = await connection.sendCommand('Runtime.evaluate', {
			expression: 'document.activeElement',
			returnByValue: false,
		}) as { result: { objectId?: string } };

		if (!result?.objectId) {
			return;
		}

		const nodeData = await extractNodeData(connection, { objectId: result.objectId });
		this._onDidSelectElement.fire({
			...nodeData,
			url: this.browser.getURL()
		});
	}

src/vs/workbench/contrib/browserView/electron-browser/features/browserEditorChatFeatures.ts:418

  • In AddElementToChatAction.run, toggleElementSelection is started with void ... which detaches it from the command execution. If the promise rejects, it will be unhandled and the command infrastructure won’t be able to report the error. Prefer await (or .catch with logging) so failures are handled consistently.
	async run(accessor: ServicesAccessor, browserEditor = accessor.get(IEditorService).activeEditorPane): Promise<void> {
		if (browserEditor instanceof BrowserEditor) {
			browserEditor.ensureBrowserFocus();
			void browserEditor.model?.toggleElementSelection(undefined);
		}
  • Files reviewed: 6/6 changed files
  • Comments generated: 3

Comment thread src/vs/platform/browserView/electron-main/browserViewElementInspector.ts Outdated
Comment thread src/vs/platform/browserView/electron-main/browserView.ts
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@kycutler kycutler marked this pull request as ready for review May 8, 2026 21:18
@kycutler kycutler enabled auto-merge (squash) May 8, 2026 21:18
@vs-code-engineering
Copy link
Copy Markdown
Contributor

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

@jruales

Matched files:

  • src/vs/platform/browserView/common/browserView.ts
  • src/vs/platform/browserView/electron-main/browserView.ts
  • src/vs/platform/browserView/electron-main/browserViewElementInspector.ts
  • src/vs/platform/browserView/electron-main/browserViewMainService.ts
  • src/vs/workbench/contrib/browserView/common/browserView.ts
  • src/vs/workbench/contrib/browserView/electron-browser/features/browserEditorChatFeatures.ts

@kycutler kycutler merged commit 0e2919b into main May 8, 2026
25 checks passed
@kycutler kycutler deleted the kycutler/selectevents branch May 8, 2026 21:19
@vs-code-engineering vs-code-engineering Bot added this to the 1.120.0 milestone May 8, 2026
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.

3 participants