notebook multiselect revamp for actions #10678

dhruvisompura · 2025-11-20T01:03:02Z

This PR revamps a good portion of our notebook cell actions to support multi-selections and ensure that undo/redo work as expected.

Updated actions that now support multi-select properly with undo/redo:

cut
delete
copy
paste
move cell up
move cell down

I also fixed #10482 which fixes the "Run Below Cells" action to also include the current cell in its execution.

Run Below Cells - BEFORE

execute_cell_belows_before.mov

Run Below Cells - AFTER

execute_cell_belows_after.mov

Move Cells with Undo- AFTER

Screen.Recording.2025-11-20.at.2.24.18.PM.mov

Cut/Copy/Paste/Delete with Undo- AFTER

Screen.Recording.2025-11-20.at.2.20.16.PM.mov

Release Notes

New Features

N/A

Bug Fixes

N/A

QA Notes

@:positron-notebooks

github-actions · 2025-11-20T01:03:21Z

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

^readme ^{valid tags}

The delete cell action uses the `{ multiSelect: true }` option to handle multi-select scenarios. This causes `cell.delete()` to be called once for each selected cell, creating separate undo operations for each cell which is not correct for a multi-select scenario. When cells are deleted via multi-select, the deletes should happen as one atomic transaction and undo should bring back all deleted cells in the multi-select. To fix these, we are using the `deleteCells` function instead of the `deleteCell` function which internally handled multiple cells and the undo/redo logic for all cells the operation should work. I have also updated the action to extend NotebookAction2 to clearly communicate that this action relies on the notebook instance and is not run per cell.

Actions that rely on a function which internally handled multi-select scenarios and undo/redo history should not use the CellAction2 class. The CellAction2 class for multi-select scenarios applies the action to each cell individually and tracks each operation in the undo/redo history as a separate transaction. This is confusing for users because the expectation is that all cells are part of a single transaction as far as undo/redo is concerned. For example: When cutting 3 cells together, the undo operation should bring all 3 cells back. Prior to this change, undo would only bring the last cell back and the user would have to undo the operation per cell.

Update paste action to be a notebook level action since the internal pasteCells command handled multi-selection and utilizes the active cell for anchoring the pasted cells.

dhruvisompura · 2025-11-20T22:28:46Z

src/vs/workbench/contrib/positronNotebook/browser/contrib/undoRedo/positronNotebookUndoRedo.ts

The change in this file allows undo/redo to work when a notebook file has no cells

dhruvisompura · 2025-11-20T22:32:32Z

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

+		/**
+		 * Runs the cell action for each selected cell as an operation per cell
+		 * and not as a single operation on all selected cells. This means that
+		 * each cell action will be undoable/redoable individually and not as a
+		 * single transaction for all cells.
+		 */
 		if (this.options?.multiSelect) {
 			// Handle multiple selected cells
 			const selectedCells = getSelectedCells(activeNotebook.selectionStateMachine.state.get());

 			for (const cell of selectedCells) {
 				this.runCellAction(cell, activeNotebook, accessor);
 			}
 		} else {
-			// Handle single cell
+			// Handle single cell (the active cell which will also be the editing cell if in edit mode)
 			const state = activeNotebook.selectionStateMachine.state.get();
-			// Always check editing cell if actionBar is present (action bar items should work in edit mode).
-			// Otherwise, only check editing cell if editMode option is enabled.
-			const cell = getActiveCell(state) || ((isCellActionBarAction(this) || this.options?.editMode) ? getEditingCell(state) : undefined);
+			const cell = getActiveCell(state);


@seeM I updated the logic in here for determining the cell we should operate the cell action on when not in a mulit-selection scenario. I think having this just return the active cell makes sense since the active cell is the only one that can be in edit mode.

Let me know if this seems right to you!

dhruvisompura · 2025-11-20T22:39:20Z

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

@@ -700,17 +692,17 @@
 				order: 100,
 				group: 'Cell'
 			},
 			keybinding: {
 				when: POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED,
 				weight: KeybindingWeight.EditorContrib,
 				primary: KeyCode.Backspace,
 				secondary: [KeyChord(KeyCode.KeyD, KeyCode.KeyD)]
 			}
