diff --git a/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/IPositronNotebookCell.ts b/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/IPositronNotebookCell.ts index 925011458a88..70f2f53138df 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/IPositronNotebookCell.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/IPositronNotebookCell.ts @@ -165,6 +165,11 @@ export interface IPositronNotebookCell extends Disposable { */ attachContainer(container: HTMLElement): void; + /** + * Get the container that the cell is attached to + */ + get container(): HTMLElement | undefined; + /** * * @param editor Code editor widget associated with cell. diff --git a/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/PositronNotebookCell.ts b/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/PositronNotebookCell.ts index ce40bc69cc3d..3f5d6c511b85 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/PositronNotebookCell.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookCells/PositronNotebookCell.ts @@ -143,6 +143,9 @@ export abstract class PositronNotebookCellGeneral extends Disposable implements this._container = container; } + get container(): HTMLElement | undefined { + return this._container; + } attachEditor(editor: CodeEditorWidget): void { this._editor.set(editor, undefined); diff --git a/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookEditor.tsx b/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookEditor.tsx index 629b1c8c0e7e..4089ae68ec3b 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookEditor.tsx +++ b/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookEditor.tsx @@ -277,13 +277,20 @@ export class PositronNotebookEditor extends AbstractEditorWithViewState diff --git a/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookInstance.ts b/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookInstance.ts index 08354d05478b..dbd91e0c2b6d 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookInstance.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/PositronNotebookInstance.ts @@ -979,6 +979,41 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot this._positronConsoleService.showNotebookConsole(this.uri, true); } + /** + * Grabs focus for this notebook based on the current selection state. + * Called when the notebook editor receives focus from the workbench. + * + * Note: This method may be called twice during tab switches: + * - First call: Early, cells may not be rendered yet (no-op via optional chaining) + * - Second call: After render completes, focus succeeds + */ + grabFocus(): void { + const state = this.selectionStateMachine.state.get(); + + switch (state.type) { + case SelectionState.EditingSelection: + // Focus the editor - enterEditor() already has idempotency checks + this.selectionStateMachine.enterEditor(state.selected); + break; + + case SelectionState.SingleSelection: + case SelectionState.MultiSelection: { + // Focus the first selected cell's container + // Optional chaining handles undefined containers gracefully + const cell = state.type === SelectionState.SingleSelection + ? state.selected + : state.selected[0]; + cell.container?.focus(); + break; + } + + case SelectionState.NoCells: + // Fall back to notebook container + this._container?.focus(); + break; + } + } + /** * Clears the outputs of all cells in the notebook. */ diff --git a/src/vs/workbench/contrib/positronNotebook/browser/SelectPositronNotebookKernelAction.ts b/src/vs/workbench/contrib/positronNotebook/browser/SelectPositronNotebookKernelAction.ts index d14ee3b35cfa..d260702e861f 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/SelectPositronNotebookKernelAction.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/SelectPositronNotebookKernelAction.ts @@ -11,8 +11,9 @@ import { IQuickInputService, IQuickPickItem } from '../../../../platform/quickin import { selectKernelIcon } from '../../notebook/browser/notebookIcons.js'; import { INotebookKernelService, INotebookKernel } from '../../notebook/common/notebookKernelService.js'; import { PositronNotebookInstance } from './PositronNotebookInstance.js'; -import { IPositronNotebookService } from './positronNotebookService.js'; import { POSITRON_RUNTIME_NOTEBOOK_KERNELS_EXTENSION_ID } from '../../runtimeNotebookKernel/common/runtimeNotebookKernelConfig.js'; +import { IEditorService } from '../../../services/editor/common/editorService.js'; +import { getNotebookInstanceFromEditorPane } from './notebookUtils.js'; export const SELECT_KERNEL_ID_POSITRON = 'positronNotebook.selectKernel'; const NOTEBOOK_ACTIONS_CATEGORY_POSITRON = localize2('positronNotebookActions.category', 'Positron Notebook'); @@ -38,8 +39,7 @@ class SelectPositronNotebookKernelAction extends Action2 { async run(accessor: ServicesAccessor, context?: SelectPositronNotebookKernelContext): Promise { const { forceDropdown } = context || { forceDropdown: false }; const notebookKernelService = accessor.get(INotebookKernelService); - const notebookService = accessor.get(IPositronNotebookService); - const activeNotebook = notebookService.getActiveInstance(); + const activeNotebook = getNotebookInstanceFromEditorPane(accessor.get(IEditorService)); const quickInputService = accessor.get(IQuickInputService); if (!activeNotebook) { diff --git a/src/vs/workbench/contrib/positronNotebook/browser/contrib/undoRedo/positronNotebookUndoRedo.ts b/src/vs/workbench/contrib/positronNotebook/browser/contrib/undoRedo/positronNotebookUndoRedo.ts index 686d3ece6303..3aea384f1198 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/contrib/undoRedo/positronNotebookUndoRedo.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/contrib/undoRedo/positronNotebookUndoRedo.ts @@ -6,10 +6,11 @@ import { Disposable } from '../../../../../../base/common/lifecycle.js'; import { WorkbenchPhase, registerWorkbenchContribution2 } from '../../../../../common/contributions.js'; import { UndoCommand, RedoCommand } from '../../../../../../editor/browser/editorExtensions.js'; -import { IPositronNotebookService } from '../../positronNotebookService.js'; import { POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED, POSITRON_NOTEBOOK_CELL_EDITOR_FOCUSED } from '../../ContextKeysManager.js'; import { IUndoRedoService } from '../../../../../../platform/undoRedo/common/undoRedo.js'; import { IContextKeyService } from '../../../../../../platform/contextkey/common/contextkey.js'; +import { IEditorService } from '../../../../../services/editor/common/editorService.js'; +import { getNotebookInstanceFromEditorPane } from '../../notebookUtils.js'; class PositronNotebookUndoRedoContribution extends Disposable { @@ -17,7 +18,7 @@ class PositronNotebookUndoRedoContribution extends Disposable { constructor( @IUndoRedoService private readonly undoRedoService: IUndoRedoService, - @IPositronNotebookService private readonly positronNotebookService: IPositronNotebookService, + @IEditorService private readonly editorService: IEditorService, @IContextKeyService private readonly contextKeyService: IContextKeyService ) { super(); @@ -30,7 +31,7 @@ class PositronNotebookUndoRedoContribution extends Disposable { private shouldHandleUndoRedo(): boolean { // Get the active notebook instance to access its scoped context key service - const instance = this.positronNotebookService.getActiveInstance(); + const instance = getNotebookInstanceFromEditorPane(this.editorService); if (!instance) { return false; } @@ -61,7 +62,7 @@ class PositronNotebookUndoRedoContribution extends Disposable { return false; } - const instance = this.positronNotebookService.getActiveInstance(); + const instance = getNotebookInstanceFromEditorPane(this.editorService); if (!instance) { return false; } @@ -79,7 +80,7 @@ class PositronNotebookUndoRedoContribution extends Disposable { return false; } - const instance = this.positronNotebookService.getActiveInstance(); + const instance = getNotebookInstanceFromEditorPane(this.editorService); if (!instance) { return false; } diff --git a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/actionBar/registerCellCommand.ts b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/actionBar/registerCellCommand.ts index a5ca25a3369f..b873003f55d0 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/actionBar/registerCellCommand.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/notebookCells/actionBar/registerCellCommand.ts @@ -5,7 +5,6 @@ import { CommandsRegistry, ICommandMetadata } from '../../../../../../platform/commands/common/commands.js'; import { ServicesAccessor } from '../../../../../../platform/instantiation/common/instantiation.js'; -import { IPositronNotebookService } from '../../positronNotebookService.js'; import { IPositronNotebookCell } from '../../PositronNotebookCells/IPositronNotebookCell.js'; import { NotebookCellActionBarRegistry, INotebookCellActionBarItem } from './actionBarRegistry.js'; import { IDisposable, DisposableStore } from '../../../../../../base/common/lifecycle.js'; @@ -15,6 +14,8 @@ import { IPositronNotebookCommandKeybinding } from './commandUtils.js'; import { IPositronNotebookInstance } from '../../IPositronNotebookInstance.js'; import { getSelectedCell, getSelectedCells, getEditingCell } from '../../selectionMachine.js'; import { ContextKeyExpr, ContextKeyExpression } from '../../../../../../platform/contextkey/common/contextkey.js'; +import { IEditorService } from '../../../../../services/editor/common/editorService.js'; +import { getNotebookInstanceFromEditorPane } from '../../notebookUtils.js'; /** * Options for registering a cell command. @@ -80,8 +81,8 @@ export function registerCellCommand({ const commandDisposable = CommandsRegistry.registerCommand({ id: commandId, handler: (accessor: ServicesAccessor) => { - const notebookService = accessor.get(IPositronNotebookService); - const activeNotebook = notebookService.getActiveInstance(); + const editorService = accessor.get(IEditorService); + const activeNotebook = getNotebookInstanceFromEditorPane(editorService); if (!activeNotebook) { return; } diff --git a/src/vs/workbench/contrib/positronNotebook/browser/notebookUtils.ts b/src/vs/workbench/contrib/positronNotebook/browser/notebookUtils.ts new file mode 100644 index 000000000000..6b20aee15865 --- /dev/null +++ b/src/vs/workbench/contrib/positronNotebook/browser/notebookUtils.ts @@ -0,0 +1,28 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (C) 2025 Posit Software, PBC. All rights reserved. + * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. + *--------------------------------------------------------------------------------------------*/ + +import { IEditorService } from '../../../services/editor/common/editorService.js'; +import { IPositronNotebookInstance } from './IPositronNotebookInstance.js'; +import { PositronNotebookEditor } from './PositronNotebookEditor.js'; +import { POSITRON_NOTEBOOK_EDITOR_ID } from '../common/positronNotebookCommon.js'; + +/** + * Retrieves the active Positron notebook instance from the editor service. + * + * @param editorService The editor service + * @returns The active notebook instance, or undefined if no Positron notebook is active + */ +export function getNotebookInstanceFromEditorPane(editorService: IEditorService): IPositronNotebookInstance | undefined { + const activeEditorPane = editorService.activeEditorPane; + + // Check if the active editor is a Positron Notebook Editor + if (!activeEditorPane || activeEditorPane.getId() !== POSITRON_NOTEBOOK_EDITOR_ID) { + return undefined; + } + + // Extract the notebook instance from the editor + const activeNotebook = (activeEditorPane as PositronNotebookEditor).notebookInstance; + return activeNotebook; +} diff --git a/src/vs/workbench/contrib/positronNotebook/browser/positronNotebookService.ts b/src/vs/workbench/contrib/positronNotebook/browser/positronNotebookService.ts index 8dc4150beb15..39ecbc6b7792 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/positronNotebookService.ts +++ b/src/vs/workbench/contrib/positronNotebook/browser/positronNotebookService.ts @@ -30,11 +30,6 @@ export interface IPositronNotebookService { */ listInstances(uri?: URI): Array; - /** - * Get the currently active notebook instance, if it exists. - */ - getActiveInstance(): IPositronNotebookInstance | null; - /** * Register a new notebook instance. * @param instance The instance to register. @@ -68,7 +63,6 @@ class PositronNotebookService extends Disposable implements IPositronNotebookSer constructor( @IConfigurationService private readonly _configurationService: IConfigurationService ) { - // Call the disposable constrcutor. super(); } @@ -90,10 +84,6 @@ class PositronNotebookService extends Disposable implements IPositronNotebookSer return instances; } - public getActiveInstance(): IPositronNotebookInstance | null { - return this._activeInstance; - } - public registerInstance(instance: IPositronNotebookInstance): void { if (!this._instanceById.has(instance.id)) { this._instanceById.set(instance.id, instance); diff --git a/src/vs/workbench/contrib/positronNotebook/browser/registerNotebookAction.tsx b/src/vs/workbench/contrib/positronNotebook/browser/registerNotebookAction.tsx index d5a3393409a1..b27b38737967 100644 --- a/src/vs/workbench/contrib/positronNotebook/browser/registerNotebookAction.tsx +++ b/src/vs/workbench/contrib/positronNotebook/browser/registerNotebookAction.tsx @@ -13,11 +13,12 @@ import { ContextKeyExpr, ContextKeyExpression } from '../../../../platform/conte import { ServicesAccessor } from '../../../../platform/instantiation/common/instantiation.js'; import { KeybindingsRegistry, KeybindingWeight } from '../../../../platform/keybinding/common/keybindingsRegistry.js'; import { PositronActionBarWidgetRegistry } from '../../../../platform/positronActionBar/browser/positronActionBarWidgetRegistry.js'; -import { IPositronNotebookService } from './positronNotebookService.js'; import { POSITRON_NOTEBOOK_EDITOR_CONTAINER_FOCUSED } from './ContextKeysManager.js'; import { POSITRON_NOTEBOOK_EDITOR_ID } from '../common/positronNotebookCommon.js'; import { IPositronNotebookInstance } from './IPositronNotebookInstance.js'; import { NotebookInstanceProvider } from './NotebookInstanceProvider.js'; +import { IEditorService } from '../../../services/editor/common/editorService.js'; +import { getNotebookInstanceFromEditorPane } from './notebookUtils.js'; /** * Keybinding configuration for notebook actions. @@ -260,8 +261,8 @@ function registerNotebookCommandInternal(options: INotebookCommandOptions): IDis const commandDisposable = CommandsRegistry.registerCommand({ id: options.commandId, handler: (accessor: ServicesAccessor) => { - const notebookService = accessor.get(IPositronNotebookService); - const activeNotebook = notebookService.getActiveInstance(); + const editorService = accessor.get(IEditorService); + const activeNotebook = getNotebookInstanceFromEditorPane(editorService); if (!activeNotebook) { return; } @@ -354,11 +355,9 @@ function registerNotebookWidgetInternal(options: INotebookWidgetOptions): IDispo componentFactory: (accessor) => { // Return a wrapper component that provides notebook context return () => { - // Get the active notebook instance from the service - const notebookService = accessor.get(IPositronNotebookService); - const notebook = notebookService.getActiveInstance(); - - // If no active notebook, don't render anything + // Get the active notebook using the VS Code pattern + const editorService = accessor.get(IEditorService); + const notebook = getNotebookInstanceFromEditorPane(editorService); if (!notebook) { return null; } 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 52a6a44c8017..72e89f9947aa 100644 --- a/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts +++ b/test/e2e/tests/notebook/notebook-focus-and-selection.test.ts @@ -191,8 +191,9 @@ test.describe('Notebook Focus and Selection', { const { notebooks, notebooksPositron } = app.workbench; const keyboard = app.code.driver.page.keyboard; - const clickTab = (name: string) => app.code.driver.page.getByRole('tab', { name }).click(); - const TAB_1 = 'Untitled-1.ipynb'; + const clickTab = (name: string | RegExp) => app.code.driver.page.getByRole('tab', { name }).click(); + // Depending on when this test is run, the untitled notebook may have a different number + const TAB_1 = /Untitled-\d+\.ipynb/; const TAB_2 = 'bitmap-notebook.ipynb'; // Start a new notebook (tab 1) @@ -216,9 +217,8 @@ test.describe('Notebook Focus and Selection', { await notebooksPositron.expectCellIndexToBeSelected(0, { inEditMode: false }); }); - // BUG: https://github.com/posit-dev/positron/issues/9849 // Switch between notebooks to ensure selection is preserved - await test.step.skip('Selection is preserved when switching between editors', async () => { + await test.step('Selection is preserved when switching between editors', async () => { // Switch back to tab 1 and verify selection is still at cell 2 await clickTab(TAB_1); await notebooksPositron.expectCellIndexToBeSelected(2, { inEditMode: false });