fix: Detect external changes to notebooks #319
+531
−0
Conversation
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
Artmann
changed the title
Contributor
📝 WalkthroughWalkthroughAdds DeepnoteFileChangeWatcher, an activation service that debounces external changes to //*.deepnote** (500ms), ignores snapshot files, reads and deserializes the file, finds matching open Deepnote notebook editors, compares cells, preserves live outputs when deserialized cells lack them, replaces notebook cells via a WorkspaceEdit, and saves the notebook to update mtime. Includes cancellation, timer management, per-notebook logging, and unit tests for debounce, error handling, snapshot filtering, output preservation, and dirty-notebook behavior. Sequence DiagramsequenceDiagram
participant Ext as External Process
participant FSW as FileSystemWatcher
participant Watcher as DeepnoteFileChangeWatcher
participant FS as Workspace.fs
participant Ser as DeepnoteNotebookSerializer
participant NbMgr as IDeepnoteNotebookManager
participant WE as Workspace.applyEdit
participant NbDoc as NotebookDocument
Ext->>FS: modify .deepnote file
FS->>FSW: emit onDidChange(uri)
FSW->>Watcher: onDidChange event
Note over Watcher: debounce 500ms
Watcher->>FS: readFile(uri)
FS-->>Watcher: file content
Watcher->>Ser: deserialize(content, token)
Ser-->>Watcher: NotebookCellData[]
Watcher->>NbMgr: find open notebooks for uri
NbMgr-->>Watcher: NotebookDocument[]
loop for each NotebookDocument
Watcher->>Watcher: compare cells
alt cells differ
Watcher->>Watcher: preserve live outputs if missing
Watcher->>WE: applyEdit(replace cells)
WE-->>Watcher: edit applied
Watcher->>NbDoc: save()
else cells same
Note left of Watcher: skip reload
end
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #319 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 150-157: The code calls workspace.applyEdit(...) but doesn't check
its boolean result before calling workspace.save(...); capture the return value
of workspace.applyEdit (used with WorkspaceEdit and
NotebookEdit.replaceCells/NotebookRange on notebook.uri) and only call
workspace.save(notebook.uri) when applyEdit returns true; if applyEdit returns
false, bail out (throw or log and return) to avoid saving after a failed edit
and potential inconsistent notebook state.
- Around line 110-116: The call to serializer.deserializeNotebook inside
reloadNotebooksForFile can throw and currently its error propagates uncaught;
change the block around tokenSource/deserializeNotebook to catch errors: create
the CancellationTokenSource as before, call await
this.serializer.deserializeNotebook(content, tokenSource.token) inside a
try/catch that logs the exception (using the same logger pattern used for the
readFile failure) and returns/handles the failure path, and ensure
tokenSource.dispose() still runs in finally; reference deserializeNotebook,
tokenSource, and reloadNotebooksForFile when making the change so the
deserialization exception is logged instead of escaping via the void call.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 238-252: Update the test comment in the 'should handle parse
errors gracefully' unit test to stop claiming "errors are caught and logged"
since the implementation currently swallows errors (they are invoked via void).
Change the inline comment above the assertion to accurately state that the
handler should not throw and that parse errors are currently swallowed/ignored
(or adjust to reflect the expected future behavior once logging in
onDidChangeFile is implemented), and keep the rest of the test using
onDidChangeFile, setupMockFs and asserting applyEditCount === 0.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 140-158: Extract the duplicated block ID extraction into a small
helper (e.g., getBlockIdFromMetadata) and use it in both loops: replace the
inline expressions that compute blockId from liveCell.metadata and cell.metadata
with calls to this helper, then update calls where blockId is checked (the loops
over liveCells and newCells that populate liveOutputsByBlockId and assign
cell.outputs) to use the new helper; ensure the helper accepts the metadata
object type used by liveCell and cell and returns string | undefined so existing
null/undefined checks remain unchanged.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 63-66: The teardown currently only calls sinon.restore() and
onDidChangeFile.dispose() but doesn't dispose the watcher-registered
disposables; update the teardown block to also dispose the watcher or iterate
and dispose mockDisposables so all timers/registrations are cleaned up (e.g.,
call watcher.dispose() or mockDisposables.forEach(d => d.dispose()) before
onDidChangeFile.dispose()); ensure you reference the existing watcher and
mockDisposables symbols so the test no longer leaks between runs.
The debounce test used emptyBlocksYaml against a notebook with 0 cells, so cellsMatchNotebook returned true and applyEdit was never called, causing a timeout in CI.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 53-180: The private methods are out of alphabetical order; reorder
the five private methods so their declarations are alphabetized by name:
clearAllTimers, cellsMatchNotebook, getBlockIdFromMetadata, handleFileChange,
reloadNotebooksForFile; move the full method blocks (including their bodies) so
that references (e.g., getBlockIdFromMetadata used in reloadNotebooksForFile and
cellsMatchNotebook used in reload logic) remain unchanged and no logic is
modified—only change the physical order of the method declarations.
- Around line 34-35: Reorder the private fields in the DeepnoteFileChangeWatcher
class so they follow accessibility then alphabetical order: move the private
readonly debounceTimers declaration to appear before private readonly
serializer; ensure the Map type and ReturnType<typeof setTimeout> remain
unchanged and that no other code references are modified.
- Around line 53-110: Add a blank line immediately before each early return to
follow the whitespace guideline: in handleFileChange insert a blank line before
the "return" inside the isSnapshotFile check; in cellsMatchNotebook add a blank
line before "return false" after the liveCells length check; in
reloadNotebooksForFile add a blank line before "return" when
affectedNotebooks.length === 0; and in getBlockIdFromMetadata (single-line
return) convert to a short body with a blank line before the return if you
expand it, or ensure surrounding const/grouped declarations have a blank line
before the return; keep formatting consistent across these methods.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 14-21: Extract the hardcoded timing magic numbers into named
constants at the top of the test module (e.g., DEFAULT_WAIT_TIMEOUT_MS and
DEFAULT_WAIT_INTERVAL_MS) and replace the numeric literals in the waitFor
function and other test helpers/usages with these constants; update the waitFor
signature to use the constants as default parameters and replace any other
standalone hardcoded timeouts/intervals in this file (including the other
wait/retry blocks referenced) to reuse the same constants so tests stay
consistent.
- Around line 82-98: Add a blank line before the return statements in the helper
functions to follow the whitespace guideline: locate createMockNotebook (the
function that builds cells and then returns the NotebookDocument) and
setupMockFs (the helper that returns the mocked FS object) and insert an empty
line between the final const/group of variable declarations and the return
statement in each function so there is a clear blank line separating the setup
from the return.
- Around line 63-66: The teardown block is using a concise arrow in
mockDisposables.forEach((d) => d.dispose()) which Biome flags as returning a
value; change that callback to a statement-bodied arrow or a simple for-of loop
so it doesn't implicitly return: update the mockDisposables cleanup in the
teardown function (where teardown, mockDisposables, and onDidChangeFile.dispose
are referenced) to use either mockDisposables.forEach((d) => { d.dispose(); })
or for (const d of mockDisposables) { d.dispose(); } and keep the
sinon.restore() and onDidChangeFile.dispose() calls unchanged.
Comment on lines
53
to
180
| private getBlockIdFromMetadata(metadata: Record<string, unknown> | undefined): string | undefined { | ||
| return (metadata?.id ?? metadata?.__deepnoteBlockId) as string | undefined; | ||
| } | ||
|
|
||
| private clearAllTimers(): void { | ||
| for (const timer of this.debounceTimers.values()) { | ||
| clearTimeout(timer); | ||
| } | ||
|
|
||
| this.debounceTimers.clear(); | ||
| } | ||
|
|
||
| private handleFileChange(uri: Uri): void { | ||
| if (isSnapshotFile(uri)) { | ||
| return; | ||
| } | ||
|
|
||
| const key = uri.toString(); | ||
|
|
||
| const existing = this.debounceTimers.get(key); | ||
|
|
||
| if (existing) { | ||
| clearTimeout(existing); | ||
| } | ||
|
|
||
| this.debounceTimers.set( | ||
| key, | ||
| setTimeout(() => { | ||
| this.debounceTimers.delete(key); | ||
|
|
||
| void this.reloadNotebooksForFile(uri); | ||
| }, debounceTimeInMilliseconds) | ||
| ); | ||
| } | ||
|
|
||
| private cellsMatchNotebook(notebook: NotebookDocument, newCells: NotebookCellData[]): boolean { | ||
| const liveCells = notebook.getCells(); | ||
|
|
||
| if (liveCells.length !== newCells.length) { | ||
| return false; | ||
| } | ||
|
|
||
| return liveCells.every( | ||
| (live, i) => live.document.getText() === newCells[i].value && live.kind === newCells[i].kind | ||
| ); | ||
| } | ||
|
|
||
| private async reloadNotebooksForFile(uri: Uri): Promise<void> { | ||
| const uriString = uri.toString(); | ||
|
|
||
| const affectedNotebooks = workspace.notebookDocuments.filter( | ||
| (doc) => | ||
| doc.notebookType === 'deepnote' && doc.uri.with({ query: '', fragment: '' }).toString() === uriString | ||
| ); | ||
|
|
||
| if (affectedNotebooks.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| let content: Uint8Array; | ||
|
|
||
| try { | ||
| content = await workspace.fs.readFile(uri); | ||
| } catch (error) { | ||
| logger.warn(`[FileChangeWatcher] Failed to read changed file: ${uri.path}`, error); | ||
| return; | ||
| } | ||
|
|
||
| const tokenSource = new CancellationTokenSource(); | ||
| let newData; | ||
| try { | ||
| newData = await this.serializer.deserializeNotebook(content, tokenSource.token); | ||
| } catch (error) { | ||
| logger.warn(`[FileChangeWatcher] Failed to parse changed file: ${uri.path}`, error); | ||
| return; | ||
| } finally { | ||
| tokenSource.dispose(); | ||
| } | ||
|
|
||
| for (const notebook of affectedNotebooks) { | ||
| try { | ||
| const newCells = newData.cells.map((cell) => ({ ...cell })); | ||
|
|
||
| if (this.cellsMatchNotebook(notebook, newCells)) { | ||
| continue; | ||
| } | ||
|
|
||
| // Preserve outputs from live cells that the deserialized data may lack. | ||
| // In snapshot mode the main file has outputs stripped; AI agents | ||
| // typically don't preserve outputs when editing code. | ||
| const liveCells = notebook.getCells(); | ||
| const liveOutputsByBlockId = new Map<string, readonly NotebookCellOutput[]>(); | ||
| for (const liveCell of liveCells) { | ||
| const blockId = this.getBlockIdFromMetadata(liveCell.metadata); | ||
| if (blockId && liveCell.outputs.length > 0) { | ||
| liveOutputsByBlockId.set(blockId, liveCell.outputs); | ||
| } | ||
| } | ||
|
|
||
| for (const cell of newCells) { | ||
| const blockId = this.getBlockIdFromMetadata(cell.metadata); | ||
| if (blockId && (!cell.outputs || cell.outputs.length === 0)) { | ||
| const liveOutputs = liveOutputsByBlockId.get(blockId); | ||
| if (liveOutputs) { | ||
| cell.outputs = [...liveOutputs]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const edit = new WorkspaceEdit(); | ||
| edit.set(notebook.uri, [NotebookEdit.replaceCells(new NotebookRange(0, notebook.cellCount), newCells)]); | ||
| const applied = await workspace.applyEdit(edit); | ||
| if (!applied) { | ||
| logger.warn(`[FileChangeWatcher] Failed to apply edit: ${notebook.uri.path}`); | ||
| continue; | ||
| } | ||
|
|
||
| // Save immediately so VS Code updates its internal mtime for the file. | ||
| // Without this, the user gets a "content is newer" conflict dialog on | ||
| // their next manual save because VS Code still remembers the old mtime. | ||
| await workspace.save(notebook.uri); | ||
|
|
||
| logger.info(`[FileChangeWatcher] Reloaded notebook from external change: ${notebook.uri.path}`); | ||
| } catch (error) { | ||
| logger.error(`[FileChangeWatcher] Failed to reload notebook: ${notebook.uri.path}`, error); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Alphabetize private methods.
Private methods are out of alphabetical order; please reorder them to comply with the guideline (e.g., clearAllTimers, cellsMatchNotebook, getBlockIdFromMetadata, handleFileChange, reloadNotebooksForFile).
As per coding guidelines: "Order method, fields and properties, first by accessibility and then by alphabetical order".
🤖 Prompt for AI Agents
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts` around lines 53 - 180,
The private methods are out of alphabetical order; reorder the five private
methods so their declarations are alphabetized by name: clearAllTimers,
cellsMatchNotebook, getBlockIdFromMetadata, handleFileChange,
reloadNotebooksForFile; move the full method blocks (including their bodies) so
that references (e.g., getBlockIdFromMetadata used in reloadNotebooksForFile and
cellsMatchNotebook used in reload logic) remain unchanged and no logic is
modified—only change the physical order of the method declarations.
Comment on lines
53
to
110
| private getBlockIdFromMetadata(metadata: Record<string, unknown> | undefined): string | undefined { | ||
| return (metadata?.id ?? metadata?.__deepnoteBlockId) as string | undefined; | ||
| } | ||
|
|
||
| private clearAllTimers(): void { | ||
| for (const timer of this.debounceTimers.values()) { | ||
| clearTimeout(timer); | ||
| } | ||
|
|
||
| this.debounceTimers.clear(); | ||
| } | ||
|
|
||
| private handleFileChange(uri: Uri): void { | ||
| if (isSnapshotFile(uri)) { | ||
| return; | ||
| } | ||
|
|
||
| const key = uri.toString(); | ||
|
|
||
| const existing = this.debounceTimers.get(key); | ||
|
|
||
| if (existing) { | ||
| clearTimeout(existing); | ||
| } | ||
|
|
||
| this.debounceTimers.set( | ||
| key, | ||
| setTimeout(() => { | ||
| this.debounceTimers.delete(key); | ||
|
|
||
| void this.reloadNotebooksForFile(uri); | ||
| }, debounceTimeInMilliseconds) | ||
| ); | ||
| } | ||
|
|
||
| private cellsMatchNotebook(notebook: NotebookDocument, newCells: NotebookCellData[]): boolean { | ||
| const liveCells = notebook.getCells(); | ||
|
|
||
| if (liveCells.length !== newCells.length) { | ||
| return false; | ||
| } | ||
|
|
||
| return liveCells.every( | ||
| (live, i) => live.document.getText() === newCells[i].value && live.kind === newCells[i].kind | ||
| ); | ||
| } | ||
|
|
||
| private async reloadNotebooksForFile(uri: Uri): Promise<void> { | ||
| const uriString = uri.toString(); | ||
|
|
||
| const affectedNotebooks = workspace.notebookDocuments.filter( | ||
| (doc) => | ||
| doc.notebookType === 'deepnote' && doc.uri.with({ query: '', fragment: '' }).toString() === uriString | ||
| ); | ||
|
|
||
| if (affectedNotebooks.length === 0) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Add blank lines before return statements.
Several early returns don’t have a preceding blank line; apply consistently across this file.
Proposed fix (illustrative)
private getBlockIdFromMetadata(metadata: Record<string, unknown> | undefined): string | undefined {
+
return (metadata?.id ?? metadata?.__deepnoteBlockId) as string | undefined;
}
private handleFileChange(uri: Uri): void {
if (isSnapshotFile(uri)) {
+
return;
}🤖 Prompt for AI Agents
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts` around lines 53 - 110,
Add a blank line immediately before each early return to follow the whitespace
guideline: in handleFileChange insert a blank line before the "return" inside
the isSnapshotFile check; in cellsMatchNotebook add a blank line before "return
false" after the liveCells length check; in reloadNotebooksForFile add a blank
line before "return" when affectedNotebooks.length === 0; and in
getBlockIdFromMetadata (single-line return) convert to a short body with a blank
line before the return if you expand it, or ensure surrounding const/grouped
declarations have a blank line before the return; keep formatting consistent
across these methods.
- Alphabetize private fields and methods - Add blank lines before return statements - Extract timing magic numbers into named constants in tests - Use block-body arrow in forEach to avoid implicit return - Update misleading test comment
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 99-131: The deserialization CancellationTokenSource in
reloadNotebooksForFile is ephemeral and never cancelled; change to track a
per-file CancellationTokenSource map (e.g. this.deserializeTokenSources keyed by
uri.toString()) so that before creating a new CancellationTokenSource you cancel
and dispose any existing token for that uri, pass the new token into
this.serializer.deserializeNotebook, and ensure those token sources are disposed
when the watcher is disposed to tie cancellation to real lifecycle events and
prevent overlapping deserializations.
dinohamzic
reviewed
Feb 10, 2026
| * Watches .deepnote files for external changes and reloads open notebook editors. | ||
| * | ||
| * When AI agents (Cursor, Claude Code) modify a .deepnote file on disk, | ||
| * VS Code's NotebookSerializer does not reliably detect and reload the notebook. |
Contributor
There was a problem hiding this comment.
Curious if you figured out why this is the case / root cause? What makes a .deepnote file behave differently?
Contributor
Author
There was a problem hiding this comment.
I think it's mostly because our multiple editors (notebooks) are in a single file setup.
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.
Fixes #315
Summary by CodeRabbit
New Features
Tests
Chores