fix: Fix deepnote notebook deserializer and file change watcher#363
Open
fix: Fix deepnote notebook deserializer and file change watcher#363
Conversation
Contributor
📝 WalkthroughWalkthroughdeserializeNotebook gained an optional Sequence Diagram(s)sequenceDiagram
participant Watcher as FileChangeWatcher
participant Manager as NotebookManager
participant Serializer as DeepnoteSerializer
participant Workspace as WorkspaceFS
Note over Watcher,Manager: Snapshot or main-file FS change triggers sync
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?)
alt explicit notebookId provided
Serializer-->>Watcher: use provided notebookId
else omitted
Serializer->>Manager: findCurrentNotebookId(projectId, projectNotebookIds)
Manager-->>Serializer: resolvedNotebookId | undefined
Serializer-->>Watcher: resolvedNotebookId (or undefined)
end
Serializer-->>Watcher: produce Notebook edits/metadata
Watcher->>Workspace: applyNotebookEdits(uri, edits)
Workspace-->>Watcher: edits applied (success/fail)
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
alt no open notebooks for project after serialize
Serializer->>Manager: queueNotebookResolution(projectId, notebookId)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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.
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
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)
550-557: 🧹 Nitpick | 🔵 TrivialMissing self-write consumption if
workspace.savethrows.Line 555 consumes, but then immediately re-throws. The caller (
drainQueue) catches and logs but doesn't propagate. This is fine, but the throw after consume is slightly unusual—could just log and return.Consider returning instead of throwing
} catch (error) { this.consumeSelfWrite(notebook.uri); - throw error; + logger.warn(`[FileChangeWatcher] Save after snapshot update failed: ${notebook.uri.path}`, error); + return; }🤖 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 550 - 557, In the save block inside deepnoteFileChangeWatcher, ensure self-write is always consumed but don't re-throw from the catch — after calling this.consumeSelfWrite(notebook.uri) on workspace.save failure, log the error and return (or otherwise swallow it) instead of re-throwing so drainQueue doesn't get unusual thrown errors; locate the try/catch around workspace.save and replace the "throw error" behavior with logging and an early return while keeping calls to this.markSelfWrite and this.consumeSelfWrite.
♻️ Duplicate comments (3)
src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (2)
566-573: 🧹 Nitpick | 🔵 Trivial
normalizeFileUriplacement breaks alphabetical order.Should appear between
markSelfWriteandoutputsMatch. Already flagged in past review.🤖 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 566 - 573, The normalizeFileUri method is out of alphabetical order; move the private method normalizeFileUri so it appears immediately after markSelfWrite and before outputsMatch to restore the correct alphabetical ordering of methods (keep the method signature and implementation unchanged, just relocate it between markSelfWrite and outputsMatch).
381-392:⚠️ Potential issue | 🟠 MajorDouble
markSelfWritemay cause marker leak if events coalesce.Past review flagged this:
writeFile(line 366) andworkspace.save(line 381) both arm self-write. If FileSystemWatcher coalesces them into one event, the second marker persists 30s, suppressing the next real external change.Consider keeping only the
writeFilemarker and lettingworkspace.saveevents pass throughcontentActuallyChanged().Proposed fix
// 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); }🤖 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 381 - 392, The problem is duplicate self-write markers: markSelfWrite is called both in writeFile and again around workspace.save which can leave an extra marker if file watcher coalesces events; remove the second call to markSelfWrite inside the workspace.save try/catch (and only call consumeSelfWrite on errors or when you know you created a marker) so that writeFile is the single place that arms the marker and workspace.save events are allowed to be processed by contentActuallyChanged(); update uses of consumeSelfWrite in the save error path to only clear markers actually created by writeFile to avoid leaking markers.src/notebooks/deepnote/deepnoteSerializer.ts (1)
557-573: 🧹 Nitpick | 🔵 TrivialMethod ordering:
findOpenNotebookIdsshould followfindNotebookUrialphabetically.Per coding guidelines, methods ordered by accessibility then alphabetically.
findOpen...>findNotebook....🤖 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 557 - 573, Move the private method findOpenNotebookIds so it appears alphabetically after findNotebookUri within the class, keeping its private accessibility and current implementation unchanged; locate the definitions for findNotebookUri and findOpenNotebookIds in deepnoteSerializer.ts and reorder them so the methods are grouped by accessibility and sorted alphabetically (findNotebookUri then findOpenNotebookIds).
🤖 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/deepnoteSerializer.ts`:
- Around line 558-572: Return currently spreads a Set created by
workspace.notebookDocuments.reduce into an array; for clarity replace the final
spread with Array.from to convert the Set to an array. Locate the reducer that
iterates over workspace.notebookDocuments (checking doc.notebookType,
doc.metadata.deepnoteProjectId, and parsing doc.metadata.deepnoteNotebookId) and
change the return from [...<Set>] to Array.from(<Set>) so the intent is clearer
while keeping the same logic.
- Around line 196-214: The code currently calls
notebookManager.consumePendingNotebookResolution(projectId) which removes the
pending hint before validation; change this to first peek non-destructively
(e.g. notebookManager.peekPendingNotebookResolution(projectId) or
notebookManager.getPendingNotebookResolution(projectId)) to obtain
pendingNotebookId, then perform the existence/validation checks and only call
notebookManager.consumePendingNotebookResolution(projectId) to remove it after
the notebook is confirmed valid; update findCurrentNotebookId to use peek/get
first and consume only on successful validation so the hint isn't lost on
validation failure.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 550-557: In the save block inside deepnoteFileChangeWatcher,
ensure self-write is always consumed but don't re-throw from the catch — after
calling this.consumeSelfWrite(notebook.uri) on workspace.save failure, log the
error and return (or otherwise swallow it) instead of re-throwing so drainQueue
doesn't get unusual thrown errors; locate the try/catch around workspace.save
and replace the "throw error" behavior with logging and an early return while
keeping calls to this.markSelfWrite and this.consumeSelfWrite.
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 566-573: The normalizeFileUri method is out of alphabetical order;
move the private method normalizeFileUri so it appears immediately after
markSelfWrite and before outputsMatch to restore the correct alphabetical
ordering of methods (keep the method signature and implementation unchanged,
just relocate it between markSelfWrite and outputsMatch).
- Around line 381-392: The problem is duplicate self-write markers:
markSelfWrite is called both in writeFile and again around workspace.save which
can leave an extra marker if file watcher coalesces events; remove the second
call to markSelfWrite inside the workspace.save try/catch (and only call
consumeSelfWrite on errors or when you know you created a marker) so that
writeFile is the single place that arms the marker and workspace.save events are
allowed to be processed by contentActuallyChanged(); update uses of
consumeSelfWrite in the save error path to only clear markers actually created
by writeFile to avoid leaking markers.
In `@src/notebooks/deepnote/deepnoteSerializer.ts`:
- Around line 557-573: Move the private method findOpenNotebookIds so it appears
alphabetically after findNotebookUri within the class, keeping its private
accessibility and current implementation unchanged; locate the definitions for
findNotebookUri and findOpenNotebookIds in deepnoteSerializer.ts and reorder
them so the methods are grouped by accessibility and sorted alphabetically
(findNotebookUri then findOpenNotebookIds).
🪄 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: e4305fa8-0bd2-42d0-b46f-76b9bab46f7f
📒 Files selected for processing (2)
src/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteSerializer.ts
…pnoteNotebookManager - 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.
…Watcher - 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.
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/deepnoteNotebookManager.ts (1)
179-189: 🧹 Nitpick | 🔵 TrivialMethod ordering:
hasInitNotebookBeenRunandmarkInitNotebookAsRunare misplaced.Alphabetically:
hasInit...<markInit...<queue.... Move them beforequeueNotebookResolution.🤖 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 179 - 189, The two methods are out of alphabetical order; move hasInitNotebookBeenRun(projectId: string) and markInitNotebookAsRun(projectId: string) so they appear before queueNotebookResolution in the class. Locate the methods by name (hasInitNotebookBeenRun and markInitNotebookAsRun) and cut/paste them to the section preceding the queueNotebookResolution method, preserving their signatures and bodies and any associated comments or JSDoc.
♻️ Duplicate comments (4)
src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (2)
575-582: 🧹 Nitpick | 🔵 TrivialMethod ordering:
normalizeFileUriis misplaced.Should come after
markSelfWritealphabetically among private methods.🤖 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 575 - 582, The private method normalizeFileUri is out of alphabetical order; move the normalizeFileUri(uri: Uri) method so it appears immediately after the private method markSelfWrite (i.e., reorder the private methods alphabetically) to satisfy the project's method ordering convention and preserve visibility and functionality.
374-401:⚠️ Potential issue | 🟡 MinorDouble
markSelfWritestill arms twice for one file.Lines 375 and 390 both call
markSelfWrite(fileUri). IfwriteFileandworkspace.savecoalesce into a single FS event, the second marker persists for 30s, potentially suppressing the next external change.Keep marker on
writeFileonly.Proposed fix
// 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); }🤖 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 374 - 401, The duplicate markSelfWrite(fileUri) call should be removed so the self-write marker is only set before workspace.fs.writeFile(fileUri, serializedBytes); keep the existing consumeSelfWrite(fileUri) calls in the error branches (writeError and saveError) and the early-return path when save() returns falsy (notebook.uri), but do not re-arm markSelfWrite before calling workspace.save; in short, keep markSelfWrite tied to writeFile only and ensure consumeSelfWrite is still invoked on any failure paths.src/notebooks/deepnote/deepnoteSerializer.ts (1)
594-610: 🧹 Nitpick | 🔵 TrivialMethod ordering:
findOpenNotebookIdsshould come afterfindNotebookUri.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 594 - 610, The method findOpenNotebookIds is out of the required alphabetical ordering and should be moved to appear after findNotebookUri (and after the findNotebook... group and before any findOpen... group) to match the project's method order (e.g., keep findDefault... then findNotebook... then findOpen...); relocate the entire findOpenNotebookIds method block so its declaration follows the findNotebookUri function declaration (preserving its implementation and imports) to satisfy the ordering rule.src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
274-279: 🧹 Nitpick | 🔵 TrivialAssert by notebook ID, not array position.
Assertions still rely on
notebooks[0]/notebooks[1]ordering. A reorder would fail for the wrong reason.Suggested fix
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((nb) => nb.id === 'notebook-1'); +const secondNotebook = serializedProject.project.notebooks.find((nb) => nb.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 274 - 279, The test is asserting notebook contents by array index which can break if order changes; update the assertions in deepnoteSerializer.unit.test.ts to locate notebooks by their id instead of using notebooks[0]/notebooks[1]: after calling serializer.serializeNotebook and deserializeDeepnoteFile, find the notebook objects inside serializedProject.project.notebooks by matching the notebook id values present in mockNotebookData (or known IDs) and then assert their blocks[0].content equals the expected strings; replace direct index access with a lookup (e.g., find by notebook.id) so assertions are stable against reordering.
🤖 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 1382-1418: Extract the edit-capturing block into a typed test
helper (e.g., captureNotebookEdits(uri: Uri, edits: NotebookEdit[])) and call
that from the sinon stub so you can remove the unsafe casts; specifically, move
the logic that computes outputPlainJoined and pushes into interactionCaptures
into captureNotebookEdits, then change the stub on
snapshotWatcher.applyNotebookEdits to a properly typed callsFake like async
function(this: DeepnoteFileChangeWatcher, uri: Uri, edits: NotebookEdit[]) {
captureNotebookEdits(uri, edits); return
DeepnoteFileChangeWatcher.prototype.applyNotebookEdits.apply(this, [uri,
edits]); } so you no longer need the `as keyof`/`as never` casts and keep
references to snapshotApplyEditStub, snapshotWatcher, applyNotebookEdits,
DeepnoteFileChangeWatcher and interactionCaptures for locating the change.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteNotebookManager.ts`:
- Around line 179-189: The two methods are out of alphabetical order; move
hasInitNotebookBeenRun(projectId: string) and markInitNotebookAsRun(projectId:
string) so they appear before queueNotebookResolution in the class. Locate the
methods by name (hasInitNotebookBeenRun and markInitNotebookAsRun) and cut/paste
them to the section preceding the queueNotebookResolution method, preserving
their signatures and bodies and any associated comments or JSDoc.
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 575-582: The private method normalizeFileUri is out of
alphabetical order; move the normalizeFileUri(uri: Uri) method so it appears
immediately after the private method markSelfWrite (i.e., reorder the private
methods alphabetically) to satisfy the project's method ordering convention and
preserve visibility and functionality.
- Around line 374-401: The duplicate markSelfWrite(fileUri) call should be
removed so the self-write marker is only set before
workspace.fs.writeFile(fileUri, serializedBytes); keep the existing
consumeSelfWrite(fileUri) calls in the error branches (writeError and saveError)
and the early-return path when save() returns falsy (notebook.uri), but do not
re-arm markSelfWrite before calling workspace.save; in short, keep markSelfWrite
tied to writeFile only and ensure consumeSelfWrite is still invoked on any
failure paths.
In `@src/notebooks/deepnote/deepnoteSerializer.ts`:
- Around line 594-610: The method findOpenNotebookIds is out of the required
alphabetical ordering and should be moved to appear after findNotebookUri (and
after the findNotebook... group and before any findOpen... group) to match the
project's method order (e.g., keep findDefault... then findNotebook... then
findOpen...); relocate the entire findOpenNotebookIds method block so its
declaration follows the findNotebookUri function declaration (preserving its
implementation and imports) to satisfy the ordering rule.
In `@src/notebooks/deepnote/deepnoteSerializer.unit.test.ts`:
- Around line 274-279: The test is asserting notebook contents by array index
which can break if order changes; update the assertions in
deepnoteSerializer.unit.test.ts to locate notebooks by their id instead of using
notebooks[0]/notebooks[1]: after calling serializer.serializeNotebook and
deserializeDeepnoteFile, find the notebook objects inside
serializedProject.project.notebooks by matching the notebook id values present
in mockNotebookData (or known IDs) and then assert their blocks[0].content
equals the expected strings; replace direct index access with a lookup (e.g.,
find by notebook.id) so assertions are stable against reordering.
🪄 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: 0c0731d1-c0c7-49be-a941-ec0e63e0475d
📒 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1370-1371: The setup block in
deepnoteFileChangeWatcher.unit.test.ts is setting an unnecessary long timeout
via this.timeout(12_000); — remove that explicit timeout (or reduce it to a
small value like 2000 if you prefer) inside the setup(...) function that only
creates stubs and resets mocks so the test uses the default timeout and avoids
an excessive delay.
- Around line 138-140: Replace the testFileUri helper to use Uri.joinPath for
platform-correct separators: instead of building a path string with
join(testFixturesDir, ...), create a base Uri via Uri.file(testFixturesDir) and
return Uri.joinPath(baseUri, ...pathSegments); update the testFileUri function
implementation accordingly (keep the same signature) so callers are unchanged.
🪄 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: 3824eddd-865d-4721-af9c-d42462c99bf5
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts (1)
138-140: 🧹 Nitpick | 🔵 TrivialBuild test URIs with
Uri.joinPath().This helper is back to string-joining file paths.
Uri.joinPath(Uri.file(testFixturesDir), ...)keeps the tests aligned with the repo’s URI handling rules.As per coding guidelines, "Use `Uri.joinPath()` for constructing file paths to ensure platform-correct path separators (e.g., `Uri.joinPath(venvPath, 'share', 'jupyter', 'kernels')` instead of string concatenation with `/`)."Suggested change
function testFileUri(...pathSegments: string[]): Uri { - return Uri.file(join(testFixturesDir, ...pathSegments)); + return Uri.joinPath(Uri.file(testFixturesDir), ...pathSegments); }🤖 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 138 - 140, The test helper testFileUri currently builds file paths by string-joining via join(testFixturesDir, ...), which can produce incorrect separators; update testFileUri to construct URIs using Uri.joinPath(Uri.file(testFixturesDir), ...pathSegments) so the tests follow the repo's URI handling rules and are platform-safe; locate the function named testFileUri in deepnoteFileChangeWatcher.unit.test.ts and replace the join-based construction with Uri.joinPath(Uri.file(testFixturesDir), ...pathSegments).src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
277-280: 🧹 Nitpick | 🔵 TrivialAssert by notebook ID, not array position.
This test is about notebook selection, but these checks still bake in
project.notebooksordering. A harmless reorder will fail the test for the wrong reason.Suggested change
- 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'); + 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 277 - 280, The assertions in deepnoteSerializer.unit.test.ts currently depend on project.notebooks array order; change them to locate notebooks by id instead of by index: use serializedProject.project.notebooks.find(n => n.id === 'notebook-1') and similarly for 'notebook-2' to assert their .blocks?.[0].content and .id, and replace the index-based asserts (project.notebooks[0] / [1]) with these id-based lookups so the test is order-independent.src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (3)
102-109:⚠️ Potential issue | 🟠 MajorDon’t arm save self-writes in two places.
This listener overlaps with the explicit
markSelfWrite()calls around the watcher’s ownworkspace.save()paths. If programmatic notebook saves emit this event, one sync leaves extra counts behind and the next real.deepnotechange can be skipped.🤖 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 102 - 109, The onDidSaveNotebookDocument listener duplicates the explicit markSelfWrite calls used when the watcher itself calls workspace.save, causing doubled counts and skipped real changes; update the logic so saves initiated by this watcher are not re-armed: either remove this workspace.onDidSaveNotebookDocument(...) handler for 'deepnote' notebooks, or add a guard that ignores events for URIs currently being saved by the watcher (use the same tracking used around workspace.save/markSelfWrite), i.e., check a per-URI in-flight/save-initiator flag or Set before calling markSelfWrite so markSelfWrite is only invoked once per programmatic save (refer to markSelfWrite, workspace.save, and workspace.onDidSaveNotebookDocument).
557-566:⚠️ Potential issue | 🟠 MajorMirror the unsuccessful-save branch here too.
This path only clears the self-write marker on exceptions. If
workspace.save()completes without actually saving, the marker survives and can suppress the next real file change.🤖 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 557 - 566, The current save path marks the file as a self-write but only clears that marker if workspace.save throws; if workspace.save resolves but returns falsy the marker is left set. Change the code in deepnoteFileChangeWatcher.ts so you capture the result of await workspace.save(notebook.uri) (e.g. const saved = await workspace.save(...)) and if saved is falsy call this.consumeSelfWrite(notebook.uri) and throw or handle the failure the same way as in the catch block; keep the existing markSelfWrite, applyNotebookEdits, and consumeSelfWrite calls and behavior otherwise.
575-582: 🧹 Nitpick | 🔵 TrivialRestore private-method ordering around
normalizeFileUri().
normalizeFileUri()is still placed before thehandle*methods, so the private-member alpha ordering rule is broken again.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 575 - 582, normalizeFileUri() is out of order among private members: move the private method normalizeFileUri so it follows the class's private-member alphabetical ordering (i.e., place it after the private handle* methods or otherwise reorder private methods alphabetically), ensuring private-method ordering rules are satisfied; locate normalizeFileUri and the private handle... methods (e.g., handleFileChange, handleFileCreate, handleFileDelete) and reposition normalizeFileUri accordingly so the private members are sorted by accessibility then alphabetically.
🤖 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 509-510: The metadata-restore branches currently call
applyNotebookEdits(notebook.uri, metadataEdits) and ignore its boolean result;
update both restore paths to capture the return value (e.g., const ok = await
this.applyNotebookEdits(notebook.uri, metadataEdits)) and return early (return
false or propagate failure) when ok is false so the function aborts on failed
metadata restoration — mirror the pattern used around the earlier checks (the
block that checks applyNotebookEdits' result and returns on failure) to prevent
leaving VS Code internal IDs in place and breaking cell-to-block mapping.
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 102-109: The onDidSaveNotebookDocument listener duplicates the
explicit markSelfWrite calls used when the watcher itself calls workspace.save,
causing doubled counts and skipped real changes; update the logic so saves
initiated by this watcher are not re-armed: either remove this
workspace.onDidSaveNotebookDocument(...) handler for 'deepnote' notebooks, or
add a guard that ignores events for URIs currently being saved by the watcher
(use the same tracking used around workspace.save/markSelfWrite), i.e., check a
per-URI in-flight/save-initiator flag or Set before calling markSelfWrite so
markSelfWrite is only invoked once per programmatic save (refer to
markSelfWrite, workspace.save, and workspace.onDidSaveNotebookDocument).
- Around line 557-566: The current save path marks the file as a self-write but
only clears that marker if workspace.save throws; if workspace.save resolves but
returns falsy the marker is left set. Change the code in
deepnoteFileChangeWatcher.ts so you capture the result of await
workspace.save(notebook.uri) (e.g. const saved = await workspace.save(...)) and
if saved is falsy call this.consumeSelfWrite(notebook.uri) and throw or handle
the failure the same way as in the catch block; keep the existing markSelfWrite,
applyNotebookEdits, and consumeSelfWrite calls and behavior otherwise.
- Around line 575-582: normalizeFileUri() is out of order among private members:
move the private method normalizeFileUri so it follows the class's
private-member alphabetical ordering (i.e., place it after the private handle*
methods or otherwise reorder private methods alphabetically), ensuring
private-method ordering rules are satisfied; locate normalizeFileUri and the
private handle... methods (e.g., handleFileChange, handleFileCreate,
handleFileDelete) and reposition normalizeFileUri accordingly so the private
members are sorted by accessibility then alphabetically.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 138-140: The test helper testFileUri currently builds file paths
by string-joining via join(testFixturesDir, ...), which can produce incorrect
separators; update testFileUri to construct URIs using
Uri.joinPath(Uri.file(testFixturesDir), ...pathSegments) so the tests follow the
repo's URI handling rules and are platform-safe; locate the function named
testFileUri in deepnoteFileChangeWatcher.unit.test.ts and replace the join-based
construction with Uri.joinPath(Uri.file(testFixturesDir), ...pathSegments).
In `@src/notebooks/deepnote/deepnoteSerializer.unit.test.ts`:
- Around line 277-280: The assertions in deepnoteSerializer.unit.test.ts
currently depend on project.notebooks array order; change them to locate
notebooks by id instead of by index: use
serializedProject.project.notebooks.find(n => n.id === 'notebook-1') and
similarly for 'notebook-2' to assert their .blocks?.[0].content and .id, and
replace the index-based asserts (project.notebooks[0] / [1]) with these id-based
lookups so the test is order-independent.
🪄 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: b8a30679-24a8-4cd6-bf94-38e343a79bcc
📒 Files selected for processing (3)
src/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.ts
…pnoteFileChangeWatcher - 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.
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.ts (1)
569-576:⚠️ Potential issue | 🟡 MinorSame double-marking concern.
Line 570
markSelfWrite(notebook.uri)plusonDidSaveNotebookDocumentlistener creates two markers for one save. Similar to the main file sync path.🤖 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 569 - 576, The code double-marks a self-write by calling markSelfWrite(notebook.uri) before awaiting workspace.save(notebook.uri) while the onDidSaveNotebookDocument listener also marks saves; to fix, remove the redundant markSelfWrite call here and rely on the existing onDidSaveNotebookDocument handling (or adjust onDidSaveNotebookDocument to check and not add a second marker), ensuring consumeSelfWrite(notebook.uri) is still called on save errors; update the DeepnoteFileChangeWatcher logic around workspace.save, markSelfWrite, consumeSelfWrite, and onDidSaveNotebookDocument so each physical save only creates one self-write marker.
♻️ Duplicate comments (2)
src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (2)
585-592: 🧹 Nitpick | 🔵 TrivialPrivate method ordering.
normalizeFileUribreaks alphabetical order among private methods. Should come aftermarkSelfWrite(m < n).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 585 - 592, The private method normalizeFileUri is out of alphabetical order among private methods; move its definition so it appears after markSelfWrite to comply with the ordering rule (accessibility then alphabetical). Locate normalizeFileUri and cut/paste its entire method body to immediately follow the private method markSelfWrite, ensuring no other private methods intervene and preserving imports/comments and formatting.
374-401:⚠️ Potential issue | 🟡 MinorPotential over-marking of self-writes.
Three
markSelfWritecalls can occur for one sync:
- Line 375: before
writeFile- Line 390: before
workspace.save()onDidSaveNotebookDocumentlistener (line 106) fires when save completesIf FS events coalesce, unconsumed markers persist for 30s and may suppress real external changes.
Consider removing line 390's marker since
onDidSaveNotebookDocumentalready covers save events.Proposed fix
// 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); }🤖 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 374 - 401, The code currently calls markSelfWrite(fileUri) twice for a single sync (before workspace.fs.writeFile and again before workspace.save), which can leave extra unconsumed markers if FS events coalesce; remove the second markSelfWrite call that precedes workspace.save() and rely on the existing onDidSaveNotebookDocument handler to mark self-writes when the save completes, but keep the consumeSelfWrite(fileUri) calls in the save-failure branches (both the !saved and catch(saveError) paths) so failures still clear the marker.
🤖 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 1542-1576: The test stub for snapshotApplyEditStub contains
verbose inline type assertions while extracting cell-capture logic would improve
readability; create a typed helper function (e.g., extractCellCapture or
parseNewCells) that accepts the edits/replaceCellsEdit and returns the
structured capture ({ uriKey, cellSourcesJoined, outputPlainJoined }), then call
that helper from the sinon.stub callback instead of the inline casting; update
the sinon.stub callback in DeepnoteFileChangeWatcher.applyNotebookEdits to use
the helper and push its result into interactionCaptures, keeping the final
return call to
DeepnoteFileChangeWatcher.prototype.applyNotebookEdits.apply(this, [uri, edits])
unchanged.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 569-576: The code double-marks a self-write by calling
markSelfWrite(notebook.uri) before awaiting workspace.save(notebook.uri) while
the onDidSaveNotebookDocument listener also marks saves; to fix, remove the
redundant markSelfWrite call here and rely on the existing
onDidSaveNotebookDocument handling (or adjust onDidSaveNotebookDocument to check
and not add a second marker), ensuring consumeSelfWrite(notebook.uri) is still
called on save errors; update the DeepnoteFileChangeWatcher logic around
workspace.save, markSelfWrite, consumeSelfWrite, and onDidSaveNotebookDocument
so each physical save only creates one self-write marker.
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 585-592: The private method normalizeFileUri is out of
alphabetical order among private methods; move its definition so it appears
after markSelfWrite to comply with the ordering rule (accessibility then
alphabetical). Locate normalizeFileUri and cut/paste its entire method body to
immediately follow the private method markSelfWrite, ensuring no other private
methods intervene and preserving imports/comments and formatting.
- Around line 374-401: The code currently calls markSelfWrite(fileUri) twice for
a single sync (before workspace.fs.writeFile and again before workspace.save),
which can leave extra unconsumed markers if FS events coalesce; remove the
second markSelfWrite call that precedes workspace.save() and rely on the
existing onDidSaveNotebookDocument handler to mark self-writes when the save
completes, but keep the consumeSelfWrite(fileUri) calls in the save-failure
branches (both the !saved and catch(saveError) paths) so failures still clear
the marker.
🪄 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: afc636f6-7589-4497-9efe-1be61adb636d
📒 Files selected for processing (2)
src/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts
dinohamzic
reviewed
Apr 2, 2026
Contributor
dinohamzic
left a comment
dinohamzic
left a comment
There was a problem hiding this comment.
Loom with debugging sessions for reference: https://www.loom.com/share/e8a5df55955248929e7f244cc07ba289
…book 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.
- 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 96-101: The manifest exposes the command
"deepnote.copyNotebookDetails" globally but its handler only works for Deepnote
notebooks; update the command entry for "deepnote.copyNotebookDetails" to
include the scope gating used elsewhere by adding a when clause of "notebookType
== 'deepnote'". Locate the command object with "command":
"deepnote.copyNotebookDetails" and add the property "when": "notebookType ==
'deepnote'" so the command is only shown in the command palette for Deepnote
notebooks.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 567-569: The call to workspace.save(notebook.uri) in
deepnoteFileChangeWatcher.ts should check its return value for undefined/falsy
like the other path (see the earlier check around lines 391-396) and log a
warning if the save did not succeed; update the save flow in the function that
contains workspace.save to capture the result, validate it (undefined or false),
and call processLogger.warn or the existing logger with a consistent message
indicating the save failed for the notebook URI and return early or handle the
failure the same way as the earlier branch.
In `@src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts`:
- Around line 99-106: Extract the inline metadata shape used in
deepnoteNotebookInfoStatusBar.ts (the object with deepnoteNotebookId,
deepnoteNotebookName, deepnoteProjectId, deepnoteProjectName, deepnoteVersion,
name) into a named type or interface in deepnoteTypes.ts (e.g.,
DeepnoteNotebookMetadata), export it, then import that type into
deepnoteNotebookInfoStatusBar.ts and replace the inline cast "as { ... }" on the
metadata variable with "as DeepnoteNotebookMetadata"; update any other
usages/imports to use the centralized type so the status-bar stays aligned with
the serializer/manager types.
- Around line 41-43: The command registration should await and propagate the
clipboard write result and move the metadata type to a shared types file: make
the handler call to copyActiveNotebookDetails async and return its Promise from
the commands.registerCommand callback; update copyActiveNotebookDetails to await
env.clipboard.writeText(...) inside a try/catch so failures are caught and an
error toast is shown (only show the success toast after a successful await), and
rethrow or return the rejection so the command caller can observe it. Also
extract the inline metadata interface/type currently declared near
copyActiveNotebookDetails (the Deepnote notebook metadata type) into a new
deepnoteTypes.ts export and import that type where copyActiveNotebookDetails
uses it.
🪄 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: 597098d8-0778-425b-8150-37160cbb6181
📒 Files selected for processing (8)
package.jsonpackage.nls.jsonsrc/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookInfoStatusBar.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/serviceRegistry.web.tssrc/platform/common/constants.ts
Comment on lines
+96
to
+101
| { | ||
| "command": "deepnote.copyNotebookDetails", | ||
| "title": "%deepnote.commands.copyNotebookDetails.title%", | ||
| "category": "Deepnote", | ||
| "icon": "$(copy)" | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine package.json around the command definition and menus section
fd -t f "package.json" | head -1 | xargs wc -lRepository: deepnote/vscode-deepnote
Length of output: 87
🏁 Script executed:
# Look for the command definition and any menus sections
rg -n "deepnote.copyNotebookDetails" -A 5 -B 5Repository: deepnote/vscode-deepnote
Length of output: 1794
🏁 Script executed:
# Check if there are any menus.commandPalette entries for this command
rg -n "commandPalette" -A 10 -B 2Repository: deepnote/vscode-deepnote
Length of output: 6998
🏁 Script executed:
# Find and examine the handler implementation
fd -t f "deepnoteNotebookInfoStatusBar.ts" | head -1Repository: deepnote/vscode-deepnote
Length of output: 123
🏁 Script executed:
# Search more specifically for commandPalette entries related to copyNotebookDetails
rg -n "copyNotebookDetails" -A 3 -B 1Repository: deepnote/vscode-deepnote
Length of output: 1409
🏁 Script executed:
# Get the full menus.commandPalette section to verify if copyNotebookDetails is listed
sed -n '1202,1250p' package.jsonRepository: deepnote/vscode-deepnote
Length of output: 2412
🏁 Script executed:
# Now examine the handler implementation
cat -n src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts | head -100Repository: deepnote/vscode-deepnote
Length of output: 3805
🏁 Script executed:
# Check if there's a context variable for active Deepnote notebooks
rg -n "deepnote.isdeepnotenotebook|deepnote.isactive" package.jsonRepository: deepnote/vscode-deepnote
Length of output: 50
🏁 Script executed:
# Look for similar commands that are scoped to Deepnote notebook context
rg -n "deepnote.enable|deepnote.disable" -A 3 package.json | head -40Repository: deepnote/vscode-deepnote
Length of output: 2138
🏁 Script executed:
# Check the full handler to see if it validates for Deepnote notebook type
sed -n '77,110p' src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.tsRepository: deepnote/vscode-deepnote
Length of output: 1183
🏁 Script executed:
# Check if enableSnapshots and disableSnapshots have commandPalette entries with scoping
rg -n "enableSnapshots|disableSnapshots" package.json | grep -A 3 "command"Repository: deepnote/vscode-deepnote
Length of output: 335
🏁 Script executed:
# Search for all commandPalette entries to see if any use "deepnote.isdeepnotenotebook" or similar context
rg -n '"when"' package.json | grep -i deepnote | head -20Repository: deepnote/vscode-deepnote
Length of output: 2223
🏁 Script executed:
# Check revealInExplorer command for comparison
rg -n "revealInExplorer" package.json -A 4 -B 1Repository: deepnote/vscode-deepnote
Length of output: 601
Add commandPalette scoping to gate this command to Deepnote notebooks.
The command is globally discoverable in the palette but the handler silently returns when no active Deepnote notebook exists, creating a dead command outside that context. Add "when": "notebookType == 'deepnote'" to the command definition, following the pattern used elsewhere in the manifest.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 96 - 101, The manifest exposes the command
"deepnote.copyNotebookDetails" globally but its handler only works for Deepnote
notebooks; update the command entry for "deepnote.copyNotebookDetails" to
include the scope gating used elsewhere by adding a when clause of "notebookType
== 'deepnote'". Locate the command object with "command":
"deepnote.copyNotebookDetails" and add the property "when": "notebookType ==
'deepnote'" so the command is only shown in the command palette for Deepnote
notebooks.
Comment on lines
+567
to
+569
| // Save to sync mtime. | ||
| // NOTE: onDidSaveNotebookDocument handles the self-write mark for this save. | ||
| await workspace.save(notebook.uri); |
Contributor
There was a problem hiding this comment.
Check workspace.save() return for consistency.
Lines 391-396 check for undefined and log a warning. This path doesn't. Minor, but inconsistent error handling.
Suggested fix
// Save to sync mtime.
// NOTE: onDidSaveNotebookDocument handles the self-write mark for this save.
-await workspace.save(notebook.uri);
+const saved = await workspace.save(notebook.uri);
+if (!saved) {
+ logger.warn(`[FileChangeWatcher] Save after snapshot update returned undefined: ${notebook.uri.path}`);
+}📝 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
| // Save to sync mtime. | |
| // NOTE: onDidSaveNotebookDocument handles the self-write mark for this save. | |
| await workspace.save(notebook.uri); | |
| // Save to sync mtime. | |
| // NOTE: onDidSaveNotebookDocument handles the self-write mark for this save. | |
| const saved = await workspace.save(notebook.uri); | |
| if (!saved) { | |
| logger.warn(`[FileChangeWatcher] Save after snapshot update returned undefined: ${notebook.uri.path}`); | |
| } |
🤖 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 567 - 569,
The call to workspace.save(notebook.uri) in deepnoteFileChangeWatcher.ts should
check its return value for undefined/falsy like the other path (see the earlier
check around lines 391-396) and log a warning if the save did not succeed;
update the save flow in the function that contains workspace.save to capture the
result, validate it (undefined or false), and call processLogger.warn or the
existing logger with a consistent message indicating the save failed for the
notebook URI and return early or handle the failure the same way as the earlier
branch.
Comment on lines
+41
to
+43
| commands.registerCommand(Commands.CopyNotebookDetails, () => { | ||
| this.copyActiveNotebookDetails(); | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.tsRepository: deepnote/vscode-deepnote
Length of output: 5772
Await clipboard write and handle failures explicitly.
Lines 41-43 drop the async result, and lines 90-91 show success immediately without awaiting env.clipboard.writeText(). Clipboard failures will be silently ignored while the user sees a false success toast. Make the method async, return the promise from the command, and wrap the clipboard call in try/catch.
Also extract the metadata type definition (lines 99-106) to deepnoteTypes.ts per the architecture guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts` around lines 41 -
43, The command registration should await and propagate the clipboard write
result and move the metadata type to a shared types file: make the handler call
to copyActiveNotebookDetails async and return its Promise from the
commands.registerCommand callback; update copyActiveNotebookDetails to await
env.clipboard.writeText(...) inside a try/catch so failures are caught and an
error toast is shown (only show the success toast after a successful await), and
rethrow or return the rejection so the command caller can observe it. Also
extract the inline metadata interface/type currently declared near
copyActiveNotebookDetails (the Deepnote notebook metadata type) into a new
deepnoteTypes.ts export and import that type where copyActiveNotebookDetails
uses it.
Comment on lines
+99
to
+106
| const metadata = notebook.metadata as { | ||
| deepnoteNotebookId?: string; | ||
| deepnoteNotebookName?: string; | ||
| deepnoteProjectId?: string; | ||
| deepnoteProjectName?: string; | ||
| deepnoteVersion?: string; | ||
| name?: string; | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move this metadata contract into deepnoteTypes.ts.
This inline shape is now another Deepnote notebook metadata contract. Centralizing it will keep the status-bar path aligned with the serializer/manager types.
As per coding guidelines, "Deepnote integration components should follow refactored architecture with separation: deepnoteTypes.ts for type definitions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts` around lines 99 -
106, Extract the inline metadata shape used in deepnoteNotebookInfoStatusBar.ts
(the object with deepnoteNotebookId, deepnoteNotebookName, deepnoteProjectId,
deepnoteProjectName, deepnoteVersion, name) into a named type or interface in
deepnoteTypes.ts (e.g., DeepnoteNotebookMetadata), export it, then import that
type into deepnoteNotebookInfoStatusBar.ts and replace the inline cast "as { ...
}" on the metadata variable with "as DeepnoteNotebookMetadata"; update any other
usages/imports to use the centralized type so the status-bar stays aligned with
the serializer/manager types.
- 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
614-653: 🧹 Nitpick | 🔵 TrivialMethod ordering incorrect per guidelines.
Alphabetically:
findDefaultNotebook<findNotebookIdsFromTabs<findNotebookUri<findOpenNotebookIds. Move these methods to match.🤖 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 614 - 653, Reorder the methods so they follow the alphabetical guideline: move findNotebookIdsFromTabs and findOpenNotebookIds so the final sequence is findDefaultNotebook, findNotebookIdsFromTabs, findNotebookUri, findOpenNotebookIds; ensure you relocate the entire method bodies (including signatures and return statements) for findNotebookIdsFromTabs and findOpenNotebookIds and update any nearby comments so method grouping remains consistent.
🤖 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/deepnoteSerializer.ts`:
- Around line 59-69: consumedTabResolutions and recentSerializations grow
without bound; add cleanup on notebook/project close by clearing or pruning
entries when notebooks are unloaded. Implement a handler (e.g., in the
DeepnoteSerializer class or wherever notebook lifecycle is observed) that
listens for notebook close/project close events and removes related keys from
consumedTabResolutions and recentSerializations (use project id or notebookId to
identify entries), and/or enforce a TTL/size limit when writing to
recentSerializations in findCurrentNotebookId to evict old entries. Ensure the
cleanup runs on the same lifecycle hooks used for saving/closing notebooks so
maps don’t accumulate stale data.
- Around line 223-230: The code redundantly fetches the same Set twice: remove
the second retrieval of consumed (the get on this.consumedTabResolutions) and
reuse the already-defined consumedIds variable; i.e., after computing remaining,
call consumedIds.add(remaining[0]) and then
this.consumedTabResolutions.set(projectId, consumedIds) so you only read the Set
once and update it in place (references: consumedIds, remaining,
this.consumedTabResolutions).
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteSerializer.ts`:
- Around line 614-653: Reorder the methods so they follow the alphabetical
guideline: move findNotebookIdsFromTabs and findOpenNotebookIds so the final
sequence is findDefaultNotebook, findNotebookIdsFromTabs, findNotebookUri,
findOpenNotebookIds; ensure you relocate the entire method bodies (including
signatures and return statements) for findNotebookIdsFromTabs and
findOpenNotebookIds and update any nearby comments so method grouping remains
consistent.
🪄 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: d14860d7-4742-4cca-bcce-2a924895100c
📒 Files selected for processing (3)
src/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/test/vscode-mock.ts
Comment on lines
+59
to
+69
| private readonly consumedTabResolutions = new Map<string, Set<string>>(); | ||
|
|
||
| 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<string, { notebookId: string; timestamp: number }>(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Memory unbounded: consider cleanup on project close.
consumedTabResolutions and recentSerializations grow indefinitely. While unlikely to cause issues in practice, consider clearing entries when notebooks close.
🤖 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 59 - 69,
consumedTabResolutions and recentSerializations grow without bound; add cleanup
on notebook/project close by clearing or pruning entries when notebooks are
unloaded. Implement a handler (e.g., in the DeepnoteSerializer class or wherever
notebook lifecycle is observed) that listens for notebook close/project close
events and removes related keys from consumedTabResolutions and
recentSerializations (use project id or notebookId to identify entries), and/or
enforce a TTL/size limit when writing to recentSerializations in
findCurrentNotebookId to evict old entries. Ensure the cleanup runs on the same
lifecycle hooks used for saving/closing notebooks so maps don’t accumulate stale
data.
Comment on lines
+223
to
+230
| const consumedIds = this.consumedTabResolutions.get(projectId) ?? new Set<string>(); | ||
| const remaining = tabNotebookIds.filter((id) => !consumedIds.has(id) && !openNotebookIds.includes(id)); | ||
|
|
||
| if (remaining.length > 0) { | ||
| const consumed = this.consumedTabResolutions.get(projectId) ?? new Set<string>(); | ||
|
|
||
| consumed.add(remaining[0]); | ||
| this.consumedTabResolutions.set(projectId, consumed); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Redundant Set retrieval.
consumedIds on line 223 is identical to consumed on line 227. Reuse the existing variable.
Proposed fix
const consumedIds = this.consumedTabResolutions.get(projectId) ?? new Set<string>();
const remaining = tabNotebookIds.filter((id) => !consumedIds.has(id) && !openNotebookIds.includes(id));
if (remaining.length > 0) {
- const consumed = this.consumedTabResolutions.get(projectId) ?? new Set<string>();
-
- consumed.add(remaining[0]);
- this.consumedTabResolutions.set(projectId, consumed);
+ consumedIds.add(remaining[0]);
+ this.consumedTabResolutions.set(projectId, consumedIds);
return remaining[0];
}🤖 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 223 - 230, The
code redundantly fetches the same Set twice: remove the second retrieval of
consumed (the get on this.consumedTabResolutions) and reuse the already-defined
consumedIds variable; i.e., after computing remaining, call
consumedIds.add(remaining[0]) and then
this.consumedTabResolutions.set(projectId, consumedIds) so you only read the Set
once and update it in place (references: consumedIds, remaining,
this.consumedTabResolutions).
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