PositroNB extension API integration and debugging #9860

seeM · 2025-10-09T14:49:44Z

This PR integrates Positron notebook editors with the extension API (addresses #9440) and hooks up the notebook runtime debugging extension (#9251).

This is a rework of #9843. It turned out to be simplest to manage Positron notebook state in the same place as upstream state, otherwise we have to restore to more complicated state merging and conflicts.

Debug cell button position:

Release Notes

New Features

N/A

Bug Fixes

N/A

QA Notes

Nothing should break. You should be able to debug a cell in a Positro notebook editor.

@:notebooks

seeM · 2025-10-09T15:09:06Z

src/vs/workbench/contrib/notebook/browser/controller/coreActions.ts

+		if (notebookInstance) {
+			const id = toPositronNotebookCommand(this.desc.id);
+			if (id) {
+				return accessor.get(ICommandService).executeCommand(id);


This is an idea for how we could get compatibility with upstream commands. We could also get a bit smarter and translate and pass the arguments as well e.g. to reference cells.

oh interesting. Seems good to me.

cindyytong · 2025-10-09T15:12:12Z

Awesome!

For the icon can we line them up
Can we fix the spacing in between the icons to be consistent - i believe I mentioned this previously it might have been captured by something @nstrayer is working on already
Are we planning to add a shortcut for this? Can capture this in a separate ticket if needed
Do we have the ability to add a tooltip on hover to show i.e. Debug Cell (Shortcut if available)
We should add this for all the buttons => Can pull out to seaprate ticket if needed too

Can we see a video of what happens when you click the debug button

github-actions · 2025-10-09T15:28:53Z

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:notebooks

^readme ^{valid tags}

add/remove notebook instance events make `public textModel` observable connect positron notebook instances to mainThreadNotebookEditors.$tryShowNotebookDocument integrate positron notebooks with the extension api fix circular imports fix import fixes comment

seeM · 2025-10-10T14:37:21Z

@cindyytong Thanks for the review!

1, 2 and 3 done. The keyboard shortcut isn't working consistently, but I suspect it's the same issue @nstrayer mentioned elsewhere about context keys. There might still be some inconsistency with spacing -- I'm not sure how we want to handle the ellipsis and trash can icons
Also fixed the clipping of the editor "gutter" (on the left) and cell selection bar (see video below), although this has now pushed the code contents 7 pixels to the right.
Can we track tooltips separately? @rodrigosf672 brought this up as well and may have already filed
Video of a simple debugging case:
Shouldn't be hard to change the debugging UX details now that all the infrastructure is setup.

Screen.Recording.2025-10-10.at.16.11.43.mov

cindyytong · 2025-10-10T16:12:16Z

LGTM I don't see a ticket for tooltips so i will file separately

rodrigosf672 · 2025-10-10T16:48:42Z

LGTM I don't see a ticket for tooltips so i will file separately

@cindyytong and @seeM, I added the issue in #9896. Please feel free to make changes or add anything you want there.

cindyytong · 2025-10-12T20:10:42Z

Sorry for the late comment, I just noticed this when looking through version:

Positron Version: 2025.11.0 (Universal) build 50
Code - OSS Version: 1.104.0
Commit: a3249405df14df4c023426de4b587345bda0ac7f

Hovering over a breakpoint shows a modal that gets cut off. Does this live within the cell? Perhaps it can be overlaid ontop of the cell.

nstrayer

A few comments on code style etc but in general I think the approach is good. Much nicer to have to just have to match the MainThreadNotebookEditorsShape interface.

nstrayer · 2025-10-14T19:55:09Z

src/vs/workbench/api/browser/positron/mainThreadPositronNotebookEditors.ts

@@ -0,0 +1,227 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (C) 2024-2025 Posit Software, PBC. All rights reserved.


Suggested change

* Copyright (C) 2024-2025 Posit Software, PBC. All rights reserved.

* Copyright (C) 2025 Posit Software, PBC. All rights reserved.

nstrayer · 2025-10-14T19:55:44Z

build/lib/stylelint/vscode-known-variables.json

 		"--positron-data-grid-column-header-sort-index-font-weight",
-		"--positron-data-grid-column-header-title-font-weight"
+		"--positron-data-grid-column-header-title-font-weight",
+		"--positron-notebook-selection-bar-width"


If the variable is not used for themeing you can avoid having to declare here with the format --_positron-notebook-selection-bar-width

nstrayer · 2025-10-14T19:57:04Z

src/vs/workbench/api/browser/positron/mainThreadPositronNotebookEditors.ts

+/**
+ * Represents a PositronNotebookInstance on the main thread.
+ */
+export class MainThreadPositronNotebookInstance extends Disposable {
+	private readonly _onDidChangeProperties = this._register(new Emitter<INotebookEditorPropertiesChangeData>());
+
+	/** Event that fires when the notebook editor properties change */
+	readonly onDidChangeProperties = this._onDidChangeProperties.event;
+
+	constructor(
+		private readonly _instance: IPositronNotebookInstance,
+	) {
+		super();
+
+		// Fire an event when selections change
+		this._register(runOnChange(this._instance.selectionStateMachine.state, (state) => {
+			const selections = this.getSelections(state);
+			this._onDidChangeProperties.fire({ selections: { selections } });
+		}));
+
+		// TODO: Fire an event when visible ranges change
+		// this._onDidChangeProperties.fire({ visibleRanges: { ranges: [] } });
+	}
+
+	getId(): string {
+		return this._instance.id;
+	}
+
+	getDocumentUri(): UriComponents {
+		return this._instance.uri;
+	}
+
+	getViewType(): string {
+		return this._instance.textModel.get()?.viewType ?? 'jupyter-notebook';
+	}
+
+	getSelections(state?: SelectionStates): ICellRange[] {
+		// TODO: Double check this
+		state = state ?? this._instance.selectionStateMachine.state.get();
+		const selectedCells = getSelectedCells(state);
+
+		if (selectedCells.length === 0) {
+			return [];
+		}
+
+		// Group consecutive cells into ranges
+		const ranges: ICellRange[] = [];
+		let currentStart = selectedCells[0].index;
+		let currentEnd = currentStart + 1;
+
+		for (let i = 1; i < selectedCells.length; i++) {
+			const cellIndex = selectedCells[i].index;
+			if (cellIndex === currentEnd) {
+				// Consecutive cell, extend the range
+				currentEnd++;
+			} else {
+				// Non-consecutive, save current range and start new one
+				ranges.push({ start: currentStart, end: currentEnd });
+				currentStart = cellIndex;
+				currentEnd = currentStart + 1;
+			}
+		}
+
+		// Add the last range
+		ranges.push({ start: currentStart, end: currentEnd });
+
+		return ranges;
+	}
+
+	getVisibleRanges(): ICellRange[] {
+		// For now, return all cells as visible if we have a container
+		// TODO: Implement actual viewport calculation based on scroll position
+		if (this._instance.cellsContainer && this._instance.cells.get().length > 0) {
+			return [{ start: 0, end: this._instance.cells.get().length }];
+		}
+		return [];
+	}
+
+	setSelections(selections: readonly ICellRange[]): void {
+		// TODO: Implement set selections
+	}
+
+	revealRange(range: ICellRange, revealType: NotebookEditorRevealType): void {
+		// TODO: Implement reveal range
+	}
+
+	matches(editor: IEditorPane): boolean {
+		return editor instanceof PositronNotebookEditor && editor.notebookInstance === this._instance;
+	}
+}


Could we just make the notebook instance conform to this interface and avoid a secondary class?

nstrayer · 2025-10-14T19:59:26Z

src/vs/workbench/api/browser/mainThreadNotebookDocumentsAndEditors.ts

+		Since map values are never referenced, we can retype as unknown and include Positron notebook instances
 		readonly visibleEditors: Map<string, IActiveNotebookEditor>
+		*/
+		readonly visibleEditors: Map<string, unknown>


Big fan of use of unknown here. Vastly underutilized type.

nstrayer · 2025-10-14T20:02:59Z

src/vs/workbench/api/browser/mainThreadNotebookDocumentsAndEditors.ts

+		for (const instance of this._positronNotebookService.listInstances()) {
+			// Don't add the instance until it has a text model, otherwise it'll get
+			// dropped in the extension host
+			if (instance.textModel.get()) {


Maybe we should have a hasTextModel method or getter.

I just get mildly uneasy about the observable implementation leaking. That being said it's a pretty straightforward api so it's probably never going to be an issue.

Ah, I agree, and INotebookEditor has hasModel as well

nstrayer · 2025-10-14T20:06:29Z

src/vs/workbench/api/browser/mainThreadNotebookEditors.ts

+		// If this is a Positron notebook, return the notebook instance ID.
+		const notebookInstance = getNotebookInstanceFromEditorPane(editorPane);
+		if (notebookInstance) {
+			return notebookInstance.id;


nice improvement.

nstrayer · 2025-10-14T20:07:22Z

src/vs/workbench/contrib/notebook/browser/controller/coreActions.ts

+		if (notebookInstance) {
+			const id = toPositronNotebookCommand(this.desc.id);
+			if (id) {
+				return accessor.get(ICommandService).executeCommand(id);


oh interesting. Seems good to me.

nstrayer · 2025-10-14T20:07:47Z

src/vs/workbench/contrib/positronNotebook/browser/notebookCells/CellEditorMonacoWidget.css

+	.monaco-editor {
+		/* Offset the editor to allow space for the selection bar. */
+		padding-left: var(--positron-notebook-selection-bar-width);


nstrayer · 2025-10-14T20:08:14Z

src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookCellActionBar.css

+
+			/* The debug icon is misaligned with run above / run below icons by 1pixel */
+			&.codicon-debug-alt-small {
+				position: relative;
+				top: -1px;
+			}


Is this a codicon issue or something about the layout code?

nstrayer · 2025-10-14T20:10:00Z

src/vs/workbench/contrib/positronNotebook/browser/toPositronNotebookCommand.ts

+const _code2PositronCommandId: Record<string, string> = {
+	[EXECUTE_CELL_COMMAND_ID]: POSITRON_EXECUTE_CELL_COMMAND_ID,
+};
+
+export function toPositronNotebookCommand(commandId: string): string | undefined {
+	try {
+		return _code2PositronCommandId[commandId];
+	} catch {
+		return undefined;
+	}
+}


I like this a good bit.

seeM requested review from dhruvisompura and nstrayer October 9, 2025 14:49

seeM commented Oct 9, 2025

View reviewed changes

seeM added 19 commits October 10, 2025 16:19

extension api

e3ec913

add/remove notebook instance events make `public textModel` observable connect positron notebook instances to mainThreadNotebookEditors.$tryShowNotebookDocument integrate positron notebooks with the extension api fix circular imports fix import fixes comment

more useful error

fab56a8

implement debug cell

8ad0090

cleanup

2e827c8

fix undefined check

6cb9e50

fixes

b5d39bc

working

91b36a3

cleanup

7f6935a

fix order error

508cf1d

only override active editor if defined and prev falsy

4fe4bf8

rework: patch upstream class

4abc727

cleanup

ce210d9

move button

0dec22a

nic suggestion

787f8ff

fix cell action bar item spacing; fix debug icon vertical alignment

da9f118

add keybinding

1401859

define editor css var overrides in main notebook element

d85a4be

fix editor gutter clipping selection bar

6c47433

update secrets

c62920c

seeM force-pushed the feature/debug-cell branch from 8bfbe91 to c62920c Compare October 10, 2025 14:27

fix circular import

71ba66b

cindyytong mentioned this pull request Oct 10, 2025

New Notebooks UI fixes from design sprint 1 - gutter padding, update icons, fix markdown cell naming #9636

Open

4 tasks

nstrayer approved these changes Oct 14, 2025

View reviewed changes

nstrayer mentioned this pull request Oct 16, 2025

Positron Notebooks UI sprint #9998

Open

		@@ -0,0 +1,227 @@
		/*---------------------------------------------------------------------------------------------
		* Copyright (C) 2024-2025 Posit Software, PBC. All rights reserved.

	* Copyright (C) 2024-2025 Posit Software, PBC. All rights reserved.
	* Copyright (C) 2025 Posit Software, PBC. All rights reserved.

PositroNB extension API integration and debugging #9860

Are you sure you want to change the base?

PositroNB extension API integration and debugging #9860

Uh oh!

Conversation

seeM commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Notes

New Features

Bug Fixes

QA Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cindyytong commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seeM commented Oct 10, 2025

Uh oh!

cindyytong commented Oct 10, 2025

Uh oh!

rodrigosf672 commented Oct 10, 2025

Uh oh!

cindyytong commented Oct 12, 2025

Uh oh!

nstrayer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seeM commented Oct 9, 2025 •

edited

Loading

cindyytong commented Oct 9, 2025 •

edited

Loading

github-actions bot commented Oct 9, 2025 •

edited

Loading