From 53675d1f8806c0646d66f605f64902a4a7bc88a7 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Mon, 6 Oct 2025 11:45:13 -0400 Subject: [PATCH 01/16] Add new positron notebooks test tag --- test/e2e/infra/test-runner/test-tags.ts | 1 + test/e2e/tests/notebook/cell-deletion-action-bar.test.ts | 2 +- test/e2e/tests/notebook/cell-execution-info.test.ts | 2 +- test/e2e/tests/notebook/notebook-focus-and-selection.test.ts | 2 +- test/e2e/tests/notebook/positron-notebook-copy-paste.test.ts | 2 +- test/e2e/tests/notebook/positron-notebook-editor.test.ts | 2 +- test/e2e/tests/notebook/positron-notebook-undo-redo.test.ts | 2 +- 7 files changed, 7 insertions(+), 6 deletions(-) diff --git a/test/e2e/infra/test-runner/test-tags.ts b/test/e2e/infra/test-runner/test-tags.ts index be4c5443ec47..0d1cbb8c0a6f 100644 --- a/test/e2e/infra/test-runner/test-tags.ts +++ b/test/e2e/infra/test-runner/test-tags.ts @@ -41,6 +41,7 @@ export enum TestTags { MODAL = '@:modal', NEW_FOLDER_FLOW = '@:new-folder-flow', NOTEBOOKS = '@:notebooks', + POSITRON_NOTEBOOKS = '@:positron-notebooks', OUTLINE = '@:outline', OUTPUT = '@:output', PLOTS = '@:plots', diff --git a/test/e2e/tests/notebook/cell-deletion-action-bar.test.ts b/test/e2e/tests/notebook/cell-deletion-action-bar.test.ts index 5ee3dbe82df0..c3e2b9b21436 100644 --- a/test/e2e/tests/notebook/cell-deletion-action-bar.test.ts +++ b/test/e2e/tests/notebook/cell-deletion-action-bar.test.ts @@ -22,7 +22,7 @@ async function getCellCount(app: Application): Promise { test.describe('Cell Deletion Action Bar Behavior', { - tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS] + tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS, tags.POSITRON_NOTEBOOKS] }, () => { test('Cell deletion using action bar', async function ({ app, settings }) { diff --git a/test/e2e/tests/notebook/cell-execution-info.test.ts b/test/e2e/tests/notebook/cell-execution-info.test.ts index 9e26294bb472..07265aba329a 100644 --- a/test/e2e/tests/notebook/cell-execution-info.test.ts +++ b/test/e2e/tests/notebook/cell-execution-info.test.ts @@ -32,7 +32,7 @@ async function activateInfoPopup({ app, icon }: { app: Application; icon: Locato } test.describe('Cell Execution Info Popup', { - tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS] + tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS, tags.POSITRON_NOTEBOOKS] }, () => { test.beforeAll(async function ({ app, settings }) { await app.workbench.notebooksPositron.enablePositronNotebooks(settings); diff --git a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts index a2b2b5c26657..d00d4310625f 100644 --- a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts +++ b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts @@ -87,7 +87,7 @@ function normalizeCellContent(content: string): string { // Not running on web due to Positron notebooks being desktop-only test.describe('Notebook Focus and Selection', { - tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS] + tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS, tags.POSITRON_NOTEBOOKS] }, () => { test.beforeAll(async function ({ app, settings }) { await app.workbench.notebooksPositron.enablePositronNotebooks(settings); diff --git a/test/e2e/tests/notebook/positron-notebook-copy-paste.test.ts b/test/e2e/tests/notebook/positron-notebook-copy-paste.test.ts index 635bb770b08d..58e9422e5d6d 100644 --- a/test/e2e/tests/notebook/positron-notebook-copy-paste.test.ts +++ b/test/e2e/tests/notebook/positron-notebook-copy-paste.test.ts @@ -50,7 +50,7 @@ async function pasteCellsWithKeyboard(app: Application): Promise { // Not running on web due to https://github.com/posit-dev/positron/issues/9193 test.describe('Notebook Cell Copy-Paste Behavior', { - tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS] + tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS, tags.POSITRON_NOTEBOOKS] }, () => { test.beforeAll(async function ({ app, settings }) { await app.workbench.notebooksPositron.enablePositronNotebooks(settings); diff --git a/test/e2e/tests/notebook/positron-notebook-editor.test.ts b/test/e2e/tests/notebook/positron-notebook-editor.test.ts index 24f56e451d21..03dfca004f27 100644 --- a/test/e2e/tests/notebook/positron-notebook-editor.test.ts +++ b/test/e2e/tests/notebook/positron-notebook-editor.test.ts @@ -14,7 +14,7 @@ test.use({ test.describe('Positron notebook opening and saving', { - tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS] + tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS, tags.POSITRON_NOTEBOOKS] }, () => { test.beforeAll(async function ({ app, settings }) { await app.workbench.notebooksPositron.enablePositronNotebooks(settings); diff --git a/test/e2e/tests/notebook/positron-notebook-undo-redo.test.ts b/test/e2e/tests/notebook/positron-notebook-undo-redo.test.ts index 758d8bcbb495..2e1a07824752 100644 --- a/test/e2e/tests/notebook/positron-notebook-undo-redo.test.ts +++ b/test/e2e/tests/notebook/positron-notebook-undo-redo.test.ts @@ -58,7 +58,7 @@ async function addCodeCellBelowWithKeyboard(app: Application): Promise { // Not running on web due to https://github.com/posit-dev/positron/issues/9193 test.describe('Notebook Cell Undo-Redo Behavior', { - tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS] + tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS, tags.POSITRON_NOTEBOOKS] }, () => { test.beforeAll(async function ({ app, settings }) { await app.workbench.notebooksPositron.enablePositronNotebooks(settings); From e1bd650ff1cdd5dd5d98dda2e272018edc39f0ce Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Mon, 6 Oct 2025 13:30:36 -0400 Subject: [PATCH 02/16] Add failing tests for focus-on-open logic --- .../notebook-focus-and-selection.test.ts | 147 ++++++++++++++++-- 1 file changed, 131 insertions(+), 16 deletions(-) diff --git a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts index d00d4310625f..819e026f33af 100644 --- a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts +++ b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts @@ -3,6 +3,7 @@ * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. *--------------------------------------------------------------------------------------------*/ +import path from 'path'; import { Application } from '../../infra/index.js'; import { test, tags } from '../_test.setup'; import { expect } from '@playwright/test'; @@ -56,6 +57,14 @@ async function getCellCount(app: Application): Promise { */ async function waitForFocusSettle(app: Application, timeoutMs: number = 2000): Promise { const page = app.code.driver.page; + + // First, ensure at least one cell exists in the DOM + await page.locator('[data-testid="notebook-cell"]').first().waitFor({ + state: 'attached', + timeout: timeoutMs + }); + + // Now wait for one of them to have focus await page.waitForFunction(() => { const cells = Array.from(document.querySelectorAll('[data-testid="notebook-cell"]')); return cells.some(cell => @@ -85,6 +94,29 @@ function normalizeCellContent(content: string): string { return content.replace(/\u00A0/g, ' ').replace(/ /g, ' '); } +/** + * Helper function to create a fresh notebook with 5 pre-populated cells + * Call this in tests that need a notebook with existing cells + */ +async function createNotebookWith5Cells(app: Application): Promise { + await app.workbench.notebooks.createNewNotebook(); + await app.workbench.notebooksPositron.expectToBeVisible(); + + // Add content to cells + await app.workbench.notebooksPositron.addCodeToCellAtIndex('print("Cell 0")', 0); + await app.workbench.notebooksPositron.addCodeToCellAtIndex('print("Cell 1")', 1); + await app.workbench.notebooksPositron.addCodeToCellAtIndex('print("Cell 2")', 2); + await app.workbench.notebooksPositron.addCodeToCellAtIndex('print("Cell 3")', 3); + await app.workbench.notebooksPositron.addCodeToCellAtIndex('print("Cell 4")', 4); + + expect(await getCellCount(app)).toBe(5); + + // After bulk adding cells, select the first cell to simulate proper initial state + // (In reality, opening an existing notebook selects first cell automatically via invariant) + await app.workbench.notebooksPositron.selectCellAtIndex(0); + await app.code.driver.page.waitForTimeout(100); +} + // Not running on web due to Positron notebooks being desktop-only test.describe('Notebook Focus and Selection', { tag: [tags.CRITICAL, tags.WIN, tags.NOTEBOOKS, tags.POSITRON_NOTEBOOKS] @@ -94,26 +126,13 @@ test.describe('Notebook Focus and Selection', { await app.workbench.notebooksPositron.setNotebookEditor(settings, 'positron'); }); - test.beforeEach(async function ({ app }) { - // Create a fresh notebook with 5 cells for each test - await app.workbench.notebooks.createNewNotebook(); - await app.workbench.notebooksPositron.expectToBeVisible(); - - // Add content to cells - await app.workbench.notebooksPositron.addCodeToCellAtIndex('print("Cell 0")', 0); - await app.workbench.notebooksPositron.addCodeToCellAtIndex('print("Cell 1")', 1); - await app.workbench.notebooksPositron.addCodeToCellAtIndex('print("Cell 2")', 2); - await app.workbench.notebooksPositron.addCodeToCellAtIndex('print("Cell 3")', 3); - await app.workbench.notebooksPositron.addCodeToCellAtIndex('print("Cell 4")', 4); - - expect(await getCellCount(app)).toBe(5); - }); - - test.afterEach(async function ({ app, hotKeys }) { + test.afterEach(async function ({ hotKeys }) { await hotKeys.closeAllEditors(); }); test('Cell selection via click focuses cell and adds selection styling', async function ({ app }) { + await createNotebookWith5Cells(app); + // Click on cell 2 await app.workbench.notebooksPositron.selectCellAtIndex(2); await waitForFocusSettle(app, 200); @@ -134,6 +153,8 @@ test.describe('Notebook Focus and Selection', { // test('Clicking a selected cell deselects it', async function ({ app }) { ... }); test('Arrow Down navigation moves focus to next cell', async function ({ app }) { + await createNotebookWith5Cells(app); + // Select cell 1 await app.workbench.notebooksPositron.selectCellAtIndex(1); await waitForFocusSettle(app, 200); @@ -153,6 +174,8 @@ test.describe('Notebook Focus and Selection', { }); test('Arrow Up navigation moves focus to previous cell', async function ({ app }) { + await createNotebookWith5Cells(app); + // Select cell 3 await app.workbench.notebooksPositron.selectCellAtIndex(3); await waitForFocusSettle(app, 200); @@ -172,6 +195,8 @@ test.describe('Notebook Focus and Selection', { }); test('Arrow Down at last cell does not change selection', async function ({ app }) { + await createNotebookWith5Cells(app); + // Select last cell (index 4) await app.workbench.notebooksPositron.selectCellAtIndex(4); await waitForFocusSettle(app, 200); @@ -190,6 +215,8 @@ test.describe('Notebook Focus and Selection', { }); test('Arrow Up at first cell does not change selection', async function ({ app }) { + await createNotebookWith5Cells(app); + // Select first cell (index 0) await app.workbench.notebooksPositron.selectCellAtIndex(0); await waitForFocusSettle(app, 200); @@ -208,6 +235,8 @@ test.describe('Notebook Focus and Selection', { }); test('Shift+Arrow Down adds next cell to selection', async function ({ app }) { + await createNotebookWith5Cells(app); + // Select cell 1 await app.workbench.notebooksPositron.selectCellAtIndex(1); await waitForFocusSettle(app, 200); @@ -226,6 +255,8 @@ test.describe('Notebook Focus and Selection', { }); test('Focus is maintained across multiple navigation operations', async function ({ app }) { + await createNotebookWith5Cells(app); + // Start at cell 0 await app.workbench.notebooksPositron.selectCellAtIndex(0); await waitForFocusSettle(app, 200); @@ -253,6 +284,8 @@ test.describe('Notebook Focus and Selection', { }); test('Sanity check: clicking editor focuses it (validates isEditorFocused helper)', async function ({ app }) { + await createNotebookWith5Cells(app); + // Select cell 1 await app.workbench.notebooksPositron.selectCellAtIndex(1); await waitForFocusSettle(app, 200); @@ -282,6 +315,8 @@ test.describe('Notebook Focus and Selection', { }); test('Enter key on selected cell enters edit mode', async function ({ app }) { + await createNotebookWith5Cells(app); + // Select cell 2 await app.workbench.notebooksPositron.selectCellAtIndex(2); await waitForFocusSettle(app, 200); @@ -316,6 +351,8 @@ test.describe('Notebook Focus and Selection', { }); test('Shift+Enter on last cell creates new cell and enters edit mode', async function ({ app }) { + await createNotebookWith5Cells(app); + // Select last cell (index 4) await app.workbench.notebooksPositron.selectCellAtIndex(4); await waitForFocusSettle(app, 200); @@ -350,4 +387,82 @@ test.describe('Notebook Focus and Selection', { expect(normalizedContent).toContain('new cell content'); }); + // NEW TESTS: Initial focus behavior (should fail with current implementation) + test('First cell is automatically selected when notebook loads', async function ({ app, hotKeys }) { + // Close the notebook from beforeEach to avoid focus contamination + await hotKeys.closeAllEditors(); + + // Open a real notebook file to test initial load behavior + const notebookPath = path.join('workspaces', 'bitmap-notebook', 'bitmap-notebook.ipynb'); + await app.workbench.notebooks.openNotebook(notebookPath, false); + await app.workbench.notebooksPositron.expectToBeVisible(); + + // Wait for cells to be in DOM + await app.code.driver.page.locator('[data-testid="notebook-cell"]').first().waitFor({ state: 'visible', timeout: 5000 }); + // Give a moment for initial selection to happen + await app.code.driver.page.waitForTimeout(500); + + // EXPECTED: First cell should be automatically selected without any user interaction + const focusedIndex = await getFocusedCellIndex(app); + expect(focusedIndex).toBe(0); + expect(await isCellSelected(app, 0)).toBe(true); + }); + + test('Keyboard navigation works immediately without clicking any cell', async function ({ app }) { + await createNotebookWith5Cells(app); + + // Give time for initial selection to happen (or not, if bug exists) + await app.code.driver.page.waitForTimeout(300); + + // Press Escape first to ensure we're not in edit mode + await app.code.driver.page.keyboard.press('Escape'); + await app.code.driver.page.waitForTimeout(100); + + // Press Arrow Down - EXPECTED: should move from cell 0 to cell 1 + await app.code.driver.page.keyboard.press('ArrowDown'); + await app.code.driver.page.waitForTimeout(200); + + expect(await getFocusedCellIndex(app)).toBe(1); + expect(await isCellSelected(app, 1)).toBe(true); + + // Arrow Down again should move to cell 2 + await app.code.driver.page.keyboard.press('ArrowDown'); + await app.code.driver.page.waitForTimeout(200); + + expect(await getFocusedCellIndex(app)).toBe(2); + expect(await isCellSelected(app, 2)).toBe(true); + }); + + test('Selection is preserved when switching between editors', async function ({ app }) { + await createNotebookWith5Cells(app); + + // Select cell 2 explicitly + await app.workbench.notebooksPositron.selectCellAtIndex(2); + await waitForFocusSettle(app, 200); + expect(await getFocusedCellIndex(app)).toBe(2); + + // Press Escape to exit edit mode + await app.code.driver.page.keyboard.press('Escape'); + await waitForFocusSettle(app, 100); + + // Create a new untitled file (switches editor focus away) + await app.workbench.quickaccess.runCommand('workbench.action.files.newUntitledFile'); + await app.code.driver.page.waitForTimeout(500); + + // Switch back to notebook using Ctrl/Cmd+Tab (or keyboard navigation) + // Use Cmd+Shift+P to open command palette, then navigate back + await app.workbench.quickaccess.runCommand('workbench.action.previousEditor'); + await app.code.driver.page.waitForTimeout(500); + + // EXPECTED: Cell 2 should still be selected and focused + await waitForFocusSettle(app, 300); + expect(await getFocusedCellIndex(app)).toBe(2); + expect(await isCellSelected(app, 2)).toBe(true); + + // Keyboard navigation should still work + await app.code.driver.page.keyboard.press('ArrowDown'); + await waitForFocusSettle(app, 200); + expect(await getFocusedCellIndex(app)).toBe(3); + }); + }); From 7ed74180facb4ae0f9d4cdbfff0f56340ffa2806 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Mon, 6 Oct 2025 14:14:21 -0400 Subject: [PATCH 03/16] state machine refactor --- .../browser/selectionMachine.ts | 325 +++++++++++++++--- 1 file changed, 270 insertions(+), 55 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts index 5f2b8c1019e2..dc3e255802af 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts @@ -6,9 +6,119 @@ import { autorunDelta, IObservable, observableValueOpts } from '../../../../base import { CellSelectionStatus, IPositronNotebookCell } from '../../../contrib/positronNotebook/browser/PositronNotebookCells/IPositronNotebookCell.js'; import { ILogService } from '../../../../platform/log/common/log.js'; import { Disposable } from '../../../../base/common/lifecycle.js'; +import { IEnvironmentService } from '../../../../platform/environment/common/environment.js'; + +/** + * STATE MACHINE SPECIFICATION + * =========================== + * + * This file implements a selection state machine for Positron notebooks. + * + * STATES: + * ------- + * - NoCells: No cells exist in the notebook + * - SingleSelection: Exactly one cell is selected (not editing) + * - MultiSelection: Multiple cells are selected + * - EditingSelection: One cell is selected and its editor is focused + * + * STATE TRANSITIONS: + * ------------------ + * Valid transitions between states: + * + * 1. NoCells → SingleSelection + * Trigger: Cells appear in notebook (automatic via _enforceInvariant) + * Guard: cells.length > 0 + * Result: First cell is automatically selected + * + * 2. SingleSelection → NoCells + * Trigger: All cells removed (automatic via _enforceInvariant) + * Guard: cells.length === 0 + * Result: Selection cleared + * + * 3. NoCells → EditingSelection + * Trigger: selectCell(cell, CellSelectionType.Edit) when a cell is created in an empty notebook + * Guard: None (valid whenever transitioning from empty notebook to editing) + * Result: Cell is created and immediately enters edit mode without intermediate selection state + * + * 4. SingleSelection → EditingSelection + * Trigger: enterEditor() called or selectCell(cell, CellSelectionType.Edit) + * Guard: Must be in SingleSelection state + * Result: Cell editor is shown and focused + * + * 5. SingleSelection → MultiSelection + * Trigger: selectCell(cell, CellSelectionType.Add) or moveUp/moveDown(addMode: true) + * Guard: Must have at least one cell already selected + * Result: Additional cell added to selection + * + * 6. SingleSelection → SingleSelection + * Trigger: selectCell(cell, CellSelectionType.Normal) or moveUp/moveDown(addMode: false) + * Guard: None + * Result: New cell becomes the only selected cell + * + * 7. EditingSelection → SingleSelection + * Trigger: exitEditor() called + * Guard: Must be in EditingSelection state + * Result: Editor focus removed, cell remains selected + * + * 8. EditingSelection → NoCells + * Trigger: Cell being edited is removed (automatic via _setCells) + * Guard: cells.length === 0 + * Result: Selection cleared + * + * 9. MultiSelection → SingleSelection + * Trigger: deselectCell() reduces to 1 cell, or selectCell(cell, CellSelectionType.Normal) + * Guard: None + * Result: Single cell selected + * + * 10. MultiSelection → MultiSelection + * Trigger: selectCell(cell, CellSelectionType.Add), deselectCell(), or moveUp/moveDown(addMode: true) + * Guard: Result must have >= 2 cells selected + * Result: Selection modified but remains multi-cell + * + * 11. MultiSelection → NoCells + * Trigger: All selected cells removed (automatic via _setCells) + * Guard: cells.length === 0 + * Result: Selection cleared + * + * INVARIANTS: + * ----------- + * 1. NoCells ↔ cells.length === 0 + * Enforced by: _enforceInvariant() called on every cell array change + * + * 2. If cells exist, something must be selected + * Enforced by: Automatic transition to SingleSelection when cells appear + * + * 3. EditingSelection requires exactly one cell + * Enforced by: enterEditor() guard and type system + * + * 4. SingleSelection.selected and MultiSelection.selected arrays are never empty + * Enforced by: Type system and transition logic + * + * STATE GUARDS (conditions that must be true for transitions): + * ------------------------------------------------------------- + * - enterEditor(): Current state must be SingleSelection + * - exitEditor(): Current state must be EditingSelection + * - selectCell(Add): Current state must not be NoCells + * - moveUp/moveDown: Current state must not be EditingSelection or NoCells + * + * AUTOMATIC TRANSITIONS: + * ---------------------- + * Some transitions happen automatically without explicit method calls: + * - Any state → NoCells: When cells array becomes empty + * - NoCells → SingleSelection: When cells array becomes non-empty + * - Any state → SingleSelection: When selected cells are removed, selects neighboring cell + * + * METHODS BY STATE: + * ----------------- + * Available operations depend on current state: + * - NoCells: No operations available (guards prevent all actions) + * - SingleSelection: selectCell, deselectCell, moveUp, moveDown, enterEditor + * - MultiSelection: selectCell, deselectCell, moveUp, moveDown + * - EditingSelection: exitEditor (movement and selection operations blocked by guards) + */ export enum SelectionState { - NoSelection = 'NoSelection', + NoCells = 'NoCells', SingleSelection = 'SingleSelection', MultiSelection = 'MultiSelection', EditingSelection = 'EditingSelection' @@ -16,7 +126,7 @@ export enum SelectionState { type SelectionStates = | { - type: SelectionState.NoSelection; + type: SelectionState.NoCells; } | { type: SelectionState.SingleSelection; @@ -40,18 +150,17 @@ export enum CellSelectionType { /** * Get all selected cells based on the current selection state. * @param state The selection state to extract cells from - * @returns An array of selected cells, empty if no selection. + * @returns An array of selected cells, empty if no cells exist. */ export function getSelectedCells(state: SelectionStates): IPositronNotebookCell[] { switch (state.type) { + case SelectionState.NoCells: + return []; case SelectionState.SingleSelection: case SelectionState.MultiSelection: return state.selected; case SelectionState.EditingSelection: return [state.selectedCell]; - case SelectionState.NoSelection: - default: - return []; } } @@ -84,7 +193,7 @@ export function getEditingCell(state: SelectionStates): IPositronNotebookCell | */ function isSelectionStateEqual(a: SelectionStates, b: SelectionStates): boolean { switch (a.type) { - case SelectionState.NoSelection: + case SelectionState.NoCells: return a.type === b.type; case SelectionState.SingleSelection: case SelectionState.MultiSelection: @@ -94,8 +203,6 @@ function isSelectionStateEqual(a: SelectionStates, b: SelectionStates): boolean case SelectionState.EditingSelection: return a.type === b.type && a.selectedCell === (b as typeof a).selectedCell; - default: - return false; } } @@ -105,7 +212,7 @@ export class SelectionStateMachine extends Disposable { private readonly _state = observableValueOpts({ debugName: 'selectionState', equalsFn: isSelectionStateEqual - }, { type: SelectionState.NoSelection }); + }, { type: SelectionState.NoCells }); //#endregion Private Properties @@ -113,6 +220,7 @@ export class SelectionStateMachine extends Disposable { constructor( private readonly _cells: IObservable, @ILogService private readonly _logService: ILogService, + @IEnvironmentService private readonly _environmentService: IEnvironmentService, ) { super(); this._register(autorunDelta(this.state, ({ lastValue, newValue }) => { @@ -125,6 +233,8 @@ export class SelectionStateMachine extends Disposable { this._register(autorunDelta(this._cells, ({ lastValue, newValue }) => { if (lastValue !== undefined) { this._setCells(newValue, lastValue); + // Enforce invariant after cell changes + this._enforceInvariant(newValue); } })); } @@ -149,36 +259,17 @@ export class SelectionStateMachine extends Disposable { * @param selectType The type of selection to perform. */ selectCell(cell: IPositronNotebookCell, selectType: CellSelectionType = CellSelectionType.Normal): void { - if (selectType === CellSelectionType.Normal) { - this._setState({ type: SelectionState.SingleSelection, selected: [cell] }); - return; - } - - if (selectType === CellSelectionType.Edit) { - this._setState({ type: SelectionState.EditingSelection, selectedCell: cell }); - return; + switch (selectType) { + case CellSelectionType.Normal: + this._selectCellNormal(cell); + break; + case CellSelectionType.Edit: + this._selectCellEdit(cell); + break; + case CellSelectionType.Add: + this._selectCellAdd(cell); + break; } - - const state = this._state.get(); - - if (selectType === CellSelectionType.Add) { - if (state.type === SelectionState.NoSelection) { - this._setState({ type: SelectionState.SingleSelection, selected: [cell] }); - return; - } - - if (state.type === SelectionState.SingleSelection || state.type === SelectionState.MultiSelection) { - // Check if cell is already selected - if (state.selected.includes(cell)) { - return; - } - this._setState({ type: SelectionState.MultiSelection, selected: [...state.selected, cell] }); - return; - } - } - - // Shouldn't get here. - this._logService.error('Unknown selection state', state, { selectType }); } /** @@ -189,16 +280,21 @@ export class SelectionStateMachine extends Disposable { deselectCell(cell: IPositronNotebookCell): void { const state = this._state.get(); - if (state.type === SelectionState.NoSelection) { + if (state.type === SelectionState.NoCells) { return; } - const deselectingCurrentSelection = state.type === SelectionState.SingleSelection - || state.type === SelectionState.EditingSelection - && state.selectedCell === cell; + const deselectingCurrentSelection = + (state.type === SelectionState.SingleSelection && state.selected[0] === cell) || + (state.type === SelectionState.EditingSelection && state.selectedCell === cell); if (deselectingCurrentSelection) { - this._setState({ type: SelectionState.NoSelection }); + // Don't manually set NoCells - let invariant enforcement handle it + // If cells still exist, select the first one + const cells = this._cells.get(); + if (cells.length > 0) { + this._setState({ type: SelectionState.SingleSelection, selected: [cells[0]] }); + } return; } @@ -258,6 +354,57 @@ export class SelectionStateMachine extends Disposable { //#region Private Methods + /** + * Performs a normal selection - replaces current selection with the specified cell. + * @param cell The cell to select. + */ + private _selectCellNormal(cell: IPositronNotebookCell): void { + this._setState({ type: SelectionState.SingleSelection, selected: [cell] }); + } + + /** + * Selects a cell for editing - enters edit mode with the specified cell. + * + * Note: This method supports the NoCells → EditingSelection transition for cases where + * a cell is created in an empty notebook and immediately entered for editing (e.g., notebook + * initialization, or adding a cell after deleting all cells). This avoids forcing a two-step + * transition (NoCells → SingleSelection → EditingSelection) which would trigger unnecessary + * intermediate state updates and side effects. + * + * @param cell The cell to select and edit. + */ + private _selectCellEdit(cell: IPositronNotebookCell): void { + this._setState({ type: SelectionState.EditingSelection, selectedCell: cell }); + } + + /** + * Adds a cell to the current selection (multi-select mode). + * @param cell The cell to add to the selection. + */ + private _selectCellAdd(cell: IPositronNotebookCell): void { + const state = this._state.get(); + + if (state.type === SelectionState.NoCells) { + // Should not happen - can't add selection to non-existent cells + // Invariant enforcement will handle this + this._logService.warn('SelectionMachine: Cannot add cell selection in NoCells state'); + return; + } + + if (state.type === SelectionState.EditingSelection) { + // Cannot add to selection while editing + this._logService.warn('SelectionMachine: Cannot add cell selection in EditingSelection state'); + return; + } + + // Check if cell is already selected + if (state.selected.includes(cell)) { + return; + } + + this._setState({ type: SelectionState.MultiSelection, selected: [...state.selected, cell] }); + } + /** * Updates the selection state when cells change. * @@ -267,7 +414,8 @@ export class SelectionStateMachine extends Disposable { private _setCells(cells: IPositronNotebookCell[], previousCells: IPositronNotebookCell[]): void { const state = this._state.get(); - if (state.type === SelectionState.NoSelection) { + // Let invariant enforcement handle NoCells transitions + if (state.type === SelectionState.NoCells) { return; } @@ -280,9 +428,8 @@ export class SelectionStateMachine extends Disposable { const cellToSelect = this._selectNeighboringCell(cells, deletedCellIndex); if (cellToSelect) { this._setState({ type: SelectionState.SingleSelection, selected: [cellToSelect] }); - } else { - this._setState({ type: SelectionState.NoSelection }); } + // If no cell to select, invariant enforcement will handle transition to NoCells return; } return; @@ -296,9 +443,8 @@ export class SelectionStateMachine extends Disposable { const cellToSelect = this._selectNeighboringCell(cells, deletedCellIndex); if (cellToSelect) { this._setState({ type: SelectionState.SingleSelection, selected: [cellToSelect] }); - } else { - this._setState({ type: SelectionState.NoSelection }); } + // If no cell to select, invariant enforcement will handle transition to NoCells return; } @@ -325,7 +471,79 @@ export class SelectionStateMachine extends Disposable { return cells[cells.length - 1]; } + /** + * Enforces the invariant: NoCells ↔ cells.length === 0 + * Called automatically when cells array changes + */ + private _enforceInvariant(cells: IPositronNotebookCell[]): void { + const currentState = this._state.get(); + + if (cells.length === 0 && currentState.type !== SelectionState.NoCells) { + // Cells disappeared → force NoCells state + this._logService.debug('SelectionMachine: Enforcing NoCells state (no cells exist)'); + this._setState({ type: SelectionState.NoCells }); + } + else if (cells.length > 0 && currentState.type === SelectionState.NoCells) { + // Cells appeared → automatically select first cell + this._logService.debug('SelectionMachine: Auto-selecting first cell (cells appeared)'); + this._setState({ + type: SelectionState.SingleSelection, + selected: [cells[0]] + }); + } + } + + /** + * Validates whether a transition from one state to another is valid. + * @param from The source state + * @param to The destination state + * @returns True if the transition is valid, false otherwise + */ + private _isValidTransition(from: SelectionState, to: SelectionState): boolean { + // Define valid transitions as a map + const validTransitions: Record = { + [SelectionState.NoCells]: [ + SelectionState.NoCells, // Can stay in NoCells + SelectionState.SingleSelection, // Cells appear → select first + SelectionState.EditingSelection, // Cell created in empty notebook and immediately edited + ], + [SelectionState.SingleSelection]: [ + SelectionState.SingleSelection, // Select different cell + SelectionState.MultiSelection, // Add cell to selection + SelectionState.EditingSelection, // Enter edit mode + SelectionState.NoCells, // All cells removed + ], + [SelectionState.MultiSelection]: [ + SelectionState.MultiSelection, // Modify selection + SelectionState.SingleSelection, // Reduce to single cell + SelectionState.NoCells, // All cells removed + ], + [SelectionState.EditingSelection]: [ + SelectionState.EditingSelection, // Can stay in editing (same cell) + SelectionState.SingleSelection, // Exit editor + SelectionState.NoCells, // Cell being edited removed + ], + }; + + return validTransitions[from].includes(to); + } + private _setState(state: SelectionStates) { + const currentState = this._state.get(); + + // Validate transition + if (!this._isValidTransition(currentState.type, state.type)) { + const message = `SelectionMachine: Invalid state transition from ${currentState.type} to ${state.type}`; + + // In development mode, throw an error to catch bugs early + if (!this._environmentService.isBuilt) { + throw new Error(message); + } + + // In production, log a warning but allow the transition + this._logService.warn(message); + } + // Alert the observable that the state has changed. this._state.set(state, undefined); } @@ -396,12 +614,9 @@ export class SelectionStateMachine extends Disposable { if (state.type === SelectionState.EditingSelection) { return; } - if (state.type === SelectionState.NoSelection) { - // Select first cell if selecting down and the last cell if selecting up. - const cellToSelect = cells.at(up ? -1 : 0); - if (cellToSelect) { - this.selectCell(cellToSelect, CellSelectionType.Normal); - } + // NoCells case: Cannot move selection when no cells exist + // Invariant ensures we're never in NoCells when cells exist + if (state.type === SelectionState.NoCells) { return; } From 6dba64c8016d05ffdbdba66be345edb4dcd79d1d Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 11:00:42 -0400 Subject: [PATCH 04/16] Remove unneccesary comment about new tests and logic covered already in after-each --- .../tests/notebook/notebook-focus-and-selection.test.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts index 819e026f33af..0136ad8763cb 100644 --- a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts +++ b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts @@ -387,11 +387,8 @@ test.describe('Notebook Focus and Selection', { expect(normalizedContent).toContain('new cell content'); }); - // NEW TESTS: Initial focus behavior (should fail with current implementation) - test('First cell is automatically selected when notebook loads', async function ({ app, hotKeys }) { - // Close the notebook from beforeEach to avoid focus contamination - await hotKeys.closeAllEditors(); - + // Initial focus behavior (should fail with current implementation) + test('First cell is automatically selected when notebook loads', async function ({ app }) { // Open a real notebook file to test initial load behavior const notebookPath = path.join('workspaces', 'bitmap-notebook', 'bitmap-notebook.ipynb'); await app.workbench.notebooks.openNotebook(notebookPath, false); From 79ed4b217169dc362bc4715a92ff97e2301de3c4 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 11:10:56 -0400 Subject: [PATCH 05/16] Remove all hardcoded waits --- .../notebook-focus-and-selection.test.ts | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts index 0136ad8763cb..b16b148d1fbd 100644 --- a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts +++ b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts @@ -394,10 +394,9 @@ test.describe('Notebook Focus and Selection', { await app.workbench.notebooks.openNotebook(notebookPath, false); await app.workbench.notebooksPositron.expectToBeVisible(); - // Wait for cells to be in DOM + // Wait for cells to be in DOM and for initial focus to settle await app.code.driver.page.locator('[data-testid="notebook-cell"]').first().waitFor({ state: 'visible', timeout: 5000 }); - // Give a moment for initial selection to happen - await app.code.driver.page.waitForTimeout(500); + await waitForFocusSettle(app); // EXPECTED: First cell should be automatically selected without any user interaction const focusedIndex = await getFocusedCellIndex(app); @@ -408,23 +407,23 @@ test.describe('Notebook Focus and Selection', { test('Keyboard navigation works immediately without clicking any cell', async function ({ app }) { await createNotebookWith5Cells(app); - // Give time for initial selection to happen (or not, if bug exists) - await app.code.driver.page.waitForTimeout(300); + // Wait for initial selection to settle + await waitForFocusSettle(app); // Press Escape first to ensure we're not in edit mode await app.code.driver.page.keyboard.press('Escape'); - await app.code.driver.page.waitForTimeout(100); + await waitForFocusSettle(app, 500); // Press Arrow Down - EXPECTED: should move from cell 0 to cell 1 await app.code.driver.page.keyboard.press('ArrowDown'); - await app.code.driver.page.waitForTimeout(200); + await waitForFocusSettle(app, 500); expect(await getFocusedCellIndex(app)).toBe(1); expect(await isCellSelected(app, 1)).toBe(true); // Arrow Down again should move to cell 2 await app.code.driver.page.keyboard.press('ArrowDown'); - await app.code.driver.page.waitForTimeout(200); + await waitForFocusSettle(app, 500); expect(await getFocusedCellIndex(app)).toBe(2); expect(await isCellSelected(app, 2)).toBe(true); @@ -444,15 +443,13 @@ test.describe('Notebook Focus and Selection', { // Create a new untitled file (switches editor focus away) await app.workbench.quickaccess.runCommand('workbench.action.files.newUntitledFile'); - await app.code.driver.page.waitForTimeout(500); // Switch back to notebook using Ctrl/Cmd+Tab (or keyboard navigation) // Use Cmd+Shift+P to open command palette, then navigate back await app.workbench.quickaccess.runCommand('workbench.action.previousEditor'); - await app.code.driver.page.waitForTimeout(500); // EXPECTED: Cell 2 should still be selected and focused - await waitForFocusSettle(app, 300); + await waitForFocusSettle(app, 1000); expect(await getFocusedCellIndex(app)).toBe(2); expect(await isCellSelected(app, 2)).toBe(true); From 3b386aa9213a9272cfbcd9416a2044c7c213b451 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 11:15:30 -0400 Subject: [PATCH 06/16] Remove comments and code left over from development --- .../notebook-focus-and-selection.test.ts | 50 +++---------------- 1 file changed, 7 insertions(+), 43 deletions(-) diff --git a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts index b16b148d1fbd..2c765104a3f6 100644 --- a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts +++ b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts @@ -13,7 +13,7 @@ test.use({ }); /** - * Helper function to get the currently focused cell index + * Get the currently focused cell index * Checks if the cell or any of its children contain the active element */ async function getFocusedCellIndex(app: Application): Promise { @@ -36,7 +36,7 @@ async function getFocusedCellIndex(app: Application): Promise { } /** - * Helper function to check if a cell is selected (has selection styling) + * Check if a cell is selected (has selection styling) */ async function isCellSelected(app: Application, index: number): Promise { const cell = app.code.driver.page.locator('[data-testid="notebook-cell"]').nth(index); @@ -45,14 +45,14 @@ async function isCellSelected(app: Application, index: number): Promise } /** - * Helper function to get cell count + * Get cell count */ async function getCellCount(app: Application): Promise { return await app.code.driver.page.locator('[data-testid="notebook-cell"]').count(); } /** - * Helper function to wait for focus to settle (useful after DOM changes) + * Wait for focus to settle (useful after DOM changes) * Waits until any notebook cell has focus (or until timeout) */ async function waitForFocusSettle(app: Application, timeoutMs: number = 2000): Promise { @@ -74,7 +74,7 @@ async function waitForFocusSettle(app: Application, timeoutMs: number = 2000): P } /** - * Helper function to check if the Monaco editor in a cell is focused + * Check if the Monaco editor in a cell is focused */ async function isEditorFocused(app: Application, cellIndex: number): Promise { const cell = app.code.driver.page.locator('[data-testid="notebook-cell"]').nth(cellIndex); @@ -87,7 +87,7 @@ async function isEditorFocused(app: Application, cellIndex: number): Promise { @@ -148,10 +148,6 @@ test.describe('Notebook Focus and Selection', { expect(await isCellSelected(app, 1)).toBe(false); }); - // NOTE: Clicking a selected cell currently does NOT deselect it in the current implementation - // This test is commented out as the behavior may change during refactoring - // test('Clicking a selected cell deselects it', async function ({ app }) { ... }); - test('Arrow Down navigation moves focus to next cell', async function ({ app }) { await createNotebookWith5Cells(app); @@ -283,37 +279,6 @@ test.describe('Notebook Focus and Selection', { expect(await getFocusedCellIndex(app)).toBe(2); }); - test('Sanity check: clicking editor focuses it (validates isEditorFocused helper)', async function ({ app }) { - await createNotebookWith5Cells(app); - - // Select cell 1 - await app.workbench.notebooksPositron.selectCellAtIndex(1); - await waitForFocusSettle(app, 200); - - // Press Escape to exit edit mode (selectCellAtIndex may enter edit mode) - await app.code.driver.page.keyboard.press('Escape'); - await waitForFocusSettle(app, 200); - - // Editor should not be focused after pressing Escape - expect(await isEditorFocused(app, 1)).toBe(false); - - // Click directly into the Monaco editor - const cell = app.code.driver.page.locator('[data-testid="notebook-cell"]').nth(1); - const editor = cell.locator('.monaco-editor'); - await editor.click(); - await waitForFocusSettle(app, 200); - - // Now editor should be focused - expect(await isEditorFocused(app, 1)).toBe(true); - - // Type some text to confirm editor is really focused - await app.code.driver.page.keyboard.type('# editor good'); - const cellContent = await app.workbench.notebooksPositron.getCellContent(1); - // Normalize content to handle non-breaking spaces - const normalizedContent = normalizeCellContent(cellContent); - expect(normalizedContent).toContain('# editor good'); - }); - test('Enter key on selected cell enters edit mode', async function ({ app }) { await createNotebookWith5Cells(app); @@ -387,7 +352,6 @@ test.describe('Notebook Focus and Selection', { expect(normalizedContent).toContain('new cell content'); }); - // Initial focus behavior (should fail with current implementation) test('First cell is automatically selected when notebook loads', async function ({ app }) { // Open a real notebook file to test initial load behavior const notebookPath = path.join('workspaces', 'bitmap-notebook', 'bitmap-notebook.ipynb'); From 5cc4bcb6e1546f819b605553c9064624f70574b7 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 11:23:41 -0400 Subject: [PATCH 07/16] Extract out helper to wait for cells in dom --- .../notebook-focus-and-selection.test.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts index 2c765104a3f6..49543a01af7e 100644 --- a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts +++ b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts @@ -51,6 +51,16 @@ async function getCellCount(app: Application): Promise { return await app.code.driver.page.locator('[data-testid="notebook-cell"]').count(); } +/** + * Wait for at least one cell to exist in the DOM + */ +async function waitForCellsInDOM(app: Application, timeoutMs: number = 2000): Promise { + await app.code.driver.page.locator('[data-testid="notebook-cell"]').first().waitFor({ + state: 'visible', + timeout: timeoutMs + }); +} + /** * Wait for focus to settle (useful after DOM changes) * Waits until any notebook cell has focus (or until timeout) @@ -59,10 +69,7 @@ async function waitForFocusSettle(app: Application, timeoutMs: number = 2000): P const page = app.code.driver.page; // First, ensure at least one cell exists in the DOM - await page.locator('[data-testid="notebook-cell"]').first().waitFor({ - state: 'attached', - timeout: timeoutMs - }); + await waitForCellsInDOM(app, timeoutMs); // Now wait for one of them to have focus await page.waitForFunction(() => { @@ -359,7 +366,7 @@ test.describe('Notebook Focus and Selection', { await app.workbench.notebooksPositron.expectToBeVisible(); // Wait for cells to be in DOM and for initial focus to settle - await app.code.driver.page.locator('[data-testid="notebook-cell"]').first().waitFor({ state: 'visible', timeout: 5000 }); + await waitForCellsInDOM(app, 5000); await waitForFocusSettle(app); // EXPECTED: First cell should be automatically selected without any user interaction From 346695f28fa11b04df3abd8b6cf017abedb27acf Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 11:36:35 -0400 Subject: [PATCH 08/16] Remove docstring for selection machine so code is single source of truth --- .../browser/selectionMachine.ts | 106 +----------------- 1 file changed, 1 insertion(+), 105 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts index dc3e255802af..3ff401ced291 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts @@ -14,109 +14,7 @@ import { IEnvironmentService } from '../../../../platform/environment/common/env * * This file implements a selection state machine for Positron notebooks. * - * STATES: - * ------- - * - NoCells: No cells exist in the notebook - * - SingleSelection: Exactly one cell is selected (not editing) - * - MultiSelection: Multiple cells are selected - * - EditingSelection: One cell is selected and its editor is focused - * - * STATE TRANSITIONS: - * ------------------ - * Valid transitions between states: - * - * 1. NoCells → SingleSelection - * Trigger: Cells appear in notebook (automatic via _enforceInvariant) - * Guard: cells.length > 0 - * Result: First cell is automatically selected - * - * 2. SingleSelection → NoCells - * Trigger: All cells removed (automatic via _enforceInvariant) - * Guard: cells.length === 0 - * Result: Selection cleared - * - * 3. NoCells → EditingSelection - * Trigger: selectCell(cell, CellSelectionType.Edit) when a cell is created in an empty notebook - * Guard: None (valid whenever transitioning from empty notebook to editing) - * Result: Cell is created and immediately enters edit mode without intermediate selection state - * - * 4. SingleSelection → EditingSelection - * Trigger: enterEditor() called or selectCell(cell, CellSelectionType.Edit) - * Guard: Must be in SingleSelection state - * Result: Cell editor is shown and focused - * - * 5. SingleSelection → MultiSelection - * Trigger: selectCell(cell, CellSelectionType.Add) or moveUp/moveDown(addMode: true) - * Guard: Must have at least one cell already selected - * Result: Additional cell added to selection - * - * 6. SingleSelection → SingleSelection - * Trigger: selectCell(cell, CellSelectionType.Normal) or moveUp/moveDown(addMode: false) - * Guard: None - * Result: New cell becomes the only selected cell - * - * 7. EditingSelection → SingleSelection - * Trigger: exitEditor() called - * Guard: Must be in EditingSelection state - * Result: Editor focus removed, cell remains selected - * - * 8. EditingSelection → NoCells - * Trigger: Cell being edited is removed (automatic via _setCells) - * Guard: cells.length === 0 - * Result: Selection cleared - * - * 9. MultiSelection → SingleSelection - * Trigger: deselectCell() reduces to 1 cell, or selectCell(cell, CellSelectionType.Normal) - * Guard: None - * Result: Single cell selected - * - * 10. MultiSelection → MultiSelection - * Trigger: selectCell(cell, CellSelectionType.Add), deselectCell(), or moveUp/moveDown(addMode: true) - * Guard: Result must have >= 2 cells selected - * Result: Selection modified but remains multi-cell - * - * 11. MultiSelection → NoCells - * Trigger: All selected cells removed (automatic via _setCells) - * Guard: cells.length === 0 - * Result: Selection cleared - * - * INVARIANTS: - * ----------- - * 1. NoCells ↔ cells.length === 0 - * Enforced by: _enforceInvariant() called on every cell array change - * - * 2. If cells exist, something must be selected - * Enforced by: Automatic transition to SingleSelection when cells appear - * - * 3. EditingSelection requires exactly one cell - * Enforced by: enterEditor() guard and type system - * - * 4. SingleSelection.selected and MultiSelection.selected arrays are never empty - * Enforced by: Type system and transition logic - * - * STATE GUARDS (conditions that must be true for transitions): - * ------------------------------------------------------------- - * - enterEditor(): Current state must be SingleSelection - * - exitEditor(): Current state must be EditingSelection - * - selectCell(Add): Current state must not be NoCells - * - moveUp/moveDown: Current state must not be EditingSelection or NoCells - * - * AUTOMATIC TRANSITIONS: - * ---------------------- - * Some transitions happen automatically without explicit method calls: - * - Any state → NoCells: When cells array becomes empty - * - NoCells → SingleSelection: When cells array becomes non-empty - * - Any state → SingleSelection: When selected cells are removed, selects neighboring cell - * - * METHODS BY STATE: - * ----------------- - * Available operations depend on current state: - * - NoCells: No operations available (guards prevent all actions) - * - SingleSelection: selectCell, deselectCell, moveUp, moveDown, enterEditor - * - MultiSelection: selectCell, deselectCell, moveUp, moveDown - * - EditingSelection: exitEditor (movement and selection operations blocked by guards) - */ - + **/ export enum SelectionState { NoCells = 'NoCells', SingleSelection = 'SingleSelection', @@ -164,7 +62,6 @@ export function getSelectedCells(state: SelectionStates): IPositronNotebookCell[ } } - /** * Get the selected cell if there is exactly one selected. * @returns The selected cell. Null if there is no selection. @@ -213,7 +110,6 @@ export class SelectionStateMachine extends Disposable { debugName: 'selectionState', equalsFn: isSelectionStateEqual }, { type: SelectionState.NoCells }); - //#endregion Private Properties //#region Constructor & Dispose From e8bcad3abfda70a902e893682be1e1be27d45be1 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 11:44:40 -0400 Subject: [PATCH 09/16] Make selection state union a bit less confusing by not calling the edit state selection 'selectedCell' in favor of always using selected with type descrimination. --- .../browser/selectionMachine.ts | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts index 3ff401ced291..b68dd5ed23c9 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts @@ -22,6 +22,19 @@ export enum SelectionState { EditingSelection = 'EditingSelection' } +/** + * Selection state discriminated union. + * + * Design rationale for field types: + * - SingleSelection and MultiSelection use `selected: IPositronNotebookCell[]` (array) + * to enable uniform handling: filtering, equality checks, and automatic transitions + * based on array.length. + * - EditingSelection uses `selected: IPositronNotebookCell` (singular) for type safety, + * ensuring compile-time enforcement that exactly one cell is being edited. + * + * This design provides API consistency (all states use `selected`) while leveraging + * TypeScript's discriminated unions to enforce different constraints per state. + */ type SelectionStates = | { type: SelectionState.NoCells; @@ -36,7 +49,7 @@ type SelectionStates = } | { type: SelectionState.EditingSelection; - selectedCell: IPositronNotebookCell; + selected: IPositronNotebookCell; }; export enum CellSelectionType { @@ -58,7 +71,7 @@ export function getSelectedCells(state: SelectionStates): IPositronNotebookCell[ case SelectionState.MultiSelection: return state.selected; case SelectionState.EditingSelection: - return [state.selectedCell]; + return [state.selected]; } } @@ -82,7 +95,7 @@ export function getEditingCell(state: SelectionStates): IPositronNotebookCell | if (state.type !== SelectionState.EditingSelection) { return null; } - return state.selectedCell; + return state.selected; } /** @@ -99,7 +112,7 @@ function isSelectionStateEqual(a: SelectionStates, b: SelectionStates): boolean a.selected.every(cell => (b as typeof a).selected.includes(cell)); case SelectionState.EditingSelection: return a.type === b.type && - a.selectedCell === (b as typeof a).selectedCell; + a.selected === (b as typeof a).selected; } } @@ -182,7 +195,7 @@ export class SelectionStateMachine extends Disposable { const deselectingCurrentSelection = (state.type === SelectionState.SingleSelection && state.selected[0] === cell) || - (state.type === SelectionState.EditingSelection && state.selectedCell === cell); + (state.type === SelectionState.EditingSelection && state.selected === cell); if (deselectingCurrentSelection) { // Don't manually set NoCells - let invariant enforcement handle it @@ -229,7 +242,7 @@ export class SelectionStateMachine extends Disposable { } const cellToEdit = state.selected[0]; - this._setState({ type: SelectionState.EditingSelection, selectedCell: cellToEdit }); + this._setState({ type: SelectionState.EditingSelection, selected: cellToEdit }); // Ensure editor is shown first (important for markdown cells and lazy-loaded editors) await cellToEdit.showEditor(); // Request editor focus through observable - React will handle it @@ -242,7 +255,7 @@ export class SelectionStateMachine extends Disposable { exitEditor(): void { const state = this._state.get(); if (state.type !== SelectionState.EditingSelection) { return; } - this._setState({ type: SelectionState.SingleSelection, selected: [state.selectedCell] }); + this._setState({ type: SelectionState.SingleSelection, selected: [state.selected] }); } //#endregion Public Methods @@ -270,7 +283,7 @@ export class SelectionStateMachine extends Disposable { * @param cell The cell to select and edit. */ private _selectCellEdit(cell: IPositronNotebookCell): void { - this._setState({ type: SelectionState.EditingSelection, selectedCell: cell }); + this._setState({ type: SelectionState.EditingSelection, selected: cell }); } /** @@ -318,9 +331,9 @@ export class SelectionStateMachine extends Disposable { // If we're editing a cell when setCells is called. We need to check if the cell is still in the new cells. // If it isn't we need to select an appropriate neighboring cell. if (state.type === SelectionState.EditingSelection) { - if (!cells.includes(state.selectedCell)) { + if (!cells.includes(state.selected)) { // Find the index where the deleted cell was in the previous array - const deletedCellIndex = previousCells.indexOf(state.selectedCell); + const deletedCellIndex = previousCells.indexOf(state.selected); const cellToSelect = this._selectNeighboringCell(cells, deletedCellIndex); if (cellToSelect) { this._setState({ type: SelectionState.SingleSelection, selected: [cellToSelect] }); From 0b2eceb128318a42a9d37aa07a97990ee147e461 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 11:58:44 -0400 Subject: [PATCH 10/16] Be more explicit about structure of selected elements array --- .../browser/selectionMachine.ts | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts index b68dd5ed23c9..084e1045a580 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts @@ -22,6 +22,19 @@ export enum SelectionState { EditingSelection = 'EditingSelection' } +type NonEmptyArray = [T, ...T[]]; + +function assertNonEmptyArray(array: T[]): asserts array is NonEmptyArray { + if (array.length === 0) { + throw new Error('Array must be non-empty'); + } +} + +function verifyNonEmptyArray(array: T[]): NonEmptyArray { + assertNonEmptyArray(array); + return array as NonEmptyArray; +} + /** * Selection state discriminated union. * @@ -41,11 +54,11 @@ type SelectionStates = } | { type: SelectionState.SingleSelection; - selected: IPositronNotebookCell[]; + selected: NonEmptyArray; } | { type: SelectionState.MultiSelection; - selected: IPositronNotebookCell[]; + selected: NonEmptyArray; } | { type: SelectionState.EditingSelection; @@ -208,7 +221,7 @@ export class SelectionStateMachine extends Disposable { } if (state.type === SelectionState.MultiSelection) { - const updatedSelection = state.selected.filter(c => c !== cell); + const updatedSelection = verifyNonEmptyArray(state.selected.filter(c => c !== cell)); // React will handle focus based on selection state change this._setState({ type: updatedSelection.length === 1 ? SelectionState.SingleSelection : SelectionState.MultiSelection, selected: updatedSelection }); } @@ -357,7 +370,7 @@ export class SelectionStateMachine extends Disposable { return; } - this._setState({ type: newSelection.length === 1 ? SelectionState.SingleSelection : SelectionState.MultiSelection, selected: newSelection }); + this._setState({ type: newSelection.length === 1 ? SelectionState.SingleSelection : SelectionState.MultiSelection, selected: verifyNonEmptyArray(newSelection) }); } /** @@ -529,7 +542,8 @@ export class SelectionStateMachine extends Disposable { return; } - const edgeCell = state.selected.at(up ? 0 : -1)!; + // Direct access is safe because NonEmptyArray guarantees at least one element + const edgeCell = state.selected[up ? 0 : state.selected.length - 1]; const indexOfEdgeCell = edgeCell.index; const nextCell = cells[indexOfEdgeCell + (up ? -1 : 1)]; @@ -543,7 +557,7 @@ export class SelectionStateMachine extends Disposable { // Already at the edge of the cells. return; } - const newSelection = up ? [nextCell, ...state.selected] : [...state.selected, nextCell]; + const newSelection = verifyNonEmptyArray(up ? [nextCell, ...state.selected] : [...state.selected, nextCell]); this._setState({ type: SelectionState.MultiSelection, selected: newSelection From fbd5939ceed68cf6723a62206bd6b737e6b5b32d Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 12:09:55 -0400 Subject: [PATCH 11/16] Move to more obvious typing of selected cell for single selection state --- .../browser/selectionMachine.ts | 77 +++++++++++++------ 1 file changed, 52 insertions(+), 25 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts index 084e1045a580..6ca922b572ea 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts @@ -39,14 +39,15 @@ function verifyNonEmptyArray(array: T[]): NonEmptyArray { * Selection state discriminated union. * * Design rationale for field types: - * - SingleSelection and MultiSelection use `selected: IPositronNotebookCell[]` (array) - * to enable uniform handling: filtering, equality checks, and automatic transitions - * based on array.length. + * - SingleSelection uses `selected: IPositronNotebookCell` (singular) for type safety, + * ensuring compile-time enforcement that exactly one cell is selected. + * - MultiSelection uses `selected: NonEmptyArray` (array) to ensure + * at least one cell is selected and enable filtering and equality checks. * - EditingSelection uses `selected: IPositronNotebookCell` (singular) for type safety, * ensuring compile-time enforcement that exactly one cell is being edited. * - * This design provides API consistency (all states use `selected`) while leveraging - * TypeScript's discriminated unions to enforce different constraints per state. + * This design provides type safety while maintaining clear distinctions between + * single and multi-cell selection states. */ type SelectionStates = | { @@ -54,7 +55,7 @@ type SelectionStates = } | { type: SelectionState.SingleSelection; - selected: NonEmptyArray; + selected: IPositronNotebookCell; } | { type: SelectionState.MultiSelection; @@ -81,6 +82,7 @@ export function getSelectedCells(state: SelectionStates): IPositronNotebookCell[ case SelectionState.NoCells: return []; case SelectionState.SingleSelection: + return [state.selected]; case SelectionState.MultiSelection: return state.selected; case SelectionState.EditingSelection: @@ -94,7 +96,7 @@ export function getSelectedCells(state: SelectionStates): IPositronNotebookCell[ */ export function getSelectedCell(state: SelectionStates): IPositronNotebookCell | null { if (state.type === SelectionState.SingleSelection) { - return state.selected[0]; + return state.selected; } return null; } @@ -119,6 +121,8 @@ function isSelectionStateEqual(a: SelectionStates, b: SelectionStates): boolean case SelectionState.NoCells: return a.type === b.type; case SelectionState.SingleSelection: + return a.type === b.type && + a.selected === (b as typeof a).selected; case SelectionState.MultiSelection: return a.type === b.type && a.selected.length === (b as typeof a).selected.length && @@ -207,7 +211,7 @@ export class SelectionStateMachine extends Disposable { } const deselectingCurrentSelection = - (state.type === SelectionState.SingleSelection && state.selected[0] === cell) || + (state.type === SelectionState.SingleSelection && state.selected === cell) || (state.type === SelectionState.EditingSelection && state.selected === cell); if (deselectingCurrentSelection) { @@ -215,15 +219,29 @@ export class SelectionStateMachine extends Disposable { // If cells still exist, select the first one const cells = this._cells.get(); if (cells.length > 0) { - this._setState({ type: SelectionState.SingleSelection, selected: [cells[0]] }); + this._setState({ type: SelectionState.SingleSelection, selected: cells[0] }); } return; } if (state.type === SelectionState.MultiSelection) { - const updatedSelection = verifyNonEmptyArray(state.selected.filter(c => c !== cell)); + const updatedSelection = state.selected.filter(c => c !== cell); + if (updatedSelection.length === 0) { + // All cells deselected - let invariant enforcement handle transition to NoCells + // If cells still exist, select the first one + const cells = this._cells.get(); + if (cells.length > 0) { + this._setState({ type: SelectionState.SingleSelection, selected: cells[0] }); + } + return; + } + const verifiedSelection = verifyNonEmptyArray(updatedSelection); // React will handle focus based on selection state change - this._setState({ type: updatedSelection.length === 1 ? SelectionState.SingleSelection : SelectionState.MultiSelection, selected: updatedSelection }); + if (verifiedSelection.length === 1) { + this._setState({ type: SelectionState.SingleSelection, selected: verifiedSelection[0] }); + } else { + this._setState({ type: SelectionState.MultiSelection, selected: verifiedSelection }); + } } // If the cell is not in the selection, do nothing. @@ -254,7 +272,7 @@ export class SelectionStateMachine extends Disposable { return; } - const cellToEdit = state.selected[0]; + const cellToEdit = state.selected; this._setState({ type: SelectionState.EditingSelection, selected: cellToEdit }); // Ensure editor is shown first (important for markdown cells and lazy-loaded editors) await cellToEdit.showEditor(); @@ -268,7 +286,7 @@ export class SelectionStateMachine extends Disposable { exitEditor(): void { const state = this._state.get(); if (state.type !== SelectionState.EditingSelection) { return; } - this._setState({ type: SelectionState.SingleSelection, selected: [state.selected] }); + this._setState({ type: SelectionState.SingleSelection, selected: state.selected }); } //#endregion Public Methods @@ -281,7 +299,7 @@ export class SelectionStateMachine extends Disposable { * @param cell The cell to select. */ private _selectCellNormal(cell: IPositronNotebookCell): void { - this._setState({ type: SelectionState.SingleSelection, selected: [cell] }); + this._setState({ type: SelectionState.SingleSelection, selected: cell }); } /** @@ -320,11 +338,12 @@ export class SelectionStateMachine extends Disposable { } // Check if cell is already selected - if (state.selected.includes(cell)) { + const selectedCells = state.type === SelectionState.SingleSelection ? [state.selected] : state.selected; + if (selectedCells.includes(cell)) { return; } - this._setState({ type: SelectionState.MultiSelection, selected: [...state.selected, cell] }); + this._setState({ type: SelectionState.MultiSelection, selected: verifyNonEmptyArray([...selectedCells, cell]) }); } /** @@ -349,7 +368,7 @@ export class SelectionStateMachine extends Disposable { const deletedCellIndex = previousCells.indexOf(state.selected); const cellToSelect = this._selectNeighboringCell(cells, deletedCellIndex); if (cellToSelect) { - this._setState({ type: SelectionState.SingleSelection, selected: [cellToSelect] }); + this._setState({ type: SelectionState.SingleSelection, selected: cellToSelect }); } // If no cell to select, invariant enforcement will handle transition to NoCells return; @@ -357,20 +376,25 @@ export class SelectionStateMachine extends Disposable { return; } - const newSelection = state.selected.filter(c => cells.includes(c)); + const currentSelection = getSelectedCells(state); + const newSelection = currentSelection.filter(c => cells.includes(c)); if (newSelection.length === 0) { // Cells were removed - select an appropriate neighboring cell // Use the index of the first selected cell that was removed in the previous array - const deletedCellIndex = previousCells.indexOf(state.selected[0]); + const deletedCellIndex = previousCells.indexOf(currentSelection[0]); const cellToSelect = this._selectNeighboringCell(cells, deletedCellIndex); if (cellToSelect) { - this._setState({ type: SelectionState.SingleSelection, selected: [cellToSelect] }); + this._setState({ type: SelectionState.SingleSelection, selected: cellToSelect }); } // If no cell to select, invariant enforcement will handle transition to NoCells return; } - this._setState({ type: newSelection.length === 1 ? SelectionState.SingleSelection : SelectionState.MultiSelection, selected: verifyNonEmptyArray(newSelection) }); + if (newSelection.length === 1) { + this._setState({ type: SelectionState.SingleSelection, selected: newSelection[0] }); + } else { + this._setState({ type: SelectionState.MultiSelection, selected: verifyNonEmptyArray(newSelection) }); + } } /** @@ -410,7 +434,7 @@ export class SelectionStateMachine extends Disposable { this._logService.debug('SelectionMachine: Auto-selecting first cell (cells appeared)'); this._setState({ type: SelectionState.SingleSelection, - selected: [cells[0]] + selected: cells[0] }); } } @@ -542,8 +566,10 @@ export class SelectionStateMachine extends Disposable { return; } - // Direct access is safe because NonEmptyArray guarantees at least one element - const edgeCell = state.selected[up ? 0 : state.selected.length - 1]; + // Direct access is safe because state invariants guarantee at least one element + const edgeCell = state.type === SelectionState.SingleSelection + ? state.selected + : state.selected[up ? 0 : state.selected.length - 1]; const indexOfEdgeCell = edgeCell.index; const nextCell = cells[indexOfEdgeCell + (up ? -1 : 1)]; @@ -557,7 +583,8 @@ export class SelectionStateMachine extends Disposable { // Already at the edge of the cells. return; } - const newSelection = verifyNonEmptyArray(up ? [nextCell, ...state.selected] : [...state.selected, nextCell]); + const currentSelection = getSelectedCells(state); + const newSelection = verifyNonEmptyArray(up ? [nextCell, ...currentSelection] : [...currentSelection, nextCell]); this._setState({ type: SelectionState.MultiSelection, selected: newSelection From 727fc8a35d1974ce1af315a4826fd437456d244c Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 12:12:11 -0400 Subject: [PATCH 12/16] Remove unneccesary comment --- .../positronNotebook/browser/selectionMachine.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts index 6ca922b572ea..886f05a15d80 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts @@ -227,12 +227,7 @@ export class SelectionStateMachine extends Disposable { if (state.type === SelectionState.MultiSelection) { const updatedSelection = state.selected.filter(c => c !== cell); if (updatedSelection.length === 0) { - // All cells deselected - let invariant enforcement handle transition to NoCells - // If cells still exist, select the first one - const cells = this._cells.get(); - if (cells.length > 0) { - this._setState({ type: SelectionState.SingleSelection, selected: cells[0] }); - } + // All cells deselected - let invariant enforcement handle transition return; } const verifiedSelection = verifyNonEmptyArray(updatedSelection); @@ -305,12 +300,6 @@ export class SelectionStateMachine extends Disposable { /** * Selects a cell for editing - enters edit mode with the specified cell. * - * Note: This method supports the NoCells → EditingSelection transition for cases where - * a cell is created in an empty notebook and immediately entered for editing (e.g., notebook - * initialization, or adding a cell after deleting all cells). This avoids forcing a two-step - * transition (NoCells → SingleSelection → EditingSelection) which would trigger unnecessary - * intermediate state updates and side effects. - * * @param cell The cell to select and edit. */ private _selectCellEdit(cell: IPositronNotebookCell): void { From 46c86690d928a6f115e16fac4dab286a2e82ccb1 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 12:32:49 -0400 Subject: [PATCH 13/16] Shore up logic around invariant enforcement --- .../browser/selectionMachine.ts | 47 +++++++++++++------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts index 886f05a15d80..c336afe0811f 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts @@ -159,7 +159,6 @@ export class SelectionStateMachine extends Disposable { this._register(autorunDelta(this._cells, ({ lastValue, newValue }) => { if (lastValue !== undefined) { this._setCells(newValue, lastValue); - // Enforce invariant after cell changes this._enforceInvariant(newValue); } })); @@ -215,19 +214,18 @@ export class SelectionStateMachine extends Disposable { (state.type === SelectionState.EditingSelection && state.selected === cell); if (deselectingCurrentSelection) { - // Don't manually set NoCells - let invariant enforcement handle it // If cells still exist, select the first one - const cells = this._cells.get(); - if (cells.length > 0) { - this._setState({ type: SelectionState.SingleSelection, selected: cells[0] }); - } + this._selectFirstCell(); + // Don't manually set NoCells - let invariant enforcement handle it return; } if (state.type === SelectionState.MultiSelection) { const updatedSelection = state.selected.filter(c => c !== cell); if (updatedSelection.length === 0) { - // All cells deselected - let invariant enforcement handle transition + // All cells deselected - if cells still exist, select the first one + this._selectFirstCell(); + // If no cells exist, invariant enforcement will handle transition to NoCells return; } const verifiedSelection = verifyNonEmptyArray(updatedSelection); @@ -336,7 +334,11 @@ export class SelectionStateMachine extends Disposable { } /** - * Updates the selection state when cells change. + * Updates the selection state when cells change (Phase 1 of cell updates). + * + * Handles selection updates when cells are added/removed but cells still exist. + * Intentionally delegates boundary conditions (transitions to/from NoCells state) + * to _enforceInvariant, which is always called immediately after this method. * * @param cells The new cells array. * @param previousCells The previous cells array. @@ -386,6 +388,19 @@ export class SelectionStateMachine extends Disposable { } } + /** + * Selects the first cell if cells exist, transitioning to SingleSelection state. + * @returns True if a cell was selected, false if no cells exist + */ + private _selectFirstCell(): boolean { + const cells = this._cells.get(); + if (cells.length > 0) { + this._setState({ type: SelectionState.SingleSelection, selected: cells[0] }); + return true; + } + return false; + } + /** * Selects an appropriate neighboring cell when the current selection is removed. * @param cells The current cells array @@ -407,8 +422,15 @@ export class SelectionStateMachine extends Disposable { } /** - * Enforces the invariant: NoCells ↔ cells.length === 0 - * Called automatically when cells array changes + * Enforces the invariant: NoCells ↔ cells.length === 0 (Phase 2 of cell updates). + * + * Invariants in state machines are conditions that must always be true regardless of state transitions. + * This invariant ensures the selection state accurately reflects cell existence - we cannot be + * in NoCells state when cells exist, and we cannot have a selection when no cells exist. + * + * This method handles boundary conditions when transitioning between empty/non-empty cell arrays. + * Always called after _setCells to maintain separation of concerns: _setCells handles selection + * logic for existing cells, this method handles the boundary enforcement. */ private _enforceInvariant(cells: IPositronNotebookCell[]): void { const currentState = this._state.get(); @@ -421,10 +443,7 @@ export class SelectionStateMachine extends Disposable { else if (cells.length > 0 && currentState.type === SelectionState.NoCells) { // Cells appeared → automatically select first cell this._logService.debug('SelectionMachine: Auto-selecting first cell (cells appeared)'); - this._setState({ - type: SelectionState.SingleSelection, - selected: cells[0] - }); + this._selectFirstCell(); } } From b463d73f1d874d81c41d20f6f0cca658471a6286 Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 13:27:27 -0400 Subject: [PATCH 14/16] Restructure invariant enforecement to all state changes --- .../browser/selectionMachine.ts | 129 +++++++++++------- 1 file changed, 80 insertions(+), 49 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts index c336afe0811f..212ea13f220f 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts @@ -159,7 +159,6 @@ export class SelectionStateMachine extends Disposable { this._register(autorunDelta(this._cells, ({ lastValue, newValue }) => { if (lastValue !== undefined) { this._setCells(newValue, lastValue); - this._enforceInvariant(newValue); } })); } @@ -334,11 +333,10 @@ export class SelectionStateMachine extends Disposable { } /** - * Updates the selection state when cells change (Phase 1 of cell updates). + * Updates the selection state when cells change. * - * Handles selection updates when cells are added/removed but cells still exist. - * Intentionally delegates boundary conditions (transitions to/from NoCells state) - * to _enforceInvariant, which is always called immediately after this method. + * Computes the intended selection state based on which cells were added/removed, + * then delegates to _setState which handles invariant enforcement. * * @param cells The new cells array. * @param previousCells The previous cells array. @@ -346,41 +344,39 @@ export class SelectionStateMachine extends Disposable { private _setCells(cells: IPositronNotebookCell[], previousCells: IPositronNotebookCell[]): void { const state = this._state.get(); - // Let invariant enforcement handle NoCells transitions - if (state.type === SelectionState.NoCells) { + // If no cells existed and none exist now, nothing to do + if (state.type === SelectionState.NoCells && cells.length === 0) { + return; + } + + // If we went from NoCells to having cells, _setState will auto-select first cell + if (state.type === SelectionState.NoCells && cells.length > 0) { + // Delegate to _setState with any valid state - it will correct to SingleSelection + this._setState({ type: SelectionState.SingleSelection, selected: cells[0] }); return; } - // If we're editing a cell when setCells is called. We need to check if the cell is still in the new cells. - // If it isn't we need to select an appropriate neighboring cell. + // If we're editing a cell when cells change, check if that cell still exists if (state.type === SelectionState.EditingSelection) { if (!cells.includes(state.selected)) { - // Find the index where the deleted cell was in the previous array - const deletedCellIndex = previousCells.indexOf(state.selected); - const cellToSelect = this._selectNeighboringCell(cells, deletedCellIndex); - if (cellToSelect) { - this._setState({ type: SelectionState.SingleSelection, selected: cellToSelect }); - } - // If no cell to select, invariant enforcement will handle transition to NoCells - return; + // Cell being edited was removed - handle selection removal + this._handleSelectionRemoved(state.selected, cells, previousCells); } + // Cell still exists, keep current state return; } + // Filter current selection to only include cells that still exist const currentSelection = getSelectedCells(state); const newSelection = currentSelection.filter(c => cells.includes(c)); + if (newSelection.length === 0) { - // Cells were removed - select an appropriate neighboring cell - // Use the index of the first selected cell that was removed in the previous array - const deletedCellIndex = previousCells.indexOf(currentSelection[0]); - const cellToSelect = this._selectNeighboringCell(cells, deletedCellIndex); - if (cellToSelect) { - this._setState({ type: SelectionState.SingleSelection, selected: cellToSelect }); - } - // If no cell to select, invariant enforcement will handle transition to NoCells + // All selected cells were removed - handle selection removal + this._handleSelectionRemoved(currentSelection[0], cells, previousCells); return; } + // Update selection with remaining cells if (newSelection.length === 1) { this._setState({ type: SelectionState.SingleSelection, selected: newSelection[0] }); } else { @@ -422,30 +418,25 @@ export class SelectionStateMachine extends Disposable { } /** - * Enforces the invariant: NoCells ↔ cells.length === 0 (Phase 2 of cell updates). - * - * Invariants in state machines are conditions that must always be true regardless of state transitions. - * This invariant ensures the selection state accurately reflects cell existence - we cannot be - * in NoCells state when cells exist, and we cannot have a selection when no cells exist. - * - * This method handles boundary conditions when transitioning between empty/non-empty cell arrays. - * Always called after _setCells to maintain separation of concerns: _setCells handles selection - * logic for existing cells, this method handles the boundary enforcement. + * Handles the case where the current selection (editing or otherwise) is removed due to cell deletion. + * Selects a neighboring cell if possible, or transitions to NoCells state if none remain. + * @param removedCell The cell that was deleted (or first of current selection) + * @param cells The new array of cells + * @param previousCells The previous array of cells */ - private _enforceInvariant(cells: IPositronNotebookCell[]): void { - const currentState = this._state.get(); - - if (cells.length === 0 && currentState.type !== SelectionState.NoCells) { - // Cells disappeared → force NoCells state - this._logService.debug('SelectionMachine: Enforcing NoCells state (no cells exist)'); + private _handleSelectionRemoved(removedCell: IPositronNotebookCell | undefined, cells: IPositronNotebookCell[], previousCells: IPositronNotebookCell[]): void { + if (!removedCell) { this._setState({ type: SelectionState.NoCells }); + return; } - else if (cells.length > 0 && currentState.type === SelectionState.NoCells) { - // Cells appeared → automatically select first cell - this._logService.debug('SelectionMachine: Auto-selecting first cell (cells appeared)'); - this._selectFirstCell(); + const removedCellIndex = previousCells.indexOf(removedCell); + const cellToSelect = this._selectNeighboringCell(cells, removedCellIndex); + if (cellToSelect) { + this._setState({ type: SelectionState.SingleSelection, selected: cellToSelect }); + } else { + this._setState({ type: SelectionState.NoCells }); } - } + }; /** * Validates whether a transition from one state to another is valid. @@ -482,10 +473,46 @@ export class SelectionStateMachine extends Disposable { return validTransitions[from].includes(to); } + /** + * Validates and corrects state to maintain invariants. + * + * This is the single source of truth for what constitutes a valid state. + * All state changes MUST go through this validation to ensure invariants. + * + * Core invariant: NoCells ↔ cells.length === 0 + * + * @param intended The state that was requested + * @param cells The current cells array + * @returns A valid state (either the intended state or a corrected version) + */ + private _validateAndCorrect( + intended: SelectionStates, + cells: IPositronNotebookCell[] + ): SelectionStates { + // Invariant: NoCells ↔ cells.length === 0 + if (cells.length === 0) { + // No cells exist → MUST be NoCells + if (intended.type !== SelectionState.NoCells) { + this._logService.debug('SelectionMachine: Auto-correcting to NoCells (no cells exist)'); + } + return { type: SelectionState.NoCells }; + } + + if (intended.type === SelectionState.NoCells) { + // NoCells but cells exist → MUST select something + this._logService.debug('SelectionMachine: Auto-correcting from NoCells (cells exist)'); + return { type: SelectionState.SingleSelection, selected: cells[0] }; + } + + // State is valid + return intended; + } + private _setState(state: SelectionStates) { const currentState = this._state.get(); + const cells = this._cells.get(); - // Validate transition + // Step 1: Validate transition is legal if (!this._isValidTransition(currentState.type, state.type)) { const message = `SelectionMachine: Invalid state transition from ${currentState.type} to ${state.type}`; @@ -494,12 +521,16 @@ export class SelectionStateMachine extends Disposable { throw new Error(message); } - // In production, log a warning but allow the transition + // In production, log a warning but don't apply invalid transition this._logService.warn(message); + return; } - // Alert the observable that the state has changed. - this._state.set(state, undefined); + // Step 2: Validate and correct state to maintain invariants + const correctedState = this._validateAndCorrect(state, cells); + + // Step 3: Apply the corrected state + this._state.set(correctedState, undefined); } /** From efe6011e5b8d0acf0947c15b452815ebc74002df Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 13:35:36 -0400 Subject: [PATCH 15/16] Move valid selection states to module-wide map for clearer state machine rules --- .../browser/selectionMachine.ts | 62 ++++++++----------- 1 file changed, 26 insertions(+), 36 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts index 212ea13f220f..ac848fd0efea 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts @@ -35,6 +35,31 @@ function verifyNonEmptyArray(array: T[]): NonEmptyArray { return array as NonEmptyArray; } +// Valid selection state transitions +const ValidSelectionStateTransitions: Record = { + [SelectionState.NoCells]: [ + SelectionState.NoCells, // Can stay in NoCells + SelectionState.SingleSelection, // Cells appear → select first + SelectionState.EditingSelection, // Cell created in empty notebook and immediately edited + ], + [SelectionState.SingleSelection]: [ + SelectionState.SingleSelection, // Select different cell + SelectionState.MultiSelection, // Add cell to selection + SelectionState.EditingSelection, // Enter edit mode + SelectionState.NoCells, // All cells removed + ], + [SelectionState.MultiSelection]: [ + SelectionState.MultiSelection, // Modify selection + SelectionState.SingleSelection, // Reduce to single cell + SelectionState.NoCells, // All cells removed + ], + [SelectionState.EditingSelection]: [ + SelectionState.EditingSelection, // Can stay in editing (same cell) + SelectionState.SingleSelection, // Exit editor + SelectionState.NoCells, // Cell being edited removed + ], +}; + /** * Selection state discriminated union. * @@ -438,41 +463,6 @@ export class SelectionStateMachine extends Disposable { } }; - /** - * Validates whether a transition from one state to another is valid. - * @param from The source state - * @param to The destination state - * @returns True if the transition is valid, false otherwise - */ - private _isValidTransition(from: SelectionState, to: SelectionState): boolean { - // Define valid transitions as a map - const validTransitions: Record = { - [SelectionState.NoCells]: [ - SelectionState.NoCells, // Can stay in NoCells - SelectionState.SingleSelection, // Cells appear → select first - SelectionState.EditingSelection, // Cell created in empty notebook and immediately edited - ], - [SelectionState.SingleSelection]: [ - SelectionState.SingleSelection, // Select different cell - SelectionState.MultiSelection, // Add cell to selection - SelectionState.EditingSelection, // Enter edit mode - SelectionState.NoCells, // All cells removed - ], - [SelectionState.MultiSelection]: [ - SelectionState.MultiSelection, // Modify selection - SelectionState.SingleSelection, // Reduce to single cell - SelectionState.NoCells, // All cells removed - ], - [SelectionState.EditingSelection]: [ - SelectionState.EditingSelection, // Can stay in editing (same cell) - SelectionState.SingleSelection, // Exit editor - SelectionState.NoCells, // Cell being edited removed - ], - }; - - return validTransitions[from].includes(to); - } - /** * Validates and corrects state to maintain invariants. * @@ -513,7 +503,7 @@ export class SelectionStateMachine extends Disposable { const cells = this._cells.get(); // Step 1: Validate transition is legal - if (!this._isValidTransition(currentState.type, state.type)) { + if (!ValidSelectionStateTransitions[currentState.type].includes(state.type)) { const message = `SelectionMachine: Invalid state transition from ${currentState.type} to ${state.type}`; // In development mode, throw an error to catch bugs early From f54c49de905c39e52a76447fae292e1dcbe519ca Mon Sep 17 00:00:00 2001 From: Nick Strayer Date: Tue, 7 Oct 2025 13:47:38 -0400 Subject: [PATCH 16/16] housekeeping --- .../browser/selectionMachine.ts | 92 +++++++++---------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts index ac848fd0efea..91a0dc49a771 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts @@ -9,12 +9,8 @@ import { Disposable } from '../../../../base/common/lifecycle.js'; import { IEnvironmentService } from '../../../../platform/environment/common/environment.js'; /** - * STATE MACHINE SPECIFICATION - * =========================== - * - * This file implements a selection state machine for Positron notebooks. - * - **/ + * Represents the possible selection states for the notebook. + */ export enum SelectionState { NoCells = 'NoCells', SingleSelection = 'SingleSelection', @@ -22,20 +18,30 @@ export enum SelectionState { EditingSelection = 'EditingSelection' } -type NonEmptyArray = [T, ...T[]]; - -function assertNonEmptyArray(array: T[]): asserts array is NonEmptyArray { - if (array.length === 0) { - throw new Error('Array must be non-empty'); +/** + * Selection state discriminated union. + */ +type SelectionStates = + | { + type: SelectionState.NoCells; } -} + | { + type: SelectionState.SingleSelection; + selected: IPositronNotebookCell; + } + | { + type: SelectionState.MultiSelection; + selected: NonEmptyArray; + } + | { + type: SelectionState.EditingSelection; + selected: IPositronNotebookCell; + }; -function verifyNonEmptyArray(array: T[]): NonEmptyArray { - assertNonEmptyArray(array); - return array as NonEmptyArray; -} -// Valid selection state transitions +/** + * Valid selection state transitions. + */ const ValidSelectionStateTransitions: Record = { [SelectionState.NoCells]: [ SelectionState.NoCells, // Can stay in NoCells @@ -60,43 +66,37 @@ const ValidSelectionStateTransitions: Record = ], }; + /** - * Selection state discriminated union. - * - * Design rationale for field types: - * - SingleSelection uses `selected: IPositronNotebookCell` (singular) for type safety, - * ensuring compile-time enforcement that exactly one cell is selected. - * - MultiSelection uses `selected: NonEmptyArray` (array) to ensure - * at least one cell is selected and enable filtering and equality checks. - * - EditingSelection uses `selected: IPositronNotebookCell` (singular) for type safety, - * ensuring compile-time enforcement that exactly one cell is being edited. - * - * This design provides type safety while maintaining clear distinctions between - * single and multi-cell selection states. + * Defines the different modes of cell selection operations in the notebook. + * Used to specify how a cell selection should be applied when selecting cells. */ -type SelectionStates = - | { - type: SelectionState.NoCells; - } - | { - type: SelectionState.SingleSelection; - selected: IPositronNotebookCell; - } - | { - type: SelectionState.MultiSelection; - selected: NonEmptyArray; - } - | { - type: SelectionState.EditingSelection; - selected: IPositronNotebookCell; - }; - export enum CellSelectionType { + /** Adds a cell to the current selection (enables multi-selection mode) */ Add = 'Add', + /** Selects a cell and immediately enters edit mode */ Edit = 'Edit', + /** Performs a normal selection, replacing any current selection with the specified cell */ Normal = 'Normal' } +/** + * A non-empty array type. + */ +type NonEmptyArray = [T, ...T[]]; + +/** + * Verifies that an array is non-empty and returns a non-empty array. + * @param array The array to verify. + * @returns The non-empty array. + */ +function verifyNonEmptyArray(array: T[]): NonEmptyArray { + if (array.length === 0) { + throw new Error('Array must be non-empty'); + } + return array as NonEmptyArray; +} + /** * Get all selected cells based on the current selection state. * @param state The selection state to extract cells from