-		}, { multiSelect: true, editMode: false });
+		});
 	}

-	override runCellAction(cell: IPositronNotebookCell, _notebook: IPositronNotebookInstance, _accessor: ServicesAccessor) {
-		cell.delete();
+	override runNotebookAction(notebook: IPositronNotebookInstance, _accessor: ServicesAccessor) {
+		notebook.deleteCells();


I noticed that the { multiSelect: true } option on CellAction2 does not play well with undo/redo operations. This is because it runs the action for each cell which creates a separate undo operation per cell instead of handling it as one atomic transaction.

To fix this, we need operations that effect multiple cells to be handled in one transaction. Most of the operations that support multiSelect are on the notebook instance and internally handle multiple cells (cut/copy/paste). We just needed to update the delete function to behave similarly.

After making that change I also updated all of the actions that support multi-selection to use the NotebookAction2 class since they don't need a specific cell to work (they use the active cell or selected cells).

dhruvisompura · 2025-11-20T22:41:12Z

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

+				when: ContextKeyExpr.or(
+					POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED,
+					ContextKeyExpr.equals('activeEditor', POSITRON_NOTEBOOK_EDITOR_ID)
+				),


Pasting cells into a notebook needs to work regardless of the number of cells in the notebook. This means that all we really care about is the notebook being the active editor. I added that check. here. I don't think we need to check POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED but I left it in. Do you think I should just remove this now?

I think we can remolve it. One possible issue to check is whether this action consumes Cmd+v when editing a cell instead of the regular paste action

dhruvisompura · 2025-11-20T22:42:33Z

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

I made deleteCells public so we could use it for the delete action and renamed moveCellUp/moveCellDown to be plural since it supports multi-select.

dhruvisompura · 2025-11-20T22:42:40Z

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

-		// Run all code cells below the current cell
-		for (let i = cell.index + 1; i < cells.length; i++) {
+		// Run all code cells below the current cell (including the current cell)
+		for (let i = cell.index; i < cells.length; i++) {


This change just allows the current cell to also be executed: #10482

dhruvisompura · 2025-11-20T22:49:05Z

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

This file changes the "paste" and "move cell" functions to use the active/selected cell for the operation.

The delete cell function has been updated to use the active/selected cells for the operation if a set of cells aren't provided.

cc @nstrayer will these changes effect the assistant tool calls you added? Do I need to rewrite some of these to support an optional array of cells?

dhruvisompura · 2025-11-20T22:49:37Z

test/e2e/tests/notebooks-positron/notebook-focus-and-selection.test.ts

 	});

-	test("`+ Code` and `+ Markdown` buttons insert the cell after the active cell and make it the new active cell)", async function ({ app }) {
+	test("`+ Code` and `+ Markdown` buttons insert the cell after the active cell and make it the new active cell", async function ({ app }) {


Just fixing a typo in the test name here

seeM

This works really well in my testing, very smooth! As you pointed out, we need to double-check with @nstrayer whether the changes affect any tools before merging

There are some considerations if we want our commands to be compatible with VSCode counter parts but I don't think that's a priority right now and shouldn't be a heavy lift in future.

seeM · 2025-11-21T10:01:31Z

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

+				when: ContextKeyExpr.or(
+					POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED,
+					ContextKeyExpr.equals('activeEditor', POSITRON_NOTEBOOK_EDITOR_ID)
+				),


I think we can remolve it. One possible issue to check is whether this action consumes Cmd+v when editing a cell instead of the regular paste action

…-redo

dhruvisompura · 2025-11-21T20:09:14Z

@seeM

I think we can remolve it. One possible issue to check is whether this action consumes Cmd+v when editing a cell instead of the regular paste action

Thanks for mentioning this. I did need to add a fix for this to make sure we weren't in edit mode like we do for all other cell actions.

There are some considerations if we want our commands to be compatible with VSCode counter parts but I don't think that's a priority right now and shouldn't be a heavy lift in future.

We should chat a bit about this! I'm not too familiar with why this is important and what the VS Code counterparts are but happy to keep things in sync while doing any work.

This fix updates the POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED key to not be false when there are no cells in a notebook. This ensures the paste action (and any other action that uses the POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED context key) to only fire when the notebook editor has focus! We can't rely on the POSITRON_NOTEBOOK_CELL_EDITOR_FOCUSED context key because it is false when there are no cells in the notebook and we can't rely on the activeEditor context keys because it is always true and causes the action to fire EVERY time a user presses v in an input.

dhruvisompura · 2025-11-22T00:22:57Z

The last couple commits should fix the issue where a positron notebook didn't have focus when it is empty.

This caused a number of actions to not be able to operate in an empty notebook (specifically undo/redo/paste).

We use the POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED in almost all of our actions to determine if the user is actively focused on a notebook. This context key was being set to false when the notebook was empty, but it should have been true since the user is still focused on the notebook.

When a user removed the last cell in a notebook the onBlur event for the container was firing because the last focusable element in the notebook container was removed (focus moved out of the notebook up to another parent element). In ContextKeyManager.ts we listened for that blur event and set POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED to false.

We don't actually want the notebook container to lose focus when its empty. We need to make it programmatically focused when its empty. Doing so will prevent the onBlur event from firing and will NOT set POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED to false.

Side note: I also removed the ContextKeyExpr.equals('activeEditor', POSITRON_NOTEBOOK_EDITOR_ID) check from the actions since this caused the paste action to run anytime a user typed v anywhere in the IDE 🙈 since this tracks the (last) active editor and not the focused editor. All notebook actions should only be allowed to run when the notebook is focused. Fixing the POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED also made it so the paste cell action only works in a notebook!

dhruvisompura force-pushed the feature/notebook-multiselect-cut-delete-undo-redo branch from d50e141 to a3561dc Compare November 20, 2025 17:57

dhruvisompura added 10 commits November 20, 2025 11:40

Simplify check that determines what cell action is run for

b21fe6f

Allow undo/redo to work for empty notebook

2312604

Update paste cell action to operate off of active cell location

8424278

Update paste action to be a notebook level action since the internal pasteCells command handled multi-selection and utilizes the active cell for anchoring the pasted cells.

Fix typo in e2e test name

b97e16c

Clean up unused cell reference from move cell operations

66786c1

#10482 execute current cell in "Run Below Cells" command

203350a

Support paste operation for empty notebooks

dbf6df9

Improve check that allows undo/redo to work for empty notebook

cc40353

dhruvisompura force-pushed the feature/notebook-multiselect-cut-delete-undo-redo branch from 973caaa to cc40353 Compare November 20, 2025 19:40

dhruvisompura commented Nov 20, 2025

View reviewed changes

dhruvisompura marked this pull request as ready for review November 20, 2025 22:49

dhruvisompura requested review from midleman and seeM November 20, 2025 22:50

seeM previously approved these changes Nov 21, 2025

View reviewed changes

midleman added 3 commits November 21, 2025 09:41

Merge branch 'main' into feature/notebook-multiselect-cut-delete-undo…

732d3f5

…-redo

Merge branch 'main' into feature/notebook-multiselect-cut-delete-undo…

23e0208

…-redo

unskip e2e tests

87c942b

midleman dismissed seeM’s stale review via 87c942b November 21, 2025 17:30

Do not trigger paste action when a cell is in edit mode

62a7890

Simplify paste operation when clause

8b02e83

dhruvisompura requested a review from seeM November 21, 2025 20:09

dhruvisompura added 2 commits November 21, 2025 15:40

Hide notebook container focus outline

47e3838

notebook multiselect revamp for actions #10678

Are you sure you want to change the base?

notebook multiselect revamp for actions #10678

Uh oh!

Conversation

dhruvisompura commented Nov 20, 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!

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

Uh oh!

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!

seeM left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvisompura commented Nov 21, 2025

Uh oh!

dhruvisompura commented Nov 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dhruvisompura commented Nov 20, 2025 •

edited

Loading

github-actions bot commented Nov 20, 2025 •

edited

Loading