feat: Watch snapshot files and update notebook outputs#331
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRestores and expands DeepnoteDataConverter (async initialize, findConverter, richer output transformers and fallbacks), adds per-notebook operation queuing/coalescing to DeepnoteFileChangeWatcher with snapshot-output-update and main-file-sync paths, implements deterministic self-write tracking and debouncing, integrates SnapshotService for snapshot reads/writes and notifications, adds snapshot filename parsing and project slugification utilities, replaces YAML library in a sidecar writer, improves serializer snapshot logging, and expands unit tests for snapshot and watcher workflows. Sequence Diagram(s)sequenceDiagram
actor FSW as FileSystemWatcher
participant Q as Per-Notebook Queue
participant Svc as SnapshotService
participant Reader as FileReader/Deserializer
participant Conv as DeepnoteDataConverter
participant Exec as ExecutionController
participant Editor as NotebookEditor
participant Meta as MetadataRestorer
FSW->>Svc: wasRecentlyWritten(uri)?
alt recently written
FSW->>FSW: ignore event
else not recent
FSW->>Q: enqueue(main-file-sync | snapshot-output-update)
Q->>Q: coalesce & drainQueue(nbKey)
alt main-file-sync
Q->>Reader: read & deserialize main file
Reader->>Reader: contentActuallyChanged?
Reader->>Conv: preserve live outputs by block ID
Reader->>Editor: apply replaceCells + metadata
Editor->>Meta: restore/append block IDs
Editor->>Svc: trackWrittenUri(file)
else snapshot-output-update
Q->>Reader: read snapshot file
Reader->>Conv: transformOutputsForVsCode(outputs)
Conv->>Exec: tryApplyOutputsViaExecution(outputs)
alt execution API available & succeeds
Exec->>Meta: restore metadata for fallback IDs
Exec->>Svc: trackWrittenUri(file)
else
Conv->>Editor: replaceCells with snapshot outputs
Editor->>Meta: restore/append block IDs
Editor->>Svc: trackWrittenUri(file)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/notebooks/deepnote/deepnoteDataConverter.ts`:
- Line 300: The public method transformOutputsForVsCode is located after private
methods; move transformOutputsForVsCode up into the public-methods section of
the DeepnoteDataConverter class so all public methods appear before private ones
and are ordered alphabetically with the other public methods (reposition
transformOutputsForVsCode among the public members accordingly).
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 137-159: outputsMatchNotebook currently only compares output array
lengths which can cause redundant edits; update outputsMatchNotebook to perform
a deep/content comparison per output instead of just length. For each cell with
a blockId (use getBlockIdFromMetadata) retrieve snapshotBlockOutputs and then
iterate outputs comparing cell.outputs[i] to snapshotBlockOutputs[i] by a
stable/content equality (e.g., compare relevant fields or JSON-serialize
DeepnoteOutput objects) and return false on the first mismatch; keep
short-circuit behavior for performance and ensure types reference DeepnoteOutput
and NotebookDocument when accessing outputs.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 440-455: The test creates a DeepnoteFileChangeWatcher instance
(noSnapshotWatcher) but never disposes it; update the test to call its
dispose/teardown method after assertions to avoid leaking resources—locate the
DeepnoteFileChangeWatcher creation in the test ('noSnapshotWatcher = new
DeepnoteFileChangeWatcher(...)') and add a cleanup step (e.g.,
noSnapshotWatcher.dispose() or noSnapshotWatcher.deactivate()/disposing the
returned Disposable) in a finally block or after the assertions so the watcher
is properly disposed even on failure.
In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 345-351: The timeout duration in trackWrittenUri is a magic
number; extract it as a named constant (e.g. SELF_WRITE_TTL_MS) declared near
the top of the module with other constants and replace the literal 2000 in
trackWrittenUri with that constant, leaving the logic that adds and later
deletes the key from recentlyWrittenUris unchanged.
When an external process writes a snapshot file, detect the change and update the open notebook's cell outputs. Adds self-write tracking to SnapshotService to avoid re-processing snapshots the extension itself wrote, and debounces by projectId so timestamped+latest writes coalesce.
Artmann
force-pushed
the
feat/watch-snapshot-files
branch
from
02725db to
c42737a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
=====================================
Coverage ? 0
=====================================
Files ? 0
Lines ? 0
Branches ? 0
=====================================
Hits ? 0
Misses ? 0
Partials ? 0 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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)
335-347:⚠️ Potential issue | 🟡 MinorTie deserialization token to watcher lifecycle. Currently, a fresh
CancellationTokenSourceis created and immediately disposed on every file change—never actually cancelled. Instead, create a lifecycle-bound source and reuse it across calls.♻️ Suggested change
- const tokenSource = new CancellationTokenSource(); let newData; try { - newData = await this.serializer.deserializeNotebook(content, tokenSource.token); + newData = await this.serializer.deserializeNotebook(content, this.reloadTokenSource.token); } catch (error) { logger.warn(`[FileChangeWatcher] Failed to parse changed file: ${uri.path}`, error); return; - } finally { - tokenSource.dispose(); }// class field (place alphabetically with other private fields) private readonly reloadTokenSource = new CancellationTokenSource(); // in activate() this.disposables.push({ dispose: () => this.reloadTokenSource.cancel() }); this.disposables.push(this.reloadTokenSource);Per coding guidelines: "Use real cancellation tokens tied to lifecycle events instead of fake/never-cancelled tokens."
🤖 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 335 - 347, Replace the ephemeral CancellationTokenSource created inside the file-change handler with a lifecycle-bound field so deserialization can be cancelled when the watcher is disposed; add a private readonly reloadTokenSource = new CancellationTokenSource() to the class, use reloadTokenSource.token when calling this.serializer.deserializeNotebook, and in activate push both this.reloadTokenSource and a disposable that calls this.reloadTokenSource.cancel() into this.disposables so the token is cancelled on watcher disposal instead of being created-and-disposed per change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kernels/deepnote/environments/deepnoteExtensionSidecarWriter.node.ts`:
- Around line 2-3: Replace the YAML import in
deepnoteExtensionSidecarWriter.node.ts to use the existing js-yaml package:
change the module imported for the symbol YAML from 'yaml' to 'js-yaml' so the
code uses the same library as deepnoteSerializer.ts, snapshotService.ts, and
deepnoteExplorerView.ts and matches declared dependencies.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 35-41: Reorder the private fields in class
DeepnoteFileChangeWatcher so they follow alphabetical order by name while
preserving accessibility and modifiers: place the private readonly serializer:
DeepnoteNotebookSerializer declaration before private readonly suppressionTimers
= new Map<string, ReturnType<typeof setTimeout>>(); ensure other private fields
(converter, debounceTimers, lastSnapshotFingerprints,
recentlySnapshotUpdatedUris) remain in alphabetical order and keep their
readonly and type annotations intact.
In `@src/notebooks/deepnote/deepnoteProjectUtils.ts`:
- Around line 4-8: In readDeepnoteProjectFile, add a single blank line before
the return statement: after the const assignments (fileContent, yamlContent,
projectData) insert an empty line so the return projectData; is separated for
readability; locate this in the readDeepnoteProjectFile function that uses
workspace.fs.readFile, TextDecoder, and deserializeDeepnoteFile.
In `@src/notebooks/deepnote/snapshots/snapshotFiles.ts`:
- Around line 22-34: The function extractProjectIdFromSnapshotUri should include
a blank line before the final return for readability; after the guard that
checks firstUnderscore/lastUnderscore and before the line "return
stem.slice(firstUnderscore + 1, lastUnderscore);" insert a single empty line
(keeping the existing const variables basename, stem, firstUnderscore,
lastUnderscore intact).
In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 347-373: The private helper trackWrittenUri is defined between
public methods (wasRecentlyWritten and mergeOutputsIntoBlocks), violating the
accessibility ordering; move the trackWrittenUri method out of the public-method
group into the class's private section (near other private methods/properties)
so all public methods like wasRecentlyWritten and mergeOutputsIntoBlocks remain
together and private methods remain grouped and alphabetized by name.
- Around line 832-841: The loop over data.project.notebooks and their blocks
should not abort on a single malformed block; wrap the inner per-block
processing (where you increment totalBlocks, check block.outputs, and set
outputsMap with block.id/DeepnoteOutput) in a try/catch so exceptions for one
block are logged/ignored and the loop continues; specifically, in the code
around deserializeDeepnoteFile(...) and the for (const block of notebook.blocks)
loop, catch errors per iteration (log a brief message including block.id and
continue) rather than letting an exception bubble out and stop processing.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 335-347: Replace the ephemeral CancellationTokenSource created
inside the file-change handler with a lifecycle-bound field so deserialization
can be cancelled when the watcher is disposed; add a private readonly
reloadTokenSource = new CancellationTokenSource() to the class, use
reloadTokenSource.token when calling this.serializer.deserializeNotebook, and in
activate push both this.reloadTokenSource and a disposable that calls
this.reloadTokenSource.cancel() into this.disposables so the token is cancelled
on watcher disposal instead of being created-and-disposed per change.
---
Duplicate comments:
In `@src/notebooks/deepnote/deepnoteDataConverter.ts`:
- Around line 300-306: The public method transformOutputsForVsCode is located
after private helper methods; move it into the public-methods section and place
it alphabetically among other public members to follow the project's ordering
rule (accessibility first, then alphabetical). Locate transformOutputsForVsCode
and cut/paste it so it appears with other public methods (not after private
helpers), and ensure its placement respects alphabetical order relative to other
public method names in the class; no logic changes are needed.
In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 355-369: The 2,000ms hardcoded TTL in trackWrittenUri should be
extracted to a named module-level constant (e.g. RECENTLY_WRITTEN_TTL_MS or
SELF_WRITE_TTL_MS) placed near the top of the file; replace the numeric literal
in setTimeout(...) with that constant and keep the rest of the logic unchanged
so trackWrittenUri(uri: Uri) continues to clear and delete timers using the new
constant.
src/kernels/deepnote/environments/deepnoteExtensionSidecarWriter.node.ts
Show resolved
Hide resolved
Add required metadata field and use string version in test YAML data to match updated @deepnote/blocks validation. Fix cspell error in snapshot files test.
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/snapshots/snapshotService.ts`:
- Around line 350-356: The method wasRecentlyWritten is out of alphabetical
order among public methods; cut the entire wasRecentlyWritten(uri: Uri): boolean
{ return this.recentlyWrittenUris.has(uri.toString()); } and paste it
immediately after the existing stripOutputsFromBlocks(...) method so public
methods are alphabetized; verify no references or comments break and run
tests/linter to confirm ordering is satisfied.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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)
124-640: 🧹 Nitpick | 🔵 TrivialPrivate methods not alphabetized.
Methods are grouped by logical flow (self-write → file-change → queue → execute). Per guidelines, alphabetical ordering is expected. Consider reordering:
clearAllTimers,consumeSelfWrite,contentActuallyChanged,drainQueue,enqueueMainFileSync,enqueueSnapshotOutputUpdate,executeMainFileSync,executeSnapshotOutputUpdate,getBlockIdFromMetadata,handleFileChange,handleSnapshotFileChange,markSelfWrite,outputsMatch,tryApplyOutputsViaExecution.🤖 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 124 - 640, The private methods in deepnoteFileChangeWatcher.ts are not alphabetized; reorder the private method declarations to follow alphabetical order as requested (clearAllTimers, consumeSelfWrite, contentActuallyChanged, drainQueue, enqueueMainFileSync, enqueueSnapshotOutputUpdate, executeMainFileSync, executeSnapshotOutputUpdate, getBlockIdFromMetadata, handleFileChange, handleSnapshotFileChange, markSelfWrite, outputsMatch, tryApplyOutputsViaExecution) while preserving each method's implementation and any internal references (e.g., markSelfWrite, consumeSelfWrite, handleFileChange, enqueueMainFileSync, drainQueue, executeMainFileSync, executeSnapshotOutputUpdate, tryApplyOutputsViaExecution, outputsMatch, getBlockIdFromMetadata) so behavior and call sites remain unchanged.
🤖 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 59-82: Reorder the private fields in DeepnoteFileChangeWatcher to
follow the project's guideline (group by accessibility then alphabetical): move
and sort the private properties so the sequence becomes converter,
debounceTimers, pendingOperations, runningOperations, selfWriteCounts,
selfWriteTimers, serializer, snapshotSelfWriteTimers, snapshotSelfWriteUris;
update only the declaration order (leave types, initializers and comments
intact) referencing the existing symbols converter, debounceTimers,
pendingOperations, runningOperations, selfWriteCounts, selfWriteTimers,
serializer, snapshotSelfWriteTimers, snapshotSelfWriteUris.
- Around line 588-599: The loop currently applies outputs one-by-one (for const
update of cellUpdates) so if createNotebookCellExecution/update.replaceOutput
fails mid-loop earlier cells are already mutated; change the flow to first map
cellUpdates to an array of executions via
selectedController.controller.createNotebookCellExecution(update.cell) without
calling start/replace/end, then in a separate step call execution.start() for
all executions, then await execution.replaceOutput(...) for all, then call
execution.end(true) for all (and keep the existing try/catch around the whole
sequence); on any error log via logger.warn(`[FileChangeWatcher] Execution API
failed, falling back to replaceCells`, error) and return false so the existing
fallback to replaceCells runs.
In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 351-376: Reorder the public methods to follow accessibility then
alphabetical order by moving mergeOutputsIntoBlocks so it appears immediately
after isSnapshotsEnabled and before onFileWritten; specifically place the
mergeOutputsIntoBlocks method above onFileWritten (so the sequence becomes
isSnapshotsEnabled → mergeOutputsIntoBlocks → onFileWritten → readSnapshot →
stripOutputsFromBlocks → wasRecentlyWritten) while keeping each method's
implementation unchanged (symbols: isSnapshotsEnabled, mergeOutputsIntoBlocks,
onFileWritten, readSnapshot, stripOutputsFromBlocks, wasRecentlyWritten).
- Around line 127-129: The three private fields are out of the required
alphabetical order: move the declaration of recentlyWrittenTimers to come before
recentlyWrittenUris so fields read: fileWrittenCallbacks, recentlyWrittenTimers,
recentlyWrittenUris; update the declarations for recentlyWrittenTimers
(Map<string, ReturnType<typeof setTimeout>>) and recentlyWrittenUris
(Set<string>) in snapshotService.ts so their order follows the
accessibility-then-alphabetical guideline and nothing else changes.
- Around line 1001-1003: The loop invoking each callback from
this.fileWrittenCallbacks currently lets a thrown exception abort the remaining
callbacks; update the invocation in snapshotService so each callback(uri) is
wrapped in a try/catch that logs the error and continues (e.g. try {
callback(uri) } catch (err) { this.logger?.error?.('fileWritten callback
failed', err) || console.error('fileWritten callback failed', err) }); reference
the this.fileWrittenCallbacks iteration and the callback(uri) call when making
the change so every callback runs even if one throws and errors include the
error object/stack for debugging.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 124-640: The private methods in deepnoteFileChangeWatcher.ts are
not alphabetized; reorder the private method declarations to follow alphabetical
order as requested (clearAllTimers, consumeSelfWrite, contentActuallyChanged,
drainQueue, enqueueMainFileSync, enqueueSnapshotOutputUpdate,
executeMainFileSync, executeSnapshotOutputUpdate, getBlockIdFromMetadata,
handleFileChange, handleSnapshotFileChange, markSelfWrite, outputsMatch,
tryApplyOutputsViaExecution) while preserving each method's implementation and
any internal references (e.g., markSelfWrite, consumeSelfWrite,
handleFileChange, enqueueMainFileSync, drainQueue, executeMainFileSync,
executeSnapshotOutputUpdate, tryApplyOutputsViaExecution, outputsMatch,
getBlockIdFromMetadata) so behavior and call sites remain unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.tssrc/notebooks/deepnote/snapshots/snapshotService.ts
tkislan
reviewed
Feb 23, 2026
Comment on lines
+27
to
+32
| const stem = basename.slice(0, -SNAPSHOT_FILE_SUFFIX.length); | ||
| const firstUnderscore = stem.indexOf('_'); | ||
| const lastUnderscore = stem.lastIndexOf('_'); | ||
| if (firstUnderscore === -1 || lastUnderscore === -1 || firstUnderscore === lastUnderscore) { | ||
| return undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
I would just use regex here. It will also be much more readable
…iles # Conflicts: # src/notebooks/deepnote/deepnoteDataConverter.ts # src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts # src/notebooks/deepnote/deepnoteProjectUtils.ts # src/notebooks/deepnote/deepnoteProjectUtils.unit.test.ts # src/notebooks/deepnote/deepnoteSerializer.ts # src/notebooks/deepnote/deepnoteSerializer.unit.test.ts # src/notebooks/deepnote/snapshots/snapshotService.ts
- Rewrite extractProjectIdFromSnapshotUri with regex (tkislan) - Fix alphabetical ordering of fields and public methods in snapshotService.ts - Fix alphabetical ordering of private fields in deepnoteFileChangeWatcher.ts - Add try/catch for callbacks in trackWrittenUri to prevent one failure from aborting remaining callbacks - Collect all executions before starting in tryApplyOutputsViaExecution to reduce partial update risk
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)src/notebooks/deepnote/snapshots/snapshotService.ts (1)
101-149:⚠️ Potential issue | 🟡 MinorGood self-write tracking; clear
recentlyWrittenUrison dispose and iterate callbacks defensivelyOn dispose you clear timers but leave
recentlyWrittenUrispopulated (stale “recent write” state). Also consider iteratingfor (const cb of [...this.fileWrittenCallbacks])so callbacks can unregister safely without affecting the current loop.Also applies to: 983-1007
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/snapshots/snapshotService.ts` around lines 101 - 149, The dispose callback in SnapshotService currently clears recentlyWrittenTimers but leaves recentlyWrittenUris populated and may iterate fileWrittenCallbacks directly, which can break if callbacks unregister during iteration; update the dispose logic in SnapshotService to also clear this.recentlyWrittenUris and to iterate callbacks defensively using a shallow copy (e.g., for (const cb of [...this.fileWrittenCallbacks])) wherever fileWrittenCallbacks are invoked so removals during iteration are safe; ensure you update both the dispose block that clears timers and any places that call fileWrittenCallbacks (methods referencing fileWrittenCallbacks, recentlyWrittenTimers, and recentlyWrittenUris).src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (2)
58-659: 🛠️ Refactor suggestion | 🟠 MajorMember ordering: private methods aren’t alphabetical
Within the private section, methods are currently out of alphabetical order (e.g.,
markSelfWriteprecedesconsumeSelfWrite,clearAllTimersis last, etc.). Please reorder private methods alphabetically to match repo conventions.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 58 - 659, Private methods are out of alphabetical order; reorder the private methods (not the fields) so they follow alphabetical order by name while preserving behavior and references. Specifically, move and re-sequence the methods into this order: clearAllTimers, consumeSelfWrite, contentActuallyChanged, drainQueue, enqueueMainFileSync, enqueueSnapshotOutputUpdate, executeMainFileSync, executeSnapshotOutputUpdate, getBlockIdFromMetadata, handleFileChange, handleSnapshotFileChange, markSelfWrite, outputsMatch, tryApplyOutputsViaExecution; ensure no logic is changed, all callers (e.g., enqueueMainFileSync, drainQueue, executeSnapshotOutputUpdate) keep the same signatures and that any timers/maps/fields (debounceTimers, pendingOperations, selfWriteCounts, snapshotSelfWriteUris, etc.) remain declared where they are.
330-410:⚠️ Potential issue | 🟠 MajorConstruct
NotebookCellDatainstances instead of spreading plain objects
const newCells = newData.cells.map((cell) => ({ ...cell }));creates plain objects that lose theNotebookCellDataprototype. While structurally compatible withNotebookEdit.replaceCells, this deviates from the codebase pattern (used elsewhere in this file and throughout the repo) of constructing instances vianew NotebookCellData(...). Future VS Code versions may rely on prototype checks, causing runtime failures.🤖 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 330 - 410, The newCells array is built by spreading plain objects (newCells = newData.cells.map((cell) => ({ ...cell }))) which loses the NotebookCellData prototype; change executeMainFileSync to construct real NotebookCellData instances (using new NotebookCellData(...) with appropriate kind, value, language, and metadata) for each entry produced by this.serializer.deserializeNotebook before passing to NotebookEdit.replaceCells so the cells retain their proper prototype and behavior.
🤖 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/deepnoteDataConverter.ts`:
- Around line 438-460: Preserve existing block identity/order when converting
unknown types: in createFallbackBlock, if the source NotebookCellData.metadata
contains an existing id (check keys deepnoteBlockId, id, and __deepnoteBlockId)
and/or a sorting key (check deepnoteSortingKey and sortingKey), reuse those
values instead of always calling generateBlockId()/generateSortingKey(index);
otherwise fall back to generating new values. In createFallbackCell, populate
the cell.metadata with those same canonical keys (deepnoteBlockId plus also set
id and __deepnoteBlockId) and preserve deepnoteSortingKey/sortingKey and
deepnoteMetadata so round-tripping does not change identity/order; keep other
existing metadata fields intact. Ensure you update references to DeepnoteBlock,
createFallbackBlock, and createFallbackCell accordingly.
- Around line 427-437: Wrap each iteration of the outputs conversion loop in a
try/catch so a malformed JSON.parse or other per-item error doesn't abort the
whole conversion; on catch, log or skip the offending output and continue.
Additionally, when converting outputs back (the inverse of
transformOutputsForVsCode), add a case for CHART_BIG_NUMBER_MIME_TYPE to restore
it as "text/plain" (i.e., map CHART_BIG_NUMBER_MIME_TYPE to "text/plain" and
preserve the original string payload), and ensure JSON.parse is only applied
inside the try block for each item. Target the loop that processes transformed
outputs and the code path that reverses transformOutputsForVsCode to implement
these changes.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 525-562: In the snapshot fallback path (after applying
WorkspaceEdit in deepnoteFileChangeWatcher.ts) calling
markSelfWrite(notebook.uri) without ensuring a save can suppress real external
changes; update this code to either (A) remove the markSelfWrite call and rely
on contentActuallyChanged to filter self-changes, or (B) only call markSelfWrite
after you successfully invoke workspace.save(notebook.uri) (or call
workspace.save immediately and then mark), or (C) if keeping the marker, set a
very short TTL in markSelfWrite so it expires before external edits; modify the
logic around applyEdit and markSelfWrite accordingly (referencing markSelfWrite,
contentActuallyChanged, workspace.applyEdit, workspace.save).
- Around line 644-659: clearAllTimers currently clears debounceTimers,
selfWriteTimers, and snapshotSelfWriteTimers but leaves suppression state in
selfWriteCounts and snapshotSelfWriteUris, which can cause stale suppression
after cleanup; update clearAllTimers to also reset/clear selfWriteCounts and
snapshotSelfWriteUris (and any related per-file state) after clearing timers so
all in-memory watcher state is fully cleared — locate the clearAllTimers method
and add calls to clear selfWriteCounts and snapshotSelfWriteUris (and any
analogous containers) to ensure deactivation cleanup removes both timers and
suppression state.
- Around line 282-325: enqueueSnapshotOutputUpdate currently enqueues a
project-scoped snapshot op per notebook, but
executeSnapshotOutputUpdate(projectId) updates all notebooks for that project
causing races/duplicates; fix by turning snapshot ops into notebook-scoped ops:
when setting this.pendingOperations in enqueueSnapshotOutputUpdate include the
target notebook identifier (e.g. notebook.uri.toString()) on the op object,
change the drainQueue handling for 'snapshot-output-update' to pass that
notebook identity (or the NotebookDocument) into executeSnapshotOutputUpdate,
and update the executeSnapshotOutputUpdate signature/implementation so it only
updates the single target notebook instead of iterating all notebooks for the
project; ensure pendingOperations keys and runningOperations behavior remain
per-notebook so only one op runs for that notebook at a time.
- Around line 580-604: The tryApplyOutputsViaExecution method can leave notebook
cells stuck "executing" if exec.replaceOutput throws because exec.end() is only
called in the outer flow; modify the loop over executions (created from
controller.createNotebookCellExecution) so each exec.start() is paired with a
per-iteration try/finally (or try/catch) that always calls
exec.end(success:boolean) — call exec.end(true) on successful replaceOutput and
exec.end(false) in the finally/catch when replaceOutput throws, then re-throw or
handle the error as needed so the outer catch still logs and returns false;
update the loop inside tryApplyOutputsViaExecution accordingly.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 401-1107: Tests in the "snapshot file watching" suite rely on real
setTimeout waits (debounceWaitMs / rapidChangeIntervalMs) which slows and
destabilizes CI; switch to Sinon fake timers by creating a clock
(sinon.useFakeTimers()) in setup and restoring it in teardown, then replace
ad-hoc sleeps (new Promise(resolve => setTimeout(...))) and waits that depend on
real time with clock.tick(debounceWaitMs) / clock.tick(rapidChangeIntervalMs)
(and advance the clock to let pending microtasks/promises run) and adapt waitFor
usages to use the fake clock so snapshotOnDidChange.fire /
snapshotOnDidCreate.fire events and assertions run deterministically without
real delays.
In `@src/notebooks/deepnote/deepnoteSerializer.ts`:
- Around line 124-158: The routine snapshot logging is too noisy; change the
non-error logger calls in the snapshot merge block to use logger.debug instead
of logger.info: update messages that report snapshots enabled, "Merging X block
outputs", "No outputs found in snapshot", and the "Snapshots disabled/service
not available, skipping snapshot merge" branch to logger.debug, while leaving
logger.error in the catch (and any real failure logs) unchanged; modify calls
around snapshotService.isSnapshotsEnabled(), snapshotService.readSnapshot(),
snapshotService.mergeOutputsIntoBlocks(...), and
converter.convertBlocksToCells(...) accordingly.
In `@src/notebooks/deepnote/snapshots/snapshotFiles.ts`:
- Around line 15-25: The current regex in extractProjectIdFromSnapshotUri
hardcodes ".snapshot.deepnote"; replace it by building the pattern from the
existing SNAPSHOT_FILE_SUFFIX constant to keep a single source of truth: create
or reuse an escapeRegExp utility to escape SNAPSHOT_FILE_SUFFIX, then construct
a RegExp like new RegExp(`^[a-z0-9-]+_(.+)_[^_]+${escapedSuffix}$`) and use that
against basename (keeping the existing basename extraction logic). Ensure
escapedSuffix includes escaped dots so the RegExp matches literally and preserve
the function's return of match?.[1].
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 58-659: Private methods are out of alphabetical order; reorder the
private methods (not the fields) so they follow alphabetical order by name while
preserving behavior and references. Specifically, move and re-sequence the
methods into this order: clearAllTimers, consumeSelfWrite,
contentActuallyChanged, drainQueue, enqueueMainFileSync,
enqueueSnapshotOutputUpdate, executeMainFileSync, executeSnapshotOutputUpdate,
getBlockIdFromMetadata, handleFileChange, handleSnapshotFileChange,
markSelfWrite, outputsMatch, tryApplyOutputsViaExecution; ensure no logic is
changed, all callers (e.g., enqueueMainFileSync, drainQueue,
executeSnapshotOutputUpdate) keep the same signatures and that any
timers/maps/fields (debounceTimers, pendingOperations, selfWriteCounts,
snapshotSelfWriteUris, etc.) remain declared where they are.
- Around line 330-410: The newCells array is built by spreading plain objects
(newCells = newData.cells.map((cell) => ({ ...cell }))) which loses the
NotebookCellData prototype; change executeMainFileSync to construct real
NotebookCellData instances (using new NotebookCellData(...) with appropriate
kind, value, language, and metadata) for each entry produced by
this.serializer.deserializeNotebook before passing to NotebookEdit.replaceCells
so the cells retain their proper prototype and behavior.
In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 101-149: The dispose callback in SnapshotService currently clears
recentlyWrittenTimers but leaves recentlyWrittenUris populated and may iterate
fileWrittenCallbacks directly, which can break if callbacks unregister during
iteration; update the dispose logic in SnapshotService to also clear
this.recentlyWrittenUris and to iterate callbacks defensively using a shallow
copy (e.g., for (const cb of [...this.fileWrittenCallbacks])) wherever
fileWrittenCallbacks are invoked so removals during iteration are safe; ensure
you update both the dispose block that clears timers and any places that call
fileWrittenCallbacks (methods referencing fileWrittenCallbacks,
recentlyWrittenTimers, and recentlyWrittenUris).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/snapshotFiles.tssrc/notebooks/deepnote/snapshots/snapshotService.ts
- Fix critical queue bug: snapshot ops now per-notebook instead of per-project - Add per-execution try/catch in tryApplyOutputsViaExecution to prevent stuck cells - Add per-item try/catch in transformOutputsForDeepnote output loop - Handle CHART_BIG_NUMBER_MIME_TYPE roundtrip in output conversion - Preserve block id/sortingKey in fallback converters for round-trip fidelity - Fix markSelfWrite in snapshot fallback: now followed by workspace.save - Clear selfWriteCounts/snapshotSelfWriteUris in clearAllTimers - Downgrade routine snapshot logs from info to debug in serializer - Build snapshot filename regex from SNAPSHOT_FILE_SUFFIX constant
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
181-187:⚠️ Potential issue | 🟡 MinorReorder public methods alphabetically.
Line 181 introduces another public method, but the public block isn’t alphabetical. Please reorder the public methods (
convert*,findConverter,initialize,transformOutputsForVsCode) to comply. 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/deepnoteDataConverter.ts` around lines 181 - 187, The public methods in DeepnoteDataConverter are out of alphabetical order; reorder the public method declarations so their names appear alphabetically within the public section: place convert* methods first (alphabetically among themselves), then findConverter, then initialize, then transformOutputsForVsCode; update the class so the public methods (convert*, findConverter, initialize, transformOutputsForVsCode) follow that alphabetical ordering without changing signatures or behavior.
🤖 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 405-408: The code sets a self-write marker via
this.markSelfWrite(notebook.uri) before calling await
workspace.save(notebook.uri); if save rejects the marker remains and may
suppress real external changes—wrap the save call in a try/catch (both the
main-file save and the snapshot-fallback save paths where markSelfWrite is used)
and on any error call the corresponding marker-clear method (e.g.,
this.clearSelfWrite(notebook.uri) or undo the marker) before rethrowing or
propagating the error; ensure you reference the same notebook.uri and perform
the clear in the catch so the marker is rolled back when save fails.
- Around line 15-23: The import for the third‑party type DeepnoteBlock is
currently placed with local imports; move the line "import type { DeepnoteBlock
} from '@deepnote/blocks';" up to join the other third‑party imports (keeping it
adjacent to other external modules) and ensure there is a single blank line
between the block of third‑party imports and the subsequent local imports
(IControllerRegistration, IExtensionSyncActivationService, IDisposableRegistry,
logger, IDeepnoteNotebookManager, DeepnoteDataConverter,
DeepnoteNotebookSerializer, extractProjectIdFromSnapshotUri, isSnapshotFile) so
third‑party and local imports are clearly separated.
- Around line 593-628: The outputsMatch function currently only compares output
items; update outputsMatch(liveOutputs, newOutputs) to also consider execution
metadata so snapshots aren't skipped when metadata like executionCount changes:
for each corresponding output in liveOutputs and newOutputs, compare
executionCount (or execution order field used) and shallow/deep-equal the
output.metadata (e.g., JSON.stringify comparison) in addition to the existing
item/mime/data checks, and return false if any metadata or executionCount
differs.
- Around line 454-471: The code falls back to 'code' when cell.metadata.type is
missing, losing the original block type; update the blockType computation in
deepnoteFileChangeWatcher.ts so it first uses cell.metadata?.type, then
originalBlocks[i]?.type, and only then defaults to 'code' (ensure casting to
DeepnoteBlock['type']); this will preserve original block types for
converter.transformOutputsForVsCode when metadata was lost and keep behavior of
the transformOutputsForVsCode call intact.
---
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteDataConverter.ts`:
- Around line 181-187: The public methods in DeepnoteDataConverter are out of
alphabetical order; reorder the public method declarations so their names appear
alphabetically within the public section: place convert* methods first
(alphabetically among themselves), then findConverter, then initialize, then
transformOutputsForVsCode; update the class so the public methods (convert*,
findConverter, initialize, transformOutputsForVsCode) follow that alphabetical
ordering without changing signatures or behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/snapshotFiles.ts
- Group third-party imports before local imports in file change watcher - Rollback self-write marker if workspace.save() fails - Preserve block type from originalBlocks fallback for non-code blocks - Include executionCount metadata in output equality check - Reorder public methods alphabetically in data converter
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 `@src/notebooks/deepnote/deepnoteDataConverter.ts`:
- Around line 551-565: The Deepnote output metadata assignment in
deepnoteDataConverter.ts is persisting transient execution-only fields (e.g.,
cellId, cellIndex, contentHash); update the block that builds deepnoteOutput
(the handling of output.metadata / restMetadata) to strip these runtime-only
keys before assigning metadata: filter out known transient keys from
output.metadata (after removing executionCount) and only attach the cleaned
metadata when it has remaining keys; refer to the deepnoteOutput construction
and the destructured restMetadata to locate where to apply the filter.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 125-183: The private methods in this block are out of alphabetical
order; move consumeSelfWrite so its declaration comes before markSelfWrite
(alphabetical by method name) while preserving their implementations and any
references; specifically, locate the private methods consumeSelfWrite(...) and
markSelfWrite(...) in deepnoteFileChangeWatcher.ts and reorder them so
consumeSelfWrite appears above markSelfWrite, keeping the timers and maps logic
and any clearTimeout/cleanup code unchanged.
- Around line 454-485: Wrap the body of the for-loop iterating over liveCells in
deepnoteFileChangeWatcher.ts in a per-iteration try/catch so a single conversion
error doesn’t abort the whole snapshot update; specifically, surround the logic
that calls this.getBlockIdFromMetadata(cell.metadata),
this.converter.transformOutputsForVsCode(...), the outputsMatch check
(this.outputsMatch), and the subsequent cellUpdates.push with a try block and
catch errors to log (or otherwise handle) conversion failures and continue to
the next cell, preserving blockIdFromFallback behavior when applicable.
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 393-396: The test uses an inline 200ms timeout as a magic number;
define a named constant (e.g., EXTRA_WAIT_MS or ADDITIONAL_TIMEOUT_MS) near the
other wait constants at the top of the module and replace the inline value in
the new Promise(setTimeout(..., 200)) with that constant; update any related
comments to reference the constant and ensure the test continues to await
waitFor(() => readFileCalls > 0) followed by await new Promise(resolve =>
setTimeout(resolve, EXTRA_WAIT_MS)).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts
- Strip transient cellId/cellIndex from output metadata before persisting - Reorder private methods alphabetically in DeepnoteFileChangeWatcher - Add per-cell try/catch in snapshot update loop for resilience - Extract magic number 200 into named autoSaveGraceMs constant in tests
Contributor
There was a problem hiding this comment.
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/deepnoteDataConverter.ts (1)
181-424:⚠️ Potential issue | 🟠 MajorAdd per-output try/catch in
transformOutputsForVsCodeA single malformed output (e.g., bad Vega spec) can abort the whole conversion. Wrap each output’s conversion in a try/catch and continue.
🔧 Suggested fix
- return outputs.map((output) => { - if ('output_type' in output) { + return outputs.map((output) => { + try { + if ('output_type' in output) { // existing logic... - return new NotebookCellOutput([]); + return new NotebookCellOutput([]); + } catch (error) { + console.warn('[DeepnoteDataConverter] Failed to convert output', error); + return new NotebookCellOutput([]); + } });As per coding guidelines, “Use per-iteration error handling in loops - wrap each iteration in try/catch so one failure doesn't stop the rest”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooks/deepnote/deepnoteDataConverter.ts` around lines 181 - 424, transformOutputsForVsCode currently maps over outputs so a single malformed output can throw and abort the whole conversion; wrap the body of the outputs.map callback (the logic that inspects output.output_type and builds NotebookCellOutput items) in a per-output try/catch inside transformOutputsForVsCode, catching any error when processing an individual output, logging or recording the error (include cellId and cellIndex) and returning a safe fallback NotebookCellOutput (empty items with the same metadata or an error NotebookCellOutputItem) for that output so the rest of the outputs continue to be converted; update references inside the catch to use the same metadata construction logic (cellId, blockMetadata, cellIndex) used elsewhere in transformOutputsForVsCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/notebooks/deepnote/deepnoteDataConverter.ts`:
- Around line 181-424: transformOutputsForVsCode currently maps over outputs so
a single malformed output can throw and abort the whole conversion; wrap the
body of the outputs.map callback (the logic that inspects output.output_type and
builds NotebookCellOutput items) in a per-output try/catch inside
transformOutputsForVsCode, catching any error when processing an individual
output, logging or recording the error (include cellId and cellIndex) and
returning a safe fallback NotebookCellOutput (empty items with the same metadata
or an error NotebookCellOutputItem) for that output so the rest of the outputs
continue to be converted; update references inside the catch to use the same
metadata construction logic (cellId, blockMetadata, cellIndex) used elsewhere in
transformOutputsForVsCode.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts
tkislan
approved these changes
Feb 24, 2026
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.
When an external process writes a snapshot file, detect the change and update the open notebook's cell outputs. Adds self-write tracking to SnapshotService to avoid re-processing snapshots the extension itself wrote, and debounces by projectId so timestamped+latest writes coalesce.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores