feat(deepnote-file): Migration to single notebook deepnote file#381
Draft
feat(deepnote-file): Migration to single notebook deepnote file#381
Conversation
…mtime conflicts during workspace saves
…out block IDs and implement a valid project structure for testing
…ration result and log warnings for undefined saves
…ngeWatcher to ensure proper handling of snapshot reads
… deserialization logic - Introduced `clearNotebookSelection` method in `DeepnoteNotebookManager` to reset notebook selection for a project. - Updated `DeepnoteFileChangeWatcher` to call `clearNotebookSelection` during file change events, ensuring the active editor is prioritized during re-deserialization. - Modified `deserializeNotebook` method in `DeepnoteNotebookSerializer` to accept an optional `notebookId` parameter, preventing race conditions when multiple notebooks from the same project are open.
…gement - Enhanced `createMockChildProcess` to provide a more comprehensive mock implementation for testing. - Updated `DeepnoteServerStarter` to ensure proper disposal of existing disposables when monitoring server output, improving resource management. - Adjusted error handling in server startup to streamline diagnostics and output tracking.
- Introduced a new `applyNotebookEdits` method in `DeepnoteFileChangeWatcher` to centralize the application of notebook edits, improving code readability and maintainability. - Updated existing calls to `workspace.applyEdit` to utilize the new method, reducing redundancy in the codebase. - Adjusted unit tests to reflect changes in the edit application process, ensuring consistent behavior across the application.
…ling - Introduced `queueNotebookResolution` and `consumePendingNotebookResolution` methods in `DeepnoteNotebookManager` to manage transient notebook resolutions, improving the handling of notebook selections during file operations. - Updated `DeepnoteNotebookSerializer` to prioritize queued notebook resolutions, ensuring more reliable notebook ID retrieval during deserialization. - Refactored `DeepnoteExplorerView` to utilize a new `registerNotebookOpenIntent` method, streamlining the process of selecting and opening notebooks. - Improved error handling in `DeepnoteServerStarter` to log warnings when disposing listeners fails, enhancing diagnostics during server operations. - Adjusted unit tests to cover new functionality and ensure consistent behavior across notebook management processes.
…n DeepnoteFileChangeWatcher
…pnoteNotebookManager - Added new unit tests for self-write detection in `DeepnoteFileChangeWatcher`, ensuring that saving a deepnote notebook suppresses subsequent file change events. - Implemented tests in `DeepnoteNotebookManager` to verify project data updates without altering the current notebook ID, ensuring data integrity during updates. - Introduced a utility function for creating notebook documents in tests, improving test setup consistency. - Expanded multi-notebook save scenarios in `DeepnoteNotebookSerializer` to validate notebook ID resolution during serialization and deserialization processes.
…Watcher - Introduced new unit tests to validate snapshot changes and deserialization interactions in `DeepnoteFileChangeWatcher`. - Enhanced test setup to capture interactions with notebook edits, ensuring accurate application of changes across multi-notebook projects. - Improved organization of imports and added missing type definitions for better clarity and maintainability.
…pnoteFileChangeWatcher - Updated `applyNotebookEdits` calls to handle failures in restoring block IDs, logging warnings when restoration fails after execution API and replaceCells operations. - Added unit tests to verify that saving does not occur when metadata restoration fails, ensuring data integrity during notebook edits. - Improved test coverage for scenarios involving metadata restoration failures, enhancing the reliability of the DeepnoteFileChangeWatcher functionality.
…book details in the status bar - Introduced `DeepnoteNotebookInfoStatusBar` to show the active Deepnote notebook name and provide a copy action for notebook details. - Updated service registration in both `serviceRegistry.node.ts` and `serviceRegistry.web.ts` to include the new status bar service. - Added a new command `CopyNotebookDetails` to facilitate copying notebook information to the clipboard.
- Introduced a new command `deepnote.copyNotebookDetails` to allow users to copy details of the active Deepnote notebook. - Updated localization file to include the title for the new command, enhancing user experience with clear labeling.
- Added support for resolving notebook IDs from open tabs, improving the handling of notebook selections during reloads. - Introduced a new method `findNotebookIdsFromTabs` to extract notebook IDs from the current tab groups. - Updated `findCurrentNotebookId` to prioritize tab-based resolution when available, alongside existing resolution strategies. - Enhanced unit tests to cover various scenarios for tab-based resolution, ensuring robust functionality.
…atch resolution - Removed the `selectNotebookForProject` and `clearNotebookSelection` methods from `DeepnoteNotebookManager` to simplify notebook selection logic. - Introduced a new mechanism in `DeepnoteActivationService` to check and fix notebook ID mismatches when opening documents, improving the reliability of notebook loading. - Updated `DeepnoteSerializer` to prioritize current notebook IDs and handle active tab resolutions more effectively. - Enhanced unit tests to cover the new mismatch resolution logic and ensure proper functionality across various scenarios.
…tebook ID resolution - Removed unused variables and methods related to mismatch checking in `DeepnoteActivationService`, streamlining the activation process. - Updated `findCurrentNotebookId` in `DeepnoteSerializer` to improve notebook ID resolution logic, prioritizing active tab detection. - Adjusted unit tests to reflect changes in notebook ID resolution, ensuring accurate behavior when no pending resolutions exist.
…unused logic - Eliminated unnecessary variables and methods related to recent serialization tracking in `DeepnoteNotebookSerializer`, simplifying the notebook ID resolution process. - Updated the logic for finding the current notebook ID to focus on active tab detection, enhancing overall efficiency. - Removed outdated unit tests that were no longer relevant to the current implementation, ensuring test suite accuracy.
…oteNotebookManager - Introduced debug logging in `DeepnoteNotebookManager` to track valid pending notebook resolutions, enhancing traceability during resolution processes. - Simplified block iteration in `DeepnoteNotebookSerializer` by removing unnecessary null checks, improving code clarity and performance.
- Added a new event listener in `DeepnoteActivationService` to verify deserialized notebooks upon opening. - Improved `DeepnoteNotebookSerializer` to handle cases where no notebook ID is resolved, returning an empty state instead of throwing an error. - Updated unit tests to reflect changes in notebook deserialization behavior, ensuring accurate handling of scenarios with no available notebooks.
Contributor
📝 WalkthroughWalkthroughThis PR implements automated multi-notebook Deepnote file splitting and refactors notebook selection tracking. A new Sequence DiagramsequenceDiagram
participant User
participant Editor as VS Code Editor
participant ActivationService as DeepnoteActivationService
participant AutoSplitter as DeepnoteAutoSplitter
participant FileWatcher as FileChangeWatcher
participant SnapshotService
participant FileSystem
participant ExplorerView
User->>Editor: Opens multi-notebook.deepnote
Editor->>ActivationService: onDidOpenNotebookDocument event
ActivationService->>ActivationService: Filter notebookType === 'deepnote'
ActivationService->>ActivationService: Deserialize file from disk
ActivationService->>AutoSplitter: splitIfNeeded(fileUri, deepnoteFile)
AutoSplitter->>AutoSplitter: Count non-init notebooks
alt Multiple notebooks detected
AutoSplitter->>AutoSplitter: Keep first in original, create new files for rest
AutoSplitter->>FileSystem: Write new .deepnote files
AutoSplitter->>SnapshotService: Split snapshot files by notebookId
SnapshotService->>FileSystem: Create per-notebook snapshots
SnapshotService->>FileSystem: Delete old combined snapshot
AutoSplitter->>FileSystem: Update original file (keep first notebook)
AutoSplitter->>ActivationService: Return {wasSplit: true, newFiles}
ActivationService->>ExplorerView: refreshTree()
ExplorerView->>ExplorerView: Reload and group by projectId
else Single or no non-init notebooks
AutoSplitter->>ActivationService: Return {wasSplit: false}
end
ActivationService->>FileWatcher: Notebook loaded, ready for edits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Multiple interacting systems with non-trivial logic: file-splitting algorithm with snapshot management, tree structure reorganization from flat to grouped layout, snapshot filename format changes, extensive manager API refactoring across 6+ files, file watcher control-flow changes, and corresponding test rewrites spanning 1300+ lines. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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 | 🟠 MajorFilter 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 | 🟡 MinorRename 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
notebookIdis extracted, then discarded.
handleSnapshotFileChange()derives anotebookId, and this method filters editors with it, but the queued operation only keepsprojectId. 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 | 🟠 MajorNotebook-scoped reads dropped backward compatibility.
When
notebookIdis 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
📒 Files selected for processing (28)
build/mocha-esm-loader.jspackage.jsonpackage.nls.jsonsrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/deepnote/deepnoteTestHelpers.node.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteAutoSplitter.tssrc/notebooks/deepnote/deepnoteExplorerView.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.tssrc/notebooks/deepnote/deepnoteFileChangeWatcher.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookInfoStatusBar.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/snapshots/snapshotFiles.tssrc/notebooks/deepnote/snapshots/snapshotService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/serviceRegistry.web.tssrc/notebooks/types.tssrc/platform/common/constants.tssrc/test/mocks/vsc/extHostedTypes.tssrc/test/vscode-mock.ts
Comment on lines
+405
to
+414
| const existing = this.disposablesByFile.get(fileKey); | ||
| if (existing) { | ||
| for (const d of existing) { | ||
| try { | ||
| d.dispose(); | ||
| } catch (ex) { | ||
| logger.warn(`Error disposing listener for ${fileKey}`, ex); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract listener-disposal into one helper.
This block duplicates disposal behavior already present elsewhere in the class. Centralize it (e.g., disposeOutputListeners(fileKey)) and reuse from monitorServerOutput, stopServerForEnvironment, and dispose() to avoid drift.
As per coding guidelines, "Extract duplicate logic into helper methods to prevent drift following DRY principle".
🤖 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 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.
Comment on lines
+15
to
+66
| 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, |
Contributor
There was a problem hiding this comment.
🧹 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
doneRepository: 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.
Comment on lines
+59
to
+63
| this.extensionContext.subscriptions.push( | ||
| workspace.onDidOpenNotebookDocument((doc) => { | ||
| void this.checkAndSplitIfNeeded(doc); | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
Handle notebooks that are already open when activation runs.
This listener only covers future opens. Restored/open .deepnote notebooks that exist before activate() finishes will never hit checkAndSplitIfNeeded(), so multi-notebook files can stay unsplit until the user closes and reopens them.
Possible fix
this.extensionContext.subscriptions.push(
workspace.onDidOpenNotebookDocument((doc) => {
void this.checkAndSplitIfNeeded(doc);
})
);
+
+ for (const doc of workspace.notebookDocuments) {
+ void this.checkAndSplitIfNeeded(doc);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.extensionContext.subscriptions.push( | |
| workspace.onDidOpenNotebookDocument((doc) => { | |
| void this.checkAndSplitIfNeeded(doc); | |
| }) | |
| ); | |
| this.extensionContext.subscriptions.push( | |
| workspace.onDidOpenNotebookDocument((doc) => { | |
| void this.checkAndSplitIfNeeded(doc); | |
| }) | |
| ); | |
| for (const doc of workspace.notebookDocuments) { | |
| void this.checkAndSplitIfNeeded(doc); | |
| } |
🤖 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 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.
Comment on lines
+46
to
+49
| for (const notebook of extraNotebooks) { | ||
| const notebookSlug = this.slugifyNotebookName(notebook.name); | ||
| const newFileName = `${originalStem}_${notebookSlug}.deepnote`; | ||
| const newFileUri = Uri.joinPath(parentDir, newFileName); |
Contributor
There was a problem hiding this comment.
Make split filenames collision-proof.
notebookSlug is not unique. Two notebooks with the same slug — or any names that hit the 'notebook' fallback — will write the same path and silently overwrite each other during migration.
Proposed fix
- const newFileName = `${originalStem}_${notebookSlug}.deepnote`;
+ const newFileName = `${originalStem}_${notebookSlug}_${notebook.id}.deepnote`;Also applies to: 283-289
🤖 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 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.
Comment on lines
+120
to
+290
| private async splitSnapshots( | ||
| _projectFileUri: Uri, | ||
| projectId: string, | ||
| projectName: string, | ||
| primaryNotebookId: string, | ||
| extraNotebookIds: string[] | ||
| ): Promise<void> { | ||
| const workspaceFolders = workspace.workspaceFolders; | ||
|
|
||
| if (!workspaceFolders || workspaceFolders.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const snapshotGlob = `**/snapshots/*_${projectId}_*.snapshot.deepnote`; | ||
| let allSnapshotFiles: Uri[] = []; | ||
|
|
||
| for (const folder of workspaceFolders) { | ||
| const pattern = new RelativePattern(folder, snapshotGlob); | ||
| const files = await workspace.findFiles(pattern, null, 100); | ||
| allSnapshotFiles = allSnapshotFiles.concat(files); | ||
| } | ||
|
|
||
| if (allSnapshotFiles.length === 0) { | ||
| logger.debug(`[AutoSplitter] No snapshots found for project ${projectId}`); | ||
| return; | ||
| } | ||
|
|
||
| let slug: string; | ||
|
|
||
| try { | ||
| slug = slugifyProjectName(projectName); | ||
| } catch { | ||
| logger.warn(`[AutoSplitter] Cannot slugify project name, skipping snapshot split`); | ||
| return; | ||
| } | ||
|
|
||
| const allNotebookIds = [primaryNotebookId, ...extraNotebookIds]; | ||
|
|
||
| for (const snapshotUri of allSnapshotFiles) { | ||
| try { | ||
| await this.splitSingleSnapshot(snapshotUri, slug, projectId, allNotebookIds); | ||
| } catch (error) { | ||
| logger.warn(`[AutoSplitter] Failed to split snapshot ${snapshotUri.path}`, error); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private async splitSingleSnapshot( | ||
| snapshotUri: Uri, | ||
| slug: string, | ||
| projectId: string, | ||
| notebookIds: string[] | ||
| ): Promise<void> { | ||
| const content = await workspace.fs.readFile(snapshotUri); | ||
| const { deserializeDeepnoteFile: parseFile } = await import('@deepnote/blocks'); | ||
| const snapshotData = parseFile(new TextDecoder().decode(content)); | ||
|
|
||
| const snapshotDir = Uri.joinPath(snapshotUri, '..'); | ||
|
|
||
| // Extract variant from existing filename | ||
| const basename = snapshotUri.path.split('/').pop() ?? ''; | ||
| const variant = this.extractVariantFromSnapshotFilename(basename, projectId); | ||
|
|
||
| if (!variant) { | ||
| logger.debug(`[AutoSplitter] Could not extract variant from ${basename}, skipping`); | ||
| return; | ||
| } | ||
|
|
||
| // Create per-notebook snapshot files | ||
| for (const notebookId of notebookIds) { | ||
| const notebookData = structuredClone(snapshotData); | ||
|
|
||
| // Keep only this notebook (and init) | ||
| notebookData.project.notebooks = notebookData.project.notebooks.filter( | ||
| (nb) => | ||
| nb.id === notebookId || | ||
| (notebookData.project.initNotebookId && nb.id === notebookData.project.initNotebookId) | ||
| ); | ||
|
|
||
| // Recompute hash | ||
| if (notebookData.metadata) { | ||
| (notebookData.metadata as Record<string, unknown>).snapshotHash = await this.computeSnapshotHash( | ||
| notebookData | ||
| ); | ||
| } | ||
|
|
||
| const newFilename = `${slug}_${projectId}_${notebookId}_${variant}.snapshot.deepnote`; | ||
| const newUri = Uri.joinPath(snapshotDir, newFilename); | ||
| const yaml = serializeDeepnoteFile(notebookData); | ||
|
|
||
| await workspace.fs.writeFile(newUri, new TextEncoder().encode(yaml)); | ||
|
|
||
| logger.debug(`[AutoSplitter] Created notebook snapshot: ${newFilename}`); | ||
| } | ||
|
|
||
| // Delete the old snapshot file (it's been replaced by per-notebook files) | ||
| try { | ||
| await workspace.fs.delete(snapshotUri); | ||
| logger.debug(`[AutoSplitter] Deleted old snapshot: ${basename}`); | ||
| } catch { | ||
| logger.warn(`[AutoSplitter] Failed to delete old snapshot: ${basename}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Extracts the variant portion from a snapshot filename. | ||
| * Old format: {slug}_{projectId}_{variant}.snapshot.deepnote | ||
| */ | ||
| private extractVariantFromSnapshotFilename(basename: string, projectId: string): string | undefined { | ||
| const suffix = '.snapshot.deepnote'; | ||
|
|
||
| if (!basename.endsWith(suffix)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const withoutSuffix = basename.slice(0, -suffix.length); | ||
| const projectIdIndex = withoutSuffix.indexOf(`_${projectId}_`); | ||
|
|
||
| if (projectIdIndex === -1) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return withoutSuffix.slice(projectIdIndex + 1 + projectId.length + 1); | ||
| } | ||
|
|
||
| /** | ||
| * Computes snapshotHash using the same algorithm as DeepnoteNotebookSerializer. | ||
| */ | ||
| private async computeSnapshotHash(project: DeepnoteFile): Promise<string> { | ||
| const contentHashes: string[] = []; | ||
|
|
||
| for (const notebook of project.project.notebooks) { | ||
| for (const block of notebook.blocks ?? []) { | ||
| if (block.contentHash) { | ||
| contentHashes.push(block.contentHash); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| contentHashes.sort(); | ||
|
|
||
| const hashInput = { | ||
| contentHashes, | ||
| environmentHash: project.environment?.hash ?? null, | ||
| integrations: (project.project.integrations ?? []) | ||
| .map((i: { id: string; name: string; type: string }) => ({ id: i.id, name: i.name, type: i.type })) | ||
| .sort((a: { id: string }, b: { id: string }) => a.id.localeCompare(b.id)), | ||
| version: project.version | ||
| }; | ||
|
|
||
| const hashData = JSON.stringify(hashInput); | ||
| const hash = await computeHash(hashData, 'SHA-256'); | ||
|
|
||
| return `sha256:${hash}`; | ||
| } | ||
|
|
||
| private getFileStem(uri: Uri): string { | ||
| const basename = uri.path.split('/').pop() ?? ''; | ||
| const dotIndex = basename.indexOf('.'); | ||
|
|
||
| return dotIndex > 0 ? basename.slice(0, dotIndex) : basename; | ||
| } | ||
|
|
||
| private slugifyNotebookName(name: string): string { | ||
| try { | ||
| return slugifyProjectName(name); | ||
| } catch { | ||
| // Fallback for names that produce empty slugs | ||
| return 'notebook'; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ 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.
Comment on lines
+47
to
+60
| storeOriginalProject(projectId: string, project: DeepnoteProject): void { | ||
| const clonedProject = structuredClone(project); | ||
|
|
||
| this.originalProjects.set(projectId, clonedProject); | ||
| this.currentNotebookId.set(projectId, notebookId); | ||
| } | ||
|
|
||
| /** | ||
| * Updates the current notebook ID for a project. | ||
| * Used when switching notebooks within the same project. | ||
| * Updates the stored project data. | ||
| * Used during serialization where we need to cache the updated project state. | ||
| * @param projectId Project identifier | ||
| * @param notebookId New current notebook ID | ||
| * @param project Updated project data to store | ||
| */ | ||
| updateCurrentNotebookId(projectId: string, notebookId: string): void { | ||
| this.currentNotebookId.set(projectId, notebookId); | ||
| updateOriginalProject(projectId: string, project: DeepnoteProject): void { | ||
| const clonedProject = structuredClone(project); | ||
| this.originalProjects.set(projectId, clonedProject); |
Contributor
There was a problem hiding this comment.
Project-only cache keys will collide after splitting.
This PR groups multiple .deepnote files under one projectId, but both cache writes overwrite a single Map entry keyed only by projectId. Opening notebook B after notebook A will replace A's cached project, and later saves/execution lookups for A can fail because its notebook is no longer in the cached project.
🤖 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 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.
Comment on lines
+224
to
+226
| test('should update integrations when project was stored via updateOriginalProject and return true', () => { | ||
| // Use updateOriginalProject which doesn't set currentNotebookId | ||
| manager.updateOriginalProject('project-123', mockProject); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Drop the stale currentNotebookId comment.
That note describes behavior this PR removes, so it now misstates what the test is covering.
🤖 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 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.
Comment on lines
148
to
+150
| private async performInitialScan(): Promise<void> { | ||
| try { | ||
| await this.getDeepnoteProjectFiles(); | ||
| await this.loadAllProjectFiles(); |
Contributor
There was a problem hiding this comment.
🧹 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.
Comment on lines
+78
to
+79
| const fileName = this.context.filePath.split('/').pop() ?? ''; | ||
| this.label = fileName || project.project.name || 'Untitled Project'; |
Contributor
There was a problem hiding this comment.
split('/') is not safe for filePath.
context.filePath is a native path elsewhere (Uri.file(treeItem.context.filePath)), so on Windows this leaves backslashes intact and the tree label becomes the full path. Use a basename helper that understands platform separators.
Also applies to: 141-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooks/deepnote/deepnoteTreeItem.ts` around lines 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.
Comment on lines
+2547
to
+2554
| export class TabInputNotebook { | ||
| readonly uri: vscUri.URI; | ||
| readonly notebookType: string; | ||
|
|
||
| constructor(uri: vscUri.URI, notebookType: string) { | ||
| this.uri = uri; | ||
| this.notebookType = notebookType; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements