From aadde96e4e01e2be23342845b214009bb3236e00 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Wed, 28 Oct 2026 17:40:06 -0700 Subject: [PATCH] fix(editor): keep notebook cell undo across cell and tab switches (#34952) Notebook cell models were anonymous and disposed whenever a cell lost focus or the notebook view unmounted on a tab switch, so Undo did nothing after returning to a cell. Give cells that have a stable nbformat id a vscode-userdata model URI, scoped to the view, so Monaco's model service keeps and restores the disposed model's undo stack. Monaco restores only when the text is unchanged and bounds retained history to its closed-file budget. Id-less cells keep anonymous models because their position-based key can move to a different cell, and a duplicate id falls back to an anonymous model instead of colliding. Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- .../IpynbCellEditor.external-sync.test.tsx | 123 +++++++++++++++++- .../src/components/editor/IpynbCellEditor.tsx | 14 +- .../components/editor/IpynbRendering.test.tsx | 1 + .../src/components/editor/IpynbViewer.tsx | 3 + 4 files changed, 138 insertions(+), 3 deletions(-) diff --git a/src/renderer/src/components/editor/IpynbCellEditor.external-sync.test.tsx b/src/renderer/src/components/editor/IpynbCellEditor.external-sync.test.tsx index 777e452ee1e..699d1b878ed 100644 --- a/src/renderer/src/components/editor/IpynbCellEditor.external-sync.test.tsx +++ b/src/renderer/src/components/editor/IpynbCellEditor.external-sync.test.tsx @@ -82,7 +82,7 @@ vi.mock('@/lib/monaco-setup', async () => { }) function notebookContent( - cells: { id: string; source: string; kind?: 'code' | 'markdown' }[], + cells: { id: string | null; source: string; kind?: 'code' | 'markdown' }[], language = 'python' ): string { return JSON.stringify({ @@ -90,7 +90,7 @@ function notebookContent( nbformat_minor: 5, metadata: { language_info: { name: language } }, cells: cells.map(({ id, source, kind }) => ({ - id, + ...(id === null ? {} : { id }), cell_type: kind ?? 'code', metadata: {}, execution_count: null, @@ -126,6 +126,7 @@ function NotebookCells({ return ( { + modelScope += 1 vi.useFakeTimers() vi.clearAllMocks() }) @@ -314,3 +318,118 @@ describe('active notebook cell external reload', () => { expect(onContentChange).not.toHaveBeenCalled() }) }) + +describe('notebook cell undo lifetime', () => { + const callbacks = () => ({ + onContentChange: vi.fn<(content: string) => void>(), + onDirtyStateHint: vi.fn<(dirty: boolean) => void>() + }) + + async function undo(model: editor.ITextModel): Promise { + await act(async () => { + await model.undo() + }) + } + + it('restores cell undo after switching cells and after the notebook view remounts', async () => { + const handlers = callbacks() + const content = notebookContent([ + { id: 'a', source: 'first source' }, + { id: 'b', source: 'second source' } + ]) + const firstView = render() + fireEvent.mouseDown(screen.getByRole('button', { name: 'first source' }), { button: 0 }) + appendText(activeModel(), '!') + fireEvent.mouseDown(screen.getByRole('button', { name: 'second source' }), { button: 0 }) + appendText(activeModel(), '?') + fireEvent.mouseDown(screen.getByRole('button', { name: 'first source!' }), { button: 0 }) + await undo(activeModel()) + expect(activeModel().getValue()).toBe('first source') + expect(screen.getByRole('button', { name: 'second source?' })).toBeDefined() + + const edited = handlers.onContentChange.mock.calls.at(-1)?.[0] ?? '' + firstView.unmount() + render() + fireEvent.mouseDown(screen.getByRole('button', { name: 'second source?' }), { button: 0 }) + await undo(activeModel()) + expect(activeModel().getValue()).toBe('second source') + }) + + it('drops cell undo when the source changed while the cell was inactive', async () => { + const handlers = callbacks() + const { rerender } = render( + + ) + fireEvent.mouseDown(screen.getByRole('button', { name: 'original' }), { button: 0 }) + appendText(activeModel(), '!') + fireEvent.mouseDown(screen.getByRole('button', { name: 'other' }), { button: 0 }) + rerender( + + ) + rerender( + + ) + fireEvent.mouseDown(screen.getByRole('button', { name: 'from disk' }), { button: 0 }) + await undo(activeModel()) + expect(activeModel().getValue()).toBe('from disk') + }) + + it('does not carry undo to another id-less cell that moves into the same position', async () => { + const handlers = callbacks() + const { rerender } = render( + + ) + fireEvent.mouseDown(screen.getByRole('button', { name: 'text' }), { button: 0 }) + appendText(activeModel(), '!') + fireEvent.mouseDown(screen.getByRole('button', { name: 'text!' }), { button: 0 }) + rerender( + + ) + rerender( + + ) + fireEvent.mouseDown(screen.getByRole('button', { name: 'text!' }), { button: 0 }) + handlers.onContentChange.mockClear() + await undo(activeModel()) + expect(activeModel().getValue()).toBe('text!') + expect(handlers.onContentChange).not.toHaveBeenCalled() + }) + + it('opens a cell whose id is duplicated in the notebook', () => { + render( + + ) + fireEvent.mouseDown(screen.getByRole('button', { name: 'first' }), { button: 0 }) + expect(liveModels.map((model) => model.getValue())).toEqual(['first', 'second']) + }) +}) diff --git a/src/renderer/src/components/editor/IpynbCellEditor.tsx b/src/renderer/src/components/editor/IpynbCellEditor.tsx index 8758ca37ad2..238bc4d5a8e 100644 --- a/src/renderer/src/components/editor/IpynbCellEditor.tsx +++ b/src/renderer/src/components/editor/IpynbCellEditor.tsx @@ -37,6 +37,8 @@ export function IpynbMarkdownCell({ source }: { source: string }): React.JSX.Ele } type IpynbCellSourceProps = { + /** Null for cells without a stable id, whose position-based key can move to another cell. */ + modelKey: string | null cell: IpynbCell source: string active: boolean @@ -140,6 +142,7 @@ function IpynbCodePreview({ } function IpynbSourceEditor({ + modelKey, cell, source, pressedAt, @@ -172,7 +175,16 @@ function IpynbSourceEditor({ return } const { lineHeight, paddingX, paddingY } = CODE_LAYOUT - const model = monaco.editor.createModel(source, cell.language) + // Monaco restores undo for disposed models with this scheme; no other Monaco feature special-cases it. + const uri = + modelKey === null + ? null + : monaco.Uri.from({ scheme: 'vscode-userdata', path: `/${modelKey}` }) + const model = monaco.editor.createModel( + source, + cell.language, + uri && !monaco.editor.getModel(uri) ? uri : undefined + ) const editorInstance = monaco.editor.create(container, { dropIntoEditor: { enabled: false }, model, diff --git a/src/renderer/src/components/editor/IpynbRendering.test.tsx b/src/renderer/src/components/editor/IpynbRendering.test.tsx index 12952e4709f..c7b1ba7aecf 100644 --- a/src/renderer/src/components/editor/IpynbRendering.test.tsx +++ b/src/renderer/src/components/editor/IpynbRendering.test.tsx @@ -24,6 +24,7 @@ function cell(kind: IpynbCell['kind'], source: string, outputs: IpynbOutput[] = function renderSource(target: IpynbCell, onActivate = vi.fn()) { render(