feat(deepnote-file): Migration to single notebook deepnote file

[tkislan]

Summary by CodeRabbit

• New Features

  • Status bar item and "Copy Active Deepnote Notebook Details" command.
  • Automatic splitting of multi-notebook Deepnote files into separate notebook files on open.
  • Creating/duplicating notebooks now produces new single‑notebook sibling files.

• Improvements

  • Explorer groups files by project and applies project-wide rename/export/delete.
  • Notebook‑scoped snapshots, deferred save behavior, and improved snapshot restore safety.
  • Better save/self‑write detection and project‑scoped server/environment handling; enforced unique notebook names.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR large-scale refactors Deepnote integration: adds DeepnoteAutoSplitter to split multi-notebook .deepnote files (and snapshots) into per-notebook files; rekeys server lifecycle and environment mapping to projectId; threads notebookId through serializer/manager/snapshot flows; restructures explorer into project groups and sibling .deepnote file semantics; introduces DeepnoteNotebookInfoStatusBar and the deepnote.copyNotebookDetails command; improves file-change handling (applyNotebookEdits, normalized self-write keys, save suppression); adds notebook file factory/hash helpers and extensive unit test and mock updates (including TabInputNotebook).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant VSCode
    participant Activation
    participant AutoSplitter
    participant FS
    participant Snapshot
    participant Explorer

    User->>VSCode: open Deepnote notebook
    VSCode->>Activation: onDidOpenNotebookDocument(uri)
    Activation->>Activation: deserializeDeepnoteFile(uri)
    Activation->>AutoSplitter: splitIfNeeded(uri, deepnoteFile)
    alt file contains multiple non-init notebooks
        AutoSplitter->>FS: write sibling .deepnote files
        AutoSplitter->>FS: overwrite original with primary notebook
        AutoSplitter->>Snapshot: splitSnapshots(projectId, notebookIds)
        Snapshot->>FS: write per-notebook snapshot files
        Snapshot->>FS: delete old combined snapshot
        AutoSplitter-->>Activation: return {wasSplit: true, newFiles}
        Activation->>Explorer: refreshTree()
    else no split required
        AutoSplitter-->>Activation: return {wasSplit: false}
    end

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• feat: Handle multiple notebooks using the sidebar. #12 — Overlapping changes to Deepnote Explorer, tree/provider, serializer/manager and project/group handling.

Suggested reviewers

• andyjakubowski

🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 46.88% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Updates Docs	⚠️ Warning	PR implements substantial architectural changes without corresponding documentation updates to README, specs, or changelog.	Update repository documentation, primary OSS repo docs, and internal roadmap to reflect new architecture changes.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main architectural change: migrating from multi-notebook to single-notebook Deepnote files, which aligns with the widespread refactoring across codebase structure, file handling, and project management.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0%. Comparing base (eb8d457) to head (bfd0cc3).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@     Coverage Diff     @@
##   main   #381   +/-   ##
===========================
===========================

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 15

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)

290-307: ⚠️ Potential issue | 🟠 Major

Filter out the init notebook here too.

This path reintroduces project.project.initNotebookId, so grouped multi-notebook files will show an internal init notebook as a normal explorer child.

