feat(deepnote): one notebook per .deepnote file

[tkislan]

Migrates the extension to a single notebook per .deepnote file model, removing the multi-notebook-per-file machinery (the ?notebook=<id> URI selection and its timers/manager state).

What changes

• Deserialize renders one notebook — the first non-init notebook (falling back to the init only when it is the file's only notebook); serialize resolves the target from document metadata with an exact (projectId, notebookId) lookup.
• Legacy multi-notebook files are split on demand — opening one shows a notification that writes one new single-notebook file per notebook, migrates the environment selection, then deletes the original (write-before-delete; the original is never lost on failure).
• New / duplicated notebooks become sibling files (never appended into one file).
• Project metadata fans out across siblings — integration/rename edits are written to every sibling .deepnote of the project on disk (open or closed), with file-watcher self-write suppression.
• Explorer is grouped by project (ProjectGroup → file → notebook) plus a status bar showing the active notebook with a "Copy details" command.
• Server & environment are keyed per notebook (notebook.uri.toString(), 1:1 with the kernel/controller). Environment deletion now actually stops every server using it (including closed-but-running ones), fixing a real bug.
• Snapshots are notebook-scoped with a backward-compatible reader (legacy project-scoped snapshots still load as a fallback and are never migrated) and a deferred, output-settled save.
• Init notebook runs per kernel from its sibling file — re-running after an in-place restart (tracked in a WeakSet<IKernel>, not a persistent project flag).

How it was built

Implemented in 7 sequential, independently-reviewed chunks (8 commits). Each chunk was implemented, reviewed against the plan, unit-tested against the plan's use cases, and committed green.

Verification

• tsc clean, both esbuild bundles build, prettier --check clean.
• Unit suite: 2501 passing, 234 pending.
• The 1 failing TextBlockConverter test and 3 cloud-sql tsc errors are pre-existing local symlink drift (the @deepnote/* packages are ahead of the repo locally; CI uses the published versions) — not introduced by this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro

Summary by CodeRabbit

• New Features
  • Added a Deepnote status bar item to copy the active notebook’s details.
  • Added multi-notebook splitting into single-notebook sibling files, including environment mapping migration.
  • Refreshed the Explorer into grouped project/file hierarchy with notebook-scoped actions.
• Bug Fixes
  • Improved per-notebook handling across server lifecycle, environments deletion, snapshots, SQL/integrations, and federated-auth kernel restarts.
  • Prevented multi-select Jupyter imports from overwriting each other.
  • Normalized bullet text rendering for bullet cells.
• Chores
  • Updated extension commands/menus and refreshed unit/e2e coverage and fixtures.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0%. Comparing base (735e5f5) to head (72677c4).
✅ All tests successful. No failed tests found.

Additional details and impacted files

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

🚀 New features to boost your workflow:

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

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c55d3a89-419b-4fc8-82f3-5b69e1619709

📥 Commits

Reviewing files that changed from the base of the PR and between ea42807 and 72677c4.

📒 Files selected for processing (1)

• src/notebooks/deepnote/snapshots/executionMetadataTracker.unit.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/notebooks/deepnote/snapshots/executionMetadataTracker.unit.test.ts

---

📝 Walkthrough

Walkthrough

Deepnote notebook identity is now notebook-scoped across caching, serialization, snapshots, kernels, server handles, integrations, and explorer actions. The explorer groups sibling .deepnote files under project groups and supports sibling-file creation, rename, delete, duplicate, export, and split flows. The PR also adds a notebook details status bar, new command wiring, and broader unit/e2e coverage for opening, splitting, environments, snapshots, integrations, and file watching.

Estimated code review effort: 5 (Critical) | ~150 minutes

Possibly related PRs

• deepnote/vscode-deepnote#11: Builds on the same Deepnote activation and notebook-opening plumbing.
• deepnote/vscode-deepnote#16: Shares the kernel/server lifecycle area now made notebook-scoped.
• deepnote/vscode-deepnote#24: Shares the integrations contract changes now keyed by notebook id.

Suggested reviewers: andyjakubowski, saltenasl, Artmann, mfranczel

🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Updates Docs	⚠️ Warning	README/specs still describe the old project-based multi-notebook flow, and this patch adds no docs files.	Update the OSS docs (README/specs) for the single-notebook flow, and add the roadmap note in deepnote/deepnote-internal’s landing page.

✅ 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 is concise and accurately summarizes the main change: moving Deepnote to one notebook per .deepnote file.
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.

---

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

[coderabbitai]

Actionable comments posted: 16

Caution

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

⚠️ Outside diff range comments (3)

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

900-934: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Check all import targets in one collision set.

The current checks miss collisions between selected .deepnote files and Jupyter-derived .deepnote outputs, and between two selected .deepnote files with the same basename. Build one seenTargetNames set for all selected outputs before any writes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 900 - 934, The
import flow in deepnoteExplorerView should validate all output names together
instead of checking only existing workspace files per .deepnote input. In the
import logic around checkJupyterImportTargetsAvailable and
convertJupyterUrisToDeepnoteFiles, build a single seenTargetNames set for every
selected output basename before any writes, and reject duplicates across
selected .deepnote files and Jupyter-derived .deepnote targets as well as
collisions with existing workspace files.

src/kernels/deepnote/types.ts (1)

174-179: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Stale JSDoc on stopServer: @param environmentId no longer exists.

The method now takes deepnoteFileUri, but the doc still documents @param environmentId The environment ID and the summary says "for a kernel environment" instead of a notebook. Compare with startServer's doc a few lines up, which was correctly updated to describe notebook-URI keying.

📝 Fix stale doc

     /**
-     * Stops the deepnote-toolkit server for a kernel environment.
-     * `@param` environmentId The environment ID
+     * Stops the deepnote-toolkit server for a notebook.
+     * `@param` deepnoteFileUri The URI of the .deepnote file
      * `@param` token Cancellation token to cancel the operation
      */
     stopServer(deepnoteFileUri: vscode.Uri, token?: vscode.CancellationToken): Promise<void>;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/kernels/deepnote/types.ts` around lines 174 - 179, The JSDoc for
stopServer is stale and still describes the old environmentId-based API. Update
the comment above stopServer in the Deepnote types so the summary matches its
notebook-URI-based behavior and the `@param` entry documents deepnoteFileUri
instead of environmentId, keeping it consistent with the nearby startServer
documentation.

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

568-607: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Duplicate self-write marker logic.

markSelfWrite and markSnapshotSelfWrite are structurally identical, differing only in which Set/Map pair they touch. As per coding guidelines, "Extract duplicate logic into helper methods to prevent drift following DRY principle."

♻️ Proposed consolidation

-    private markSelfWrite(uri: Uri): void {
-        const key = this.selfWriteKey(uri);
-        this.selfWriteUris.add(key);
-
-        // Safety net: clean stale entries after 30s
-        const existing = this.selfWriteTimers.get(key);
-        if (existing) {
-            clearTimeout(existing);
-        }
-        this.selfWriteTimers.set(
-            key,
-            setTimeout(() => {
-                this.selfWriteUris.delete(key);
-                this.selfWriteTimers.delete(key);
-            }, selfWriteExpirationMs)
-        );
-    }
+    private markSelfWrite(uri: Uri): void {
+        this.markSelfWriteInternal(uri, this.selfWriteUris, this.selfWriteTimers);
+    }
+
+    private markSelfWriteInternal(
+        uri: Uri,
+        uris: Set<string>,
+        timers: Map<string, ReturnType<typeof setTimeout>>
+    ): void {
+        const key = this.selfWriteKey(uri);
+        uris.add(key);
+
+        const existing = timers.get(key);
+        if (existing) {
+            clearTimeout(existing);
+        }
+        timers.set(
+            key,
+            setTimeout(() => {
+                uris.delete(key);
+                timers.delete(key);
+            }, selfWriteExpirationMs)
+        );
+    }
 
     private markSnapshotSelfWrite(uri: Uri): void {
-        const key = this.selfWriteKey(uri);
-        this.snapshotSelfWriteUris.add(key);
-
-        // Safety net: clean stale entries after 30s
-        const existing = this.snapshotSelfWriteTimers.get(key);
-        if (existing) {
-            clearTimeout(existing);
-        }
-        this.snapshotSelfWriteTimers.set(
-            key,
-            setTimeout(() => {
-                this.snapshotSelfWriteUris.delete(key);
-                this.snapshotSelfWriteTimers.delete(key);
-            }, selfWriteExpirationMs)
-        );
+        this.markSelfWriteInternal(uri, this.snapshotSelfWriteUris, this.snapshotSelfWriteTimers);
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteFileChangeWatcher.ts` around lines 568 - 607,
`markSelfWrite` and `markSnapshotSelfWrite` duplicate the same timer/cleanup
flow and should be consolidated to avoid drift. Extract the shared self-write
tracking behavior into a helper in `deepnoteFileChangeWatcher` that accepts the
target Set and Map (or equivalent parameters), then have both methods delegate
to it while preserving their distinct markers and `selfWriteKey` usage.

Source: Coding guidelines

🧹 Nitpick comments (30)

.vscode/launch.json (1)

20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

preLaunchTask disabled only for the main "Extension" config.

Debugging will now run against stale dist/ output unless devs remember to build manually first, while every other launch config still builds automatically. If intentional (e.g. faster iteration with a separate watch task), fine — otherwise this is likely to cause confusing "my fix isn't showing up" reports.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.vscode/launch.json at line 20, The main “Extension” launch config has
`preLaunchTask` disabled, so debugging can use stale `dist/` output while the
other configs still build automatically. Either re-enable `preLaunchTask` for
the `Extension` entry in `launch.json`, or make the intent explicit by wiring it
to the same build/watch workflow used by the other launch configurations so
`dist/` stays up to date.

build/mocha-esm-loader.js (1)

107-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Truncated comment.

// Intercept @deepnote/runtime-core - needed because the real startServer/stopServer cuts off mid-sentence. The equivalent comment at Line 355-357 completes the thought ("spawn/kill real Python processes..."). Consider mirroring that text here for consistency.

✏️ Suggested fix

-    // Intercept `@deepnote/runtime-core` - needed because the real startServer/stopServer
+    // Intercept `@deepnote/runtime-core` - needed because the real startServer/stopServer
+    // spawn/kill real Python processes.
     if (specifier === '`@deepnote/runtime-core`') {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build/mocha-esm-loader.js` around lines 107 - 113, The comment in the
`@deepnote/runtime-core` intercept block is truncated and should be made
consistent with the later explanatory comment in the same loader. Update the
inline comment near the specifier check in mocha-esm-loader.js so it mirrors the
full wording used elsewhere about avoiding real startServer/stopServer behavior
that would spawn/kill real Python processes, keeping the intent clear and
consistent across the file.

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

11-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor ordering nit vs. alphabetical convention.

As per coding guidelines, order exports alphabetically; allocateSiblingUri should precede deepnoteFileExists.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteSiblingFileAllocator.ts` around lines 11 - 61,
The exported helpers in deepnoteSiblingFileAllocator are not in alphabetical
order, so reorder the exports so allocateSiblingUri appears before
deepnoteFileExists. Keep the existing implementations intact and only adjust the
declaration order in this module, including the splitBasename helper remaining
non-exported below them.

Source: Coding guidelines

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

15-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Function order doesn't follow alphabetical convention.

As per coding guidelines, **/*.{ts,tsx} files should order methods/fields alphabetically. Current order is getFileStem, buildSingleNotebookFile, buildSiblingNotebookFileUri; alphabetical would put buildSiblingNotebookFileUri/buildSingleNotebookFile first.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteNotebookFileFactory.ts` around lines 15 - 61,
The exported functions in deepnoteNotebookFileFactory are not ordered
alphabetically. Reorder the declarations in this module so the build* functions
come before getFileStem, keeping the existing behavior unchanged and preserving
the current symbols buildSiblingNotebookFileUri, buildSingleNotebookFile, and
getFileStem.

Source: Coding guidelines

src/notebooks/deepnote/deepnoteMultiNotebookSplitter.ts (3)

75-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Private methods not in alphabetical order.

Order is handleNotebookOpened, splitFile, allocateLegacyUri, closeNotebookTab; alphabetical would be allocateLegacyUri, closeNotebookTab, handleNotebookOpened, splitFile.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteMultiNotebookSplitter.ts` around lines 75 -
213, The private methods in DeepnoteMultiNotebookSplitter are not ordered
alphabetically, so rearrange them to match the expected order:
allocateLegacyUri, closeNotebookTab, handleNotebookOpened, then splitFile. Keep
the method bodies unchanged and move only the declarations in the class so the
ordering is consistent for maintainability.

Source: Coding guidelines

---

52-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Avoid void for fire-and-forget async calls.

Based on learnings, this repo prefers explicit handling (await, return the promise, or .catch(...)) over void someAsyncFn(). Line 60's surrounding try/catch also can't catch async rejections from the void-wrapped call — it only guards synchronous throws, so it's misleading as written.

♻️ Proposed fix

-            workspace.onDidOpenNotebookDocument((notebook) => {
-                void this.handleNotebookOpened(notebook);
-            })
+            workspace.onDidOpenNotebookDocument((notebook) => {
+                this.handleNotebookOpened(notebook).catch((error) =>
+                    this.logger.error('Unhandled error inspecting Deepnote notebook for split', error)
+                );
+            })

-        for (const notebook of workspace.notebookDocuments) {
-            try {
-                void this.handleNotebookOpened(notebook);
-            } catch (error) {
-                this.logger.error('Failed to inspect open Deepnote notebook for multi-notebook split', error);
-            }
-        }
+        for (const notebook of workspace.notebookDocuments) {
+            this.handleNotebookOpened(notebook).catch((error) =>
+                this.logger.error('Failed to inspect open Deepnote notebook for multi-notebook split', error)
+            );
+        }

Also applies to: 58-64

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteMultiNotebookSplitter.ts` around lines 52 -
55, The fire-and-forget call in onDidOpenNotebookDocument is using void around
handleNotebookOpened, which this repo avoids and which bypasses the surrounding
try/catch for async failures. Update the callback to explicitly handle the
promise from this.handleNotebookOpened(notebook) by awaiting it in an async
handler, returning the promise, or chaining a .catch(...) so rejections are
handled consistently. Keep the fix centered on DeepnoteMultiNotebookSplitter and
its handleNotebookOpened usage.

Source: Learnings

---

187-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

allocateLegacyUri duplicates the bounded-attempt allocation loop shape used by allocateSiblingUri.

Both loops walk numbered suffixes up to a max-attempts cap and throw a similar error; consider a small shared helper (e.g., a generic findFreeName(candidates, exists) generator) to avoid drift between the two cap/error-message conventions.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteMultiNotebookSplitter.ts` around lines 187 -
198, The bounded allocation loop in allocateLegacyUri duplicates the same
numbered-suffix pattern already used by allocateSiblingUri, so refactor both to
share a small helper for trying candidate names and checking exists(). Extract
the common attempt/max-cap logic into a reusable helper in
deepnoteMultiNotebookSplitter.ts, then have allocateLegacyUri and
allocateSiblingUri both call it so the suffix-generation and error-message
conventions stay aligned. Keep the existing behavior and unique symbols
(allocateLegacyUri, allocateSiblingUri, MAX_LEGACY_ALLOCATION_ATTEMPTS) intact
while removing the duplicated loop shape.

Source: Coding guidelines

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

879-888: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use one object comparison for the written notebook shape.

These assertions validate one object shape; prefer assert.deepStrictEqual() on the relevant picked fields. As per coding guidelines, “Use assert.deepStrictEqual() for object comparisons instead of checking individual properties.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts` around lines 879 -
888, The updated notebook shape check in the Deepnote explorer test is asserting
the same object field-by-field, so replace the individual property expectations
with one deep object comparison. In the test around deserializeDeepnoteFile and
updatedProjectData.project.notebooks[0], compare the picked notebook fields with
assert.deepStrictEqual() against the expected object shape, keeping the project
id and notebook contents validation but avoiding separate per-property
assertions.

Source: Coding guidelines

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

20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate DEEPNOTE_NOTEBOOK_TYPE constant.

This re-declares a constant already exported from src/kernels/deepnote/types.ts. Import the shared constant instead of duplicating the literal — prevents drift if the canonical value ever changes.

♻️ Proposed fix

-const DEEPNOTE_NOTEBOOK_TYPE = 'deepnote';
+import { DEEPNOTE_NOTEBOOK_TYPE } from '../../kernels/deepnote/types';

As per path instructions, "Extract duplicate logic into helper methods to prevent drift following DRY principle."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts` at line 20,
Duplicate DEEPNOTE_NOTEBOOK_TYPE is being re-declared instead of reusing the
shared source of truth. Update deepnoteNotebookInfoStatusBar to import the
exported DEEPNOTE_NOTEBOOK_TYPE from src/kernels/deepnote/types.ts and remove
the local literal so the status bar logic uses the canonical constant
consistently.

Source: Path instructions

---

72-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Private methods aren't alphabetically ordered.

Current order (handleActiveEditorChanged → handleNotebookDocumentChanged → updateStatusBar → copyActiveNotebookDetails → formatNotebookDetails) isn't alphabetical.

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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts` around lines 72 -
137, The private methods in the Deepnote notebook info status bar class are not
ordered alphabetically after accessibility. Reorder the private members in this
block so the methods in DeepnoteNotebookInfoStatusBar follow the guideline by
alphabetical name, keeping related behavior unchanged; specifically adjust the
order of handleActiveEditorChanged, handleNotebookDocumentChanged,
updateStatusBar, copyActiveNotebookDetails, and formatNotebookDetails.

Source: Coding guidelines

src/notebooks/deepnote/integrations/integrationManager.ts (1)

98-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated project/notebook ID extraction pattern.

The const projectId = ...deepnoteProjectId; const notebookId = ...deepnoteNotebookId; if (!projectId || !notebookId) {...} pattern appears here twice (Lines 99-105, 130-135) and again in federatedAuthKernelRestartBridge.node.ts (Lines 60-64). Extracting a shared helper (e.g. getDeepnoteNotebookIdentifiers(notebook)) would prevent drift as more consumers adopt notebook-scoping.

♻️ Suggested helper

export function getDeepnoteNotebookIdentifiers(
    notebook: NotebookDocument
): { projectId: string; notebookId: string } | undefined {
    const projectId = notebook.metadata?.deepnoteProjectId as string | undefined;
    const notebookId = notebook.metadata?.deepnoteNotebookId as string | undefined;
    return projectId && notebookId ? { projectId, notebookId } : undefined;
}

As per coding guidelines, "Extract duplicate logic into helper methods to prevent drift following DRY principle".

Also applies to: 131-132

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/integrations/integrationManager.ts` around lines 98 -
108, The Deepnote notebook project/notebook ID lookup is duplicated in
IntegrationManager and should be centralized to avoid drift. Extract the
repeated metadata access and missing-ID guard in integrationManager.ts into a
shared helper such as getDeepnoteNotebookIdentifiers(notebook), then use that
helper wherever projectId/notebookId are needed, including the other occurrence
in IntegrationManager and the similar logic in
federatedAuthKernelRestartBridge.node.ts. Keep the existing early-return
behavior when identifiers are missing, but source it from the helper so all
callers stay consistent.

Source: Coding guidelines

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

233-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Inconsistent guard style for missing notebookId.

Line 233-234 silently falls back to undefined project when notebookId is missing, while line 330-334 explicitly surfaces an error for the same missing-metadata case. Under the new single-notebook-per-file model this should rarely trigger, but aligning both paths (e.g., both silent-degrade or both explicit) would make the missing-metadata handling more predictable.

Also applies to: 330-337

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/sqlCellStatusBarProvider.ts` around lines 233 - 234,
The missing-`notebookId` handling in the status bar provider is inconsistent
between the project lookup path and the later metadata check. Align the behavior
in `sqlCellStatusBarProvider` by choosing one approach for both
`getProjectForNotebook` usage and the explicit `deepnoteNotebookId` validation
path: either consistently return a safe fallback without special logging, or
consistently surface an error when metadata is absent. Keep the handling
centered around the existing `notebookId`, `project`, and the later
missing-metadata branch so the behavior is predictable across both code paths.

src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts (1)

438-481: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider a regression test for projectName caching.

Good coverage of the cache-only update path. No test exercises the projectName overwrite behavior flagged in integrationWebview.ts (calling show() a second time without projectName clears the cached value). Worth a follow-up test once that behavior is confirmed/resolved.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts` around
lines 438 - 481, Add a regression test around the `show()` flow in
`IntegrationWebviewProvider` to cover `projectName` caching: verify that a
second call to `show()` without a `projectName` does not clear an existing
cached name, or update the implementation if the overwrite behavior in
`integrationWebview.ts` is unintended. Reuse the existing `show(provider, ...)`
helper and the `IntegrationWebviewProvider` setup so the test directly exercises
the cache state transition.

test/e2e/suite/environment.e2e.test.ts (1)

58-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate stale-element reload detection vs. workspace.ts's openFolderViaDialog.

reloadWindow re-implements the same "poll previousWorkbench.getTagName() until it throws" pattern already in test/e2e/helpers/workspace.ts (lines 58-70), just with different timeout/retry wiring. The two have already diverged (one-shot wait here vs. retry loop there) — a good candidate to consolidate into a shared helper (e.g., waitForWorkbenchStale(el, timeout)) in helpers/ before they drift further apart.

As per coding guidelines: "Extract duplicate logic into helper methods to prevent drift following DRY principle."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/suite/environment.e2e.test.ts` around lines 58 - 83, The
stale-element reload detection in reloadWindow duplicates the same polling logic
already used by openFolderViaDialog in workspace.ts, so extract it into a shared
helper under helpers/ to keep the behavior consistent. Create a reusable
wait-for-stale-workbench utility that encapsulates the
previousWorkbench.getTagName() retry pattern and timeout handling, then have
reloadWindow call that helper instead of inlining its own wait/catch flow.

Source: Coding guidelines

test/e2e/helpers/workspace.ts (1)

33-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Doc wording differs slightly from clickDialogOkButton's JSDoc.

This comment says clicking OK "navigates one level toward the typed path rather than accepting it" (hence the retry loop), while quickInput.ts's updated JSDoc for clickDialogOkButton calls it "the deterministic accept" in contrast to Enter. Worth tightening the wording in one place so a future reader isn't confused about whether a single OK click accepts or only converges after several clicks.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/helpers/workspace.ts` around lines 33 - 37, Tighten the wording
between the workspace helper docs and clickDialogOkButton’s JSDoc so they
describe the same OK-button behavior. Update the comment in workspace.ts around
the open-absolute-folder retry loop to match the terminology used by
clickDialogOkButton in quickInput.ts, clarifying whether OK is the deterministic
accept action or a step toward acceptance, and keep the explanation consistent
across both symbols.

test/e2e/suite/notebookCommands.e2e.test.ts (1)

63-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Hardcoded sleep durations.

sleep(1200), sleep(800), sleep(500), sleep(1500) are inline literals rather than named constants (unlike TREE_LOAD_TIMEOUT/WORKBENCH_TIMEOUT used elsewhere in this file).

As per coding guidelines, **/*.{ts,tsx}: "Extract magic numbers (retry counts, delays, timeouts) as named constants near the top of the module."

Also applies to: 97-97, 113-113, 183-183

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/suite/notebookCommands.e2e.test.ts` at line 63, The notebookCommands
e2e test uses hardcoded sleep delays, which should be replaced with named
constants near the top of the module. Update the test cases in
notebookCommands.e2e.test.ts that use sleep(1200), sleep(800), sleep(500), and
sleep(1500) to reference shared timeout/delay constants, following the existing
TREE_LOAD_TIMEOUT and WORKBENCH_TIMEOUT pattern so the timing values are
centralized and easier to maintain.

Source: Coding guidelines

test/e2e/suite/explorerGrouping.e2e.test.ts (3)

98-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Hardcoded sleep durations.

driver.sleep(2000) (Line 104) and driver.sleep(1000) (Line 128) are inline magic numbers; TREE_LOAD_TIMEOUT is already extracted as a constant above, but these aren't.

As per coding guidelines, **/*.{ts,tsx}: "Extract magic numbers (retry counts, delays, timeouts) as named constants near the top of the module."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/suite/explorerGrouping.e2e.test.ts` around lines 98 - 129, The e2e
test in explorerGrouping.e2e.test.ts still uses inline delay values in the
Explorer setup and expand flow, which should be extracted as named constants
like the existing TREE_LOAD_TIMEOUT. Replace the hardcoded wait durations used
after opening the Deepnote view and after expanding the Marketing group with
clearly named top-of-module constants, and reference them from the test body to
keep timing values centralized and maintainable. Keep the logic in the same test
helpers and flow around getExplorerSection, readRows, and the marketing.expand
call unchanged apart from swapping in the constants.

Source: Coding guidelines

---

89-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated fixture-copy loop.

Manually copying additional sibling fixtures via fs.copyFileSync duplicates the same pattern in notebookCommands.e2e.test.ts (Lines 154-156). Consider extending copyFixtureToTempDir (or adding a copyFixturesToTempDir(names[])) to cover multi-file copies once, in fixtures.ts.

copyFixtureToTempDir resolves fixtureName from test/e2e/fixtures, copies it into a temp directory, and returns a cleanup function. Extending it to accept multiple names would remove this repeated boilerplate.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/suite/explorerGrouping.e2e.test.ts` around lines 89 - 96, The test
setup repeats manual sibling-fixture copying, duplicating the pattern used
elsewhere and making the e2e tests harder to maintain. Update the shared fixture
helper in fixtures.ts, specifically copyFixtureToTempDir, to support copying
multiple fixture names at once (or add a copyFixturesToTempDir(names[]) helper)
and use that from explorerGrouping.e2e.test.ts instead of the inline
fs.copyFileSync loop, so the multi-file copy behavior lives in one place.

---

30-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse the shared Deepnote tree helpers test/e2e/suite/explorerGrouping.e2e.test.ts:30-84 still duplicates the same tree-reading helpers already exported from test/e2e/helpers/deepnoteTree.ts. Import those instead of keeping local copies.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/suite/explorerGrouping.e2e.test.ts` around lines 30 - 84, The
Explorer grouping e2e test still duplicates tree helper logic instead of reusing
the shared Deepnote helpers. Replace the local helper implementations in
explorerGrouping.e2e.test.ts, including getExplorerSection and readRows and the
label/description accessors, by importing and using the existing exports from
deepnoteTree.ts so the test relies on one shared source of truth.

Source: Coding guidelines

test/e2e/helpers/deepnoteTree.ts (1)

20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Inline sleep magic numbers bypass the shared constants module.

1200, 800, 500 ms sleeps are hardcoded here while test/e2e/helpers/constants.ts exists precisely to centralize such timeouts/delays.

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

♻️ Proposed fix

+// in constants.ts
+export const EXPLORER_VIEW_OPEN_DELAY = 1_200;
+export const TREE_EXPAND_DELAY = 800;
+export const CONTEXT_MENU_OPEN_DELAY = 500;

-    await VSBrowser.instance.driver.sleep(1200);
+    await VSBrowser.instance.driver.sleep(EXPLORER_VIEW_OPEN_DELAY);
...
-    await VSBrowser.instance.driver.sleep(800);
+    await VSBrowser.instance.driver.sleep(TREE_EXPAND_DELAY);
...
-    await driver.sleep(500);
+    await driver.sleep(CONTEXT_MENU_OPEN_DELAY);

Also applies to: 75-75, 91-91

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/helpers/deepnoteTree.ts` at line 20, Hardcoded sleep delays are
bypassing the shared timeout constants and should be centralized. Update the
sleep calls in deepnoteTree helper methods to use named delay constants from
test/e2e/helpers/constants.ts instead of inline values, and add any missing
delay constants near the top of the module so the waits in the relevant helper
functions stay consistent and easy to tune.

Source: Coding guidelines

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

255-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Dead code: onControllerSelectionChanged body is entirely commented out.

The reselection safety-net logic (notebookKey lookup, deselection check, updateNotebookAffinity) is now fully commented rather than removed. If deliberately disabled, delete it instead of leaving it commented; if it was meant to stay active as a fallback, it's now a no-op.

🧹 Suggested cleanup

-        // const notebookKey = event.notebook.uri.toString();
-
-        // // If the Deepnote controller for this notebook was deselected, try to reselect it
-        // // Since controllers are now protected from disposal, this should rarely happen
-        // if (!event.selected) {
-        //     const ourController = this.notebookControllers.get(notebookKey);
-        //     if (ourController && ourController.id === event.controller.id) {
-        //         logger.warn(
-        //             `Deepnote controller was unexpectedly deselected for ${getDisplayPath(
-        //                 event.notebook.uri
-        //             )}. Reselecting...`
-        //         );
-        //         // Reselect the controller
-        //         ourController.controller.updateNotebookAffinity(event.notebook, NotebookControllerAffinity.Preferred);
-        //     }
-        // }
+        // Controllers are protected from disposal, so no reselection safety-net is needed here anymore.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts` around lines 255 -
287, The onControllerSelectionChanged method in
deepnoteKernelAutoSelector.node.ts contains dead, fully commented-out
reselection logic, so the handler is effectively a no-op beyond logging and
notebookType gating. Remove the commented notebookKey / deselection /
updateNotebookAffinity block from onControllerSelectionChanged if it is
intentionally disabled, or restore it as live code if the fallback is still
required; keep the behavior centered around the existing event,
notebookControllers map, and NotebookControllerAffinity usage.

src/notebooks/deepnote/deepnoteInitNotebookRunner.node.unit.test.ts (2)

31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated magic constant risks drift.

INIT_COMPLETE_DISPLAY_DELAY_MS here is a hand-copied duplicate of the production constant in deepnoteInitNotebookRunner.node.ts. If the production value changes, this test constant won't update and timing assumptions silently go stale.

♻️ Suggested fix: export and import the constant

-const INIT_COMPLETE_DISPLAY_DELAY_MS = 1000;
+// in deepnoteInitNotebookRunner.node.ts: export const INIT_COMPLETE_DISPLAY_DELAY_MS = 1000;
+import { INIT_COMPLETE_DISPLAY_DELAY_MS } from './deepnoteInitNotebookRunner.node';

As per coding guidelines, "Extract duplicate logic into helper methods to prevent drift following DRY principle."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteInitNotebookRunner.node.unit.test.ts` around
lines 31 - 36, The test is duplicating the production timing constant, which can
drift from the real value. Export INIT_COMPLETE_DISPLAY_DELAY_MS from
deepnoteInitNotebookRunner.node.ts and import it into
deepnoteInitNotebookRunner.node.unit.test.ts, then keep RUN_FULLY_SETTLED_MS
derived from that imported symbol so the test always tracks the runner’s actual
delay.

Source: Coding guidelines

---

125-402: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

No test covers the same-kernel overlapping-run race.

Given the concurrency gap flagged in deepnoteInitNotebookRunner.node.ts (double onDidStartKernel fire before the WeakSet is marked, or onDidRestartKernel firing while a start-triggered run is still executing blocks), consider adding a regression test once that fix lands, to lock in the expected single-run/cancel-and-restart behavior.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteInitNotebookRunner.node.unit.test.ts` around
lines 125 - 402, Add a regression test for the same-kernel overlapping-run race
in DeepnoteInitNotebookRunner: cover the case where onDidStartKernel fires twice
before the WeakSet marker is set, and the case where onDidRestartKernel fires
while a start-triggered init is still executing. Use the existing
DeepnoteInitNotebookRunner setup, makeKernel helper, executeHiddenSpy, and the
onDidStartKernel/onDidRestartKernel emitters to assert only one init run happens
in the overlap case and that restart behavior remains correct once the in-flight
run is cancelled or completes.

src/platform/deepnote/deepnoteServerUtils.node.unit.test.ts (1)

16-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Coverage gap vs. stated intent.

Suite docstring claims to pin the "exact format" and "byte-stability" of the handle, but only sibling-uniqueness is tested — no test asserts a fixed expected string or that identical inputs are idempotent.

✅ Suggested additional assertions

     test('two different notebook URIs produce DIFFERENT handles (catches sibling collision)', () => {
         const uriA = Uri.file('/workspace/project/notebook-a.deepnote');
         const uriB = Uri.file('/workspace/project/notebook-b.deepnote');

         assert.notStrictEqual(
             createDeepnoteServerConfigHandle('env-1', uriA),
             createDeepnoteServerConfigHandle('env-1', uriB),
             'sibling notebooks sharing one environment must still get distinct server handles'
         );
     });
+
+    test('same inputs produce the SAME handle (idempotent/byte-stable)', () => {
+        const uri = Uri.file('/workspace/project/notebook-a.deepnote');
+        assert.strictEqual(
+            createDeepnoteServerConfigHandle('env-1', uri),
+            createDeepnoteServerConfigHandle('env-1', uri)
+        );
+    });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/platform/deepnote/deepnoteServerUtils.node.unit.test.ts` around lines 16
- 27, The current DeepnoteServerUtils test only checks that different notebook
URIs produce different handles, but it does not verify the exact handle format
or byte-stability promised by the suite description. Update the
createDeepnoteServerConfigHandle coverage in
deepnoteServerUtils.node.unit.test.ts to add assertions for a fixed expected
handle string for a known env/URI pair and for idempotence by calling
createDeepnoteServerConfigHandle with identical inputs multiple times and
asserting the results are equal.

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

12-22: 🩺 Stability & Availability | 🔵 Trivial

No eviction path for closed notebooks.

originalProjects only grows; nothing removes a (projectId, notebookId) entry when its notebook is closed/deleted. Worth confirming a cleanup hook exists elsewhere (e.g. on notebook close) to bound memory in long sessions with many opened/closed .deepnote siblings.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteNotebookManager.ts` around lines 12 - 22, The
notebook project cache in DeepnoteNotebookManager only ever adds entries to
originalProjects and has no cleanup for closed or deleted notebooks. Add a
removal path for the specific (projectId, notebookId) entry, ideally in the
notebook close/delete lifecycle hook that owns notebook teardown, and make sure
getProjectForNotebook still only reads the exact pair while the new cleanup
keeps the cache bounded.

src/platform/deepnote/deepnoteProjectFileReader.unit.test.ts (1)

65-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use deepStrictEqual for the round-trip check.

Six separate assert.strictEqual calls verify one round-tripped object; a single assert.deepStrictEqual(result.project, original.project) (plus the version check) would be more concise and give a full diff on failure.

As per path instructions, **/*.unit.test.ts: "Use assert.deepStrictEqual() for object comparisons instead of checking individual properties."

♻️ Proposed refactor

-        assert.strictEqual(result.project.id, original.project.id);
-        assert.strictEqual(result.project.name, original.project.name);
-        assert.strictEqual(result.project.notebooks.length, 1);
-        assert.strictEqual(result.project.notebooks[0].id, 'notebook-1');
-        assert.strictEqual(result.project.notebooks[0].name, 'Notebook One');
-        assert.strictEqual(result.version, '1.0.0');
+        assert.deepStrictEqual(result.project, original.project);
+        assert.strictEqual(result.version, original.version);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/platform/deepnote/deepnoteProjectFileReader.unit.test.ts` around lines 65
- 70, The round-trip assertion in the deepnoteProjectFileReader unit test is
comparing the project object property-by-property instead of using a full object
comparison. Update the test in deepnoteProjectFileReader.unit.test.ts to use
assert.deepStrictEqual for result.project versus original.project, and keep the
separate version assertion; this aligns with the test helper guidance and gives
clearer failure diffs.

Source: Path instructions

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

478-497: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Wrap per-folder scanning in try/catch so one bad workspace folder doesn't abort the whole scan.

workspace.findFiles isn't guarded per folder here, unlike the per-candidate read loop below it. A single throwing folder aborts snapshot discovery for every other folder in a multi-root workspace.

As per path instructions, **/*.{ts,tsx}: "Use per-iteration error handling in loops - wrap each iteration in try/catch so one failure doesn't stop the rest."

🔧 Proposed fix

         for (const folder of workspaceFolders) {
             const pattern = new RelativePattern(folder, glob);
-            const files = await workspace.findFiles(pattern, null, maxSnapshotFilesPerFolder);
-
-            for (const uri of files) {
+            let files: Uri[] = [];
+            try {
+                files = await workspace.findFiles(pattern, null, maxSnapshotFilesPerFolder);
+            } catch (error) {
+                logger.warn(`[Snapshot] Failed to scan folder ${folder.uri.toString()} for snapshots`, error);
+                continue;
+            }
+
+            for (const uri of files) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/snapshots/snapshotService.ts` around lines 478 - 497,
The per-folder snapshot scan in snapshotService’s workspace folder loop should
be hardened so one failing folder does not stop discovery for the rest. Wrap the
`workspace.findFiles` call inside the `for (const folder of workspaceFolders)`
loop in a try/catch, keep the existing candidate filtering logic in
`parseSnapshotFilename` and `candidates.push` unchanged, and continue to the
next folder when an error occurs.

Source: Path instructions

---

809-859: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

New private methods break the class's alphabetical ordering.

armSnapshotSave/cancelPendingSnapshotSave/handleNotebookDocumentChange are inserted after waitForPendingCellStateChanges, and compareSnapshotCandidates/extractOutputsFromFile after performSnapshotSave — both break alphabetical order among private methods.

As per path instructions, **/*.{ts,tsx}: "Order method, fields and properties, first by accessibility and then by alphabetical order."

Also applies to: 1006-1046

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/snapshots/snapshotService.ts` around lines 809 - 859,
The new private methods in SnapshotService are out of alphabetical order, so
move armSnapshotSave, cancelPendingSnapshotSave, and
handleNotebookDocumentChange into the correct position among the other private
methods, and likewise place compareSnapshotCandidates and extractOutputsFromFile
in alphabetical order near performSnapshotSave. Keep the existing accessibility
grouping intact and reorder only the method declarations so the class follows
the prescribed method ordering rule.

Source: Path instructions

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

280-284: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer deepStrictEqual over chained strictEqual for object shape checks.

Same pattern as elsewhere in this PR — consolidate into one deepStrictEqual where practical for clearer failure diffs.

As per path instructions, **/*.unit.test.ts: "Use assert.deepStrictEqual() for object comparisons instead of checking individual properties."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteSerializer.unit.test.ts` around lines 280 -
284, The notebook serialization test is checking the parsed notebook shape with
multiple individual asserts instead of a single object comparison. In
deepnoteSerializer.unit.test.ts, replace the chained property checks around
parsed.project.notebooks with one assert.deepStrictEqual() against the expected
notebook object shape, using the existing nbA/nbB and block-b symbols to keep
the intent clear and the failure diff more useful.

Source: Path instructions

src/notebooks/deepnote/snapshots/snapshotFiles.unit.test.ts (1)

124-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use assert.deepStrictEqual instead of individual property checks.

Lines 132-135 check projectId, timestamp, and notebookId one at a time. As per coding guidelines, "Use assert.deepStrictEqual() for object comparisons instead of checking individual properties."

♻️ Proposed fix

             const parsed = parseSnapshotFilename(filename);

-            assert.isDefined(parsed);
-            assert.strictEqual(parsed!.projectId, projectId);
-            assert.strictEqual(parsed!.timestamp, 'latest');
-            assert.isUndefined(parsed!.notebookId);
+            assert.deepStrictEqual(parsed, { slug: 'my-project', projectId, timestamp: 'latest' });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/snapshots/snapshotFiles.unit.test.ts` around lines 124
- 136, The legacy snapshot parsing test is asserting object fields one by one
instead of comparing the parsed result as a whole. Update the test in the
snapshot filename parsing suite to use assert.deepStrictEqual on the parsed
object from parseSnapshotFilename, comparing it against the expected projectId,
timestamp, and notebookId: undefined shape, while still keeping the existing
undefined/null safety check if needed.

Source: Coding guidelines

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e5c5c2ba-e1ea-419a-b234-e01823634219

📥 Commits

Reviewing files that changed from the base of the PR and between 735e5f5 and 5502f34.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (114)

• .gitignore
• .vscode/launch.json
• build/mocha-esm-loader.js
• cspell.json
• package.json
• package.nls.json
• src/commands.ts
• src/kernels/deepnote/deepnoteLspClientManager.node.ts
• src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.ts
• src/kernels/deepnote/deepnoteServerStarter.node.ts
• src/kernels/deepnote/deepnoteServerStarter.unit.test.ts
• src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts
• src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.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/types.ts
• src/notebooks/deepnote/converters/textBlockConverter.ts
• src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts
• src/notebooks/deepnote/deepnoteActivationService.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/deepnoteInitNotebookRunner.node.ts
• src/notebooks/deepnote/deepnoteInitNotebookRunner.node.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/deepnoteMultiNotebookSplitter.ts
• src/notebooks/deepnote/deepnoteMultiNotebookSplitter.unit.test.ts
• src/notebooks/deepnote/deepnoteNotebookFileFactory.ts
• src/notebooks/deepnote/deepnoteNotebookFileFactory.unit.test.ts
• src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.ts
• src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.unit.test.ts
• src/notebooks/deepnote/deepnoteNotebookManager.ts
• src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts
• src/notebooks/deepnote/deepnoteProjectUtils.ts
• src/notebooks/deepnote/deepnoteSerializer.ts
• src/notebooks/deepnote/deepnoteSerializer.unit.test.ts
• src/notebooks/deepnote/deepnoteSiblingFileAllocator.ts
• src/notebooks/deepnote/deepnoteSiblingFileAllocator.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/integrations/federatedAuth/federatedAuthKernelRestartBridge.node.ts
• src/notebooks/deepnote/integrations/federatedAuth/federatedAuthKernelRestartBridge.node.unit.test.ts
• src/notebooks/deepnote/integrations/integrationDetector.ts
• src/notebooks/deepnote/integrations/integrationManager.ts
• src/notebooks/deepnote/integrations/integrationWebview.ts
• src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts
• src/notebooks/deepnote/integrations/types.ts
• src/notebooks/deepnote/snapshots/snapshotFiles.ts
• src/notebooks/deepnote/snapshots/snapshotFiles.unit.test.ts
• src/notebooks/deepnote/snapshots/snapshotService.ts
• src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts
• src/notebooks/deepnote/sqlCellStatusBarProvider.ts
• src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
• src/notebooks/serviceRegistry.node.ts
• src/notebooks/serviceRegistry.web.ts
• src/notebooks/types.ts
• src/platform/common/constants.ts
• src/platform/deepnote/deepnoteProjectFileReader.ts
• src/platform/deepnote/deepnoteProjectFileReader.unit.test.ts
• src/platform/deepnote/deepnoteProjectIdResolver.ts
• src/platform/deepnote/deepnoteProjectIdResolver.unit.test.ts
• src/platform/deepnote/deepnoteServerUtils.node.ts
• src/platform/deepnote/deepnoteServerUtils.node.unit.test.ts
• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
• src/platform/notebooks/deepnote/types.ts
• src/test/unittests.ts
• src/webviews/webview-side/integrations/serviceAccountValidation.ts
• test/e2e/fixtures/bootstrap-only.deepnote
• test/e2e/fixtures/etl-pipeline-extract.deepnote
• test/e2e/fixtures/etl-pipeline-init.deepnote
• test/e2e/fixtures/etl-pipeline.deepnote
• test/e2e/fixtures/legacy-snapshot-demo.deepnote
• test/e2e/fixtures/marketing-campaigns.deepnote
• test/e2e/fixtures/marketing-metrics.deepnote
• test/e2e/fixtures/marketing-overview.deepnote
• test/e2e/fixtures/quick-notes.deepnote
• test/e2e/fixtures/sales-analytics-revenue.deepnote
• test/e2e/fixtures/sales-analytics.deepnote
• test/e2e/fixtures/snapshots/legacy-snapshot-demo_ffffffff-ffff-4fff-8fff-ffffffffffff_latest.snapshot.deepnote
• test/e2e/helpers/constants.ts
• test/e2e/helpers/deepnoteEnvironment.ts
• test/e2e/helpers/deepnoteTree.ts
• test/e2e/helpers/fixtures.ts
• test/e2e/helpers/index.ts
• test/e2e/helpers/modals.ts
• test/e2e/helpers/notebook.ts
• test/e2e/helpers/notifications.ts
• test/e2e/helpers/quickInput.ts
• test/e2e/helpers/screenshots.ts
• test/e2e/helpers/workspace.ts
• test/e2e/settings.json
• test/e2e/suite/environment.e2e.test.ts
• test/e2e/suite/explorerGrouping.e2e.test.ts
• test/e2e/suite/fileWatcher.e2e.test.ts
• test/e2e/suite/helloWorld.e2e.test.ts
• test/e2e/suite/initNotebookRunner.e2e.test.ts
• test/e2e/suite/initOnlyFile.e2e.test.ts
• test/e2e/suite/integrations.e2e.test.ts
• test/e2e/suite/notebookCommands.e2e.test.ts
• test/e2e/suite/openSingleNotebook.e2e.test.ts
• test/e2e/suite/projectRename.e2e.test.ts
• test/e2e/suite/snapshots.e2e.test.ts
• test/e2e/suite/splitInitNotebook.e2e.test.ts
• test/e2e/suite/splitMultiNotebook.e2e.test.ts
• test/e2e/suite/splitSafety.e2e.test.ts
• test/e2e/suite/statusBar.e2e.test.ts

💤 Files with no reviewable changes (2)

• src/commands.ts
• src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts

[coderabbitai]

Actionable comments posted: 1

Caution

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

⚠️ Outside diff range comments (2)

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

668-691: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Duplicate fallback-message template.

The identical "Active notebook: … in project …" showInformationMessage call is repeated verbatim in the not-found branch and the catch block.

♻️ Proposed fix

+        const showFallbackMessage = () =>
+            window.showInformationMessage(
+                `Active notebook: ${notebookMetadata?.deepnoteNotebookName || 'Untitled'} in project ${
+                    notebookMetadata?.deepnoteProjectName || 'Untitled'
+                }`
+            );
+
         try {
             const treeItem = await this.treeDataProvider.findTreeItem(projectId, notebookId);
 
             if (treeItem) {
                 await this.treeView.reveal(treeItem, { select: true, focus: true, expand: true });
             } else {
-                // Fall back to showing information if node not found
-                await window.showInformationMessage(
-                    `Active notebook: ${notebookMetadata?.deepnoteNotebookName || 'Untitled'} in project ${
-                        notebookMetadata?.deepnoteProjectName || 'Untitled'
-                    }`
-                );
+                await showFallbackMessage();
             }
         } catch (error) {
-            // Fall back to showing information if reveal fails
             this.logger.error('Failed to reveal notebook in explorer', error);
-            await window.showInformationMessage(
-                `Active notebook: ${notebookMetadata?.deepnoteNotebookName || 'Untitled'} in project ${
-                    notebookMetadata?.deepnoteProjectName || 'Untitled'
-                }`
-            );
+            await showFallbackMessage();
         }

As per coding guidelines: "Extract duplicate logic into helper methods to prevent drift following DRY principle."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 668 - 691, The
Active notebook fallback message is duplicated in both the not-found branch and
the catch block, so extract that repeated `window.showInformationMessage` logic
into a small helper in `DeepnoteExplorerView` and call it from both places. Keep
the existing message text and metadata formatting the same, but centralize it so
the `treeDataProvider.findTreeItem` and `treeView.reveal` flow stays DRY and
avoids drift.

Source: Coding guidelines

---

955-981: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Deepnote-file import loop lacks per-iteration error handling.

convertJupyterUrisToDeepnoteFiles (Line 854) already isolates per-item failures so one bad conversion doesn't stop the rest, but the sibling "Import deepnote files" loop at Line 956 has no try/catch: a single failed readFile/writeFile throws, aborting remaining .deepnote copies and skipping the Jupyter conversion step entirely, with only a generic "Failed to import notebook" error surfacing — silently dropping any progress already made.

🐛 Proposed fix

-            // Import deepnote files
-            for (const deepnoteUri of deepnoteUris) {
-                const fileName = deepnoteUri.path.split('/').pop() || 'imported.deepnote';
-                const targetUri = Uri.joinPath(workspaceFolder.uri, fileName);
-
-                const content = await workspace.fs.readFile(deepnoteUri);
-
-                await workspace.fs.writeFile(targetUri, content);
-            }
+            // Import deepnote files (per-iteration error handling so one failure doesn't stop the rest)
+            const failedDeepnoteNames: string[] = [];
+
+            for (const deepnoteUri of deepnoteUris) {
+                const fileName = deepnoteUri.path.split('/').pop() || 'imported.deepnote';
+                const targetUri = Uri.joinPath(workspaceFolder.uri, fileName);
+
+                try {
+                    const content = await workspace.fs.readFile(deepnoteUri);
+                    await workspace.fs.writeFile(targetUri, content);
+                } catch (error) {
+                    failedDeepnoteNames.push(fileName);
+                    this.logger.error(`Failed to import Deepnote file ${fileName}`, error);
+                }
+            }
 
-            const failedCount = await this.convertJupyterUrisToDeepnoteFiles(jupyterUris, workspaceFolder.uri);
+            const failedJupyterCount = await this.convertJupyterUrisToDeepnoteFiles(jupyterUris, workspaceFolder.uri);
+            const failedCount = failedJupyterCount + failedDeepnoteNames.length;

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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 955 - 981, The
“Import deepnote files” loop in deepnoteExplorerView should use per-iteration
error handling so one failed read/write does not abort the rest of the imports
or skip the subsequent Jupyter conversion step. Wrap each iteration around the
workspace.fs.readFile/workspace.fs.writeFile flow in a try/catch, mirror the
isolation already used by convertJupyterUrisToDeepnoteFiles, and continue
processing remaining deepnoteUris while preserving any successful copies and
reporting failures appropriately.

Source: Coding guidelines

🧹 Nitpick comments (1)

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

444-484: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Sequential per-file reads in a workspace-wide scan.

collectNotebookNamesForProject runs on every rename/duplicate/create/add-notebook action and reads every sibling .deepnote file one at a time with await inside the loop. Parallelizing with Promise.all would cut latency in workspaces with many project files.

⚡ Proposed fix

-            for (const fileUri of files) {
-                // Skip snapshot sidecars: they are full project clones whose stale notebook names
-                // would otherwise pollute the uniqueness set.
-                if (isSnapshotFile(fileUri)) {
-                    continue;
-                }
-
-                try {
-                    const projectData = await readDeepnoteProjectFile(fileUri);
-
-                    if (projectData?.project?.id !== projectId) {
-                        continue;
-                    }
-
-                    for (const notebook of getNonInitNotebooks(projectData)) {
-                        if (notebook.name && notebook.name !== excludeName) {
-                            names.add(notebook.name);
-                        }
-                    }
-                } catch (error) {
-                    this.logger.error(`Failed to read ${fileUri.path} for name collection`, error);
-                }
-            }
+            await Promise.all(
+                files.map(async (fileUri) => {
+                    // Skip snapshot sidecars: they are full project clones whose stale notebook names
+                    // would otherwise pollute the uniqueness set.
+                    if (isSnapshotFile(fileUri)) {
+                        return;
+                    }
+
+                    try {
+                        const projectData = await readDeepnoteProjectFile(fileUri);
+
+                        if (projectData?.project?.id !== projectId) {
+                            return;
+                        }
+
+                        for (const notebook of getNonInitNotebooks(projectData)) {
+                            if (notebook.name && notebook.name !== excludeName) {
+                                names.add(notebook.name);
+                            }
+                        }
+                    } catch (error) {
+                        this.logger.error(`Failed to read ${fileUri.path} for name collection`, error);
+                    }
+                })
+            );

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/deepnoteExplorerView.ts` around lines 444 - 484,
`collectNotebookNamesForProject` is doing workspace-wide `.deepnote` file reads
sequentially inside the `for` loop, which makes rename/duplicate/create flows
slow. Update the file-processing logic in `deepnoteExplorerView` to read and
inspect sibling files in parallel using `Promise.all` (or equivalent
concurrency) while preserving the existing `isSnapshotFile`,
`projectData.project.id`, and `excludeName` filtering, and keep per-file error
handling/logging intact for `readDeepnoteProjectFile`.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/e2e/suite/initOnlyFile.e2e.test.ts`:
- Line 156: The deleted-toast assertion is hardcoded to “Bootstrap” and can
drift from the rest of the test if the fixture name changes. Update the
notification check in the `waitForNotification` call within the e2e test to use
`NOTEBOOK_NAME` so it stays aligned with the notebook name used elsewhere in
`initOnlyFile.e2e.test.ts`.

---

Outside diff comments:
In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 668-691: The Active notebook fallback message is duplicated in
both the not-found branch and the catch block, so extract that repeated
`window.showInformationMessage` logic into a small helper in
`DeepnoteExplorerView` and call it from both places. Keep the existing message
text and metadata formatting the same, but centralize it so the
`treeDataProvider.findTreeItem` and `treeView.reveal` flow stays DRY and avoids
drift.
- Around line 955-981: The “Import deepnote files” loop in deepnoteExplorerView
should use per-iteration error handling so one failed read/write does not abort
the rest of the imports or skip the subsequent Jupyter conversion step. Wrap
each iteration around the workspace.fs.readFile/workspace.fs.writeFile flow in a
try/catch, mirror the isolation already used by
convertJupyterUrisToDeepnoteFiles, and continue processing remaining
deepnoteUris while preserving any successful copies and reporting failures
appropriately.

---

Nitpick comments:
In `@src/notebooks/deepnote/deepnoteExplorerView.ts`:
- Around line 444-484: `collectNotebookNamesForProject` is doing workspace-wide
`.deepnote` file reads sequentially inside the `for` loop, which makes
rename/duplicate/create flows slow. Update the file-processing logic in
`deepnoteExplorerView` to read and inspect sibling files in parallel using
`Promise.all` (or equivalent concurrency) while preserving the existing
`isSnapshotFile`, `projectData.project.id`, and `excludeName` filtering, and
keep per-file error handling/logging intact for `readDeepnoteProjectFile`.

🪄 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: CHILL

Plan: Pro

Run ID: aae9a710-ee6e-4b3a-92de-fb8bf05951a9

📥 Commits

Reviewing files that changed from the base of the PR and between 2bd8f3e and 36fc854.

📒 Files selected for processing (7)

• 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/deepnoteTreeDataProvider.ts
• test/e2e/suite/initOnlyFile.e2e.test.ts
• test/e2e/suite/revealInExplorer.e2e.test.ts

🚧 Files skipped from review as they are similar to previous changes (3)

• src/notebooks/deepnote/deepnoteTreeDataProvider.ts
• src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts
• src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts`:
- Around line 1579-1580: The mock in snapshotService.unit.test.ts is too broad
because getProjectForNotebook is stubbed with anything(), anything(), which
won’t catch incorrect project/notebook lookups from performSnapshotSave().
Update the mock to use the exact expected notebook pair in both affected test
cases so the test verifies the notebook-scoped contract through
IDeepnoteNotebookManager.getProjectForNotebook and fails if the wrong
identifiers are passed.

🪄 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: CHILL

Plan: Pro

Run ID: e583365e-31bc-4645-9d19-3e2ebbb578a2

📥 Commits

Reviewing files that changed from the base of the PR and between 14769f9 and 5c74813.

📒 Files selected for processing (10)

• src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
• src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts
• src/notebooks/deepnote/deepnoteInitNotebookRunner.node.unit.test.ts
• src/notebooks/deepnote/deepnoteMultiNotebookSplitter.unit.test.ts
• src/notebooks/deepnote/deepnoteNotebookFileFactory.unit.test.ts
• src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.unit.test.ts
• src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
• src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts
• src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts
• src/platform/deepnote/deepnoteProjectIdResolver.unit.test.ts

🚧 Files skipped from review as they are similar to previous changes (8)

• src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts
• src/notebooks/deepnote/deepnoteNotebookFileFactory.unit.test.ts
• src/notebooks/deepnote/deepnoteNotebookInfoStatusBar.unit.test.ts
• src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
• src/notebooks/deepnote/deepnoteInitNotebookRunner.node.unit.test.ts
• src/platform/deepnote/deepnoteProjectIdResolver.unit.test.ts
• src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
• src/notebooks/deepnote/deepnoteMultiNotebookSplitter.unit.test.ts

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Constrain this mock to the exact notebook pair.

anything(), anything() lets the test pass even if performSnapshotSave() asks the manager for the wrong (projectId, notebookId). Stub the expected pair so this covers the notebook-scoped contract.

Proposed fix

-                when(mockNotebookManager.getProjectForNotebook(anything(), anything())).thenReturn(originalProject);
+                when(mockNotebookManager.getProjectForNotebook(projectId, notebookId)).thenReturn(originalProject);

-                when(mockNotebookManager.getProjectForNotebook(anything(), anything())).thenReturn(originalProject);
+                when(mockNotebookManager.getProjectForNotebook(sharedProjectId, 'notebook-b')).thenReturn(originalProject);

Also applies to: 1662-1663

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts` around lines
1579 - 1580, The mock in snapshotService.unit.test.ts is too broad because
getProjectForNotebook is stubbed with anything(), anything(), which won’t catch
incorrect project/notebook lookups from performSnapshotSave(). Update the mock
to use the exact expected notebook pair in both affected test cases so the test
verifies the notebook-scoped contract through
IDeepnoteNotebookManager.getProjectForNotebook and fails if the wrong
identifiers are passed.

[coderabbitai]

🧹 Nitpick comments (1)

src/notebooks/deepnote/snapshots/executionMetadataTracker.unit.test.ts (1)

58-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use assert.deepStrictEqual() instead of checking individual summary properties.

Several tests (e.g. Lines 60-63, 106-109) assert multiple properties of the same metadata.summary/metadata object one-by-one. As per coding guidelines, "Use assert.deepStrictEqual() for object comparisons instead of checking individual properties."

♻️ Example for Lines 58-64

             const metadata = tracker.getExecutionMetadata(notebookUri);
             assert.isDefined(metadata);
-            assert.strictEqual(metadata!.summary!.blocksExecuted, 2);
-            assert.strictEqual(metadata!.summary!.blocksSucceeded, 1);
-            assert.strictEqual(metadata!.summary!.blocksFailed, 1);
+            assert.deepStrictEqual(metadata!.summary, {
+                blocksExecuted: 2,
+                blocksFailed: 1,
+                blocksSucceeded: 1,
+                totalDurationMs: metadata!.summary!.totalDurationMs
+            });
             assert.strictEqual(metadata!.error!.name, 'ValueError');

Also applies to: 98-110

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/notebooks/deepnote/snapshots/executionMetadataTracker.unit.test.ts`
around lines 58 - 64, The tests around execution metadata are asserting object
fields one-by-one instead of comparing the full expected structure. Update the
affected assertions in executionMetadataTracker.unit.test.ts to use
assert.deepStrictEqual() for the metadata.summary and metadata objects, covering
the cases near the existing getExecutionMetadata() checks so the expectations
are expressed as whole-object comparisons rather than individual properties.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/notebooks/deepnote/snapshots/executionMetadataTracker.unit.test.ts`:
- Around line 58-64: The tests around execution metadata are asserting object
fields one-by-one instead of comparing the full expected structure. Update the
affected assertions in executionMetadataTracker.unit.test.ts to use
assert.deepStrictEqual() for the metadata.summary and metadata objects, covering
the cases near the existing getExecutionMetadata() checks so the expectations
are expressed as whole-object comparisons rather than individual properties.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 10e2024d-070e-42df-99d6-8002fab02ee3

📥 Commits

Reviewing files that changed from the base of the PR and between 5c74813 and ea42807.

📒 Files selected for processing (16)

• src/kernels/deepnote/deepnoteServerHandleRegistry.node.ts
• src/kernels/deepnote/deepnoteServerHandleRegistry.unit.test.ts
• src/kernels/deepnote/deepnoteServerStarter.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/deepnoteTreeDataProvider.unit.test.ts
• src/notebooks/deepnote/deepnoteTreeItem.ts
• src/notebooks/deepnote/snapshots/executionMetadataTracker.ts
• src/notebooks/deepnote/snapshots/executionMetadataTracker.unit.test.ts
• src/notebooks/deepnote/snapshots/snapshotFiles.ts
• src/notebooks/deepnote/snapshots/snapshotService.ts
• src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts
• src/notebooks/serviceRegistry.node.ts

🚧 Files skipped from review as they are similar to previous changes (7)

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