feat: Handle multiple notebooks using the sidebar. #12
Conversation
📝 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
| { | ||
| "id": "deepnote", | ||
| "title": "Deepnote", | ||
| "icon": "$(notebook)" | ||
| } |
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.
| { | ||
| "id": "deepnoteExplorer", | ||
| "name": "Explorer", | ||
| "when": "workspaceFolderCount != 0" | ||
| } |
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.
| { | |
| "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'; | |||
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
| // Should have resource URI | ||
| assert.isDefined(item.resourceUri); | ||
| assert.strictEqual( | ||
| item.resourceUri!.toString(), | ||
| 'deepnote-notebook:/workspace/project.deepnote#notebook-789' | ||
| ); | ||
| }); |
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.
| // 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}`); |
| async provideTextDocumentContent(uri: Uri, token: CancellationToken): Promise<string> { | ||
| if (token.isCancellationRequested) { | ||
| throw new Error('Content provision cancelled'); | ||
| } | ||
|
|
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.
| 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; | ||
|
|
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.
| public static createVirtualUri(filePath: string, notebookId: string): Uri { | ||
| const query = new URLSearchParams({ | ||
| filePath, | ||
| notebookId | ||
| }); | ||
|
|
||
| return Uri.parse(`deepnotenotebook://${notebookId}?${query.toString()}`); | ||
| } |
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.
|
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: |
Thanks! I'll try to reproduce it :) |
Artmann
force-pushed
the
multiple-notebook-support
branch
from
292eaef to
731eba0
Compare
Artmann
force-pushed
the
multiple-notebook-support
branch
from
731eba0 to
fa71c66
Compare
andyjakubowski
left a comment
andyjakubowski
left a comment
There was a problem hiding this comment.
I can switch the notebooks inside a .deepnote file now. Nice!
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;
| - Separate third-party and local file imports | ||
| - How the extension works is described in @architecture.md |
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.
| - 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.
| 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'; | ||
|
|
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'; |
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.
| 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.
| } 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'); | ||
| } |
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.
| // 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); |
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.
| 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: {} | ||
| } | ||
| ], |
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.
| export class DeepnoteVirtualDocumentProvider implements TextDocumentContentProvider { | ||
| private onDidChangeEmitter = new EventEmitter<Uri>(); | ||
| private converter = new DeepnoteDataConverter(); | ||
|
|
||
| readonly onDidChange: Event<Uri> = this.onDidChangeEmitter.event; | ||
|
|
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.
| 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.
| 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(); | ||
| } | ||
| } |
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.
| 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; | ||
| } |
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.
| 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; | |
| } |
| export const RefreshDeepnoteExplorer = 'deepnote.refreshExplorer'; | ||
| export const OpenDeepnoteNotebook = 'deepnote.openNotebook'; | ||
| export const OpenDeepnoteFile = 'deepnote.openFile'; | ||
| export const RevealInDeepnoteExplorer = 'deepnote.revealInExplorer'; |
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.
Awesome! |
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
| 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 | ||
|
|
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.
| 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.
| - id: 'notebook-uuid' | ||
| name: 'Notebook Name' | ||
| blocks: | ||
| - id: 'block-uuid' | ||
| type: 'code' | ||
| source: "print('Hello')" | ||
| outputs: [] | ||
| ``` | ||
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.
| - 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 | ||
| - Separate third-party and local file imports | ||
| - How the extension works is described in @architecture.md |
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.
| "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" |
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.
| "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" |
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.
| const context: DeepnoteTreeItemContext = { | ||
| filePath: file.path, | ||
| projectId: project.project.id | ||
| }; | ||
|
|
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.
| const context: DeepnoteTreeItemContext = { | |
| filePath: file.path, | |
| projectId: project.project.id | |
| }; | |
| export interface DeepnoteTreeItemContext { | |
| readonly filePath: string; | |
| projectId: string; | |
| notebookId?: string; | |
| resourceUri?: Uri; | |
| } |
| 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 | |
| }; |
| 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.
| 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); | ||
| }); | ||
|
|
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.
| 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'; | |||
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.
| 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; | ||
| } |
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.
| 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.
| export const RefreshDeepnoteExplorer = 'deepnote.refreshExplorer'; | ||
| export const OpenDeepnoteNotebook = 'deepnote.openNotebook'; | ||
| export const OpenDeepnoteFile = 'deepnote.openFile'; | ||
| export const RevealInDeepnoteExplorer = 'deepnote.revealInExplorer'; |
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.
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
| - **State Management**: Maintains original project data for accurate serialization | ||
|
|
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.
| - **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.
| - `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 | ||
|
|
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.
| - `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.
| 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 | ||
|
|
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.
| 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.
| - Separate third-party and local file imports | ||
| - How the extension works is described in @architecture.md |
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.
| - 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.
| "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" | ||
| } |
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.
| 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); | ||
| }); | ||
| }); |
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.
| 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'); | ||
| } |
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.
| 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 }; | ||
| } |
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
| 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; | ||
| } |
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.
| 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.
| 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; |
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.
| 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.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests