fix: Fix deepnote notebook deserializer and file change watcher#363
Draft
fix: Fix deepnote notebook deserializer and file change watcher#363
Conversation
Contributor
📝 WalkthroughWalkthroughfindCurrentNotebookId now prefers a stored notebook selection from the notebook manager, falls back to the active notebook editor if no stored selection exists, and finally searches open Deepnote notebook documents by project ID. deepnoteFileChangeWatcher normalizes URIs for deterministic self-write tracking, applies WorkspaceEdits immediately in-memory, writes serialized notebook bytes directly to disk via workspace.fs.writeFile, then calls workspace.save to clear dirty state; self-write marking/consumption was adjusted and serialization/write/save errors are logged. Unit tests were updated and expanded to cover selection priority and rapid switching scenarios. Sequence Diagram(s)sequenceDiagram
participant UI as "User"
participant Manager as "NotebookManager"
participant Editor as "ActiveEditor"
participant Docs as "OpenDocuments"
UI->>Manager: request current notebook id
alt Manager has stored selection
Manager-->>UI: return stored selection
else no stored selection
Manager->>Editor: query active editor notebook
alt Editor is deepnote with notebookId
Editor-->>UI: return editor notebook id
else
Editor->>Docs: search open Deepnote docs by projectId
Docs-->>UI: return matching notebook id or null
end
end
sequenceDiagram
participant Caller as "Serializer.executeMainFileSync"
participant Workspace as "vscode.workspace"
participant FS as "vscode.workspace.fs"
participant Watcher as "FileChangeWatcher"
Caller->>Workspace: applyEdit(WorkspaceEdit) -- in-memory update
Caller->>Serializer: serialize notebook -> bytes
Caller->>FS: writeFile(fileUri, bytes) -- direct disk write
FS-->>Watcher: file change event (normalized URI)
Caller->>Workspace: save(notebook.uri) -- clear dirty state / update mtime
Note right of Watcher: consumeSelfWrite(normalizeFileUri(uri))
Watcher-->>Caller: ignore self-write if matched
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
❌ 4 Tests Failed:
View the full list of 4 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
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
Comment on lines
+356
to
+373
| this.markSelfWrite(fileUri); | ||
| try { | ||
| await workspace.fs.writeFile(fileUri, serializedBytes); | ||
| } catch (writeError) { | ||
| this.consumeSelfWrite(fileUri); | ||
| logger.warn(`[FileChangeWatcher] Failed to write synced file: ${fileUri.path}`, writeError); | ||
| } | ||
|
|
||
| // Now save — VS Code serializes (same bytes), sees the mtime is from our | ||
| // recent write (which its internal watcher has picked up), and writes | ||
| // successfully without a "content is newer" conflict. | ||
| this.markSelfWrite(fileUri); | ||
| try { | ||
| await workspace.save(notebook.uri); | ||
| } catch (saveError) { | ||
| this.consumeSelfWrite(fileUri); | ||
| logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing return after writeFile failure may cause unintended behavior.
If writeFile fails (line 358), execution continues to markSelfWrite and workspace.save (lines 367-373). The comment at line 364-366 assumes the file was written ("mtime is from our recent write"), which doesn't hold on failure.
Consider returning early after a writeFile failure, or clarify if the fallback to save is intentional.
Proposed fix
try {
await workspace.fs.writeFile(fileUri, serializedBytes);
} catch (writeError) {
this.consumeSelfWrite(fileUri);
logger.warn(`[FileChangeWatcher] Failed to write synced file: ${fileUri.path}`, writeError);
+ return;
}📝 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
| this.markSelfWrite(fileUri); | |
| try { | |
| await workspace.fs.writeFile(fileUri, serializedBytes); | |
| } catch (writeError) { | |
| this.consumeSelfWrite(fileUri); | |
| logger.warn(`[FileChangeWatcher] Failed to write synced file: ${fileUri.path}`, writeError); | |
| } | |
| // Now save — VS Code serializes (same bytes), sees the mtime is from our | |
| // recent write (which its internal watcher has picked up), and writes | |
| // successfully without a "content is newer" conflict. | |
| this.markSelfWrite(fileUri); | |
| try { | |
| await workspace.save(notebook.uri); | |
| } catch (saveError) { | |
| this.consumeSelfWrite(fileUri); | |
| logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError); | |
| } | |
| this.markSelfWrite(fileUri); | |
| try { | |
| await workspace.fs.writeFile(fileUri, serializedBytes); | |
| } catch (writeError) { | |
| this.consumeSelfWrite(fileUri); | |
| logger.warn(`[FileChangeWatcher] Failed to write synced file: ${fileUri.path}`, writeError); | |
| return; | |
| } | |
| // Now save — VS Code serializes (same bytes), sees the mtime is from our | |
| // recent write (which its internal watcher has picked up), and writes | |
| // successfully without a "content is newer" conflict. | |
| this.markSelfWrite(fileUri); | |
| try { | |
| await workspace.save(notebook.uri); | |
| } catch (saveError) { | |
| this.consumeSelfWrite(fileUri); | |
| logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError); | |
| } |
🤖 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 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.
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
1 participant
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
Improvements