Proposed fix

     private getNotebooksForProject(projectItem: DeepnoteTreeItem): DeepnoteTreeItem[] {
         const project = projectItem.data as DeepnoteProject;
-        const notebooks = project.project.notebooks || [];
+        const initNotebookId = project.project.initNotebookId;
+        const notebooks = (project.project.notebooks || []).filter((notebook) => notebook.id !== initNotebookId);
 
         return notebooks.map((notebook: DeepnoteNotebook) => {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteTreeDataProvider.ts` around lines 290 - 307,
getNotebooksForProject currently returns all notebooks including the project's
init notebook (project.project.initNotebookId), which causes the internal init
notebook to appear as a normal child; modify getNotebooksForProject to filter
out any notebook whose id matches project.project.initNotebookId before mapping
to DeepnoteTreeItem (i.e., compute notebooks.filter(n => n.id !==
project.project.initNotebookId) and then map that filtered list to new
DeepnoteTreeItem instances using the existing context construction and
TreeItemCollapsibleState.None).

src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (1)

538-593: ⚠️ Potential issue | 🟡 Minor

Rename these tests to match what they assert.

They no longer verify sorting; they verify that notebook order is preserved. Keeping “should sort...” in the names will mislead readers and make regressions harder to spot.

Also applies to: 595-650

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts` around lines
538 - 593, Rename the test case titles so they accurately describe behavior
being asserted: update the test with description "should sort notebooks
alphabetically by name within a project" (and the similar test around lines
595-650) to something like "should preserve original notebook order within a
project" or "should return notebooks in original order" so the test name matches
the assertions in DeepnoteTreeDataProvider unit tests that call
provider.getChildren and verify the original order of notebook items; update
only the test description strings for the test(...) calls (e.g., the one
constructing mockProjectWithNotebooks and mockProjectItem) to avoid misleading
readers.

src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (1)

270-286: ⚠️ Potential issue | 🟠 Major

notebookId is extracted, then discarded.

handleSnapshotFileChange() derives a notebookId, and this method filters editors with it, but the queued operation only keeps projectId. The downstream read still uses project-only snapshot lookup, so notebook-scoped snapshot files will not update the matching editor reliably.

Also applies to: 627-639

🤖 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 270 - 286,
enqueueSnapshotOutputUpdate currently filters by notebookId but only stores
projectId in the pending operation, causing notebook-scoped snapshot lookups to
be lost; update the pending operation stored in pendingOperations (keyed by
nbKey) to include notebookId (e.g., set { type: 'snapshot-output-update',
projectId, notebookId }) and ensure any code that reads this pending op
(including drainQueue and the similar block referenced around the second
occurrence) uses the notebookId when performing snapshot lookup/updates so
notebook-scoped snapshot files update the correct editor.

src/notebooks/deepnote/snapshots/snapshotService.ts (1)

401-445: ⚠️ Potential issue | 🟠 Major

Notebook-scoped reads dropped backward compatibility.

When notebookId is provided, this only searches *_${projectId}_${notebookId}_.... Existing legacy snapshots named *_${projectId}_... will stop restoring outputs after the migration unless the user reruns cells and generates new files.

🤖 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 401 - 445,
readSnapshot currently loses backward compatibility when notebookId is provided
because latestGlob and timestampedGlob only match filenames containing
_{projectId}_{notebookId}_..., skipping legacy files named *_{{projectId}}_...;
update readSnapshot to try both notebook-scoped and legacy patterns when
notebookId is present by constructing an additional legacy glob (e.g.,
`**/snapshots/*_${projectId}_...`) and searching with both patterns inside the
existing workspace.findFiles loop(s), falling back to the legacy match if no
notebook-scoped file is found; ensure the same dual-pattern logic is applied for
both the "latest" search (latestGlob) and the timestamped search
(timestampedGlob) before calling parseSnapshotFile so parseSnapshotFile,
RelativePattern, and workspace.findFiles are reused without changing their APIs.

🤖 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/deepnoteServerStarter.node.ts`:
- Around line 405-414: Extract the duplicated disposal logic into a single
helper method named disposeOutputListeners(fileKey): it should look up
this.disposablesByFile.get(fileKey), iterate disposables, call dispose() inside
a try/catch, and log warnings on exceptions (same behavior as current block).
Replace the duplicated blocks in monitorServerOutput, stopServerForEnvironment,
and dispose() to call this.disposeOutputListeners(fileKey) and ensure the helper
handles the case of no entry and leaves map state consistent. Keep the helper as
a private method on the same class and reuse it everywhere the original disposal
loop appears.

In `@src/kernels/deepnote/deepnoteTestHelpers.node.ts`:
- Around line 15-66: Replace the hand-rolled mock object with one that extends
Node's EventEmitter and uses real emitter methods
(emit/on/once/removeListener/listeners/etc.) and mutable internal state exposed
via getters for properties like killed and connected; specifically update the
mock that currently defines killed, connected, signalCode, spawnargs, spawnfile
and methods kill, send, disconnect, unref, ref and all the listener stubs so the
mock class (or factory) wraps EventEmitter, maintains internal booleans (e.g.,
_killed, _connected) that are updated by kill/disconnect/ref/unref and exposed
as getters killed and connected, forward listener methods to the EventEmitter
instance (or inherit them) and remove the hardcoded stub implementations and the
Symbol.dispose no-op in favor of proper EventEmitter behavior.

In `@src/notebooks/deepnote/deepnoteActivationService.ts`:
- Around line 59-63: The current registration of
workspace.onDidOpenNotebookDocument only handles future opens and misses
notebooks already open at activation; after you push the
onDidOpenNotebookDocument listener (or inside activate where
extensionContext.subscriptions is set up), iterate over
workspace.notebookDocuments and call the existing checkAndSplitIfNeeded(doc) for
each open notebook to ensure restored/.deepnote notebooks are processed—use the
same method name checkAndSplitIfNeeded and keep adding the subscriptions onto
extensionContext.subscriptions as appropriate.

In `@src/notebooks/deepnote/deepnoteAutoSplitter.ts`:
- Around line 133-160: snapshotGlob currently matches both legacy and new
snapshot filename forms but extractVariantFromSnapshotFilename only understands
the legacy shape, causing filenames like
{slug}_{projectId}_{notebookId}_{variant}.snapshot.deepnote to be mis-parsed and
re-suffixed; before calling splitSingleSnapshot for each snapshotUri in
allSnapshotFiles, detect and skip files that already match the new format (or
extend extractVariantFromSnapshotFilename to parse the new pattern) by checking
the filename parts for the extra notebookId segment (use snapshotUri.fsPath or
basename), and only call splitSingleSnapshot when the file is in the legacy
shape; update the loop around splitSingleSnapshot (and the identical block at
the other occurrence) to perform this guard so valid new-format snapshots are
left untouched.
- Around line 46-49: The filename generation for extraNotebooks (inside the loop
using slugifyNotebookName, originalStem, and Uri.joinPath) can collide when
notebookSlug is identical; update the logic to produce collision-proof names by
including a unique identifier (e.g., notebook.id or an incrementing index/UUID)
or by checking for existing target paths and appending a disambiguator until the
path is unique before writing; apply the same fix to the similar block
referenced around lines 283-289 so newFileName/ newFileUri never overwrite
earlier notebooks.
- Around line 120-290: The private methods in this class are out of order;
reorder them to follow the repo TS file-order rule (grouped by accessibility
then alphabetically). Move the private methods so they appear in alphabetical
order: computeSnapshotHash, extractVariantFromSnapshotFilename, getFileStem,
slugifyNotebookName, splitSingleSnapshot, splitSnapshots (use these exact
function names to locate each method) and leave their implementations unchanged.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 523-525: The tree item always opens the default notebook because
context.notebookId is ignored; fix the open logic in deepnoteExplorerView by
including context.notebookId when resolving the notebook (instead of just
Uri.file(context.filePath)). Pass the notebook identifier through to
workspace.openNotebookDocument (or construct a notebook URI using the
fragment/query) or call deserializeNotebook with the notebookId so the correct
notebook instance is opened; update the code paths around fileUri,
workspace.openNotebookDocument and deserializeNotebook to use
context.notebookId.
- Around line 79-80: The issue is that after creating or duplicating a notebook
the code appends the new notebook but reopens only the fileUri (e.g. via
saveProjectAndOpenNotebook(fileUri,...)), which lets
DeepnoteNotebookSerializer.findDefaultNotebook() pick the wrong notebook; modify
the create/duplicate flows to capture the new notebook's identifier (e.g. the
created/duplicated notebook object or its id/path) and pass that specific
notebook target into the reopen call (either by extending
saveProjectAndOpenNotebook to accept a notebookId/notebookUri parameter or by
calling whatever openNotebook/openEditor function exists with the precise
notebook target) so the editor opens the newly created/duplicated notebook
rather than the file's default. Ensure you update all affected call sites (the
create flow, duplicate flow and the other spots noted) to forward the new
notebook identifier.

In `@src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts`:
- Around line 189-190: The test creates two DeepnoteExplorerView instances
(view1 and view2) which each construct a DeepnoteTreeDataProvider that registers
file watchers; to avoid leaking watchers, dispose of these views at the end of
the test (or add an afterEach/tearDown) by calling their dispose method (or
otherwise invoking the DeepnoteExplorerView cleanup) so both view1 and view2 are
properly torn down; locate usages of DeepnoteExplorerView in the test and ensure
view1.dispose() and view2.dispose() (or centralized cleanup) are invoked.

In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 757-759: The test "should update outputs when snapshot file
changes" (and/or add a new unit test in deepnoteFileChangeWatcher.unit.test.ts)
currently only uses the legacy snapshot filename pattern produced by testFileUri
and createMockNotebook; update that test or add one to also exercise the new
per-notebook snapshot filename pattern produced by snapshotService (the
..._{projectId}_{notebookId}_{variant}.snapshot.deepnote shape) by calling
testFileUri with the new filename (including notebookId) and asserting the same
"happy-path/self-write" behavior so the watcher reacts as expected; reference
the existing test name and helpers (testFileUri, createMockNotebook) to locate
where to add the additional case.

In `@src/notebooks/deepnote/deepnoteNotebookManager.ts`:
- Around line 47-60: storeOriginalProject and updateOriginalProject both write
cloned DeepnoteProject objects into a single Map (originalProjects) keyed only
by projectId, causing different .deepnote files within the same project to
collide; change the cache to be per-file (either use a composite key like
`${projectId}:${fileId}` where fileId is the unique notebook/.deepnote filename
or switch originalProjects to a nested Map keyed first by projectId then by
fileId) and update both methods (storeOriginalProject, updateOriginalProject) to
use that per-file key, ensuring any other lookups that read originalProjects
also use the same composite or nested-key pattern so each notebook's cached
project is stored separately.

In `@src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts`:
- Around line 224-226: The inline comment above the test that says "Use
updateOriginalProject which doesn't set currentNotebookId" is stale; remove or
update that comment to reflect the new behavior being tested (e.g., that
updateOriginalProject is used and integrations should be updated and true
returned). Locate the comment near the test invoking
manager.updateOriginalProject('project-123', mockProject) and either delete the
sentence about currentNotebookId or replace it with a brief accurate description
of what the test now verifies.

In `@src/notebooks/deepnote/deepnoteTreeDataProvider.ts`:
- Around line 148-150: performInitialScan currently calls loadAllProjectFiles
and then the root render triggers getProjectGroups which repeats the same file
walk; to fix, capture the LoadedProjectFile[] result from loadAllProjectFiles in
performInitialScan (e.g. assign to a new class field like
this.loadedProjectFiles) and have getProjectGroups and any subsequent
group-building logic use that cached array instead of calling
loadAllProjectFiles again. Remove duplicate calls to loadAllProjectFiles found
around getProjectGroups, and add cache invalidation/update logic tied to file
system or workspace change handlers so the cached LoadedProjectFile[] stays in
sync.

In `@src/notebooks/deepnote/deepnoteTreeItem.ts`:
- Around line 78-79: The label construction uses
this.context.filePath.split('/') which fails on Windows; replace split('/')
usages with a platform-aware basename (e.g., import path from 'path' and call
path.basename(this.context.filePath)) when setting this.label and at the other
identical occurrence where filePath is parsed; ensure you fallback to
project.project.name || 'Untitled Project' exactly as before.

In `@src/test/mocks/vsc/extHostedTypes.ts`:
- Around line 2547-2554: The class TabInputNotebook declares two public readonly
properties out of alphabetical order; reorder the member declarations so public
readonly notebookType appears before public readonly uri to satisfy the repo
member-ordering rule, and keep the constructor (constructor(uri: vscUri.URI,
notebookType: string)) assignments consistent by assigning this.notebookType and
this.uri after reordering the property declarations.

---

Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 270-286: enqueueSnapshotOutputUpdate currently filters by
notebookId but only stores projectId in the pending operation, causing
notebook-scoped snapshot lookups to be lost; update the pending operation stored
in pendingOperations (keyed by nbKey) to include notebookId (e.g., set { type:
'snapshot-output-update', projectId, notebookId }) and ensure any code that
reads this pending op (including drainQueue and the similar block referenced
around the second occurrence) uses the notebookId when performing snapshot
lookup/updates so notebook-scoped snapshot files update the correct editor.

In `@src/notebooks/deepnote/deepnoteTreeDataProvider.ts`:
- Around line 290-307: getNotebooksForProject currently returns all notebooks
including the project's init notebook (project.project.initNotebookId), which
causes the internal init notebook to appear as a normal child; modify
getNotebooksForProject to filter out any notebook whose id matches
project.project.initNotebookId before mapping to DeepnoteTreeItem (i.e., compute
notebooks.filter(n => n.id !== project.project.initNotebookId) and then map that
filtered list to new DeepnoteTreeItem instances using the existing context
construction and TreeItemCollapsibleState.None).

In `@src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts`:
- Around line 538-593: Rename the test case titles so they accurately describe
behavior being asserted: update the test with description "should sort notebooks
alphabetically by name within a project" (and the similar test around lines
595-650) to something like "should preserve original notebook order within a
project" or "should return notebooks in original order" so the test name matches
the assertions in DeepnoteTreeDataProvider unit tests that call
provider.getChildren and verify the original order of notebook items; update
only the test description strings for the test(...) calls (e.g., the one
constructing mockProjectWithNotebooks and mockProjectItem) to avoid misleading
readers.

In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 401-445: readSnapshot currently loses backward compatibility when
notebookId is provided because latestGlob and timestampedGlob only match
filenames containing _{projectId}_{notebookId}_..., skipping legacy files named
*_{{projectId}}_...; update readSnapshot to try both notebook-scoped and legacy
patterns when notebookId is present by constructing an additional legacy glob
(e.g., `**/snapshots/*_${projectId}_...`) and searching with both patterns
inside the existing workspace.findFiles loop(s), falling back to the legacy
match if no notebook-scoped file is found; ensure the same dual-pattern logic is
applied for both the "latest" search (latestGlob) and the timestamped search
(timestampedGlob) before calling parseSnapshotFile so parseSnapshotFile,
RelativePattern, and workspace.findFiles are reused without changing their APIs.

🪄 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: c9598c5c-28d4-4c7f-98d8-4d917f6120bb

📥 Commits

Reviewing files that changed from the base of the PR and between 3b7356a and 477b790.

📒 Files selected for processing (28)

• build/mocha-esm-loader.js
• package.json
• package.nls.json
• src/kernels/deepnote/deepnoteServerStarter.node.ts
• src/kernels/deepnote/deepnoteTestHelpers.node.ts
• src/notebooks/deepnote/deepnoteActivationService.ts
• src/notebooks/deepnote/deepnoteAutoSplitter.ts
• src/notebooks/deepnote/deepnoteExplorerView.ts
• src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts
• src/notebooks/deepnote/deepnoteFileChangeWatcher.ts
• src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts
• src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts
• src/notebooks/deepnote/deepnoteNotebookManager.ts
• src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts
• src/notebooks/deepnote/deepnoteSerializer.ts
• src/notebooks/deepnote/deepnoteSerializer.unit.test.ts
• src/notebooks/deepnote/deepnoteTreeDataProvider.ts
• src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
• src/notebooks/deepnote/deepnoteTreeItem.ts
• src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
• src/notebooks/deepnote/snapshots/snapshotFiles.ts
• src/notebooks/deepnote/snapshots/snapshotService.ts
• src/notebooks/serviceRegistry.node.ts
• src/notebooks/serviceRegistry.web.ts
• src/notebooks/types.ts
• src/platform/common/constants.ts
• src/test/mocks/vsc/extHostedTypes.ts
• src/test/vscode-mock.ts

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

usage_files="$(rg -l --type=ts '\bcreateMockChildProcess\s*\(' || true)"
if [ -z "$usage_files" ]; then
  echo "No call sites found."
  exit 0
fi

echo "createMockChildProcess call sites:"
printf '%s\n' "$usage_files"

echo
echo "Potential event/state-dependent usages to inspect:"
while IFS= read -r file; do
  rg -n -C2 '\bcreateMockChildProcess\s*\(|\.(on|once|emit|kill|disconnect)\s*\(|\.(killed|connected)\b' "$file"
done <<< "$usage_files"

Repository: deepnote/vscode-deepnote

Length of output: 2366

---

🏁 Script executed:

# Check how mockProcess is actually used in the test files
for file in src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts \
            src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts \
            src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts; do
  if [ -f "$file" ]; then
    echo "=== $file ==="
    # Look for any usage of killed, connected, on, once, emit, kill, disconnect on the process mock
    rg -n '(\.killed|\.connected|\.on\(|\.once\(|\.emit\(|\.kill\(|\.disconnect\()\b' "$file" || echo "  (no state/event usage found)"
  fi
done

Repository: deepnote/vscode-deepnote

Length of output: 401

---

Refactor mock to use real EventEmitter and mutable state.

The mock's listener stubs and hardcoded killed/connected flags don't match ChildProcess semantics. While current tests don't use these features, a test that awaits process events or checks post-call state will get a lying mock. Use EventEmitter as a base and expose state via getters instead.

Suggested fix

+import { EventEmitter } from 'node:events';
 import type { ChildProcess } from 'node:child_process';
 
 export function createMockChildProcess(overrides?: Partial<ChildProcess>): ChildProcess {
-    const mockProcess: ChildProcess = {
+    const emitter = new EventEmitter();
+    let killed = false;
+    let connected = false;
+
+    const mockProcess = Object.assign(emitter, {
         pid: undefined,
         stdio: [null, null, null, null, null],
         stdin: null,
         stdout: null,
         stderr: null,
         exitCode: null,
-        killed: false,
-        connected: false,
+        get killed() {
+            return killed;
+        },
+        get connected() {
+            return connected;
+        },
         signalCode: null,
         spawnargs: [],
         spawnfile: '',
-        kill: () => true,
+        kill: () => {
+            killed = true;
+            return true;
+        },
         send: () => true,
-        disconnect: () => true,
-        unref: () => true,
-        ref: () => true,
-        addListener: function () {
-            return this;
-        },
-        emit: () => true,
-        on: function () {
-            return this;
-        },
-        once: function () {
-            return this;
-        },
-        removeListener: function () {
-            return this;
-        },
-        removeAllListeners: function () {
-            return this;
-        },
-        prependListener: function () {
-            return this;
-        },
-        prependOnceListener: function () {
-            return this;
+        disconnect: () => {
+            connected = false;
         },
         [Symbol.dispose]: () => {
-            return undefined;
-        },
-        off: function () {
-            return this;
-        },
-        setMaxListeners: function () {
-            return this;
-        },
-        getMaxListeners: () => 10,
-        listeners: function () {
-            return [];
-        },
-        rawListeners: function () {
-            return [];
-        },
-        eventNames: function () {
-            return [];
-        },
-        listenerCount: () => 0,
+            killed = true;
+        },
         ...overrides
-    };
+    }) as ChildProcess;
     return mockProcess;
 }

📝 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

	killed: false,
	connected: false,
	signalCode: null,
	spawnargs: [],
	spawnfile: '',
	kill: () => true,
	send: () => true,
	disconnect: () => true,
	unref: () => true,
	ref: () => true,
	addListener: function () {
	return this;
	},
	emit: () => true,
	on: function () {
	return this;
	},
	once: function () {
	return this;
	},
	removeListener: function () {
	return this;
	},
	removeAllListeners: function () {
	return this;
	},
	prependListener: function () {
	return this;
	},
	prependOnceListener: function () {
	return this;
	},
	[Symbol.dispose]: () => {
	return undefined;
	},
	off: function () {
	return this;
	},
	setMaxListeners: function () {
	return this;
	},
	getMaxListeners: () => 10,
	listeners: function () {
	return [];
	},
	rawListeners: function () {
	return [];
	},
	eventNames: function () {
	return [];
	},
	listenerCount: () => 0,
	import { EventEmitter } from 'node:events';
	import type { ChildProcess } from 'node:child_process';
	export function createMockChildProcess(overrides?: Partial<ChildProcess>): ChildProcess {
	const emitter = new EventEmitter();
	let killed = false;
	let connected = false;
	const mockProcess = Object.assign(emitter, {
	pid: undefined,
	stdio: [null, null, null, null, null],
	stdin: null,
	stdout: null,
	stderr: null,
	exitCode: null,
	get killed() {
	return killed;
	},
	get connected() {
	return connected;
	},
	signalCode: null,
	spawnargs: [],
	spawnfile: '',
	kill: () => {
	killed = true;
	return true;
	},
	send: () => true,
	disconnect: () => {
	connected = false;
	},
	[Symbol.dispose]: () => {
	killed = true;
	},
	...overrides
	}) as ChildProcess;
	return mockProcess;
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/kernels/deepnote/deepnoteTestHelpers.node.ts` around lines 15 - 66,
Replace the hand-rolled mock object with one that extends Node's EventEmitter
and uses real emitter methods (emit/on/once/removeListener/listeners/etc.) and
mutable internal state exposed via getters for properties like killed and
connected; specifically update the mock that currently defines killed,
connected, signalCode, spawnargs, spawnfile and methods kill, send, disconnect,
unref, ref and all the listener stubs so the mock class (or factory) wraps
EventEmitter, maintains internal booleans (e.g., _killed, _connected) that are
updated by kill/disconnect/ref/unref and exposed as getters killed and
connected, forward listener methods to the EventEmitter instance (or inherit
them) and remove the hardcoded stub implementations and the Symbol.dispose no-op
in favor of proper EventEmitter behavior.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Reorder the private methods to match the TS file-order rule.

The private members are not alphabetized, so this new file drifts from the repo’s class layout convention.

As per coding guidelines, **/*.{ts,tsx}: 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/deepnoteAutoSplitter.ts` around lines 120 - 290, The
private methods in this class are out of order; reorder them to follow the repo
TS file-order rule (grouped by accessibility then alphabetically). Move the
private methods so they appear in alphabetical order: computeSnapshotHash,
extractVariantFromSnapshotFilename, getFileStem, slugifyNotebookName,
splitSingleSnapshot, splitSnapshots (use these exact function names to locate
each method) and leave their implementations unchanged.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Don't rescan the workspace right after the initial scan.

performInitialScan() already walks every .deepnote file, then the first real root render calls getProjectGroups() and does the same discovery again. Cache the LoadedProjectFile[] from the scan and build groups from that instead.

Also applies to: 162-184, 189-190

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteTreeDataProvider.ts` around lines 148 - 150,
performInitialScan currently calls loadAllProjectFiles and then the root render
triggers getProjectGroups which repeats the same file walk; to fix, capture the
LoadedProjectFile[] result from loadAllProjectFiles in performInitialScan (e.g.
assign to a new class field like this.loadedProjectFiles) and have
getProjectGroups and any subsequent group-building logic use that cached array
instead of calling loadAllProjectFiles again. Remove duplicate calls to
loadAllProjectFiles found around getProjectGroups, and add cache
invalidation/update logic tied to file system or workspace change handlers so
the cached LoadedProjectFile[] stays in sync.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Match the repo member-ordering rule here.

uri and notebookType are both public readonly properties, so they should be alphabetized within the class for consistency with the TS style rule.

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/test/mocks/vsc/extHostedTypes.ts` around lines 2547 - 2554, The class
TabInputNotebook declares two public readonly properties out of alphabetical
order; reorder the member declarations so public readonly notebookType appears
before public readonly uri to satisfy the repo member-ordering rule, and keep
the constructor (constructor(uri: vscUri.URI, notebookType: string)) assignments
consistent by assigning this.notebookType and this.uri after reordering the
property declarations.

[coderabbitai]

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)

120-139: ⚠️ Potential issue | 🟡 Minor

URI key mismatch: snapshotSelfWriteUris populated without normalization.

onFileWritten callback stores uri.toString() (line 123), but consumeSelfWrite (line 173) checks using normalizeFileUri(uri). If a snapshot URI ever contains query/fragment, the self-write won't be detected.

🛠️ Suggested fix

 this.disposables.push(
     this.snapshotService.onFileWritten((uri) => {
-        const key = uri.toString();
+        const key = this.normalizeFileUri(uri);
         this.snapshotSelfWriteUris.add(key);

🤖 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 120 - 139,
The onFileWritten handler populates snapshotSelfWriteUris and
snapshotSelfWriteTimers using uri.toString(), but consumeSelfWrite looks up keys
via normalizeFileUri(uri), causing mismatches when URIs have queries/fragments;
update the onFileWritten callback in deepnoteFileChangeWatcher.ts to compute the
key with normalizeFileUri(uri) (use the same normalization for both the Set
snapshotSelfWriteUris and the Map snapshotSelfWriteTimers and for clearing
timers) so stored keys match consumeSelfWrite's lookup and timers are
tracked/cleared consistently.

🤖 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 398-406: The save failure handling is inconsistent: when
workspace.save(notebook.uri) returns undefined the function returns, but when it
throws the catch only logs and execution continues to the success path; update
the catch for workspace.save(notebook.uri) to mirror the undefined case (i.e.,
after logging the saveError via logger.warn include an early return or rethrow)
so both failure modes prevent reaching the subsequent success log; locate this
in deepnoteFileChangeWatcher.ts around the workspace.save(...) call and the
surrounding try/catch that references notebook.uri and logger.warn.

In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 131-143: The suite currently creates a temp dir with mkdtempSync
in suiteSetup and removes it in suiteTeardown, but if the test process crashes
the cleanup can be skipped; add a fail-safe removal by registering a process
exit handler that checks testFixturesDir and calls rmSync(testFixturesDir, {
recursive: true, force: true }) (and also handle SIGINT/SIGTERM) so the temp
folder is removed on abrupt termination; ensure testFixturesDir is assigned
before registering the handler and keep the existing suiteTeardown removal in
place, and reference the testFixturesDir variable and the
suiteSetup/suiteTeardown functions when making the change.

---

Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 120-139: The onFileWritten handler populates snapshotSelfWriteUris
and snapshotSelfWriteTimers using uri.toString(), but consumeSelfWrite looks up
keys via normalizeFileUri(uri), causing mismatches when URIs have
queries/fragments; update the onFileWritten callback in
deepnoteFileChangeWatcher.ts to compute the key with normalizeFileUri(uri) (use
the same normalization for both the Set snapshotSelfWriteUris and the Map
snapshotSelfWriteTimers and for clearing timers) so stored keys match
consumeSelfWrite's lookup and timers are tracked/cleared consistently.

🪄 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: a0848e8a-4b19-412e-9a1b-f4c2c6e89f74

📥 Commits

Reviewing files that changed from the base of the PR and between 477b790 and 1140fa0.

📒 Files selected for processing (2)

• src/notebooks/deepnote/deepnoteFileChangeWatcher.ts
• src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Inconsistent early-return on save failure.

If save() returns undefined, the method returns early. If save() throws, it logs and falls through to the success log at line 413.

🛠️ Suggested fix

             } catch (saveError) {
                 logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError);
+                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

	try {
	const saved = await workspace.save(notebook.uri);
	if (!saved) {
	logger.warn(`[FileChangeWatcher] Save after sync write returned undefined: ${notebook.uri.path}`);
	return;
	}
	} catch (saveError) {
	logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError);
	}
	try {
	const saved = await workspace.save(notebook.uri);
	if (!saved) {
	logger.warn(`[FileChangeWatcher] Save after sync write returned undefined: ${notebook.uri.path}`);
	return;
	}
	} catch (saveError) {
	logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError);
	return;
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts` around lines 398 - 406,
The save failure handling is inconsistent: when workspace.save(notebook.uri)
returns undefined the function returns, but when it throws the catch only logs
and execution continues to the success path; update the catch for
workspace.save(notebook.uri) to mirror the undefined case (i.e., after logging
the saveError via logger.warn include an early return or rethrow) so both
failure modes prevent reaching the subsequent success log; locate this in
deepnoteFileChangeWatcher.ts around the workspace.save(...) call and the
surrounding try/catch that references notebook.uri and logger.warn.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Temp directory with cleanup in suiteTeardown.

mkdtempSync + rmSync with recursive: true is correct. Ensure tests don't leave artifacts if the suite crashes mid-run.

🤖 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
131 - 143, The suite currently creates a temp dir with mkdtempSync in suiteSetup
and removes it in suiteTeardown, but if the test process crashes the cleanup can
be skipped; add a fail-safe removal by registering a process exit handler that
checks testFixturesDir and calls rmSync(testFixturesDir, { recursive: true,
force: true }) (and also handle SIGINT/SIGTERM) so the temp folder is removed on
abrupt termination; ensure testFixturesDir is assigned before registering the
handler and keep the existing suiteTeardown removal in place, and reference the
testFixturesDir variable and the suiteSetup/suiteTeardown functions when making
the change.

[coderabbitai]

Actionable comments posted: 6

♻️ Duplicate comments (3)

src/notebooks/deepnote/deepnoteFileChangeWatcher.ts (1)

398-406: ⚠️ Potential issue | 🟡 Minor

Return after workspace.save() throws.

The false/undefined path returns, but the exception path just logs and then falls through to the success log at Line 413.

Patch

             } catch (saveError) {
                 logger.warn(`[FileChangeWatcher] Save after sync write failed: ${notebook.uri.path}`, saveError);
+                return;
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts` around lines 398 - 406,
The catch for the workspace.save(notebook.uri) call currently only logs the
error (logger.warn) and then execution falls through to the success log; update
the catch to stop further processing by returning (or otherwise exiting) after
logging the saveError so the code does not incorrectly log a successful save for
notebook.uri.path. Ensure you modify the catch block that wraps
workspace.save(notebook.uri) (the logger.warn call that references
notebook.uri.path and saveError) to return immediately after logging.

src/notebooks/deepnote/deepnoteExplorerView.ts (2)

77-85: ⚠️ Potential issue | 🟠 Major

Creating a notebook still falls back to the file’s default notebook.

This path appends a second notebook into the same .deepnote file and then reopens only fileUri. Since deserialization now resolves a default notebook, the editor will usually land on the preexisting/default notebook instead of the one that was just created. Either create a new per-notebook file here, or carry the target notebook through the reopen path.

Also applies to: 493-510

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 77 - 85, The
current flow appends newNotebook to projectData.project.notebooks then calls
saveProjectAndOpenNotebook(fileUri), which reopens the .deepnote file and
usually selects the default notebook instead of the newly created one; fix by
carrying the target notebook through the reopen path: change
saveProjectAndOpenNotebook to accept an identifier/ordinal for the notebook
(e.g., newNotebook.id or the newNotebook object) and use that to select/open the
specific notebook after deserialization, then update this call site to pass
newNotebook (or newNotebook.id) instead of only fileUri; alternatively, if you
prefer per-notebook files, create a new .deepnote file for newNotebook instead
of appending to projectData.project.notebooks and open that new file—adjust
saveProjectAndOpenNotebook and its callers accordingly (symbols:
projectData.project.notebooks, newNotebook, saveProjectAndOpenNotebook,
fileUri).

---

253-271: ⚠️ Potential issue | 🟠 Major

Duplicate reopens the original/default notebook, not the copy.

newNotebook is appended, but the reopen step still targets only fileUri. In a multi-notebook file that resolves back to the default notebook, so the duplicated notebook is not what the user sees after the command completes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 253 - 271, The
code writes the updated file but reopens using fileUri, which opens the default
notebook instead of the duplicated one; update the reopen step to open the
specific notebook entry for newNotebook by constructing a notebook-specific URI
(e.g., derive a fragment/identifier from newNotebook.id or its index in
projectData.project.notebooks) and pass that URI to
workspace.openNotebookDocument and window.showNotebookDocument so the duplicated
notebook (newNotebook) is opened rather than the file's default notebook.

🤖 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/deepnoteServerStarter.node.ts`:
- Around line 446-462: The private method disposeOutputListeners is placed
immediately before the public dispose method, violating the rule to order class
members first by accessibility then alphabetically; move disposeOutputListeners
so that all public members (including dispose) appear before private members, or
otherwise reorder members so that public dispose() comes before private
disposeOutputListeners and ensure disposablesByFile usage remains correct;
update the class member sequence so accessibility groups (public then private)
and alphabetical ordering within each group are respected for dispose() and
disposeOutputListeners.

In `@src/notebooks/deepnote/deepnoteNotebookManager.ts`:
- Around line 35-43: The current logic in the notebook lookup returns the first
entry from notebookMap even when a caller requested a specific notebookId and it
wasn't found, which can return the wrong notebook; change the behavior in the
function handling notebook lookups (the block referencing notebookId and
notebookMap) so that when notebookId is provided and notebookMap.get(notebookId)
returns undefined you return undefined immediately, and only use the
notebookMap.values().next().value fallback when notebookId was not provided
(i.e., the caller intentionally omitted it).

In `@src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts`:
- Around line 35-40: The tests store using
manager.storeOriginalProject('project-123', 'notebook-1', mockProject') but read
back with manager.getOriginalProject('project-123'), which doesn't verify
notebook-aware behavior; update the assertions in the new cache-contract tests
so they call manager.getOriginalProject('project-123', 'notebook-1') to read
with the same notebookId (do the same for the second case at the other test
block), and keep one separate test that asserts the fallback behavior using
getOriginalProject('project-123') only once.

In `@src/notebooks/deepnote/deepnoteTreeItem.ts`:
- Around line 53-58: The ProjectGroup/ProjectFile visual-field formatting logic
is duplicated between the DeepnoteTreeItem constructor and updateVisualFields();
extract shared helpers (e.g., formatProjectGroupVisuals(projectGroupData) and
formatProjectFileVisuals(projectFileData)) that return label, tooltip,
description, and icon info, and call those helpers from both the constructor and
updateVisualFields(); update usages for DeepnoteTreeItemType.ProjectGroup and
DeepnoteTreeItemType.ProjectFile so the rendering rules live in one place and
both code paths consume the same helper functions.

In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 773-779: The code currently calls findProjectUriFromId(projectId)
which can return a sibling file's URI; instead derive the snapshot location from
the current notebook's URI (the `notebook` variable) so snapshots are written
for the correct file. Replace the findProjectUriFromId(projectId) call with
logic that uses `notebook.uri` (or a helper like
findProjectUriFromUri(notebook.uri) if available) as the source of truth for
`projectUri`, keep the same early-return and logger.warn behavior but reference
the notebook URI in the warning.
- Around line 415-418: The notebook-scoped glob construction (latestGlob)
currently omits legacy project-scoped snapshots when notebookId is present;
change the logic in snapshotService.ts so that when notebookId is provided you
first build the notebook-scoped pattern
(`*_{projectId}_{notebookId}_latest.snapshot.deepnote`) and if that yields no
results fall back to the legacy project-scoped pattern
(`*_{projectId}_latest.snapshot.deepnote`); apply the same change to the other
similar block around the code referenced at 443-445 so both the "latest" and the
other snapshot-glob checks try the notebook-scoped pattern first and then
fallback to the legacy project-scoped pattern when no matches are found.

---

Duplicate comments:
In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 77-85: The current flow appends newNotebook to
projectData.project.notebooks then calls saveProjectAndOpenNotebook(fileUri),
which reopens the .deepnote file and usually selects the default notebook
instead of the newly created one; fix by carrying the target notebook through
the reopen path: change saveProjectAndOpenNotebook to accept an
identifier/ordinal for the notebook (e.g., newNotebook.id or the newNotebook
object) and use that to select/open the specific notebook after deserialization,
then update this call site to pass newNotebook (or newNotebook.id) instead of
only fileUri; alternatively, if you prefer per-notebook files, create a new
.deepnote file for newNotebook instead of appending to
projectData.project.notebooks and open that new file—adjust
saveProjectAndOpenNotebook and its callers accordingly (symbols:
projectData.project.notebooks, newNotebook, saveProjectAndOpenNotebook,
fileUri).
- Around line 253-271: The code writes the updated file but reopens using
fileUri, which opens the default notebook instead of the duplicated one; update
the reopen step to open the specific notebook entry for newNotebook by
constructing a notebook-specific URI (e.g., derive a fragment/identifier from
newNotebook.id or its index in projectData.project.notebooks) and pass that URI
to workspace.openNotebookDocument and window.showNotebookDocument so the
duplicated notebook (newNotebook) is opened rather than the file's default
notebook.

In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts`:
- Around line 398-406: The catch for the workspace.save(notebook.uri) call
currently only logs the error (logger.warn) and then execution falls through to
the success log; update the catch to stop further processing by returning (or
otherwise exiting) after logging the saveError so the code does not incorrectly
log a successful save for notebook.uri.path. Ensure you modify the catch block
that wraps workspace.save(notebook.uri) (the logger.warn call that references
notebook.uri.path and saveError) to return immediately after logging.

🪄 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: 95692a9c-a6cd-4c59-91ec-1fc63e37e257

📥 Commits

Reviewing files that changed from the base of the PR and between 1140fa0 and 966bf96.

📒 Files selected for processing (13)

• src/kernels/deepnote/deepnoteServerStarter.node.ts
• src/notebooks/deepnote/deepnoteExplorerView.ts
• src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts
• src/notebooks/deepnote/deepnoteFileChangeWatcher.ts
• src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts
• src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
• src/notebooks/deepnote/deepnoteNotebookManager.ts
• src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts
• src/notebooks/deepnote/deepnoteSerializer.ts
• src/notebooks/deepnote/deepnoteSerializer.unit.test.ts
• src/notebooks/deepnote/deepnoteTreeItem.ts
• src/notebooks/deepnote/snapshots/snapshotService.ts
• src/notebooks/types.ts

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Reorder members to match required class ordering.

private disposeOutputListeners is introduced immediately before public dispose(), which breaks the required accessibility/alphabetical ordering in this section. Please reorder this block to align with the class member ordering rule. 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/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 446 - 462,
The private method disposeOutputListeners is placed immediately before the
public dispose method, violating the rule to order class members first by
accessibility then alphabetically; move disposeOutputListeners so that all
public members (including dispose) appear before private members, or otherwise
reorder members so that public dispose() comes before private
disposeOutputListeners and ensure disposablesByFile usage remains correct;
update the class member sequence so accessibility groups (public then private)
and alphabetical ordering within each group are respected for dispose() and
disposeOutputListeners.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don’t hide an exact notebook-cache miss behind an arbitrary project entry.

After the split-file migration, cached entries under one projectId are no longer interchangeable: each can carry a different project.notebooks subset. If a caller asked for a specific notebookId and it is missing, returning the first cached value can hand back a sibling notebook’s file and make later save/restore logic act on the wrong project. Return undefined on an exact miss; keep the fallback only for calls that omitted notebookId on purpose.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteNotebookManager.ts` around lines 35 - 43, The
current logic in the notebook lookup returns the first entry from notebookMap
even when a caller requested a specific notebookId and it wasn't found, which
can return the wrong notebook; change the behavior in the function handling
notebook lookups (the block referencing notebookId and notebookMap) so that when
notebookId is provided and notebookMap.get(notebookId) returns undefined you
return undefined immediately, and only use the notebookMap.values().next().value
fallback when notebookId was not provided (i.e., the caller intentionally
omitted it).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Read back with the notebook ID in these new cache-contract tests.

These cases store under 'notebook-1' and then assert via getOriginalProject('project-123'). That still passes if the manager ignores notebookId, which is the main behavior this PR is introducing. Keep one explicit fallback test, but make the new contract tests read back with the same notebook ID.

♻️ Minimal adjustment

-const result = manager.getOriginalProject('project-123');
+const result = manager.getOriginalProject('project-123', 'notebook-1');

Also applies to: 45-50

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts` around lines 35
- 40, The tests store using manager.storeOriginalProject('project-123',
'notebook-1', mockProject') but read back with
manager.getOriginalProject('project-123'), which doesn't verify notebook-aware
behavior; update the assertions in the new cache-contract tests so they call
manager.getOriginalProject('project-123', 'notebook-1') to read with the same
notebookId (do the same for the second case at the other test block), and keep
one separate test that asserts the fallback behavior using
getOriginalProject('project-123') only once.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Notebook-scoped reads dropped the legacy snapshot fallback.

When notebookId is present, these globs only search *_${projectId}_${notebookId}_.... Older *_${projectId}_... snapshots will never be found, so pre-migration outputs disappear until a new notebook-scoped snapshot is written. Please try the notebook-scoped pattern first and then fall back to the legacy project-scoped one.

Also applies to: 443-445

🤖 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 415 - 418,
The notebook-scoped glob construction (latestGlob) currently omits legacy
project-scoped snapshots when notebookId is present; change the logic in
snapshotService.ts so that when notebookId is provided you first build the
notebook-scoped pattern (`*_{projectId}_{notebookId}_latest.snapshot.deepnote`)
and if that yields no results fall back to the legacy project-scoped pattern
(`*_{projectId}_latest.snapshot.deepnote`); apply the same change to the other
similar block around the code referenced at 443-445 so both the "latest" and the
other snapshot-glob checks try the notebook-scoped pattern first and then
fallback to the legacy project-scoped pattern when no matches are found.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Resolve the snapshot location from the current notebook, not just projectId.

After the split-file migration, several open .deepnote files can share one projectId. findProjectUriFromId(projectId) can therefore pick a sibling file, which writes this notebook’s snapshots into the wrong directory. You already have notebook here, so use its URI as the source of truth.

🤖 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 773 - 779,
The code currently calls findProjectUriFromId(projectId) which can return a
sibling file's URI; instead derive the snapshot location from the current
notebook's URI (the `notebook` variable) so snapshots are written for the
correct file. Replace the findProjectUriFromId(projectId) call with logic that
uses `notebook.uri` (or a helper like findProjectUriFromUri(notebook.uri) if
available) as the source of truth for `projectUri`, keep the same early-return
and logger.warn behavior but reference the notebook URI in the warning.

[coderabbitai]

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.unit.test.ts (1)

787-799: ⚠️ Potential issue | 🟠 Major

This test never isolates noSnapshotWatcher.

noSnapshotWatcher is created here, but the fired event (snapshotOnDidChange) and the asserted counters still belong to the already-active snapshotWatcher. That means this can stay green without proving the new watcher ignores snapshot events. Give noSnapshotWatcher its own FileSystemWatcher mock/event emitters, then fire those and assert no read/apply happens.

🤖 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
787 - 799, The test incorrectly reuses the existing snapshot event stream so
noSnapshotWatcher isn't isolated; create a separate mocked FileSystemWatcher and
event emitter for the new DeepnoteFileChangeWatcher instance (noSnapshotWatcher)
instead of reusing snapshotOnDidChange or snapshotWatcher, wire that mock into
noSnapshotWatcher during construction/activation, fire the new watcher’s change
event (the new event emitter) and then await debounceWaitMs, and finally assert
readSnapshotCallCount and any applySnapshot counts remain unchanged to prove the
new watcher ignored snapshot events; ensure workspace.notebookDocuments is still
mocked to [] when firing the new event.

🤖 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 295-296: Replace scattered per-test numeric timeouts (e.g., 8000,
10000, 12000, 15000, 20000) with named constants declared near the top of the
module and reuse them in each test's this.timeout(...) call; specifically add
constants like TEST_TIMEOUT_SHORT, TEST_TIMEOUT_MEDIUM, TEST_TIMEOUT_LONG (or
similar) and update the tests referenced by their unique names such as the
"saving a non-deepnote notebook should not suppress FS change events" test and
the other tests noted in the comment so that each this.timeout(...) uses the
appropriate named constant instead of a literal; keep constants alongside the
existing timing constants block for easy CI tuning.
- Around line 1618-1630: The test is falsely passing because a suite-level
one-arg stub for mockSnapshotService.readSnapshot still exists; call
reset(mockSnapshotService) (not resetCalls) before re-stubbing so previous stubs
are removed, then stub readSnapshot explicitly with two args via
when(mockSnapshotService.readSnapshot(anything(), anything())).thenCall(...) and
increment/read readSnapshotCallCount as done, and finally verify the exact call
happened with verify(mockSnapshotService.readSnapshot('project-1',
scopedNotebookId)).once(); apply the same reset+explicit two-arg stubbing and
exact-args verification for the other occurrence around the 1655–1660 region.

---

Outside diff comments:
In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts`:
- Around line 787-799: The test incorrectly reuses the existing snapshot event
stream so noSnapshotWatcher isn't isolated; create a separate mocked
FileSystemWatcher and event emitter for the new DeepnoteFileChangeWatcher
instance (noSnapshotWatcher) instead of reusing snapshotOnDidChange or
snapshotWatcher, wire that mock into noSnapshotWatcher during
construction/activation, fire the new watcher’s change event (the new event
emitter) and then await debounceWaitMs, and finally assert readSnapshotCallCount
and any applySnapshot counts remain unchanged to prove the new watcher ignored
snapshot events; ensure workspace.notebookDocuments is still mocked to [] when
firing the new event.

🪄 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: 162ff883-0820-4b85-ba7f-22126c875efd

📥 Commits

Reviewing files that changed from the base of the PR and between 966bf96 and 367db44.

📒 Files selected for processing (1)

• src/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.ts

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Extract the per-test timeout literals into named constants.

The new 8_000/10_000/12_000/15_000/20_000 budgets are now scattered through the suite, which makes CI tuning harder than it needs to be. Hoist them next to the other timing constants and reuse them.

As per coding guidelines, "Extract magic numbers (retry counts, delays, timeouts) as named constants near the top of the module".

Also applies to: 319-320, 510-512, 566-567, 1898-1899, 1972-1973, 2041-2042, 2543-2544

🤖 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
295 - 296, Replace scattered per-test numeric timeouts (e.g., 8000, 10000,
12000, 15000, 20000) with named constants declared near the top of the module
and reuse them in each test's this.timeout(...) call; specifically add constants
like TEST_TIMEOUT_SHORT, TEST_TIMEOUT_MEDIUM, TEST_TIMEOUT_LONG (or similar) and
update the tests referenced by their unique names such as the "saving a
non-deepnote notebook should not suppress FS change events" test and the other
tests noted in the comment so that each this.timeout(...) uses the appropriate
named constant instead of a literal; keep constants alongside the existing
timing constants block for easy CI tuning.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

This regression check is masked by the leftover 1-arg readSnapshot stub.

resetCalls(mockSnapshotService) clears invocations, not stubs. The suite-level readSnapshot(anything()) setup is still active, so a regression back to readSnapshot('project-1') can still return snapshotOutputs and satisfy this test because the notebook also uses block-1. Reset the mock here and assert the exact (projectId, notebookId) pair that was read.

Patch sketch

-            resetCalls(mockSnapshotService);
+            reset(mockSnapshotService);
             when(mockSnapshotService.isSnapshotsEnabled()).thenReturn(true);
+            const readArgs: Array<{ projectId: string; notebookId?: string }> = [];
             when(mockSnapshotService.readSnapshot(anything(), anything())).thenCall((pid: string, nid?: string) => {
                 readSnapshotCallCount++;
+                readArgs.push({ projectId: pid, notebookId: nid });
                 if (typeof nid === 'undefined') {
                     return Promise.resolve(undefined);
                 }
                 if (pid === 'project-1' && nid === scopedNotebookId) {
                     return Promise.resolve(snapshotOutputs);
                 }

                 return Promise.resolve(undefined);
             });
…
-            assert.isAtLeast(readSnapshotCallCount, 1, 'readSnapshot should be called');
+            assert.deepStrictEqual(readArgs, [{ projectId: 'project-1', notebookId: scopedNotebookId }]);

Also applies to: 1655-1660

🤖 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
1618 - 1630, The test is falsely passing because a suite-level one-arg stub for
mockSnapshotService.readSnapshot still exists; call reset(mockSnapshotService)
(not resetCalls) before re-stubbing so previous stubs are removed, then stub
readSnapshot explicitly with two args via
when(mockSnapshotService.readSnapshot(anything(), anything())).thenCall(...) and
increment/read readSnapshotCallCount as done, and finally verify the exact call
happened with verify(mockSnapshotService.readSnapshot('project-1',
scopedNotebookId)).once(); apply the same reset+explicit two-arg stubbing and
exact-args verification for the other occurrence around the 1655–1660 region.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (4)

src/notebooks/deepnote/deepnoteExplorerView.ts (2)

288-289: ⚠️ Potential issue | 🟠 Major

Don't reopen the base file after duplication.

This writes the copy into the source .deepnote and then reopens fileUri again, so the UI lands back on the source file instead of the duplicated notebook. In the single-notebook flow, duplication should open the duplicated notebook explicitly, or better, create/open a sibling notebook file the same way createAndAddNotebookToProject() does.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 288 - 289, The
code currently reopens the original fileUri after duplication which returns the
UI to the source instead of the new copy; change the logic to open the
duplicated notebook's URI (not fileUri) — locate where
workspace.openNotebookDocument(fileUri) is called and replace it to open the new
duplicatedFileUri (or use the same sibling-creation flow as
createAndAddNotebookToProject()), ensuring the duplicated notebook is
created/added as a sibling and then passed to workspace.openNotebookDocument and
shown in the editor so the UI lands on the new copy.

---

559-560: ⚠️ Potential issue | 🟠 Major

Pass the notebook target through this open path.

context.notebookId is ignored here, so notebook nodes under the same file become indistinguishable and all reopen the same document.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 559 - 560, The
code opens the file URI without including the notebook identifier so different
notebook nodes for the same file collide; include context.notebookId when
building the URI passed to workspace.openNotebookDocument (e.g. use
Uri.file(context.filePath).with({ query: `notebookId=${context.notebookId}` })
or otherwise encode context.notebookId into the Uri) and then call
workspace.openNotebookDocument with that enriched URI so the correct notebook
target (context.notebookId) is preserved; update the usage around Uri.file and
workspace.openNotebookDocument in deepnoteExplorerView.ts to reference
context.notebookId.

src/notebooks/deepnote/deepnoteAutoSplitter.ts (2)

51-63: ⚠️ Potential issue | 🔴 Critical

Make the split loop collision-safe and failure-safe.

Slug-only filenames still collide for duplicate names or the 'notebook' fallback, and one failed writeFile aborts the loop after earlier siblings were already written. That can overwrite notebooks or leave the project half-split on disk.

Proposed direction

-        for (const notebook of extraNotebooks) {
-            const notebookSlug = slugifyNotebookNameOrFallback(notebook.name);
-            const newFileName = `${originalStem}_${notebookSlug}.deepnote`;
-            const newFileUri = Uri.joinPath(parentDir, newFileName);
-
-            const newProject = await buildSingleNotebookFile(deepnoteFile, notebook);
-
-            const yaml = serializeDeepnoteFile(newProject);
-            await workspace.fs.writeFile(newFileUri, new TextEncoder().encode(yaml));
-
-            newFiles.push(newFileUri);
-
-            logger.info(`[AutoSplitter] Created ${newFileName} for notebook "${notebook.name}"`);
+        for (const notebook of extraNotebooks) {
+            try {
+                const newFileUri = await buildSiblingNotebookFileUri(fileUri, notebook.name, async (candidate) => {
+                    try {
+                        await workspace.fs.stat(candidate);
+                        return true;
+                    } catch {
+                        return false;
+                    }
+                });
+                const newFileName = newFileUri.path.split('/').pop() ?? newFileUri.path;
+                const newProject = await buildSingleNotebookFile(deepnoteFile, notebook);
+                const yaml = serializeDeepnoteFile(newProject);
+
+                await workspace.fs.writeFile(newFileUri, new TextEncoder().encode(yaml));
+                newFiles.push(newFileUri);
+                logger.info(`[AutoSplitter] Created ${newFileName} for notebook "${notebook.name}"`);
+            } catch (error) {
+                logger.warn(`[AutoSplitter] Failed to split notebook "${notebook.name}"`, error);
+            }
         }

As per coding guidelines, Extract duplicate logic into helper methods to prevent drift following DRY principle and 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/deepnoteAutoSplitter.ts` around lines 51 - 63, The
split loop must be made collision-safe and failure-safe: extract the
filename-generation logic used around slugifyNotebookNameOrFallback into a
helper (e.g., generateUniqueFileUri(parentDir, originalStem, notebook.name))
that checks for existing URIs and appends a unique suffix/counter if needed;
wrap each iteration in a try/catch so a failing workspace.fs.writeFile for one
notebook doesn't abort the loop and still records/logs the error via
logger.error; make writes atomic by writing to a temp URI (e.g., newFileUri +
".tmp") and then renaming/moving to newFileUri after successful write; ensure
newFiles is only pushed after the final successful rename and use
buildSingleNotebookFile/serializeDeepnoteFile as before.

---

143-146: ⚠️ Potential issue | 🟠 Major

Skip snapshots that already use notebook-scoped names.

snapshotGlob matches both legacy and new snapshot filenames, but extractVariantFromSnapshotFilename() only parses the legacy form. For an already-migrated file, this path treats <notebookId>_<variant> as the variant, writes a doubly suffixed filename, and then deletes the valid source snapshot.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteAutoSplitter.ts` around lines 143 - 146, The
loop over allSnapshotFiles is treating already-migrated snapshot filenames as
legacy and re-suffixed; update the logic in the iteration (or inside
splitSingleSnapshot) to detect and skip snapshot files that already use the
notebook-scoped naming scheme before attempting to parse or rewrite them.
Specifically, add a guard that recognizes the new filename form (the
notebook-scoped pattern your migration uses) — e.g., check the snapshot filename
against the notebook-scoped regex or detect that
extractVariantFromSnapshotFilename would mis-interpret a notebookId_prefix — and
return/continue early so splitSingleSnapshot is not called and the original
snapshot is not deleted. Ensure the check references the same helper(s)
(extractVariantFromSnapshotFilename, splitSingleSnapshot) and preserves behavior
for legacy filenames.

🤖 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/deepnoteAutoSplitter.ts`:
- Around line 70-75: The metadata.modifiedAt field on deepnoteFile should be
refreshed before rewriting the source file: in the block that sets
(deepnoteFile.metadata as Record<string, unknown>).snapshotHash = await
computeSnapshotHash(deepnoteFile), also update (deepnoteFile.metadata as
Record<string, unknown>).modifiedAt to the current timestamp (e.g. new
Date().toISOString() or Date.now()) so serializeDeepnoteFile(deepnoteFile) and
the subsequent workspace.fs.writeFile(fileUri, new
TextEncoder().encode(updatedYaml)) persist a fresh modification time consistent
with newly created sibling files.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 547-549: The public method refreshTree() is currently located
after private helpers; move the refreshTree() declaration into the class's
public member block so it sits with other public methods and is alphabetized
among them (ensure its signature public refreshTree(): void {
this.treeDataProvider.refresh(); } is unchanged and references
treeDataProvider.refresh()).

---

Duplicate comments:
In `@src/notebooks/deepnote/deepnoteAutoSplitter.ts`:
- Around line 51-63: The split loop must be made collision-safe and
failure-safe: extract the filename-generation logic used around
slugifyNotebookNameOrFallback into a helper (e.g.,
generateUniqueFileUri(parentDir, originalStem, notebook.name)) that checks for
existing URIs and appends a unique suffix/counter if needed; wrap each iteration
in a try/catch so a failing workspace.fs.writeFile for one notebook doesn't
abort the loop and still records/logs the error via logger.error; make writes
atomic by writing to a temp URI (e.g., newFileUri + ".tmp") and then
renaming/moving to newFileUri after successful write; ensure newFiles is only
pushed after the final successful rename and use
buildSingleNotebookFile/serializeDeepnoteFile as before.
- Around line 143-146: The loop over allSnapshotFiles is treating
already-migrated snapshot filenames as legacy and re-suffixed; update the logic
in the iteration (or inside splitSingleSnapshot) to detect and skip snapshot
files that already use the notebook-scoped naming scheme before attempting to
parse or rewrite them. Specifically, add a guard that recognizes the new
filename form (the notebook-scoped pattern your migration uses) — e.g., check
the snapshot filename against the notebook-scoped regex or detect that
extractVariantFromSnapshotFilename would mis-interpret a notebookId_prefix — and
return/continue early so splitSingleSnapshot is not called and the original
snapshot is not deleted. Ensure the check references the same helper(s)
(extractVariantFromSnapshotFilename, splitSingleSnapshot) and preserves behavior
for legacy filenames.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 288-289: The code currently reopens the original fileUri after
duplication which returns the UI to the source instead of the new copy; change
the logic to open the duplicated notebook's URI (not fileUri) — locate where
workspace.openNotebookDocument(fileUri) is called and replace it to open the new
duplicatedFileUri (or use the same sibling-creation flow as
createAndAddNotebookToProject()), ensuring the duplicated notebook is
created/added as a sibling and then passed to workspace.openNotebookDocument and
shown in the editor so the UI lands on the new copy.
- Around line 559-560: The code opens the file URI without including the
notebook identifier so different notebook nodes for the same file collide;
include context.notebookId when building the URI passed to
workspace.openNotebookDocument (e.g. use Uri.file(context.filePath).with({
query: `notebookId=${context.notebookId}` }) or otherwise encode
context.notebookId into the Uri) and then call workspace.openNotebookDocument
with that enriched URI so the correct notebook target (context.notebookId) is
preserved; update the usage around Uri.file and workspace.openNotebookDocument
in deepnoteExplorerView.ts to reference context.notebookId.

🪄 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: 018b4118-b36b-4f59-8ea2-55f48ebf9c0c

📥 Commits

Reviewing files that changed from the base of the PR and between 367db44 and b73d313.

📒 Files selected for processing (5)

• src/notebooks/deepnote/deepnoteAutoSplitter.ts
• src/notebooks/deepnote/deepnoteExplorerView.ts
• src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts
• src/notebooks/deepnote/deepnoteNotebookFileFactory.ts
• src/notebooks/deepnote/deepnoteNotebookFileFactory.unit.test.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Refresh metadata.modifiedAt before rewriting the source file.

The splitter mutates and overwrites the original project here, but only snapshotHash changes. That leaves the migrated source file with stale metadata while the newly created sibling files get a fresh timestamp.

Proposed fix

-        if (deepnoteFile.metadata) {
-            (deepnoteFile.metadata as Record<string, unknown>).snapshotHash = await computeSnapshotHash(deepnoteFile);
-        }
+        if (!deepnoteFile.metadata) {
+            deepnoteFile.metadata = { createdAt: new Date().toISOString() };
+        }
+        deepnoteFile.metadata.modifiedAt = new Date().toISOString();
+        (deepnoteFile.metadata as Record<string, unknown>).snapshotHash = await computeSnapshotHash(deepnoteFile);

📝 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

	if (deepnoteFile.metadata) {
	(deepnoteFile.metadata as Record<string, unknown>).snapshotHash = await computeSnapshotHash(deepnoteFile);
	}
	const updatedYaml = serializeDeepnoteFile(deepnoteFile);
	await workspace.fs.writeFile(fileUri, new TextEncoder().encode(updatedYaml));
	if (!deepnoteFile.metadata) {
	deepnoteFile.metadata = { createdAt: new Date().toISOString() };
	}
	deepnoteFile.metadata.modifiedAt = new Date().toISOString();
	(deepnoteFile.metadata as Record<string, unknown>).snapshotHash = await computeSnapshotHash(deepnoteFile);
	const updatedYaml = serializeDeepnoteFile(deepnoteFile);
	await workspace.fs.writeFile(fileUri, new TextEncoder().encode(updatedYaml));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteAutoSplitter.ts` around lines 70 - 75, The
metadata.modifiedAt field on deepnoteFile should be refreshed before rewriting
the source file: in the block that sets (deepnoteFile.metadata as Record<string,
unknown>).snapshotHash = await computeSnapshotHash(deepnoteFile), also update
(deepnoteFile.metadata as Record<string, unknown>).modifiedAt to the current
timestamp (e.g. new Date().toISOString() or Date.now()) so
serializeDeepnoteFile(deepnoteFile) and the subsequent
workspace.fs.writeFile(fileUri, new TextEncoder().encode(updatedYaml)) persist a
fresh modification time consistent with newly created sibling files.

[coderabbitai]

Actionable comments posted: 17

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)

src/notebooks/deepnote/deepnoteTreeDataProvider.ts (2)

270-287: ⚠️ Potential issue | 🟠 Major

Filter out the init notebook before creating child rows.

Everywhere else in this flow treats initNotebookId as hidden metadata, but getNotebooksForProject() maps all notebooks. A multi-notebook file that carries an init notebook will surface that internal notebook in the explorer.

Suggested fix

     private getNotebooksForProject(projectItem: DeepnoteTreeItem): DeepnoteTreeItem[] {
         const project = projectItem.data as DeepnoteProject;
-        const notebooks = project.project.notebooks || [];
+        const initNotebookId = project.project.initNotebookId;
+        const notebooks = (project.project.notebooks || []).filter((notebook) => notebook.id !== initNotebookId);
 
         return notebooks.map((notebook: DeepnoteNotebook) => {
             const context: DeepnoteTreeItemContext = {
                 filePath: projectItem.context.filePath,
                 projectId: projectItem.context.projectId,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteTreeDataProvider.ts` around lines 270 - 287,
getNotebooksForProject is currently mapping all notebooks, which surfaces the
internal init notebook; filter out the project's init notebook (compare
notebook.id to project.project.initNotebookId) before creating DeepnoteTreeItem
instances. Update getNotebooksForProject to read const initId =
project.project.initNotebookId and use notebooks.filter(n => n.id !== initId)
(or equivalent) prior to mapping to DeepnoteTreeItem so the init notebook is
omitted from the explorer.

---

165-179: ⚠️ Potential issue | 🟠 Major

Wrap each workspace-folder scan in its own try/catch.

workspace.findFiles(pattern) is outside the per-file guard. If one folder scan rejects, the whole load stops and later workspace folders are skipped.

Suggested fix

         for (const workspaceFolder of workspace.workspaceFolders || []) {
-            const pattern = new RelativePattern(workspaceFolder, '**/*.deepnote');
-            const files = await workspace.findFiles(pattern);
-            const projectFiles = files.filter((file) => !file.path.endsWith(SNAPSHOT_FILE_SUFFIX));
-
-            for (const file of projectFiles) {
-                try {
-                    const project = await this.loadDeepnoteProject(file);
-
-                    if (project) {
-                        results.push({ filePath: file.path, project });
-                    }
-                } catch (error) {
-                    this.logger?.error(`Failed to load Deepnote project from ${file.path}`, error);
-                }
-            }
+            try {
+                const pattern = new RelativePattern(workspaceFolder, '**/*.deepnote');
+                const files = await workspace.findFiles(pattern);
+                const projectFiles = files.filter((file) => !file.path.endsWith(SNAPSHOT_FILE_SUFFIX));
+
+                for (const file of projectFiles) {
+                    try {
+                        const project = await this.loadDeepnoteProject(file);
+
+                        if (project) {
+                            results.push({ filePath: file.path, project });
+                        }
+                    } catch (error) {
+                        this.logger?.error(`Failed to load Deepnote project from ${file.path}`, error);
+                    }
+                }
+            } catch (error) {
+                this.logger?.error(`Failed to scan workspace folder ${workspaceFolder.uri.path}`, error);
+            }
         }

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/deepnoteTreeDataProvider.ts` around lines 165 - 179,
The workspace-level file scan (workspace.findFiles with new RelativePattern) can
throw and currently lives outside the per-file try/catch, so a failing folder
aborts scanning subsequent workspaceFolders; wrap each workspaceFolder iteration
in its own try/catch: call workspace.findFiles(pattern) and the subsequent
projectFiles loop inside that try block, call this.logger?.error(...) in the
catch (including the workspaceFolder context) and continue so one folder failure
doesn’t stop processing other folders; keep existing per-file try/catch around
this.loadDeepnoteProject(file) as-is.

src/notebooks/deepnote/deepnoteExplorerView.ts (1)

778-788: ⚠️ Potential issue | 🟡 Minor

executionCount: 0 vs undefined inconsistency.

New project block uses executionCount: 0 but createNotebookWithFirstBlock at line 591 uses undefined. Pick one convention.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 778 - 788, The
firstBlock object in deepnoteExplorerView.ts sets executionCount: 0 while
createNotebookWithFirstBlock uses undefined, causing inconsistent conventions;
pick one and make both match—either set executionCount to undefined in the
firstBlock definition (DeepnoteBlock firstBlock) or change
createNotebookWithFirstBlock to use 0—update every occurrence so the
DeepnoteBlock instances (firstBlock and createNotebookWithFirstBlock) use the
same executionCount convention and ensure any consumers expect that type (number
or undefined).

src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (1)

491-497: ⚠️ Potential issue | 🟠 Major

Rollback the project mapping if the controller rebuild fails.

setEnvironmentForProject() runs before rebuildController(). If rebuild throws, the project stays pointed at the new environment even though the switch never completed, so sibling notebooks will also pick up the broken mapping.

Possible rollback pattern

+                    const previousEnvironmentId = currentEnvironmentId;
                     // Update the project-to-environment mapping
                     await this.projectEnvironmentMapper.setEnvironmentForProject(projectId, selectedEnvironmentId);

                     // Force rebuild the controller with the new environment
-                    await this.kernelAutoSelector.rebuildController(notebook, progress, token);
+                    try {
+                        await this.kernelAutoSelector.rebuildController(notebook, progress, token);
+                    } catch (error) {
+                        if (previousEnvironmentId) {
+                            await this.projectEnvironmentMapper.setEnvironmentForProject(projectId, previousEnvironmentId);
+                        } else {
+                            await this.projectEnvironmentMapper.removeEnvironmentForProject(projectId);
+                        }
+                        throw error;
+                    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts` around
lines 491 - 497, The code sets the project mapping via
projectEnvironmentMapper.setEnvironmentForProject(projectId,
selectedEnvironmentId) before calling
kernelAutoSelector.rebuildController(notebook, progress, token), so if
rebuildController throws the project remains pointed at the new environment;
change this by capturing the current environment (call
projectEnvironmentMapper.getEnvironmentForProject(projectId) or equivalent)
before updating, then perform rebuildController in a try block and only commit
the new mapping after successful rebuild OR perform the update first but wrap
rebuildController in try/catch and on any error call
projectEnvironmentMapper.setEnvironmentForProject(projectId,
previousEnvironmentId) to rollback; reference
projectEnvironmentMapper.setEnvironmentForProject,
projectEnvironmentMapper.getEnvironmentForProject, and
kernelAutoSelector.rebuildController when implementing the rollback.

src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)

691-712: ⚠️ Potential issue | 🟠 Major

Resolver fallback can orphan the pending init-notebook entry.

This queue is keyed by the projectId resolved earlier via resolveProjectIdForNotebook(), but onKernelStarted() still reads only notebook.metadata.deepnoteProjectId. If metadata is missing and the resolver fell back to parsing the file, the pending entry is never consumed and the init notebook never runs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts` around lines 691 -
712, The pending-init entry is keyed by the resolved projectId but
onKernelStarted() reads notebook.metadata.deepnoteProjectId, so entries can be
orphaned when resolveProjectIdForNotebook() falls back to parsing; update
onKernelStarted() to call resolveProjectIdForNotebook(notebook) (same resolver
used earlier) and use that resolved projectId to look up and consume
projectsPendingInitNotebook, or alternatively store the pending entry under both
keys, ensuring consistency between the code paths (functions referenced:
resolveProjectIdForNotebook, onKernelStarted, projectsPendingInitNotebook,
notebook.metadata.deepnoteProjectId).

♻️ Duplicate comments (2)

src/notebooks/deepnote/snapshots/snapshotService.ts (2)

991-995: ⚠️ Potential issue | 🟠 Major

Resolve the snapshot directory from the current notebook URI.

Several split .deepnote files can now share one projectId. findProjectUriFromId(projectId) can pick a sibling file and write this notebook's snapshot into the wrong folder. Use notebook.uri as the source of truth here.

🤖 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 991 - 995,
The code currently uses findProjectUriFromId(projectId) to resolve projectUri
which can pick a sibling .deepnote file; instead derive the snapshot directory
from the current notebook URI (notebook.uri) so snapshots are written into the
correct folder for the active notebook. Replace or guard uses of projectUri
resolved via findProjectUriFromId in snapshotService.ts (references:
findProjectUriFromId(projectId), projectUri) and construct the snapshot path
from notebook.uri (or fallback to findProjectUriFromId only if notebook.uri
cannot be parsed), ensuring all snapshot write operations use the
notebook-derived directory.

---

467-470: ⚠️ Potential issue | 🟠 Major

Keep the legacy snapshot fallback.

When notebookId is present, these globs only search notebook-scoped files. Older project-scoped snapshots then become invisible until a new notebook-scoped save is written, so pre-migration outputs disappear. Try the notebook-scoped pattern first, then fall back to *_${projectId}_... when nothing matches.

Also applies to: 515-517

🤖 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 467 - 470,
The current glob building for latest snapshots only uses notebook-scoped
patterns when notebookId is present, which hides legacy project-scoped
snapshots; change the lookup to first attempt the notebook-scoped glob (the
existing pattern assigned to latestGlob) and if that returns no matches then run
a fallback glob using the project-scoped pattern
`**/snapshots/*_${projectId}_...` (e.g.,
`**/snapshots/*_${projectId}_${notebookId}_latest.snapshot.deepnote` → fallback
`**/snapshots/*_${projectId}_latest.snapshot.deepnote`), and apply the same
two-step try/fallback logic for the similar block at the other occurrence (the
code around the other latest/project glob at lines 515-517) so legacy
project-scoped snapshots remain visible until a notebook-scoped save exists.

🤖 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/deepnoteServerStarter.node.ts`:
- Around line 100-107: The reuse logic in startServer/startServerForEnvironment
lets a project-level reused server retain file-specific context
(workingDirectory and SQL env vars) from the original deepnoteFileUri, so update
the reuse validation to either (a) when attempting to reuse an existing server
for the same project, compare the file-derived context (workingDirectory and any
SQL-related envs) computed from the incoming deepnoteFileUri against the
existing server's stored file-derived context and refuse reuse (force new
server) if they differ, or (b) alternatively derive a project-stable
workingDirectory/env (based on projectId) when marking servers project-scoped;
implement this check/derivation inside startServerForEnvironment and ensure
startServer passes through the deepnoteFileUri-derived context so the comparison
can be made before reusing an existing server instance.

In `@src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts`:
- Line 27: The environmentServers Map field is never populated, so deletion code
that expects environmentServers.get(envId) returns the empty fallback; populate
and maintain this map where environment servers are created/started (e.g., in
the method that launches servers — look for functions like createEnvironment,
startEnvironment, or whatever starts the server instances) by calling
environmentServers.set(environmentId, serverIdArray) and update the array if
servers are added/removed; then in the environment deletion/cleanup path (the
loop that reads environmentServers.get(environmentId) and stops servers) iterate
the stored serverIdArray, stop each server (call the existing stop/cleanup
method), and finally call environmentServers.delete(environmentId) so the map
reflects the current state.

In `@src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts`:
- Around line 304-310: The current flow calls
projectEnvironmentMapper.getProjectsUsingEnvironment(environmentId) and then
removes mappings via
projectEnvironmentMapper.removeEnvironmentForProject(projectId) before the
environment delete completes; move this mapping cleanup to after a confirmed
successful delete (or alternatively capture the list of projectIds and restore
them if delete fails), and wrap each removeEnvironmentForProject call in its own
try/catch so a single mapper error doesn't abort the loop; keep the initial
waitForInitialization() but ensure mapping removals happen only after delete
success (or use captured projectIds + restore-on-error logic) and reference
projectEnvironmentMapper, waitForInitialization, getProjectsUsingEnvironment,
removeEnvironmentForProject, and environmentId to locate the changes.

In `@src/kernels/deepnote/environments/deepnoteProjectEnvironmentMapper.node.ts`:
- Around line 20-28: Reorder the class members so they are grouped by
accessibility (public then private) and alphabetized within each group: move the
public event properties onDidRemoveEnvironment and onDidSetEnvironment together
first (alphabetical), then place the private fields _onDidRemoveEnvironment,
_onDidSetEnvironment, initializationPromise, mappings, and workspaceState
together and alphabetize them (e.g., _onDidRemoveEnvironment,
_onDidSetEnvironment, initializationPromise, mappings, workspaceState) so the
class follows the repo rule.
- Around line 103-110: Replace the misspelled word "unparseable" with
"unparsable" in the JSDoc comment block describing the migration in
deepnoteProjectEnvironmentMapper.node.ts (the comment that starts "Migrate
entries from the legacy `deepnote.notebookEnvironmentMappings`...");
alternatively, if you prefer to keep the original spelling, add "unparseable" to
the cspell dictionary, but default to changing the comment text to "unparsable"
to fix the CI failure.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 92-100: Extract the duplicate exists callback into a reusable
helper method (e.g., private async fileExists(uri: Uri): Promise<boolean>) that
wraps workspace.fs.stat and returns true/false, then replace the inline async
callbacks passed to buildSiblingNotebookFileUri (and the duplicate at the other
occurrence around the second pattern) with calls to fileExists; reference the
helper from deepnoteExplorerView methods that currently use the inline try/catch
around workspace.fs.stat to keep behavior identical.
- Around line 647-649: The public method refreshTree() is located after private
helpers; move its declaration (the public refreshTree() {
this.treeDataProvider.refresh(); } method) into the class's public methods block
so all public APIs appear before private helpers, preserving its signature and
behavior and ensuring it sits with other public methods in
deepnoteExplorerView.ts.
- Around line 43-46: Reorder the public methods in the DeepnoteExplorerView
class so they follow alphabetical order: move activate() to come before
dispose(), and apply the same alphabetical ordering to the other public methods
referenced around lines 48-58 (e.g., any reveal*/show* methods) so that all
public method names are sorted lexicographically; update the class method order
accordingly without changing implementations.
- Around line 614-645: collectNotebookNamesForProject currently scans/parses all
.deepnote files in the entire workspace which can be slow; update it to either
(A) add a scoped search parameter (e.g., accept an optional
workspaceFolder/RelativePattern arg and use that instead of iterating all
workspaceFolders and calling workspace.findFiles) for callers that know a
narrower scope, or (B) implement a simple in-memory cache keyed by projectId
(e.g., a Map<string, Set<string>>) and invalidate it via a file watcher or
on-write events so subsequent calls use the cached Set instead of repeatedly
invoking readDeepnoteProjectFile for every file; keep SNAPSHOT_FILE_SUFFIX
filtering and preserve error handling around readDeepnoteProjectFile and
workspace.findFiles.
- Line 656: Replace the direct console.log call that prints `Opening notebook in
project: ${context.projectId}.` with the injected logger on the
DeepnoteExplorerView class (use this.logger.info or this.logger.debug as
appropriate) so diagnostics are consistent; ensure you remove the console.log
usage, call this.logger with the same message and context.projectId, and confirm
the method where this occurs still has access to `this` (bind/arrow function
changes if necessary).

In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 423-427: Old server handles are only removed from the local Map
(projectServerHandles.delete(projectId)) leaving the previously-registered
handle still registered in serverProvider; call the serverProvider
unregister/dispose method for the old handle (e.g.,
serverProvider.unregister(oldServerHandle) or
serverProvider.disposeServerHandle(oldServerHandle) depending on your API)
before deleting it from projectServerHandles, and mirror that same
unregister-before-replace logic where you register a new handle for the
environmentId+projectId (the code around the register call currently at lines
~594-595) so no dead server entries remain.

In `@src/notebooks/deepnote/deepnoteNotebookFileFactory.unit.test.ts`:
- Around line 223-297: The test duplicates two DeepnoteFile fixtures (fileA and
fileB) that differ only by the order of block hashes; extract a helper factory
function (e.g., makeDeepnoteFile or makeNotebookWithBlocks) used by the test
'should be insensitive to block order (hashes are sorted internally)' to produce
the common DeepnoteFile shape and accept a blocks array (or an order flag) to
vary block order; update fileA and fileB to call that factory instead of
repeating the literal so the fixture is shared and easier to maintain.

In `@src/notebooks/deepnote/deepnoteTreeDataProvider.ts`:
- Around line 351-357: findTreeItem currently only matches on projectId and
returns the top-level group, so Reveal in Explorer can't find the actual
notebook/file row; update the method findTreeItem(projectId: string) to also
accept or use the notebook identifier (the deepnoteNotebookId the callers
extract in deepnoteExplorerView) and traverse group children to return the
specific DeepnoteTreeItem whose context.deepnoteNotebookId (or equivalent file
id in item.context) matches that notebook id instead of returning the top-level
project group; ensure you reference and search within the same item.context
properties used elsewhere (e.g., deepnoteNotebookId) and return the matching
child item.

In `@src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts`:
- Around line 823-828: Update the inline comment in
deepnoteTreeItem.unit.test.ts to use American spelling to satisfy cspell: change
the word "behaviour" to "behavior" in the explanatory block that mentions
invoking the method via the class prototype for DeepnoteTreeItem so the comment
reads "...same behaviour, just side-steps..." -> "...same behavior, just
side-steps..."; reference the comment discussing TreeItem proxy and
DeepnoteTreeItem.prototype.updateVisualFields to locate and edit it.

In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 115-121: The token "iopub" is tripping CI spelling rules in
snapshotService.ts; update occurrences to the canonical "IOPub" (e.g., inside
the comment and any string literals related to iopub messages) or add a local
spellcheck suppression if the lowercase form is intentionally required. Look for
the comment and any code around onDidCompleteQueueExecution,
workspace.onDidChangeNotebookDocument, and references to cell.outputs / iopub
messages and either rename "iopub" → "IOPub" or add a targeted suppression
(spellcheck/eslint) immediately above the offending line.

In `@src/platform/deepnote/deepnoteProjectIdResolver.ts`:
- Line 4: This import breaks platform module boundaries: remove the direct
import of readDeepnoteProjectFile from notebooks in
deepnoteProjectIdResolver.ts; either move the readDeepnoteProjectFile
implementation into a platform-level module (and update
deepnoteProjectIdResolver.ts to import that new module) or convert
deepnoteProjectIdResolver.ts into a node-only file by renaming it to
deepnoteProjectIdResolver.node.ts so the cross-boundary import is allowed;
update the import path for readDeepnoteProjectFile (or the new platform-level
function name) and run the build/tests to ensure no other modules rely on the
old location.
- Line 32: The warning log uses fileUri.fsPath which is forbidden in non-.node
files; update the logger.warn call in deepnoteProjectIdResolver.ts (where
logger.warn(`Failed to resolve Deepnote project id for ${fileUri.fsPath}`,
error)) to use fileUri.toString() instead, or alternatively rename the file to
deepnoteProjectIdResolver.node.ts if filesystem access via fsPath is required;
ensure the log message and error parameter remain unchanged except for replacing
fsPath with toString().

---

Outside diff comments:
In `@src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts`:
- Around line 491-497: The code sets the project mapping via
projectEnvironmentMapper.setEnvironmentForProject(projectId,
selectedEnvironmentId) before calling
kernelAutoSelector.rebuildController(notebook, progress, token), so if
rebuildController throws the project remains pointed at the new environment;
change this by capturing the current environment (call
projectEnvironmentMapper.getEnvironmentForProject(projectId) or equivalent)
before updating, then perform rebuildController in a try block and only commit
the new mapping after successful rebuild OR perform the update first but wrap
rebuildController in try/catch and on any error call
projectEnvironmentMapper.setEnvironmentForProject(projectId,
previousEnvironmentId) to rollback; reference
projectEnvironmentMapper.setEnvironmentForProject,
projectEnvironmentMapper.getEnvironmentForProject, and
kernelAutoSelector.rebuildController when implementing the rollback.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 778-788: The firstBlock object in deepnoteExplorerView.ts sets
executionCount: 0 while createNotebookWithFirstBlock uses undefined, causing
inconsistent conventions; pick one and make both match—either set executionCount
to undefined in the firstBlock definition (DeepnoteBlock firstBlock) or change
createNotebookWithFirstBlock to use 0—update every occurrence so the
DeepnoteBlock instances (firstBlock and createNotebookWithFirstBlock) use the
same executionCount convention and ensure any consumers expect that type (number
or undefined).

In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts`:
- Around line 691-712: The pending-init entry is keyed by the resolved projectId
but onKernelStarted() reads notebook.metadata.deepnoteProjectId, so entries can
be orphaned when resolveProjectIdForNotebook() falls back to parsing; update
onKernelStarted() to call resolveProjectIdForNotebook(notebook) (same resolver
used earlier) and use that resolved projectId to look up and consume
projectsPendingInitNotebook, or alternatively store the pending entry under both
keys, ensuring consistency between the code paths (functions referenced:
resolveProjectIdForNotebook, onKernelStarted, projectsPendingInitNotebook,
notebook.metadata.deepnoteProjectId).

In `@src/notebooks/deepnote/deepnoteTreeDataProvider.ts`:
- Around line 270-287: getNotebooksForProject is currently mapping all
notebooks, which surfaces the internal init notebook; filter out the project's
init notebook (compare notebook.id to project.project.initNotebookId) before
creating DeepnoteTreeItem instances. Update getNotebooksForProject to read const
initId = project.project.initNotebookId and use notebooks.filter(n => n.id !==
initId) (or equivalent) prior to mapping to DeepnoteTreeItem so the init
notebook is omitted from the explorer.
- Around line 165-179: The workspace-level file scan (workspace.findFiles with
new RelativePattern) can throw and currently lives outside the per-file
try/catch, so a failing folder aborts scanning subsequent workspaceFolders; wrap
each workspaceFolder iteration in its own try/catch: call
workspace.findFiles(pattern) and the subsequent projectFiles loop inside that
try block, call this.logger?.error(...) in the catch (including the
workspaceFolder context) and continue so one folder failure doesn’t stop
processing other folders; keep existing per-file try/catch around
this.loadDeepnoteProject(file) as-is.

---

Duplicate comments:
In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 991-995: The code currently uses findProjectUriFromId(projectId)
to resolve projectUri which can pick a sibling .deepnote file; instead derive
the snapshot directory from the current notebook URI (notebook.uri) so snapshots
are written into the correct folder for the active notebook. Replace or guard
uses of projectUri resolved via findProjectUriFromId in snapshotService.ts
(references: findProjectUriFromId(projectId), projectUri) and construct the
snapshot path from notebook.uri (or fallback to findProjectUriFromId only if
notebook.uri cannot be parsed), ensuring all snapshot write operations use the
notebook-derived directory.
- Around line 467-470: The current glob building for latest snapshots only uses
notebook-scoped patterns when notebookId is present, which hides legacy
project-scoped snapshots; change the lookup to first attempt the notebook-scoped
glob (the existing pattern assigned to latestGlob) and if that returns no
matches then run a fallback glob using the project-scoped pattern
`**/snapshots/*_${projectId}_...` (e.g.,
`**/snapshots/*_${projectId}_${notebookId}_latest.snapshot.deepnote` → fallback
`**/snapshots/*_${projectId}_latest.snapshot.deepnote`), and apply the same
two-step try/fallback logic for the similar block at the other occurrence (the
code around the other latest/project glob at lines 515-517) so legacy
project-scoped snapshots remain visible until a notebook-scoped save exists.

🪄 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: 9354a30e-77fe-46ba-912d-3355026d73e0

📥 Commits

Reviewing files that changed from the base of the PR and between b73d313 and b415441.

📒 Files selected for processing (29)

• package.json
• src/kernels/deepnote/deepnoteServerStarter.node.ts
• src/kernels/deepnote/deepnoteServerStarter.unit.test.ts
• src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts
• src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
• src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
• src/kernels/deepnote/environments/deepnoteExtensionSidecarWriter.node.ts
• src/kernels/deepnote/environments/deepnoteExtensionSidecarWriter.unit.test.ts
• src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts
• src/kernels/deepnote/environments/deepnoteProjectEnvironmentMapper.node.ts
• src/kernels/deepnote/environments/deepnoteProjectEnvironmentMapper.unit.test.ts
• src/kernels/deepnote/types.ts
• src/notebooks/deepnote/deepnoteExplorerView.ts
• src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts
• src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
• src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
• src/notebooks/deepnote/deepnoteKernelStatusIndicator.node.ts
• src/notebooks/deepnote/deepnoteKernelStatusIndicator.ts
• src/notebooks/deepnote/deepnoteNotebookFileFactory.unit.test.ts
• src/notebooks/deepnote/deepnoteTreeDataProvider.ts
• src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
• src/notebooks/deepnote/deepnoteTreeItem.ts
• src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
• src/notebooks/deepnote/snapshots/environmentCapture.node.ts
• src/notebooks/deepnote/snapshots/snapshotService.ts
• src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts
• src/notebooks/serviceRegistry.node.ts
• src/platform/deepnote/deepnoteProjectIdResolver.ts
• src/platform/deepnote/deepnoteServerUtils.node.ts

💤 Files with no reviewable changes (1)

• src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Project-only reuse drops file-specific server context.

A reused server keeps the workingDirectory and SQL env vars from whichever .deepnote file started it first, because those still come from deepnoteFileUri in startServerForEnvironment(). If the same project is opened from a snapshot or sibling file in a different directory, the second notebook silently inherits the wrong cwd/env. Reuse needs to validate that file-derived context matches, or derive a project-stable cwd/env instead.

Also applies to: 122-143

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 100 - 107,
The reuse logic in startServer/startServerForEnvironment lets a project-level
reused server retain file-specific context (workingDirectory and SQL env vars)
from the original deepnoteFileUri, so update the reuse validation to either (a)
when attempting to reuse an existing server for the same project, compare the
file-derived context (workingDirectory and any SQL-related envs) computed from
the incoming deepnoteFileUri against the existing server's stored file-derived
context and refuse reuse (force new server) if they differ, or (b) alternatively
derive a project-stable workingDirectory/env (based on projectId) when marking
servers project-scoped; implement this check/derivation inside
startServerForEnvironment and ensure startServer passes through the
deepnoteFileUri-derived context so the comparison can be made before reusing an
existing server instance.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if environmentServers is populated anywhere
rg -n 'environmentServers\.set' --type ts

Repository: deepnote/vscode-deepnote

Length of output: 50

---

🏁 Script executed:

cat -n src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts | head -40

Repository: deepnote/vscode-deepnote

Length of output: 2705

---

🏁 Script executed:

cat -n src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts | sed -n '200,220p'

Repository: deepnote/vscode-deepnote

Length of output: 1021

---

🏁 Script executed:

rg -n 'environmentServers' --type ts -A 2 -B 2

Repository: deepnote/vscode-deepnote

Length of output: 1320

---

🏁 Script executed:

rg -n '(serverStarter|stopServer|startServer|environmentServers)' src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts

Repository: deepnote/vscode-deepnote

Length of output: 384

---

🏁 Script executed:

rg -n 'TODO|FIXME|TODO:' src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts

Repository: deepnote/vscode-deepnote

Length of output: 50

---

environmentServers map is never populated, preventing server cleanup on deletion.

Declared on line 27 but never assigned via .set(). The loop on line 210 always receives an empty array from the nullish coalesce fallback, so servers won't be stopped when environments are deleted.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts` at line
27, The environmentServers Map field is never populated, so deletion code that
expects environmentServers.get(envId) returns the empty fallback; populate and
maintain this map where environment servers are created/started (e.g., in the
method that launches servers — look for functions like createEnvironment,
startEnvironment, or whatever starts the server instances) by calling
environmentServers.set(environmentId, serverIdArray) and update the array if
servers are added/removed; then in the environment deletion/cleanup path (the
loop that reads environmentServers.get(environmentId) and stops servers) iterate
the stored serverIdArray, stop each server (call the existing stop/cleanup
method), and finally call environmentServers.delete(environmentId) so the map
reflects the current state.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't clear project mappings before the delete is durable.

These removals happen before the environment is actually deleted. If the delete fails or is cancelled, the environment still exists but every notebook sharing that projectId loses its selection. Move mapping cleanup after a successful delete, or capture/restore the previous project ids on failure. Also isolate each removal so one mapper error does not abort the rest.

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/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts` around
lines 304 - 310, The current flow calls
projectEnvironmentMapper.getProjectsUsingEnvironment(environmentId) and then
removes mappings via
projectEnvironmentMapper.removeEnvironmentForProject(projectId) before the
environment delete completes; move this mapping cleanup to after a confirmed
successful delete (or alternatively capture the list of projectIds and restore
them if delete fails), and wrap each removeEnvironmentForProject call in its own
try/catch so a single mapper error doesn't abort the loop; keep the initial
waitForInitialization() but ensure mapping removals happen only after delete
success (or use captured projectIds + restore-on-error logic) and reference
projectEnvironmentMapper, waitForInitialization, getProjectsUsingEnvironment,
removeEnvironmentForProject, and environmentId to locate the changes.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Reorder the members to match the repo rule.

This new class interleaves private fields, public events, and private state. Please group members by accessibility first, then alphabetize within each group.

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/kernels/deepnote/environments/deepnoteProjectEnvironmentMapper.node.ts`
around lines 20 - 28, Reorder the class members so they are grouped by
accessibility (public then private) and alphabetized within each group: move the
public event properties onDidRemoveEnvironment and onDidSetEnvironment together
first (alphabetical), then place the private fields _onDidRemoveEnvironment,
_onDidSetEnvironment, initializationPromise, mappings, and workspaceState
together and alphabetize them (e.g., _onDidRemoveEnvironment,
_onDidSetEnvironment, initializationPromise, mappings, workspaceState) so the
class follows the repo rule.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

findTreeItem() no longer returns the notebook row the caller is trying to reveal.

src/notebooks/deepnote/deepnoteExplorerView.ts still extracts deepnoteNotebookId, but this method now matches only projectId and returns the top-level group. Reveal in Explorer will stop selecting the actual notebook/file row and only focus the project group.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteTreeDataProvider.ts` around lines 351 - 357,
findTreeItem currently only matches on projectId and returns the top-level
group, so Reveal in Explorer can't find the actual notebook/file row; update the
method findTreeItem(projectId: string) to also accept or use the notebook
identifier (the deepnoteNotebookId the callers extract in deepnoteExplorerView)
and traverse group children to return the specific DeepnoteTreeItem whose
context.deepnoteNotebookId (or equivalent file id in item.context) matches that
notebook id instead of returning the top-level project group; ensure you
reference and search within the same item.context properties used elsewhere
(e.g., deepnoteNotebookId) and return the matching child item.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/notebooks/deepnote/snapshots/snapshotService.ts (1)

356-358: 🧹 Nitpick | 🔵 Trivial

Extract timeout as a named constant.

Hardcoded 10_000 should join the other constants at module level for consistency.

Suggested fix

+/** Timeout for waiting on environment capture before giving up. */
+const environmentCaptureTimeoutMs = 10_000;

Then at line 357:

-                setTimeout(() => reject(new TimeoutError('Timeout waiting for environment capture.')), 10_000);
+                setTimeout(() => reject(new TimeoutError('Timeout waiting for environment capture.')), environmentCaptureTimeoutMs);

🤖 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 356 - 358,
Extract the hardcoded 10_000 into a descriptive module-level constant (e.g.,
ENVIRONMENT_CAPTURE_TIMEOUT_MS) alongside the other top-level constants, then
replace the literal in the Timeout promise creation inside snapshotService.ts
(the timeoutPromise where setTimeout rejects with new TimeoutError('Timeout
waiting for environment capture.')) with that constant to keep configuration
consistent and readable.

🤖 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/deepnoteEnvironmentsView.unit.test.ts`:
- Around line 1092-1097: The test currently uses assert.deepEqual to compare the
rebuildCalls array to expected values; replace that call with
assert.deepStrictEqual to enforce strict equality for the array comparison.
Update the assertion that references rebuildCalls and the expected array
(containing initiatingNotebook.uri.toString() and
siblingNotebook.uri.toString()) to use assert.deepStrictEqual(rebuildCalls,
[...]) so the test performs strict deep comparison.

In `@src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts`:
- Around line 159-161: The test contains a redundant assertion checking
item.contextValue twice; remove the duplicate literal assertion and keep the
single assertion using the constant NOTEBOOK_FILE_CONTEXT_VALUE (i.e., keep
assert.strictEqual(item.contextValue, NOTEBOOK_FILE_CONTEXT_VALUE) and delete
the assert.strictEqual(item.contextValue, 'notebookFile') line) so the test
relies on the canonical constant.

In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 863-866: The hardcoded timeout 100ms passed to
waitForPendingCellStateChanges(notebookUri, 100) should be replaced with a named
constant for clarity and reuse; define a constant (e.g., cellStateChangeWaitMs)
near the top of snapshotService.ts or in an appropriate constants section,
initialize it to 100, and use it in the call so it reads await
this.waitForPendingCellStateChanges(notebookUri, cellStateChangeWaitMs).

---

Outside diff comments:
In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 356-358: Extract the hardcoded 10_000 into a descriptive
module-level constant (e.g., ENVIRONMENT_CAPTURE_TIMEOUT_MS) alongside the other
top-level constants, then replace the literal in the Timeout promise creation
inside snapshotService.ts (the timeoutPromise where setTimeout rejects with new
TimeoutError('Timeout waiting for environment capture.')) with that constant to
keep configuration consistent and readable.

🪄 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: 7b8197da-2cff-4dd7-967a-e71887ac6a67

📥 Commits

Reviewing files that changed from the base of the PR and between b415441 and 32671f0.

📒 Files selected for processing (9)

• src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
• src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
• src/kernels/deepnote/environments/deepnoteProjectEnvironmentMapper.node.ts
• src/notebooks/deepnote/deepnoteProjectUtils.ts
• src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
• src/notebooks/deepnote/snapshots/snapshotService.ts
• src/platform/deepnote/deepnoteProjectFileReader.ts
• src/platform/deepnote/deepnoteProjectFileReader.unit.test.ts
• src/platform/deepnote/deepnoteProjectIdResolver.ts

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Use assert.deepStrictEqual() for array comparison.

Per coding guidelines, prefer strict equality.

Proposed fix

-            assert.deepEqual(rebuildCalls, [
+            assert.deepStrictEqual(rebuildCalls, [
                 initiatingNotebook.uri.toString(),
                 siblingNotebook.uri.toString(),
                 siblingNotebook.uri.toString(),
                 initiatingNotebook.uri.toString()
             ]);

📝 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

	assert.deepEqual(rebuildCalls, [
	initiatingNotebook.uri.toString(),
	siblingNotebook.uri.toString(),
	siblingNotebook.uri.toString(),
	initiatingNotebook.uri.toString()
	]);
	assert.deepStrictEqual(rebuildCalls, [
	initiatingNotebook.uri.toString(),
	siblingNotebook.uri.toString(),
	siblingNotebook.uri.toString(),
	initiatingNotebook.uri.toString()
	]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts`
around lines 1092 - 1097, The test currently uses assert.deepEqual to compare
the rebuildCalls array to expected values; replace that call with
assert.deepStrictEqual to enforce strict equality for the array comparison.
Update the assertion that references rebuildCalls and the expected array
(containing initiatingNotebook.uri.toString() and
siblingNotebook.uri.toString()) to use assert.deepStrictEqual(rebuildCalls,
[...]) so the test performs strict deep comparison.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Redundant assertion.

Lines 160 and 161 assert the same value twice — once via constant, once via literal. Keep one.

♻️ Suggested fix

             assert.strictEqual(item.label, 'First Notebook');
-            assert.strictEqual(item.contextValue, NOTEBOOK_FILE_CONTEXT_VALUE);
-            assert.strictEqual(item.contextValue, 'notebookFile');
+            assert.strictEqual(item.contextValue, NOTEBOOK_FILE_CONTEXT_VALUE); // 'notebookFile'
             assert.strictEqual(item.tooltip, 'Deepnote Project: Test Project\nFile: /workspace/my-project.deepnote');

📝 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

	assert.strictEqual(item.label, 'First Notebook');
	assert.strictEqual(item.contextValue, NOTEBOOK_FILE_CONTEXT_VALUE);
	assert.strictEqual(item.contextValue, 'notebookFile');
	assert.strictEqual(item.label, 'First Notebook');
	assert.strictEqual(item.contextValue, NOTEBOOK_FILE_CONTEXT_VALUE); // 'notebookFile'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts` around lines 159 - 161,
The test contains a redundant assertion checking item.contextValue twice; remove
the duplicate literal assertion and keep the single assertion using the constant
NOTEBOOK_FILE_CONTEXT_VALUE (i.e., keep assert.strictEqual(item.contextValue,
NOTEBOOK_FILE_CONTEXT_VALUE) and delete the
assert.strictEqual(item.contextValue, 'notebookFile') line) so the test relies
on the canonical constant.

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

Extract hardcoded 100ms.

Minor: 100 could be a named constant like cellStateChangeWaitMs.

🤖 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 863 - 866,
The hardcoded timeout 100ms passed to
waitForPendingCellStateChanges(notebookUri, 100) should be replaced with a named
constant for clarity and reuse; define a constant (e.g., cellStateChangeWaitMs)
near the top of snapshotService.ts or in an appropriate constants section,
initialize it to 100, and use it in the call so it reads await
this.waitForPendingCellStateChanges(notebookUri, cellStateChangeWaitMs).

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/notebooks/deepnote/deepnoteAutoSplitter.ts (1)

155-158: ⚠️ Potential issue | 🟠 Major

Skip notebook-scoped snapshots in this loop.

snapshotGlob also matches the new {slug}_{projectId}_{notebookId}_{variant}.snapshot.deepnote names, but extractVariantFromSnapshotFilename() only understands the legacy shape. Reprocessing a new-format file here turns notebookId_variant into the variant, writes doubly-suffixed filenames, and then deletes the valid source snapshot.

Possible fix

-import { slugifyProjectName } from './snapshots/snapshotFiles';
+import { extractNotebookIdFromSnapshotUri, slugifyProjectName } from './snapshots/snapshotFiles';
...
         for (const snapshotUri of allSnapshotFiles) {
             try {
+                if (extractNotebookIdFromSnapshotUri(snapshotUri)) {
+                    continue;
+                }
                 await this.splitSingleSnapshot(snapshotUri, slug, projectId, allNotebookIds);
             } catch (error) {
                 logger.warn(`[AutoSplitter] Failed to split snapshot ${snapshotUri.path}`, error);
             }
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteAutoSplitter.ts` around lines 155 - 158, The
loop that calls splitSingleSnapshot on each snapshotUri must skip new-format
notebook-scoped snapshots because extractVariantFromSnapshotFilename only
handles the legacy shape; before calling splitSingleSnapshot (inside the
for...of over allSnapshotFiles) add a guard that detects the new-format filename
pattern (e.g., contains slug + "_" + projectId + "_" + notebookId + "_...
.snapshot.deepnote" — match with a regex like `^${slug}_${projectId}_\\d+_`) and
continue if it matches so those notebook-scoped files are not reprocessed or
deleted.

🤖 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 89-98: The checkAndSplitIfNeeded path reads the on-disk file and
can overwrite doc.uri contents, so avoid operating on an in-memory dirty
notebook: inside checkAndSplitIfNeeded, add an early return when the
NotebookDocument is dirty (check doc.isDirty) before calling
workspace.fs.readFile / deserializeDeepnoteFile / autoSplitter.splitIfNeeded;
this ensures you only read from disk and auto-split for clean documents and
prevents the editor and saved .deepnote file from getting out of sync.

In `@src/notebooks/deepnote/deepnoteAutoSplitter.ts`:
- Around line 133-136: The loop that collects snapshot files uses
workspace.findFiles(pattern, null, 100) which truncates results to 100 per
folder and causes incomplete migrations; change the call in the for loop that
iterates workspaceFolders (where pattern is built from RelativePattern(folder,
snapshotGlob) and results are concatenated into allSnapshotFiles) to fetch all
matches (i.e., remove the 100 maxResults argument or pass undefined) so every
snapshot file is returned and included in allSnapshotFiles.

---

Duplicate comments:
In `@src/notebooks/deepnote/deepnoteAutoSplitter.ts`:
- Around line 155-158: The loop that calls splitSingleSnapshot on each
snapshotUri must skip new-format notebook-scoped snapshots because
extractVariantFromSnapshotFilename only handles the legacy shape; before calling
splitSingleSnapshot (inside the for...of over allSnapshotFiles) add a guard that
detects the new-format filename pattern (e.g., contains slug + "_" + projectId +
"_" + notebookId + "_... .snapshot.deepnote" — match with a regex like
`^${slug}_${projectId}_\\d+_`) and continue if it matches so those
notebook-scoped files are not reprocessed or deleted.

🪄 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: b9bb52e6-ae15-4b09-a6b4-75a647fbba89

📥 Commits

Reviewing files that changed from the base of the PR and between 32671f0 and bfd0cc3.

📒 Files selected for processing (4)

• src/notebooks/deepnote/deepnoteActivationService.ts
• src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
• src/notebooks/deepnote/deepnoteAutoSplitter.ts
• src/notebooks/deepnote/deepnoteAutoSplitter.unit.test.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Skip dirty notebooks before auto-splitting.

This path reads from disk and can rewrite doc.uri, so an already-open dirty notebook gets split from stale bytes instead of the in-memory document. That can leave the open editor and the saved .deepnote file out of sync.

Possible fix

     private async checkAndSplitIfNeeded(doc: import('vscode').NotebookDocument): Promise<void> {
         if (doc.notebookType !== 'deepnote') {
             return;
         }
+
+        if (doc.isDirty) {
+            this.logger.debug(`Skipping auto-split for dirty notebook: ${doc.uri.path}`);
+            return;
+        }
 
         try {
             const fileUri = doc.uri;
             const content = await workspace.fs.readFile(fileUri);

🤖 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 89 - 98,
The checkAndSplitIfNeeded path reads the on-disk file and can overwrite doc.uri
contents, so avoid operating on an in-memory dirty notebook: inside
checkAndSplitIfNeeded, add an early return when the NotebookDocument is dirty
(check doc.isDirty) before calling workspace.fs.readFile /
deserializeDeepnoteFile / autoSplitter.splitIfNeeded; this ensures you only read
from disk and auto-split for clean documents and prevents the editor and saved
.deepnote file from getting out of sync.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't truncate snapshot migration at 100 files.

workspace.findFiles(..., 100) makes this migration partial for projects with more than 100 matching snapshots. That leaves a mixed legacy/new snapshot set for the same project.

Possible fix

         for (const folder of workspaceFolders) {
             const pattern = new RelativePattern(folder, snapshotGlob);
-            const files = await workspace.findFiles(pattern, null, 100);
+            const files = await workspace.findFiles(pattern);
             allSnapshotFiles = allSnapshotFiles.concat(files);
         }

📝 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

	for (const folder of workspaceFolders) {
	const pattern = new RelativePattern(folder, snapshotGlob);
	const files = await workspace.findFiles(pattern, null, 100);
	allSnapshotFiles = allSnapshotFiles.concat(files);
	for (const folder of workspaceFolders) {
	const pattern = new RelativePattern(folder, snapshotGlob);
	const files = await workspace.findFiles(pattern);
	allSnapshotFiles = allSnapshotFiles.concat(files);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/notebooks/deepnote/deepnoteAutoSplitter.ts` around lines 133 - 136, The
loop that collects snapshot files uses workspace.findFiles(pattern, null, 100)
which truncates results to 100 per folder and causes incomplete migrations;
change the call in the for loop that iterates workspaceFolders (where pattern is
built from RelativePattern(folder, snapshotGlob) and results are concatenated
into allSnapshotFiles) to fetch all matches (i.e., remove the 100 maxResults
argument or pass undefined) so every snapshot file is returned and included in
allSnapshotFiles.