fix: Fix deepnote notebook deserializer and file change watcher#363
Draft
fix: Fix deepnote notebook deserializer and file change watcher#363
Conversation
Contributor
📝 WalkthroughWalkthroughdeserializeNotebook now accepts an optional notebookId and will return an empty notebook (cells: []) with project/version metadata when no notebook ID can be resolved. Notebook resolution no longer relies on active editor selection; findCurrentNotebookId prefers notebookManager.consumePendingNotebookResolution(projectId) and otherwise derives an ID from open documents via a new helper and resolveDefaultNotebookId(content). verifyDeserializedNotebook re-reads opened notebook content, computes the intended notebook ID (from URI query or default), and patches the opened document’s cells/metadata with a WorkspaceEdit if inconsistent. serializeNotebook uses notebookManager.getCurrentNotebookId(projectId), records lastSerializedNotebookId+timestamp to avoid immediate re-deserialize within a short TTL, and may queue/consume pending resolutions; it also calls notebookManager.updateOriginalProject(projectId, originalProject). File-change syncing adds applyNotebookEdits, normalizes URIs for self-write tracking, writes canonical bytes via workspace.fs.writeFile before workspace.save, and listens to workspace.onDidSaveNotebookDocument to consume self-writes. DeepnoteNotebookManager replaces persistent per-project selection with a per-project pending-resolution FIFO queue (60s TTL) and exposes queue/consume methods. A new DeepnoteNotebookInfoStatusBar service provides a copy-notebook-details command, registered in DI, and activation now triggers serializer.verifyDeserializedNotebook on notebook open. Sequence Diagram(s)sequenceDiagram
participant Explorer as ExplorerView
participant Manager as NotebookManager
participant Watcher as FileChangeWatcher
participant Serializer as DeepnoteSerializer
participant Workspace as WorkspaceFS
participant Editor as NotebookEditor
Note over Explorer,Manager: user opens/duplicates/creates notebook
Explorer->>Manager: queueNotebookResolution(projectId, notebookId)
Note over Watcher,Serializer: external main-file or snapshot triggers reload
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: NotebookData (with provided notebookId)
else omitted
Serializer->>Editor: findNotebookIdFromOpenDocuments()
Editor-->>Serializer: notebookId | undefined
Serializer-->>Watcher: NotebookData (resolved or empty state)
end
Note over Editor,Serializer: document opened in editor later
Editor->>Serializer: onDidOpenNotebookDocument(doc)
Serializer->>Serializer: verifyDeserializedNotebook(doc)
Serializer->>Editor: apply WorkspaceEdit to patch cells/metadata (if needed)
Note over Watcher,Workspace: when saving from memory
Watcher->>Serializer: serializeNotebook(document)
Serializer->>Manager: getCurrentNotebookId(projectId)
Serializer-->>Watcher: bytes (and maybe queueNotebookResolution)
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
Workspace->>Watcher: onDidSaveNotebookDocument(savedDoc)
Watcher->>Watcher: consumeSelfWrite(normalizeFileUri(savedDoc.uri))
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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.
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
…atch resolution - Removed the `selectNotebookForProject` and `clearNotebookSelection` methods from `DeepnoteNotebookManager` to simplify notebook selection logic. - Introduced a new mechanism in `DeepnoteActivationService` to check and fix notebook ID mismatches when opening documents, improving the reliability of notebook loading. - Updated `DeepnoteSerializer` to prioritize current notebook IDs and handle active tab resolutions more effectively. - Enhanced unit tests to cover the new mismatch resolution logic and ensure proper functionality across various scenarios.
…tebook ID resolution - Removed unused variables and methods related to mismatch checking in `DeepnoteActivationService`, streamlining the activation process. - Updated `findCurrentNotebookId` in `DeepnoteSerializer` to improve notebook ID resolution logic, prioritizing active tab detection. - Adjusted unit tests to reflect changes in notebook ID resolution, ensuring accurate behavior when no pending resolutions exist.
…unused logic - Eliminated unnecessary variables and methods related to recent serialization tracking in `DeepnoteNotebookSerializer`, simplifying the notebook ID resolution process. - Updated the logic for finding the current notebook ID to focus on active tab detection, enhancing overall efficiency. - Removed outdated unit tests that were no longer relevant to the current implementation, ensuring test suite accuracy.
…oteNotebookManager - Introduced debug logging in `DeepnoteNotebookManager` to track valid pending notebook resolutions, enhancing traceability during resolution processes. - Simplified block iteration in `DeepnoteNotebookSerializer` by removing unnecessary null checks, improving code clarity and performance.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (1)
102-109:⚠️ Potential issue | 🟠 MajorDon’t arm a second self-write for the post-sync save.
executeMainFileSync()already increments the counter beforeworkspace.fs.writeFile(). When that same sync flow reachesworkspace.save(), the newonDidSaveNotebookDocumentlistener increments it again. If those writes collapse into one watcher notification, or the save produces no second file event, one count survives for 30s and the next real external edit is dropped. Gate this listener for sync-originated saves, or skip save-based marking on this path entirely.Possible direction
+ private readonly suppressNextSaveSelfWrite = new Set<string>(); + this.disposables.push( workspace.onDidSaveNotebookDocument((notebook) => { if (notebook.notebookType === 'deepnote') { - const fileUri = notebook.uri.with({ query: '', fragment: '' }); - this.markSelfWrite(fileUri); + const fileKey = this.normalizeFileUri(notebook.uri); + if (this.suppressNextSaveSelfWrite.delete(fileKey)) { + return; + } + this.markSelfWrite(notebook.uri); } }) ); ... + const fileKey = this.normalizeFileUri(fileUri); + this.suppressNextSaveSelfWrite.add(fileKey); try { const saved = await workspace.save(notebook.uri); if (!saved) { + this.suppressNextSaveSelfWrite.delete(fileKey); logger.warn(`[FileChangeWatcher] Save after sync write returned undefined: ${notebook.uri.path}`); return; } } catch (saveError) { + this.suppressNextSaveSelfWrite.delete(fileKey); logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError); }Also applies to: 385-390
🤖 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 is double-marking sync-originated saves: executeMainFileSync() already increments the self-write counter before workspace.fs.writeFile(), and the save listener in deepnoteFileChangeWatcher.ts (the workspace.onDidSaveNotebookDocument callback that calls markSelfWrite(fileUri)) increments it again; change this listener to ignore saves that are part of our sync flow (e.g., check a per-file "isSyncing" flag or transaction id set by executeMainFileSync/its caller, or skip calling markSelfWrite when a sync-in-progress marker is present) so only external/unsolicited saves call markSelfWrite; update both the listener at the shown location and the similar block around lines 385-390 to use the same gating mechanism (use the existing markSelfWrite, executeMainFileSync symbols to locate code).
🤖 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 572-580: The test's mocked save handler
(mockedVSCodeNamespaces.workspace.save) currently fires two filesystem events
causing a phantom self-write; update the save stub so it emits only a single
filesystem notification (i.e., call onDidChangeFile.fire once or only call
onDidSaveNotebook.fire once so writeFile+save collapse to one FS event) and
adjust the mirrored block (the similar stub around lines 614–621) the same way,
then run the test to ensure Step 4's second external edit still triggers a
reload as expected.
In `@src/notebooks/deepnote/deepnoteNotebookManager.ts`:
- Around line 157-163: The helper
DeepnoteNotebookManager.getValidPendingNotebookResolutions is logging a full
JSON dump of pending resolutions on every call (via
JSON.stringify(allPendingResolutions, null, 2)), which is noisy and costly since
queueNotebookResolution() and consumePendingNotebookResolution() call it
frequently; remove the full dump and keep only the concise count/projectId
logger.debug line and/or a compact summary (e.g., length or IDs joined) if
absolutely needed, leaving pendingNotebookResolutions filtering logic unchanged.
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 102-109: The onDidSaveNotebookDocument listener is double-marking
sync-originated saves: executeMainFileSync() already increments the self-write
counter before workspace.fs.writeFile(), and the save listener in
deepnoteFileChangeWatcher.ts (the workspace.onDidSaveNotebookDocument callback
that calls markSelfWrite(fileUri)) increments it again; change this listener to
ignore saves that are part of our sync flow (e.g., check a per-file "isSyncing"
flag or transaction id set by executeMainFileSync/its caller, or skip calling
markSelfWrite when a sync-in-progress marker is present) so only
external/unsolicited saves call markSelfWrite; update both the listener at the
shown location and the similar block around lines 385-390 to use the same gating
mechanism (use the existing markSelfWrite, executeMainFileSync symbols to locate
code).
🪄 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: e2bc4093-e1f8-42b9-b8ac-e36638bbd6c1
📒 Files selected for processing (11)
build/mocha-esm-loader.jssrc/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.tssrc/test/mocks/vsc/extHostedTypes.tssrc/test/vscode-mock.ts
👮 Files not reviewed due to content moderation or server errors (6)
- build/mocha-esm-loader.js
- src/test/mocks/vsc/extHostedTypes.ts
- src/test/vscode-mock.ts
- src/notebooks/deepnote/deepnoteExplorerView.ts
- src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts
- src/notebooks/deepnote/deepnoteSerializer.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts (2)
534-537:⚠️ Potential issue | 🟡 MinorUse one synthetic FS change in these leak regressions.
The stale-marker bug shows up when
writeFileandsavecollapse into one filesystem notification. Firing twoonDidChangeFileevents here clears both marks and can let the regression slip.Also applies to: 609-612
🤖 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 534 - 537, In the unit test for Deepnote file change watcher, replace the two synthetic filesystem notifications with a single notification so the test simulates the real-world collapse of writeFile+save into one event: change the calls using onDidChangeFile.fire(uri) (the two occurrences around the first reload and the similar block later) to a single onDidChangeFile.fire(uri) call so the stale-marker regression is exercised properly; ensure both places (the block that consumes the "self-write markers" and the later duplicate block) are updated to fire only once.
2244-2287:⚠️ Potential issue | 🟡 MinorThis test doesn't exercise its claim.
It only waits for two reloads. Nothing here observes notebook-manager state, so a pre-sync routing regression would still pass. Either assert the relevant manager interaction/state or rename the test.
🤖 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 2244 - 2287, The test "should not clear notebook selection before processing file change" currently only waits for applyEditCount >= 2 and doesn't assert notebook-manager state; update the test to either (A) assert the manager interaction/state around selection (e.g., use the mocked notebook manager instance used elsewhere in tests and assert its selection property/ method call: expect(mockNotebookManager.getActiveNotebook() or mockNotebookManager.activeNotebook).toEqual(notebook1) before and during the onDidChangeFile handling, or assert that a clearSelection/ deselect method was not called: expect(mockNotebookManager.clearSelection).not.toHaveBeenCalled()) or (B) if you intend to keep it purely timing-based, rename the test to reflect it only waits for reloads; ensure you reference the same symbols used in this file such as onDidChangeFile.fire, applyEditCount, mockedVSCodeNamespaces.workspace.notebookDocuments, and the mock notebook manager (mockNotebookManager) so the assertion verifies the pre-sync routing/selection behavior rather than only reload count.src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
253-259:⚠️ Potential issue | 🟡 MinorFind notebooks by ID, not by array slot.
These assertions couple the test to
project.notebooksordering. A harmless reorder breaks 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].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 253 - 259, The test currently asserts notebook properties by array indices which couples it to ordering; update the assertions to locate notebooks by their id instead: after deserializing (deserializeDeepnoteFile) search serializedProject.project.notebooks for entries with id 'notebook-1' and 'notebook-2' and assert each found notebook's blocks[0].content and id match expectations; adjust uses of serializer.serializeNotebook and any variable names accordingly so the test no longer depends on array ordering.
🤖 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 17-18: The two local imports (import { join } from
'../../platform/vscode-path/path' and import { logger } from
'../../platform/logging') should be moved out of the third-party import block
and placed into a separate local-import group below third-party imports in
deepnoteFileChangeWatcher.unit.test.ts; update the import ordering so
third-party modules remain first and then start a new group containing the join
and logger imports to follow the project's "Separate third-party and local file
imports" guideline.
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 534-537: In the unit test for Deepnote file change watcher,
replace the two synthetic filesystem notifications with a single notification so
the test simulates the real-world collapse of writeFile+save into one event:
change the calls using onDidChangeFile.fire(uri) (the two occurrences around the
first reload and the similar block later) to a single onDidChangeFile.fire(uri)
call so the stale-marker regression is exercised properly; ensure both places
(the block that consumes the "self-write markers" and the later duplicate block)
are updated to fire only once.
- Around line 2244-2287: The test "should not clear notebook selection before
processing file change" currently only waits for applyEditCount >= 2 and doesn't
assert notebook-manager state; update the test to either (A) assert the manager
interaction/state around selection (e.g., use the mocked notebook manager
instance used elsewhere in tests and assert its selection property/ method call:
expect(mockNotebookManager.getActiveNotebook() or
mockNotebookManager.activeNotebook).toEqual(notebook1) before and during the
onDidChangeFile handling, or assert that a clearSelection/ deselect method was
not called: expect(mockNotebookManager.clearSelection).not.toHaveBeenCalled())
or (B) if you intend to keep it purely timing-based, rename the test to reflect
it only waits for reloads; ensure you reference the same symbols used in this
file such as onDidChangeFile.fire, applyEditCount,
mockedVSCodeNamespaces.workspace.notebookDocuments, and the mock notebook
manager (mockNotebookManager) so the assertion verifies the pre-sync
routing/selection behavior rather than only reload count.
In `@src/notebooks/deepnote/deepnoteSerializer.unit.test.ts`:
- Around line 253-259: The test currently asserts notebook properties by array
indices which couples it to ordering; update the assertions to locate notebooks
by their id instead: after deserializing (deserializeDeepnoteFile) search
serializedProject.project.notebooks for entries with id 'notebook-1' and
'notebook-2' and assert each found notebook's blocks[0].content and id match
expectations; adjust uses of serializer.serializeNotebook and any variable names
accordingly so the test no longer depends on array ordering.
🪄 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: b9c73949-529e-43da-bc09-55a6e9dc6f5f
📒 Files selected for processing (2)
src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.ts
Comment on lines
+17
to
+18
| import { join } from '../../platform/vscode-path/path'; | ||
| import { logger } from '../../platform/logging'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Move these local imports below the third-party block.
../../platform/vscode-path/path and ../../platform/logging are repo-local imports, so they should start a new import group.
As per coding guidelines: "Separate third-party and local file imports".
🤖 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
17 - 18, The two local imports (import { join } from
'../../platform/vscode-path/path' and import { logger } from
'../../platform/logging') should be moved out of the third-party import block
and placed into a separate local-import group below third-party imports in
deepnoteFileChangeWatcher.unit.test.ts; update the import ordering so
third-party modules remain first and then start a new group containing the join
and logger imports to follow the project's "Separate third-party and local file
imports" guideline.
- Added a new event listener in `DeepnoteActivationService` to verify deserialized notebooks upon opening. - Improved `DeepnoteNotebookSerializer` to handle cases where no notebook ID is resolved, returning an empty state instead of throwing an error. - Updated unit tests to reflect changes in notebook deserialization behavior, ensuring accurate handling of scenarios with no available notebooks.
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/deepnoteActivationService.ts`:
- Around line 54-58: The onDidOpenNotebookDocument handler currently
fire-and-forgets serializer.verifyDeserializedNotebook(doc), which can overlap
under rapid opens; change this to serialize or debounce calls per notebook URI:
implement a lightweight per-document queue or a Map<string, Promise> (keyed by
doc.uri.toString()) that stores the last pending verifyDeserializedNotebook
promise and chains the next call onto it (or use a short debounce timer keyed by
URI to coalesce rapid opens) so each verifyDeserializedNotebook invocation waits
for the prior one for the same notebook to finish before
reading/deserializing/applying edits; update the handler in the subscription
registered around workspace.onDidOpenNotebookDocument to use that
queuing/debouncing mechanism and clear Map entries when done or when errors
occur.
🪄 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: 211ac348-90c6-4f46-a5e8-3533a48a435a
📒 Files selected for processing (3)
src/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.ts
Comment on lines
+54
to
+58
| this.extensionContext.subscriptions.push( | ||
| workspace.onDidOpenNotebookDocument((doc) => { | ||
| void this.serializer.verifyDeserializedNotebook(doc); | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fire-and-forget async calls may overlap.
Rapid notebook opens trigger concurrent verifyDeserializedNotebook calls. Each reads file, deserializes, and applies edits independently. Works correctly due to VS Code's edit serialization, but consider whether queuing/debouncing would reduce unnecessary I/O.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteActivationService.ts` around lines 54 - 58,
The onDidOpenNotebookDocument handler currently fire-and-forgets
serializer.verifyDeserializedNotebook(doc), which can overlap under rapid opens;
change this to serialize or debounce calls per notebook URI: implement a
lightweight per-document queue or a Map<string, Promise> (keyed by
doc.uri.toString()) that stores the last pending verifyDeserializedNotebook
promise and chains the next call onto it (or use a short debounce timer keyed by
URI to coalesce rapid opens) so each verifyDeserializedNotebook invocation waits
for the prior one for the same notebook to finish before
reading/deserializing/applying edits; update the handler in the subscription
registered around workspace.onDidOpenNotebookDocument to use that
queuing/debouncing mechanism and clear Map entries when done or when errors
occur.
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