From 6802960643401d1c278dd4d34da196e16fe6c6a2 Mon Sep 17 00:00:00 2001 From: tomas Date: Wed, 18 Mar 2026 19:08:34 +0000 Subject: [PATCH 01/35] fix: Fix deepnote notebook deserializer --- src/notebooks/deepnote/deepnoteSerializer.ts | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index 17443e93b9..69637db668 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -188,7 +188,15 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { * @returns The notebook ID to deserialize, or undefined if none found */ findCurrentNotebookId(projectId: string): string | undefined { - // Prefer the active notebook editor when it matches the project + // Check the manager's stored selection first - this is set explicitly when the user + // picks a notebook from the explorer, and must take priority over the active editor + const storedNotebookId = this.notebookManager.getTheSelectedNotebookForAProject(projectId); + + if (storedNotebookId) { + return storedNotebookId; + } + + // Fallback: prefer the active notebook editor when it matches the project const activeEditorNotebook = window.activeNotebookEditor?.notebook; if ( @@ -199,14 +207,7 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return activeEditorNotebook.metadata.deepnoteNotebookId; } - // Check the manager's stored selection - this should be set when opening from explorer - const storedNotebookId = this.notebookManager.getTheSelectedNotebookForAProject(projectId); - - if (storedNotebookId) { - return storedNotebookId; - } - - // Fallback: Check if there's an active notebook document for this project + // Last fallback: Check if there's an active notebook document for this project const openNotebook = workspace.notebookDocuments.find( (doc) => doc.notebookType === 'deepnote' && doc.metadata?.deepnoteProjectId === projectId ); From 2de7885ff4a08db49042a1582d6c805076e553f8 Mon Sep 17 00:00:00 2001 From: tomas Date: Wed, 18 Mar 2026 19:12:10 +0000 Subject: [PATCH 02/35] Add tests --- .../deepnote/deepnoteSerializer.unit.test.ts | 79 ++++++++++++++++--- 1 file changed, 68 insertions(+), 11 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index c3332f974d..53334ff5c3 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -275,11 +275,9 @@ project: assert.strictEqual(result2, 'notebook-2'); }); - test('should prioritize active notebook editor over stored selection', () => { - // Store a selection for the project + test('should prioritize stored selection over active editor', () => { manager.selectNotebookForProject('project-123', 'stored-notebook'); - // Mock the active notebook editor to return a different notebook const mockActiveNotebook = { notebookType: 'deepnote', metadata: { @@ -294,14 +292,30 @@ project: const result = serializer.findCurrentNotebookId('project-123'); - // Should return the active editor's notebook, not the stored one + assert.strictEqual(result, 'stored-notebook'); + }); + + test('should return active editor notebook when no stored selection exists', () => { + const mockActiveNotebook = { + notebookType: 'deepnote', + metadata: { + deepnoteProjectId: 'project-123', + deepnoteNotebookId: 'active-editor-notebook' + } + }; + + when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ + notebook: mockActiveNotebook + } as any); + + const result = serializer.findCurrentNotebookId('project-123'); + assert.strictEqual(result, 'active-editor-notebook'); }); test('should ignore active editor when project ID does not match', () => { manager.selectNotebookForProject('project-123', 'stored-notebook'); - // Mock active editor with a different project const mockActiveNotebook = { notebookType: 'deepnote', metadata: { @@ -316,14 +330,12 @@ project: const result = serializer.findCurrentNotebookId('project-123'); - // Should fall back to stored selection since active editor is for different project assert.strictEqual(result, 'stored-notebook'); }); test('should ignore active editor when notebook type is not deepnote', () => { manager.selectNotebookForProject('project-123', 'stored-notebook'); - // Mock active editor with non-deepnote notebook type const mockActiveNotebook = { notebookType: 'jupyter-notebook', metadata: { @@ -338,19 +350,16 @@ project: const result = serializer.findCurrentNotebookId('project-123'); - // Should fall back to stored selection since active editor is not a deepnote notebook assert.strictEqual(result, 'stored-notebook'); }); test('should ignore active editor when notebook ID is missing', () => { manager.selectNotebookForProject('project-123', 'stored-notebook'); - // Mock active editor without notebook ID in metadata const mockActiveNotebook = { notebookType: 'deepnote', metadata: { deepnoteProjectId: 'project-123' - // Missing deepnoteNotebookId } }; @@ -360,9 +369,57 @@ project: const result = serializer.findCurrentNotebookId('project-123'); - // Should fall back to stored selection since active editor has no notebook ID assert.strictEqual(result, 'stored-notebook'); }); + + test('switching notebooks: selecting a different notebook while one is open should return the new selection', () => { + manager.selectNotebookForProject('project-123', 'notebook-A'); + + const mockActiveNotebook = { + notebookType: 'deepnote', + metadata: { + deepnoteProjectId: 'project-123', + deepnoteNotebookId: 'notebook-A' + } + }; + + when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ + notebook: mockActiveNotebook + } as any); + + assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-A'); + + manager.selectNotebookForProject('project-123', 'notebook-B'); + + assert.strictEqual( + serializer.findCurrentNotebookId('project-123'), + 'notebook-B', + 'Should return the newly selected notebook, not the one currently in the active editor' + ); + }); + + test('switching notebooks: rapidly switching between three notebooks should always return the latest selection', () => { + const mockActiveNotebook = { + notebookType: 'deepnote', + metadata: { + deepnoteProjectId: 'project-123', + deepnoteNotebookId: 'notebook-1' + } + }; + + when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ + notebook: mockActiveNotebook + } as any); + + manager.selectNotebookForProject('project-123', 'notebook-1'); + assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-1'); + + manager.selectNotebookForProject('project-123', 'notebook-2'); + assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-2'); + + manager.selectNotebookForProject('project-123', 'notebook-3'); + assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-3'); + }); }); suite('component integration', () => { From 3a418c1f04cdb789aef629a50935613aebdbfbc0 Mon Sep 17 00:00:00 2001 From: tomas Date: Thu, 19 Mar 2026 14:00:11 +0000 Subject: [PATCH 03/35] Fix file change watcher --- .../deepnote/deepnoteFileChangeWatcher.ts | 52 ++++++++++++++++--- 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index b7da2080f3..4f0ff53847 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -145,7 +145,7 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic * Consumes a self-write marker. Returns true if the fs event was self-triggered. */ private consumeSelfWrite(uri: Uri): boolean { - const key = uri.toString(); + const key = this.normalizeFileUri(uri); // Check snapshot self-writes first if (this.snapshotSelfWriteUris.has(key)) { @@ -187,6 +187,7 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic if (liveCells.length !== newCells.length) { return true; } + return liveCells.some( (live, i) => live.kind !== newCells[i].kind || @@ -332,6 +333,7 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic } } + // Apply the edit to update in-memory cells immediately (responsive UX). const wsEdit = new WorkspaceEdit(); wsEdit.set(notebook.uri, edits); const applied = await workspace.applyEdit(wsEdit); @@ -341,13 +343,38 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic return; } - // Save to sync mtime — mark as self-write first - this.markSelfWrite(notebook.uri); + // Serialize the notebook and write canonical bytes to disk. This ensures + // the file on disk matches what VS Code's serializer would produce. + // Then save via workspace.save() to clear dirty state and update VS Code's + // internal mtime tracker. Since WE just wrote the file, its mtime is from + // our write (not the external change), avoiding the "content is newer" conflict. + const serializeTokenSource = new CancellationTokenSource(); try { - await workspace.save(notebook.uri); - } catch (error) { - this.consumeSelfWrite(notebook.uri); - throw error; + const serializedBytes = await this.serializer.serializeNotebook(newData, serializeTokenSource.token); + + // Write to disk first — this updates the file mtime to "now" + this.markSelfWrite(fileUri); + try { + await workspace.fs.writeFile(fileUri, serializedBytes); + } catch (writeError) { + this.consumeSelfWrite(fileUri); + logger.warn(`[FileChangeWatcher] Failed to write synced file: ${fileUri.path}`, writeError); + } + + // Now save — VS Code serializes (same bytes), sees the mtime is from our + // recent write (which its internal watcher has picked up), and writes + // successfully without a "content is newer" conflict. + this.markSelfWrite(fileUri); + try { + await workspace.save(notebook.uri); + } catch (saveError) { + this.consumeSelfWrite(fileUri); + logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError); + } + } catch (serializeError) { + logger.warn(`[FileChangeWatcher] Failed to serialize for sync write: ${fileUri.path}`, serializeError); + } finally { + serializeTokenSource.dispose(); } logger.info(`[FileChangeWatcher] Reloaded notebook from external change: ${notebook.uri.path}`); @@ -523,6 +550,15 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic return (metadata?.__deepnoteBlockId ?? metadata?.id) as string | undefined; } + /** + * Normalizes a URI to the underlying file path by stripping query and fragment. + * Notebook URIs include query params (e.g., ?notebook=id) but the filesystem + * watcher fires with the raw file URI — keys must match for self-write detection. + */ + private normalizeFileUri(uri: Uri): string { + return uri.with({ query: '', fragment: '' }).toString(); + } + private handleFileChange(uri: Uri): void { // Deterministic self-write check — no timers involved if (this.consumeSelfWrite(uri)) { @@ -581,7 +617,7 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic * Call before workspace.save() to prevent the resulting fs event from triggering a reload. */ private markSelfWrite(uri: Uri): void { - const key = uri.toString(); + const key = this.normalizeFileUri(uri); const count = this.selfWriteCounts.get(key) ?? 0; this.selfWriteCounts.set(key, count + 1); From 77c0347d76d2dfe5b22eff4c2ea757b0661d706d Mon Sep 17 00:00:00 2001 From: tomas Date: Fri, 20 Mar 2026 07:46:10 +0000 Subject: [PATCH 04/35] Improve error handling in DeepnoteFileChangeWatcher to prevent stale mtime conflicts during workspace saves --- src/notebooks/deepnote/deepnoteFileChangeWatcher.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index 4f0ff53847..9b7f2a31b1 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -359,11 +359,15 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic } catch (writeError) { this.consumeSelfWrite(fileUri); logger.warn(`[FileChangeWatcher] Failed to write synced file: ${fileUri.path}`, writeError); + + // Bail out — without a successful write, workspace.save() would hit + // the stale mtime and show a "content is newer" conflict dialog. + // The notebook stays dirty but cells are already up-to-date from applyEdit. + return; } - // Now save — VS Code serializes (same bytes), sees the mtime is from our - // recent write (which its internal watcher has picked up), and writes - // successfully without a "content is newer" conflict. + // Save to clear dirty state. VS Code serializes (same bytes) and sees the + // mtime from our recent write, so no "content is newer" conflict. this.markSelfWrite(fileUri); try { await workspace.save(notebook.uri); From 6dc77a0ed094954a49e97d38e2aec15d7597066d Mon Sep 17 00:00:00 2001 From: tomas Date: Fri, 20 Mar 2026 08:10:50 +0000 Subject: [PATCH 05/35] Add unit tests for DeepnoteFileChangeWatcher to handle scenarios without block IDs and implement a valid project structure for testing --- .../deepnoteFileChangeWatcher.unit.test.ts | 79 +++++++++++++++++-- 1 file changed, 72 insertions(+), 7 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index c9229820e2..ebcb61e987 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -11,6 +11,22 @@ import { IDeepnoteNotebookManager } from '../types'; import { DeepnoteFileChangeWatcher } from './deepnoteFileChangeWatcher'; import { SnapshotService } from './snapshots/snapshotService'; +const validProject = { + version: '1.0.0', + metadata: { createdAt: '2025-01-01T00:00:00Z' }, + project: { + id: 'project-1', + name: 'Test Project', + notebooks: [ + { + id: 'notebook-1', + name: 'Notebook 1', + blocks: [{ id: 'block-1', type: 'code', sortingKey: 'a0', blockGroup: '1', content: 'print("hello")' }] + } + ] + } +} as DeepnoteProject; + const waitForTimeoutMs = 5000; const waitForIntervalMs = 50; const debounceWaitMs = 800; @@ -37,6 +53,7 @@ async function waitFor( suite('DeepnoteFileChangeWatcher', () => { let watcher: DeepnoteFileChangeWatcher; let mockDisposables: IDisposableRegistry; + let mockedNotebookManager: IDeepnoteNotebookManager; let mockNotebookManager: IDeepnoteNotebookManager; let onDidChangeFile: EventEmitter; let onDidCreateFile: EventEmitter; @@ -51,7 +68,11 @@ suite('DeepnoteFileChangeWatcher', () => { saveCount = 0; mockDisposables = []; - mockNotebookManager = instance(mock()); + + mockedNotebookManager = mock(); + when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(validProject); + when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); + mockNotebookManager = instance(mockedNotebookManager); // Set up FileSystemWatcher mock onDidChangeFile = new EventEmitter(); @@ -126,6 +147,7 @@ suite('DeepnoteFileChangeWatcher', () => { readFileCalls++; return Promise.resolve(new TextEncoder().encode(yamlContent)); }); + when(mockFs.writeFile(anything(), anything())).thenReturn(Promise.resolve()); when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); return mockFs; @@ -325,8 +347,9 @@ project: onDidChangeFile.fire(uri); await waitFor(() => saveCount >= 1); - // The save from the first reload set a self-write marker. - // Simulate the auto-save fs event being consumed (as it would in real VS Code). + // The first reload sets 2 self-write markers (writeFile + save). + // Consume them both with simulated fs events. + onDidChangeFile.fire(uri); onDidChangeFile.fire(uri); // Second real external change: use different YAML content @@ -1053,6 +1076,42 @@ project: }); test('should not apply updates when cells have no block IDs and no fallback', async () => { + const noFallbackDisposables: IDisposableRegistry = []; + const noFallbackOnDidChange = new EventEmitter(); + const noFallbackOnDidCreate = new EventEmitter(); + const fsWatcherNf = mock(); + when(fsWatcherNf.onDidChange).thenReturn(noFallbackOnDidChange.event); + when(fsWatcherNf.onDidCreate).thenReturn(noFallbackOnDidCreate.event); + when(fsWatcherNf.dispose()).thenReturn(); + when(mockedVSCodeNamespaces.workspace.createFileSystemWatcher(anything())).thenReturn( + instance(fsWatcherNf) + ); + + let nfApplyEditCount = 0; + when(mockedVSCodeNamespaces.workspace.applyEdit(anything())).thenCall(() => { + nfApplyEditCount++; + return Promise.resolve(true); + }); + + let nfReadSnapshotCount = 0; + const nfSnapshotService = mock(); + when(nfSnapshotService.isSnapshotsEnabled()).thenReturn(true); + when(nfSnapshotService.readSnapshot(anything())).thenCall(() => { + nfReadSnapshotCount++; + return Promise.resolve(snapshotOutputs); + }); + when(nfSnapshotService.onFileWritten(anything())).thenReturn({ dispose: () => {} } as Disposable); + + const nfManager = mock(); + when(nfManager.getOriginalProject(anything())).thenReturn(undefined); + + const nfWatcher = new DeepnoteFileChangeWatcher( + noFallbackDisposables, + instance(nfManager), + instance(nfSnapshotService) + ); + nfWatcher.activate(); + const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ uri: Uri.file('/workspace/test.deepnote'), @@ -1068,16 +1127,22 @@ project: when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); - snapshotOnDidChange.fire(snapshotUri); + noFallbackOnDidChange.fire(snapshotUri); - await waitFor(() => readSnapshotCallCount >= 1); + await waitFor(() => nfReadSnapshotCount >= 1); - assert.isAtLeast(readSnapshotCallCount, 1, 'readSnapshot should be called'); + assert.isAtLeast(nfReadSnapshotCount, 1, 'readSnapshot should be called'); assert.strictEqual( - snapshotApplyEditCount, + nfApplyEditCount, 0, 'applyEdit should NOT be called when no block IDs can be resolved' ); + + for (const d of noFallbackDisposables) { + d.dispose(); + } + noFallbackOnDidChange.dispose(); + noFallbackOnDidCreate.dispose(); }); test('should fall back to replaceCells when no kernel is active', async () => { From 8898be926586ef9680227c2a21e05bafa26abf78 Mon Sep 17 00:00:00 2001 From: tomas Date: Fri, 20 Mar 2026 08:19:44 +0000 Subject: [PATCH 06/35] Format code --- .../deepnote/deepnoteFileChangeWatcher.unit.test.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index ebcb61e987..6bb58b4d3d 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -1132,11 +1132,7 @@ project: await waitFor(() => nfReadSnapshotCount >= 1); assert.isAtLeast(nfReadSnapshotCount, 1, 'readSnapshot should be called'); - assert.strictEqual( - nfApplyEditCount, - 0, - 'applyEdit should NOT be called when no block IDs can be resolved' - ); + assert.strictEqual(nfApplyEditCount, 0, 'applyEdit should NOT be called when no block IDs can be resolved'); for (const d of noFallbackDisposables) { d.dispose(); From c74ffa7b044b390cfcb1b2846700ebfe27be03fe Mon Sep 17 00:00:00 2001 From: tomas Date: Mon, 23 Mar 2026 09:54:45 +0000 Subject: [PATCH 07/35] Enhance error handling in DeepnoteFileChangeWatcher to check save operation result and log warnings for undefined saves --- src/notebooks/deepnote/deepnoteFileChangeWatcher.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index 9b7f2a31b1..f01b0950a3 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -370,7 +370,12 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic // mtime from our recent write, so no "content is newer" conflict. this.markSelfWrite(fileUri); try { - await workspace.save(notebook.uri); + const saved = await workspace.save(notebook.uri); + if (!saved) { + this.consumeSelfWrite(fileUri); + logger.warn(`[FileChangeWatcher] Save after sync write returned undefined: ${notebook.uri.path}`); + return; + } } catch (saveError) { this.consumeSelfWrite(fileUri); logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError); From 8d061ac84cfb1f8d7dbf93181818385e1b664fac Mon Sep 17 00:00:00 2001 From: tomas Date: Mon, 23 Mar 2026 09:54:50 +0000 Subject: [PATCH 08/35] Add post-snapshot read grace period in unit tests for DeepnoteFileChangeWatcher to ensure proper handling of snapshot reads --- src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index 6bb58b4d3d..72e6713ef4 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -32,6 +32,7 @@ const waitForIntervalMs = 50; const debounceWaitMs = 800; const rapidChangeIntervalMs = 100; const autoSaveGraceMs = 200; +const postSnapshotReadGraceMs = 100; /** * Polls until a condition is met or a timeout is reached. @@ -1130,6 +1131,7 @@ project: noFallbackOnDidChange.fire(snapshotUri); await waitFor(() => nfReadSnapshotCount >= 1); + await new Promise((resolve) => setTimeout(resolve, postSnapshotReadGraceMs)); assert.isAtLeast(nfReadSnapshotCount, 1, 'readSnapshot should be called'); assert.strictEqual(nfApplyEditCount, 0, 'applyEdit should NOT be called when no block IDs can be resolved'); From 5388ead064c41029071cd33c4b45fdbcd3e5a511 Mon Sep 17 00:00:00 2001 From: tomas Date: Thu, 26 Mar 2026 11:14:15 +0000 Subject: [PATCH 09/35] Remove the second markSelfWrite() on the workspace.save() path. --- src/notebooks/deepnote/deepnoteFileChangeWatcher.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index f01b0950a3..531b102f12 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -368,16 +368,13 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic // Save to clear dirty state. VS Code serializes (same bytes) and sees the // mtime from our recent write, so no "content is newer" conflict. - this.markSelfWrite(fileUri); try { const saved = await workspace.save(notebook.uri); if (!saved) { - this.consumeSelfWrite(fileUri); logger.warn(`[FileChangeWatcher] Save after sync write returned undefined: ${notebook.uri.path}`); return; } } catch (saveError) { - this.consumeSelfWrite(fileUri); logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError); } } catch (serializeError) { From 3e7dfc97c25e8e5de357a73f64b1c8dc08bc0203 Mon Sep 17 00:00:00 2001 From: tomas Date: Fri, 27 Mar 2026 07:56:08 +0000 Subject: [PATCH 10/35] feat(deepnote): Add clearNotebookSelection method and update notebook deserialization logic - Introduced `clearNotebookSelection` method in `DeepnoteNotebookManager` to reset notebook selection for a project. - Updated `DeepnoteFileChangeWatcher` to call `clearNotebookSelection` during file change events, ensuring the active editor is prioritized during re-deserialization. - Modified `deserializeNotebook` method in `DeepnoteNotebookSerializer` to accept an optional `notebookId` parameter, preventing race conditions when multiple notebooks from the same project are open. --- .../deepnote/deepnoteFileChangeWatcher.ts | 18 +++++- .../deepnote/deepnoteNotebookManager.ts | 8 +++ .../deepnoteNotebookManager.unit.test.ts | 27 +++++++++ src/notebooks/deepnote/deepnoteSerializer.ts | 14 +++-- .../deepnote/deepnoteSerializer.unit.test.ts | 58 +++++++++++++++++++ src/notebooks/types.ts | 1 + 6 files changed, 120 insertions(+), 6 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index 531b102f12..c31774cd27 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -235,6 +235,17 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic doc.notebookType === 'deepnote' && doc.uri.with({ query: '', fragment: '' }).toString() === uriString ); + // Clear the global notebook selection so that any VS Code-triggered + // re-deserialization (e.g. from workspace.save) falls back to the + // active editor rather than a stale global selection. + for (const notebook of affectedNotebooks) { + const projectId = notebook.metadata?.deepnoteProjectId as string | undefined; + if (projectId) { + this.notebookManager.clearNotebookSelection(projectId); + break; + } + } + for (const notebook of affectedNotebooks) { const nbKey = notebook.uri.toString(); // main-file-sync always replaces any pending operation @@ -276,10 +287,15 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic return; } + // Pass the notebook ID explicitly to avoid mutating the global selection state. + // Multiple notebooks from the same project may be open simultaneously, and + // mutating selectedNotebookByProject would cause race conditions. + const notebookNotebookId = notebook.metadata?.deepnoteNotebookId as string | undefined; + const tokenSource = new CancellationTokenSource(); let newData; try { - newData = await this.serializer.deserializeNotebook(content, tokenSource.token); + newData = await this.serializer.deserializeNotebook(content, tokenSource.token, notebookNotebookId); } catch (error) { logger.warn(`[FileChangeWatcher] Failed to parse changed file: ${fileUri.path}`, error); return; diff --git a/src/notebooks/deepnote/deepnoteNotebookManager.ts b/src/notebooks/deepnote/deepnoteNotebookManager.ts index 069f3a570c..1d80822cc6 100644 --- a/src/notebooks/deepnote/deepnoteNotebookManager.ts +++ b/src/notebooks/deepnote/deepnoteNotebookManager.ts @@ -14,6 +14,14 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { private readonly projectsWithInitNotebookRun = new Set(); private readonly selectedNotebookByProject = new Map(); + /** + * Clears the notebook selection for a project so that subsequent + * deserializations fall back to the active editor or open documents. + */ + clearNotebookSelection(projectId: string): void { + this.selectedNotebookByProject.delete(projectId); + } + /** * Gets the currently selected notebook ID for a project. * @param projectId Project identifier diff --git a/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts b/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts index feb2842848..7c23b7e922 100644 --- a/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts @@ -123,6 +123,33 @@ suite('DeepnoteNotebookManager', () => { }); }); + suite('clearNotebookSelection', () => { + test('should clear selection for a project', () => { + manager.selectNotebookForProject('project-123', 'notebook-456'); + manager.clearNotebookSelection('project-123'); + + const selectedNotebook = manager.getTheSelectedNotebookForAProject('project-123'); + + assert.strictEqual(selectedNotebook, undefined); + }); + + test('should not affect other projects', () => { + manager.selectNotebookForProject('project-1', 'notebook-1'); + manager.selectNotebookForProject('project-2', 'notebook-2'); + manager.clearNotebookSelection('project-1'); + + assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-1'), undefined); + assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-2'), 'notebook-2'); + }); + + test('should be idempotent for unknown project', () => { + assert.doesNotThrow(() => { + manager.clearNotebookSelection('unknown-project'); + manager.clearNotebookSelection('unknown-project'); + }); + }); + }); + suite('storeOriginalProject', () => { test('should store both project and current notebook ID', () => { manager.storeOriginalProject('project-123', mockProject, 'notebook-456'); diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index 69637db668..532b1eb48e 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -67,7 +67,11 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { * @param token Cancellation token (unused) * @returns Promise resolving to notebook data */ - async deserializeNotebook(content: Uint8Array, token: CancellationToken): Promise { + async deserializeNotebook( + content: Uint8Array, + token: CancellationToken, + notebookId?: string + ): Promise { logger.debug('DeepnoteSerializer: Deserializing Deepnote notebook'); if (token?.isCancellationRequested) { @@ -90,16 +94,16 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { } const projectId = deepnoteFile.project.id; - const notebookId = this.findCurrentNotebookId(projectId); + const resolvedNotebookId = notebookId ?? this.findCurrentNotebookId(projectId); - logger.debug(`DeepnoteSerializer: Project ID: ${projectId}, Selected notebook ID: ${notebookId}`); + logger.debug(`DeepnoteSerializer: Project ID: ${projectId}, Selected notebook ID: ${resolvedNotebookId}`); if (deepnoteFile.project.notebooks.length === 0) { throw new Error('Deepnote project contains no notebooks.'); } - const selectedNotebook = notebookId - ? deepnoteFile.project.notebooks.find((nb) => nb.id === notebookId) + const selectedNotebook = resolvedNotebookId + ? deepnoteFile.project.notebooks.find((nb) => nb.id === resolvedNotebookId) : this.findDefaultNotebook(deepnoteFile); if (!selectedNotebook) { diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index 53334ff5c3..0e27d79283 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -147,6 +147,34 @@ project: /no notebooks|notebooks.*must contain at least 1/i ); }); + + test('should deserialize the specified notebook when notebookId is passed', async () => { + const content = projectToYaml(mockProject); + const result = await serializer.deserializeNotebook(content, {} as any, 'notebook-2'); + + assert.strictEqual(result.metadata?.deepnoteNotebookId, 'notebook-2'); + assert.strictEqual(result.metadata?.deepnoteNotebookName, 'Second Notebook'); + assert.strictEqual(result.cells.length, 1); + assert.include(result.cells[0].value, 'Title'); + }); + + test('should ignore stored selection when explicit notebookId is provided', async () => { + manager.selectNotebookForProject('project-123', 'notebook-1'); + const content = projectToYaml(mockProject); + const result = await serializer.deserializeNotebook(content, {} as any, 'notebook-2'); + + assert.strictEqual(result.metadata?.deepnoteNotebookId, 'notebook-2'); + assert.strictEqual(result.metadata?.deepnoteNotebookName, 'Second Notebook'); + }); + + test('should fall back to findCurrentNotebookId when notebookId is undefined', async () => { + manager.selectNotebookForProject('project-123', 'notebook-1'); + const content = projectToYaml(mockProject); + const result = await serializer.deserializeNotebook(content, {} as any); + + assert.strictEqual(result.metadata?.deepnoteNotebookId, 'notebook-1'); + assert.strictEqual(result.metadata?.deepnoteNotebookName, 'First Notebook'); + }); }); suite('serializeNotebook', () => { @@ -420,6 +448,36 @@ project: manager.selectNotebookForProject('project-123', 'notebook-3'); assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-3'); }); + + test('should return undefined when selection is cleared and no active editor', () => { + manager.selectNotebookForProject('project-123', 'notebook-456'); + manager.clearNotebookSelection('project-123'); + + const result = serializer.findCurrentNotebookId('project-123'); + + assert.strictEqual(result, undefined); + }); + + test('should fall back to active editor after selection is cleared', () => { + manager.selectNotebookForProject('project-123', 'stored-wrong'); + manager.clearNotebookSelection('project-123'); + + const mockActiveNotebook = { + notebookType: 'deepnote', + metadata: { + deepnoteProjectId: 'project-123', + deepnoteNotebookId: 'active-editor-notebook' + } + }; + + when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ + notebook: mockActiveNotebook + } as any); + + const result = serializer.findCurrentNotebookId('project-123'); + + assert.strictEqual(result, 'active-editor-notebook'); + }); }); suite('component integration', () => { diff --git a/src/notebooks/types.ts b/src/notebooks/types.ts index a12dacf52a..fd2fd83c44 100644 --- a/src/notebooks/types.ts +++ b/src/notebooks/types.ts @@ -37,6 +37,7 @@ export interface ProjectIntegration { export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); export interface IDeepnoteNotebookManager { + clearNotebookSelection(projectId: string): void; getCurrentNotebookId(projectId: string): string | undefined; getOriginalProject(projectId: string): DeepnoteProject | undefined; getTheSelectedNotebookForAProject(projectId: string): string | undefined; From c160ae3af655e8d093541ed7b4df11af1241aeeb Mon Sep 17 00:00:00 2001 From: tomas Date: Fri, 27 Mar 2026 07:56:15 +0000 Subject: [PATCH 11/35] Update test --- .../deepnoteFileChangeWatcher.unit.test.ts | 262 +++++++++++++++++- 1 file changed, 257 insertions(+), 5 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index 72e6713ef4..98c00e0efb 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -1,7 +1,17 @@ +import { DeepnoteFile, serializeDeepnoteFile } from '@deepnote/blocks'; import { assert } from 'chai'; import * as sinon from 'sinon'; -import { anything, instance, mock, when } from 'ts-mockito'; -import { Disposable, EventEmitter, FileSystemWatcher, NotebookCellKind, NotebookDocument, Uri } from 'vscode'; +import { anything, instance, mock, reset, resetCalls, verify, when } from 'ts-mockito'; +import { + Disposable, + EventEmitter, + FileSystemWatcher, + NotebookCellKind, + NotebookDocument, + NotebookEdit, + Uri, + WorkspaceEdit +} from 'vscode'; import type { IControllerRegistration } from '../controllers/types'; import type { IDisposableRegistry } from '../../platform/common/types'; @@ -11,7 +21,7 @@ import { IDeepnoteNotebookManager } from '../types'; import { DeepnoteFileChangeWatcher } from './deepnoteFileChangeWatcher'; import { SnapshotService } from './snapshots/snapshotService'; -const validProject = { +const validProject: DeepnoteFile = { version: '1.0.0', metadata: { createdAt: '2025-01-01T00:00:00Z' }, project: { @@ -21,11 +31,61 @@ const validProject = { { id: 'notebook-1', name: 'Notebook 1', - blocks: [{ id: 'block-1', type: 'code', sortingKey: 'a0', blockGroup: '1', content: 'print("hello")' }] + blocks: [ + { + id: 'block-1', + type: 'code', + sortingKey: 'a0', + blockGroup: '1', + content: 'print("hello")', + metadata: {} + } + ] + } + ] + } +}; + +const multiNotebookProject: DeepnoteFile = { + version: '1.0.0', + metadata: { createdAt: '2025-01-01T00:00:00Z' }, + project: { + id: 'project-1', + name: 'Multi Notebook Project', + notebooks: [ + { + id: 'notebook-1', + name: 'Notebook 1', + blocks: [ + { + id: 'block-nb1', + type: 'code', + sortingKey: 'a0', + blockGroup: '1', + content: 'print("nb1-new")', + metadata: {} + } + ] + }, + { + id: 'notebook-2', + name: 'Notebook 2', + blocks: [ + { + id: 'block-nb2', + type: 'code', + sortingKey: 'a0', + blockGroup: '1', + content: 'print("nb2-new")', + metadata: {} + } + ] } ] } -} as DeepnoteProject; +}; + +const multiNotebookYaml = serializeDeepnoteFile(multiNotebookProject); const waitForTimeoutMs = 5000; const waitForIntervalMs = 50; @@ -73,6 +133,7 @@ suite('DeepnoteFileChangeWatcher', () => { mockedNotebookManager = mock(); when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(validProject); when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); + when(mockedNotebookManager.clearNotebookSelection(anything())).thenReturn(); mockNotebookManager = instance(mockedNotebookManager); // Set up FileSystemWatcher mock @@ -1200,4 +1261,195 @@ project: fbOnDidCreate.dispose(); }); }); + + suite('multi-notebook file sync', () => { + let workspaceSetCaptures: { uriKey: string; cellSourceJoined: string }[] = []; + let workspaceEditSetStub: sinon.SinonStub | undefined; + + setup(() => { + reset(mockedNotebookManager); + when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(multiNotebookProject); + when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); + when(mockedNotebookManager.clearNotebookSelection(anything())).thenReturn(); + resetCalls(mockedNotebookManager); + workspaceSetCaptures = []; + workspaceEditSetStub = sinon.stub(WorkspaceEdit.prototype, 'set').callsFake((uri: Uri, edits: unknown) => { + if (!Array.isArray(edits) || edits.length === 0) { + return; + } + + const firstEdit = edits[0] as NotebookEdit; + if (firstEdit?.newCells && firstEdit.newCells.length > 0) { + workspaceSetCaptures.push({ + uriKey: uri.toString(), + cellSourceJoined: firstEdit.newCells.map((c) => c.value).join('\n') + }); + } + }); + }); + + teardown(() => { + workspaceEditSetStub?.restore(); + workspaceEditSetStub = undefined; + }); + + test('should reload each notebook with its own content when multiple notebooks are open', async () => { + const basePath = Uri.file('/workspace/multi.deepnote'); + const uriNb1 = basePath.with({ query: 'view=1' }); + const uriNb2 = basePath.with({ query: 'view=2' }); + + const notebook1 = createMockNotebook({ + uri: uriNb1, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-1' + }, + cells: [ + { + metadata: { id: 'block-nb1', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb1-old")', languageId: 'python' } + } + ] + }); + + const notebook2 = createMockNotebook({ + uri: uriNb2, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-2' + }, + cells: [ + { + metadata: { id: 'block-nb2', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb2-old")', languageId: 'python' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook1, notebook2]); + setupMockFs(multiNotebookYaml); + + onDidChangeFile.fire(basePath); + + await waitFor(() => applyEditCount >= 2); + + assert.strictEqual(applyEditCount, 2, 'applyEdit should run once per open notebook'); + assert.strictEqual(workspaceSetCaptures.length, 2, 'each notebook should get a replaceCells edit'); + + const byUri = new Map(workspaceSetCaptures.map((c) => [c.uriKey, c.cellSourceJoined])); + + assert.include(byUri.get(uriNb1.toString()) ?? '', 'nb1-new'); + assert.notInclude(byUri.get(uriNb1.toString()) ?? '', 'nb2-new'); + assert.include(byUri.get(uriNb2.toString()) ?? '', 'nb2-new'); + assert.notInclude(byUri.get(uriNb2.toString()) ?? '', 'nb1-new'); + }); + + test('should clear notebook selection before processing file change', async () => { + const basePath = Uri.file('/workspace/multi.deepnote'); + const uriNb1 = basePath.with({ query: 'a=1' }); + const uriNb2 = basePath.with({ query: 'b=2' }); + + const notebook1 = createMockNotebook({ + uri: uriNb1, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-1' + }, + cells: [ + { + metadata: { id: 'block-nb1' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb1-old")' } + } + ] + }); + + const notebook2 = createMockNotebook({ + uri: uriNb2, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-2' + }, + cells: [ + { + metadata: { id: 'block-nb2' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb2-old")' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook1, notebook2]); + setupMockFs(multiNotebookYaml); + + onDidChangeFile.fire(basePath); + + await waitFor(() => applyEditCount >= 2); + + verify(mockedNotebookManager.clearNotebookSelection('project-1')).once(); + }); + + test('should not corrupt other notebooks when one notebook triggers a file change', async () => { + const basePath = Uri.file('/workspace/multi.deepnote'); + const uriNb1 = basePath.with({ query: 'n=1' }); + const uriNb2 = basePath.with({ query: 'n=2' }); + + const notebook1 = createMockNotebook({ + uri: uriNb1, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-1' + }, + cells: [ + { + metadata: { id: 'block-nb1' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb1-old")' } + } + ] + }); + + const notebook2 = createMockNotebook({ + uri: uriNb2, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-2' + }, + cells: [ + { + metadata: { id: 'block-nb2' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb2-old")' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook1, notebook2]); + setupMockFs(multiNotebookYaml); + + onDidChangeFile.fire(basePath); + + await waitFor(() => applyEditCount >= 2); + + const nb1Cells = workspaceSetCaptures.find((c) => c.uriKey === uriNb1.toString())?.cellSourceJoined; + const nb2Cells = workspaceSetCaptures.find((c) => c.uriKey === uriNb2.toString())?.cellSourceJoined; + + assert.isDefined(nb1Cells); + assert.isDefined(nb2Cells); + assert.notStrictEqual(nb1Cells, nb2Cells, 'each open notebook must receive distinct deserialized content'); + + assert.include(nb1Cells!, 'nb1-new'); + assert.include(nb2Cells!, 'nb2-new'); + assert.notInclude(nb1Cells!, 'nb2-new', 'notebook-1 must not receive notebook-2 block content'); + assert.notInclude(nb2Cells!, 'nb1-new', 'notebook-2 must not receive notebook-1 block content'); + }); + }); }); From 0b2504a6e1b53c5281658405c137b9dda70698a7 Mon Sep 17 00:00:00 2001 From: tomas Date: Fri, 27 Mar 2026 09:52:27 +0000 Subject: [PATCH 12/35] refactor(deepnote): Improve mock child process and server output management - Enhanced `createMockChildProcess` to provide a more comprehensive mock implementation for testing. - Updated `DeepnoteServerStarter` to ensure proper disposal of existing disposables when monitoring server output, improving resource management. - Adjusted error handling in server startup to streamline diagnostics and output tracking. --- .../deepnote/deepnoteServerStarter.node.ts | 8 ++- .../deepnote/deepnoteTestHelpers.node.ts | 59 ++++++++++++++- .../deepnoteFileChangeWatcher.unit.test.ts | 34 ++++----- src/test/mocks/vsc/extHostedTypes.ts | 71 ++++++++++++++++--- 4 files changed, 143 insertions(+), 29 deletions(-) diff --git a/src/kernels/deepnote/deepnoteServerStarter.node.ts b/src/kernels/deepnote/deepnoteServerStarter.node.ts index 112f7f0e09..6b4219cdd5 100644 --- a/src/kernels/deepnote/deepnoteServerStarter.node.ts +++ b/src/kernels/deepnote/deepnoteServerStarter.node.ts @@ -280,7 +280,7 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension throw new DeepnoteServerStartupError( interpreter.uri.fsPath, - serverInfo?.jupyterPort ?? 0, + 0, 'unknown', capturedOutput?.stdout || '', capturedOutput?.stderr || '', @@ -402,6 +402,12 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension */ private monitorServerOutput(fileKey: string, serverInfo: DeepnoteServerInfo): void { const proc = serverInfo.process; + const existing = this.disposablesByFile.get(fileKey); + if (existing) { + for (const d of existing) { + d.dispose(); + } + } const disposables: IDisposable[] = []; this.disposablesByFile.set(fileKey, disposables); diff --git a/src/kernels/deepnote/deepnoteTestHelpers.node.ts b/src/kernels/deepnote/deepnoteTestHelpers.node.ts index 524c6e0e47..42d1ae19ec 100644 --- a/src/kernels/deepnote/deepnoteTestHelpers.node.ts +++ b/src/kernels/deepnote/deepnoteTestHelpers.node.ts @@ -5,11 +5,66 @@ import type { ChildProcess } from 'node:child_process'; * Satisfies the ChildProcess interface with minimal stub values. */ export function createMockChildProcess(overrides?: Partial): ChildProcess { - return { + const mockProcess: ChildProcess = { pid: undefined, + stdio: [null, null, null, null, null], + stdin: null, stdout: null, stderr: null, exitCode: null, + killed: false, + connected: false, + signalCode: null, + spawnargs: [], + spawnfile: '', + kill: () => true, + send: () => true, + disconnect: () => true, + unref: () => true, + ref: () => true, + addListener: function () { + return this; + }, + emit: () => true, + on: function () { + return this; + }, + once: function () { + return this; + }, + removeListener: function () { + return this; + }, + removeAllListeners: function () { + return this; + }, + prependListener: function () { + return this; + }, + prependOnceListener: function () { + return this; + }, + [Symbol.dispose]: () => { + return undefined; + }, + off: function () { + return this; + }, + setMaxListeners: function () { + return this; + }, + getMaxListeners: () => 10, + listeners: function () { + return []; + }, + rawListeners: function () { + return []; + }, + eventNames: function () { + return []; + }, + listenerCount: () => 0, ...overrides - } as ChildProcess; + }; + return mockProcess; } diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index 98c00e0efb..bb71908937 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -1264,7 +1264,6 @@ project: suite('multi-notebook file sync', () => { let workspaceSetCaptures: { uriKey: string; cellSourceJoined: string }[] = []; - let workspaceEditSetStub: sinon.SinonStub | undefined; setup(() => { reset(mockedNotebookManager); @@ -1273,24 +1272,27 @@ project: when(mockedNotebookManager.clearNotebookSelection(anything())).thenReturn(); resetCalls(mockedNotebookManager); workspaceSetCaptures = []; - workspaceEditSetStub = sinon.stub(WorkspaceEdit.prototype, 'set').callsFake((uri: Uri, edits: unknown) => { - if (!Array.isArray(edits) || edits.length === 0) { - return; - } + when(mockedVSCodeNamespaces.workspace.applyEdit(anything())).thenCall((wsEdit: WorkspaceEdit) => { + applyEditCount++; + + const ext = wsEdit as WorkspaceEdit & { notebookEntries(): [Uri, NotebookEdit[]][] }; - const firstEdit = edits[0] as NotebookEdit; - if (firstEdit?.newCells && firstEdit.newCells.length > 0) { - workspaceSetCaptures.push({ - uriKey: uri.toString(), - cellSourceJoined: firstEdit.newCells.map((c) => c.value).join('\n') - }); + for (const [uri, edits] of ext.notebookEntries()) { + if (!edits.length) { + continue; + } + + const firstEdit = edits[0]; + if (firstEdit?.newCells && firstEdit.newCells.length > 0) { + workspaceSetCaptures.push({ + uriKey: uri.toString(), + cellSourceJoined: firstEdit.newCells.map((c) => c.value).join('\n') + }); + } } - }); - }); - teardown(() => { - workspaceEditSetStub?.restore(); - workspaceEditSetStub = undefined; + return Promise.resolve(true); + }); }); test('should reload each notebook with its own content when multiple notebooks are open', async () => { diff --git a/src/test/mocks/vsc/extHostedTypes.ts b/src/test/mocks/vsc/extHostedTypes.ts index 4a58f1c077..ed93be5c2c 100644 --- a/src/test/mocks/vsc/extHostedTypes.ts +++ b/src/test/mocks/vsc/extHostedTypes.ts @@ -707,6 +707,7 @@ export namespace vscMockExtHostedTypes { private _seqPool: number = 0; private _resourceEdits: { seq: number; from: vscUri.URI; to: vscUri.URI }[] = []; + private _notebookEdits = new Map(); private _textEdits = new Map(); // createResource(uri: vscode.Uri): void { @@ -759,7 +760,20 @@ export namespace vscMockExtHostedTypes { } has(uri: vscUri.URI): boolean { - return this._textEdits.has(uri.toString()); + return this._textEdits.has(uri.toString()) || this._notebookEdits.has(uri.toString()); + } + + /** + * Notebook edits passed to {@link set} (for tests that inspect edits via workspace.applyEdit). + */ + notebookEntries(): [vscUri.URI, NotebookEdit[]][] { + const res: [vscUri.URI, NotebookEdit[]][] = []; + + this._notebookEdits.forEach((value) => { + res.push([value.uri, value.edits.slice()]); + }); + + return res.slice(); } set(uri: vscode.Uri, edits: NotebookEdit[]): void; @@ -779,17 +793,54 @@ export namespace vscMockExtHostedTypes { | [TextEdit | SnippetTextEdit, vscode.WorkspaceEditEntryMetadata] )[] ): void { - let data = this._textEdits.get(uri.toString()); + const uriKey = uri.toString(); + + if (!edits) { + const data = this._textEdits.get(uriKey); + if (data) { + // @ts-ignore + data.edits = undefined; + } + this._notebookEdits.delete(uriKey); + + return; + } + + if (edits.length === 0) { + let data = this._textEdits.get(uriKey); + if (!data) { + data = { seq: this._seqPool++, uri, edits: [] }; + this._textEdits.set(uriKey, data); + } + data.edits = []; + + return; + } + + const first = edits[0]; + const firstEdit = Array.isArray(first) ? first[0] : first; + + if (firstEdit instanceof NotebookEdit) { + const normalized: NotebookEdit[] = edits.map((item) => { + if (Array.isArray(item)) { + return item[0] as NotebookEdit; + } + + return item as NotebookEdit; + }); + + this._notebookEdits.set(uriKey, { uri, edits: normalized }); + + return; + } + + let data = this._textEdits.get(uriKey); if (!data) { data = { seq: this._seqPool++, uri, edits: [] }; - this._textEdits.set(uri.toString(), data); - } - if (!edits) { - // @ts-ignore - data.edits = undefined; - } else { - //data.edits = edits.slice(0); + this._textEdits.set(uriKey, data); } + + data.edits = edits.slice(0) as TextEdit[]; } get(uri: vscUri.URI): TextEdit[] { @@ -825,7 +876,7 @@ export namespace vscMockExtHostedTypes { } get size(): number { - return this._textEdits.size + this._resourceEdits.length; + return this._textEdits.size + this._notebookEdits.size + this._resourceEdits.length; } toJSON(): any { From 9d5c6c055d3a58717f4925691fdd3ea62e92b101 Mon Sep 17 00:00:00 2001 From: tomas Date: Fri, 27 Mar 2026 17:30:03 +0000 Subject: [PATCH 13/35] refactor(deepnote): Simplify notebook edit application logic - Introduced a new `applyNotebookEdits` method in `DeepnoteFileChangeWatcher` to centralize the application of notebook edits, improving code readability and maintainability. - Updated existing calls to `workspace.applyEdit` to utilize the new method, reducing redundancy in the codebase. - Adjusted unit tests to reflect changes in the edit application process, ensuring consistent behavior across the application. --- .../deepnote/deepnoteFileChangeWatcher.ts | 23 +++--- .../deepnoteFileChangeWatcher.unit.test.ts | 37 +++++----- src/test/mocks/vsc/extHostedTypes.ts | 71 +++---------------- 3 files changed, 39 insertions(+), 92 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index c31774cd27..3b0bb7a57c 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -122,6 +122,13 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic } } + protected async applyNotebookEdits(uri: Uri, edits: NotebookEdit[]): Promise { + const wsEdit = new WorkspaceEdit(); + wsEdit.set(uri, edits); + + return workspace.applyEdit(wsEdit); + } + private clearAllTimers(): void { for (const timer of this.debounceTimers.values()) { clearTimeout(timer); @@ -350,9 +357,7 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic } // Apply the edit to update in-memory cells immediately (responsive UX). - const wsEdit = new WorkspaceEdit(); - wsEdit.set(notebook.uri, edits); - const applied = await workspace.applyEdit(wsEdit); + const applied = await this.applyNotebookEdits(notebook.uri, edits); if (!applied) { logger.warn(`[FileChangeWatcher] Failed to apply edit: ${notebook.uri.path}`); @@ -501,9 +506,7 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic } } if (metadataEdits.length > 0) { - const wsEdit = new WorkspaceEdit(); - wsEdit.set(notebook.uri, metadataEdits); - await workspace.applyEdit(wsEdit); + await this.applyNotebookEdits(notebook.uri, metadataEdits); } logger.info(`[FileChangeWatcher] Updated notebook outputs via execution API: ${notebook.uri.path}`); @@ -531,9 +534,7 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic ); } - const wsEdit = new WorkspaceEdit(); - wsEdit.set(notebook.uri, replaceEdits); - const applied = await workspace.applyEdit(wsEdit); + const applied = await this.applyNotebookEdits(notebook.uri, replaceEdits); if (!applied) { logger.warn(`[FileChangeWatcher] Failed to apply snapshot outputs: ${notebook.uri.path}`); @@ -552,9 +553,7 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic }) ); } - const metaEdit = new WorkspaceEdit(); - metaEdit.set(notebook.uri, metadataEdits); - await workspace.applyEdit(metaEdit); + await this.applyNotebookEdits(notebook.uri, metadataEdits); // Save to sync mtime — mark as self-write first this.markSelfWrite(notebook.uri); diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index bb71908937..2b1eae7496 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -9,8 +9,7 @@ import { NotebookCellKind, NotebookDocument, NotebookEdit, - Uri, - WorkspaceEdit + Uri } from 'vscode'; import type { IControllerRegistration } from '../controllers/types'; @@ -94,6 +93,11 @@ const rapidChangeIntervalMs = 100; const autoSaveGraceMs = 200; const postSnapshotReadGraceMs = 100; +interface NotebookEditCapture { + uriKey: string; + cellSourceJoined: string; +} + /** * Polls until a condition is met or a timeout is reached. */ @@ -1263,7 +1267,7 @@ project: }); suite('multi-notebook file sync', () => { - let workspaceSetCaptures: { uriKey: string; cellSourceJoined: string }[] = []; + let workspaceSetCaptures: NotebookEditCapture[] = []; setup(() => { reset(mockedNotebookManager); @@ -1272,26 +1276,21 @@ project: when(mockedNotebookManager.clearNotebookSelection(anything())).thenReturn(); resetCalls(mockedNotebookManager); workspaceSetCaptures = []; - when(mockedVSCodeNamespaces.workspace.applyEdit(anything())).thenCall((wsEdit: WorkspaceEdit) => { - applyEditCount++; - - const ext = wsEdit as WorkspaceEdit & { notebookEntries(): [Uri, NotebookEdit[]][] }; + sinon.stub(watcher, 'applyNotebookEdits' as any).callsFake(async (...args: unknown[]) => { + const uri = args[0] as Uri; + const edits = args[1] as NotebookEdit[]; - for (const [uri, edits] of ext.notebookEntries()) { - if (!edits.length) { - continue; - } + applyEditCount++; - const firstEdit = edits[0]; - if (firstEdit?.newCells && firstEdit.newCells.length > 0) { - workspaceSetCaptures.push({ - uriKey: uri.toString(), - cellSourceJoined: firstEdit.newCells.map((c) => c.value).join('\n') - }); - } + const replaceCellsEdit = edits.find((e) => e.newCells?.length > 0); + if (replaceCellsEdit) { + workspaceSetCaptures.push({ + uriKey: uri.toString(), + cellSourceJoined: replaceCellsEdit.newCells.map((c: any) => c.value).join('\n') + }); } - return Promise.resolve(true); + return true; }); }); diff --git a/src/test/mocks/vsc/extHostedTypes.ts b/src/test/mocks/vsc/extHostedTypes.ts index ed93be5c2c..4a58f1c077 100644 --- a/src/test/mocks/vsc/extHostedTypes.ts +++ b/src/test/mocks/vsc/extHostedTypes.ts @@ -707,7 +707,6 @@ export namespace vscMockExtHostedTypes { private _seqPool: number = 0; private _resourceEdits: { seq: number; from: vscUri.URI; to: vscUri.URI }[] = []; - private _notebookEdits = new Map(); private _textEdits = new Map(); // createResource(uri: vscode.Uri): void { @@ -760,20 +759,7 @@ export namespace vscMockExtHostedTypes { } has(uri: vscUri.URI): boolean { - return this._textEdits.has(uri.toString()) || this._notebookEdits.has(uri.toString()); - } - - /** - * Notebook edits passed to {@link set} (for tests that inspect edits via workspace.applyEdit). - */ - notebookEntries(): [vscUri.URI, NotebookEdit[]][] { - const res: [vscUri.URI, NotebookEdit[]][] = []; - - this._notebookEdits.forEach((value) => { - res.push([value.uri, value.edits.slice()]); - }); - - return res.slice(); + return this._textEdits.has(uri.toString()); } set(uri: vscode.Uri, edits: NotebookEdit[]): void; @@ -793,54 +779,17 @@ export namespace vscMockExtHostedTypes { | [TextEdit | SnippetTextEdit, vscode.WorkspaceEditEntryMetadata] )[] ): void { - const uriKey = uri.toString(); - - if (!edits) { - const data = this._textEdits.get(uriKey); - if (data) { - // @ts-ignore - data.edits = undefined; - } - this._notebookEdits.delete(uriKey); - - return; - } - - if (edits.length === 0) { - let data = this._textEdits.get(uriKey); - if (!data) { - data = { seq: this._seqPool++, uri, edits: [] }; - this._textEdits.set(uriKey, data); - } - data.edits = []; - - return; - } - - const first = edits[0]; - const firstEdit = Array.isArray(first) ? first[0] : first; - - if (firstEdit instanceof NotebookEdit) { - const normalized: NotebookEdit[] = edits.map((item) => { - if (Array.isArray(item)) { - return item[0] as NotebookEdit; - } - - return item as NotebookEdit; - }); - - this._notebookEdits.set(uriKey, { uri, edits: normalized }); - - return; - } - - let data = this._textEdits.get(uriKey); + let data = this._textEdits.get(uri.toString()); if (!data) { data = { seq: this._seqPool++, uri, edits: [] }; - this._textEdits.set(uriKey, data); + this._textEdits.set(uri.toString(), data); + } + if (!edits) { + // @ts-ignore + data.edits = undefined; + } else { + //data.edits = edits.slice(0); } - - data.edits = edits.slice(0) as TextEdit[]; } get(uri: vscUri.URI): TextEdit[] { @@ -876,7 +825,7 @@ export namespace vscMockExtHostedTypes { } get size(): number { - return this._textEdits.size + this._notebookEdits.size + this._resourceEdits.length; + return this._textEdits.size + this._resourceEdits.length; } toJSON(): any { From 44bd482eb25df6a2f90bb7414a9fd79336cc1a54 Mon Sep 17 00:00:00 2001 From: tomas Date: Mon, 30 Mar 2026 15:35:21 +0000 Subject: [PATCH 14/35] feat(deepnote): Enhance notebook resolution management and error handling - Introduced `queueNotebookResolution` and `consumePendingNotebookResolution` methods in `DeepnoteNotebookManager` to manage transient notebook resolutions, improving the handling of notebook selections during file operations. - Updated `DeepnoteNotebookSerializer` to prioritize queued notebook resolutions, ensuring more reliable notebook ID retrieval during deserialization. - Refactored `DeepnoteExplorerView` to utilize a new `registerNotebookOpenIntent` method, streamlining the process of selecting and opening notebooks. - Improved error handling in `DeepnoteServerStarter` to log warnings when disposing listeners fails, enhancing diagnostics during server operations. - Adjusted unit tests to cover new functionality and ensure consistent behavior across notebook management processes. --- .../deepnote/deepnoteServerStarter.node.ts | 6 +- .../deepnote/deepnoteExplorerView.ts | 13 +- .../deepnote/deepnoteFileChangeWatcher.ts | 11 - .../deepnoteFileChangeWatcher.unit.test.ts | 12 +- .../deepnote/deepnoteNotebookManager.ts | 65 +++- .../deepnoteNotebookManager.unit.test.ts | 40 +++ src/notebooks/deepnote/deepnoteSerializer.ts | 61 ++-- .../deepnote/deepnoteSerializer.unit.test.ts | 330 +++++++++--------- src/notebooks/types.ts | 7 +- 9 files changed, 324 insertions(+), 221 deletions(-) diff --git a/src/kernels/deepnote/deepnoteServerStarter.node.ts b/src/kernels/deepnote/deepnoteServerStarter.node.ts index 6b4219cdd5..0ea58022cd 100644 --- a/src/kernels/deepnote/deepnoteServerStarter.node.ts +++ b/src/kernels/deepnote/deepnoteServerStarter.node.ts @@ -405,7 +405,11 @@ export class DeepnoteServerStarter implements IDeepnoteServerStarter, IExtension const existing = this.disposablesByFile.get(fileKey); if (existing) { for (const d of existing) { - d.dispose(); + try { + d.dispose(); + } catch (ex) { + logger.warn(`Error disposing listener for ${fileKey}`, ex); + } } } const disposables: IDisposable[] = []; diff --git a/src/notebooks/deepnote/deepnoteExplorerView.ts b/src/notebooks/deepnote/deepnoteExplorerView.ts index 33599da2f0..6d450401ba 100644 --- a/src/notebooks/deepnote/deepnoteExplorerView.ts +++ b/src/notebooks/deepnote/deepnoteExplorerView.ts @@ -260,7 +260,7 @@ export class DeepnoteExplorerView { await this.treeDataProvider.refreshNotebook(treeItem.context.projectId); // Optionally open the duplicated notebook - this.manager.selectNotebookForProject(treeItem.context.projectId, newNotebook.id); + this.registerNotebookOpenIntent(treeItem.context.projectId, newNotebook.id); const notebookUri = fileUri.with({ query: `notebook=${newNotebook.id}` }); const document = await workspace.openNotebookDocument(notebookUri); await window.showNotebookDocument(document, { @@ -508,7 +508,7 @@ export class DeepnoteExplorerView { await this.treeDataProvider.refreshNotebook(projectData.project.id); // Open the new notebook - this.manager.selectNotebookForProject(projectData.project.id, notebookId); + this.registerNotebookOpenIntent(projectData.project.id, notebookId); const notebookUri = fileUri.with({ query: `notebook=${notebookId}` }); const document = await workspace.openNotebookDocument(notebookUri); await window.showNotebookDocument(document, { @@ -517,6 +517,11 @@ export class DeepnoteExplorerView { }); } + private registerNotebookOpenIntent(projectId: string, notebookId: string): void { + this.manager.queueNotebookResolution(projectId, notebookId); + this.manager.selectNotebookForProject(projectId, notebookId); + } + private refreshExplorer(): void { this.treeDataProvider.refresh(); } @@ -537,7 +542,7 @@ export class DeepnoteExplorerView { console.log(`Selecting notebook in manager.`); - this.manager.selectNotebookForProject(context.projectId, context.notebookId); + this.registerNotebookOpenIntent(context.projectId, context.notebookId); console.log(`Opening notebook document.`, fileUri); @@ -701,7 +706,7 @@ export class DeepnoteExplorerView { this.treeDataProvider.refresh(); - this.manager.selectNotebookForProject(projectId, notebookId); + this.registerNotebookOpenIntent(projectId, notebookId); const notebookUri = fileUri.with({ query: `notebook=${notebookId}` }); const document = await workspace.openNotebookDocument(notebookUri); diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index 3b0bb7a57c..caa2d01fb7 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -242,17 +242,6 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic doc.notebookType === 'deepnote' && doc.uri.with({ query: '', fragment: '' }).toString() === uriString ); - // Clear the global notebook selection so that any VS Code-triggered - // re-deserialization (e.g. from workspace.save) falls back to the - // active editor rather than a stale global selection. - for (const notebook of affectedNotebooks) { - const projectId = notebook.metadata?.deepnoteProjectId as string | undefined; - if (projectId) { - this.notebookManager.clearNotebookSelection(projectId); - break; - } - } - for (const notebook of affectedNotebooks) { const nbKey = notebook.uri.toString(); // main-file-sync always replaces any pending operation diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index 2b1eae7496..5090c26d57 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -135,9 +135,10 @@ suite('DeepnoteFileChangeWatcher', () => { mockDisposables = []; mockedNotebookManager = mock(); + when(mockedNotebookManager.consumePendingNotebookResolution(anything())).thenReturn(undefined); when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(validProject); when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); - when(mockedNotebookManager.clearNotebookSelection(anything())).thenReturn(); + when(mockedNotebookManager.queueNotebookResolution(anything(), anything())).thenReturn(); mockNotebookManager = instance(mockedNotebookManager); // Set up FileSystemWatcher mock @@ -452,7 +453,7 @@ project: onDidChangeFile.fire(uri); - await waitFor(() => saveCount > 0); + await waitFor(() => applyEditCount > 0); // Only ONE applyEdit call (atomic: replaceCells + metadata in single WorkspaceEdit) assert.strictEqual(applyEditCount, 1, 'applyEdit should be called exactly once (atomic edit)'); @@ -1271,9 +1272,10 @@ project: setup(() => { reset(mockedNotebookManager); + when(mockedNotebookManager.consumePendingNotebookResolution(anything())).thenReturn(undefined); when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(multiNotebookProject); when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); - when(mockedNotebookManager.clearNotebookSelection(anything())).thenReturn(); + when(mockedNotebookManager.queueNotebookResolution(anything(), anything())).thenReturn(); resetCalls(mockedNotebookManager); workspaceSetCaptures = []; sinon.stub(watcher, 'applyNotebookEdits' as any).callsFake(async (...args: unknown[]) => { @@ -1349,7 +1351,7 @@ project: assert.notInclude(byUri.get(uriNb2.toString()) ?? '', 'nb1-new'); }); - test('should clear notebook selection before processing file change', async () => { + test('should not clear notebook selection before processing file change', async () => { const basePath = Uri.file('/workspace/multi.deepnote'); const uriNb1 = basePath.with({ query: 'a=1' }); const uriNb2 = basePath.with({ query: 'b=2' }); @@ -1393,7 +1395,7 @@ project: await waitFor(() => applyEditCount >= 2); - verify(mockedNotebookManager.clearNotebookSelection('project-1')).once(); + verify(mockedNotebookManager.clearNotebookSelection(anything())).never(); }); test('should not corrupt other notebooks when one notebook triggers a file change', async () => { diff --git a/src/notebooks/deepnote/deepnoteNotebookManager.ts b/src/notebooks/deepnote/deepnoteNotebookManager.ts index 1d80822cc6..11ea318b83 100644 --- a/src/notebooks/deepnote/deepnoteNotebookManager.ts +++ b/src/notebooks/deepnote/deepnoteNotebookManager.ts @@ -3,6 +3,13 @@ import { injectable } from 'inversify'; import { IDeepnoteNotebookManager, ProjectIntegration } from '../types'; import type { DeepnoteProject } from '../../platform/deepnote/deepnoteTypes'; +const pendingNotebookResolutionTtlMs = 2_000; + +interface PendingNotebookResolution { + notebookId: string; + queuedAt: number; +} + /** * Centralized manager for tracking Deepnote notebook selections and project state. * Manages per-project state including current selections and project data caching. @@ -11,17 +18,36 @@ import type { DeepnoteProject } from '../../platform/deepnote/deepnoteTypes'; export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { private readonly currentNotebookId = new Map(); private readonly originalProjects = new Map(); + private readonly pendingNotebookResolutions = new Map(); private readonly projectsWithInitNotebookRun = new Set(); private readonly selectedNotebookByProject = new Map(); /** - * Clears the notebook selection for a project so that subsequent - * deserializations fall back to the active editor or open documents. + * Clears the remembered notebook selection and any pending resolution hints for a project. */ clearNotebookSelection(projectId: string): void { + this.pendingNotebookResolutions.delete(projectId); this.selectedNotebookByProject.delete(projectId); } + /** + * Consumes the next short-lived notebook resolution hint for a project. + * These hints are queued immediately before operations that trigger a + * deserialize without explicit URI context. + */ + consumePendingNotebookResolution(projectId: string): string | undefined { + const pendingResolutions = this.getValidPendingNotebookResolutions(projectId); + const nextResolution = pendingResolutions.shift(); + + if (pendingResolutions.length > 0) { + this.pendingNotebookResolutions.set(projectId, pendingResolutions); + } else { + this.pendingNotebookResolutions.delete(projectId); + } + + return nextResolution?.notebookId; + } + /** * Gets the currently selected notebook ID for a project. * @param projectId Project identifier @@ -50,9 +76,24 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { } /** - * Associates a notebook ID with a project to remember user's notebook selection. - * When a Deepnote project contains multiple notebooks, this mapping persists the user's - * choice so we can automatically open the same notebook on subsequent file opens. + * Queues a short-lived notebook resolution hint for the next deserialize. + * + * @param projectId - The project ID that identifies the Deepnote project + * @param notebookId - The notebook ID the next deserialize should resolve to + */ + queueNotebookResolution(projectId: string, notebookId: string): void { + const pendingResolutions = this.getValidPendingNotebookResolutions(projectId); + + pendingResolutions.push({ + notebookId, + queuedAt: Date.now() + }); + + this.pendingNotebookResolutions.set(projectId, pendingResolutions); + } + + /** + * Associates a notebook ID with a project to remember the user's last explicit selection. * * @param projectId - The project ID that identifies the Deepnote project * @param notebookId - The ID of the selected notebook within the project @@ -134,4 +175,18 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { markInitNotebookAsRun(projectId: string): void { this.projectsWithInitNotebookRun.add(projectId); } + + private getValidPendingNotebookResolutions(projectId: string): PendingNotebookResolution[] { + const cutoffTime = Date.now() - pendingNotebookResolutionTtlMs; + const pendingResolutions = (this.pendingNotebookResolutions.get(projectId) ?? []).filter( + (resolution) => resolution.queuedAt >= cutoffTime + ); + + if (pendingResolutions.length === 0) { + this.pendingNotebookResolutions.delete(projectId); + return []; + } + + return pendingResolutions; + } } diff --git a/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts b/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts index 7c23b7e922..4888138eef 100644 --- a/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts @@ -93,6 +93,39 @@ suite('DeepnoteNotebookManager', () => { }); }); + suite('consumePendingNotebookResolution', () => { + test('should return undefined when no pending resolution exists', () => { + const result = manager.consumePendingNotebookResolution('unknown-project'); + + assert.strictEqual(result, undefined); + }); + + test('should consume queued notebook resolutions in order', () => { + manager.queueNotebookResolution('project-123', 'notebook-1'); + manager.queueNotebookResolution('project-123', 'notebook-2'); + + assert.strictEqual(manager.consumePendingNotebookResolution('project-123'), 'notebook-1'); + assert.strictEqual(manager.consumePendingNotebookResolution('project-123'), 'notebook-2'); + assert.strictEqual(manager.consumePendingNotebookResolution('project-123'), undefined); + }); + + test('should keep pending resolutions isolated per project', () => { + manager.queueNotebookResolution('project-1', 'notebook-1'); + manager.queueNotebookResolution('project-2', 'notebook-2'); + + assert.strictEqual(manager.consumePendingNotebookResolution('project-1'), 'notebook-1'); + assert.strictEqual(manager.consumePendingNotebookResolution('project-2'), 'notebook-2'); + }); + }); + + suite('queueNotebookResolution', () => { + test('should queue a notebook resolution for later consumption', () => { + manager.queueNotebookResolution('project-123', 'notebook-456'); + + assert.strictEqual(manager.consumePendingNotebookResolution('project-123'), 'notebook-456'); + }); + }); + suite('selectNotebookForProject', () => { test('should store notebook selection for project', () => { manager.selectNotebookForProject('project-123', 'notebook-456'); @@ -148,6 +181,13 @@ suite('DeepnoteNotebookManager', () => { manager.clearNotebookSelection('unknown-project'); }); }); + + test('should clear pending notebook resolutions for a project', () => { + manager.queueNotebookResolution('project-123', 'notebook-456'); + manager.clearNotebookSelection('project-123'); + + assert.strictEqual(manager.consumePendingNotebookResolution('project-123'), undefined); + }); }); suite('storeOriginalProject', () => { diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index 532b1eb48e..ad68f90b4f 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -1,7 +1,7 @@ import type { DeepnoteBlock, DeepnoteFile, DeepnoteSnapshot } from '@deepnote/blocks'; import { deserializeDeepnoteFile, isExecutableBlock, serializeDeepnoteSnapshot } from '@deepnote/blocks'; import { inject, injectable, optional } from 'inversify'; -import { l10n, window, workspace, type CancellationToken, type NotebookData, type NotebookSerializer } from 'vscode'; +import { l10n, workspace, type CancellationToken, type NotebookData, type NotebookSerializer } from 'vscode'; import { logger } from '../../platform/logging'; import { IDeepnoteNotebookManager } from '../types'; @@ -62,7 +62,8 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { /** * Deserializes a Deepnote YAML file into VS Code notebook format. * Parses YAML and converts the selected notebook's blocks to cells. - * The notebook to deserialize must be pre-selected and stored in the manager. + * Notebook resolution prefers an explicit notebook ID, then transient + * resolver state, and finally a deterministic default notebook. * @param content Raw file content as bytes * @param token Cancellation token (unused) * @returns Promise resolving to notebook data @@ -186,37 +187,29 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { } /** - * Finds the notebook ID to deserialize by checking the manager's stored selection. - * The notebook ID should be set via selectNotebookForProject before opening the document. + * Finds the notebook ID to deserialize without relying on active-editor state. + * Prefers a pending resolution hint, then current/open-document state. * @param projectId The project ID to find a notebook for * @returns The notebook ID to deserialize, or undefined if none found */ findCurrentNotebookId(projectId: string): string | undefined { - // Check the manager's stored selection first - this is set explicitly when the user - // picks a notebook from the explorer, and must take priority over the active editor - const storedNotebookId = this.notebookManager.getTheSelectedNotebookForAProject(projectId); + const pendingNotebookId = this.notebookManager.consumePendingNotebookResolution(projectId); + const openNotebookIds = this.findOpenNotebookIds(projectId); + const currentNotebookId = this.notebookManager.getCurrentNotebookId(projectId); - if (storedNotebookId) { - return storedNotebookId; + if (pendingNotebookId) { + return pendingNotebookId; } - // Fallback: prefer the active notebook editor when it matches the project - const activeEditorNotebook = window.activeNotebookEditor?.notebook; - - if ( - activeEditorNotebook?.notebookType === 'deepnote' && - activeEditorNotebook.metadata?.deepnoteProjectId === projectId && - activeEditorNotebook.metadata?.deepnoteNotebookId - ) { - return activeEditorNotebook.metadata.deepnoteNotebookId; + if (currentNotebookId && (openNotebookIds.length === 0 || openNotebookIds.includes(currentNotebookId))) { + return currentNotebookId; } - // Last fallback: Check if there's an active notebook document for this project - const openNotebook = workspace.notebookDocuments.find( - (doc) => doc.notebookType === 'deepnote' && doc.metadata?.deepnoteProjectId === projectId - ); + if (openNotebookIds.length === 1) { + return openNotebookIds[0]; + } - return openNotebook?.metadata?.deepnoteNotebookId; + return undefined; } /** @@ -264,7 +257,7 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { logger.debug('SerializeNotebook: Got and cloned original project'); const notebookId = - data.metadata?.deepnoteNotebookId || this.notebookManager.getTheSelectedNotebookForAProject(projectId); + data.metadata?.deepnoteNotebookId || this.notebookManager.getCurrentNotebookId(projectId); if (!notebookId) { throw new Error('Cannot determine which notebook to save'); @@ -353,6 +346,11 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { // Store the updated project back so subsequent saves start from correct state this.notebookManager.storeOriginalProject(projectId, originalProject, notebookId); + const openNotebookIdsAtSerialize = this.findOpenNotebookIds(projectId); + + if (openNotebookIdsAtSerialize.length === 0) { + this.notebookManager.queueNotebookResolution(projectId, notebookId); + } logger.debug('SerializeNotebook: Serializing to YAML'); @@ -555,6 +553,21 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return false; } + private findOpenNotebookIds(projectId: string): string[] { + return [ + ...new Set( + workspace.notebookDocuments + .filter( + (doc) => + doc.notebookType === 'deepnote' && + doc.metadata?.deepnoteProjectId === projectId && + typeof doc.metadata?.deepnoteNotebookId === 'string' + ) + .map((doc) => doc.metadata.deepnoteNotebookId as string) + ) + ]; + } + /** * Finds the default notebook to open when no selection is made. * @param file diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index 0e27d79283..b562c85f1d 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -74,9 +74,8 @@ suite('DeepnoteNotebookSerializer', () => { } suite('deserializeNotebook', () => { - test('should deserialize valid project with selected notebook', async () => { - // Set up the manager to select the first notebook - manager.selectNotebookForProject('project-123', 'notebook-1'); + test('should deserialize valid project with queued notebook resolution', async () => { + manager.queueNotebookResolution('project-123', 'notebook-1'); const yamlContent = ` version: '1.0.0' @@ -168,7 +167,7 @@ project: }); test('should fall back to findCurrentNotebookId when notebookId is undefined', async () => { - manager.selectNotebookForProject('project-123', 'notebook-1'); + manager.queueNotebookResolution('project-123', 'notebook-1'); const content = projectToYaml(mockProject); const result = await serializer.deserializeNotebook(content, {} as any); @@ -233,6 +232,56 @@ project: assert.include(yamlString, 'project-123'); assert.include(yamlString, 'notebook-1'); }); + + test('should use current notebook ID instead of stale selected notebook when metadata notebook ID is missing', async () => { + manager.storeOriginalProject('project-123', mockProject, 'notebook-2'); + manager.selectNotebookForProject('project-123', 'notebook-1'); + + const mockNotebookData = { + cells: [ + { + kind: 1, // NotebookCellKind.Markup + value: '# Updated second notebook', + languageId: 'markdown', + metadata: {} + } + ], + metadata: { + deepnoteProjectId: 'project-123' + } + }; + + const result = await serializer.serializeNotebook(mockNotebookData as any, {} as any); + const serializedProject = deserializeDeepnoteFile(new TextDecoder().decode(result)); + + assert.strictEqual(serializedProject.project.notebooks[0].blocks?.[0].content, 'print("hello")'); + assert.strictEqual(serializedProject.project.notebooks[1].blocks?.[0].content, '# Updated second notebook'); + }); + + test('should queue the serialized notebook for the next resolution hint', async () => { + manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); + + const mockNotebookData = { + cells: [ + { + kind: 1, // NotebookCellKind.Markup + value: '# Updated second notebook', + languageId: 'markdown', + metadata: {} + } + ], + metadata: { + deepnoteProjectId: 'project-123', + deepnoteNotebookId: 'notebook-2' + } + }; + + await serializer.serializeNotebook(mockNotebookData as any, {} as any); + + manager.updateCurrentNotebookId('project-123', 'notebook-1'); + + assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-2'); + }); }); suite('findCurrentNotebookId', () => { @@ -242,22 +291,31 @@ project: when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([]); }); - test('should return stored notebook ID when available', () => { - manager.selectNotebookForProject('project-123', 'notebook-456'); + test('should return queued notebook resolution when available', () => { + manager.queueNotebookResolution('project-123', 'queued-notebook'); const result = serializer.findCurrentNotebookId('project-123'); - assert.strictEqual(result, 'notebook-456'); + assert.strictEqual(result, 'queued-notebook'); + }); + + test('should consume queued notebook resolution only once', () => { + manager.queueNotebookResolution('project-123', 'queued-notebook'); + + assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'queued-notebook'); + assert.strictEqual(serializer.findCurrentNotebookId('project-123'), undefined); }); - test('should fall back to active notebook document when no stored selection', () => { - // Create a mock notebook document + test('should prioritize queued notebook resolution over current notebook and open documents', () => { + manager.queueNotebookResolution('project-123', 'queued-notebook'); + manager.updateCurrentNotebookId('project-123', 'current-notebook'); + const mockNotebookDoc = { - then: undefined, // Prevent mock from being treated as a Promise-like thenable + then: undefined, notebookType: 'deepnote', metadata: { deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'notebook-from-workspace' + deepnoteNotebookId: 'open-notebook' }, uri: {} as any, version: 1, @@ -270,213 +328,147 @@ project: save: async () => true } as NotebookDocument; - // Configure the mocked workspace.notebookDocuments (same pattern as other tests) when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([mockNotebookDoc]); const result = serializer.findCurrentNotebookId('project-123'); - assert.strictEqual(result, 'notebook-from-workspace'); + assert.strictEqual(result, 'queued-notebook'); }); - test('should return undefined for unknown project', () => { - const result = serializer.findCurrentNotebookId('unknown-project'); - - assert.strictEqual(result, undefined); - }); - - test('should prioritize stored selection over fallback', () => { - manager.selectNotebookForProject('project-123', 'stored-notebook'); - - const result = serializer.findCurrentNotebookId('project-123'); - - assert.strictEqual(result, 'stored-notebook'); - }); - - test('should handle multiple projects independently', () => { - manager.selectNotebookForProject('project-1', 'notebook-1'); - manager.selectNotebookForProject('project-2', 'notebook-2'); - - const result1 = serializer.findCurrentNotebookId('project-1'); - const result2 = serializer.findCurrentNotebookId('project-2'); - - assert.strictEqual(result1, 'notebook-1'); - assert.strictEqual(result2, 'notebook-2'); - }); - - test('should prioritize stored selection over active editor', () => { - manager.selectNotebookForProject('project-123', 'stored-notebook'); - - const mockActiveNotebook = { - notebookType: 'deepnote', - metadata: { - deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'active-editor-notebook' - } - }; - - when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ - notebook: mockActiveNotebook - } as any); + test('should return current notebook ID when no pending resolution exists', () => { + manager.updateCurrentNotebookId('project-123', 'current-notebook'); const result = serializer.findCurrentNotebookId('project-123'); - assert.strictEqual(result, 'stored-notebook'); + assert.strictEqual(result, 'current-notebook'); }); - test('should return active editor notebook when no stored selection exists', () => { - const mockActiveNotebook = { + test('should return the only open notebook when current notebook ID is unavailable', () => { + const mockNotebookDoc = { + then: undefined, notebookType: 'deepnote', metadata: { deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'active-editor-notebook' - } - }; + deepnoteNotebookId: 'notebook-from-workspace' + }, + uri: {} as any, + version: 1, + isDirty: false, + isUntitled: false, + isClosed: false, + cellCount: 0, + cellAt: () => ({}) as any, + getCells: () => [], + save: async () => true + } as NotebookDocument; - when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ - notebook: mockActiveNotebook - } as any); + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([mockNotebookDoc]); const result = serializer.findCurrentNotebookId('project-123'); - assert.strictEqual(result, 'active-editor-notebook'); + assert.strictEqual(result, 'notebook-from-workspace'); }); - test('should ignore active editor when project ID does not match', () => { - manager.selectNotebookForProject('project-123', 'stored-notebook'); + test('should prefer current notebook ID when multiple notebooks are open for a project', () => { + manager.updateCurrentNotebookId('project-123', 'notebook-b'); - const mockActiveNotebook = { + const notebookA = { + then: undefined, notebookType: 'deepnote', - metadata: { - deepnoteProjectId: 'different-project', - deepnoteNotebookId: 'active-editor-notebook' - } - }; - - when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ - notebook: mockActiveNotebook - } as any); - - const result = serializer.findCurrentNotebookId('project-123'); - - assert.strictEqual(result, 'stored-notebook'); - }); - - test('should ignore active editor when notebook type is not deepnote', () => { - manager.selectNotebookForProject('project-123', 'stored-notebook'); - - const mockActiveNotebook = { - notebookType: 'jupyter-notebook', metadata: { deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'active-editor-notebook' - } - }; - - when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ - notebook: mockActiveNotebook - } as any); - - const result = serializer.findCurrentNotebookId('project-123'); - - assert.strictEqual(result, 'stored-notebook'); - }); - - test('should ignore active editor when notebook ID is missing', () => { - manager.selectNotebookForProject('project-123', 'stored-notebook'); - - const mockActiveNotebook = { + deepnoteNotebookId: 'notebook-a' + }, + uri: {} as any, + version: 1, + isDirty: false, + isUntitled: false, + isClosed: false, + cellCount: 0, + cellAt: () => ({}) as any, + getCells: () => [], + save: async () => true + } as NotebookDocument; + const notebookB = { + then: undefined, notebookType: 'deepnote', metadata: { - deepnoteProjectId: 'project-123' - } - }; + deepnoteProjectId: 'project-123', + deepnoteNotebookId: 'notebook-b' + }, + uri: {} as any, + version: 1, + isDirty: false, + isUntitled: false, + isClosed: false, + cellCount: 0, + cellAt: () => ({}) as any, + getCells: () => [], + save: async () => true + } as NotebookDocument; - when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ - notebook: mockActiveNotebook - } as any); + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookA, notebookB]); const result = serializer.findCurrentNotebookId('project-123'); - assert.strictEqual(result, 'stored-notebook'); + assert.strictEqual(result, 'notebook-b'); }); - test('switching notebooks: selecting a different notebook while one is open should return the new selection', () => { - manager.selectNotebookForProject('project-123', 'notebook-A'); - - const mockActiveNotebook = { + test('should return undefined when multiple notebooks are open and no stronger signal exists', () => { + const notebookA = { + then: undefined, notebookType: 'deepnote', metadata: { deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'notebook-A' - } - }; - - when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ - notebook: mockActiveNotebook - } as any); - - assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-A'); - - manager.selectNotebookForProject('project-123', 'notebook-B'); - - assert.strictEqual( - serializer.findCurrentNotebookId('project-123'), - 'notebook-B', - 'Should return the newly selected notebook, not the one currently in the active editor' - ); - }); - - test('switching notebooks: rapidly switching between three notebooks should always return the latest selection', () => { - const mockActiveNotebook = { + deepnoteNotebookId: 'notebook-a' + }, + uri: {} as any, + version: 1, + isDirty: false, + isUntitled: false, + isClosed: false, + cellCount: 0, + cellAt: () => ({}) as any, + getCells: () => [], + save: async () => true + } as NotebookDocument; + const notebookB = { + then: undefined, notebookType: 'deepnote', metadata: { deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'notebook-1' - } - }; - - when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ - notebook: mockActiveNotebook - } as any); + deepnoteNotebookId: 'notebook-b' + }, + uri: {} as any, + version: 1, + isDirty: false, + isUntitled: false, + isClosed: false, + cellCount: 0, + cellAt: () => ({}) as any, + getCells: () => [], + save: async () => true + } as NotebookDocument; - manager.selectNotebookForProject('project-123', 'notebook-1'); - assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-1'); + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookA, notebookB]); - manager.selectNotebookForProject('project-123', 'notebook-2'); - assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-2'); + const result = serializer.findCurrentNotebookId('project-123'); - manager.selectNotebookForProject('project-123', 'notebook-3'); - assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-3'); + assert.strictEqual(result, undefined); }); - test('should return undefined when selection is cleared and no active editor', () => { - manager.selectNotebookForProject('project-123', 'notebook-456'); - manager.clearNotebookSelection('project-123'); + test('should ignore stale selected notebook state when no other resolver state exists', () => { + manager.selectNotebookForProject('project-123', 'stored-notebook'); const result = serializer.findCurrentNotebookId('project-123'); assert.strictEqual(result, undefined); }); - test('should fall back to active editor after selection is cleared', () => { - manager.selectNotebookForProject('project-123', 'stored-wrong'); - manager.clearNotebookSelection('project-123'); - - const mockActiveNotebook = { - notebookType: 'deepnote', - metadata: { - deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'active-editor-notebook' - } - }; - - when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn({ - notebook: mockActiveNotebook - } as any); - - const result = serializer.findCurrentNotebookId('project-123'); + test('should return undefined for unknown project', () => { + const result = serializer.findCurrentNotebookId('unknown-project'); - assert.strictEqual(result, 'active-editor-notebook'); + assert.strictEqual(result, undefined); }); }); @@ -503,12 +495,14 @@ project: }); test('should handle manager state operations', () => { + assert.isFunction(manager.consumePendingNotebookResolution, 'has consumePendingNotebookResolution method'); assert.isFunction(manager.getCurrentNotebookId, 'has getCurrentNotebookId method'); assert.isFunction(manager.getOriginalProject, 'has getOriginalProject method'); assert.isFunction( manager.getTheSelectedNotebookForAProject, 'has getTheSelectedNotebookForAProject method' ); + assert.isFunction(manager.queueNotebookResolution, 'has queueNotebookResolution method'); assert.isFunction(manager.selectNotebookForProject, 'has selectNotebookForProject method'); assert.isFunction(manager.storeOriginalProject, 'has storeOriginalProject method'); }); diff --git a/src/notebooks/types.ts b/src/notebooks/types.ts index fd2fd83c44..c460019913 100644 --- a/src/notebooks/types.ts +++ b/src/notebooks/types.ts @@ -38,9 +38,13 @@ export interface ProjectIntegration { export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); export interface IDeepnoteNotebookManager { clearNotebookSelection(projectId: string): void; + consumePendingNotebookResolution(projectId: string): string | undefined; getCurrentNotebookId(projectId: string): string | undefined; getOriginalProject(projectId: string): DeepnoteProject | undefined; getTheSelectedNotebookForAProject(projectId: string): string | undefined; + hasInitNotebookBeenRun(projectId: string): boolean; + markInitNotebookAsRun(projectId: string): void; + queueNotebookResolution(projectId: string, notebookId: string): void; selectNotebookForProject(projectId: string, notebookId: string): void; storeOriginalProject(projectId: string, project: DeepnoteProject, notebookId: string): void; updateCurrentNotebookId(projectId: string, notebookId: string): void; @@ -54,7 +58,4 @@ export interface IDeepnoteNotebookManager { * @returns `true` if the project was found and updated successfully, `false` if the project does not exist */ updateProjectIntegrations(projectId: string, integrations: ProjectIntegration[]): boolean; - - hasInitNotebookBeenRun(projectId: string): boolean; - markInitNotebookAsRun(projectId: string): void; } From 36cc1e61577c1cecadec64cf60555ebe8d5cc770 Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 31 Mar 2026 07:46:49 +0000 Subject: [PATCH 15/35] refactor(deepnote): Improve notebook ID retrieval with zod validation --- src/notebooks/deepnote/deepnoteSerializer.ts | 28 +++++++++++--------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index ad68f90b4f..2a79a4c7af 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -2,13 +2,14 @@ import type { DeepnoteBlock, DeepnoteFile, DeepnoteSnapshot } from '@deepnote/bl import { deserializeDeepnoteFile, isExecutableBlock, serializeDeepnoteSnapshot } from '@deepnote/blocks'; import { inject, injectable, optional } from 'inversify'; import { l10n, workspace, type CancellationToken, type NotebookData, type NotebookSerializer } from 'vscode'; +import { z } from 'zod'; +import { computeHash } from '../../platform/common/crypto'; +import type { DeepnoteNotebook } from '../../platform/deepnote/deepnoteTypes'; import { logger } from '../../platform/logging'; import { IDeepnoteNotebookManager } from '../types'; import { DeepnoteDataConverter } from './deepnoteDataConverter'; -import type { DeepnoteNotebook } from '../../platform/deepnote/deepnoteTypes'; import { SnapshotService } from './snapshots/snapshotService'; -import { computeHash } from '../../platform/common/crypto'; export type { DeepnoteBlock, DeepnoteFile } from '@deepnote/blocks'; export { DeepnoteNotebook, DeepnoteOutput } from '../../platform/deepnote/deepnoteTypes'; @@ -555,16 +556,19 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { private findOpenNotebookIds(projectId: string): string[] { return [ - ...new Set( - workspace.notebookDocuments - .filter( - (doc) => - doc.notebookType === 'deepnote' && - doc.metadata?.deepnoteProjectId === projectId && - typeof doc.metadata?.deepnoteNotebookId === 'string' - ) - .map((doc) => doc.metadata.deepnoteNotebookId as string) - ) + ...workspace.notebookDocuments.reduce((ids, doc) => { + if (doc.notebookType !== 'deepnote' || doc.metadata.deepnoteProjectId !== projectId) { + return ids; + } + + const parsed = z.string().safeParse(doc.metadata.deepnoteNotebookId); + + if (parsed.success) { + ids.add(parsed.data); + } + + return ids; + }, new Set()) ]; } From 71579cfc8e92ef83acdb7bf8cc3d370e17650c83 Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 31 Mar 2026 08:28:07 +0000 Subject: [PATCH 16/35] refactor(deepnote): Improve variable naming and import organization in DeepnoteFileChangeWatcher --- src/notebooks/deepnote/deepnoteFileChangeWatcher.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index caa2d01fb7..60b0d08667 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -1,3 +1,5 @@ +import type { DeepnoteBlock } from '@deepnote/blocks'; +import { inject, injectable, optional } from 'inversify'; import { CancellationTokenSource, NotebookCell, @@ -10,13 +12,11 @@ import { WorkspaceEdit, workspace } from 'vscode'; -import { inject, injectable, optional } from 'inversify'; -import type { DeepnoteBlock } from '@deepnote/blocks'; -import { IControllerRegistration } from '../controllers/types'; import { IExtensionSyncActivationService } from '../../platform/activation/types'; import { IDisposableRegistry } from '../../platform/common/types'; import { logger } from '../../platform/logging'; +import { IControllerRegistration } from '../controllers/types'; import { IDeepnoteNotebookManager } from '../types'; import { DeepnoteDataConverter } from './deepnoteDataConverter'; import { DeepnoteNotebookSerializer } from './deepnoteSerializer'; @@ -286,12 +286,12 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic // Pass the notebook ID explicitly to avoid mutating the global selection state. // Multiple notebooks from the same project may be open simultaneously, and // mutating selectedNotebookByProject would cause race conditions. - const notebookNotebookId = notebook.metadata?.deepnoteNotebookId as string | undefined; + const targetNotebookId = notebook.metadata?.deepnoteNotebookId as string | undefined; const tokenSource = new CancellationTokenSource(); let newData; try { - newData = await this.serializer.deserializeNotebook(content, tokenSource.token, notebookNotebookId); + newData = await this.serializer.deserializeNotebook(content, tokenSource.token, targetNotebookId); } catch (error) { logger.warn(`[FileChangeWatcher] Failed to parse changed file: ${fileUri.path}`, error); return; @@ -378,13 +378,16 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic // Save to clear dirty state. VS Code serializes (same bytes) and sees the // mtime from our recent write, so no "content is newer" conflict. + this.markSelfWrite(fileUri); try { const saved = await workspace.save(notebook.uri); if (!saved) { + this.consumeSelfWrite(fileUri); logger.warn(`[FileChangeWatcher] Save after sync write returned undefined: ${notebook.uri.path}`); return; } } catch (saveError) { + this.consumeSelfWrite(fileUri); logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError); } } catch (serializeError) { From 23fdccac6e6ea3a44f8f97104d1cc44b7f0280de Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 31 Mar 2026 11:55:29 +0000 Subject: [PATCH 17/35] Fix notebook deserialization race conditions --- .../deepnote/deepnoteFileChangeWatcher.ts | 9 ++++ .../deepnoteFileChangeWatcher.unit.test.ts | 7 +++ .../deepnote/deepnoteNotebookManager.ts | 14 +++++- src/notebooks/deepnote/deepnoteSerializer.ts | 43 +++++++++++++++++-- src/notebooks/types.ts | 1 + 5 files changed, 70 insertions(+), 4 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index 60b0d08667..e413200a12 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -99,6 +99,15 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic this.disposables.push(watcher.onDidCreate((uri) => this.handleFileChange(uri))); this.disposables.push({ dispose: () => this.clearAllTimers() }); + this.disposables.push( + workspace.onDidSaveNotebookDocument((notebook) => { + if (notebook.notebookType === 'deepnote') { + const fileUri = notebook.uri.with({ query: '', fragment: '' }); + this.markSelfWrite(fileUri); + } + }) + ); + if (this.snapshotService) { this.disposables.push( this.snapshotService.onFileWritten((uri) => { diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index 5090c26d57..5d2d375d48 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -122,6 +122,7 @@ suite('DeepnoteFileChangeWatcher', () => { let mockNotebookManager: IDeepnoteNotebookManager; let onDidChangeFile: EventEmitter; let onDidCreateFile: EventEmitter; + let onDidSaveNotebook: EventEmitter; let readFileCalls: number; let applyEditCount: number; let saveCount: number; @@ -139,6 +140,7 @@ suite('DeepnoteFileChangeWatcher', () => { when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(validProject); when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); when(mockedNotebookManager.queueNotebookResolution(anything(), anything())).thenReturn(); + when(mockedNotebookManager.updateOriginalProject(anything(), anything())).thenReturn(); mockNotebookManager = instance(mockedNotebookManager); // Set up FileSystemWatcher mock @@ -151,6 +153,9 @@ suite('DeepnoteFileChangeWatcher', () => { when(mockedVSCodeNamespaces.workspace.createFileSystemWatcher(anything())).thenReturn(instance(fsWatcher)); + onDidSaveNotebook = new EventEmitter(); + when(mockedVSCodeNamespaces.workspace.onDidSaveNotebookDocument).thenReturn(onDidSaveNotebook.event); + when(mockedVSCodeNamespaces.workspace.applyEdit(anything())).thenCall(() => { applyEditCount++; return Promise.resolve(true); @@ -171,6 +176,7 @@ suite('DeepnoteFileChangeWatcher', () => { } onDidChangeFile.dispose(); onDidCreateFile.dispose(); + onDidSaveNotebook.dispose(); }); function createMockNotebook(opts: { @@ -1276,6 +1282,7 @@ project: when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(multiNotebookProject); when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); when(mockedNotebookManager.queueNotebookResolution(anything(), anything())).thenReturn(); + when(mockedNotebookManager.updateOriginalProject(anything(), anything())).thenReturn(); resetCalls(mockedNotebookManager); workspaceSetCaptures = []; sinon.stub(watcher, 'applyNotebookEdits' as any).callsFake(async (...args: unknown[]) => { diff --git a/src/notebooks/deepnote/deepnoteNotebookManager.ts b/src/notebooks/deepnote/deepnoteNotebookManager.ts index 11ea318b83..68360c7df4 100644 --- a/src/notebooks/deepnote/deepnoteNotebookManager.ts +++ b/src/notebooks/deepnote/deepnoteNotebookManager.ts @@ -3,7 +3,7 @@ import { injectable } from 'inversify'; import { IDeepnoteNotebookManager, ProjectIntegration } from '../types'; import type { DeepnoteProject } from '../../platform/deepnote/deepnoteTypes'; -const pendingNotebookResolutionTtlMs = 2_000; +const pendingNotebookResolutionTtlMs = 60_000; interface PendingNotebookResolution { notebookId: string; @@ -120,6 +120,18 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { this.currentNotebookId.set(projectId, notebookId); } + /** + * Updates the stored project data without changing the current notebook selection. + * Used during serialization where we need to cache the updated project state + * but must not alter notebook routing for other open notebooks. + * @param projectId Project identifier + * @param project Updated project data to store + */ + updateOriginalProject(projectId: string, project: DeepnoteProject): void { + const clonedProject = structuredClone(project); + this.originalProjects.set(projectId, clonedProject); + } + /** * Updates the current notebook ID for a project. * Used when switching notebooks within the same project. diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index 2a79a4c7af..a9a0c8491d 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -51,10 +51,21 @@ function cloneWithoutCircularRefs(obj: T, seen = new WeakSet()): T { * Serializer for converting between Deepnote YAML files and VS Code notebook format. * Handles reading/writing .deepnote files and manages project state persistence. */ +/** Window (ms) during which a post-serialize deserialize excludes the serialized notebook. */ +const recentSerializeTtlMs = 5_000; + @injectable() export class DeepnoteNotebookSerializer implements NotebookSerializer { private converter = new DeepnoteDataConverter(); + /** + * Tracks the most recently serialized notebook per project. + * When VS Code calls $dataToNotebook after a save, it's re-reading the + * file for a SIBLING tab (the saved notebook already has its content). + * This tracker lets findCurrentNotebookId pick a sibling instead. + */ + private readonly recentSerializations = new Map(); + constructor( @inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager, @inject(SnapshotService) @optional() private readonly snapshotService?: SnapshotService @@ -202,7 +213,7 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return pendingNotebookId; } - if (currentNotebookId && (openNotebookIds.length === 0 || openNotebookIds.includes(currentNotebookId))) { + if (currentNotebookId && openNotebookIds.length === 0) { return currentNotebookId; } @@ -210,6 +221,27 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return openNotebookIds[0]; } + // Multiple notebooks open — VS Code may be re-reading the file for a + // sibling tab after a save. Pick a sibling (any open notebook that is + // NOT the one just serialized). If no recent serialization, fall back + // to currentNotebookId for backward compatibility. + if (openNotebookIds.length > 1) { + const recent = this.recentSerializations.get(projectId); + + if (recent && Date.now() - recent.timestamp < recentSerializeTtlMs) { + this.recentSerializations.delete(projectId); + const sibling = openNotebookIds.find((id) => id !== recent.notebookId); + + if (sibling) { + return sibling; + } + } + + if (currentNotebookId && openNotebookIds.includes(currentNotebookId)) { + return currentNotebookId; + } + } + return undefined; } @@ -345,8 +377,13 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { originalProject.metadata.modifiedAt = new Date().toISOString(); } - // Store the updated project back so subsequent saves start from correct state - this.notebookManager.storeOriginalProject(projectId, originalProject, notebookId); + // Store the updated project back so subsequent saves start from correct state. + // Use updateOriginalProject (not storeOriginalProject) to avoid overwriting + // currentNotebookId — when multiple notebooks share the same file, changing + // currentNotebookId here would cause VS Code's follow-up deserialize calls + // for other open notebooks to resolve to the wrong notebook. + this.notebookManager.updateOriginalProject(projectId, originalProject); + this.recentSerializations.set(projectId, { notebookId, timestamp: Date.now() }); const openNotebookIdsAtSerialize = this.findOpenNotebookIds(projectId); if (openNotebookIdsAtSerialize.length === 0) { diff --git a/src/notebooks/types.ts b/src/notebooks/types.ts index c460019913..527a56ddee 100644 --- a/src/notebooks/types.ts +++ b/src/notebooks/types.ts @@ -48,6 +48,7 @@ export interface IDeepnoteNotebookManager { selectNotebookForProject(projectId: string, notebookId: string): void; storeOriginalProject(projectId: string, project: DeepnoteProject, notebookId: string): void; updateCurrentNotebookId(projectId: string, notebookId: string): void; + updateOriginalProject(projectId: string, project: DeepnoteProject): void; /** * Updates the integrations list in the project data. From 824f471a753ee5fe44dbd88e133ba82189ac5e24 Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 31 Mar 2026 16:38:14 +0000 Subject: [PATCH 18/35] feat(deepnote): Enhance testing for DeepnoteFileChangeWatcher and DeepnoteNotebookManager - Added new unit tests for self-write detection in `DeepnoteFileChangeWatcher`, ensuring that saving a deepnote notebook suppresses subsequent file change events. - Implemented tests in `DeepnoteNotebookManager` to verify project data updates without altering the current notebook ID, ensuring data integrity during updates. - Introduced a utility function for creating notebook documents in tests, improving test setup consistency. - Expanded multi-notebook save scenarios in `DeepnoteNotebookSerializer` to validate notebook ID resolution during serialization and deserialization processes. --- .../deepnoteFileChangeWatcher.unit.test.ts | 298 ++++++++++++++---- .../deepnoteNotebookManager.unit.test.ts | 70 ++++ .../deepnote/deepnoteSerializer.unit.test.ts | 233 ++++++++++++++ 3 files changed, 547 insertions(+), 54 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index 5d2d375d48..f3b03a4935 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -1,5 +1,8 @@ import { DeepnoteFile, serializeDeepnoteFile } from '@deepnote/blocks'; import { assert } from 'chai'; +import { mkdtempSync, rmSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; import * as sinon from 'sinon'; import { anything, instance, mock, reset, resetCalls, verify, when } from 'ts-mockito'; import { @@ -116,6 +119,20 @@ async function waitFor( } suite('DeepnoteFileChangeWatcher', () => { + let testFixturesDir: string; + + suiteSetup(() => { + testFixturesDir = mkdtempSync(join(tmpdir(), 'deepnote-fcw-')); + }); + + suiteTeardown(() => { + rmSync(testFixturesDir, { recursive: true, force: true }); + }); + + function testFileUri(...pathSegments: string[]): Uri { + return Uri.file(join(testFixturesDir, ...pathSegments)); + } + let watcher: DeepnoteFileChangeWatcher; let mockDisposables: IDisposableRegistry; let mockedNotebookManager: IDeepnoteNotebookManager; @@ -160,9 +177,9 @@ suite('DeepnoteFileChangeWatcher', () => { applyEditCount++; return Promise.resolve(true); }); - when(mockedVSCodeNamespaces.workspace.save(anything())).thenCall(() => { + when(mockedVSCodeNamespaces.workspace.save(anything())).thenCall((uri: Uri) => { saveCount++; - return Promise.resolve(Uri.file('/workspace/test.deepnote')); + return Promise.resolve(uri); }); watcher = new DeepnoteFileChangeWatcher(mockDisposables, mockNotebookManager); @@ -244,8 +261,78 @@ project: content: print("hello") `; + suite('save-triggered self-write detection', () => { + test('saving a deepnote notebook should suppress the next FS change event', async () => { + const uri = testFileUri('self-write.deepnote'); + const notebook = createMockNotebook({ + notebookType: 'deepnote', + uri + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); + setupMockFs(validYaml); + + onDidSaveNotebook.fire(notebook); + onDidChangeFile.fire(uri); + + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + + assert.strictEqual(applyEditCount, 0, 'FS change after deepnote save should be treated as self-write'); + }); + + test('saving a non-deepnote notebook should not suppress FS change events', async function () { + this.timeout(8000); + const fileUri = testFileUri('jupyter-save.deepnote'); + const jupyterNotebook = createMockNotebook({ + cellCount: 0, + notebookType: 'jupyter-notebook', + uri: fileUri + }); + const deepnoteNotebook = createMockNotebook({ + cellCount: 0, + uri: fileUri.with({ query: 'view=1' }) + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([jupyterNotebook, deepnoteNotebook]); + setupMockFs(validYaml); + + onDidSaveNotebook.fire(jupyterNotebook); + onDidChangeFile.fire(fileUri); + + await waitFor(() => applyEditCount >= 1); + + assert.isAtLeast(applyEditCount, 1); + }); + + test('self-write is consumed exactly once', async function () { + this.timeout(8000); + const uri = testFileUri('self-write-once.deepnote'); + const notebook = createMockNotebook({ + notebookType: 'deepnote', + uri, + cellCount: 0 + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); + setupMockFs(validYaml); + + onDidSaveNotebook.fire(notebook); + onDidChangeFile.fire(uri); + + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + + assert.strictEqual(applyEditCount, 0, 'first FS event after save should be skipped'); + + onDidChangeFile.fire(uri); + + await waitFor(() => applyEditCount >= 1); + + assert.isAtLeast(applyEditCount, 1); + }); + }); + test('should skip reload when content matches notebook cells', async () => { - const uri = Uri.file('/workspace/test.deepnote'); + const uri = testFileUri('test.deepnote'); // Create a notebook whose cell content already matches validYaml const notebook = createMockNotebook({ uri, @@ -273,7 +360,7 @@ project: }); test('should reload on external change', async () => { - const uri = Uri.file('/workspace/test.deepnote'); + const uri = testFileUri('test.deepnote'); const notebook = createMockNotebook({ uri, cellCount: 0 }); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); @@ -290,7 +377,7 @@ project: }); test('should skip snapshot files when no SnapshotService', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/project_abc_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'project_abc_latest.snapshot.deepnote'); setupMockFs(validYaml); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([]); @@ -305,7 +392,7 @@ project: }); test('should reload dirty notebooks', async () => { - const uri = Uri.file('/workspace/test.deepnote'); + const uri = testFileUri('test.deepnote'); const notebook = createMockNotebook({ uri, isDirty: true }); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); @@ -323,7 +410,7 @@ project: }); test('should debounce rapid changes', async () => { - const uri = Uri.file('/workspace/test.deepnote'); + const uri = testFileUri('test.deepnote'); const notebook = createMockNotebook({ uri, cellCount: 0 }); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); @@ -344,7 +431,7 @@ project: }); test('should handle parse errors gracefully', async () => { - const uri = Uri.file('/workspace/test.deepnote'); + const uri = testFileUri('test.deepnote'); const notebook = createMockNotebook({ uri, cellCount: 0 }); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); @@ -360,7 +447,7 @@ project: }); test('should preserve live cell outputs during reload', async () => { - const uri = Uri.file('/workspace/test.deepnote'); + const uri = testFileUri('test.deepnote'); const fakeOutput = { items: [{ mime: 'text/plain', data: new Uint8Array([72]) }] }; const notebook = createMockNotebook({ uri, @@ -384,7 +471,7 @@ project: }); test('should reload dirty notebooks and preserve outputs', async () => { - const uri = Uri.file('/workspace/test.deepnote'); + const uri = testFileUri('test.deepnote'); const fakeOutput = { items: [{ mime: 'text/plain', data: new Uint8Array([72]) }] }; const notebook = createMockNotebook({ uri, @@ -410,7 +497,7 @@ project: test('should not suppress real changes after auto-save', async function () { this.timeout(5000); - const uri = Uri.file('/workspace/test.deepnote'); + const uri = testFileUri('test.deepnote'); // First change: notebook has no cells, YAML has one cell -> different -> reload const notebook = createMockNotebook({ uri, cellCount: 0, cells: [] }); @@ -451,7 +538,7 @@ project: }); test('should use atomic edit (single applyEdit for replaceCells + metadata)', async () => { - const uri = Uri.file('/workspace/test.deepnote'); + const uri = testFileUri('test.deepnote'); const notebook = createMockNotebook({ uri, cellCount: 0 }); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); @@ -466,7 +553,7 @@ project: }); test('should skip auto-save-triggered changes via content comparison', async () => { - const uri = Uri.file('/workspace/test.deepnote'); + const uri = testFileUri('test.deepnote'); // Notebook already has the same source as validYaml but with outputs const fakeOutput = { items: [{ mime: 'text/plain', data: new Uint8Array([72]) }] }; const notebook = createMockNotebook({ @@ -554,9 +641,9 @@ project: snapshotApplyEditCount++; return Promise.resolve(true); }); - when(mockedVSCodeNamespaces.workspace.save(anything())).thenCall(() => { + when(mockedVSCodeNamespaces.workspace.save(anything())).thenCall((uri: Uri) => { snapshotSaveCount++; - return Promise.resolve(Uri.file('/workspace/test.deepnote')); + return Promise.resolve(uri); }); snapshotWatcher = new DeepnoteFileChangeWatcher( @@ -576,9 +663,9 @@ project: }); test('should update outputs when snapshot file changes', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -605,7 +692,7 @@ project: const noSnapshotWatcher = new DeepnoteFileChangeWatcher(noSnapshotDisposables, mockNotebookManager); noSnapshotWatcher.activate(); - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([]); @@ -622,9 +709,9 @@ project: }); test('should skip self-triggered snapshot writes via onFileWritten', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [{ metadata: { id: 'block-1', type: 'code' }, outputs: [] }] }); @@ -645,7 +732,7 @@ project: test('should skip when snapshots are disabled', async () => { when(mockSnapshotService.isSnapshotsEnabled()).thenReturn(false); - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); snapshotOnDidChange.fire(snapshotUri); @@ -655,12 +742,10 @@ project: }); test('should debounce rapid snapshot changes for same project', async () => { - const snapshotUri1 = Uri.file( - '/workspace/snapshots/my-project_project-1_2025-01-15T10-31-48.snapshot.deepnote' - ); - const snapshotUri2 = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri1 = testFileUri('snapshots', 'my-project_project-1_2025-01-15T10-31-48.snapshot.deepnote'); + const snapshotUri2 = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -683,9 +768,9 @@ project: }); test('should handle onDidCreate for new snapshot files', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -707,9 +792,9 @@ project: }); test('should skip update when snapshot outputs match live state', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -734,7 +819,7 @@ project: data: new TextEncoder().encode('Hello World') }; const notebookWithOutputs = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -753,10 +838,10 @@ project: }); test('should update outputs when content changed but count is the same', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const existingOutput = { items: [{ mime: 'text/plain', data: new Uint8Array([72]) }] }; const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -778,8 +863,8 @@ project: }); test('should skip main-file reload after snapshot update via self-write tracking', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); - const notebookUri = Uri.file('/workspace/test.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); + const notebookUri = testFileUri('test.deepnote'); const notebook = createMockNotebook({ uri: notebookUri, cells: [ @@ -822,9 +907,9 @@ project: }); test('should use two-phase edit for snapshot updates (replaceCells + metadata restore)', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -850,9 +935,9 @@ project: }); test('should call workspace.save after snapshot fallback output update', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -874,10 +959,10 @@ project: }); test('should preserve outputs for cells not covered by snapshot', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const existingOutput = { items: [{ mime: 'text/plain', data: new Uint8Array([72]) }] }; const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -949,10 +1034,10 @@ project: ); fallbackWatcher.activate(); - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); // Cell has NO id in metadata — simulates VS Code losing metadata after replaceCells const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), metadata: { deepnoteProjectId: 'project-1', deepnoteNotebookId: 'notebook-1' }, cells: [ { @@ -996,7 +1081,7 @@ project: }); test('should only update cells whose outputs changed (per-cell updates)', async () => { - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); // Two cells: block-1 has no outputs (will get updated), block-2 already has matching outputs const outputItem = { @@ -1004,7 +1089,7 @@ project: data: new TextEncoder().encode('Existing output') }; const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -1116,9 +1201,9 @@ project: ); execWatcher.activate(); - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -1185,9 +1270,9 @@ project: ); nfWatcher.activate(); - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: {}, @@ -1244,9 +1329,9 @@ project: ); fbWatcher.activate(); - const snapshotUri = Uri.file('/workspace/snapshots/my-project_project-1_latest.snapshot.deepnote'); + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); const notebook = createMockNotebook({ - uri: Uri.file('/workspace/test.deepnote'), + uri: testFileUri('test.deepnote'), cells: [ { metadata: { id: 'block-1', type: 'code' }, @@ -1304,7 +1389,7 @@ project: }); test('should reload each notebook with its own content when multiple notebooks are open', async () => { - const basePath = Uri.file('/workspace/multi.deepnote'); + const basePath = testFileUri('multi.deepnote'); const uriNb1 = basePath.with({ query: 'view=1' }); const uriNb2 = basePath.with({ query: 'view=2' }); @@ -1359,7 +1444,7 @@ project: }); test('should not clear notebook selection before processing file change', async () => { - const basePath = Uri.file('/workspace/multi.deepnote'); + const basePath = testFileUri('multi.deepnote'); const uriNb1 = basePath.with({ query: 'a=1' }); const uriNb2 = basePath.with({ query: 'b=2' }); @@ -1406,7 +1491,7 @@ project: }); test('should not corrupt other notebooks when one notebook triggers a file change', async () => { - const basePath = Uri.file('/workspace/multi.deepnote'); + const basePath = testFileUri('multi.deepnote'); const uriNb1 = basePath.with({ query: 'n=1' }); const uriNb2 = basePath.with({ query: 'n=2' }); @@ -1461,5 +1546,110 @@ project: assert.notInclude(nb1Cells!, 'nb2-new', 'notebook-1 must not receive notebook-2 block content'); assert.notInclude(nb2Cells!, 'nb1-new', 'notebook-2 must not receive notebook-1 block content'); }); + + test('external edit to disk should update each open notebook and not be suppressed as self-write', async () => { + const basePath = testFileUri('multi-external.deepnote'); + const uriNb1 = basePath.with({ query: 'view=1' }); + const uriNb2 = basePath.with({ query: 'view=2' }); + + const notebook1 = createMockNotebook({ + uri: uriNb1, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-1' + }, + cells: [ + { + metadata: { id: 'block-nb1', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb1-old")', languageId: 'python' } + } + ] + }); + + const notebook2 = createMockNotebook({ + uri: uriNb2, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-2' + }, + cells: [ + { + metadata: { id: 'block-nb2', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb2-old")', languageId: 'python' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook1, notebook2]); + setupMockFs(multiNotebookYaml); + + onDidChangeFile.fire(basePath); + + await waitFor(() => applyEditCount >= 2); + + const byUri = new Map(workspaceSetCaptures.map((c) => [c.uriKey, c.cellSourceJoined])); + + assert.include(byUri.get(uriNb1.toString()) ?? '', 'nb1-new'); + assert.include(byUri.get(uriNb2.toString()) ?? '', 'nb2-new'); + }); + + test('external edit after a user save should still be processed', async function () { + this.timeout(8000); + const basePath = testFileUri('multi-save-then-external.deepnote'); + const uriNb1 = basePath.with({ query: 'view=1' }); + const uriNb2 = basePath.with({ query: 'view=2' }); + + const notebook1 = createMockNotebook({ + uri: uriNb1, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-1' + }, + cells: [ + { + metadata: { id: 'block-nb1' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb1-old")' } + } + ] + }); + + const notebook2 = createMockNotebook({ + uri: uriNb2, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-2' + }, + cells: [ + { + metadata: { id: 'block-nb2' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb2-old")' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook1, notebook2]); + setupMockFs(multiNotebookYaml); + + onDidSaveNotebook.fire(notebook1); + onDidChangeFile.fire(basePath); + + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + + assert.strictEqual(applyEditCount, 0, 'first FS event after save should be suppressed as self-write'); + + onDidChangeFile.fire(basePath); + + await waitFor(() => applyEditCount >= 1); + + assert.isAtLeast(applyEditCount, 1); + }); }); }); diff --git a/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts b/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts index 4888138eef..b4276c605e 100644 --- a/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts @@ -221,6 +221,76 @@ suite('DeepnoteNotebookManager', () => { }); }); + suite('updateOriginalProject', () => { + test('should update project data without changing currentNotebookId', () => { + const updatedProject: DeepnoteProject = { + ...mockProject, + project: { + ...mockProject.project, + name: 'Updated Name Only' + } + }; + + manager.storeOriginalProject('project-123', mockProject, 'notebook-456'); + manager.updateOriginalProject('project-123', updatedProject); + + const storedProject = manager.getOriginalProject('project-123'); + const currentNotebookId = manager.getCurrentNotebookId('project-123'); + + assert.deepStrictEqual(storedProject, updatedProject); + assert.strictEqual(currentNotebookId, 'notebook-456'); + }); + + test('should deep-clone project data so mutations to input do not affect stored state', () => { + const updatedProject: DeepnoteProject = { + ...mockProject, + project: { + ...mockProject.project, + name: 'Before Mutation' + } + }; + + manager.storeOriginalProject('project-123', mockProject, 'notebook-456'); + manager.updateOriginalProject('project-123', updatedProject); + + updatedProject.project.name = 'After Mutation'; + + const storedProject = manager.getOriginalProject('project-123'); + + assert.strictEqual(storedProject?.project.name, 'Before Mutation'); + }); + + test('should overwrite existing project data while preserving currentNotebookId', () => { + const firstUpdate: DeepnoteProject = { + ...mockProject, + project: { ...mockProject.project, name: 'First Update' } + }; + const secondUpdate: DeepnoteProject = { + ...mockProject, + project: { ...mockProject.project, name: 'Second Update' } + }; + + manager.storeOriginalProject('project-123', mockProject, 'notebook-456'); + manager.updateOriginalProject('project-123', firstUpdate); + manager.updateOriginalProject('project-123', secondUpdate); + + assert.strictEqual(manager.getCurrentNotebookId('project-123'), 'notebook-456'); + assert.strictEqual(manager.getOriginalProject('project-123')?.project.name, 'Second Update'); + }); + + test('should store project when no currentNotebookId has been set', () => { + const projectOnly: DeepnoteProject = { + ...mockProject, + project: { ...mockProject.project, name: 'No Notebook Id Yet' } + }; + + manager.updateOriginalProject('project-123', projectOnly); + + assert.strictEqual(manager.getCurrentNotebookId('project-123'), undefined); + assert.deepStrictEqual(manager.getOriginalProject('project-123'), projectOnly); + }); + }); + suite('updateCurrentNotebookId', () => { test('should update notebook ID for existing project', () => { manager.storeOriginalProject('project-123', mockProject, 'notebook-456'); diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index b562c85f1d..baa5e49591 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -73,6 +73,26 @@ suite('DeepnoteNotebookSerializer', () => { return new TextEncoder().encode(yamlString); } + function createNotebookDoc(notebookId: string, projectId: string = 'project-123'): NotebookDocument { + return { + cellAt: () => ({}) as any, + cellCount: 0, + getCells: () => [], + isClosed: false, + isDirty: false, + isUntitled: false, + metadata: { + deepnoteNotebookId: notebookId, + deepnoteProjectId: projectId + }, + notebookType: 'deepnote', + save: async () => true, + then: undefined, + uri: {} as any, + version: 1 + } as NotebookDocument; + } + suite('deserializeNotebook', () => { test('should deserialize valid project with queued notebook resolution', async () => { manager.queueNotebookResolution('project-123', 'notebook-1'); @@ -282,6 +302,125 @@ project: assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-2'); }); + + suite('multi-notebook save scenarios', () => { + teardown(() => { + when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn(undefined); + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([]); + }); + + test('serializing notebook-1 should not change currentNotebookId', async () => { + manager.queueNotebookResolution('project-123', 'notebook-2'); + await serializer.deserializeNotebook(projectToYaml(mockProject), {} as any); + + assert.strictEqual(manager.getCurrentNotebookId('project-123'), 'notebook-2'); + + const mockNotebookData = { + cells: [ + { + kind: 2, + languageId: 'python', + metadata: {}, + value: 'print("edited nb1")' + } + ], + metadata: { + deepnoteNotebookId: 'notebook-1', + deepnoteProjectId: 'project-123' + } + }; + + await serializer.serializeNotebook(mockNotebookData as any, {} as any); + + assert.strictEqual(manager.getCurrentNotebookId('project-123'), 'notebook-2'); + }); + + test('serializing notebook-1 then deserializing without explicit ID should resolve to sibling notebook-2', async () => { + const yamlBytes = projectToYaml(mockProject); + + manager.queueNotebookResolution('project-123', 'notebook-1'); + await serializer.deserializeNotebook(yamlBytes, {} as any); + manager.queueNotebookResolution('project-123', 'notebook-2'); + await serializer.deserializeNotebook(yamlBytes, {} as any); + + const notebookDoc1 = createNotebookDoc('notebook-1'); + const notebookDoc2 = createNotebookDoc('notebook-2'); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookDoc1, notebookDoc2]); + + const mockNotebookData = { + cells: [ + { + kind: 2, + languageId: 'python', + metadata: {}, + value: 'print("saved from nb1")' + } + ], + metadata: { + deepnoteNotebookId: 'notebook-1', + deepnoteProjectId: 'project-123' + } + }; + + const saved = await serializer.serializeNotebook(mockNotebookData as any, {} as any); + const afterSave = await serializer.deserializeNotebook(saved, {} as any); + + assert.strictEqual(afterSave.metadata?.deepnoteNotebookId, 'notebook-2'); + }); + + test('sequential saves: contextless deserialize returns the sibling after each serialize', async () => { + const yamlBytes = projectToYaml(mockProject); + + manager.queueNotebookResolution('project-123', 'notebook-1'); + await serializer.deserializeNotebook(yamlBytes, {} as any); + manager.queueNotebookResolution('project-123', 'notebook-2'); + await serializer.deserializeNotebook(yamlBytes, {} as any); + + const notebookDoc1 = createNotebookDoc('notebook-1'); + const notebookDoc2 = createNotebookDoc('notebook-2'); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookDoc1, notebookDoc2]); + + const dataNb1 = { + cells: [ + { + kind: 2, + languageId: 'python', + metadata: {}, + value: 'print("round-a")' + } + ], + metadata: { + deepnoteNotebookId: 'notebook-1', + deepnoteProjectId: 'project-123' + } + }; + let bytes = await serializer.serializeNotebook(dataNb1 as any, {} as any); + let meta = await serializer.deserializeNotebook(bytes, {} as any); + + assert.strictEqual(meta.metadata?.deepnoteNotebookId, 'notebook-2'); + + const dataNb2 = { + cells: [ + { + kind: 1, + languageId: 'markdown', + metadata: {}, + value: '# round-b' + } + ], + metadata: { + deepnoteNotebookId: 'notebook-2', + deepnoteProjectId: 'project-123' + } + }; + bytes = await serializer.serializeNotebook(dataNb2 as any, {} as any); + meta = await serializer.deserializeNotebook(bytes, {} as any); + + assert.strictEqual(meta.metadata?.deepnoteNotebookId, 'notebook-1'); + }); + }); }); suite('findCurrentNotebookId', () => { @@ -465,6 +604,100 @@ project: assert.strictEqual(result, undefined); }); + test('after serializing notebook-1 with both notebooks open should return sibling notebook-2', async () => { + manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); + + const notebookDoc1 = createNotebookDoc('notebook-1'); + const notebookDoc2 = createNotebookDoc('notebook-2'); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookDoc1, notebookDoc2]); + + const mockNotebookData = { + cells: [ + { + kind: 2, + languageId: 'python', + metadata: {}, + value: 'print("findOpen")' + } + ], + metadata: { + deepnoteNotebookId: 'notebook-1', + deepnoteProjectId: 'project-123' + } + }; + + await serializer.serializeNotebook(mockNotebookData as any, {} as any); + + const result = serializer.findCurrentNotebookId('project-123'); + + assert.strictEqual(result, 'notebook-2'); + }); + + test('recent serialization hint is one-shot', async () => { + manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); + + const notebookDoc1 = createNotebookDoc('notebook-1'); + const notebookDoc2 = createNotebookDoc('notebook-2'); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookDoc1, notebookDoc2]); + + const mockNotebookData = { + cells: [ + { + kind: 2, + languageId: 'python', + metadata: {}, + value: 'print("one-shot")' + } + ], + metadata: { + deepnoteNotebookId: 'notebook-1', + deepnoteProjectId: 'project-123' + } + }; + + await serializer.serializeNotebook(mockNotebookData as any, {} as any); + + assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-2'); + assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-1'); + }); + + test('recent serialization hint expires after TTL', async () => { + manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); + + const notebookDoc1 = createNotebookDoc('notebook-1'); + const notebookDoc2 = createNotebookDoc('notebook-2'); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookDoc1, notebookDoc2]); + + const mockNotebookData = { + cells: [ + { + kind: 2, + languageId: 'python', + metadata: {}, + value: 'print("ttl")' + } + ], + metadata: { + deepnoteNotebookId: 'notebook-1', + deepnoteProjectId: 'project-123' + } + }; + + await serializer.serializeNotebook(mockNotebookData as any, {} as any); + + (serializer as any).recentSerializations.set('project-123', { + notebookId: 'notebook-1', + timestamp: Date.now() - 6000 + }); + + const result = serializer.findCurrentNotebookId('project-123'); + + assert.strictEqual(result, 'notebook-1'); + }); + test('should return undefined for unknown project', () => { const result = serializer.findCurrentNotebookId('unknown-project'); From c6d21e6b45735f9cebf0545be99141c905eb7aa1 Mon Sep 17 00:00:00 2001 From: tomas Date: Wed, 1 Apr 2026 07:52:31 +0000 Subject: [PATCH 19/35] feat(deepnote): Add snapshot interaction tests for DeepnoteFileChangeWatcher - Introduced new unit tests to validate snapshot changes and deserialization interactions in `DeepnoteFileChangeWatcher`. - Enhanced test setup to capture interactions with notebook edits, ensuring accurate application of changes across multi-notebook projects. - Improved organization of imports and added missing type definitions for better clarity and maintainability. --- .../deepnoteFileChangeWatcher.unit.test.ts | 560 +++++++++++++++++- 1 file changed, 558 insertions(+), 2 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index f3b03a4935..35591319b5 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -2,7 +2,6 @@ import { DeepnoteFile, serializeDeepnoteFile } from '@deepnote/blocks'; import { assert } from 'chai'; import { mkdtempSync, rmSync } from 'fs'; import { tmpdir } from 'os'; -import { join } from 'path'; import * as sinon from 'sinon'; import { anything, instance, mock, reset, resetCalls, verify, when } from 'ts-mockito'; import { @@ -14,11 +13,12 @@ import { NotebookEdit, Uri } from 'vscode'; +import { join } from '../../platform/vscode-path/path'; -import type { IControllerRegistration } from '../controllers/types'; import type { IDisposableRegistry } from '../../platform/common/types'; import type { DeepnoteOutput, DeepnoteProject } from '../../platform/deepnote/deepnoteTypes'; import { mockedVSCodeNamespaces, resetVSCodeMocks } from '../../test/vscode-mock'; +import type { IControllerRegistration } from '../controllers/types'; import { IDeepnoteNotebookManager } from '../types'; import { DeepnoteFileChangeWatcher } from './deepnoteFileChangeWatcher'; import { SnapshotService } from './snapshots/snapshotService'; @@ -101,6 +101,12 @@ interface NotebookEditCapture { cellSourceJoined: string; } +interface SnapshotInteractionCapture { + cellSourcesJoined: string; + outputPlainJoined: string; + uriKey: string; +} + /** * Polls until a condition is met or a timeout is reached. */ @@ -1356,6 +1362,556 @@ project: fbOnDidChange.dispose(); fbOnDidCreate.dispose(); }); + + suite('snapshot and deserialization interaction', () => { + let interactionCaptures: SnapshotInteractionCapture[]; + let snapshotApplyEditStub: sinon.SinonStub; + + setup(function () { + this.timeout(12_000); + interactionCaptures = []; + + reset(mockedNotebookManager); + when(mockedNotebookManager.consumePendingNotebookResolution(anything())).thenReturn(undefined); + when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(validProject); + when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); + when(mockedNotebookManager.queueNotebookResolution(anything(), anything())).thenReturn(); + when(mockedNotebookManager.updateOriginalProject(anything(), anything())).thenReturn(); + resetCalls(mockedNotebookManager); + + snapshotApplyEditStub = sinon + .stub(snapshotWatcher, 'applyNotebookEdits' as keyof DeepnoteFileChangeWatcher) + .callsFake(async function (this: DeepnoteFileChangeWatcher, ...args: unknown[]) { + const uri = args[0] as Uri; + const edits = args[1] as NotebookEdit[]; + + const replaceCellsEdit = edits.find((e) => (e as { newCells?: unknown[] }).newCells?.length); + if (replaceCellsEdit) { + const newCells = ( + replaceCellsEdit as { + newCells: Array<{ + outputs?: Array<{ items: Array<{ data?: Uint8Array }> }>; + value: string; + }>; + } + ).newCells; + const outputPlainJoined = newCells + .map((c) => { + const data = c.outputs?.[0]?.items?.[0]?.data; + + return data ? new TextDecoder().decode(data) : ''; + }) + .filter(Boolean) + .join(';'); + + interactionCaptures.push({ + uriKey: uri.toString(), + cellSourcesJoined: newCells.map((c) => c.value).join('\n'), + outputPlainJoined + }); + } + + return DeepnoteFileChangeWatcher.prototype.applyNotebookEdits.apply(this, [ + uri, + edits + ] as never); + }); + }); + + teardown(() => { + snapshotApplyEditStub.restore(); + }); + + test('snapshot change with multi-notebook project applies only matching block outputs per notebook', async () => { + when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(multiNotebookProject); + + const multiOutputs = new Map([ + [ + 'block-nb1', + [ + { + output_type: 'execute_result', + data: { 'text/plain': 'OutputForNb1Only' }, + execution_count: 1 + } as DeepnoteOutput + ] + ], + [ + 'block-nb2', + [ + { + output_type: 'execute_result', + data: { 'text/plain': 'OutputForNb2Only' }, + execution_count: 1 + } as DeepnoteOutput + ] + ] + ]); + when(mockSnapshotService.readSnapshot(anything())).thenCall(() => { + readSnapshotCallCount++; + + return Promise.resolve(multiOutputs); + }); + + const basePath = testFileUri('multi-snap.deepnote'); + const uriNb1 = basePath.with({ query: 'view=1' }); + const uriNb2 = basePath.with({ query: 'view=2' }); + + const notebook1 = createMockNotebook({ + uri: uriNb1, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-1' + }, + cells: [ + { + metadata: { id: 'block-nb1', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb1-old")', languageId: 'python' } + } + ] + }); + + const notebook2 = createMockNotebook({ + uri: uriNb2, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-2' + }, + cells: [ + { + metadata: { id: 'block-nb2', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb2-old")', languageId: 'python' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook1, notebook2]); + + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); + snapshotOnDidChange.fire(snapshotUri); + + await waitFor(() => snapshotApplyEditCount >= 4); + + const byUri = new Map(interactionCaptures.map((c) => [c.uriKey, c])); + + assert.include(byUri.get(uriNb1.toString())?.outputPlainJoined ?? '', 'OutputForNb1Only'); + assert.notInclude(byUri.get(uriNb1.toString())?.outputPlainJoined ?? '', 'OutputForNb2Only'); + + assert.include(byUri.get(uriNb2.toString())?.outputPlainJoined ?? '', 'OutputForNb2Only'); + assert.notInclude(byUri.get(uriNb2.toString())?.outputPlainJoined ?? '', 'OutputForNb1Only'); + }); + + test('main file change after snapshot update deserializes updated cell source', async function () { + this.timeout(10_000); + const notebookUri = testFileUri('after-snap.deepnote'); + const notebook = createMockNotebook({ + uri: notebookUri, + cells: [ + { + metadata: { id: 'block-1', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("hello")', languageId: 'python' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); + + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); + when(mockSnapshotService.readSnapshot(anything())).thenCall(() => { + readSnapshotCallCount++; + + return Promise.resolve(snapshotOutputs); + }); + + snapshotOnDidChange.fire(snapshotUri); + await waitFor(() => snapshotApplyEditCount >= 2); + + const changedYaml = ` +version: '1.0.0' +metadata: + createdAt: '2025-01-01T00:00:00Z' +project: + id: project-1 + name: Test Project + notebooks: + - id: notebook-1 + name: Notebook 1 + blocks: + - id: block-1 + type: code + sortingKey: a0 + blockGroup: '1' + content: print("after-snapshot-main-sync") +`; + + const mockFs = mock(); + when(mockFs.readFile(anything())).thenCall(() => { + return Promise.resolve(new TextEncoder().encode(changedYaml)); + }); + when(mockFs.writeFile(anything(), anything())).thenReturn(Promise.resolve()); + when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); + + // Snapshot save marks self-write; first FS event consumes it without reloading. + snapshotOnDidChange.fire(notebookUri); + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + + snapshotOnDidChange.fire(notebookUri); + + await waitFor(() => + interactionCaptures.some((c) => c.cellSourcesJoined.includes('after-snapshot-main-sync')) + ); + + const mainSyncCapture = interactionCaptures.find((c) => + c.cellSourcesJoined.includes('after-snapshot-main-sync') + ); + + assert.isDefined(mainSyncCapture); + assert.include( + mainSyncCapture!.cellSourcesJoined, + 'after-snapshot-main-sync', + 'main-file sync should deserialize new source after snapshot outputs were applied' + ); + }); + + test('snapshot save self-write is consumed once then external main-file change applies', async function () { + this.timeout(10_000); + const baseUri = testFileUri('snap-selfwrite.deepnote'); + const notebook = createMockNotebook({ + uri: baseUri, + cells: [ + { + metadata: { id: 'block-1', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("hello")', languageId: 'python' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); + + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); + snapshotOnDidChange.fire(snapshotUri); + await waitFor(() => snapshotSaveCount >= 1); + + const editsBefore = snapshotApplyEditCount; + + snapshotOnDidChange.fire(baseUri); + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + + assert.strictEqual( + snapshotApplyEditCount, + editsBefore, + 'first main-file FS event after snapshot save should be consumed as self-write' + ); + + const externalYaml = ` +version: '1.0.0' +metadata: + createdAt: '2025-01-01T00:00:00Z' +project: + id: project-1 + name: Test Project + notebooks: + - id: notebook-1 + name: Notebook 1 + blocks: + - id: block-1 + type: code + sortingKey: a0 + blockGroup: '1' + content: print("external-after-self-write") +`; + + const mockFs = mock(); + when(mockFs.readFile(anything())).thenCall(() => { + return Promise.resolve(new TextEncoder().encode(externalYaml)); + }); + when(mockFs.writeFile(anything(), anything())).thenReturn(Promise.resolve()); + when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); + + snapshotOnDidChange.fire(baseUri); + + await waitFor(() => + interactionCaptures.some((c) => c.cellSourcesJoined.includes('external-after-self-write')) + ); + + assert.isTrue( + interactionCaptures.some((c) => c.cellSourcesJoined.includes('external-after-self-write')), + 'second main-file change should deserialize external content' + ); + }); + + test('main-file sync runs after in-flight snapshot when both are triggered close together', async function () { + this.timeout(12_000); + let releaseSnapshot!: () => void; + const snapshotGate = new Promise((resolve) => { + releaseSnapshot = resolve; + }); + + when(mockSnapshotService.readSnapshot(anything())).thenCall(() => { + readSnapshotCallCount++; + + return snapshotGate.then(() => snapshotOutputs); + }); + + const baseUri = testFileUri('coalesce.deepnote'); + const notebook = createMockNotebook({ + uri: baseUri, + cells: [ + { + metadata: { id: 'block-1', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("hello")', languageId: 'python' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); + + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); + snapshotOnDidChange.fire(snapshotUri); + + await waitFor(() => readSnapshotCallCount >= 1); + + const coalescedYaml = ` +version: '1.0.0' +metadata: + createdAt: '2025-01-01T00:00:00Z' +project: + id: project-1 + name: Test Project + notebooks: + - id: notebook-1 + name: Notebook 1 + blocks: + - id: block-1 + type: code + sortingKey: a0 + blockGroup: '1' + content: print("main-wins-after-snapshot") +`; + + const mockFs = mock(); + when(mockFs.readFile(anything())).thenCall(() => { + return Promise.resolve(new TextEncoder().encode(coalescedYaml)); + }); + when(mockFs.writeFile(anything(), anything())).thenReturn(Promise.resolve()); + when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); + + snapshotOnDidChange.fire(baseUri); + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + + releaseSnapshot(); + + await waitFor(() => + interactionCaptures.some((c) => c.cellSourcesJoined.includes('main-wins-after-snapshot')) + ); + + assert.isTrue( + interactionCaptures.some((c) => c.cellSourcesJoined.includes('main-wins-after-snapshot')), + 'main-file sync should deserialize disk YAML after snapshot operation completes' + ); + }); + + test('multi-notebook: snapshot outputs then external YAML update keeps per-notebook sources', async function () { + this.timeout(12_000); + when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(multiNotebookProject); + + const multiOutputs = new Map([ + [ + 'block-nb1', + [ + { + output_type: 'execute_result', + data: { 'text/plain': 'SnapNb1' }, + execution_count: 1 + } as DeepnoteOutput + ] + ], + [ + 'block-nb2', + [ + { + output_type: 'execute_result', + data: { 'text/plain': 'SnapNb2' }, + execution_count: 1 + } as DeepnoteOutput + ] + ] + ]); + when(mockSnapshotService.readSnapshot(anything())).thenCall(() => { + readSnapshotCallCount++; + + return Promise.resolve(multiOutputs); + }); + + const basePath = testFileUri('multi-snap-then-yaml.deepnote'); + const uriNb1 = basePath.with({ query: 'view=1' }); + const uriNb2 = basePath.with({ query: 'view=2' }); + + const notebook1 = createMockNotebook({ + uri: uriNb1, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-1' + }, + cells: [ + { + metadata: { id: 'block-nb1', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb1-old")', languageId: 'python' } + } + ] + }); + + const notebook2 = createMockNotebook({ + uri: uriNb2, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-2' + }, + cells: [ + { + metadata: { id: 'block-nb2', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb2-old")', languageId: 'python' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook1, notebook2]); + + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); + snapshotOnDidChange.fire(snapshotUri); + await waitFor(() => snapshotApplyEditCount >= 4); + + const round2Project = structuredClone(multiNotebookProject); + round2Project.project.notebooks[0].blocks![0].content = 'print("nb1-round2")'; + round2Project.project.notebooks[1].blocks![0].content = 'print("nb2-round2")'; + const yamlRound2 = serializeDeepnoteFile(round2Project); + + const mockFs = mock(); + when(mockFs.readFile(anything())).thenCall(() => { + return Promise.resolve(new TextEncoder().encode(yamlRound2)); + }); + when(mockFs.writeFile(anything(), anything())).thenReturn(Promise.resolve()); + when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); + + // Two snapshot saves increment self-write count to 2 for the shared base file URI. + snapshotOnDidChange.fire(basePath); + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + snapshotOnDidChange.fire(basePath); + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + + snapshotOnDidChange.fire(basePath); + + await waitFor(() => + interactionCaptures.some( + (c) => c.uriKey === uriNb1.toString() && c.cellSourcesJoined.includes('nb1-round2') + ) + ); + + assert.isTrue( + interactionCaptures.some( + (c) => c.uriKey === uriNb1.toString() && c.outputPlainJoined.includes('SnapNb1') + ), + 'snapshot phase should apply SnapNb1 output to notebook-1' + ); + assert.isTrue( + interactionCaptures.some( + (c) => c.uriKey === uriNb2.toString() && c.outputPlainJoined.includes('SnapNb2') + ), + 'snapshot phase should apply SnapNb2 output to notebook-2' + ); + + const nb1Main = interactionCaptures.find( + (c) => c.uriKey === uriNb1.toString() && c.cellSourcesJoined.includes('nb1-round2') + ); + const nb2Main = interactionCaptures.find( + (c) => c.uriKey === uriNb2.toString() && c.cellSourcesJoined.includes('nb2-round2') + ); + + assert.isDefined(nb1Main); + assert.include(nb1Main!.cellSourcesJoined, 'nb1-round2'); + assert.notInclude(nb1Main!.cellSourcesJoined, 'nb2-round2'); + + assert.isDefined(nb2Main); + assert.include(nb2Main!.cellSourcesJoined, 'nb2-round2'); + assert.notInclude(nb2Main!.cellSourcesJoined, 'nb1-round2'); + }); + + test('snapshot outputs for sibling notebook blocks do not leak into a single open notebook', async () => { + when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(multiNotebookProject); + + const multiOutputs = new Map([ + [ + 'block-nb1', + [ + { + output_type: 'execute_result', + data: { 'text/plain': 'OnlyNb1' }, + execution_count: 1 + } as DeepnoteOutput + ] + ], + [ + 'block-nb2', + [ + { + output_type: 'execute_result', + data: { 'text/plain': 'LeakIfApplied' }, + execution_count: 1 + } as DeepnoteOutput + ] + ] + ]); + when(mockSnapshotService.readSnapshot(anything())).thenCall(() => { + readSnapshotCallCount++; + + return Promise.resolve(multiOutputs); + }); + + const uriNb1 = testFileUri('only-nb1.deepnote'); + const notebook1Only = createMockNotebook({ + uri: uriNb1, + metadata: { + deepnoteProjectId: 'project-1', + deepnoteNotebookId: 'notebook-1' + }, + cells: [ + { + metadata: { id: 'block-nb1', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("nb1-only")', languageId: 'python' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook1Only]); + + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); + snapshotOnDidChange.fire(snapshotUri); + + await waitFor(() => snapshotApplyEditCount >= 2); + + const cap = interactionCaptures.find((c) => c.uriKey === uriNb1.toString()); + + assert.isDefined(cap); + assert.include(cap!.outputPlainJoined, 'OnlyNb1'); + assert.notInclude(cap!.outputPlainJoined, 'LeakIfApplied'); + }); + }); }); suite('multi-notebook file sync', () => { From 5d9d6e2f6d3d0fcd15237f81367c7984da745e6b Mon Sep 17 00:00:00 2001 From: tomas Date: Wed, 1 Apr 2026 08:35:15 +0000 Subject: [PATCH 20/35] Fix cspell --- src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index 35591319b5..2284e75304 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -1581,7 +1581,7 @@ project: test('snapshot save self-write is consumed once then external main-file change applies', async function () { this.timeout(10_000); - const baseUri = testFileUri('snap-selfwrite.deepnote'); + const baseUri = testFileUri('snap-self-write.deepnote'); const notebook = createMockNotebook({ uri: baseUri, cells: [ From 496261623b261481bbbae5228f3c82956a564893 Mon Sep 17 00:00:00 2001 From: tomas Date: Wed, 1 Apr 2026 08:44:18 +0000 Subject: [PATCH 21/35] Fix tests --- .../deepnote/deepnoteFileChangeWatcher.ts | 2 +- .../deepnoteFileChangeWatcher.unit.test.ts | 70 +++++++++---------- .../deepnote/deepnoteSerializer.unit.test.ts | 2 + 3 files changed, 37 insertions(+), 37 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index e413200a12..940ae8a3ec 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -131,7 +131,7 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic } } - protected async applyNotebookEdits(uri: Uri, edits: NotebookEdit[]): Promise { + public async applyNotebookEdits(uri: Uri, edits: NotebookEdit[]): Promise { const wsEdit = new WorkspaceEdit(); wsEdit.set(uri, edits); diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index 2284e75304..47fe1668d8 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -1379,43 +1379,41 @@ project: when(mockedNotebookManager.updateOriginalProject(anything(), anything())).thenReturn(); resetCalls(mockedNotebookManager); - snapshotApplyEditStub = sinon - .stub(snapshotWatcher, 'applyNotebookEdits' as keyof DeepnoteFileChangeWatcher) - .callsFake(async function (this: DeepnoteFileChangeWatcher, ...args: unknown[]) { - const uri = args[0] as Uri; - const edits = args[1] as NotebookEdit[]; - - const replaceCellsEdit = edits.find((e) => (e as { newCells?: unknown[] }).newCells?.length); - if (replaceCellsEdit) { - const newCells = ( - replaceCellsEdit as { - newCells: Array<{ - outputs?: Array<{ items: Array<{ data?: Uint8Array }> }>; - value: string; - }>; - } - ).newCells; - const outputPlainJoined = newCells - .map((c) => { - const data = c.outputs?.[0]?.items?.[0]?.data; - - return data ? new TextDecoder().decode(data) : ''; - }) - .filter(Boolean) - .join(';'); - - interactionCaptures.push({ - uriKey: uri.toString(), - cellSourcesJoined: newCells.map((c) => c.value).join('\n'), - outputPlainJoined - }); - } + snapshotApplyEditStub = sinon.stub(snapshotWatcher, 'applyNotebookEdits').callsFake(async function ( + this: DeepnoteFileChangeWatcher, + ...args: unknown[] + ) { + const uri = args[0] as Uri; + const edits = args[1] as NotebookEdit[]; + + const replaceCellsEdit = edits.find((e) => (e as { newCells?: unknown[] }).newCells?.length); + if (replaceCellsEdit) { + const newCells = ( + replaceCellsEdit as { + newCells: Array<{ + outputs?: Array<{ items: Array<{ data?: Uint8Array }> }>; + value: string; + }>; + } + ).newCells; + const outputPlainJoined = newCells + .map((c) => { + const data = c.outputs?.[0]?.items?.[0]?.data; + + return data ? new TextDecoder().decode(data) : ''; + }) + .filter(Boolean) + .join(';'); + + interactionCaptures.push({ + uriKey: uri.toString(), + cellSourcesJoined: newCells.map((c) => c.value).join('\n'), + outputPlainJoined + }); + } - return DeepnoteFileChangeWatcher.prototype.applyNotebookEdits.apply(this, [ - uri, - edits - ] as never); - }); + return DeepnoteFileChangeWatcher.prototype.applyNotebookEdits.apply(this, [uri, edits]); + }); }); teardown(() => { diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index baa5e49591..4cde84ebb5 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -274,6 +274,8 @@ project: const result = await serializer.serializeNotebook(mockNotebookData as any, {} as any); const serializedProject = deserializeDeepnoteFile(new TextDecoder().decode(result)); + assert.strictEqual(serializedProject.project.notebooks[0].id, 'notebook-1'); + assert.strictEqual(serializedProject.project.notebooks[1].id, 'notebook-2'); assert.strictEqual(serializedProject.project.notebooks[0].blocks?.[0].content, 'print("hello")'); assert.strictEqual(serializedProject.project.notebooks[1].blocks?.[0].content, '# Updated second notebook'); }); From 552366b28e7558d468f0a5e22bcd66f274f0c150 Mon Sep 17 00:00:00 2001 From: tomas Date: Wed, 1 Apr 2026 09:43:26 +0000 Subject: [PATCH 22/35] Minor improvements --- .../deepnote/deepnoteFileChangeWatcher.unit.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index 47fe1668d8..c57c398616 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -136,7 +136,7 @@ suite('DeepnoteFileChangeWatcher', () => { }); function testFileUri(...pathSegments: string[]): Uri { - return Uri.file(join(testFixturesDir, ...pathSegments)); + return Uri.joinPath(Uri.file(testFixturesDir), ...pathSegments); } let watcher: DeepnoteFileChangeWatcher; @@ -1367,8 +1367,7 @@ project: let interactionCaptures: SnapshotInteractionCapture[]; let snapshotApplyEditStub: sinon.SinonStub; - setup(function () { - this.timeout(12_000); + setup(() => { interactionCaptures = []; reset(mockedNotebookManager); From bd4d5fd51fb497880f210e845d5f9cf98d9b65fa Mon Sep 17 00:00:00 2001 From: tomas Date: Wed, 1 Apr 2026 14:04:11 +0000 Subject: [PATCH 23/35] fix(deepnote): Enhance error handling for metadata restoration in DeepnoteFileChangeWatcher - Updated `applyNotebookEdits` calls to handle failures in restoring block IDs, logging warnings when restoration fails after execution API and replaceCells operations. - Added unit tests to verify that saving does not occur when metadata restoration fails, ensuring data integrity during notebook edits. - Improved test coverage for scenarios involving metadata restoration failures, enhancing the reliability of the DeepnoteFileChangeWatcher functionality. --- .../deepnote/deepnoteFileChangeWatcher.ts | 14 +- .../deepnoteFileChangeWatcher.unit.test.ts | 161 ++++++++++++++++++ 2 files changed, 173 insertions(+), 2 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index 940ae8a3ec..efa916b7fe 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -507,7 +507,13 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic } } if (metadataEdits.length > 0) { - await this.applyNotebookEdits(notebook.uri, metadataEdits); + const metadataApplied = await this.applyNotebookEdits(notebook.uri, metadataEdits); + if (!metadataApplied) { + logger.warn( + `[FileChangeWatcher] Failed to restore block IDs via execution path: ${notebook.uri.path}` + ); + return; + } } logger.info(`[FileChangeWatcher] Updated notebook outputs via execution API: ${notebook.uri.path}`); @@ -554,7 +560,11 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic }) ); } - await this.applyNotebookEdits(notebook.uri, metadataEdits); + const metadataApplied = await this.applyNotebookEdits(notebook.uri, metadataEdits); + if (!metadataApplied) { + logger.warn(`[FileChangeWatcher] Failed to restore block IDs after replaceCells: ${notebook.uri.path}`); + return; + } // Save to sync mtime — mark as self-write first this.markSelfWrite(notebook.uri); diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index c57c398616..f14a8998f3 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -14,6 +14,7 @@ import { Uri } from 'vscode'; import { join } from '../../platform/vscode-path/path'; +import { logger } from '../../platform/logging'; import type { IDisposableRegistry } from '../../platform/common/types'; import type { DeepnoteOutput, DeepnoteProject } from '../../platform/deepnote/deepnoteTypes'; @@ -1363,6 +1364,166 @@ project: fbOnDidCreate.dispose(); }); + test('should not save when metadata restore fails after replaceCells fallback', async () => { + const mdDisposables: IDisposableRegistry = []; + const mdOnDidChange = new EventEmitter(); + const mdOnDidCreate = new EventEmitter(); + const fsWatcherMd = mock(); + when(fsWatcherMd.onDidChange).thenReturn(mdOnDidChange.event); + when(fsWatcherMd.onDidCreate).thenReturn(mdOnDidCreate.event); + when(fsWatcherMd.dispose()).thenReturn(); + when(mockedVSCodeNamespaces.workspace.createFileSystemWatcher(anything())).thenReturn( + instance(fsWatcherMd) + ); + + let mdApplyEditInvocation = 0; + let mdSaveCount = 0; + when(mockedVSCodeNamespaces.workspace.applyEdit(anything())).thenCall(() => { + mdApplyEditInvocation++; + return Promise.resolve(mdApplyEditInvocation === 1); + }); + when(mockedVSCodeNamespaces.workspace.save(anything())).thenCall((uri: Uri) => { + mdSaveCount++; + return Promise.resolve(uri); + }); + + const mockedControllerRegistration = mock(); + when(mockedControllerRegistration.getSelected(anything())).thenReturn(undefined); + + const mdWatcher = new DeepnoteFileChangeWatcher( + mdDisposables, + mockNotebookManager, + instance(mockSnapshotService), + instance(mockedControllerRegistration) + ); + mdWatcher.activate(); + + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); + const notebook = createMockNotebook({ + uri: testFileUri('metadata-fail-replace.deepnote'), + cells: [ + { + metadata: { id: 'block-1', type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("hello")' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); + + mdOnDidChange.fire(snapshotUri); + + await waitFor(() => mdApplyEditInvocation >= 2); + + assert.strictEqual( + mdApplyEditInvocation, + 2, + 'replaceCells and metadata restore should each invoke applyEdit once' + ); + assert.strictEqual( + mdSaveCount, + 0, + 'workspace.save must not run when metadata restore fails (would persist wrong cell IDs)' + ); + + for (const d of mdDisposables) { + d.dispose(); + } + mdOnDidChange.dispose(); + mdOnDidCreate.dispose(); + }); + + test('should warn and return when metadata restore fails after execution API with block ID fallback', async () => { + const warnStub = sinon.stub(logger, 'warn'); + + try { + const exMdDisposables: IDisposableRegistry = []; + const exMdOnDidChange = new EventEmitter(); + const exMdOnDidCreate = new EventEmitter(); + const fsWatcherExMd = mock(); + when(fsWatcherExMd.onDidChange).thenReturn(exMdOnDidChange.event); + when(fsWatcherExMd.onDidCreate).thenReturn(exMdOnDidCreate.event); + when(fsWatcherExMd.dispose()).thenReturn(); + when(mockedVSCodeNamespaces.workspace.createFileSystemWatcher(anything())).thenReturn( + instance(fsWatcherExMd) + ); + + let exMdSaveCount = 0; + when(mockedVSCodeNamespaces.workspace.applyEdit(anything())).thenCall(() => Promise.resolve(false)); + when(mockedVSCodeNamespaces.workspace.save(anything())).thenCall((uri: Uri) => { + exMdSaveCount++; + return Promise.resolve(uri); + }); + + const mockVSCodeController = { + createNotebookCellExecution: () => ({ + start: () => {}, + replaceOutput: () => Promise.resolve(), + end: () => {} + }) + }; + const mockedControllerRegistration = mock(); + when(mockedControllerRegistration.getSelected(anything())).thenReturn({ + controller: mockVSCodeController + } as any); + + const mockedManagerEx = mock(); + when(mockedManagerEx.consumePendingNotebookResolution(anything())).thenReturn(undefined); + when(mockedManagerEx.getOriginalProject(anything())).thenReturn(validProject); + when(mockedManagerEx.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); + when(mockedManagerEx.queueNotebookResolution(anything(), anything())).thenReturn(); + when(mockedManagerEx.updateOriginalProject(anything(), anything())).thenReturn(); + + const exMdWatcher = new DeepnoteFileChangeWatcher( + exMdDisposables, + instance(mockedManagerEx), + instance(mockSnapshotService), + instance(mockedControllerRegistration) + ); + exMdWatcher.activate(); + + const snapshotUri = testFileUri('snapshots', 'my-project_project-1_latest.snapshot.deepnote'); + const notebook = createMockNotebook({ + uri: testFileUri('metadata-fail-exec.deepnote'), + cells: [ + { + metadata: { type: 'code' }, + outputs: [], + kind: NotebookCellKind.Code, + document: { getText: () => 'print("hello")' } + } + ] + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); + + exMdOnDidChange.fire(snapshotUri); + + await waitFor(() => warnStub.called); + + assert.include( + warnStub.firstCall.args[0] as string, + 'Failed to restore block IDs via execution path', + 'should log when metadata restore fails after execution API' + ); + assert.strictEqual( + exMdSaveCount, + 0, + 'execution API path should not save after failed metadata restore' + ); + + for (const d of exMdDisposables) { + d.dispose(); + } + exMdOnDidChange.dispose(); + exMdOnDidCreate.dispose(); + } finally { + warnStub.restore(); + } + }); + suite('snapshot and deserialization interaction', () => { let interactionCaptures: SnapshotInteractionCapture[]; let snapshotApplyEditStub: sinon.SinonStub; From ec96f534cd721402cfab7a84bb5b51a9e1f119a4 Mon Sep 17 00:00:00 2001 From: tomas Date: Thu, 2 Apr 2026 13:24:43 +0000 Subject: [PATCH 24/35] feat(deepnote): Add DeepnoteNotebookInfoStatusBar for displaying notebook details in the status bar - Introduced `DeepnoteNotebookInfoStatusBar` to show the active Deepnote notebook name and provide a copy action for notebook details. - Updated service registration in both `serviceRegistry.node.ts` and `serviceRegistry.web.ts` to include the new status bar service. - Added a new command `CopyNotebookDetails` to facilitate copying notebook information to the clipboard. --- .../deepnote/deepnoteNotebookInfoStatusBar.ts | 154 ++++++++++++++++++ src/notebooks/serviceRegistry.node.ts | 5 + src/notebooks/serviceRegistry.web.ts | 5 + src/platform/common/constants.ts | 1 + 4 files changed, 165 insertions(+) create mode 100644 src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts diff --git a/src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts b/src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts new file mode 100644 index 0000000000..3959cf6f91 --- /dev/null +++ b/src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts @@ -0,0 +1,154 @@ +import { inject, injectable } from 'inversify'; +import { + commands, + Disposable, + env, + l10n, + NotebookDocument, + StatusBarAlignment, + StatusBarItem, + window, + workspace +} from 'vscode'; + +import { DEEPNOTE_NOTEBOOK_TYPE } from '../../kernels/deepnote/types'; +import { IExtensionSyncActivationService } from '../../platform/activation/types'; +import { Commands } from '../../platform/common/constants'; +import { IDisposableRegistry } from '../../platform/common/types'; + +/** + * Shows the active Deepnote notebook name in the status bar; tooltip and copy action include full debug details (metadata and URI). + */ +@injectable() +export class DeepnoteNotebookInfoStatusBar implements IExtensionSyncActivationService, Disposable { + private readonly disposables: Disposable[] = []; + + private disposed = false; + + private statusBarItem: StatusBarItem | undefined; + + constructor(@inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry) { + disposableRegistry.push(this); + } + + public activate(): void { + this.statusBarItem = window.createStatusBarItem('deepnote.notebookInfo', StatusBarAlignment.Left, 99); + this.statusBarItem.name = l10n.t('Deepnote Notebook Info'); + this.statusBarItem.command = Commands.CopyNotebookDetails; + this.disposables.push(this.statusBarItem); + + this.disposables.push( + commands.registerCommand(Commands.CopyNotebookDetails, () => { + this.copyActiveNotebookDetails(); + }) + ); + + this.disposables.push(window.onDidChangeActiveNotebookEditor(() => this.updateStatusBar())); + + this.disposables.push( + workspace.onDidChangeNotebookDocument((e) => { + if (e.notebook === window.activeNotebookEditor?.notebook) { + this.updateStatusBar(); + } + }) + ); + + this.updateStatusBar(); + } + + public dispose(): void { + if (this.disposed) { + return; + } + + this.disposed = true; + + while (this.disposables.length) { + const disposable = this.disposables.pop(); + + try { + disposable?.dispose(); + } catch { + // ignore + } + } + } + + private copyActiveNotebookDetails(): void { + const notebook = window.activeNotebookEditor?.notebook; + + if (!notebook) { + return; + } + + const info = this.getNotebookDebugInfo(notebook); + + if (!info) { + return; + } + + void env.clipboard.writeText(info.detailsText); + void window.showInformationMessage(l10n.t('Copied notebook details to clipboard.')); + } + + private getNotebookDebugInfo(notebook: NotebookDocument): { detailsText: string; displayName: string } | undefined { + if (notebook.notebookType !== DEEPNOTE_NOTEBOOK_TYPE) { + return undefined; + } + + const metadata = notebook.metadata as { + deepnoteNotebookId?: string; + deepnoteNotebookName?: string; + deepnoteProjectId?: string; + deepnoteProjectName?: string; + deepnoteVersion?: string; + name?: string; + }; + + const displayName = metadata.deepnoteNotebookName ?? metadata.name ?? l10n.t('Deepnote notebook'); + const uriString = notebook.uri.toString(true); + + const lines: string[] = [ + l10n.t('Notebook: {0}', displayName), + l10n.t('Notebook ID: {0}', metadata.deepnoteNotebookId ?? l10n.t('(unknown)')), + l10n.t('Project: {0}', metadata.deepnoteProjectName ?? l10n.t('(unknown)')), + l10n.t('Project ID: {0}', metadata.deepnoteProjectId ?? l10n.t('(unknown)')) + ]; + + if (metadata.deepnoteVersion !== undefined) { + lines.push(l10n.t('Deepnote version: {0}', String(metadata.deepnoteVersion))); + } + + lines.push(l10n.t('URI: {0}', uriString)); + + return { detailsText: lines.join('\n'), displayName }; + } + + private updateStatusBar(): void { + const item = this.statusBarItem; + + if (!item) { + return; + } + + const editor = window.activeNotebookEditor; + + if (!editor) { + item.hide(); + + return; + } + + const info = this.getNotebookDebugInfo(editor.notebook); + + if (!info) { + item.hide(); + + return; + } + + item.text = `$(notebook) ${info.displayName}`; + item.tooltip = [info.detailsText, l10n.t('Click to copy details')].join('\n'); + item.show(); + } +} diff --git a/src/notebooks/serviceRegistry.node.ts b/src/notebooks/serviceRegistry.node.ts index cbd8b860fe..f4ea82b469 100644 --- a/src/notebooks/serviceRegistry.node.ts +++ b/src/notebooks/serviceRegistry.node.ts @@ -41,6 +41,7 @@ import { NotebookTracebackFormatter } from './outputs/tracebackFormatter'; import { InterpreterPackageTracker } from './telemetry/interpreterPackageTracker.node'; import { INotebookEditorProvider, INotebookPythonEnvironmentService } from './types'; import { DeepnoteActivationService } from './deepnote/deepnoteActivationService'; +import { DeepnoteNotebookInfoStatusBar } from './deepnote/deepnoteNotebookInfoStatusBar'; import { DeepnoteNotebookManager } from './deepnote/deepnoteNotebookManager'; import { IDeepnoteNotebookManager } from './types'; import { IntegrationStorage } from '../platform/notebooks/deepnote/integrationStorage'; @@ -169,6 +170,10 @@ export function registerTypes(serviceManager: IServiceManager, isDevMode: boolea IExtensionSyncActivationService, DeepnoteActivationService ); + serviceManager.addSingleton( + IExtensionSyncActivationService, + DeepnoteNotebookInfoStatusBar + ); serviceManager.addSingleton( IExtensionSyncActivationService, DeepnoteNotebookCommandListener diff --git a/src/notebooks/serviceRegistry.web.ts b/src/notebooks/serviceRegistry.web.ts index 2488ff73d7..51ea5e2b88 100644 --- a/src/notebooks/serviceRegistry.web.ts +++ b/src/notebooks/serviceRegistry.web.ts @@ -36,6 +36,7 @@ import { CellOutputMimeTypeTracker } from './outputs/cellOutputMimeTypeTracker'; import { NotebookTracebackFormatter } from './outputs/tracebackFormatter'; import { INotebookEditorProvider, INotebookPythonEnvironmentService } from './types'; import { DeepnoteActivationService } from './deepnote/deepnoteActivationService'; +import { DeepnoteNotebookInfoStatusBar } from './deepnote/deepnoteNotebookInfoStatusBar'; import { DeepnoteNotebookManager } from './deepnote/deepnoteNotebookManager'; import { IDeepnoteNotebookManager } from './types'; import { DeepnoteNotebookCommandListener } from './deepnote/deepnoteNotebookCommandListener'; @@ -108,6 +109,10 @@ export function registerTypes(serviceManager: IServiceManager, isDevMode: boolea IExtensionSyncActivationService, DeepnoteActivationService ); + serviceManager.addSingleton( + IExtensionSyncActivationService, + DeepnoteNotebookInfoStatusBar + ); serviceManager.addSingleton( IExtensionSyncActivationService, DeepnoteNotebookCommandListener diff --git a/src/platform/common/constants.ts b/src/platform/common/constants.ts index c85719faec..d60321f1cf 100644 --- a/src/platform/common/constants.ts +++ b/src/platform/common/constants.ts @@ -223,6 +223,7 @@ export namespace Commands { export const OpenDeepnoteNotebook = 'deepnote.openNotebook'; export const OpenDeepnoteFile = 'deepnote.openFile'; export const RevealInDeepnoteExplorer = 'deepnote.revealInExplorer'; + export const CopyNotebookDetails = 'deepnote.copyNotebookDetails'; export const EnableSnapshots = 'deepnote.enableSnapshots'; export const DisableSnapshots = 'deepnote.disableSnapshots'; export const ManageIntegrations = 'deepnote.manageIntegrations'; From d44c1aa3ad78b5de95470a6d8aa55825a38d9a24 Mon Sep 17 00:00:00 2001 From: tomas Date: Thu, 2 Apr 2026 14:04:33 +0000 Subject: [PATCH 25/35] feat(deepnote): Add command to copy active notebook details - Introduced a new command `deepnote.copyNotebookDetails` to allow users to copy details of the active Deepnote notebook. - Updated localization file to include the title for the new command, enhancing user experience with clear labeling. --- package.json | 6 ++++++ package.nls.json | 1 + 2 files changed, 7 insertions(+) diff --git a/package.json b/package.json index dfba8b252b..c9f71a0d14 100644 --- a/package.json +++ b/package.json @@ -93,6 +93,12 @@ "category": "Deepnote", "icon": "$(reveal)" }, + { + "command": "deepnote.copyNotebookDetails", + "title": "%deepnote.commands.copyNotebookDetails.title%", + "category": "Deepnote", + "icon": "$(copy)" + }, { "command": "deepnote.enableSnapshots", "title": "%deepnote.commands.enableSnapshots.title%", diff --git a/package.nls.json b/package.nls.json index 35ee95ae66..d13aed9b17 100644 --- a/package.nls.json +++ b/package.nls.json @@ -250,6 +250,7 @@ "deepnote.commands.openNotebook.title": "Open Notebook", "deepnote.commands.openFile.title": "Open File", "deepnote.commands.revealInExplorer.title": "Reveal in Explorer", + "deepnote.commands.copyNotebookDetails.title": "Copy Active Deepnote Notebook Details", "deepnote.commands.enableSnapshots.title": "Enable Snapshots", "deepnote.commands.disableSnapshots.title": "Disable Snapshots", "deepnote.commands.manageIntegrations.title": "Manage Integrations", From 7815f42a769c7828a02a789b8f90fd01d6945dc7 Mon Sep 17 00:00:00 2001 From: tomas Date: Thu, 2 Apr 2026 14:47:57 +0000 Subject: [PATCH 26/35] Add a failing test for external change rerender --- .../deepnoteFileChangeWatcher.unit.test.ts | 98 +++++++++++++++++-- 1 file changed, 88 insertions(+), 10 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index f14a8998f3..a4f4886baf 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -8,6 +8,7 @@ import { Disposable, EventEmitter, FileSystemWatcher, + NotebookCellData, NotebookCellKind, NotebookDocument, NotebookEdit, @@ -21,6 +22,7 @@ import type { DeepnoteOutput, DeepnoteProject } from '../../platform/deepnote/de import { mockedVSCodeNamespaces, resetVSCodeMocks } from '../../test/vscode-mock'; import type { IControllerRegistration } from '../controllers/types'; import { IDeepnoteNotebookManager } from '../types'; +import { DeepnoteDataConverter } from './deepnoteDataConverter'; import { DeepnoteFileChangeWatcher } from './deepnoteFileChangeWatcher'; import { SnapshotService } from './snapshots/snapshotService'; @@ -209,20 +211,26 @@ suite('DeepnoteFileChangeWatcher', () => { notebookType?: string; cellCount?: number; metadata?: Record; - cells?: Array<{ - metadata?: Record; - outputs: any[]; - kind?: number; - document?: { getText: () => string; languageId?: string }; - }>; + cells?: Array< + | { + metadata?: Record; + outputs: any[]; + kind?: number; + document?: { getText: () => string; languageId?: string }; + } + | NotebookCellData + >; }): NotebookDocument { const cells = (opts.cells ?? []).map((c) => ({ ...c, kind: c.kind ?? NotebookCellKind.Code, - document: { - getText: c.document?.getText ?? (() => ''), - languageId: c.document?.languageId ?? 'python' - } + document: + 'document' in c && c.document + ? { getText: c.document.getText ?? (() => ''), languageId: c.document.languageId ?? 'python' } + : { + getText: () => ('value' in c ? (c.value as string) : ''), + languageId: 'languageId' in c ? (c.languageId as string) : 'python' + } })); return { @@ -544,6 +552,76 @@ project: assert.isAtLeast(applyEditCount, 2, 'applyEdit should be called for both external changes'); }); + test('editor→external→editor→external: second external edit must reload (self-write leak regression)', async function () { + this.timeout(15_000); + const uri = testFileUri('self-write-leak.deepnote'); + + when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(multiNotebookProject); + + // Initial state: editor content matches disk — use the real converter + const converter = new DeepnoteDataConverter(); + const nb1 = multiNotebookProject.project.notebooks[0]; + const notebook = createMockNotebook({ + uri, + metadata: { deepnoteProjectId: multiNotebookProject.project.id, deepnoteNotebookId: nb1.id }, + cells: converter.convertBlocksToCells(nb1.blocks) + }); + + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); + setupMockFs(multiNotebookYaml); + + // Real VS Code behavior: workspace.save() fires onDidSaveNotebookDocument. + // executeMainFileSync calls markSelfWrite before workspace.save, AND the + // onDidSaveNotebookDocument handler also calls markSelfWrite — two marks + // for one FS event. This leaks a phantom self-write count. + when(mockedVSCodeNamespaces.workspace.save(anything())).thenCall((saveUri: Uri) => { + saveCount++; + onDidSaveNotebook.fire(notebook); + return Promise.resolve(saveUri); + }); + + // Step 1: editor edit → user saves notebook 1 + onDidSaveNotebook.fire(notebook); + onDidChangeFile.fire(uri); + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + assert.strictEqual(applyEditCount, 0, 'Step 1: editor save FS event should be suppressed'); + + // Step 2: first external edit → triggers executeMainFileSync (reload works) + const externalProject1 = structuredClone(multiNotebookProject); + externalProject1.project.notebooks[0].blocks![0].content = 'print("external-1")'; + setupMockFs(serializeDeepnoteFile(externalProject1)); + + onDidChangeFile.fire(uri); + await waitFor(() => saveCount >= 1); + assert.isAtLeast(applyEditCount, 1, 'Step 2: first external edit should reload'); + + const applyCountAfterFirstReload = applyEditCount; + + // Consume FS events from executeMainFileSync's writeFile + save + onDidChangeFile.fire(uri); + onDidChangeFile.fire(uri); + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + + // Step 3: editor edit → user saves again + onDidSaveNotebook.fire(notebook); + onDidChangeFile.fire(uri); + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs)); + + // Step 4: second external edit → should reload but phantom self-write blocks it + const externalProject2 = structuredClone(multiNotebookProject); + externalProject2.project.notebooks[0].blocks![0].content = 'print("external-2")'; + setupMockFs(serializeDeepnoteFile(externalProject2)); + + onDidChangeFile.fire(uri); + await new Promise((resolve) => setTimeout(resolve, debounceWaitMs + 500)); + + assert.isAbove( + applyEditCount, + applyCountAfterFirstReload, + 'Step 4: second external edit should reload, but phantom self-write from executeMainFileSync leaks and suppresses it' + ); + }); + test('should use atomic edit (single applyEdit for replaceCells + metadata)', async () => { const uri = testFileUri('test.deepnote'); const notebook = createMockNotebook({ uri, cellCount: 0 }); From 855f38c6b0614921d0d5a943976a913d1b5bc5c0 Mon Sep 17 00:00:00 2001 From: tomas Date: Thu, 2 Apr 2026 15:56:07 +0000 Subject: [PATCH 27/35] Refactor self write mark handling --- .../deepnote/deepnoteFileChangeWatcher.ts | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index efa916b7fe..1a5caf0459 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -387,16 +387,14 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic // Save to clear dirty state. VS Code serializes (same bytes) and sees the // mtime from our recent write, so no "content is newer" conflict. - this.markSelfWrite(fileUri); + // NOTE: onDidSaveNotebookDocument handles the self-write mark for this save. try { const saved = await workspace.save(notebook.uri); if (!saved) { - this.consumeSelfWrite(fileUri); logger.warn(`[FileChangeWatcher] Save after sync write returned undefined: ${notebook.uri.path}`); return; } } catch (saveError) { - this.consumeSelfWrite(fileUri); logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError); } } catch (serializeError) { @@ -566,14 +564,9 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic return; } - // Save to sync mtime — mark as self-write first - this.markSelfWrite(notebook.uri); - try { - await workspace.save(notebook.uri); - } catch (error) { - this.consumeSelfWrite(notebook.uri); - throw error; - } + // Save to sync mtime. + // NOTE: onDidSaveNotebookDocument handles the self-write mark for this save. + await workspace.save(notebook.uri); logger.info(`[FileChangeWatcher] Updated notebook outputs from external snapshot: ${notebook.uri.path}`); } @@ -645,8 +638,9 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic } /** - * Marks a URI as about to be written by us (workspace.save). - * Call before workspace.save() to prevent the resulting fs event from triggering a reload. + * Marks a URI as about to be written by us. + * For workspace.fs.writeFile(): call this before the write. + * For workspace.save(): do NOT call this — onDidSaveNotebookDocument handles it. */ private markSelfWrite(uri: Uri): void { const key = this.normalizeFileUri(uri); From f72b0fa4814d9e6e50f48dd5bcbf5f07f989f0e8 Mon Sep 17 00:00:00 2001 From: tomas Date: Thu, 2 Apr 2026 18:42:32 +0000 Subject: [PATCH 28/35] feat(deepnote): Enhance notebook ID resolution with tab-based logic - Added support for resolving notebook IDs from open tabs, improving the handling of notebook selections during reloads. - Introduced a new method `findNotebookIdsFromTabs` to extract notebook IDs from the current tab groups. - Updated `findCurrentNotebookId` to prioritize tab-based resolution when available, alongside existing resolution strategies. - Enhanced unit tests to cover various scenarios for tab-based resolution, ensuring robust functionality. --- src/notebooks/deepnote/deepnoteSerializer.ts | 51 ++++++++++++- .../deepnote/deepnoteSerializer.unit.test.ts | 75 ++++++++++++++++++- src/test/vscode-mock.ts | 1 + 3 files changed, 122 insertions(+), 5 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index a9a0c8491d..cddaf6a60e 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -1,7 +1,7 @@ import type { DeepnoteBlock, DeepnoteFile, DeepnoteSnapshot } from '@deepnote/blocks'; import { deserializeDeepnoteFile, isExecutableBlock, serializeDeepnoteSnapshot } from '@deepnote/blocks'; import { inject, injectable, optional } from 'inversify'; -import { l10n, workspace, type CancellationToken, type NotebookData, type NotebookSerializer } from 'vscode'; +import { l10n, window, workspace, type CancellationToken, type NotebookData, type NotebookSerializer } from 'vscode'; import { z } from 'zod'; import { computeHash } from '../../platform/common/crypto'; @@ -56,6 +56,8 @@ const recentSerializeTtlMs = 5_000; @injectable() export class DeepnoteNotebookSerializer implements NotebookSerializer { + private readonly consumedTabResolutions = new Map>(); + private converter = new DeepnoteDataConverter(); /** @@ -107,7 +109,8 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { } const projectId = deepnoteFile.project.id; - const resolvedNotebookId = notebookId ?? this.findCurrentNotebookId(projectId); + const projectNotebookIds = deepnoteFile.project.notebooks.map((nb) => nb.id); + const resolvedNotebookId = notebookId ?? this.findCurrentNotebookId(projectId, projectNotebookIds); logger.debug(`DeepnoteSerializer: Project ID: ${projectId}, Selected notebook ID: ${resolvedNotebookId}`); @@ -200,11 +203,13 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { /** * Finds the notebook ID to deserialize without relying on active-editor state. - * Prefers a pending resolution hint, then current/open-document state. + * Prefers a pending resolution hint, then tab-based resolution (for reload), + * then current/open-document state. * @param projectId The project ID to find a notebook for + * @param projectNotebookIds Optional list of notebook IDs in the project, enables tab-based resolution * @returns The notebook ID to deserialize, or undefined if none found */ - findCurrentNotebookId(projectId: string): string | undefined { + findCurrentNotebookId(projectId: string, projectNotebookIds?: string[]): string | undefined { const pendingNotebookId = this.notebookManager.consumePendingNotebookResolution(projectId); const openNotebookIds = this.findOpenNotebookIds(projectId); const currentNotebookId = this.notebookManager.getCurrentNotebookId(projectId); @@ -213,6 +218,21 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return pendingNotebookId; } + if (projectNotebookIds && projectNotebookIds.length > 0) { + const tabNotebookIds = this.findNotebookIdsFromTabs(projectNotebookIds); + const consumedIds = this.consumedTabResolutions.get(projectId) ?? new Set(); + const remaining = tabNotebookIds.filter((id) => !consumedIds.has(id) && !openNotebookIds.includes(id)); + + if (remaining.length > 0) { + const consumed = this.consumedTabResolutions.get(projectId) ?? new Set(); + + consumed.add(remaining[0]); + this.consumedTabResolutions.set(projectId, consumed); + + return remaining[0]; + } + } + if (currentNotebookId && openNotebookIds.length === 0) { return currentNotebookId; } @@ -591,6 +611,29 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return false; } + private findNotebookIdsFromTabs(projectNotebookIds: string[]): string[] { + const projectIdSet = new Set(projectNotebookIds); + const notebookIds = new Set(); + + for (const group of window.tabGroups.all) { + for (const tab of group.tabs) { + const input = tab.input as { uri?: { query?: string }; notebookType?: string } | undefined; + + if (!input || !('uri' in input) || !('notebookType' in input) || input.notebookType !== 'deepnote') { + continue; + } + + const notebookId = new URLSearchParams(input.uri?.query ?? '').get('notebook'); + + if (notebookId && projectIdSet.has(notebookId)) { + notebookIds.add(notebookId); + } + } + } + + return [...notebookIds]; + } + private findOpenNotebookIds(projectId: string): string[] { return [ ...workspace.notebookDocuments.reduce((ids, doc) => { diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index 4cde84ebb5..919af1ff23 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -2,7 +2,7 @@ import { deserializeDeepnoteFile, serializeDeepnoteFile, type DeepnoteFile } fro import { assert } from 'chai'; import { parse as parseYaml } from 'yaml'; import { when } from 'ts-mockito'; -import type { NotebookDocument } from 'vscode'; +import { Uri, type NotebookDocument } from 'vscode'; import { DeepnoteNotebookSerializer } from './deepnoteSerializer'; import { DeepnoteNotebookManager } from './deepnoteNotebookManager'; @@ -430,6 +430,7 @@ project: // Reset only the specific mocks used in this suite when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn(undefined); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([]); + when(mockedVSCodeNamespaces.window.tabGroups).thenReturn({ all: [] } as any); }); test('should return queued notebook resolution when available', () => { @@ -705,6 +706,78 @@ project: assert.strictEqual(result, undefined); }); + + suite('tab-based resolution', () => { + function createTabGroups(...notebookIds: string[]) { + const tabs = notebookIds.map((id) => ({ + input: { + uri: Uri.parse(`file:///test/project.deepnote?notebook=${id}`), + notebookType: 'deepnote' + } + })); + + return { all: [{ tabs }] } as any; + } + + teardown(() => { + when(mockedVSCodeNamespaces.window.tabGroups).thenReturn({ all: [] } as any); + }); + + test('should resolve different notebook IDs from tabs on sequential reload calls', () => { + when(mockedVSCodeNamespaces.window.tabGroups).thenReturn(createTabGroups('notebook-1', 'notebook-2')); + + const projectNotebookIds = ['notebook-1', 'notebook-2']; + + const first = serializer.findCurrentNotebookId('project-123', projectNotebookIds); + const second = serializer.findCurrentNotebookId('project-123', projectNotebookIds); + + assert.strictEqual(first, 'notebook-1'); + assert.strictEqual(second, 'notebook-2'); + }); + + test('should skip tab resolution when all tab notebook IDs are already open', () => { + when(mockedVSCodeNamespaces.window.tabGroups).thenReturn(createTabGroups('notebook-1', 'notebook-2')); + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([ + createNotebookDoc('notebook-1'), + createNotebookDoc('notebook-2') + ]); + + const projectNotebookIds = ['notebook-1', 'notebook-2']; + const result = serializer.findCurrentNotebookId('project-123', projectNotebookIds); + + assert.strictEqual(result, undefined); + }); + + test('should filter tab notebook IDs by projectNotebookIds', () => { + when(mockedVSCodeNamespaces.window.tabGroups).thenReturn( + createTabGroups('notebook-1', 'other-project-notebook') + ); + + const projectNotebookIds = ['notebook-1', 'notebook-2']; + const result = serializer.findCurrentNotebookId('project-123', projectNotebookIds); + + assert.strictEqual(result, 'notebook-1'); + }); + + test('should prioritize pending resolution over tab resolution', () => { + when(mockedVSCodeNamespaces.window.tabGroups).thenReturn(createTabGroups('notebook-1', 'notebook-2')); + + manager.queueNotebookResolution('project-123', 'queued-notebook'); + + const projectNotebookIds = ['notebook-1', 'notebook-2']; + const result = serializer.findCurrentNotebookId('project-123', projectNotebookIds); + + assert.strictEqual(result, 'queued-notebook'); + }); + + test('should skip tab resolution when projectNotebookIds is not provided', () => { + when(mockedVSCodeNamespaces.window.tabGroups).thenReturn(createTabGroups('notebook-1', 'notebook-2')); + + const result = serializer.findCurrentNotebookId('project-123'); + + assert.strictEqual(result, undefined); + }); + }); }); suite('component integration', () => { diff --git a/src/test/vscode-mock.ts b/src/test/vscode-mock.ts index b6b0f5371f..7142c44d75 100644 --- a/src/test/vscode-mock.ts +++ b/src/test/vscode-mock.ts @@ -83,6 +83,7 @@ export function resetVSCodeMocks() { when(mockedVSCodeNamespaces.window.visibleNotebookEditors).thenReturn([]); when(mockedVSCodeNamespaces.window.activeTextEditor).thenReturn(undefined); when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn(undefined); + when(mockedVSCodeNamespaces.window.tabGroups).thenReturn({ all: [] } as any); // Window dialog methods with overloads (1-5 parameters) // showInformationMessage From 8e6f679a61c4227ba1381c7e822a0700bae4cb47 Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 7 Apr 2026 13:10:11 +0000 Subject: [PATCH 29/35] feat(deepnote): Refactor notebook selection handling and enhance mismatch resolution - Removed the `selectNotebookForProject` and `clearNotebookSelection` methods from `DeepnoteNotebookManager` to simplify notebook selection logic. - Introduced a new mechanism in `DeepnoteActivationService` to check and fix notebook ID mismatches when opening documents, improving the reliability of notebook loading. - Updated `DeepnoteSerializer` to prioritize current notebook IDs and handle active tab resolutions more effectively. - Enhanced unit tests to cover the new mismatch resolution logic and ensure proper functionality across various scenarios. --- build/mocha-esm-loader.js | 1 + .../deepnote/deepnoteActivationService.ts | 118 ++++++++++++- .../deepnote/deepnoteExplorerView.ts | 1 - .../deepnote/deepnoteFileChangeWatcher.ts | 3 - .../deepnoteFileChangeWatcher.unit.test.ts | 8 +- .../deepnote/deepnoteNotebookManager.ts | 38 ----- .../deepnoteNotebookManager.unit.test.ts | 160 +----------------- src/notebooks/deepnote/deepnoteSerializer.ts | 83 +++++---- .../deepnote/deepnoteSerializer.unit.test.ts | 125 +------------- src/notebooks/types.ts | 4 - src/test/mocks/vsc/extHostedTypes.ts | 10 ++ src/test/vscode-mock.ts | 1 + 12 files changed, 179 insertions(+), 373 deletions(-) diff --git a/build/mocha-esm-loader.js b/build/mocha-esm-loader.js index 7a45afa78b..f948d8aae6 100644 --- a/build/mocha-esm-loader.js +++ b/build/mocha-esm-loader.js @@ -335,6 +335,7 @@ export async function load(url, context, nextLoad) { export const EndOfLine = createClassProxy('EndOfLine'); export const PortAutoForwardAction = createClassProxy('PortAutoForwardAction'); export const PortAttributes = createClassProxy('PortAttributes'); + export const TabInputNotebook = createClassProxy('TabInputNotebook'); `, shortCircuit: true }; diff --git a/src/notebooks/deepnote/deepnoteActivationService.ts b/src/notebooks/deepnote/deepnoteActivationService.ts index 96c35288cd..8958ad7e22 100644 --- a/src/notebooks/deepnote/deepnoteActivationService.ts +++ b/src/notebooks/deepnote/deepnoteActivationService.ts @@ -1,5 +1,17 @@ import { inject, injectable, optional } from 'inversify'; -import { commands, l10n, workspace, window, type Disposable, type NotebookDocumentContentOptions } from 'vscode'; +import { + CancellationTokenSource, + commands, + l10n, + NotebookEdit, + NotebookRange, + workspace, + window, + WorkspaceEdit, + type Disposable, + type NotebookDocument, + type NotebookDocumentContentOptions +} from 'vscode'; import { IExtensionSyncActivationService } from '../../platform/activation/types'; import { IExtensionContext } from '../../platform/common/types'; @@ -15,6 +27,9 @@ import { SnapshotService } from './snapshots/snapshotService'; * Service responsible for activating and configuring Deepnote notebook support in VS Code. * Registers serializers, command handlers, and manages the notebook selection workflow. */ +const MISMATCH_CHECK_DELAY_MS = 200; +const MAX_MISMATCH_RETRIES = 10; + @injectable() export class DeepnoteActivationService implements IExtensionSyncActivationService { private editProtection: DeepnoteInputBlockEditProtection; @@ -23,6 +38,10 @@ export class DeepnoteActivationService implements IExtensionSyncActivationServic private integrationManager: IIntegrationManager; + private mismatchCheckTimer: ReturnType | undefined; + + private mismatchRetryCount = 0; + private serializer: DeepnoteNotebookSerializer; private serializerRegistration?: Disposable; @@ -49,6 +68,18 @@ export class DeepnoteActivationService implements IExtensionSyncActivationServic this.editProtection = new DeepnoteInputBlockEditProtection(this.logger); this.snapshotsEnabled = this.isSnapshotsEnabled(); + this.extensionContext.subscriptions.push( + workspace.onDidOpenNotebookDocument((doc) => { + if (doc.notebookType !== 'deepnote') { + return; + } + + if (new URLSearchParams(doc.uri.query).has('notebook')) { + this.scheduleMismatchCheck(); + } + }) + ); + this.registerSerializer(); this.extensionContext.subscriptions.push(this.editProtection); this.extensionContext.subscriptions.push( @@ -70,6 +101,80 @@ export class DeepnoteActivationService implements IExtensionSyncActivationServic this.integrationManager.activate(); } + /** + * Checks all open deepnote documents for URI/metadata notebook ID mismatches + * and fixes them by re-deserializing with the correct notebook ID. + * This handles the case where VS Code's deserializeNotebook API does not + * pass the document URI, causing the wrong notebook to be loaded on reload. + */ + private async checkAndFixMismatches(): Promise { + const hasLoadingDocs = workspace.notebookDocuments.some( + (doc) => + doc.notebookType === 'deepnote' && + !doc.metadata?.deepnoteNotebookId && + new URLSearchParams(doc.uri.query).has('notebook') + ); + + if (hasLoadingDocs && this.mismatchRetryCount < MAX_MISMATCH_RETRIES) { + this.mismatchRetryCount++; + this.scheduleMismatchCheck(); + + return; + } + + this.mismatchRetryCount = 0; + + for (const doc of workspace.notebookDocuments) { + if (doc.notebookType !== 'deepnote' || doc.isClosed) { + continue; + } + + const uriNotebookId = new URLSearchParams(doc.uri.query).get('notebook'); + const metadataNotebookId = doc.metadata?.deepnoteNotebookId as string | undefined; + + if (!uriNotebookId || uriNotebookId === metadataNotebookId) { + continue; + } + + await this.fixDocumentNotebook(doc, uriNotebookId); + } + } + + private async fixDocumentNotebook(doc: NotebookDocument, correctNotebookId: string): Promise { + const fileUri = doc.uri.with({ query: '', fragment: '' }); + + let content: Uint8Array; + try { + content = await workspace.fs.readFile(fileUri); + } catch { + this.logger.warn(`[DeepnoteActivation] Cannot read file for mismatch fix: ${fileUri.path}`); + + return; + } + + const cts = new CancellationTokenSource(); + try { + const data = await this.serializer.deserializeNotebook(content, cts.token, correctNotebookId); + + const wsEdit = new WorkspaceEdit(); + wsEdit.set(doc.uri, [ + NotebookEdit.replaceCells(new NotebookRange(0, doc.cellCount), data.cells), + NotebookEdit.updateNotebookMetadata(data.metadata!) + ]); + await workspace.applyEdit(wsEdit); + await doc.save(); + + this.logger.info( + `[DeepnoteActivation] Fixed notebook mismatch for ${doc.uri.path}: ` + + `loaded ${correctNotebookId} (was ${doc.metadata?.deepnoteNotebookId})` + ); + } catch (error) { + this.logger.error(`[DeepnoteActivation] Failed to fix notebook mismatch: ${doc.uri.path}`, error); + } finally { + cts.dispose(); + } + } + private isSnapshotsEnabled(): boolean { if (this.snapshotService) { return this.snapshotService.isSnapshotsEnabled(); @@ -130,4 +235,15 @@ export class DeepnoteActivationService implements IExtensionSyncActivationServic this.serializerRegistration = workspace.registerNotebookSerializer('deepnote', this.serializer, contentOptions); this.extensionContext.subscriptions.push(this.serializerRegistration); } + + private scheduleMismatchCheck(): void { + if (this.mismatchCheckTimer !== undefined) { + clearTimeout(this.mismatchCheckTimer); + } + + this.mismatchCheckTimer = setTimeout(() => { + this.mismatchCheckTimer = undefined; + void this.checkAndFixMismatches(); + }, MISMATCH_CHECK_DELAY_MS); + } } diff --git a/src/notebooks/deepnote/deepnoteExplorerView.ts b/src/notebooks/deepnote/deepnoteExplorerView.ts index 6d450401ba..9c41083e5f 100644 --- a/src/notebooks/deepnote/deepnoteExplorerView.ts +++ b/src/notebooks/deepnote/deepnoteExplorerView.ts @@ -519,7 +519,6 @@ export class DeepnoteExplorerView { private registerNotebookOpenIntent(projectId: string, notebookId: string): void { this.manager.queueNotebookResolution(projectId, notebookId); - this.manager.selectNotebookForProject(projectId, notebookId); } private refreshExplorer(): void { diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts index 1a5caf0459..8e54997f2e 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.ts @@ -292,9 +292,6 @@ export class DeepnoteFileChangeWatcher implements IExtensionSyncActivationServic return; } - // Pass the notebook ID explicitly to avoid mutating the global selection state. - // Multiple notebooks from the same project may be open simultaneously, and - // mutating selectedNotebookByProject would cause race conditions. const targetNotebookId = notebook.metadata?.deepnoteNotebookId as string | undefined; const tokenSource = new CancellationTokenSource(); diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index a4f4886baf..fedca12907 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -3,7 +3,7 @@ import { assert } from 'chai'; import { mkdtempSync, rmSync } from 'fs'; import { tmpdir } from 'os'; import * as sinon from 'sinon'; -import { anything, instance, mock, reset, resetCalls, verify, when } from 'ts-mockito'; +import { anything, instance, mock, reset, resetCalls, when } from 'ts-mockito'; import { Disposable, EventEmitter, @@ -164,7 +164,6 @@ suite('DeepnoteFileChangeWatcher', () => { mockedNotebookManager = mock(); when(mockedNotebookManager.consumePendingNotebookResolution(anything())).thenReturn(undefined); when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(validProject); - when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); when(mockedNotebookManager.queueNotebookResolution(anything(), anything())).thenReturn(); when(mockedNotebookManager.updateOriginalProject(anything(), anything())).thenReturn(); mockNotebookManager = instance(mockedNotebookManager); @@ -1550,7 +1549,6 @@ project: const mockedManagerEx = mock(); when(mockedManagerEx.consumePendingNotebookResolution(anything())).thenReturn(undefined); when(mockedManagerEx.getOriginalProject(anything())).thenReturn(validProject); - when(mockedManagerEx.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); when(mockedManagerEx.queueNotebookResolution(anything(), anything())).thenReturn(); when(mockedManagerEx.updateOriginalProject(anything(), anything())).thenReturn(); @@ -1612,7 +1610,6 @@ project: reset(mockedNotebookManager); when(mockedNotebookManager.consumePendingNotebookResolution(anything())).thenReturn(undefined); when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(validProject); - when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); when(mockedNotebookManager.queueNotebookResolution(anything(), anything())).thenReturn(); when(mockedNotebookManager.updateOriginalProject(anything(), anything())).thenReturn(); resetCalls(mockedNotebookManager); @@ -2157,7 +2154,6 @@ project: reset(mockedNotebookManager); when(mockedNotebookManager.consumePendingNotebookResolution(anything())).thenReturn(undefined); when(mockedNotebookManager.getOriginalProject(anything())).thenReturn(multiNotebookProject); - when(mockedNotebookManager.getTheSelectedNotebookForAProject(anything())).thenReturn('notebook-1'); when(mockedNotebookManager.queueNotebookResolution(anything(), anything())).thenReturn(); when(mockedNotebookManager.updateOriginalProject(anything(), anything())).thenReturn(); resetCalls(mockedNotebookManager); @@ -2278,8 +2274,6 @@ project: onDidChangeFile.fire(basePath); await waitFor(() => applyEditCount >= 2); - - verify(mockedNotebookManager.clearNotebookSelection(anything())).never(); }); test('should not corrupt other notebooks when one notebook triggers a file change', async () => { diff --git a/src/notebooks/deepnote/deepnoteNotebookManager.ts b/src/notebooks/deepnote/deepnoteNotebookManager.ts index 68360c7df4..b2e654e673 100644 --- a/src/notebooks/deepnote/deepnoteNotebookManager.ts +++ b/src/notebooks/deepnote/deepnoteNotebookManager.ts @@ -20,15 +20,6 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { private readonly originalProjects = new Map(); private readonly pendingNotebookResolutions = new Map(); private readonly projectsWithInitNotebookRun = new Set(); - private readonly selectedNotebookByProject = new Map(); - - /** - * Clears the remembered notebook selection and any pending resolution hints for a project. - */ - clearNotebookSelection(projectId: string): void { - this.pendingNotebookResolutions.delete(projectId); - this.selectedNotebookByProject.delete(projectId); - } /** * Consumes the next short-lived notebook resolution hint for a project. @@ -66,15 +57,6 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { return this.originalProjects.get(projectId); } - /** - * Gets the selected notebook ID for a specific project. - * @param projectId Project identifier - * @returns Selected notebook ID or undefined if not set - */ - getTheSelectedNotebookForAProject(projectId: string): string | undefined { - return this.selectedNotebookByProject.get(projectId); - } - /** * Queues a short-lived notebook resolution hint for the next deserialize. * @@ -92,16 +74,6 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { this.pendingNotebookResolutions.set(projectId, pendingResolutions); } - /** - * Associates a notebook ID with a project to remember the user's last explicit selection. - * - * @param projectId - The project ID that identifies the Deepnote project - * @param notebookId - The ID of the selected notebook within the project - */ - selectNotebookForProject(projectId: string, notebookId: string): void { - this.selectedNotebookByProject.set(projectId, notebookId); - } - /** * Stores the original project data and sets the initial current notebook. * This is used during deserialization to cache project data and track the active notebook. @@ -132,16 +104,6 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { this.originalProjects.set(projectId, clonedProject); } - /** - * Updates the current notebook ID for a project. - * Used when switching notebooks within the same project. - * @param projectId Project identifier - * @param notebookId New current notebook ID - */ - updateCurrentNotebookId(projectId: string, notebookId: string): void { - this.currentNotebookId.set(projectId, notebookId); - } - /** * Updates the integrations list in the project data. * This modifies the stored project to reflect changes in configured integrations. diff --git a/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts b/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts index b4276c605e..8a7de52200 100644 --- a/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts @@ -39,15 +39,6 @@ suite('DeepnoteNotebookManager', () => { assert.strictEqual(result, 'notebook-456'); }); - - test('should return updated notebook ID', () => { - manager.storeOriginalProject('project-123', mockProject, 'notebook-456'); - manager.updateCurrentNotebookId('project-123', 'notebook-789'); - - const result = manager.getCurrentNotebookId('project-123'); - - assert.strictEqual(result, 'notebook-789'); - }); }); suite('getOriginalProject', () => { @@ -66,33 +57,6 @@ suite('DeepnoteNotebookManager', () => { }); }); - suite('getTheSelectedNotebookForAProject', () => { - test('should return undefined for unknown project', () => { - const result = manager.getTheSelectedNotebookForAProject('unknown-project'); - - assert.strictEqual(result, undefined); - }); - - test('should return notebook ID after setting', () => { - manager.selectNotebookForProject('project-123', 'notebook-456'); - - const result = manager.getTheSelectedNotebookForAProject('project-123'); - - assert.strictEqual(result, 'notebook-456'); - }); - - test('should handle multiple projects independently', () => { - manager.selectNotebookForProject('project-1', 'notebook-1'); - manager.selectNotebookForProject('project-2', 'notebook-2'); - - const result1 = manager.getTheSelectedNotebookForAProject('project-1'); - const result2 = manager.getTheSelectedNotebookForAProject('project-2'); - - assert.strictEqual(result1, 'notebook-1'); - assert.strictEqual(result2, 'notebook-2'); - }); - }); - suite('consumePendingNotebookResolution', () => { test('should return undefined when no pending resolution exists', () => { const result = manager.consumePendingNotebookResolution('unknown-project'); @@ -126,70 +90,6 @@ suite('DeepnoteNotebookManager', () => { }); }); - suite('selectNotebookForProject', () => { - test('should store notebook selection for project', () => { - manager.selectNotebookForProject('project-123', 'notebook-456'); - - const selectedNotebook = manager.getTheSelectedNotebookForAProject('project-123'); - - assert.strictEqual(selectedNotebook, 'notebook-456'); - }); - - test('should overwrite existing selection', () => { - manager.selectNotebookForProject('project-123', 'notebook-456'); - manager.selectNotebookForProject('project-123', 'notebook-789'); - - const result = manager.getTheSelectedNotebookForAProject('project-123'); - - assert.strictEqual(result, 'notebook-789'); - }); - - test('should handle multiple projects independently', () => { - manager.selectNotebookForProject('project-1', 'notebook-1'); - manager.selectNotebookForProject('project-2', 'notebook-2'); - - const result1 = manager.getTheSelectedNotebookForAProject('project-1'); - const result2 = manager.getTheSelectedNotebookForAProject('project-2'); - - assert.strictEqual(result1, 'notebook-1'); - assert.strictEqual(result2, 'notebook-2'); - }); - }); - - suite('clearNotebookSelection', () => { - test('should clear selection for a project', () => { - manager.selectNotebookForProject('project-123', 'notebook-456'); - manager.clearNotebookSelection('project-123'); - - const selectedNotebook = manager.getTheSelectedNotebookForAProject('project-123'); - - assert.strictEqual(selectedNotebook, undefined); - }); - - test('should not affect other projects', () => { - manager.selectNotebookForProject('project-1', 'notebook-1'); - manager.selectNotebookForProject('project-2', 'notebook-2'); - manager.clearNotebookSelection('project-1'); - - assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-1'), undefined); - assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-2'), 'notebook-2'); - }); - - test('should be idempotent for unknown project', () => { - assert.doesNotThrow(() => { - manager.clearNotebookSelection('unknown-project'); - manager.clearNotebookSelection('unknown-project'); - }); - }); - - test('should clear pending notebook resolutions for a project', () => { - manager.queueNotebookResolution('project-123', 'notebook-456'); - manager.clearNotebookSelection('project-123'); - - assert.strictEqual(manager.consumePendingNotebookResolution('project-123'), undefined); - }); - }); - suite('storeOriginalProject', () => { test('should store both project and current notebook ID', () => { manager.storeOriginalProject('project-123', mockProject, 'notebook-456'); @@ -291,36 +191,6 @@ suite('DeepnoteNotebookManager', () => { }); }); - suite('updateCurrentNotebookId', () => { - test('should update notebook ID for existing project', () => { - manager.storeOriginalProject('project-123', mockProject, 'notebook-456'); - manager.updateCurrentNotebookId('project-123', 'notebook-789'); - - const result = manager.getCurrentNotebookId('project-123'); - - assert.strictEqual(result, 'notebook-789'); - }); - - test('should set notebook ID for new project', () => { - manager.updateCurrentNotebookId('new-project', 'notebook-123'); - - const result = manager.getCurrentNotebookId('new-project'); - - assert.strictEqual(result, 'notebook-123'); - }); - - test('should handle multiple projects independently', () => { - manager.updateCurrentNotebookId('project-1', 'notebook-1'); - manager.updateCurrentNotebookId('project-2', 'notebook-2'); - - const result1 = manager.getCurrentNotebookId('project-1'); - const result2 = manager.getCurrentNotebookId('project-2'); - - assert.strictEqual(result1, 'notebook-1'); - assert.strictEqual(result2, 'notebook-2'); - }); - }); - suite('updateProjectIntegrations', () => { test('should update integrations list for existing project and return true', () => { manager.storeOriginalProject('project-123', mockProject, 'notebook-456'); @@ -409,9 +279,8 @@ suite('DeepnoteNotebookManager', () => { }); test('should update integrations when currentNotebookId is undefined and return true', () => { - // Store project with a notebook ID, then clear it to simulate the edge case - manager.storeOriginalProject('project-123', mockProject, 'notebook-456'); - manager.updateCurrentNotebookId('project-123', undefined as any); + // Use updateOriginalProject which doesn't set currentNotebookId + manager.updateOriginalProject('project-123', mockProject); const integrations: ProjectIntegration[] = [ { id: 'int-1', name: 'PostgreSQL', type: 'pgsql' }, @@ -435,38 +304,17 @@ suite('DeepnoteNotebookManager', () => { suite('integration scenarios', () => { test('should handle complete workflow for multiple projects', () => { manager.storeOriginalProject('project-1', mockProject, 'notebook-1'); - manager.selectNotebookForProject('project-1', 'notebook-1'); - manager.storeOriginalProject('project-2', mockProject, 'notebook-2'); - manager.selectNotebookForProject('project-2', 'notebook-2'); assert.strictEqual(manager.getCurrentNotebookId('project-1'), 'notebook-1'); assert.strictEqual(manager.getCurrentNotebookId('project-2'), 'notebook-2'); - assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-1'), 'notebook-1'); - assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-2'), 'notebook-2'); }); - test('should handle notebook switching within same project', () => { + test('should handle notebook switching within same project via storeOriginalProject', () => { manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); - manager.selectNotebookForProject('project-123', 'notebook-1'); - - manager.updateCurrentNotebookId('project-123', 'notebook-2'); - manager.selectNotebookForProject('project-123', 'notebook-2'); + manager.storeOriginalProject('project-123', mockProject, 'notebook-2'); assert.strictEqual(manager.getCurrentNotebookId('project-123'), 'notebook-2'); - assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-123'), 'notebook-2'); - }); - - test('should maintain separation between current and selected notebook IDs', () => { - // Store original project sets current notebook - manager.storeOriginalProject('project-123', mockProject, 'notebook-original'); - - // Selecting a different notebook for the project - manager.selectNotebookForProject('project-123', 'notebook-selected'); - - // Both should be maintained independently - assert.strictEqual(manager.getCurrentNotebookId('project-123'), 'notebook-original'); - assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-123'), 'notebook-selected'); }); }); }); diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index cddaf6a60e..0653ba57dd 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -1,7 +1,15 @@ import type { DeepnoteBlock, DeepnoteFile, DeepnoteSnapshot } from '@deepnote/blocks'; import { deserializeDeepnoteFile, isExecutableBlock, serializeDeepnoteSnapshot } from '@deepnote/blocks'; import { inject, injectable, optional } from 'inversify'; -import { l10n, window, workspace, type CancellationToken, type NotebookData, type NotebookSerializer } from 'vscode'; +import { + TabInputNotebook, + l10n, + window, + workspace, + type CancellationToken, + type NotebookData, + type NotebookSerializer +} from 'vscode'; import { z } from 'zod'; import { computeHash } from '../../platform/common/crypto'; @@ -56,8 +64,6 @@ const recentSerializeTtlMs = 5_000; @injectable() export class DeepnoteNotebookSerializer implements NotebookSerializer { - private readonly consumedTabResolutions = new Map>(); - private converter = new DeepnoteDataConverter(); /** @@ -109,8 +115,7 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { } const projectId = deepnoteFile.project.id; - const projectNotebookIds = deepnoteFile.project.notebooks.map((nb) => nb.id); - const resolvedNotebookId = notebookId ?? this.findCurrentNotebookId(projectId, projectNotebookIds); + const resolvedNotebookId = notebookId ?? this.findCurrentNotebookId(projectId); logger.debug(`DeepnoteSerializer: Project ID: ${projectId}, Selected notebook ID: ${resolvedNotebookId}`); @@ -203,13 +208,12 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { /** * Finds the notebook ID to deserialize without relying on active-editor state. - * Prefers a pending resolution hint, then tab-based resolution (for reload), - * then current/open-document state. + * Prefers a pending resolution hint, then current/open-document state, + * and falls back to the active tab's URI query param for session restore. * @param projectId The project ID to find a notebook for - * @param projectNotebookIds Optional list of notebook IDs in the project, enables tab-based resolution * @returns The notebook ID to deserialize, or undefined if none found */ - findCurrentNotebookId(projectId: string, projectNotebookIds?: string[]): string | undefined { + findCurrentNotebookId(projectId: string): string | undefined { const pendingNotebookId = this.notebookManager.consumePendingNotebookResolution(projectId); const openNotebookIds = this.findOpenNotebookIds(projectId); const currentNotebookId = this.notebookManager.getCurrentNotebookId(projectId); @@ -218,21 +222,6 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return pendingNotebookId; } - if (projectNotebookIds && projectNotebookIds.length > 0) { - const tabNotebookIds = this.findNotebookIdsFromTabs(projectNotebookIds); - const consumedIds = this.consumedTabResolutions.get(projectId) ?? new Set(); - const remaining = tabNotebookIds.filter((id) => !consumedIds.has(id) && !openNotebookIds.includes(id)); - - if (remaining.length > 0) { - const consumed = this.consumedTabResolutions.get(projectId) ?? new Set(); - - consumed.add(remaining[0]); - this.consumedTabResolutions.set(projectId, consumed); - - return remaining[0]; - } - } - if (currentNotebookId && openNotebookIds.length === 0) { return currentNotebookId; } @@ -262,6 +251,14 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { } } + const activeTabNotebookId = this.findNotebookIdFromActiveTab(); + + if (activeTabNotebookId) { + logger.debug(`DeepnoteSerializer: Resolved notebook ID from active tab URI: ${activeTabNotebookId}`); + + return activeTabNotebookId; + } + return undefined; } @@ -404,11 +401,6 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { // for other open notebooks to resolve to the wrong notebook. this.notebookManager.updateOriginalProject(projectId, originalProject); this.recentSerializations.set(projectId, { notebookId, timestamp: Date.now() }); - const openNotebookIdsAtSerialize = this.findOpenNotebookIds(projectId); - - if (openNotebookIdsAtSerialize.length === 0) { - this.notebookManager.queueNotebookResolution(projectId, notebookId); - } logger.debug('SerializeNotebook: Serializing to YAML'); @@ -611,27 +603,28 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return false; } - private findNotebookIdsFromTabs(projectNotebookIds: string[]): string[] { - const projectIdSet = new Set(projectNotebookIds); - const notebookIds = new Set(); - - for (const group of window.tabGroups.all) { - for (const tab of group.tabs) { - const input = tab.input as { uri?: { query?: string }; notebookType?: string } | undefined; + /** + * Extracts the notebook ID from the active tab's URI query params. + * During session restore or tab open, VS Code may set the active tab + * before calling deserializeNotebook. The URI retains the + * `?notebook=` query param set when the tab was originally opened. + */ + private findNotebookIdFromActiveTab(): string | undefined { + const activeTab = window.tabGroups.activeTabGroup?.activeTab; - if (!input || !('uri' in input) || !('notebookType' in input) || input.notebookType !== 'deepnote') { - continue; - } + if (!activeTab || !(activeTab.input instanceof TabInputNotebook)) { + return undefined; + } - const notebookId = new URLSearchParams(input.uri?.query ?? '').get('notebook'); + const tabInput = activeTab.input; - if (notebookId && projectIdSet.has(notebookId)) { - notebookIds.add(notebookId); - } - } + if (tabInput.notebookType !== 'deepnote') { + return undefined; } - return [...notebookIds]; + const notebookId = new URLSearchParams(tabInput.uri.query).get('notebook'); + + return notebookId ?? undefined; } private findOpenNotebookIds(projectId: string): string[] { diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index 919af1ff23..7b0367c34d 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -2,7 +2,7 @@ import { deserializeDeepnoteFile, serializeDeepnoteFile, type DeepnoteFile } fro import { assert } from 'chai'; import { parse as parseYaml } from 'yaml'; import { when } from 'ts-mockito'; -import { Uri, type NotebookDocument } from 'vscode'; +import type { NotebookDocument } from 'vscode'; import { DeepnoteNotebookSerializer } from './deepnoteSerializer'; import { DeepnoteNotebookManager } from './deepnoteNotebookManager'; @@ -177,8 +177,8 @@ project: assert.include(result.cells[0].value, 'Title'); }); - test('should ignore stored selection when explicit notebookId is provided', async () => { - manager.selectNotebookForProject('project-123', 'notebook-1'); + test('should ignore queued resolution when explicit notebookId is provided', async () => { + manager.queueNotebookResolution('project-123', 'notebook-1'); const content = projectToYaml(mockProject); const result = await serializer.deserializeNotebook(content, {} as any, 'notebook-2'); @@ -253,9 +253,8 @@ project: assert.include(yamlString, 'notebook-1'); }); - test('should use current notebook ID instead of stale selected notebook when metadata notebook ID is missing', async () => { + test('should use current notebook ID when metadata notebook ID is missing', async () => { manager.storeOriginalProject('project-123', mockProject, 'notebook-2'); - manager.selectNotebookForProject('project-123', 'notebook-1'); const mockNotebookData = { cells: [ @@ -280,31 +279,6 @@ project: assert.strictEqual(serializedProject.project.notebooks[1].blocks?.[0].content, '# Updated second notebook'); }); - test('should queue the serialized notebook for the next resolution hint', async () => { - manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); - - const mockNotebookData = { - cells: [ - { - kind: 1, // NotebookCellKind.Markup - value: '# Updated second notebook', - languageId: 'markdown', - metadata: {} - } - ], - metadata: { - deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'notebook-2' - } - }; - - await serializer.serializeNotebook(mockNotebookData as any, {} as any); - - manager.updateCurrentNotebookId('project-123', 'notebook-1'); - - assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-2'); - }); - suite('multi-notebook save scenarios', () => { teardown(() => { when(mockedVSCodeNamespaces.window.activeNotebookEditor).thenReturn(undefined); @@ -450,7 +424,7 @@ project: test('should prioritize queued notebook resolution over current notebook and open documents', () => { manager.queueNotebookResolution('project-123', 'queued-notebook'); - manager.updateCurrentNotebookId('project-123', 'current-notebook'); + manager.storeOriginalProject('project-123', mockProject, 'current-notebook'); const mockNotebookDoc = { then: undefined, @@ -478,7 +452,7 @@ project: }); test('should return current notebook ID when no pending resolution exists', () => { - manager.updateCurrentNotebookId('project-123', 'current-notebook'); + manager.storeOriginalProject('project-123', mockProject, 'current-notebook'); const result = serializer.findCurrentNotebookId('project-123'); @@ -512,7 +486,7 @@ project: }); test('should prefer current notebook ID when multiple notebooks are open for a project', () => { - manager.updateCurrentNotebookId('project-123', 'notebook-b'); + manager.storeOriginalProject('project-123', mockProject, 'notebook-b'); const notebookA = { then: undefined, @@ -599,14 +573,6 @@ project: assert.strictEqual(result, undefined); }); - test('should ignore stale selected notebook state when no other resolver state exists', () => { - manager.selectNotebookForProject('project-123', 'stored-notebook'); - - const result = serializer.findCurrentNotebookId('project-123'); - - assert.strictEqual(result, undefined); - }); - test('after serializing notebook-1 with both notebooks open should return sibling notebook-2', async () => { manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); @@ -706,78 +672,6 @@ project: assert.strictEqual(result, undefined); }); - - suite('tab-based resolution', () => { - function createTabGroups(...notebookIds: string[]) { - const tabs = notebookIds.map((id) => ({ - input: { - uri: Uri.parse(`file:///test/project.deepnote?notebook=${id}`), - notebookType: 'deepnote' - } - })); - - return { all: [{ tabs }] } as any; - } - - teardown(() => { - when(mockedVSCodeNamespaces.window.tabGroups).thenReturn({ all: [] } as any); - }); - - test('should resolve different notebook IDs from tabs on sequential reload calls', () => { - when(mockedVSCodeNamespaces.window.tabGroups).thenReturn(createTabGroups('notebook-1', 'notebook-2')); - - const projectNotebookIds = ['notebook-1', 'notebook-2']; - - const first = serializer.findCurrentNotebookId('project-123', projectNotebookIds); - const second = serializer.findCurrentNotebookId('project-123', projectNotebookIds); - - assert.strictEqual(first, 'notebook-1'); - assert.strictEqual(second, 'notebook-2'); - }); - - test('should skip tab resolution when all tab notebook IDs are already open', () => { - when(mockedVSCodeNamespaces.window.tabGroups).thenReturn(createTabGroups('notebook-1', 'notebook-2')); - when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([ - createNotebookDoc('notebook-1'), - createNotebookDoc('notebook-2') - ]); - - const projectNotebookIds = ['notebook-1', 'notebook-2']; - const result = serializer.findCurrentNotebookId('project-123', projectNotebookIds); - - assert.strictEqual(result, undefined); - }); - - test('should filter tab notebook IDs by projectNotebookIds', () => { - when(mockedVSCodeNamespaces.window.tabGroups).thenReturn( - createTabGroups('notebook-1', 'other-project-notebook') - ); - - const projectNotebookIds = ['notebook-1', 'notebook-2']; - const result = serializer.findCurrentNotebookId('project-123', projectNotebookIds); - - assert.strictEqual(result, 'notebook-1'); - }); - - test('should prioritize pending resolution over tab resolution', () => { - when(mockedVSCodeNamespaces.window.tabGroups).thenReturn(createTabGroups('notebook-1', 'notebook-2')); - - manager.queueNotebookResolution('project-123', 'queued-notebook'); - - const projectNotebookIds = ['notebook-1', 'notebook-2']; - const result = serializer.findCurrentNotebookId('project-123', projectNotebookIds); - - assert.strictEqual(result, 'queued-notebook'); - }); - - test('should skip tab resolution when projectNotebookIds is not provided', () => { - when(mockedVSCodeNamespaces.window.tabGroups).thenReturn(createTabGroups('notebook-1', 'notebook-2')); - - const result = serializer.findCurrentNotebookId('project-123'); - - assert.strictEqual(result, undefined); - }); - }); }); suite('component integration', () => { @@ -806,12 +700,7 @@ project: assert.isFunction(manager.consumePendingNotebookResolution, 'has consumePendingNotebookResolution method'); assert.isFunction(manager.getCurrentNotebookId, 'has getCurrentNotebookId method'); assert.isFunction(manager.getOriginalProject, 'has getOriginalProject method'); - assert.isFunction( - manager.getTheSelectedNotebookForAProject, - 'has getTheSelectedNotebookForAProject method' - ); assert.isFunction(manager.queueNotebookResolution, 'has queueNotebookResolution method'); - assert.isFunction(manager.selectNotebookForProject, 'has selectNotebookForProject method'); assert.isFunction(manager.storeOriginalProject, 'has storeOriginalProject method'); }); diff --git a/src/notebooks/types.ts b/src/notebooks/types.ts index 527a56ddee..2da356ee25 100644 --- a/src/notebooks/types.ts +++ b/src/notebooks/types.ts @@ -37,17 +37,13 @@ export interface ProjectIntegration { export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); export interface IDeepnoteNotebookManager { - clearNotebookSelection(projectId: string): void; consumePendingNotebookResolution(projectId: string): string | undefined; getCurrentNotebookId(projectId: string): string | undefined; getOriginalProject(projectId: string): DeepnoteProject | undefined; - getTheSelectedNotebookForAProject(projectId: string): string | undefined; hasInitNotebookBeenRun(projectId: string): boolean; markInitNotebookAsRun(projectId: string): void; queueNotebookResolution(projectId: string, notebookId: string): void; - selectNotebookForProject(projectId: string, notebookId: string): void; storeOriginalProject(projectId: string, project: DeepnoteProject, notebookId: string): void; - updateCurrentNotebookId(projectId: string, notebookId: string): void; updateOriginalProject(projectId: string, project: DeepnoteProject): void; /** diff --git a/src/test/mocks/vsc/extHostedTypes.ts b/src/test/mocks/vsc/extHostedTypes.ts index 4a58f1c077..ab429319c4 100644 --- a/src/test/mocks/vsc/extHostedTypes.ts +++ b/src/test/mocks/vsc/extHostedTypes.ts @@ -2544,6 +2544,16 @@ export namespace vscMockExtHostedTypes { */ Default = 0 } + export class TabInputNotebook { + readonly uri: vscUri.URI; + readonly notebookType: string; + + constructor(uri: vscUri.URI, notebookType: string) { + this.uri = uri; + this.notebookType = notebookType; + } + } + export class NotebookCellData { kind: NotebookCellKind; value: string; diff --git a/src/test/vscode-mock.ts b/src/test/vscode-mock.ts index 7142c44d75..d413837dc7 100644 --- a/src/test/vscode-mock.ts +++ b/src/test/vscode-mock.ts @@ -301,6 +301,7 @@ export function resetVSCodeMocks() { (mockedVSCode as any).NotebookCellExecutionState = vscodeMocks.vscMockExtHostedTypes.NotebookCellExecutionState; (mockedVSCode as any).NotebookEditorRevealType = vscodeMocks.vscMockExtHostedTypes.NotebookEditorRevealType; // Mock ColorThemeKind enum + (mockedVSCode as any).TabInputNotebook = vscodeMocks.vscMockExtHostedTypes.TabInputNotebook; (mockedVSCode as any).ColorThemeKind = { Light: 1, Dark: 2, HighContrast: 3, HighContrastLight: 4 }; mockedVSCode.EndOfLine = vscodeMocks.vscMockExtHostedTypes.EndOfLine; } From 0396a2c1c7710b53558fef3c6bd566630d096a8a Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 7 Apr 2026 16:40:51 +0000 Subject: [PATCH 30/35] refactor(deepnote): Simplify DeepnoteActivationService and enhance notebook ID resolution - Removed unused variables and methods related to mismatch checking in `DeepnoteActivationService`, streamlining the activation process. - Updated `findCurrentNotebookId` in `DeepnoteSerializer` to improve notebook ID resolution logic, prioritizing active tab detection. - Adjusted unit tests to reflect changes in notebook ID resolution, ensuring accurate behavior when no pending resolutions exist. --- .../deepnote/deepnoteActivationService.ts | 118 +----------------- src/notebooks/deepnote/deepnoteSerializer.ts | 38 +++--- .../deepnote/deepnoteSerializer.unit.test.ts | 79 +----------- 3 files changed, 19 insertions(+), 216 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteActivationService.ts b/src/notebooks/deepnote/deepnoteActivationService.ts index 8958ad7e22..96c35288cd 100644 --- a/src/notebooks/deepnote/deepnoteActivationService.ts +++ b/src/notebooks/deepnote/deepnoteActivationService.ts @@ -1,17 +1,5 @@ import { inject, injectable, optional } from 'inversify'; -import { - CancellationTokenSource, - commands, - l10n, - NotebookEdit, - NotebookRange, - workspace, - window, - WorkspaceEdit, - type Disposable, - type NotebookDocument, - type NotebookDocumentContentOptions -} from 'vscode'; +import { commands, l10n, workspace, window, type Disposable, type NotebookDocumentContentOptions } from 'vscode'; import { IExtensionSyncActivationService } from '../../platform/activation/types'; import { IExtensionContext } from '../../platform/common/types'; @@ -27,9 +15,6 @@ import { SnapshotService } from './snapshots/snapshotService'; * Service responsible for activating and configuring Deepnote notebook support in VS Code. * Registers serializers, command handlers, and manages the notebook selection workflow. */ -const MISMATCH_CHECK_DELAY_MS = 200; -const MAX_MISMATCH_RETRIES = 10; - @injectable() export class DeepnoteActivationService implements IExtensionSyncActivationService { private editProtection: DeepnoteInputBlockEditProtection; @@ -38,10 +23,6 @@ export class DeepnoteActivationService implements IExtensionSyncActivationServic private integrationManager: IIntegrationManager; - private mismatchCheckTimer: ReturnType | undefined; - - private mismatchRetryCount = 0; - private serializer: DeepnoteNotebookSerializer; private serializerRegistration?: Disposable; @@ -68,18 +49,6 @@ export class DeepnoteActivationService implements IExtensionSyncActivationServic this.editProtection = new DeepnoteInputBlockEditProtection(this.logger); this.snapshotsEnabled = this.isSnapshotsEnabled(); - this.extensionContext.subscriptions.push( - workspace.onDidOpenNotebookDocument((doc) => { - if (doc.notebookType !== 'deepnote') { - return; - } - - if (new URLSearchParams(doc.uri.query).has('notebook')) { - this.scheduleMismatchCheck(); - } - }) - ); - this.registerSerializer(); this.extensionContext.subscriptions.push(this.editProtection); this.extensionContext.subscriptions.push( @@ -101,80 +70,6 @@ export class DeepnoteActivationService implements IExtensionSyncActivationServic this.integrationManager.activate(); } - /** - * Checks all open deepnote documents for URI/metadata notebook ID mismatches - * and fixes them by re-deserializing with the correct notebook ID. - * This handles the case where VS Code's deserializeNotebook API does not - * pass the document URI, causing the wrong notebook to be loaded on reload. - */ - private async checkAndFixMismatches(): Promise { - const hasLoadingDocs = workspace.notebookDocuments.some( - (doc) => - doc.notebookType === 'deepnote' && - !doc.metadata?.deepnoteNotebookId && - new URLSearchParams(doc.uri.query).has('notebook') - ); - - if (hasLoadingDocs && this.mismatchRetryCount < MAX_MISMATCH_RETRIES) { - this.mismatchRetryCount++; - this.scheduleMismatchCheck(); - - return; - } - - this.mismatchRetryCount = 0; - - for (const doc of workspace.notebookDocuments) { - if (doc.notebookType !== 'deepnote' || doc.isClosed) { - continue; - } - - const uriNotebookId = new URLSearchParams(doc.uri.query).get('notebook'); - const metadataNotebookId = doc.metadata?.deepnoteNotebookId as string | undefined; - - if (!uriNotebookId || uriNotebookId === metadataNotebookId) { - continue; - } - - await this.fixDocumentNotebook(doc, uriNotebookId); - } - } - - private async fixDocumentNotebook(doc: NotebookDocument, correctNotebookId: string): Promise { - const fileUri = doc.uri.with({ query: '', fragment: '' }); - - let content: Uint8Array; - try { - content = await workspace.fs.readFile(fileUri); - } catch { - this.logger.warn(`[DeepnoteActivation] Cannot read file for mismatch fix: ${fileUri.path}`); - - return; - } - - const cts = new CancellationTokenSource(); - try { - const data = await this.serializer.deserializeNotebook(content, cts.token, correctNotebookId); - - const wsEdit = new WorkspaceEdit(); - wsEdit.set(doc.uri, [ - NotebookEdit.replaceCells(new NotebookRange(0, doc.cellCount), data.cells), - NotebookEdit.updateNotebookMetadata(data.metadata!) - ]); - await workspace.applyEdit(wsEdit); - await doc.save(); - - this.logger.info( - `[DeepnoteActivation] Fixed notebook mismatch for ${doc.uri.path}: ` + - `loaded ${correctNotebookId} (was ${doc.metadata?.deepnoteNotebookId})` - ); - } catch (error) { - this.logger.error(`[DeepnoteActivation] Failed to fix notebook mismatch: ${doc.uri.path}`, error); - } finally { - cts.dispose(); - } - } - private isSnapshotsEnabled(): boolean { if (this.snapshotService) { return this.snapshotService.isSnapshotsEnabled(); @@ -235,15 +130,4 @@ export class DeepnoteActivationService implements IExtensionSyncActivationServic this.serializerRegistration = workspace.registerNotebookSerializer('deepnote', this.serializer, contentOptions); this.extensionContext.subscriptions.push(this.serializerRegistration); } - - private scheduleMismatchCheck(): void { - if (this.mismatchCheckTimer !== undefined) { - clearTimeout(this.mismatchCheckTimer); - } - - this.mismatchCheckTimer = setTimeout(() => { - this.mismatchCheckTimer = undefined; - void this.checkAndFixMismatches(); - }, MISMATCH_CHECK_DELAY_MS); - } } diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index 0653ba57dd..6aac61ad8d 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -207,33 +207,35 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { } /** - * Finds the notebook ID to deserialize without relying on active-editor state. - * Prefers a pending resolution hint, then current/open-document state, - * and falls back to the active tab's URI query param for session restore. + * Finds the notebook ID to deserialize for VS Code-initiated deserialization. + * Priority: + * 1. Pending resolution hint (explicit intent from explorer view) + * 2. Active tab URI query param (VS Code's ground truth for which tab is loading) + * 3. Sibling detection (post-save re-read for other open tabs) * @param projectId The project ID to find a notebook for * @returns The notebook ID to deserialize, or undefined if none found */ findCurrentNotebookId(projectId: string): string | undefined { const pendingNotebookId = this.notebookManager.consumePendingNotebookResolution(projectId); - const openNotebookIds = this.findOpenNotebookIds(projectId); - const currentNotebookId = this.notebookManager.getCurrentNotebookId(projectId); if (pendingNotebookId) { + logger.debug(`DeepnoteSerializer: Resolved notebook ID from pending resolution: ${pendingNotebookId}`); return pendingNotebookId; } - if (currentNotebookId && openNotebookIds.length === 0) { - return currentNotebookId; - } + const activeTabNotebookId = this.findNotebookIdFromActiveTab(); + + if (activeTabNotebookId) { + logger.debug(`DeepnoteSerializer: Resolved notebook ID from active tab URI: ${activeTabNotebookId}`); - if (openNotebookIds.length === 1) { - return openNotebookIds[0]; + return activeTabNotebookId; } // Multiple notebooks open — VS Code may be re-reading the file for a // sibling tab after a save. Pick a sibling (any open notebook that is - // NOT the one just serialized). If no recent serialization, fall back - // to currentNotebookId for backward compatibility. + // NOT the one just serialized). + const openNotebookIds = this.findOpenNotebookIds(projectId); + if (openNotebookIds.length > 1) { const recent = this.recentSerializations.get(projectId); @@ -245,18 +247,6 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return sibling; } } - - if (currentNotebookId && openNotebookIds.includes(currentNotebookId)) { - return currentNotebookId; - } - } - - const activeTabNotebookId = this.findNotebookIdFromActiveTab(); - - if (activeTabNotebookId) { - logger.debug(`DeepnoteSerializer: Resolved notebook ID from active tab URI: ${activeTabNotebookId}`); - - return activeTabNotebookId; } return undefined; diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index 7b0367c34d..f0d9608f7b 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -451,83 +451,12 @@ project: assert.strictEqual(result, 'queued-notebook'); }); - test('should return current notebook ID when no pending resolution exists', () => { + test('should return undefined when no pending resolution or active tab exists', () => { manager.storeOriginalProject('project-123', mockProject, 'current-notebook'); const result = serializer.findCurrentNotebookId('project-123'); - assert.strictEqual(result, 'current-notebook'); - }); - - test('should return the only open notebook when current notebook ID is unavailable', () => { - const mockNotebookDoc = { - then: undefined, - notebookType: 'deepnote', - metadata: { - deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'notebook-from-workspace' - }, - uri: {} as any, - version: 1, - isDirty: false, - isUntitled: false, - isClosed: false, - cellCount: 0, - cellAt: () => ({}) as any, - getCells: () => [], - save: async () => true - } as NotebookDocument; - - when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([mockNotebookDoc]); - - const result = serializer.findCurrentNotebookId('project-123'); - - assert.strictEqual(result, 'notebook-from-workspace'); - }); - - test('should prefer current notebook ID when multiple notebooks are open for a project', () => { - manager.storeOriginalProject('project-123', mockProject, 'notebook-b'); - - const notebookA = { - then: undefined, - notebookType: 'deepnote', - metadata: { - deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'notebook-a' - }, - uri: {} as any, - version: 1, - isDirty: false, - isUntitled: false, - isClosed: false, - cellCount: 0, - cellAt: () => ({}) as any, - getCells: () => [], - save: async () => true - } as NotebookDocument; - const notebookB = { - then: undefined, - notebookType: 'deepnote', - metadata: { - deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'notebook-b' - }, - uri: {} as any, - version: 1, - isDirty: false, - isUntitled: false, - isClosed: false, - cellCount: 0, - cellAt: () => ({}) as any, - getCells: () => [], - save: async () => true - } as NotebookDocument; - - when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookA, notebookB]); - - const result = serializer.findCurrentNotebookId('project-123'); - - assert.strictEqual(result, 'notebook-b'); + assert.strictEqual(result, undefined); }); test('should return undefined when multiple notebooks are open and no stronger signal exists', () => { @@ -629,7 +558,7 @@ project: await serializer.serializeNotebook(mockNotebookData as any, {} as any); assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-2'); - assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-1'); + assert.strictEqual(serializer.findCurrentNotebookId('project-123'), undefined); }); test('recent serialization hint expires after TTL', async () => { @@ -664,7 +593,7 @@ project: const result = serializer.findCurrentNotebookId('project-123'); - assert.strictEqual(result, 'notebook-1'); + assert.strictEqual(result, undefined); }); test('should return undefined for unknown project', () => { From 06d31979c118b7c9c2da0fd0a7ce5726a9bf3f06 Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 7 Apr 2026 17:06:22 +0000 Subject: [PATCH 31/35] refactor(deepnote): Streamline DeepnoteNotebookSerializer and remove unused logic - Eliminated unnecessary variables and methods related to recent serialization tracking in `DeepnoteNotebookSerializer`, simplifying the notebook ID resolution process. - Updated the logic for finding the current notebook ID to focus on active tab detection, enhancing overall efficiency. - Removed outdated unit tests that were no longer relevant to the current implementation, ensuring test suite accuracy. --- src/notebooks/deepnote/deepnoteSerializer.ts | 51 ---- .../deepnote/deepnoteSerializer.unit.test.ts | 242 ------------------ 2 files changed, 293 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index 6aac61ad8d..8bbdc0b796 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -10,7 +10,6 @@ import { type NotebookData, type NotebookSerializer } from 'vscode'; -import { z } from 'zod'; import { computeHash } from '../../platform/common/crypto'; import type { DeepnoteNotebook } from '../../platform/deepnote/deepnoteTypes'; @@ -59,21 +58,10 @@ function cloneWithoutCircularRefs(obj: T, seen = new WeakSet()): T { * Serializer for converting between Deepnote YAML files and VS Code notebook format. * Handles reading/writing .deepnote files and manages project state persistence. */ -/** Window (ms) during which a post-serialize deserialize excludes the serialized notebook. */ -const recentSerializeTtlMs = 5_000; - @injectable() export class DeepnoteNotebookSerializer implements NotebookSerializer { private converter = new DeepnoteDataConverter(); - /** - * Tracks the most recently serialized notebook per project. - * When VS Code calls $dataToNotebook after a save, it's re-reading the - * file for a SIBLING tab (the saved notebook already has its content). - * This tracker lets findCurrentNotebookId pick a sibling instead. - */ - private readonly recentSerializations = new Map(); - constructor( @inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager, @inject(SnapshotService) @optional() private readonly snapshotService?: SnapshotService @@ -211,7 +199,6 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { * Priority: * 1. Pending resolution hint (explicit intent from explorer view) * 2. Active tab URI query param (VS Code's ground truth for which tab is loading) - * 3. Sibling detection (post-save re-read for other open tabs) * @param projectId The project ID to find a notebook for * @returns The notebook ID to deserialize, or undefined if none found */ @@ -219,7 +206,6 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { const pendingNotebookId = this.notebookManager.consumePendingNotebookResolution(projectId); if (pendingNotebookId) { - logger.debug(`DeepnoteSerializer: Resolved notebook ID from pending resolution: ${pendingNotebookId}`); return pendingNotebookId; } @@ -231,24 +217,6 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return activeTabNotebookId; } - // Multiple notebooks open — VS Code may be re-reading the file for a - // sibling tab after a save. Pick a sibling (any open notebook that is - // NOT the one just serialized). - const openNotebookIds = this.findOpenNotebookIds(projectId); - - if (openNotebookIds.length > 1) { - const recent = this.recentSerializations.get(projectId); - - if (recent && Date.now() - recent.timestamp < recentSerializeTtlMs) { - this.recentSerializations.delete(projectId); - const sibling = openNotebookIds.find((id) => id !== recent.notebookId); - - if (sibling) { - return sibling; - } - } - } - return undefined; } @@ -390,7 +358,6 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { // currentNotebookId here would cause VS Code's follow-up deserialize calls // for other open notebooks to resolve to the wrong notebook. this.notebookManager.updateOriginalProject(projectId, originalProject); - this.recentSerializations.set(projectId, { notebookId, timestamp: Date.now() }); logger.debug('SerializeNotebook: Serializing to YAML'); @@ -617,24 +584,6 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return notebookId ?? undefined; } - private findOpenNotebookIds(projectId: string): string[] { - return [ - ...workspace.notebookDocuments.reduce((ids, doc) => { - if (doc.notebookType !== 'deepnote' || doc.metadata.deepnoteProjectId !== projectId) { - return ids; - } - - const parsed = z.string().safeParse(doc.metadata.deepnoteNotebookId); - - if (parsed.success) { - ids.add(parsed.data); - } - - return ids; - }, new Set()) - ]; - } - /** * Finds the default notebook to open when no selection is made. * @param file diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index f0d9608f7b..139673a4b5 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -73,26 +73,6 @@ suite('DeepnoteNotebookSerializer', () => { return new TextEncoder().encode(yamlString); } - function createNotebookDoc(notebookId: string, projectId: string = 'project-123'): NotebookDocument { - return { - cellAt: () => ({}) as any, - cellCount: 0, - getCells: () => [], - isClosed: false, - isDirty: false, - isUntitled: false, - metadata: { - deepnoteNotebookId: notebookId, - deepnoteProjectId: projectId - }, - notebookType: 'deepnote', - save: async () => true, - then: undefined, - uri: {} as any, - version: 1 - } as NotebookDocument; - } - suite('deserializeNotebook', () => { test('should deserialize valid project with queued notebook resolution', async () => { manager.queueNotebookResolution('project-123', 'notebook-1'); @@ -311,91 +291,6 @@ project: assert.strictEqual(manager.getCurrentNotebookId('project-123'), 'notebook-2'); }); - test('serializing notebook-1 then deserializing without explicit ID should resolve to sibling notebook-2', async () => { - const yamlBytes = projectToYaml(mockProject); - - manager.queueNotebookResolution('project-123', 'notebook-1'); - await serializer.deserializeNotebook(yamlBytes, {} as any); - manager.queueNotebookResolution('project-123', 'notebook-2'); - await serializer.deserializeNotebook(yamlBytes, {} as any); - - const notebookDoc1 = createNotebookDoc('notebook-1'); - const notebookDoc2 = createNotebookDoc('notebook-2'); - - when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookDoc1, notebookDoc2]); - - const mockNotebookData = { - cells: [ - { - kind: 2, - languageId: 'python', - metadata: {}, - value: 'print("saved from nb1")' - } - ], - metadata: { - deepnoteNotebookId: 'notebook-1', - deepnoteProjectId: 'project-123' - } - }; - - const saved = await serializer.serializeNotebook(mockNotebookData as any, {} as any); - const afterSave = await serializer.deserializeNotebook(saved, {} as any); - - assert.strictEqual(afterSave.metadata?.deepnoteNotebookId, 'notebook-2'); - }); - - test('sequential saves: contextless deserialize returns the sibling after each serialize', async () => { - const yamlBytes = projectToYaml(mockProject); - - manager.queueNotebookResolution('project-123', 'notebook-1'); - await serializer.deserializeNotebook(yamlBytes, {} as any); - manager.queueNotebookResolution('project-123', 'notebook-2'); - await serializer.deserializeNotebook(yamlBytes, {} as any); - - const notebookDoc1 = createNotebookDoc('notebook-1'); - const notebookDoc2 = createNotebookDoc('notebook-2'); - - when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookDoc1, notebookDoc2]); - - const dataNb1 = { - cells: [ - { - kind: 2, - languageId: 'python', - metadata: {}, - value: 'print("round-a")' - } - ], - metadata: { - deepnoteNotebookId: 'notebook-1', - deepnoteProjectId: 'project-123' - } - }; - let bytes = await serializer.serializeNotebook(dataNb1 as any, {} as any); - let meta = await serializer.deserializeNotebook(bytes, {} as any); - - assert.strictEqual(meta.metadata?.deepnoteNotebookId, 'notebook-2'); - - const dataNb2 = { - cells: [ - { - kind: 1, - languageId: 'markdown', - metadata: {}, - value: '# round-b' - } - ], - metadata: { - deepnoteNotebookId: 'notebook-2', - deepnoteProjectId: 'project-123' - } - }; - bytes = await serializer.serializeNotebook(dataNb2 as any, {} as any); - meta = await serializer.deserializeNotebook(bytes, {} as any); - - assert.strictEqual(meta.metadata?.deepnoteNotebookId, 'notebook-1'); - }); }); }); @@ -459,143 +354,6 @@ project: assert.strictEqual(result, undefined); }); - test('should return undefined when multiple notebooks are open and no stronger signal exists', () => { - const notebookA = { - then: undefined, - notebookType: 'deepnote', - metadata: { - deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'notebook-a' - }, - uri: {} as any, - version: 1, - isDirty: false, - isUntitled: false, - isClosed: false, - cellCount: 0, - cellAt: () => ({}) as any, - getCells: () => [], - save: async () => true - } as NotebookDocument; - const notebookB = { - then: undefined, - notebookType: 'deepnote', - metadata: { - deepnoteProjectId: 'project-123', - deepnoteNotebookId: 'notebook-b' - }, - uri: {} as any, - version: 1, - isDirty: false, - isUntitled: false, - isClosed: false, - cellCount: 0, - cellAt: () => ({}) as any, - getCells: () => [], - save: async () => true - } as NotebookDocument; - - when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookA, notebookB]); - - const result = serializer.findCurrentNotebookId('project-123'); - - assert.strictEqual(result, undefined); - }); - - test('after serializing notebook-1 with both notebooks open should return sibling notebook-2', async () => { - manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); - - const notebookDoc1 = createNotebookDoc('notebook-1'); - const notebookDoc2 = createNotebookDoc('notebook-2'); - - when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookDoc1, notebookDoc2]); - - const mockNotebookData = { - cells: [ - { - kind: 2, - languageId: 'python', - metadata: {}, - value: 'print("findOpen")' - } - ], - metadata: { - deepnoteNotebookId: 'notebook-1', - deepnoteProjectId: 'project-123' - } - }; - - await serializer.serializeNotebook(mockNotebookData as any, {} as any); - - const result = serializer.findCurrentNotebookId('project-123'); - - assert.strictEqual(result, 'notebook-2'); - }); - - test('recent serialization hint is one-shot', async () => { - manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); - - const notebookDoc1 = createNotebookDoc('notebook-1'); - const notebookDoc2 = createNotebookDoc('notebook-2'); - - when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookDoc1, notebookDoc2]); - - const mockNotebookData = { - cells: [ - { - kind: 2, - languageId: 'python', - metadata: {}, - value: 'print("one-shot")' - } - ], - metadata: { - deepnoteNotebookId: 'notebook-1', - deepnoteProjectId: 'project-123' - } - }; - - await serializer.serializeNotebook(mockNotebookData as any, {} as any); - - assert.strictEqual(serializer.findCurrentNotebookId('project-123'), 'notebook-2'); - assert.strictEqual(serializer.findCurrentNotebookId('project-123'), undefined); - }); - - test('recent serialization hint expires after TTL', async () => { - manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); - - const notebookDoc1 = createNotebookDoc('notebook-1'); - const notebookDoc2 = createNotebookDoc('notebook-2'); - - when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebookDoc1, notebookDoc2]); - - const mockNotebookData = { - cells: [ - { - kind: 2, - languageId: 'python', - metadata: {}, - value: 'print("ttl")' - } - ], - metadata: { - deepnoteNotebookId: 'notebook-1', - deepnoteProjectId: 'project-123' - } - }; - - await serializer.serializeNotebook(mockNotebookData as any, {} as any); - - (serializer as any).recentSerializations.set('project-123', { - notebookId: 'notebook-1', - timestamp: Date.now() - 6000 - }); - - const result = serializer.findCurrentNotebookId('project-123'); - - assert.strictEqual(result, undefined); - }); - test('should return undefined for unknown project', () => { const result = serializer.findCurrentNotebookId('unknown-project'); From f4b79d3dfd389beae8602c5c1afda8dee808734a Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 7 Apr 2026 17:52:38 +0000 Subject: [PATCH 32/35] feat(deepnote): Add logging for pending notebook resolutions in DeepnoteNotebookManager - Introduced debug logging in `DeepnoteNotebookManager` to track valid pending notebook resolutions, enhancing traceability during resolution processes. - Simplified block iteration in `DeepnoteNotebookSerializer` by removing unnecessary null checks, improving code clarity and performance. --- src/notebooks/deepnote/deepnoteNotebookManager.ts | 6 ++++++ src/notebooks/deepnote/deepnoteSerializer.ts | 6 +++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteNotebookManager.ts b/src/notebooks/deepnote/deepnoteNotebookManager.ts index b2e654e673..7d80ce3391 100644 --- a/src/notebooks/deepnote/deepnoteNotebookManager.ts +++ b/src/notebooks/deepnote/deepnoteNotebookManager.ts @@ -2,6 +2,7 @@ import { injectable } from 'inversify'; import { IDeepnoteNotebookManager, ProjectIntegration } from '../types'; import type { DeepnoteProject } from '../../platform/deepnote/deepnoteTypes'; +import { logger } from '../../platform/logging'; const pendingNotebookResolutionTtlMs = 60_000; @@ -152,6 +153,11 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { private getValidPendingNotebookResolutions(projectId: string): PendingNotebookResolution[] { const cutoffTime = Date.now() - pendingNotebookResolutionTtlMs; + const allPendingResolutions = this.pendingNotebookResolutions.get(projectId) ?? []; + logger.debug( + `DeepnoteNotebookManager: getValidPendingNotebookResolutions: projectId=${projectId}, allPendingResolutions=${allPendingResolutions.length}` + ); + logger.debug(JSON.stringify(allPendingResolutions, null, 2)); const pendingResolutions = (this.pendingNotebookResolutions.get(projectId) ?? []).filter( (resolution) => resolution.queuedAt >= cutoffTime ); diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index 8bbdc0b796..ba3927e4f8 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -120,12 +120,12 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { } // Log block IDs from source file - for (let i = 0; i < (selectedNotebook.blocks ?? []).length; i++) { - const block = selectedNotebook.blocks![i]; + for (let i = 0; i < selectedNotebook.blocks.length; i++) { + const block = selectedNotebook.blocks[i]; logger.trace(`DeserializeNotebook: block[${i}] id=${block.id} from source file`); } - let cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []); + let cells = this.converter.convertBlocksToCells(selectedNotebook.blocks); logger.debug(`DeepnoteSerializer: Converted ${cells.length} cells from notebook blocks`); From c83af5532f6ebb1a9c352fcbb09702e930352d80 Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 7 Apr 2026 18:01:13 +0000 Subject: [PATCH 33/35] Reformat code --- src/notebooks/deepnote/deepnoteSerializer.unit.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index 139673a4b5..df2973ccdf 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -290,7 +290,6 @@ project: assert.strictEqual(manager.getCurrentNotebookId('project-123'), 'notebook-2'); }); - }); }); From 40f6c6703ce80400bb8503378288333a384f7f1e Mon Sep 17 00:00:00 2001 From: tomas Date: Tue, 7 Apr 2026 18:33:32 +0000 Subject: [PATCH 34/35] Fix test --- .../deepnote/deepnoteFileChangeWatcher.unit.test.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts index fedca12907..be90fbfa65 100644 --- a/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts @@ -510,12 +510,22 @@ project: }); test('should not suppress real changes after auto-save', async function () { - this.timeout(5000); + this.timeout(10_000); const uri = testFileUri('test.deepnote'); // First change: notebook has no cells, YAML has one cell -> different -> reload const notebook = createMockNotebook({ uri, cellCount: 0, cells: [] }); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([notebook]); + + // Override save mock to fire onDidSaveNotebook (matching real VS Code behavior). + // The onDidSaveNotebookDocument handler calls markSelfWrite, producing the + // second self-write marker that corresponds to the serializer's save-triggered write. + when(mockedVSCodeNamespaces.workspace.save(anything())).thenCall((saveUri: Uri) => { + saveCount++; + onDidSaveNotebook.fire(notebook); + return Promise.resolve(saveUri); + }); + setupMockFs(validYaml); onDidChangeFile.fire(uri); From 46cbcda38c9a71a8ead921aab1efa7e6e265e3bc Mon Sep 17 00:00:00 2001 From: tomas Date: Thu, 9 Apr 2026 07:41:22 +0000 Subject: [PATCH 35/35] feat(deepnote): Enhance notebook deserialization and verification logic - Added a new event listener in `DeepnoteActivationService` to verify deserialized notebooks upon opening. - Improved `DeepnoteNotebookSerializer` to handle cases where no notebook ID is resolved, returning an empty state instead of throwing an error. - Updated unit tests to reflect changes in notebook deserialization behavior, ensuring accurate handling of scenarios with no available notebooks. --- .../deepnote/deepnoteActivationService.ts | 7 +- src/notebooks/deepnote/deepnoteSerializer.ts | 185 ++++++++++++++---- .../deepnote/deepnoteSerializer.unit.test.ts | 51 ++--- 3 files changed, 178 insertions(+), 65 deletions(-) diff --git a/src/notebooks/deepnote/deepnoteActivationService.ts b/src/notebooks/deepnote/deepnoteActivationService.ts index 96c35288cd..edd3474a00 100644 --- a/src/notebooks/deepnote/deepnoteActivationService.ts +++ b/src/notebooks/deepnote/deepnoteActivationService.ts @@ -1,5 +1,5 @@ import { inject, injectable, optional } from 'inversify'; -import { commands, l10n, workspace, window, type Disposable, type NotebookDocumentContentOptions } from 'vscode'; +import { commands, l10n, window, workspace, type Disposable, type NotebookDocumentContentOptions } from 'vscode'; import { IExtensionSyncActivationService } from '../../platform/activation/types'; import { IExtensionContext } from '../../platform/common/types'; @@ -51,6 +51,11 @@ export class DeepnoteActivationService implements IExtensionSyncActivationServic this.registerSerializer(); this.extensionContext.subscriptions.push(this.editProtection); + this.extensionContext.subscriptions.push( + workspace.onDidOpenNotebookDocument((doc) => { + void this.serializer.verifyDeserializedNotebook(doc); + }) + ); this.extensionContext.subscriptions.push( workspace.onDidChangeConfiguration((event) => { if (event.affectsConfiguration('deepnote.snapshots.enabled')) { diff --git a/src/notebooks/deepnote/deepnoteSerializer.ts b/src/notebooks/deepnote/deepnoteSerializer.ts index ba3927e4f8..531c645957 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.ts @@ -2,12 +2,15 @@ import type { DeepnoteBlock, DeepnoteFile, DeepnoteSnapshot } from '@deepnote/bl import { deserializeDeepnoteFile, isExecutableBlock, serializeDeepnoteSnapshot } from '@deepnote/blocks'; import { inject, injectable, optional } from 'inversify'; import { - TabInputNotebook, + CancellationTokenSource, l10n, - window, + NotebookEdit, + NotebookRange, workspace, + WorkspaceEdit, type CancellationToken, type NotebookData, + type NotebookDocument, type NotebookSerializer } from 'vscode'; @@ -54,6 +57,8 @@ function cloneWithoutCircularRefs(obj: T, seen = new WeakSet()): T { } } +const LAST_SERIALIZED_TTL_MS = 10_000; + /** * Serializer for converting between Deepnote YAML files and VS Code notebook format. * Handles reading/writing .deepnote files and manages project state persistence. @@ -61,6 +66,8 @@ function cloneWithoutCircularRefs(obj: T, seen = new WeakSet()): T { @injectable() export class DeepnoteNotebookSerializer implements NotebookSerializer { private converter = new DeepnoteDataConverter(); + private lastSerializedNotebookId: string | undefined; + private lastSerializedTimestamp = 0; constructor( @inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager, @@ -87,13 +94,6 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { throw new Error('Serialization cancelled'); } - // Initialize vega-lite for output conversion (lazy-loaded ESM module) - await this.converter.initialize(); - - if (token?.isCancellationRequested) { - throw new Error('Serialization cancelled'); - } - try { const contentString = new TextDecoder('utf-8').decode(content); const deepnoteFile = deserializeDeepnoteFile(contentString); @@ -107,18 +107,38 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { logger.debug(`DeepnoteSerializer: Project ID: ${projectId}, Selected notebook ID: ${resolvedNotebookId}`); + if (!resolvedNotebookId) { + logger.debug( + 'DeepnoteSerializer: No notebook ID resolved, returning empty state for post-open verification' + ); + + return { + cells: [], + metadata: { + deepnoteProjectId: projectId, + deepnoteProjectName: deepnoteFile.project.name, + deepnoteVersion: deepnoteFile.version + } + }; + } + if (deepnoteFile.project.notebooks.length === 0) { throw new Error('Deepnote project contains no notebooks.'); } - const selectedNotebook = resolvedNotebookId - ? deepnoteFile.project.notebooks.find((nb) => nb.id === resolvedNotebookId) - : this.findDefaultNotebook(deepnoteFile); + const selectedNotebook = deepnoteFile.project.notebooks.find((nb) => nb.id === resolvedNotebookId); if (!selectedNotebook) { throw new Error(l10n.t('No notebook selected or found')); } + // Initialize vega-lite for output conversion (lazy-loaded ESM module) + await this.converter.initialize(); + + if (token?.isCancellationRequested) { + throw new Error('Serialization cancelled'); + } + // Log block IDs from source file for (let i = 0; i < selectedNotebook.blocks.length; i++) { const block = selectedNotebook.blocks[i]; @@ -195,12 +215,15 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { } /** - * Finds the notebook ID to deserialize for VS Code-initiated deserialization. + * Resolves the notebook ID for a deserialization call. + * * Priority: * 1. Pending resolution hint (explicit intent from explorer view) - * 2. Active tab URI query param (VS Code's ground truth for which tab is loading) - * @param projectId The project ID to find a notebook for - * @returns The notebook ID to deserialize, or undefined if none found + * 2. Already-open document metadata (re-deserialization after file change) + * — skips the notebook that was just serialized, since that's the one + * that triggered the file change, not the one being re-deserialized. + * 3. undefined — initial open; verifyDeserializedNotebook will resolve + * from the document URI after open. */ findCurrentNotebookId(projectId: string): string | undefined { const pendingNotebookId = this.notebookManager.consumePendingNotebookResolution(projectId); @@ -209,15 +232,7 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return pendingNotebookId; } - const activeTabNotebookId = this.findNotebookIdFromActiveTab(); - - if (activeTabNotebookId) { - logger.debug(`DeepnoteSerializer: Resolved notebook ID from active tab URI: ${activeTabNotebookId}`); - - return activeTabNotebookId; - } - - return undefined; + return this.findNotebookIdFromOpenDocuments(projectId); } /** @@ -228,6 +243,81 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { return this.converter; } + /** + * Parses the file content and returns the ID of the default notebook + * (alphabetically first, excluding Init when other notebooks exist). + * Used by the post-open verification handler for direct file opens + * that have no `?notebook=` query param. + */ + resolveDefaultNotebookId(content: Uint8Array): string | undefined { + try { + const contentString = new TextDecoder('utf-8').decode(content); + const deepnoteFile = deserializeDeepnoteFile(contentString); + + return this.findDefaultNotebook(deepnoteFile)?.id; + } catch { + return undefined; + } + } + + /** + * Ensures an opened notebook document shows the correct notebook. + * + * The serializer returns an empty state when it cannot determine which + * notebook to display (no pending resolution). This method is called + * after the document is created, reads the real notebook ID from + * the URI `?notebook=` query param (or picks the default notebook + * for direct file opens), and patches the document in place. + */ + async verifyDeserializedNotebook(doc: NotebookDocument): Promise { + if (doc.notebookType !== 'deepnote') { + return; + } + + const expectedNotebookId = new URLSearchParams(doc.uri.query).get('notebook'); + const actualNotebookId = doc.metadata?.deepnoteNotebookId as string | undefined; + + if (actualNotebookId && (!expectedNotebookId || expectedNotebookId === actualNotebookId)) { + return; + } + + const cts = new CancellationTokenSource(); + + try { + const fileUri = doc.uri.with({ query: '', fragment: '' }); + const content = await workspace.fs.readFile(fileUri); + + const targetNotebookId = expectedNotebookId ?? this.resolveDefaultNotebookId(content); + + if (!targetNotebookId || targetNotebookId === actualNotebookId) { + return; + } + + logger.info( + `Notebook verification: resolving notebook ${targetNotebookId} (was ${actualNotebookId ?? 'empty'}).` + ); + + const correctData = await this.deserializeNotebook(content, cts.token, targetNotebookId); + + const edit = new WorkspaceEdit(); + + edit.set(doc.uri, [ + NotebookEdit.replaceCells(new NotebookRange(0, doc.cellCount), correctData.cells), + NotebookEdit.updateNotebookMetadata(correctData.metadata ?? {}) + ]); + + const applied = await workspace.applyEdit(edit); + + if (applied) { + await doc.save(); + } + } catch (error) { + logger.error('Failed to verify/correct notebook content', error); + } finally { + cts.dispose(); + } + } + /** * Serializes VS Code notebook data back to Deepnote YAML format. * Converts cells to blocks, updates project data, and generates YAML. @@ -271,6 +361,9 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { throw new Error('Cannot determine which notebook to save'); } + this.lastSerializedNotebookId = notebookId; + this.lastSerializedTimestamp = Date.now(); + logger.debug(`SerializeNotebook: Notebook ID: ${notebookId}`); const notebook = originalProject.project.notebooks.find((nb: { id: string }) => nb.id === notebookId); @@ -561,27 +654,39 @@ export class DeepnoteNotebookSerializer implements NotebookSerializer { } /** - * Extracts the notebook ID from the active tab's URI query params. - * During session restore or tab open, VS Code may set the active tab - * before calling deserializeNotebook. The URI retains the - * `?notebook=` query param set when the tab was originally opened. + * Returns a notebook ID from an already-open document for re-deserialization. + * + * When VS Code re-reads a file (e.g. after save or external change), it + * calls deserializeNotebook again for each open document backed by that + * file. The document already exists in workspace.notebookDocuments with + * the correct deepnoteNotebookId in its metadata. + * + * Skips the notebook that was most recently serialized — that document + * triggered the file change and is not the one being re-deserialized. */ - private findNotebookIdFromActiveTab(): string | undefined { - const activeTab = window.tabGroups.activeTabGroup?.activeTab; + private findNotebookIdFromOpenDocuments(projectId: string): string | undefined { + const recentSerialize = + this.lastSerializedNotebookId && Date.now() - this.lastSerializedTimestamp < LAST_SERIALIZED_TTL_MS; - if (!activeTab || !(activeTab.input instanceof TabInputNotebook)) { - return undefined; - } + for (const doc of workspace.notebookDocuments) { + if (doc.notebookType !== 'deepnote' || doc.metadata?.deepnoteProjectId !== projectId) { + continue; + } - const tabInput = activeTab.input; + const notebookId = doc.metadata?.deepnoteNotebookId as string | undefined; - if (tabInput.notebookType !== 'deepnote') { - return undefined; - } + if (!notebookId) { + continue; + } - const notebookId = new URLSearchParams(tabInput.uri.query).get('notebook'); + if (recentSerialize && notebookId === this.lastSerializedNotebookId) { + continue; + } + + return notebookId; + } - return notebookId ?? undefined; + return undefined; } /** diff --git a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts index df2973ccdf..02b6c369ce 100644 --- a/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts +++ b/src/notebooks/deepnote/deepnoteSerializer.unit.test.ts @@ -129,7 +129,7 @@ project: ); }); - test('should throw error when no notebooks found', async () => { + test('should return empty state when no notebooks found and no ID resolved', async () => { const contentWithoutNotebooks = new TextEncoder().encode(` version: '1.0.0' metadata: @@ -141,10 +141,11 @@ project: settings: {} `); - await assert.isRejected( - serializer.deserializeNotebook(contentWithoutNotebooks, {} as any), - /no notebooks|notebooks.*must contain at least 1/i - ); + const result = await serializer.deserializeNotebook(contentWithoutNotebooks, {} as any); + + assert.deepStrictEqual(result.cells, []); + assert.strictEqual(result.metadata?.deepnoteProjectId, 'project-123'); + assert.strictEqual(result.metadata?.deepnoteNotebookId, undefined); }); test('should deserialize the specified notebook when notebookId is passed', async () => { @@ -750,7 +751,7 @@ project: }); suite('default notebook selection', () => { - test('should not select Init notebook when other notebooks are available', async () => { + test('should not select Init notebook when other notebooks are available', () => { const projectData: DeepnoteFile = { version: '1.0.0', metadata: { @@ -800,14 +801,12 @@ project: }; const content = projectToYaml(projectData); - const result = await serializer.deserializeNotebook(content, {} as any); + const result = serializer.resolveDefaultNotebookId(content); - // Should select the Main notebook, not the Init notebook - assert.strictEqual(result.metadata?.deepnoteNotebookId, 'main-notebook'); - assert.strictEqual(result.metadata?.deepnoteNotebookName, 'Main'); + assert.strictEqual(result, 'main-notebook'); }); - test('should select Init notebook when it is the only notebook', async () => { + test('should select Init notebook when it is the only notebook', () => { const projectData: DeepnoteFile = { version: '1.0.0', metadata: { @@ -841,14 +840,12 @@ project: }; const content = projectToYaml(projectData); - const result = await serializer.deserializeNotebook(content, {} as any); + const result = serializer.resolveDefaultNotebookId(content); - // Should select the Init notebook since it's the only one - assert.strictEqual(result.metadata?.deepnoteNotebookId, 'init-notebook'); - assert.strictEqual(result.metadata?.deepnoteNotebookName, 'Init'); + assert.strictEqual(result, 'init-notebook'); }); - test('should select alphabetically first notebook when no initNotebookId', async () => { + test('should select alphabetically first notebook when no initNotebookId', () => { const projectData: DeepnoteFile = { version: '1.0.0', metadata: { @@ -913,14 +910,12 @@ project: }; const content = projectToYaml(projectData); - const result = await serializer.deserializeNotebook(content, {} as any); + const result = serializer.resolveDefaultNotebookId(content); - // Should select the alphabetically first notebook - assert.strictEqual(result.metadata?.deepnoteNotebookId, 'alpha-notebook'); - assert.strictEqual(result.metadata?.deepnoteNotebookName, 'Alpha Notebook'); + assert.strictEqual(result, 'alpha-notebook'); }); - test('should sort Init notebook last when multiple notebooks exist', async () => { + test('should sort Init notebook last when multiple notebooks exist', () => { const projectData: DeepnoteFile = { version: '1.0.0', metadata: { @@ -986,11 +981,19 @@ project: }; const content = projectToYaml(projectData); - const result = await serializer.deserializeNotebook(content, {} as any); + const result = serializer.resolveDefaultNotebookId(content); // Should select Alpha, not Init even though "Init" comes before "Alpha" alphabetically when in upper case - assert.strictEqual(result.metadata?.deepnoteNotebookId, 'alpha-notebook'); - assert.strictEqual(result.metadata?.deepnoteNotebookName, 'Alpha'); + assert.strictEqual(result, 'alpha-notebook'); + }); + + test('should return empty state from deserializeNotebook when no notebook ID is resolved', async () => { + const content = projectToYaml(mockProject); + const result = await serializer.deserializeNotebook(content, {} as any); + + assert.deepStrictEqual(result.cells, []); + assert.strictEqual(result.metadata?.deepnoteProjectId, 'project-123'); + assert.strictEqual(result.metadata?.deepnoteNotebookId, undefined); }); });