fix: Fix deepnote notebook deserializer and file change watcher#363
Open
fix: Fix deepnote notebook deserializer and file change watcher#363
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR centralizes and makes notebook resolution explicit and changes file sync persistence for Deepnote notebooks. deserializeNotebook now accepts an optional notebookId and uses queued pending resolutions or current/open notebooks to resolve selection; serialization uses the manager’s current notebook ID and may queue a resolution when no open notebooks exist. A per-project pending resolution queue with TTL was added to the manager (queue/consume semantics and clear selection). File-change handling was refactored to apply notebook edits via a helper, normalize file URIs for self-write tracking, and write canonical bytes before saving. Tests and explorer open flows updated accordingly. Sequence DiagramsequenceDiagram
participant Watcher as FileChangeWatcher
participant Manager as NotebookManager
participant Serializer as NotebookSerializer
participant Workspace as WorkspaceFS
Note over Watcher,Manager: File change detected for project
Watcher->>Manager: consumePendingNotebookResolution(projectId)
alt pending resolution present
Manager-->>Watcher: notebookId
else no pending hint
Manager-->>Watcher: undefined
end
Watcher->>Serializer: deserializeNotebook(bytes, token, notebookId?)
Serializer->>Manager: getCurrentNotebookId(projectId) [if no notebookId]
Manager-->>Serializer: currentNotebookId|undefined
Serializer-->>Watcher: NotebookDocument edits/metadata
Watcher->>Workspace: applyNotebookEdits(uri, edits)
Workspace-->>Watcher: edits applied
Watcher->>Serializer: serializeNotebook(document)
Serializer->>Manager: getCurrentNotebookId(projectId)
Serializer-->>Watcher: bytes
Watcher->>Workspace: writeFile(fileUri, bytes)
alt write success
Watcher->>Watcher: markSelfWrite(normalizeFileUri(fileUri))
Watcher->>Workspace: save(notebookUri)
else write failure
Watcher->>Watcher: consumeSelfWrite(normalizeFileUri(fileUri))
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #363 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
tkislan
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 356-373: The writeFile catch path allows execution to continue to
the subsequent markSelfWrite and workspace.save calls which assume the file was
written; modify the write failure handler in deepnoteFileChangeWatcher (around
workspace.fs.writeFile) to return early after calling
this.consumeSelfWrite(fileUri) and logging the warning so we do not proceed to
markSelfWrite or call workspace.save on a failed write; ensure markSelfWrite and
consumeSelfWrite usage remains correct and only the successful-write path
reaches the save logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aacbc659-5d97-4eeb-9ad4-a231351e6a81
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteFileChangeWatcher.ts
…mtime conflicts during workspace saves
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (1)
336-373:⚠️ Potential issue | 🔴 CriticalGuard the canonical rewrite against a second external save.
A later disk write between the initial
readFile()and Line 358 gets overwritten here by the older snapshot we already deserialized. The queued follow-up sync will reread our overwrite, not the newer external bytes. Re-check the file immediately beforewriteFile(), and skip the canonical rewrite/save path if it changed again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts` around lines 336 - 373, Between the initial read and the canonical rewrite you must guard against a second external save: before calling workspace.fs.writeFile(fileUri, serializedBytes) (inside the try where serializeTokenSource and serializedBytes are prepared), re-check the file's current mtime or contents (e.g., await workspace.fs.stat(fileUri) or readFile) and compare to the snapshot used to produce serializedBytes (newData / original mtime); if the on-disk file has changed since we deserialized, call this.consumeSelfWrite(fileUri) as needed and skip the canonical write/save path (return early) so we don't overwrite a newer external change—make this check before markSelfWrite(fileUri) and before workspace.save(notebook.uri), keeping serializeTokenSource, serializeNotebook, markSelfWrite, consumeSelfWrite, workspace.fs.writeFile and workspace.save calls intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 557-564: The private helper normalizeFileUri(uri: Uri): string is
correctly implemented but is misplaced; move the normalizeFileUri method so it
follows the file's private-member alphabetical ordering (i.e., relocate it among
other private methods/fields to the position determined by its name) so the
class groups private members by accessibility then alphabetically, keeping its
implementation intact and updating surrounding spacing/comments as needed.
- Around line 371-377: When calling workspace.save(notebook.uri) in the
deepnoteFileChangeWatcher, handle the case where it resolves to undefined
(indicating save failed or no editor) instead of only catching exceptions: after
await workspace.save(notebook.uri) check the return value and if it is undefined
call this.consumeSelfWrite(fileUri) and log a warning (same as in the catch) so
the armed self-write marker set by this.markSelfWrite(fileUri) is cleared
immediately; ensure both the error catch block and the "undefined" branch clear
the marker to avoid dropping real FS events.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 336-373: Between the initial read and the canonical rewrite you
must guard against a second external save: before calling
workspace.fs.writeFile(fileUri, serializedBytes) (inside the try where
serializeTokenSource and serializedBytes are prepared), re-check the file's
current mtime or contents (e.g., await workspace.fs.stat(fileUri) or readFile)
and compare to the snapshot used to produce serializedBytes (newData / original
mtime); if the on-disk file has changed since we deserialized, call
this.consumeSelfWrite(fileUri) as needed and skip the canonical write/save path
(return early) so we don't overwrite a newer external change—make this check
before markSelfWrite(fileUri) and before workspace.save(notebook.uri), keeping
serializeTokenSource, serializeNotebook, markSelfWrite, consumeSelfWrite,
workspace.fs.writeFile and workspace.save calls intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0bec955-fde1-408e-9887-8738d66b52bc
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteFileChangeWatcher.ts
…out block IDs and implement a valid project structure for testing
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 1130-1139: The test currently asserts nfApplyEditCount === 0
immediately after detecting readSnapshot ran, which can miss an applyEdit
scheduled a tick later; after calling noFallbackOnDidChange.fire(...) and
awaiting waitFor(() => nfReadSnapshotCount >= 1), add a short post-read grace
wait (e.g. a small setTimeout delay or a brief waitFor that ensures no further
nfApplyEditCount increments occur) before asserting nfApplyEditCount is 0 so any
asynchronously scheduled applyEdit has time to run and be counted; target
symbols: noFallbackOnDidChange.fire, nfReadSnapshotCount, nfApplyEditCount,
readSnapshot, applyEdit, and waitFor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 660447d0-1752-4128-8450-fefd098bb6a5
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts
…ration result and log warnings for undefined saves
…ngeWatcher to ensure proper handling of snapshot reads
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts (1)
1080-1143: 🧹 Nitpick | 🔵 TrivialDispose the ad-hoc watcher setup in
finally.If
waitFor()or the assertion fails, this extra watcher and its emitters leak into later tests.Proposed fix
- 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'); - - for (const d of noFallbackDisposables) { - d.dispose(); - } - noFallbackOnDidChange.dispose(); - noFallbackOnDidCreate.dispose(); + try { + 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'); + } finally { + for (const d of noFallbackDisposables) { + d.dispose(); + } + noFallbackOnDidChange.dispose(); + noFallbackOnDidCreate.dispose(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts` around lines 1080 - 1143, The test creates ephemeral watchers and emitters (noFallbackOnDidChange, noFallbackOnDidCreate, noFallbackDisposables, nfWatcher) but only disposes them at the end of the happy path, risking leaks if waitFor/assertions throw; wrap the activation, firing and assertions in a try/finally and in finally call nfWatcher.dispose() or nfWatcher.deactivate() (whichever exists), dispose all entries in noFallbackDisposables, and call noFallbackOnDidChange.dispose() and noFallbackOnDidCreate.dispose() to ensure the ad-hoc watcher setup is always cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 355-381: The second call to markSelfWrite should be removed so we
only arm selfWriteCounts for the explicit workspace.fs.writeFile operation;
locate the block where markSelfWrite(fileUri) is called right before awaiting
workspace.save(notebook.uri) (paired with consumeSelfWrite(fileUri) on failures)
and delete that second markSelfWrite invocation, keeping the consumeSelfWrite
calls on the save error/unsaved paths and leaving the first markSelfWrite before
workspace.fs.writeFile and the contentActuallyChanged handling unchanged.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 1080-1143: The test creates ephemeral watchers and emitters
(noFallbackOnDidChange, noFallbackOnDidCreate, noFallbackDisposables, nfWatcher)
but only disposes them at the end of the happy path, risking leaks if
waitFor/assertions throw; wrap the activation, firing and assertions in a
try/finally and in finally call nfWatcher.dispose() or nfWatcher.deactivate()
(whichever exists), dispose all entries in noFallbackDisposables, and call
noFallbackOnDidChange.dispose() and noFallbackOnDidCreate.dispose() to ensure
the ad-hoc watcher setup is always cleaned up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1db496a-5f85-4322-b73b-faa657bc8a9d
📒 Files selected for processing (2)
src/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (1)
559-566: 🧹 Nitpick | 🔵 Trivial
normalizeFileUriis misplaced — should follow alphabetical ordering.Implementation is correct. Position is not. Should come after
markSelfWriteand beforeoutputsMatch.As per coding guidelines: "Order method, fields and properties, first by accessibility and then by alphabetical order."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts` around lines 559 - 566, Move the private method normalizeFileUri so it follows the existing private method markSelfWrite and comes immediately before outputsMatch to satisfy the alphabetical ordering rule; keep the method signature and accessibility (private normalizeFileUri(uri: Uri): string) unchanged and only relocate its declaration within the class so ordering is markSelfWrite -> normalizeFileUri -> outputsMatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 559-566: Move the private method normalizeFileUri so it follows
the existing private method markSelfWrite and comes immediately before
outputsMatch to satisfy the alphabetical ordering rule; keep the method
signature and accessibility (private normalizeFileUri(uri: Uri): string)
unchanged and only relocate its declaration within the class so ordering is
markSelfWrite -> normalizeFileUri -> outputsMatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 341457a5-34c2-4736-9dd1-1f1af080e78e
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteFileChangeWatcher.ts
… 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
97-110:⚠️ Potential issue | 🟠 MajorRecover from stale remembered notebook IDs.
Now that manager state wins first, a deleted or renamed notebook leaves
resolvedNotebookIdtruthy but unmatched, and this path throws instead of falling back. That can make a fresh open fail until the cached selection is cleared.💡 Minimal fix
- const selectedNotebook = resolvedNotebookId - ? deepnoteFile.project.notebooks.find((nb) => nb.id === resolvedNotebookId) - : this.findDefaultNotebook(deepnoteFile); + let selectedNotebook = resolvedNotebookId + ? deepnoteFile.project.notebooks.find((nb) => nb.id === resolvedNotebookId) + : undefined; + + if (!selectedNotebook && notebookId === undefined) { + if ( + resolvedNotebookId && + this.notebookManager.getTheSelectedNotebookForAProject(projectId) === resolvedNotebookId + ) { + logger.warn( + `DeepnoteSerializer: Notebook ${resolvedNotebookId} no longer exists in project ${projectId}; falling back` + ); + this.notebookManager.clearNotebookSelection(projectId); + } + selectedNotebook = this.findDefaultNotebook(deepnoteFile); + } if (!selectedNotebook) { throw new Error(l10n.t('No notebook selected or found')); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteSerializer.ts` around lines 97 - 110, The code currently throws when a truthy resolvedNotebookId doesn't match any notebook (stale selection); update the selection logic in DeepnoteSerializer so that after computing resolvedNotebookId (from findCurrentNotebookId), if resolvedNotebookId is set but deepnoteFile.project.notebooks.find(...) returns undefined, fall back to this.findDefaultNotebook(deepnoteFile) instead of throwing; ensure selectedNotebook is assigned that fallback and only throw if no notebook is found after the fallback, referencing resolvedNotebookId, selectedNotebook, findCurrentNotebookId, deepnoteFile, and findDefaultNotebook.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 238-247: The unconditional call to
this.notebookManager.clearNotebookSelection(projectId) in
deepnoteFileChangeWatcher.ts removes the user's remembered per-project notebook
choice; instead either remove this clear or replace it with a non-destructive
approach: add a scoped/signal flag to NotebookManager (e.g.,
suppressSelectionLookupForSync or pass a syncContext parameter into
selectNotebookForProject / lookup methods) so the file-change sync can ignore
the stored selection without deleting it, and update the affected code path that
iterates affectedNotebooks to use that flag/parameter rather than calling
clearNotebookSelection(projectId).
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteSerializer.ts`:
- Around line 97-110: The code currently throws when a truthy resolvedNotebookId
doesn't match any notebook (stale selection); update the selection logic in
DeepnoteSerializer so that after computing resolvedNotebookId (from
findCurrentNotebookId), if resolvedNotebookId is set but
deepnoteFile.project.notebooks.find(...) returns undefined, fall back to
this.findDefaultNotebook(deepnoteFile) instead of throwing; ensure
selectedNotebook is assigned that fallback and only throw if no notebook is
found after the fallback, referencing resolvedNotebookId, selectedNotebook,
findCurrentNotebookId, deepnoteFile, and findDefaultNotebook.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 495a5709-4444-44a8-8593-2f7e3d652f86
📒 Files selected for processing (7)
src/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/types.ts
…gement - 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts (1)
1141-1204: 🧹 Nitpick | 🔵 TrivialGuard the extra watcher cleanup with
try/finally.This test only disposes its local watcher and emitters after the assertions. If it fails early, those listeners bleed into later cases and turn one failure into a cascade.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts` around lines 1141 - 1204, The test leaks its local listeners when assertions fail because the disposal of noFallbackDisposables, noFallbackOnDidChange, and noFallbackOnDidCreate happens after assertions; wrap the test body (from firing noFallbackOnDidChange through the asserts) in a try/finally and move the cleanup into the finally block so you always call d.dispose() for each item in noFallbackDisposables and call noFallbackOnDidChange.dispose() and noFallbackOnDidCreate.dispose() even if an assertion throws; keep the existing waitFor and timing logic inside the try so the finally always runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Around line 405-410: The loop that disposes entries from disposablesByFile for
a given fileKey should guard each call so one thrown error doesn't abort
remaining disposals; in the block where you iterate existing (for (const d of
existing) { d.dispose(); }), wrap each d.dispose() in a try/catch and handle/log
the error (e.g. using this.logger.error or console.error) while continuing the
loop so all disposables are attempted to be cleaned up.
In `@src/kernels/deepnote/deepnoteTestHelpers.node.ts`:
- Line 56: The hardcoded magic number 10 used in the getMaxListeners mock should
be extracted to a named constant; define something like DEFAULT_MAX_LISTENERS =
10 near the top of this module and replace the literal in getMaxListeners with
the constant to follow the guideline for extracting magic numbers (update the
exported/used mocks that reference getMaxListeners if any).
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 1353-1398: The test currently only asserts that
mockedNotebookManager.clearNotebookSelection('project-1') was called at some
point, which doesn't ensure it happened before the first sync; modify the test
to record call order and assert clearNotebookSelection occurs before the first
applyEdit invocation: capture call timestamps/order (e.g. by spying on
mockedNotebookManager.clearNotebookSelection and the function that increments
applyEditCount or on applyEdit itself), fire onDidChangeFile, await waitFor(()
=> applyEditCount >= 2), and then assert the clearNotebookSelection call index
is less than the first applyEdit call index (or use a call order verifier) so
the clear happens before the initial applyEdit/sync.
In `@src/test/mocks/vsc/extHostedTypes.ts`:
- Around line 798-818: The mock leaves entries in _textEdits/_notebookEdits when
clearing edits, causing has()/size to report a dirty URI; instead of setting
data.edits = undefined or [] for the cases where edits is undefined or
edits.length === 0, remove the entire entry from _textEdits
(this._textEdits.delete(uriKey)) and also remove any notebook entry
(this._notebookEdits.delete(uriKey)) so the maps reflect a cleared URI; update
the logic around the existing uriKey/data/_seqPool handling to delete rather
than retain empty or undefined edit records.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 1141-1204: The test leaks its local listeners when assertions fail
because the disposal of noFallbackDisposables, noFallbackOnDidChange, and
noFallbackOnDidCreate happens after assertions; wrap the test body (from firing
noFallbackOnDidChange through the asserts) in a try/finally and move the cleanup
into the finally block so you always call d.dispose() for each item in
noFallbackDisposables and call noFallbackOnDidChange.dispose() and
noFallbackOnDidCreate.dispose() even if an assertion throws; keep the existing
waitFor and timing logic inside the try so the finally always runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aad6e30f-cffd-486f-b4b5-75193eeb82a0
📒 Files selected for processing (4)
src/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/deepnote/deepnoteTestHelpers.node.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.tssrc/test/mocks/vsc/extHostedTypes.ts
Comment on lines
+1353
to
+1398
| 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(); | ||
| }); |
Contributor
There was a problem hiding this comment.
This does not verify “before.”
verify(...).once() only proves clearNotebookSelection('project-1') happened sometime. If it runs after the first applyEdit(), this test still passes and the stale-selection regression slips through. Record call order and assert the clear happens before the first sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts` around lines
1353 - 1398, The test currently only asserts that
mockedNotebookManager.clearNotebookSelection('project-1') was called at some
point, which doesn't ensure it happened before the first sync; modify the test
to record call order and assert clearNotebookSelection occurs before the first
applyEdit invocation: capture call timestamps/order (e.g. by spying on
mockedNotebookManager.clearNotebookSelection and the function that increments
applyEditCount or on applyEdit itself), fire onDidChangeFile, await waitFor(()
=> applyEditCount >= 2), and then assert the clearNotebookSelection call index
is less than the first applyEdit call index (or use a call order verifier) so
the clear happens before the initial applyEdit/sync.
- 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.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (1)
245-254:⚠️ Potential issue | 🟠 MajorDon't turn sync into a persistent selection reset.
clearNotebookSelection()deletes the remembered per-project notebook choice, and this flow never restores it. The sync itself already bypasses shared selection state at Line 305 by passingdeepnoteNotebookIddirectly intodeserializeNotebook(...), so this mutation only discards the user's choice. If VS Code-driven reloads still need different lookup rules, make that a scoped override on lookup instead of deleting the saved selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts` around lines 245 - 254, The current loop calls this.notebookManager.clearNotebookSelection(projectId) for the first affected notebook which permanently deletes the per-project selection; remove that mutation so we don't discard the user's saved choice. Instead, stop calling clearNotebookSelection in deepnoteFileChangeWatcher; when a VS Code-triggered re-deserialization needs to bypass shared selection, use the existing mechanism of passing deepnoteNotebookId into deserializeNotebook(...) (and, if needed, add a scoped lookup override in the notebookManager lookup path) rather than clearing state. Ensure references: affectedNotebooks loop, clearNotebookSelection, deserializeNotebook(deepnoteNotebookId), and notebookManager are used to locate and adjust the change.src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts (1)
1352-1397:⚠️ Potential issue | 🟡 MinorThis still doesn't prove “before.”
verify(clearNotebookSelection('project-1')).once()only shows the call happened sometime. The test still passes if the clear runs after the first sync edit. Record call order against the firstapplyNotebookEdits()invocation and assert the clear happens earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts` around lines 1352 - 1397, The test currently only verifies clearNotebookSelection('project-1') was called, not that it happened before applyNotebookEdits; to fix this, record call ordering by stubbing mockedNotebookManager.clearNotebookSelection and mockedNotebookManager.applyNotebookEdits to push markers into a local callOrder array when invoked (e.g., when(mockedNotebookManager.clearNotebookSelection).thenCall(() => { callOrder.push('clear'); ... }) and similarly for applyNotebookEdits pushing 'apply' while still preserving their original behavior and applyEditCount updates), then after the watcher runs assert callOrder.indexOf('clear') < callOrder.indexOf('apply') in addition to the existing verify on clearNotebookSelection so the test proves the clear happened before the first applyNotebookEdits call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 245-254: The current loop calls
this.notebookManager.clearNotebookSelection(projectId) for the first affected
notebook which permanently deletes the per-project selection; remove that
mutation so we don't discard the user's saved choice. Instead, stop calling
clearNotebookSelection in deepnoteFileChangeWatcher; when a VS Code-triggered
re-deserialization needs to bypass shared selection, use the existing mechanism
of passing deepnoteNotebookId into deserializeNotebook(...) (and, if needed, add
a scoped lookup override in the notebookManager lookup path) rather than
clearing state. Ensure references: affectedNotebooks loop,
clearNotebookSelection, deserializeNotebook(deepnoteNotebookId), and
notebookManager are used to locate and adjust the change.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 1352-1397: The test currently only verifies
clearNotebookSelection('project-1') was called, not that it happened before
applyNotebookEdits; to fix this, record call ordering by stubbing
mockedNotebookManager.clearNotebookSelection and
mockedNotebookManager.applyNotebookEdits to push markers into a local callOrder
array when invoked (e.g.,
when(mockedNotebookManager.clearNotebookSelection).thenCall(() => {
callOrder.push('clear'); ... }) and similarly for applyNotebookEdits pushing
'apply' while still preserving their original behavior and applyEditCount
updates), then after the watcher runs assert callOrder.indexOf('clear') <
callOrder.indexOf('apply') in addition to the existing verify on
clearNotebookSelection so the test proves the clear happened before the first
applyNotebookEdits call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f8aa2fa-2114-45bb-8691-306a85b05b5a
📒 Files selected for processing (2)
src/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts
dinohamzic
previously approved these changes
Mar 30, 2026
Contributor
dinohamzic
left a comment
dinohamzic
left a comment
There was a problem hiding this comment.
Nice, this seems to be doing the trick. 🚀
Code looks good overall, but I don't have enough VS Code extension specific knowledge to get into the details.
…ling - 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/notebooks/deepnote/deepnoteNotebookManager.ts (1)
167-177: 🧹 Nitpick | 🔵 TrivialMethod ordering:
hasInitNotebookBeenRunandmarkInitNotebookAsRunshould precedequeueNotebookResolution.Alphabetically:
has...<mark...<queue....🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteNotebookManager.ts` around lines 167 - 177, Reorder the methods so hasInitNotebookBeenRun and markInitNotebookAsRun appear before queueNotebookResolution in the DeepnoteNotebookManager class: move the hasInitNotebookBeenRun(projectId: string): boolean and markInitNotebookAsRun(projectId: string): void method definitions to precede the queueNotebookResolution method to match the requested alphabetical ordering and logical grouping.src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
309-457: 🛠️ Refactor suggestion | 🟠 MajorExtract a
NotebookDocumentfixture helper.These resolver tests duplicate the same fake
NotebookDocumentshape four times. One helper would make API-shape changes much cheaper and reduce fixture drift.As per coding guidelines, "Extract duplicate logic into helper methods to prevent drift following DRY principle".Suggested refactor
-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; +const notebookA = createNotebookDocument('project-123', 'notebook-a'); +const notebookB = createNotebookDocument('project-123', 'notebook-b');function createNotebookDocument(projectId: string, notebookId: string): NotebookDocument { return { then: undefined, notebookType: 'deepnote', metadata: { deepnoteProjectId: projectId, deepnoteNotebookId: notebookId }, uri: {} as any, version: 1, isDirty: false, isUntitled: false, isClosed: false, cellCount: 0, cellAt: () => ({}) as any, getCells: () => [], save: async () => true } as NotebookDocument; }
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 520-523: Move the private method registerNotebookOpenIntent so
that it appears alphabetically after refreshExplorer; specifically reorder the
methods in the class so refreshExplorer comes first and
registerNotebookOpenIntent (the method that calls
this.manager.queueNotebookResolution and this.manager.selectNotebookForProject)
immediately follows, preserving the method body and visibility.
In `@src/notebooks/deepnote/deepnoteSerializer.ts`:
- Around line 556-569: The method findOpenNotebookIds is out of the requested
alphabetical ordering and should be moved to follow findNotebookUri; relocate
the entire private method findOpenNotebookIds so it comes immediately after the
findNotebookUri method (keeping its implementation unchanged) to satisfy the
alphabetical ordering rule (findDefault... < findNotebook... < findOpen...).
In `@src/notebooks/deepnote/deepnoteSerializer.unit.test.ts`:
- Around line 254-258: The test currently asserts notebook contents by array
index which couples correctness to ordering; update the assertions to locate
notebooks by their unique id instead. After calling
serializer.serializeNotebook(...) and deserializeDeepnoteFile(...), find the
notebook object in serializedProject.project.notebooks whose id matches the
expected id from mockNotebookData (use the id fields on
mockNotebookData.notebooks) and then assert that that
notebook.blocks?.[0].content equals the expected string; reference
serializer.serializeNotebook, deserializeDeepnoteFile, mockNotebookData and
serializedProject.project.notebooks to locate the code to change.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteNotebookManager.ts`:
- Around line 167-177: Reorder the methods so hasInitNotebookBeenRun and
markInitNotebookAsRun appear before queueNotebookResolution in the
DeepnoteNotebookManager class: move the hasInitNotebookBeenRun(projectId:
string): boolean and markInitNotebookAsRun(projectId: string): void method
definitions to precede the queueNotebookResolution method to match the requested
alphabetical ordering and logical grouping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b99eb80-8229-42c1-a45f-8a875f67b96e
📒 Files selected for processing (9)
src/kernels/deepnote/deepnoteServerStarter.node.tssrc/notebooks/deepnote/deepnoteExplorerView.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/types.ts
Comment on lines
+520
to
+523
| private registerNotebookOpenIntent(projectId: string, notebookId: string): void { | ||
| this.manager.queueNotebookResolution(projectId, notebookId); | ||
| this.manager.selectNotebookForProject(projectId, notebookId); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Method ordering: registerNotebookOpenIntent should follow refreshExplorer alphabetically.
Private methods should be ordered alphabetically. register... comes after refresh....
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 520 - 523, Move
the private method registerNotebookOpenIntent so that it appears alphabetically
after refreshExplorer; specifically reorder the methods in the class so
refreshExplorer comes first and registerNotebookOpenIntent (the method that
calls this.manager.queueNotebookResolution and
this.manager.selectNotebookForProject) immediately follows, preserving the
method body and visibility.
Comment on lines
+556
to
+569
| 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) | ||
| ) | ||
| ]; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Method ordering: findOpenNotebookIds should come after findNotebookUri.
Alphabetically: findDefault... < findNotebook... < findOpen....
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteSerializer.ts` around lines 556 - 569, The
method findOpenNotebookIds is out of the requested alphabetical ordering and
should be moved to follow findNotebookUri; relocate the entire private method
findOpenNotebookIds so it comes immediately after the findNotebookUri method
(keeping its implementation unchanged) to satisfy the alphabetical ordering rule
(findDefault... < findNotebook... < findOpen...).
Comment on lines
+254
to
+258
| 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'); |
Contributor
There was a problem hiding this comment.
Assert by notebook ID, not array position.
This test is about notebook selection, but the current assertions also lock in project.notebooks ordering. A harmless reorder would fail the test for the wrong reason.
Suggested change
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');
+const firstNotebook = serializedProject.project.notebooks.find((notebook) => notebook.id === 'notebook-1');
+const secondNotebook = serializedProject.project.notebooks.find((notebook) => notebook.id === 'notebook-2');
+
+assert.strictEqual(firstNotebook?.blocks?.[0].content, 'print("hello")');
+assert.strictEqual(secondNotebook?.blocks?.[0].content, '# Updated second notebook');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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'); | |
| const result = await serializer.serializeNotebook(mockNotebookData as any, {} as any); | |
| const serializedProject = deserializeDeepnoteFile(new TextDecoder().decode(result)); | |
| const firstNotebook = serializedProject.project.notebooks.find((notebook) => notebook.id === 'notebook-1'); | |
| const secondNotebook = serializedProject.project.notebooks.find((notebook) => notebook.id === 'notebook-2'); | |
| assert.strictEqual(firstNotebook?.blocks?.[0].content, 'print("hello")'); | |
| assert.strictEqual(secondNotebook?.blocks?.[0].content, '# Updated second notebook'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteSerializer.unit.test.ts` around lines 254 -
258, The test currently asserts notebook contents by array index which couples
correctness to ordering; update the assertions to locate notebooks by their
unique id instead. After calling serializer.serializeNotebook(...) and
deserializeDeepnoteFile(...), find the notebook object in
serializedProject.project.notebooks whose id matches the expected id from
mockNotebookData (use the id fields on mockNotebookData.notebooks) and then
assert that that notebook.blocks?.[0].content equals the expected string;
reference serializer.serializeNotebook, deserializeDeepnoteFile,
mockNotebookData and serializedProject.project.notebooks to locate the code to
change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features
Tests