Skip to content

Conversation

@hc2p
Copy link
Contributor

@hc2p hc2p commented Oct 14, 2025

Fixes GRN-4913

Summary by CodeRabbit

  • New Features

    • Environments: adds an Environments view, picker, notebook↔environment mapping, per-environment lifecycle, server control, venv/toolkit management, multi-step create flow, and commands for create/start/stop/restart/delete/rename/manage-packages/refresh/select-for-notebook.
  • UI

    • Tree view with status visuals; kernel display names and server labels include environment info and ports; environment command titles localized; output channel shown by default.
  • Tests

    • Extensive unit tests for environment manager, storage, view, picker, tree items, activation, auto-selector rebuild, and related flows.
  • Documentation

    • Expanded contributor guide and new debugging guide.
  • Chores

    • Added runtime uuid dependency; removed @types/uuid dev dependency.

hc2p added 3 commits October 14, 2025 20:24
Add core infrastructure for managing Deepnote kernel configurations,
enabling future user-controlled kernel lifecycle management.

## What's New

### Core Models & Storage
- Create type definitions for kernel configurations with UUID-based identity
- Implement persistent storage layer using VS Code globalState
- Build configuration manager with full CRUD operations
- Add event system for configuration change notifications

### Components Added
- `deepnoteKernelConfiguration.ts`: Type definitions and interfaces
- `deepnoteConfigurationStorage.ts`: Serialization and persistence
- `deepnoteConfigurationManager.ts`: Business logic and lifecycle management

### API Updates
- Extend `IDeepnoteToolkitInstaller` with configuration-based methods
- Extend `IDeepnoteServerStarter` with configuration-based methods
- Maintain backward compatibility with file-based APIs

### Service Registration
- Register DeepnoteConfigurationStorage as singleton
- Register DeepnoteConfigurationManager with auto-activation
- Integrate with existing dependency injection system

## Testing
- Add comprehensive unit tests for storage layer (11 tests, all passing)
- Add unit tests for configuration manager (29 tests, 22 passing)
- 7 tests intentionally failing pending Phase 2 service refactoring

## Documentation
- Create KERNEL_MANAGEMENT_VIEW_IMPLEMENTATION.md with complete architecture
- Document 8-phase implementation plan
- Define migration strategy from file-based to configuration-based system

## Dependencies
- Add uuid package for configuration ID generation

## Status
Phase 1 complete. Ready for Phase 2 (refactoring existing services).

Related: #4913
…epnoteServerStarter

Refactor DeepnoteToolkitInstaller and DeepnoteServerStarter to support
configuration-based kernel management alongside existing file-based workflow.

DeepnoteToolkitInstaller changes:
- Add ensureVenvAndToolkit() method for direct venv path management
- Add installAdditionalPackages() for installing packages in existing venvs
- Add getVenvInterpreterByPath() helper for venv path-based interpreter lookup
- Add getKernelSpecName() and getKernelDisplayName() for venv-based naming
- Refactor installImpl() to installVenvAndToolkit() using venv paths
- Update kernel spec installation to use venv directory names instead of file hashes
- Mark ensureInstalled() as deprecated, now delegates to ensureVenvAndToolkit()

DeepnoteServerStarter changes:
- Add startServer() method accepting venv path and configuration ID
- Add stopServer() method accepting configuration ID instead of file URI
- Add startServerForConfiguration() implementation for config-based lifecycle
- Add stopServerForConfiguration() implementation for config-based cleanup
- Mark getOrStartServer() as deprecated for backward compatibility
- Update server process tracking to work with configuration IDs as keys

These changes enable the kernel configuration manager to create and manage
isolated Python environments without requiring .deepnote file associations.
Legacy file-based methods are preserved for backward compatibility.

Part of Phase 2: Refactoring Existing Services
Add VS Code tree view interface for managing Deepnote kernel configurations.

Changes:
- Created DeepnoteConfigurationTreeItem with status-based icons and context values
- Created DeepnoteConfigurationTreeDataProvider implementing TreeDataProvider
- Created DeepnoteConfigurationsView handling all UI commands:
  * create: Multi-step wizard for new configurations
  * start/stop/restart: Server lifecycle management
  * delete: Configuration removal with confirmation
  * editName: Rename configurations
  * managePackages: Update package lists
  * refresh: Manual tree refresh
- Created DeepnoteConfigurationsActivationService for initialization
- Registered all services in serviceRegistry.node.ts
- Added deepnoteKernelConfigurations view to package.json
- Added 8 commands with icons and context menus
- Added view/title and view/item/context menu contributions

Technical details:
- Uses Python API to enumerate available interpreters
- Implements progress notifications for long-running operations
- Provides input validation for names and package lists
- Shows status indicators (Running/Starting/Stopped) with appropriate colors
- Displays configuration details (Python path, venv, packages, timestamps)

Fixes:
- Made DeepnoteConfigurationManager.initialize() public to match interface
- Removed unused getDisplayName() method from DeepnoteToolkitInstaller
- Added type annotations to all lambda parameters
@linear
Copy link

linear bot commented Oct 14, 2025

GRN-4913 Manage Deepnote kernels

Add a place where you can view all the Deepnote kernels started by the extension.

You should be able to restart and delete kernels.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 14, 2025

📝 Walkthrough

Walkthrough

Adds first-class Deepnote environments: runtime and persisted models, storage, mapper, picker, tree data provider, view, UI helpers, and activation service. Introduces DeepnoteEnvironmentManager with per-environment server lifecycle (start/stop/restart), refactors toolkit installer and server starter to operate per-venv (per-environment) including jupyter/lsp port allocation and locking, expands public types/interfaces and package.json contributions (commands, view), registers environment services, updates kernel auto-selector to be environment-aware, and adds unit tests across storage, manager, picker, tree items/providers, view, activation, and selector flows.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant User
    participant UI as Environments View / Picker
    participant Manager as DeepnoteEnvironmentManager
    participant Storage as DeepnoteEnvironmentStorage
    participant Toolkit as DeepnoteToolkitInstaller
    participant Server as DeepnoteServerStarter
    participant Mapper as NotebookEnvironmentMapper

    Note over User,UI: Create environment
    User->>UI: createEnvironment(options)
    UI->>Manager: createEnvironment(options)
    Manager->>Toolkit: ensureVenvAndToolkit(baseInterpreter, venvPath, token)
    Toolkit-->>Manager: VenvAndToolkitInstallation
    Manager->>Storage: saveEnvironments()
    Storage-->>Manager: ack
    Manager->>UI: emit onDidChangeEnvironments

    Note over User,UI: Select environment for notebook
    User->>UI: selectForNotebook(envId)
    UI->>Mapper: setEnvironmentForNotebook(notebookUri, envId)
    Mapper-->>UI: ack
    UI->>Manager: startServer(envId)
    Manager->>Server: startServer(interpreter, venvPath, envId, token)
    Server-->>Manager: DeepnoteServerInfo (jupyterPort, lspPort)
    Manager->>Storage: saveEnvironments()
    Manager->>UI: emit onDidChangeEnvironments
    UI->>User: show Running
Loading

Possibly related PRs

Suggested reviewers

  • jamesbhobbs
  • Artmann
  • andyjakubowski

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "feat: Deepnote kernel management view" accurately describes the primary change in this pull request—a new UI view for managing Deepnote kernels/environments. The title is concise, clear, and aligns with the linked issue GRN-4913. While the implementation is technically "environment-based" rather than kernel-specific (environments contain servers with kernels), this is a reasonable abstraction and the title conveys the user-facing feature appropriately. A teammate scanning the commit history would understand that this adds kernel management functionality.
Linked Issues Check ✅ Passed The implementation addresses all three core requirements from GRN-4913: it adds a dedicated deepnoteEnvironments view container with tree display for viewing all Deepnote kernels, implements restart functionality via the deepnote.environments.restart command, and implements delete functionality via the deepnote.environments.delete command. Supporting infrastructure includes DeepnoteEnvironmentManager for CRUD operations, DeepnoteEnvironmentStorage for persistence, DeepnoteEnvironmentTreeDataProvider for UI rendering, and command handlers in DeepnoteEnvironmentsView. The refactoring of server starter, toolkit installer, and kernel auto-selector provides the environment-based architecture needed to support this feature.
Out of Scope Changes Check ✅ Passed All code changes support the stated objective of managing Deepnote kernels. The architectural refactoring of deepnoteServerStarter, deepnoteToolkitInstaller, and deepnoteKernelAutoSelector from file-based to environment-based is foundational work necessary for the new environment management model rather than extraneous scope creep. Additional commands (create, manage packages, edit name) are reasonable extensions to a kernel management view. UI localization, type definitions, and supporting infrastructure all directly enable the core feature.

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

@hc2p
Copy link
Contributor Author

hc2p commented Oct 14, 2025

@coderabbitai pause

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 14, 2025

✅ Actions performed

Reviews paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 25

Caution

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

⚠️ Outside diff range comments (2)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)

208-213: Don’t nuke existing venvs unless they’re actually broken

You unconditionally delete the venv, wiping user-installed packages. Only delete if the interpreter is missing; otherwise reuse and install/upgrade packages.

- // Remove old venv if it exists but is broken
- if (await this.fs.exists(venvPath)) {
-     logger.info('Removing existing broken venv');
-     await workspace.fs.delete(venvPath, { recursive: true });
- }
+ // Remove venv only if it exists and is missing the interpreter
+ if (await this.fs.exists(venvPath)) {
+     const existing = await this.getVenvInterpreterByPath(venvPath);
+     if (!existing) {
+         logger.info('Venv missing interpreter, removing and recreating');
+         await workspace.fs.delete(venvPath, { recursive: true });
+     } else {
+         logger.info('Reusing existing venv');
+     }
+ }

201-206: Localize output strings

Wrap user-visible strings with l10n.t() before appending/logging.

As per coding guidelines (use l10n.t() for user-facing strings).

Also applies to: 243-257, 260-276, 279-285, 320-321

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1ca48b and 2d692a3.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (15)
  • KERNEL_MANAGEMENT_VIEW_IMPLEMENTATION.md (1 hunks)
  • package.json (4 hunks)
  • src/kernels/deepnote/configurations/deepnoteConfigurationManager.ts (1 hunks)
  • src/kernels/deepnote/configurations/deepnoteConfigurationManager.unit.test.ts (1 hunks)
  • src/kernels/deepnote/configurations/deepnoteConfigurationStorage.ts (1 hunks)
  • src/kernels/deepnote/configurations/deepnoteConfigurationStorage.unit.test.ts (1 hunks)
  • src/kernels/deepnote/configurations/deepnoteConfigurationTreeDataProvider.ts (1 hunks)
  • src/kernels/deepnote/configurations/deepnoteConfigurationTreeItem.ts (1 hunks)
  • src/kernels/deepnote/configurations/deepnoteConfigurationsActivationService.ts (1 hunks)
  • src/kernels/deepnote/configurations/deepnoteConfigurationsView.ts (1 hunks)
  • src/kernels/deepnote/configurations/deepnoteKernelConfiguration.ts (1 hunks)
  • src/kernels/deepnote/deepnoteServerStarter.node.ts (2 hunks)
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (9 hunks)
  • src/kernels/deepnote/types.ts (3 hunks)
  • src/notebooks/serviceRegistry.node.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/kernels/deepnote/configurations/deepnoteConfigurationsView.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationsActivationService.ts
  • src/kernels/deepnote/configurations/deepnoteKernelConfiguration.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationStorage.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationTreeItem.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationManager.unit.test.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationStorage.unit.test.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationManager.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationTreeDataProvider.ts
  • src/kernels/deepnote/types.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/kernels/deepnote/configurations/deepnoteConfigurationsView.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationsActivationService.ts
  • src/kernels/deepnote/configurations/deepnoteKernelConfiguration.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationStorage.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationTreeItem.ts
  • src/notebooks/serviceRegistry.node.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationManager.unit.test.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationStorage.unit.test.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationManager.ts
  • src/kernels/deepnote/deepnoteServerStarter.node.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationTreeDataProvider.ts
  • src/kernels/deepnote/types.ts
src/kernels/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)

src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations

Files:

  • src/kernels/deepnote/configurations/deepnoteConfigurationsView.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationsActivationService.ts
  • src/kernels/deepnote/configurations/deepnoteKernelConfiguration.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationStorage.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationTreeItem.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationManager.unit.test.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationStorage.unit.test.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationManager.ts
  • src/kernels/deepnote/deepnoteServerStarter.node.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationTreeDataProvider.ts
  • src/kernels/deepnote/types.ts
**/*.node.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/notebooks/serviceRegistry.node.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
  • src/kernels/deepnote/deepnoteServerStarter.node.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 have the .unit.test.ts 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/kernels/deepnote/configurations/deepnoteConfigurationManager.unit.test.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationStorage.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 then property to avoid hanging tests

Files:

  • src/kernels/deepnote/configurations/deepnoteConfigurationManager.unit.test.ts
  • src/kernels/deepnote/configurations/deepnoteConfigurationStorage.unit.test.ts
🧠 Learnings (4)
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote

Applied to files:

  • src/kernels/deepnote/configurations/deepnoteConfigurationsActivationService.ts
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/ipywidgets/serviceRegistry.{node,web}.ts : Register DI services in platform-specific registries (serviceRegistry.node.ts vs serviceRegistry.web.ts)

Applied to files:

  • src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:56:39.535Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel-jupyter.instructions.md:0-0
Timestamp: 2025-09-03T12:56:39.535Z
Learning: Applies to src/kernels/jupyter/jupyterKernelSpec.ts : Manage kernel spec registration and updates; create temporary kernel specs for non-standard kernels when needed

Applied to files:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles Deepnote notebook state management

Applied to files:

  • src/kernels/deepnote/types.ts
🧬 Code graph analysis (10)
src/kernels/deepnote/configurations/deepnoteKernelConfiguration.ts (1)
src/kernels/deepnote/types.ts (1)
  • DeepnoteServerInfo (169-173)
src/kernels/deepnote/configurations/deepnoteConfigurationStorage.ts (3)
src/kernels/deepnote/configurations/deepnoteKernelConfiguration.ts (2)
  • DeepnoteKernelConfiguration (9-60)
  • DeepnoteKernelConfigurationState (66-76)
src/platform/logging/index.ts (1)
  • logger (35-48)
src/kernels/jupyter/interpreter/jupyterCommand.node.ts (1)
  • interpreter (35-37)
src/kernels/deepnote/configurations/deepnoteConfigurationTreeItem.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (2)
  • TreeItem (2066-2086)
  • ThemeIcon (2094-2104)
src/kernels/deepnote/configurations/deepnoteKernelConfiguration.ts (1)
  • DeepnoteKernelConfiguration (9-60)
src/notebooks/serviceRegistry.node.ts (1)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteConfigurationManager (201-201)
  • IDeepnoteConfigurationManager (202-218)
src/kernels/deepnote/configurations/deepnoteConfigurationManager.unit.test.ts (1)
src/kernels/deepnote/types.ts (5)
  • IDeepnoteToolkitInstaller (68-68)
  • IDeepnoteToolkitInstaller (69-122)
  • IDeepnoteServerStarter (124-124)
  • IDeepnoteServerStarter (125-167)
  • DeepnoteServerInfo (169-173)
src/kernels/deepnote/configurations/deepnoteConfigurationStorage.unit.test.ts (1)
src/kernels/deepnote/configurations/deepnoteKernelConfiguration.ts (1)
  • DeepnoteKernelConfigurationState (66-76)
src/kernels/deepnote/configurations/deepnoteConfigurationManager.ts (2)
src/kernels/deepnote/configurations/deepnoteKernelConfiguration.ts (3)
  • DeepnoteKernelConfiguration (9-60)
  • CreateKernelConfigurationOptions (81-86)
  • DeepnoteKernelConfigurationWithStatus (116-119)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteToolkitInstaller (68-68)
  • IDeepnoteToolkitInstaller (69-122)
src/kernels/deepnote/deepnoteServerStarter.node.ts (3)
src/kernels/deepnote/types.ts (2)
  • DeepnoteServerInfo (169-173)
  • DEEPNOTE_DEFAULT_PORT (285-285)
src/platform/logging/index.ts (1)
  • logger (35-48)
src/platform/common/types.ts (1)
  • IDisposable (211-213)
src/kernels/deepnote/configurations/deepnoteConfigurationTreeDataProvider.ts (1)
src/kernels/deepnote/configurations/deepnoteConfigurationTreeItem.ts (1)
  • DeepnoteConfigurationTreeItem (19-154)
src/kernels/deepnote/types.ts (2)
src/kernels/deepnote/configurations/deepnoteConfigurationManager.ts (9)
  • getConfiguration (96-98)
  • getConfigurationWithStatus (103-120)
  • updateConfiguration (125-148)
  • deleteConfiguration (153-169)
  • startServer (174-210)
  • stopServer (215-241)
  • restartServer (246-250)
  • updateLastUsed (255-263)
  • dispose (276-278)
src/kernels/deepnote/deepnoteServerStarter.node.ts (3)
  • startServer (76-115)
  • stopServer (121-145)
  • dispose (500-578)
🪛 ESLint
src/kernels/deepnote/configurations/deepnoteConfigurationStorage.ts

[error] 71-71: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 72-72: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 124-124: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)

src/kernels/deepnote/configurations/deepnoteConfigurationTreeItem.ts

[error] 103-103: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 104-104: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)

src/kernels/deepnote/configurations/deepnoteConfigurationTreeDataProvider.ts

[error] 84-84: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 91-91: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)

🪛 GitHub Actions: CI
src/kernels/deepnote/configurations/deepnoteConfigurationManager.ts

[error] 3-3: ESLint: Unable to resolve path to module 'uuid' (import/no-unresolved)

🪛 GitHub Check: Lint & Format
src/kernels/deepnote/configurations/deepnoteConfigurationStorage.ts

[failure] 124-124:
fsPath is not allowed in anything but .node files


[failure] 72-72:
fsPath is not allowed in anything but .node files


[failure] 71-71:
fsPath is not allowed in anything but .node files

src/kernels/deepnote/configurations/deepnoteConfigurationTreeItem.ts

[failure] 104-104:
fsPath is not allowed in anything but .node files


[failure] 103-103:
fsPath is not allowed in anything but .node files

src/kernels/deepnote/configurations/deepnoteConfigurationManager.ts

[failure] 3-3:
Unable to resolve path to module 'uuid'

src/kernels/deepnote/configurations/deepnoteConfigurationTreeDataProvider.ts

[failure] 91-91:
fsPath is not allowed in anything but .node files


[failure] 84-84:
fsPath is not allowed in anything but .node files

🪛 markdownlint-cli2 (0.18.1)
KERNEL_MANAGEMENT_VIEW_IMPLEMENTATION.md

30-30: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


40-40: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


54-54: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


93-93: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


129-129: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


129-129: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


233-233: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


246-246: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


261-261: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


261-261: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


283-283: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


306-306: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


307-307: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


326-326: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


327-327: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


386-386: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


387-387: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


443-443: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


444-444: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


465-465: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


477-477: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


488-488: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


501-501: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


512-512: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


524-524: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


536-536: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


548-548: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


571-571: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


575-575: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


584-584: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


589-589: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


595-595: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


615-615: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


621-621: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


627-627: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🔇 Additional comments (9)
package.json (1)

1294-1323: Verify context keys and equality vs regex in ‘when’ clauses

Ensure your TreeItem.contextValue exactly matches:

  • deepnoteConfiguration.stopped
  • deepnoteConfiguration.running

You mix equality and regex operators; fine if values align. Please verify.

src/notebooks/serviceRegistry.node.ts (1)

148-165: Confirm manager implements activation before binding

You bind IDeepnoteConfigurationManager to IExtensionSyncActivationService. Ensure DeepnoteConfigurationManager implements IExtensionSyncActivationService; otherwise remove this binding and rely on the dedicated DeepnoteConfigurationsActivationService.

Also verify registration order doesn’t introduce circular deps.

Based on learnings (Inversify v6 DI bindings).

src/kernels/deepnote/configurations/deepnoteConfigurationStorage.unit.test.ts (1)

179-193: Register chai-as-promised for assert.isRejected
assert.isRejected is provided by chai-as-promised; ensure chai.use(require('chai-as-promised')) is invoked in your global test setup (or add it at the top of this file).

src/kernels/deepnote/configurations/deepnoteConfigurationManager.unit.test.ts (1)

11-491: Comprehensive coverage — looks good

Good end-to-end manager coverage (CRUD, events, lifecycle). No blockers spotted.

src/kernels/deepnote/configurations/deepnoteKernelConfiguration.ts (1)

1-119: Types are clear and consistent

Runtime vs serialized models and status typing are well-structured.

src/kernels/deepnote/configurations/deepnoteConfigurationsView.ts (1)

337-343: Confirm deletion message matches behavior

Prompt claims the venv will be removed. If deleteConfiguration doesn’t remove the venv on disk, adjust wording to avoid confusion or implement venv removal before messaging this.

src/kernels/deepnote/configurations/deepnoteConfigurationsActivationService.ts (1)

16-24: No API mismatch: initialize() is the public manager method and activate() on DeepnoteConfigurationManager simply forwards to initialize(), so no changes needed.

Likely an incorrect or invalid review comment.

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

124-161: Interfaces look coherent with the new configuration model

API surface for installer, server starter, and configuration manager aligns with usage. Ensure downstream implementations pass CancellationToken through.

Also applies to: 201-281

src/kernels/deepnote/configurations/deepnoteConfigurationManager.ts (1)

3-3: Stale warning: ‘uuid’ import resolved package.json already includes uuid (^13.0.0) and @types/uuid.

Likely an incorrect or invalid review comment.

hc2p added 9 commits October 16, 2025 14:44
Add VS Code tree view interface for managing Deepnote kernel configurations.

Changes:
- Created DeepnoteConfigurationTreeItem with status-based icons and context values
- Created DeepnoteConfigurationTreeDataProvider implementing TreeDataProvider
- Created DeepnoteConfigurationsView handling all UI commands:
  * create: Multi-step wizard for new configurations
  * start/stop/restart: Server lifecycle management
  * delete: Configuration removal with confirmation
  * editName: Rename configurations
  * managePackages: Update package lists
  * refresh: Manual tree refresh
- Created DeepnoteConfigurationsActivationService for initialization
- Registered all services in serviceRegistry.node.ts
- Added deepnoteKernelConfigurations view to package.json
- Added 8 commands with icons and context menus
- Added view/title and view/item/context menu contributions

Technical details:
- Uses Python API to enumerate available interpreters
- Implements progress notifications for long-running operations
- Provides input validation for names and package lists
- Shows status indicators (Running/Starting/Stopped) with appropriate colors
- Displays configuration details (Python path, venv, packages, timestamps)

Fixes:
- Made DeepnoteConfigurationManager.initialize() public to match interface
- Removed unused getDisplayName() method from DeepnoteToolkitInstaller
- Added type annotations to all lambda parameters
Add infrastructure for notebook-to-configuration mapping and selection UI.

Changes:
- Created DeepnoteConfigurationPicker for showing configuration selection UI
- Created DeepnoteNotebookConfigurationMapper for tracking notebook→config mappings
- Added interfaces to types.ts for new services
- Registered new services in serviceRegistry.node.ts
- Updated KERNEL_MANAGEMENT_VIEW_IMPLEMENTATION.md with:
  * New components documentation (picker and mapper)
  * Updated file structure with implementation status
  * Comprehensive implementation status section

Technical details:
- Picker shows configurations with status indicators (running/stopped)
- Picker includes "Create new" option
- Mapper stores selections in workspace state (per-workspace persistence)
- Mapper provides bidirectional lookups (notebook→config and config→notebooks)

Next steps:
- Integrate picker and mapper with DeepnoteKernelAutoSelector
- Show picker when notebook opens without selected configuration
- Use selected configuration's venv and server instead of auto-creating
This refactoring improves terminology clarity across the Deepnote kernel
management system by renaming "Kernel Configuration" to "Environment".

Rationale:
- "Configuration" implies kernel settings, but the system manages Python
  virtual environments (venv + packages + Jupyter server)
- "Environment" is more accurate and familiar to developers
- Reduces confusion between "configuration" (settings) and "environment"
  (Python venv)

Changes:
- Renamed directory: configurations/ → environments/
- Renamed 15 files (classes, interfaces, tests)
- Updated types.ts: 6 interface names, 6 symbol constants
- Updated package.json: 9 commands, 1 view ID, titles/labels
- Updated all import paths and references across codebase
- Updated documentation in KERNEL_MANAGEMENT_VIEW_IMPLEMENTATION.md

Terminology mapping:
- Kernel Configuration → Environment
- IDeepnoteConfigurationManager → IDeepnoteEnvironmentManager
- DeepnoteConfigurationsView → DeepnoteEnvironmentsView
- deepnote.configurations.* → deepnote.environments.*
- deepnoteKernelConfigurations view → deepnoteEnvironments view

All tests pass, TypeScript compilation successful.
this was leading to many error logs in the "Jupyter Outputs"
Added comprehensive UI for selecting and switching environments for Deepnote
notebooks, making it easy for users to choose which kernel environment to use.

New Features:
- Added "Select Environment for Notebook" command in notebook toolbar that
  shows a quick pick with all available environments
- Environment picker now directly triggers the create environment dialog when
  "Create New" is selected, then automatically re-shows the picker
- Environment names are now displayed in kernel connection labels for better
  visibility (e.g., "Deepnote: Python 3.10")
- Shows current environment selection with checkmark in the picker
- Displays environment status (Running/Stopped) with icons in picker
- Warns users before switching if cells are currently executing

Improvements:
- Environment picker integration: Clicking "Create New" now launches the full
  creation dialog instead of just showing an info message
- Added rebuildController() to IDeepnoteKernelAutoSelector interface for
  switching environments
- Updated test mocks to include new dependencies

UI/UX:
- Added notebook toolbar button with server-environment icon
- Quick pick shows environment details: interpreter path, packages, status
- Graceful handling of edge cases (no environments, same environment selected)
- Clear progress notifications during environment switching
Fixed three critical bugs that prevented proper environment switching:

1. Controller disposal race condition: Old controller was disposed before
   the new controller was ready, causing "DISPOSED" errors during cell
   execution. Fixed by deferring disposal until after new controller is
   fully registered.

2. Stale configuration caching: Configuration object wasn't refreshed after
   startServer(), so we connected with outdated serverInfo. Fixed by always
   calling getEnvironment() after startServer() to get current server info.

3. Environment manager early return: startServer() had an early return when
   config.serverInfo was set, preventing verification that the server was
   actually running. This caused connections to wrong/stale servers when
   switching TO a previously-used environment. Fixed by always calling
   serverStarter.startServer() (which is idempotent) to ensure current info.

Additional improvements:
- Made kernel spec installation idempotent and ensured it runs when reusing
  existing venvs
- Removed legacy auto-create fallback path that's no longer needed
- Added proper TypeScript non-null assertions after server info validation
This commit implements environment switching but with a known limitation
regarding controller disposal and queued cell executions.

## Changes

### Port Allocation Refactoring
- Refactored DeepnoteServerProvider to handle both jupyterPort and lspPort
- Updated DeepnoteServerStarter to allocate both ports
- Updated DeepnoteEnvironmentManager to store complete serverInfo
- All port-related code now consistently uses the dual-port model

### Kernel Selection Logic Extraction
- Extracted kernel selection logic into public `selectKernelSpec()` method
- Added 4 unit tests for kernel selection (all passing)
- Method is now testable and validates environment-specific kernel preference
- Falls back to generic Python kernels when env-specific kernel not found

### Environment Switching Implementation
- Added environment switching via tree view context menu
- Switching calls `rebuildController()` to create new controller
- Shows warning dialog if cells are currently executing
- Updates notebook-to-environment mapping on switch

## Known Issue: Controller Disposal Problem

When switching environments, we encountered a critical "notebook controller is
DISPOSED" error. The issue occurs because:

1. User queues cell execution (VS Code references current controller)
2. User switches environments
3. New controller created and marked as preferred
4. Old controller disposed
5. Queued execution tries to run 5+ seconds later → DISPOSED error

### Current Workaround (Implemented)
We do NOT dispose old controllers at all. Instead:
- Old controllers stay alive to handle queued executions
- New controller is marked as "Preferred" for new executions
- Garbage collection cleans up eventually

### Why This Is Not Ideal
- Potential memory leak with many switches
- No guarantee new executions use new controller
- Users might execute on wrong environment after switch
- VS Code controller selection not fully deterministic

### What We Tried
1. Adding delay before disposal → Failed (timing unpredictable)
2. Disposing after setting preference → Failed
3. Never disposing → Prevents error but suboptimal

### Proper Solution Needed
May require VS Code API changes to:
- Force controller selection immediately
- Cancel/migrate queued executions
- Query if controller has pending executions

See TODO.md for full analysis and discussion.

## Testing
- ✅ All 40+ unit tests passing
- ✅ Kernel selection tests passing (4 new tests)
- ✅ Port allocation working correctly
- ⚠️  Environment switching works but with limitations above

Related files:
- src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts:306-315
- src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts:599-635
- src/kernels/deepnote/environments/deepnoteEnvironmentsView.ts:542-561
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 34

Caution

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

⚠️ Outside diff range comments (4)
src/kernels/deepnote/deepnoteServerStarter.node.ts (1)

271-298: Release reserved ports even when proc is undefined

serverInfos is only cleared inside the “if (serverProcess)” branch. If start fails before proc is created, ports remain reserved in serverInfos, causing leaks/conflicts on subsequent starts.

Move serverInfos.delete(environmentId) outside the proc branch so it always runs.

Apply:

     if (serverProcess) {
         const serverPid = serverProcess.proc?.pid;
         try {
             logger.info(`Stopping Deepnote server for environment ${environmentId}...`);
             serverProcess.proc?.kill();
-            this.serverProcesses.delete(environmentId);
-            this.serverInfos.delete(environmentId);
+            this.serverProcesses.delete(environmentId);
             this.outputChannel.appendLine(`Deepnote server stopped for environment ${environmentId}`);
             if (serverPid) {
                 await this.deleteLockFile(serverPid);
             }
         } catch (ex) {
             logger.error(`Error stopping Deepnote server: ${ex}`);
         }
     }
+    // Always release reserved ports/server record.
+    this.serverInfos.delete(environmentId);
package.json (1)

2289-2289: get-port is a runtime dep; upgrade and move to dependencies

  • Move get-port to dependencies and upgrade to ^7.1.0 (typed).
  • Remove @types/get-port.
  • Update import stays the same.

Based on learnings

   "dependencies": {
+    "get-port": "^7.1.0",
     ...
   },
   "devDependencies": {
-    "@types/get-port": "^3.2.0",
-    "get-port": "^3.2.0",
+    /* remove both entries */
   }

Also applies to: 2362-2362, 2206-2270

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

320-334: Honor cancellation token and progress cancellation

The method ignores the provided token. Respect both the incoming token and the progress token to avoid doing work after cancellation.

-    public async ensureKernelSelected(notebook: NotebookDocument, _token?: CancellationToken): Promise<void> {
+    public async ensureKernelSelected(notebook: NotebookDocument, token?: CancellationToken): Promise<void> {
         return window.withProgress(
@@
-            async (progress, progressToken) => {
+            async (progress, progressToken) => {
                 try {
+                    if (token?.isCancellationRequested || progressToken.isCancellationRequested) {
+                        return;
+                    }
                     logger.info(`Ensuring Deepnote kernel is selected for ${getDisplayPath(notebook.uri)}`);

Also add lightweight cancellation checks before expensive steps (picker, server start, specs fetch). Based on learnings

Also applies to: 423-432


118-123: Use URI string as notebook key to avoid scheme collisions

fsPath can collide across schemes (e.g., different authorities). Prefer the normalized URI string.

-        const notebookKey = baseFileUri.fsPath;
+        const notebookKey = baseFileUri.toString();

Also applies to: 147-149

♻️ Duplicate comments (7)
KERNEL_MANAGEMENT_VIEW_IMPLEMENTATION.md (1)

217-240: Align documented state with implementation

The DeepnoteKernelConfigurationState schema here (isRunning/serverPort/serverUrl) conflicts with the environment-centric model and persisted fields. Update to match actual stored shape (IDs, interpreter/venv URIs, timestamps, server ports).

src/kernels/deepnote/deepnoteServerStarter.node.ts (1)

164-165: Localize user strings and use typed errors

Wrap user-facing messages with l10n.t() and replace new Error(...) with typed errors from src/platform/errors/. As per coding guidelines.

Examples:

-throw new Error('Failed to install deepnote-toolkit. Please check the output for details.');
+throw /* TypedError */ new Error(l10n.t('Failed to install Deepnote Toolkit. See output for details.'));

-this.outputChannel.appendLine(`Starting Deepnote server on jupyter port ${jupyterPort} and lsp port ${lspPort}...`);
+this.outputChannel.appendLine(l10n.t('Starting Deepnote server on Jupyter port {0} and LSP port {1}...', jupyterPort, lspPort));

-throw new Error('Deepnote server failed to start within timeout period');
+throw /* TypedError */ new Error(l10n.t('Deepnote server failed to start within the timeout period.'));

Import l10n at top if not already.

Also applies to: 176-178, 247-255, 262-264, 278-283

package.json (1)

2263-2263: Fix uuid typings mismatch

uuid@^13 ships types; remove @types/uuid to avoid conflicts.

   "dependencies": {
-    "uuid": "^13.0.0",
+    "uuid": "^13.0.0",
   },
   "devDependencies": {
-    "@types/uuid": "^3.4.3",
+    /* remove @types/uuid */
   }

Also applies to: 2318-2318

src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts (2)

1-2: Remove Microsoft header on new files

Repo forbids adding this header to new files.

-// Copyright (c) Microsoft Corporation.
-// Licensed under the MIT License.

Based on learnings


51-57: Fix ThemeIcon usage (invalid ids and unsupported color arg)

  • ThemeIcon doesn’t accept a color param.
  • 'vm-running' and 'loading~spin' aren’t valid codicons. Use supported ids.
-        if (isRunning) {
-            this.iconPath = new ThemeIcon('vm-running', { id: 'charts.green' });
-        } else if (isStarting) {
-            this.iconPath = new ThemeIcon('loading~spin', { id: 'charts.yellow' });
-        } else {
-            this.iconPath = new ThemeIcon('vm-outline', { id: 'charts.gray' });
-        }
+        if (isRunning) {
+            this.iconPath = new ThemeIcon('vm');
+        } else if (isStarting) {
+            this.iconPath = new ThemeIcon('sync');
+        } else {
+            this.iconPath = new ThemeIcon('vm-outline');
+        }

As per coding guidelines

src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.ts (1)

1-2: Remove Microsoft header on new files

Repo forbids adding this header to new files.

-// Copyright (c) Microsoft Corporation.
-// Licensed under the MIT License.

Based on learnings

src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)

144-185: Typed errors, localization, and structured logging

Repeat from prior review: replace raw Error with typed errors; localize user-facing strings; log exceptions as objects, not interpolated.

-import { CancellationToken, Uri, workspace } from 'vscode';
+import { CancellationToken, Uri, workspace, l10n } from 'vscode';
-        if (!venvInterpreter) {
-            throw new Error(`Venv not found at ${venvPath.fsPath}`);
-        }
+        if (!venvInterpreter) {
+            throw new Error(l10n.t('Virtual environment not found at {0}', venvPath.fsPath));
+            // Prefer a typed error from src/platform/errors, e.g., new ResourceNotFoundError(...)
+        }
@@
-        this.outputChannel.appendLine(`Installing packages: ${packages.join(', ')}...`);
+        this.outputChannel.appendLine(l10n.t('Installing packages: {0}…', packages.join(', ')));
@@
-            logger.info('Additional packages installed successfully');
-            this.outputChannel.appendLine('✓ Packages installed successfully');
+            logger.info('Additional packages installed successfully');
+            this.outputChannel.appendLine(l10n.t('✓ Packages installed successfully'));
@@
-        } catch (ex) {
-            logger.error(`Failed to install additional packages: ${ex}`);
-            this.outputChannel.appendLine(`✗ Failed to install packages: ${ex}`);
-            throw ex;
+        } catch (ex) {
+            logger.error('Failed to install additional packages', ex as any);
+            this.outputChannel.appendLine(l10n.t('✗ Failed to install packages'));
+            throw ex;
         }
-            this.outputChannel.appendLine(`Setting up Deepnote toolkit environment...`);
+            this.outputChannel.appendLine(l10n.t('Setting up Deepnote toolkit environment…'));
@@
-            this.outputChannel.appendLine('Upgrading pip...');
+            this.outputChannel.appendLine(l10n.t('Upgrading pip…'));
@@
-            this.outputChannel.appendLine('Installing deepnote-toolkit and ipykernel...');
+            this.outputChannel.appendLine(l10n.t('Installing deepnote-toolkit and ipykernel…'));
@@
-                this.outputChannel.appendLine('✓ Deepnote toolkit ready');
+                this.outputChannel.appendLine(l10n.t('✓ Deepnote toolkit ready'));
@@
-            logger.error(`Failed to set up deepnote-toolkit: ${ex}`);
-            this.outputChannel.appendLine(`Error setting up deepnote-toolkit: ${ex}`);
+            logger.error('Failed to set up deepnote-toolkit', ex as any);
+            this.outputChannel.appendLine(l10n.t('Error setting up deepnote-toolkit'));

If you prefer, I can replace throw new Error(...) with concrete typed errors from src/platform/errors in a follow-up patch. As per coding guidelines

Also applies to: 208-320

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2d692a3 and 460ba6d.

📒 Files selected for processing (31)
  • .vscode/launch.json (1 hunks)
  • CLAUDE.md (2 hunks)
  • DEBUGGING_KERNEL_MANAGEMENT.md (1 hunks)
  • KERNEL_MANAGEMENT_VIEW_IMPLEMENTATION.md (1 hunks)
  • TODO.md (1 hunks)
  • package.json (5 hunks)
  • src/kernels/deepnote/deepnoteServerProvider.node.ts (1 hunks)
  • src/kernels/deepnote/deepnoteServerStarter.node.ts (5 hunks)
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (8 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironment.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.ts (1 hunks)
  • src/kernels/deepnote/types.ts (7 hunks)
  • src/kernels/helpers.ts (1 hunks)
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (4 hunks)
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts (1 hunks)
  • src/notebooks/serviceRegistry.node.ts (2 hunks)
  • src/platform/common/utils/localize.ts (2 hunks)
  • src/platform/logging/index.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.node.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/kernels/deepnote/deepnoteServerStarter.node.ts
  • src/kernels/deepnote/deepnoteServerProvider.node.ts
  • src/notebooks/serviceRegistry.node.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/kernels/deepnote/deepnoteServerStarter.node.ts
  • src/platform/logging/index.ts
  • src/kernels/deepnote/deepnoteServerProvider.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironment.ts
  • src/kernels/helpers.ts
  • src/platform/common/utils/localize.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
  • src/notebooks/serviceRegistry.node.ts
  • src/kernels/deepnote/types.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
src/kernels/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)

src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations

Files:

  • src/kernels/deepnote/deepnoteServerStarter.node.ts
  • src/kernels/deepnote/deepnoteServerProvider.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironment.ts
  • src/kernels/helpers.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
  • src/kernels/deepnote/types.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/platform/logging/index.ts
  • src/kernels/deepnote/environments/deepnoteEnvironment.ts
  • src/kernels/helpers.ts
  • src/platform/common/utils/localize.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
  • src/kernels/deepnote/types.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.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/logging/index.ts
  • src/platform/common/utils/localize.ts
src/platform/logging/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)

Implement logging using OutputChannelLogger/ConsoleLogger and honor LogLevel; ensure PII scrubbing in log output

Files:

  • src/platform/logging/index.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 have the .unit.test.ts 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/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.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 then property to avoid hanging tests

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
🧠 Learnings (21)
📚 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/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles Deepnote notebook state management

Applied to files:

  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.ts
  • src/kernels/deepnote/types.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookSelector.ts : deepnoteNotebookSelector.ts implements UI selection logic for Deepnote notebooks

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-10-07T15:24:14.050Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationTypes.ts:1-7
Timestamp: 2025-10-07T15:24:14.050Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required for new files and should not be suggested.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
📚 Learning: 2025-10-07T15:24:08.041Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationManager.ts:1-3
Timestamp: 2025-10-07T15:24:08.041Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required and should not be suggested for any files.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to **/*.{ts,tsx} : Do not add the Microsoft copyright header to new files

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
📚 Learning: 2025-10-07T16:30:58.450Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationWebview.ts:1-1
Timestamp: 2025-10-07T16:30:58.450Z
Learning: In the deepnote/vscode-deepnote repository, do not suggest adding Microsoft copyright headers or any copyright headers to files. The maintainers have explicitly declined this practice.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
📚 Learning: 2025-09-09T11:31:30.479Z
Learnt from: Artmann
PR: deepnote/vscode-extension#11
File: src/notebooks/deepnote/deepnoteActivationService.ts:1-3
Timestamp: 2025-09-09T11:31:30.479Z
Learning: New files in the Deepnote VS Code extension project should not include copyright headers.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/ipywidgets/serviceRegistry.{node,web}.ts : Register DI services in platform-specific registries (serviceRegistry.node.ts vs serviceRegistry.web.ts)

Applied to files:

  • src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:56:39.535Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel-jupyter.instructions.md:0-0
Timestamp: 2025-09-03T12:56:39.535Z
Learning: Applies to src/kernels/jupyter/serviceRegistry.{node,web}.ts : Use dependency injection via the service registry for component creation and registration

Applied to files:

  • src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-03T13:01:10.017Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/standalone.instructions.md:0-0
Timestamp: 2025-09-03T13:01:10.017Z
Learning: Applies to src/standalone/serviceRegistry.{node,web}.ts : Register new services for both platforms in serviceRegistry.node.ts and serviceRegistry.web.ts as appropriate

Applied to files:

  • src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-08-11T00:47:48.931Z
Learnt from: mfranczel
PR: deepnote/deepnote#18019
File: libs/orchestrator-node/src/lib/darwin/ray-cluster-manager.ts:118-125
Timestamp: 2025-08-11T00:47:48.931Z
Learning: In the Darwin Ray cluster manager (libs/orchestrator-node/src/lib/darwin/ray-cluster-manager.ts), when a cluster lacks the 'deepnote' tag, the executor connects to Darwin's JupyterLab using the full jupyterlabUrl (port 8888) passed as JUPYTER_URL. When the 'deepnote' tag is present, the executor constructs the connection from JUPYTER_HOST and JUPYTER_PORT (port 8899). The jupyterPort value is only used when useDeepnote is true.

Applied to files:

  • src/kernels/deepnote/types.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous with signature `public activate()` and returns void. It should be called without await in tests.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous and returns void, not a Promise. It should be called without await in tests.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • CLAUDE.md
📚 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/controllers/kernelSource/**/*.ts : For new kernel sources, implement ILocalNotebookKernelSourceSelector (or similar)

Applied to files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.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:

  • CLAUDE.md
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/** : Deepnote integration code resides under src/notebooks/deepnote/

Applied to files:

  • CLAUDE.md
📚 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:

  • CLAUDE.md
📚 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 **/*.{ts,tsx} : Use typed error classes from `src/platform/errors/` when throwing or handling errors

Applied to files:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/kernelErrorHandler.ts : kernelErrorHandler must provide platform-specific handling, user-friendly messages, and offer automatic dependency installation prompts

Applied to files:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
🧬 Code graph analysis (16)
src/kernels/deepnote/deepnoteServerStarter.node.ts (2)
src/kernels/deepnote/types.ts (2)
  • DeepnoteServerInfo (159-164)
  • DEEPNOTE_DEFAULT_PORT (326-326)
src/platform/common/types.ts (1)
  • IDisposable (211-213)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
src/kernels/deepnote/types.ts (1)
  • DeepnoteServerInfo (159-164)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1)
src/kernels/deepnote/types.ts (4)
  • IDeepnoteKernelAutoSelector (182-182)
  • IDeepnoteKernelAutoSelector (183-198)
  • IDeepnoteNotebookEnvironmentMapper (294-294)
  • IDeepnoteNotebookEnvironmentMapper (295-322)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts (3)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts (1)
  • DeepnoteEnvironmentTreeItem (19-154)
src/test/mocks/vsc/extHostedTypes.ts (1)
  • ThemeIcon (2094-2104)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (1)
src/kernels/deepnote/types.ts (5)
  • IDeepnoteToolkitInstaller (72-72)
  • IDeepnoteToolkitInstaller (73-126)
  • IDeepnoteServerStarter (128-128)
  • IDeepnoteServerStarter (129-157)
  • DeepnoteServerInfo (159-164)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.ts (2)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts (1)
  • DeepnoteEnvironmentTreeItem (19-154)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteEnvironmentManager (200-200)
  • IDeepnoteEnvironmentManager (201-222)
src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts (2)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteEnvironmentManager (200-200)
  • IDeepnoteEnvironmentManager (201-222)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.ts (1)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (2)
  • DeepnoteEnvironment (9-60)
  • DeepnoteEnvironmentState (66-76)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (2)
  • TreeItem (2066-2086)
  • ThemeIcon (2094-2104)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/notebooks/serviceRegistry.node.ts (1)
src/kernels/deepnote/types.ts (6)
  • IDeepnoteEnvironmentManager (200-200)
  • IDeepnoteEnvironmentManager (201-222)
  • IDeepnoteEnvironmentPicker (282-282)
  • IDeepnoteEnvironmentPicker (283-292)
  • IDeepnoteNotebookEnvironmentMapper (294-294)
  • IDeepnoteNotebookEnvironmentMapper (295-322)
src/kernels/deepnote/types.ts (3)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.ts (9)
  • getEnvironment (110-112)
  • getEnvironmentWithStatus (117-134)
  • updateEnvironment (139-162)
  • deleteEnvironment (167-183)
  • startServer (188-226)
  • stopServer (231-257)
  • restartServer (262-266)
  • updateLastUsed (271-279)
  • dispose (292-294)
src/kernels/deepnote/deepnoteServerStarter.node.ts (3)
  • startServer (78-117)
  • stopServer (123-147)
  • dispose (432-510)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.ts (3)
  • startServer (262-286)
  • stopServer (288-312)
  • restartServer (314-338)
src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts (1)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteEnvironmentManager (200-200)
  • IDeepnoteEnvironmentManager (201-222)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts (4)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.ts (1)
  • DeepnoteEnvironmentTreeDataProvider (12-141)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteEnvironmentManager (200-200)
  • IDeepnoteEnvironmentManager (201-222)
src/test/mocks/vsc/index.ts (1)
  • EventEmitter (65-89)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (2)
  • DeepnoteEnvironment (9-60)
  • DeepnoteEnvironmentWithStatus (116-119)
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts (1)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironmentState (66-76)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (2)
src/kernels/deepnote/types.ts (8)
  • IDeepnoteEnvironmentManager (200-200)
  • IDeepnoteEnvironmentManager (201-222)
  • IDeepnoteEnvironmentPicker (282-282)
  • IDeepnoteEnvironmentPicker (283-292)
  • IDeepnoteNotebookEnvironmentMapper (294-294)
  • IDeepnoteNotebookEnvironmentMapper (295-322)
  • DeepnoteKernelConnectionMetadata (15-70)
  • DEEPNOTE_NOTEBOOK_TYPE (327-327)
src/kernels/jupyter/types.ts (1)
  • JupyterServerProviderHandle (117-130)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (3)
src/platform/logging/index.ts (1)
  • logger (35-48)
src/kernels/execution/cellExecutionCreator.ts (1)
  • token (41-43)
src/test/mocks/vsc/index.ts (1)
  • CancellationToken (100-112)
🪛 ESLint
src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.ts

[error] 31-31: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 41-41: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 44-44: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 52-52: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 55-55: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)

src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.ts

[error] 89-89: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 96-96: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)

src/kernels/deepnote/environments/deepnoteEnvironmentStorage.ts

[error] 67-67: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 68-68: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 88-88: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)

src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts

[error] 103-103: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 104-104: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)

🪛 markdownlint-cli2 (0.18.1)
TODO.md

23-23: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


23-23: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


39-39: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


39-39: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


59-59: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


59-59: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


86-86: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


101-101: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


119-119: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


138-138: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


156-156: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


180-180: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


199-199: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

KERNEL_MANAGEMENT_VIEW_IMPLEMENTATION.md

198-198: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


208-208: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


222-222: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


261-261: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


297-297: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


297-297: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


401-401: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


414-414: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


458-458: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


458-458: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


486-486: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


516-516: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


517-517: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


536-536: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


537-537: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


596-596: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


597-597: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


653-653: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


654-654: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


675-675: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


687-687: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


698-698: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


711-711: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


722-722: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


734-734: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


746-746: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


758-758: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


781-781: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


785-785: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


794-794: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


799-799: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


805-805: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


825-825: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


831-831: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


837-837: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


847-847: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


852-852: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


858-858: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


866-866: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


872-872: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


878-878: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


893-893: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


898-898: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


988-988: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (17)
src/kernels/deepnote/deepnoteServerProvider.node.ts (1)

88-88: Approve expanded label with both ports
jupyterPort and lspPort are non-optional on DeepnoteServerInfo, so they’re always defined.

src/kernels/deepnote/deepnoteServerStarter.node.ts (1)

160-166: Correct interpreter usage — LGTM

You now launch with the venv’s Python and prepend its bin to PATH. This resolves the isolation issue.

Also applies to: 202-214

src/platform/common/utils/localize.ts (1)

42-43: Branding updates — LGTM

Channel and console labels updated to Deepnote.

Also applies to: 709-711

src/kernels/helpers.ts (1)

303-310: LGTM!

Display name logic for Deepnote kernels is correct. Fallback to oldDisplayName handles missing environmentName appropriately.

src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts (1)

13-14: Ensure mocked instances have undefined then property.

Per coding guidelines, when mocks return promises, the mocked instance should have then: undefined to avoid hanging tests.

As per coding guidelines.

Add after creating mocks:

 mockConfigManager = mock<IDeepnoteEnvironmentManager>();
 mockEnvironmentsView = mock<DeepnoteEnvironmentsView>();
+
+// Prevent hanging tests when promises are returned
+when(instance(mockConfigManager).then).thenReturn(undefined);
src/notebooks/serviceRegistry.node.ts (2)

50-54: LGTM!

Import statements follow the existing pattern and are organized correctly.

Also applies to: 61-66


152-169: LGTM!

Service registrations follow the DI pattern correctly. The comments clearly delineate the environment-related services.

src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts (1)

1-24: LGTM!

Constructor follows DI patterns correctly. The comment explaining why _environmentsView is injected but not stored is helpful.

src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts (1)

1-204: LGTM!

Comprehensive test coverage for storage operations. Good coverage of success, failure, and edge cases. Use of deepEqual for verification is appropriate.

src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (2)

10-18: Note: Extensive test coverage TODOs.

The TODOs indicate significant test gaps for command execution, CRUD operations, and selection logic. While acceptable for a draft PR, these should be addressed before merging.


62-84: LGTM!

Basic constructor and dispose tests are correct. The setup and teardown logic is appropriate.

src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts (1)

1-223: LGTM!

Comprehensive test coverage for the tree data provider. Tests cover root items, environment children, status handling, refresh mechanism, and disposal. Well-structured with appropriate test fixtures.

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

324-326: Staging wheel URL — confirm before merging to main

The wheel points to a staging bucket. Ensure this is intended for main or gate it by build channel.

Would you like me to add a configuration flag (e.g., “deepnote.toolkitChannel”: “stable|staging”) and switch the URL accordingly?

src/kernels/deepnote/environments/deepnoteEnvironmentsView.ts (4)

248-251: Ensure tree updates after mutations.

If the provider doesn’t auto-refresh on manager events, call refresh after create/start/stop/restart/delete/rename/package-update/switch.

Do we get environment change events wired to the tree provider? If not, add:

+this.treeDataProvider.refresh();

after each successful operation.

Also applies to: 281-282, 307-308, 333-334, 370-371, 404-405, 458-459, 590-591


1-2: Header policy conflict: confirm requirement for new files.

Guidelines say “Do not add the Microsoft copyright header to new files,” but this file has it.

Should we remove these lines to align with repo policy? As per coding guidelines.


59-137: package.json contributions verified: view deepnoteEnvironments is declared under contributes.views.deepnote, all deepnote.environments.* commands appear in contributes.commands and are referenced in contributes.menus.


499-507: No codicon changes needed
Both 'vm-running' and 'vm-outline' are valid built-in VS Code codicons.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

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

⚠️ Outside diff range comments (1)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)

296-306: Serialize ensureKernelSelected per notebook to avoid races/duplicate controllers.

Concurrent calls can create duplicate controllers and flicker. Gate by notebookKey with a pending map. As per coding guidelines

-    public async ensureKernelSelected(notebook: NotebookDocument, _token?: CancellationToken): Promise<void> {
-        return window.withProgress(
+    // Serialize work per notebook
+    private readonly pendingSetup = new Map<string, Promise<void>>();
+
+    public async ensureKernelSelected(notebook: NotebookDocument, _token?: CancellationToken): Promise<void> {
+        const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
+        const notebookKey = baseFileUri.fsPath;
+        const existing = this.pendingSetup.get(notebookKey);
+        if (existing) {
+            return existing;
+        }
+        const runner = window.withProgress(
             {
                 location: ProgressLocation.Notification,
-                title: 'Loading Deepnote Kernel',
+                title: l10n.t('Loading Deepnote Kernel'),
                 cancellable: true
             },
             async (progress, progressToken) => {
                 try {
                     logger.info(`Ensuring Deepnote kernel is selected for ${getDisplayPath(notebook.uri)}`);
-
-                    // Extract the base file URI (without query parameters)
-                    // Notebooks from the same .deepnote file have different URIs with ?notebook=id query params
-                    const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
-                    const notebookKey = baseFileUri.fsPath;
+                    // baseFileUri/notebookKey computed above
                     logger.info(`Base Deepnote file: ${getDisplayPath(baseFileUri)}`);
@@
-                    return;
+                    return;
                 } catch (ex) {
                     logger.error(`Failed to auto-select Deepnote kernel: ${ex}`);
                     throw ex;
                 }
             }
         );
+        this.pendingSetup.set(notebookKey, runner);
+        try {
+            await runner;
+        } finally {
+            // Only clear if still the same promise
+            if (this.pendingSetup.get(notebookKey) === runner) {
+                this.pendingSetup.delete(notebookKey);
+            }
+        }
     }

Also applies to: 309-360

♻️ Duplicate comments (2)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (2)

183-189: Don’t unregister shared environment-scoped servers on notebook close.

Server handles are per-environment; unregistering here can break other notebooks bound to the same environment. Only unregister if no other notebook still references this environment. Based on coding guidelines

-        // However, we do unregister the server from the provider to keep it clean
-        const serverHandle = this.notebookServerHandles.get(notebookKey);
-        if (serverHandle) {
-            logger.info(`Unregistering server for closed notebook: ${serverHandle}`);
-            this.serverProvider.unregisterServer(serverHandle);
-            this.notebookServerHandles.delete(notebookKey);
-        }
+        // Only unregister if no other notebook uses this environment.
+        const envId = this.notebookEnvironmentMapper.getEnvironmentForNotebook(baseFileUri);
+        const serverHandle = this.notebookServerHandles.get(notebookKey);
+        if (serverHandle && envId) {
+            const others = this.notebookEnvironmentMapper
+                .getNotebooksUsingEnvironment(envId)
+                .filter((u) => u.toString() !== baseFileUri.toString());
+            if (others.length === 0) {
+                logger.info(`Unregistering server for closed notebook: ${serverHandle}`);
+                this.serverProvider.unregisterServer(serverHandle);
+            } else {
+                logger.info(
+                    `Keeping server ${serverHandle} registered; env ${envId} still used by ${others.length} notebook(s).`
+                );
+            }
+        }
+        this.notebookServerHandles.delete(notebookKey);

15-17: Localize UI strings and avoid surfacing raw errors.

Wrap user-facing text with l10n.t() and show generic errors in UI; log details to logger. Replace window.showErrorMessage interpolation with a localized, neutral message. Based on coding guidelines

@@
-import {
-    CancellationToken,
+import {
+    CancellationToken,
@@
-    Disposable,
-    Uri
+    Disposable,
+    Uri,
+    l10n
 } from 'vscode';
@@
-        return window.withProgress(
+        // Compute key early so we can serialize work and display consistent title.
+        const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
+        const notebookKey = baseFileUri.fsPath;
+        return window.withProgress(
             {
                 location: ProgressLocation.Notification,
-                title: 'Loading Deepnote Kernel',
+                title: l10n.t('Loading Deepnote Kernel'),
                 cancellable: true
             },
@@
-                        progress.report({ message: 'Reusing existing kernel...' });
+                        progress.report({ message: l10n.t('Reusing existing kernel…') });
@@
-                    logger.info(`Checking for configuration selection for ${getDisplayPath(baseFileUri)}`);
+                    logger.info(`Checking for configuration selection for ${getDisplayPath(baseFileUri)}`);
@@
-                        progress.report({ message: 'Select kernel configuration...' });
+                        progress.report({ message: l10n.t('Select a Deepnote environment…') });
@@
-                        if (!selectedConfig) {
+                        if (!selectedConfig) {
                             logger.info(`User cancelled configuration selection - no kernel will be loaded`);
-                            throw new Error(
-                                'No environment selected. Please create an environment using the Deepnote Environments view.'
-                            );
+                            throw new Error(
+                                l10n.t('No environment selected. Create an environment in the Deepnote Environments view.')
+                            );
                         }
@@
-        progress.report({ message: `Using ${configuration.name}...` });
+        progress.report({ message: l10n.t('Using {0}…', configuration.name) });
@@
-        progress.report({ message: 'Starting Deepnote server...' });
+        progress.report({ message: l10n.t('Starting Deepnote server…') });
@@
-        if (!updatedConfig?.serverInfo) {
-            throw new Error('Failed to start server for configuration');
+        if (!updatedConfig?.serverInfo) {
+            throw new Error(l10n.t('Failed to start server for environment'));
         }
@@
-        progress.report({ message: 'Connecting to kernel...' });
+        progress.report({ message: l10n.t('Connecting to kernel…') });
@@
-        progress.report({ message: 'Finalizing kernel setup...' });
+        progress.report({ message: l10n.t('Finalizing kernel setup…') });
@@
-            progress.report({ message: 'Creating requirements.txt...' });
+            progress.report({ message: l10n.t('Creating requirements.txt…') });
@@
-        progress.report({ message: 'Kernel ready!' });
+        progress.report({ message: l10n.t('Kernel ready!') });
@@
-        void this.ensureKernelSelected(notebook).catch((error) => {
-            logger.error(`Failed to auto-select Deepnote kernel for ${getDisplayPath(notebook.uri)}: ${error}`);
-            void window.showErrorMessage(`Failed to load Deepnote kernel: ${error}`);
+        void this.ensureKernelSelected(notebook).catch((error) => {
+            logger.error(`Failed to auto-select Deepnote kernel for ${getDisplayPath(notebook.uri)}: ${error}`);
+            void window.showErrorMessage(l10n.t('Failed to load Deepnote kernel. See logs for details.'));
         });

Also applies to: 131-134, 296-306, 324-326, 362-394, 379-387, 424-429, 436-437, 465-474, 494-495, 521-523, 541-545, 585-586, 626-633

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 460ba6d and 3455f33.

📒 Files selected for processing (5)
  • TODO.md (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts (1 hunks)
  • src/kernels/deepnote/logs.txt (1 hunks)
  • src/notebooks/controllers/vscodeNotebookController.ts (1 hunks)
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/notebooks/controllers/vscodeNotebookController.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/notebooks/controllers/vscodeNotebookController.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
src/kernels/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)

src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
**/*.node.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: m1so
PR: deepnote/vscode-deepnote#25
File: src/kernels/deepnote/deepnoteServerStarter.node.ts:150-154
Timestamp: 2025-10-07T19:51:40.201Z
Learning: The deepnote-toolkit server is a separate Python package maintained in a different repository from the VS Code extension (vscode-deepnote). The VS Code extension launches the toolkit as a subprocess and communicates via environment variables and HTTP. The toolkit implementation cannot be verified from the extension codebase.
📚 Learning: 2025-10-07T15:24:14.050Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationTypes.ts:1-7
Timestamp: 2025-10-07T15:24:14.050Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required for new files and should not be suggested.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
📚 Learning: 2025-10-07T15:24:08.041Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationManager.ts:1-3
Timestamp: 2025-10-07T15:24:08.041Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required and should not be suggested for any files.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to **/*.{ts,tsx} : Do not add the Microsoft copyright header to new files

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
📚 Learning: 2025-10-07T16:30:58.450Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationWebview.ts:1-1
Timestamp: 2025-10-07T16:30:58.450Z
Learning: In the deepnote/vscode-deepnote repository, do not suggest adding Microsoft copyright headers or any copyright headers to files. The maintainers have explicitly declined this practice.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
📚 Learning: 2025-09-09T11:31:30.479Z
Learnt from: Artmann
PR: deepnote/vscode-extension#11
File: src/notebooks/deepnote/deepnoteActivationService.ts:1-3
Timestamp: 2025-09-09T11:31:30.479Z
Learning: New files in the Deepnote VS Code extension project should not include copyright headers.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.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 **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote

Applied to files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles Deepnote notebook state management

Applied to files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
🧬 Code graph analysis (3)
src/notebooks/controllers/vscodeNotebookController.ts (2)
src/kernels/helpers.ts (2)
  • areKernelConnectionsEqual (586-600)
  • getDisplayNameOrNameOfKernelConnection (250-313)
src/platform/common/platform/fs-paths.ts (1)
  • getDisplayPath (33-74)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (2)
  • TreeItem (2066-2086)
  • ThemeIcon (2094-2104)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (4)
src/kernels/deepnote/types.ts (8)
  • IDeepnoteEnvironmentManager (200-200)
  • IDeepnoteEnvironmentManager (201-222)
  • IDeepnoteEnvironmentPicker (282-282)
  • IDeepnoteEnvironmentPicker (283-292)
  • IDeepnoteNotebookEnvironmentMapper (294-294)
  • IDeepnoteNotebookEnvironmentMapper (295-322)
  • DeepnoteKernelConnectionMetadata (15-70)
  • DEEPNOTE_NOTEBOOK_TYPE (327-327)
src/kernels/jupyter/types.ts (1)
  • JupyterServerProviderHandle (117-130)
src/kernels/jupyter/jupyterUtils.ts (1)
  • createJupyterConnectionInfo (83-179)
src/kernels/jupyter/session/jupyterLabHelper.ts (1)
  • JupyterLabHelper (28-251)
🪛 ESLint
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.ts

[error] 100-100: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)


[error] 101-101: fsPath is not allowed in anything but .node files

(local-rules/dont-use-fspath)

🪛 markdownlint-cli2 (0.18.1)
TODO.md

169-169: Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


181-181: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

@tkislan tkislan self-assigned this Oct 23, 2025
…nt management

This commit removes the deprecated `ensureInstalled` method from the `DeepnoteToolkitInstaller` class, streamlining the installation process by encouraging the use of `ensureVenvAndToolkit`. Additionally, the `logs.txt` file has been deleted as it is no longer necessary for the current implementation.

Changes include:
- Removal of the legacy method to enhance code clarity and maintainability.
- Deletion of the logs file to reduce clutter in the repository.

These changes contribute to a cleaner codebase and improved user experience when managing Deepnote environments.

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Caution

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

⚠️ Outside diff range comments (4)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (2)

121-123: Normalize notebook keys; stop using fsPath for identifiers.

Use a normalized URI string key to support remote/virtual schemes.

-        const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
-        const notebookKey = baseFileUri.fsPath;
+        const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
+        const notebookKey = this.normalizeKey(baseFileUri);
@@
-        const baseFileUri = event.notebook.uri.with({ query: '', fragment: '' });
-        const notebookKey = baseFileUri.fsPath;
+        const baseFileUri = event.notebook.uri.with({ query: '', fragment: '' });
+        const notebookKey = this.normalizeKey(baseFileUri);
@@
-        const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
-        const notebookKey = baseFileUri.fsPath;
+        const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
+        const notebookKey = this.normalizeKey(baseFileUri);
@@
-        const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
-        const notebookKey = baseFileUri.fsPath;
+        const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
+        const notebookKey = this.normalizeKey(baseFileUri);
@@
-                    const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
-                    const notebookKey = baseFileUri.fsPath;
+                    const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
+                    const notebookKey = this.normalizeKey(baseFileUri);

Add helper:

@@
 export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector, IExtensionSyncActivationService {
@@
+    private normalizeKey(uri: Uri): string {
+        return uri.with({ query: '', fragment: '' }).toString(true);
+    }

Also applies to: 148-153, 176-181, 262-266, 310-313


184-190: Don’t unregister an environment-scoped server if other notebooks still use it.

Scan other notebook entries for the same handle before unregistering.

-        const serverHandle = this.notebookServerHandles.get(notebookKey);
-        if (serverHandle) {
-            logger.info(`Unregistering server for closed notebook: ${serverHandle}`);
-            this.serverProvider.unregisterServer(serverHandle);
-            this.notebookServerHandles.delete(notebookKey);
-        }
+        const serverHandle = this.notebookServerHandles.get(notebookKey);
+        if (serverHandle) {
+            const stillUsed = Array.from(this.notebookServerHandles.entries()).some(
+                ([key, handle]) => key !== notebookKey && handle === serverHandle
+            );
+            if (!stillUsed) {
+                logger.info(`Unregistering server for closed notebook: ${serverHandle}`);
+                this.serverProvider.unregisterServer(serverHandle);
+            } else {
+                logger.info(`Keeping server ${serverHandle} registered; still used by other notebook(s).`);
+            }
+            this.notebookServerHandles.delete(notebookKey);
+        }
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)

84-92: Log caught exceptions for debugging.

The catch block silently swallows errors from pending installations. At minimum, log the caught exception to aid debugging when installations fail repeatedly.

Apply this diff:

         } catch {
             // If the previous installation failed, continue to retry
-            logger.info(`Previous installation for ${venvKey} failed, retrying...`);
+            logger.warn(`Previous installation for ${venvKey} failed, retrying...`, ex);
         }

113-120: Log caught exceptions for debugging.

Same issue as lines 84-92: catch block swallows error details.

Apply this diff:

         } catch {
-            logger.info(`Concurrent installation for ${venvKey} failed, retrying...`);
+            logger.warn(`Concurrent installation for ${venvKey} failed, retrying...`, ex);
         }
♻️ Duplicate comments (19)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)

62-76: Persist URI strings, not fsPaths; keep legacy fields for back-compat.

Switch to pythonInterpreterUri/venvUri (URI strings). Retain old fields temporarily for migration.

- * Uses string paths instead of Uri objects for JSON serialization.
+ * Uses URI strings instead of Uri objects for JSON serialization (back-compat fields retained).
 export interface DeepnoteEnvironmentState {
     id: string;
     name: string;
-    pythonInterpreterPath: string;
-    venvPath: string;
+    pythonInterpreterUri: string;
+    venvUri: string;
+    /** @deprecated use pythonInterpreterUri */
+    pythonInterpreterPath?: string;
+    /** @deprecated use venvUri */
+    venvPath?: string;
     createdAt: string;
     lastUsedAt: string;
     packages?: string[];
     toolkitVersion?: string;
     description?: string;
 }

As per coding guidelines

src/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.ts (2)

63-75: Serialize URIs, not fsPaths (write new + legacy keys).

Store toString(true) URIs; include legacy keys for a safe rollout.

     private serializeEnvironment(config: DeepnoteEnvironment): DeepnoteEnvironmentState {
-        return {
+        const uriState = {
             id: config.id,
             name: config.name,
-            pythonInterpreterPath: config.pythonInterpreter.uri.fsPath,
-            venvPath: config.venvPath.fsPath,
+            pythonInterpreterUri: config.pythonInterpreter.uri.toString(true),
+            venvUri: config.venvPath.toString(true),
             createdAt: config.createdAt.toISOString(),
             lastUsedAt: config.lastUsedAt.toISOString(),
             packages: config.packages,
             toolkitVersion: config.toolkitVersion,
             description: config.description
         };
+        // Back-compat (can be removed after migration)
+        return {
+            ...uriState,
+            pythonInterpreterPath: config.pythonInterpreter.uri.fsPath,
+            venvPath: config.venvPath.fsPath
+        } as any;
     }

As per coding guidelines


80-107: Deserialize with URI-first, legacy-path fallback; use URI as interpreter id.

Ensures cross-scheme support and stable IDs.

     private deserializeEnvironment(state: DeepnoteEnvironmentState): DeepnoteEnvironment | undefined {
         try {
-            const interpreterUri = Uri.file(state.pythonInterpreterPath);
+            const interpreterUri = (state as any).pythonInterpreterUri
+                ? Uri.parse((state as any).pythonInterpreterUri, true)
+                : Uri.file((state as any).pythonInterpreterPath);
...
             const interpreter: PythonEnvironment = {
-                uri: interpreterUri,
-                id: interpreterUri.fsPath
+                uri: interpreterUri,
+                id: interpreterUri.toString(true)
             };
 
             return {
                 id: state.id,
                 name: state.name,
                 pythonInterpreter: interpreter,
-                venvPath: Uri.file(state.venvPath),
+                venvPath: (state as any).venvUri
+                    ? Uri.parse((state as any).venvUri, true)
+                    : Uri.file((state as any).venvPath),
                 createdAt: new Date(state.createdAt),
                 lastUsedAt: new Date(state.lastUsedAt),
                 packages: state.packages,
                 toolkitVersion: state.toolkitVersion,
                 description: state.description,
                 serverInfo: undefined // Don't persist server info across sessions
             };

As per coding guidelines

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

239-249: Add CancellationToken to environment server ops.

Propagate cancellation from UI through manager APIs.

-    startServer(id: string): Promise<void>;
+    startServer(id: string, token?: vscode.CancellationToken): Promise<void>;
@@
-    stopServer(id: string): Promise<void>;
+    stopServer(id: string, token?: vscode.CancellationToken): Promise<void>;
@@
-    restartServer(id: string): Promise<void>;
+    restartServer(id: string, token?: vscode.CancellationToken): Promise<void>;

Based on learnings


137-142: Support cancellation on stopServer.

Mirror startServer’s token on stopServer too.

-    stopServer(environmentId: string): Promise<void>;
+    stopServer(environmentId: string, token?: vscode.CancellationToken): Promise<void>;

Based on learnings

src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts (2)

22-44: Replace setTimeout sleeps with direct verification.

activate() fire-and-forget; just verify the call. Removes flakiness.

-        test('should call initialize on environment manager', async () => {
+        test('should call initialize on environment manager', () => {
             when(mockConfigManager.initialize()).thenResolve();
 
             activationService.activate();
 
-            // Wait for async initialization
-            await new Promise((resolve) => setTimeout(resolve, 100));
-
             verify(mockConfigManager.initialize()).once();
         });
@@
-        test('should handle initialization errors gracefully', async () => {
+        test('should handle initialization errors gracefully', () => {
             when(mockConfigManager.initialize()).thenReject(new Error('Initialization failed'));
 
-            // Should not throw
-            activationService.activate();
-
-            // Wait for async initialization
-            await new Promise((resolve) => setTimeout(resolve, 100));
+            // Should not throw
+            activationService.activate();
 
             verify(mockConfigManager.initialize()).once();
         });

60-76: Remove duplicate constructor tests.

Consolidate into one constructor test.

-        test('should accept environment manager', () => {
-            const service = new DeepnoteEnvironmentsActivationService(
-                instance(mockConfigManager),
-                instance(mockEnvironmentsView)
-            );
-
-            assert.ok(service);
-        });
-
-        test('should accept environments view', () => {
+        test('should construct with dependencies', () => {
             const service = new DeepnoteEnvironmentsActivationService(
                 instance(mockConfigManager),
                 instance(mockEnvironmentsView)
             );
 
             assert.ok(service);
         });
src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts (1)

26-37: Avoid double init; improve error logging.

  • This activates environments here while the manager is also bound as an activation service in the registry, causing two concurrent initialize() calls. Drop one activation binding (see serviceRegistry.node.ts lines 152-161).
  • Log the error object, not string interpolation, to preserve stack/PII‑scrubbed details.

Apply this diff for logging:

-            (error: unknown) => {
-                logger.error(`Failed to initialize Deepnote kernel environments: ${error}`);
-            }
+            (error: unknown) => {
+                logger.error('Failed to initialize Deepnote kernel environments', error);
+            }
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (2)

130-135: Avoid showing raw error text to users; log full error and show a generic message.

Current message leaks error strings.

-        void this.ensureKernelSelected(notebook).catch((error) => {
-            logger.error(`Failed to auto-select Deepnote kernel for ${getDisplayPath(notebook.uri)}: ${error}`);
-            void window.showErrorMessage(`Failed to load Deepnote kernel: ${error}`);
-        });
+        void this.ensureKernelSelected(notebook).catch((error) => {
+            logger.error(`Failed to auto-select Deepnote kernel for ${getDisplayPath(notebook.uri)}`, error);
+            void window.showErrorMessage('Failed to load Deepnote kernel. See output for details.');
+        });

297-306: Serialize ensureKernelSelected per notebook to prevent duplicate setup.

Guard concurrent calls with a pending map.

 export class DeepnoteKernelAutoSelector implements IDeepnoteKernelAutoSelector, IExtensionSyncActivationService {
@@
     private readonly loadingControllers = new Map<string, NotebookController>();
+    private readonly pendingSetup = new Map<string, Promise<void>>();
@@
-    public async ensureKernelSelected(notebook: NotebookDocument, _token?: CancellationToken): Promise<void> {
-        return window.withProgress(
+    public async ensureKernelSelected(notebook: NotebookDocument, _token?: CancellationToken): Promise<void> {
+        const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
+        const notebookKey = this.normalizeKey(baseFileUri);
+        const inFlight = this.pendingSetup.get(notebookKey);
+        if (inFlight) return inFlight;
+        const work = window.withProgress(
             {
                 location: ProgressLocation.Notification,
                 title: 'Loading Deepnote Kernel',
                 cancellable: true
             },
             async (progress, progressToken) => {
@@
-                    const baseFileUri = notebook.uri.with({ query: '', fragment: '' });
-                    const notebookKey = baseFileUri.fsPath;
+                    // baseFileUri/notebookKey computed above

And at the end of the method:

-        );
+        );
+        this.pendingSetup.set(notebookKey, work);
+        try {
+            return await work;
+        } finally {
+            if (this.pendingSetup.get(notebookKey) === work) this.pendingSetup.delete(notebookKey);
+        }

Also applies to: 308-313

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

15-18: Normalize URIs; stop using fsPath for keys/logs (breaks non-file schemes).

Store normalized URI strings (strip query/fragment) and parse with Uri.parse when reading. Current fsPath/Uri.file usage is incorrect for remote/vscode-remote schemes.

@@
-    private mappings: Map<string, string>; // notebookUri.fsPath -> environmentId
+    private mappings: Map<string, string>; // normalized notebook URI (string) -> environmentId
@@
-    public getEnvironmentForNotebook(notebookUri: Uri): string | undefined {
-        const key = notebookUri.fsPath;
-        return this.mappings.get(key);
-    }
+    public getEnvironmentForNotebook(notebookUri: Uri): string | undefined {
+        const key = this.normalizeKey(notebookUri);
+        return this.mappings.get(key);
+    }
@@
-    public async setEnvironmentForNotebook(notebookUri: Uri, environmentId: string): Promise<void> {
-        const key = notebookUri.fsPath;
+    public async setEnvironmentForNotebook(notebookUri: Uri, environmentId: string): Promise<void> {
+        const key = this.normalizeKey(notebookUri);
         this.mappings.set(key, environmentId);
         await this.saveMappings();
-        logger.info(`Mapped notebook ${notebookUri.fsPath} to environment ${environmentId}`);
+        logger.info(`Mapped notebook ${key} to environment ${environmentId}`);
     }
@@
-    public async removeEnvironmentForNotebook(notebookUri: Uri): Promise<void> {
-        const key = notebookUri.fsPath;
+    public async removeEnvironmentForNotebook(notebookUri: Uri): Promise<void> {
+        const key = this.normalizeKey(notebookUri);
         this.mappings.delete(key);
         await this.saveMappings();
-        logger.info(`Removed environment mapping for notebook ${notebookUri.fsPath}`);
+        logger.info(`Removed environment mapping for notebook ${key}`);
     }
@@
-    public getNotebooksUsingEnvironment(environmentId: string): Uri[] {
+    public getNotebooksUsingEnvironment(environmentId: string): Uri[] {
         const notebooks: Uri[] = [];
         for (const [notebookPath, configId] of this.mappings.entries()) {
             if (configId === environmentId) {
-                notebooks.push(Uri.file(notebookPath));
+                notebooks.push(Uri.parse(notebookPath, true));
             }
         }
         return notebooks;
     }
+    private normalizeKey(notebookUri: Uri): string {
+        const normalized = notebookUri.with({ query: '', fragment: '' });
+        return normalized.toString(true);
+    }

Also applies to: 30-33, 40-45, 51-56, 63-71

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

31-47: Inject the tree data provider via DI (no new).

Wire the provider through inversify; aids testing and consistency.

-    constructor(
+    constructor(
         @inject(IDeepnoteEnvironmentManager) private readonly environmentManager: IDeepnoteEnvironmentManager,
         @inject(IPythonApiProvider) private readonly pythonApiProvider: IPythonApiProvider,
         @inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry,
         @inject(IDeepnoteKernelAutoSelector) private readonly kernelAutoSelector: IDeepnoteKernelAutoSelector,
         @inject(IDeepnoteNotebookEnvironmentMapper)
         private readonly notebookEnvironmentMapper: IDeepnoteNotebookEnvironmentMapper,
-        @inject(IKernelProvider) private readonly kernelProvider: IKernelProvider
+        @inject(IKernelProvider) private readonly kernelProvider: IKernelProvider,
+        @inject(DeepnoteEnvironmentTreeDataProvider)
+        treeDataProvider: DeepnoteEnvironmentTreeDataProvider
     ) {
-        // Create tree data provider
-        this.treeDataProvider = new DeepnoteEnvironmentTreeDataProvider(environmentManager);
+        // Use injected provider
+        this.treeDataProvider = treeDataProvider;

246-259: Don’t surface raw error objects to users; log details, show generic message.

Avoid ${error} in UI; it can leak PII/stacks. Log the error object, show a friendly message.

-                        void window.showInformationMessage(`Environment "${name}" created successfully!`);
+                        void window.showInformationMessage(`Environment "${name}" created successfully!`);
                     } catch (error) {
-                        logger.error(`Failed to create environment: ${error}`);
-                        throw error;
+                        logger.error('Failed to create environment', error);
+                        throw error;
                     }
@@
-        } catch (error) {
-            void window.showErrorMessage(`Failed to create environment: ${error}`);
+        } catch (error) {
+            logger.error('Failed to create environment', error);
+            void window.showErrorMessage('Failed to create environment. See output for details.');
         }

Apply the same pattern to start/stop/restart/delete/rename/update/select flows at the listed ranges.

Also applies to: 281-285, 309-311, 335-337, 372-374, 406-408, 458-462, 591-594


230-237: Make long operations cancellable and plumb CancellationToken.

Mark progress as cancellable and honor the token.

-            await window.withProgress(
-                {
-                    location: ProgressLocation.Notification,
-                    title: `Creating environment "${name}"...`,
-                    cancellable: false
-                },
-                async (progress: { report: (value: { message?: string; increment?: number }) => void }) => {
+            await window.withProgress(
+                {
+                    location: ProgressLocation.Notification,
+                    title: `Creating environment "${name}"...`,
+                    cancellable: true
+                },
+                async (progress, token) => {
+                    if (token.isCancellationRequested) { throw new Error('Cancelled'); }
                     progress.report({ message: 'Setting up virtual environment...' });
                     // consider passing `token` into manager methods once supported

Apply similarly to other ranges.

Also applies to: 269-276, 295-302, 321-328, 358-366, 446-454, 569-576


196-212: PEP 508: current package validation blocks valid specs (versions/extras/markers).

Loosen or remove validation; let installer report errors.

-                    // Basic validation: check for valid package names
-                    const packages = value.split(',').map((p: string) => p.trim());
-                    for (const pkg of packages) {
-                        if (!/^[a-zA-Z0-9_\-\[\]]+$/.test(pkg)) {
-                            return `Invalid package name: ${pkg}`;
-                        }
-                    }
+                    // Minimal PEP 508-ish validation; allow names, extras, and version specifiers.
+                    const packages = value.split(',').map((p: string) => p.trim());
+                    const re = /^[A-Za-z0-9._\-]+(\[[A-Za-z0-9_,.\-]+\])?(?:\s*(==|>=|<=|~=|>|<)\s*[A-Za-z0-9.*+!\-_.]+)?(?:\s*;.+)?$/;
+                    for (const pkg of packages) {
+                        if (!re.test(pkg)) {
+                            return `Invalid package specifier: ${pkg}`;
+                        }
+                    }

Also applies to: 423-433

src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (3)

143-146: Use typed errors for not-found cases.

Prefer platform typed errors (e.g., NotFoundError) over generic Error.

+import { NotFoundError } from '../../../platform/errors';
@@
-        if (!config) {
-            throw new Error(`Environment not found: ${id}`);
-        }
+        if (!config) {
+            throw new NotFoundError('Environment', id);
+        }

Apply to the other not-found sites as well.

Also applies to: 168-171, 189-192


271-279: Emit change event after updating last-used timestamp.

Keeps UI lists (e.g., “Last used”) in sync.

         config.lastUsedAt = new Date();
         await this.persistEnvironments();
+        this._onDidChangeEnvironments.fire();

197-216: Critical: server starts with base interpreter; must use venv interpreter returned by ensureVenvAndToolkit.

Pass the venv Python to serverStarter.startServer.

-            // First ensure venv is created and toolkit is installed
-            await this.toolkitInstaller.ensureVenvAndToolkit(config.pythonInterpreter, config.venvPath, undefined);
+            // First ensure venv is created and toolkit is installed; capture venv interpreter
+            const venvInterpreter = await this.toolkitInstaller.ensureVenvAndToolkit(
+                config.pythonInterpreter,
+                config.venvPath,
+                undefined
+            );
+            if (!venvInterpreter) {
+                throw new Error(`Failed to prepare virtual environment for ${config.name} (${id})`);
+            }
@@
-            const serverInfo = await this.serverStarter.startServer(
-                config.pythonInterpreter,
+            const serverInfo = await this.serverStarter.startServer(
+                venvInterpreter,
                 config.venvPath,
                 id,
                 undefined
             );

Also applies to: 205-213

src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)

144-185: Apply typed errors and localization from previous review.

The same issues flagged in the previous review remain unaddressed:

  • Line 155: Raw Error instead of typed error class
  • Lines 159, 179, 182: Unlocalized user-facing strings
  • Line 181: String interpolation in logger instead of passing exception object

As per coding guidelines (typed errors, l10n.t()).

Apply this diff:

+import { UserError } from '../../platform/errors/userError';
+import { l10n } from 'vscode';

     const venvInterpreter = await this.getVenvInterpreterByPath(venvPath);
     if (!venvInterpreter) {
-        throw new Error(`Venv not found at ${venvPath.fsPath}`);
+        throw new UserError(l10n.t('Venv not found at {0}', venvPath.fsPath));
     }

     logger.info(`Installing additional packages in ${venvPath.fsPath}: ${packages.join(', ')}`);
-    this.outputChannel.appendLine(`Installing packages: ${packages.join(', ')}...`);
+    this.outputChannel.appendLine(l10n.t('Installing packages: {0}...', packages.join(', ')));

     try {
         Cancellation.throwIfCanceled(token);
         
         const venvProcessService = await this.processServiceFactory.create(undefined);
         const installResult = await venvProcessService.exec(
             venvInterpreter.uri.fsPath,
             ['-m', 'pip', 'install', '--upgrade', ...packages],
             { throwOnStdErr: false }
         );

         if (installResult.stdout) {
             this.outputChannel.appendLine(installResult.stdout);
         }
         if (installResult.stderr) {
             this.outputChannel.appendLine(installResult.stderr);
         }

         logger.info('Additional packages installed successfully');
-        this.outputChannel.appendLine('✓ Packages installed successfully');
+        this.outputChannel.appendLine(l10n.t('✓ Packages installed successfully'));
     } catch (ex) {
-        logger.error(`Failed to install additional packages: ${ex}`);
-        this.outputChannel.appendLine(`✗ Failed to install packages: ${ex}`);
+        logger.error('Failed to install additional packages', ex);
+        this.outputChannel.appendLine(l10n.t('✗ Failed to install packages: {0}', ex));
         throw ex;
     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3455f33 and 9cd81ae.

📒 Files selected for processing (19)
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (8 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironment.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts (1 hunks)
  • src/kernels/deepnote/types.ts (8 hunks)
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (5 hunks)
  • src/notebooks/serviceRegistry.node.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironment.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts
  • src/kernels/deepnote/types.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironment.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts
  • src/notebooks/serviceRegistry.node.ts
  • src/kernels/deepnote/types.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 have the .unit.test.ts 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/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts
src/kernels/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)

src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironment.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts
  • src/kernels/deepnote/types.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 then property to avoid hanging tests

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts
**/*.node.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts
  • src/notebooks/serviceRegistry.node.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
🧠 Learnings (28)
📓 Common learnings
Learnt from: m1so
PR: deepnote/vscode-deepnote#25
File: src/kernels/deepnote/deepnoteServerStarter.node.ts:150-154
Timestamp: 2025-10-07T19:51:40.201Z
Learning: The deepnote-toolkit server is a separate Python package maintained in a different repository from the VS Code extension (vscode-deepnote). The VS Code extension launches the toolkit as a subprocess and communicates via environment variables and HTTP. The toolkit implementation cannot be verified from the extension codebase.
📚 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/**/{fileSystem.ts,fileSystem.node.ts,platformService.web.ts,platformService.node.ts} : Use URI-based file operations and vscode-path for cross-platform path handling in file system and platform service implementations

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironment.ts
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.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 **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-09-03T12:56:39.535Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel-jupyter.instructions.md:0-0
Timestamp: 2025-09-03T12:56:39.535Z
Learning: Applies to src/kernels/jupyter/{connection,launcher,session,finder}/**/*.ts : Support cancellation via CancellationToken (or equivalent) for long-running async operations (network calls, startup waits)

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
  • src/kernels/deepnote/types.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 : Respect CancellationToken in async operations and support cancellation in long-running tasks

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
  • src/kernels/deepnote/types.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/**/*.ts : Respect CancellationToken in all async operations

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.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 **/*.{ts,tsx} : Prefer async/await and handle cancellation with `CancellationToken`

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
📚 Learning: 2025-09-10T08:51:54.455Z
Learnt from: dinohamzic
PR: deepnote/deepnote#18378
File: apps/webapp/server/api/landing.ts:130-136
Timestamp: 2025-09-10T08:51:54.455Z
Learning: In the Deepnote codebase, prefer explicit error messages over using raw error messages (like err.message) when handling errors that are exposed to customers, for security and user experience reasons.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.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 **/*.{ts,tsx} : Preserve error details while scrubbing PII in messages and telemetry

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
📚 Learning: 2025-10-17T07:48:22.500Z
Learnt from: hc2p
PR: deepnote/vscode-deepnote#52
File: src/kernels/deepnote/environments/deepnoteEnvironmentsView.ts:144-146
Timestamp: 2025-10-17T07:48:22.500Z
Learning: Skip localization (l10n) feedback for this project. The team is not focusing on l10n currently.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Use dependency injection with inversify

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to **/*.{ts,tsx} : Use Uri.joinPath() to construct file paths instead of manual string concatenation

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous with signature `public activate()` and returns void. It should be called without await in tests.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous and returns void, not a Promise. It should be called without await in tests.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
📚 Learning: 2025-09-12T07:57:49.414Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:48-66
Timestamp: 2025-09-12T07:57:49.414Z
Learning: The activate() method in DeepnoteActivationService returns void (synchronous), not Promise<void>, so tests should not await it or be marked as async.

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote

Applied to files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.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/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.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 **/*.{ts,tsx} : Use typed error classes from `src/platform/errors/` when throwing or handling errors

Applied to files:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/kernelErrorHandler.ts : kernelErrorHandler must provide platform-specific handling, user-friendly messages, and offer automatic dependency installation prompts

Applied to files:

  • src/kernels/deepnote/deepnoteToolkitInstaller.node.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/controllers/kernelSource/**/*.ts : For new kernel sources, implement ILocalNotebookKernelSourceSelector (or similar)

Applied to files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookSelector.ts : deepnoteNotebookSelector.ts implements UI selection logic for Deepnote notebooks

Applied to files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles Deepnote notebook state management

Applied to files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts
  • src/kernels/deepnote/types.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/kernelProvider.base.ts : KernelProviderBase must create/manage IKernel instances, track by NotebookDocument, manage lifecycle (start/restart/dispose), and expose INotebookKernelExecution

Applied to files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.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/controllers/kernelSource/**/*.ts : Create a UI provider for kernel selection for the new kernel source

Applied to files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts contains Deepnote-related type definitions

Applied to files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/ipywidgets/serviceRegistry.{node,web}.ts : Register DI services in platform-specific registries (serviceRegistry.node.ts vs serviceRegistry.web.ts)

Applied to files:

  • src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:56:39.535Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel-jupyter.instructions.md:0-0
Timestamp: 2025-09-03T12:56:39.535Z
Learning: Applies to src/kernels/jupyter/serviceRegistry.{node,web}.ts : Use dependency injection via the service registry for component creation and registration

Applied to files:

  • src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-08-11T00:47:48.931Z
Learnt from: mfranczel
PR: deepnote/deepnote#18019
File: libs/orchestrator-node/src/lib/darwin/ray-cluster-manager.ts:118-125
Timestamp: 2025-08-11T00:47:48.931Z
Learning: In the Darwin Ray cluster manager (libs/orchestrator-node/src/lib/darwin/ray-cluster-manager.ts), when a cluster lacks the 'deepnote' tag, the executor connects to Darwin's JupyterLab using the full jupyterlabUrl (port 8888) passed as JUPYTER_URL. When the 'deepnote' tag is present, the executor constructs the connection from JUPYTER_HOST and JUPYTER_PORT (port 8899). The jupyterPort value is only used when useDeepnote is true.

Applied to files:

  • src/kernels/deepnote/types.ts
🧬 Code graph analysis (14)
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts (1)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironmentState (66-76)
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.ts (2)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1)
  • injectable (20-295)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (2)
  • DeepnoteEnvironment (9-60)
  • DeepnoteEnvironmentState (66-76)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts (2)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts (1)
  • DeepnoteEnvironmentTreeItem (17-137)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteEnvironmentManager (191-191)
  • IDeepnoteEnvironmentManager (192-265)
src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.unit.test.ts (1)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteEnvironmentManager (191-191)
  • IDeepnoteEnvironmentManager (192-265)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
src/kernels/deepnote/types.ts (1)
  • DeepnoteServerInfo (150-155)
src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts (3)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1)
  • injectable (20-295)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteEnvironmentManager (191-191)
  • IDeepnoteEnvironmentManager (192-265)
src/platform/logging/index.ts (1)
  • logger (35-48)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts (2)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts (1)
  • getDeepnoteEnvironmentStatusVisual (11-55)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1)
src/kernels/deepnote/types.ts (6)
  • IDeepnoteEnvironmentManager (191-191)
  • IDeepnoteEnvironmentManager (192-265)
  • IDeepnoteKernelAutoSelector (173-173)
  • IDeepnoteKernelAutoSelector (174-189)
  • IDeepnoteNotebookEnvironmentMapper (277-277)
  • IDeepnoteNotebookEnvironmentMapper (278-305)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (1)
src/kernels/deepnote/types.ts (5)
  • IDeepnoteToolkitInstaller (77-77)
  • IDeepnoteToolkitInstaller (78-117)
  • IDeepnoteServerStarter (119-119)
  • IDeepnoteServerStarter (120-148)
  • DeepnoteServerInfo (150-155)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts (3)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts (1)
  • DeepnoteEnvironmentTreeItem (17-137)
src/test/mocks/vsc/extHostedTypes.ts (1)
  • ThemeIcon (2094-2104)
src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts (3)
src/kernels/deepnote/environments/deepnoteEnvironmentsActivationService.ts (1)
  • injectable (14-39)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteEnvironmentManager (191-191)
  • IDeepnoteEnvironmentManager (192-265)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts (4)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts (1)
  • DeepnoteEnvironmentTreeDataProvider (12-146)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteEnvironmentManager (191-191)
  • IDeepnoteEnvironmentManager (192-265)
src/test/mocks/vsc/index.ts (1)
  • EventEmitter (65-89)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (2)
  • DeepnoteEnvironment (9-60)
  • DeepnoteEnvironmentWithStatus (116-119)
src/notebooks/serviceRegistry.node.ts (1)
src/kernels/deepnote/types.ts (6)
  • IDeepnoteEnvironmentManager (191-191)
  • IDeepnoteEnvironmentManager (192-265)
  • IDeepnoteEnvironmentPicker (267-267)
  • IDeepnoteEnvironmentPicker (268-275)
  • IDeepnoteNotebookEnvironmentMapper (277-277)
  • IDeepnoteNotebookEnvironmentMapper (278-305)
src/kernels/deepnote/types.ts (1)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (3)
  • CreateDeepnoteEnvironmentOptions (81-86)
  • DeepnoteEnvironment (9-60)
  • DeepnoteEnvironmentWithStatus (116-119)
🔇 Additional comments (8)
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts (1)

69-97: LGTM: loads entries without eager validation.

Deserialization without path validation is appropriate here; validation belongs at use time.

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

307-309: Confirm staging URL intent and versioning strategy.

The staging URL/version are hardcoded and isolated to this module with no config override or activation-time validation. If this is intentional for development, document why. If production readiness is planned, implement config sourcing (workspace settings or extension config) and validate during server initialization.

src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts (1)

23-61: Solid coverage.

Status/label/icon/tooltip and relative-time checks look good.

Also applies to: 62-93, 105-117, 118-133, 167-186, 188-253

src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (1)

48-71: Comprehensive and clear.

Good coverage across lifecycle, server flows, and events.

Also applies to: 73-127, 129-144, 146-198, 200-254, 256-295, 297-440, 442-483, 485-508, 510-531, 533-539

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

1-3: Header policy check.

New files shouldn’t include the Microsoft copyright header. Please remove or confirm project policy.

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

1-3: Header policy check.

New files shouldn’t include the Microsoft copyright header. Please remove or confirm project policy.

src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)

38-65: LGTM: Clean delegation and caching.

The refactoring to split getVenvInterpreterByPath from getVenvInterpreter provides good separation. Cache pattern and platform-specific path handling are correct.


324-377: LGTM: Kernel spec installation is well-structured.

The three helper methods provide clean separation of concerns. Idempotent kernel spec installation with proper existence checks is good practice.

…ling

This commit refines the Deepnote environment management by updating the structure of the `DeepnoteEnvironmentState` to encapsulate the `pythonInterpreterPath` as an object with `id` and `uri`. Additionally, the `venvBinDir` in `DeepnoteServerStarter` is now derived using `path.dirname()` for better path handling.

Changes include:
- Refactored `DeepnoteEnvironmentState` to use an object for `pythonInterpreterPath`.
- Improved path handling for virtual environment binaries in `DeepnoteServerStarter`.
- Updated related tests to reflect the new structure and ensure correctness.

These changes enhance code clarity and maintainability while ensuring accurate environment configurations.

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
…e-deepnote-kernels

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Signed-off-by: Tomas Kislan <tomas@kislan.sk>
@tkislan
Copy link
Member

tkislan commented Oct 24, 2025

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 24, 2025

✅ Actions performed

Full review triggered.

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)

420-593: Concurrent setup calls can still race.

Past review flagged that concurrent ensureKernelSelected calls (from open event + manual triggers) can race through this setup flow and create duplicate controllers or server registrations. The suggested pending-map-per-notebookKey serialization was never added.

Example approach from past review:

// class field
private readonly pendingSetup = new Map<string, Promise<void>>();

// at start of ensureKernelSelected after computing notebookKey:
const existing = this.pendingSetup.get(notebookKey);
if (existing) return existing;
const p = (async () => { /* current setup flow */ })()
  .finally(() => { 
    if (this.pendingSetup.get(notebookKey) === p) 
      this.pendingSetup.delete(notebookKey); 
  });
this.pendingSetup.set(notebookKey, p);
return p;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 673bc3a and 0fbe68d.

📒 Files selected for processing (1)
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.node.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (8)
src/kernels/deepnote/types.ts (7)
  • IDeepnoteEnvironmentManager (207-207)
  • IDeepnoteEnvironmentManager (208-293)
  • IDeepnoteEnvironmentPicker (295-295)
  • IDeepnoteEnvironmentPicker (296-303)
  • IDeepnoteNotebookEnvironmentMapper (305-305)
  • IDeepnoteNotebookEnvironmentMapper (306-333)
  • DeepnoteKernelConnectionMetadata (25-80)
src/kernels/kernelExecution.ts (1)
  • pendingCells (129-131)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/kernels/jupyter/types.ts (1)
  • JupyterServerProviderHandle (117-130)
src/platform/common/constants.ts (1)
  • JVSC_EXTENSION_ID (24-24)
src/kernels/jupyter/jupyterUtils.ts (1)
  • createJupyterConnectionInfo (83-179)
src/platform/common/utils.ts (1)
  • disposeAsync (458-465)
src/kernels/types.ts (1)
  • IJupyterKernelSpec (626-698)
⏰ 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). (2)
  • GitHub Check: codecov/project
  • GitHub Check: Lint & Format
🔇 Additional comments (4)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (4)

16-16: Environment-aware refactor: imports and injections look good.

Updated dependencies correctly support the new environment-centric kernel selection flow.

Also applies to: 26-28, 46-46, 48-48, 85-88


369-401: Environment selection flow is clear and handles missing configs correctly.

Checks for existing mapping, prompts picker if needed, saves selection, then delegates to setup. Fallback logic when mapped environment is deleted is well-handled.


595-628: Kernel selection logic is sound and testable.

Prefers deepnote-${environmentId} kernel, falls back to Python kernel, throws if no specs. Extracting this for unit testing is good practice.


655-675: Environment-specific controller cleanup is correct.

Identifies controllers using the specified environment by checking server handle, then unselects via affinity. Used appropriately during environment deletion.

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Signed-off-by: Tomas Kislan <tomas@kislan.sk>
coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 31, 2025
Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1)

39-46: Event mock should accept all Event parameters.

The mock only accepts listener but omits optional thisArgs and disposables parameters. If disposables is passed, the returned Disposable should be pushed to that array per the Event contract.

Apply this diff:

-        when(mockConfigManager.onDidChangeEnvironments).thenReturn((_listener: () => void) => {
-            return {
-                dispose: () => {
-                    /* noop */
-                }
-            };
+        when(mockConfigManager.onDidChangeEnvironments).thenReturn((
+            _listener: () => void,
+            _thisArgs?: any,
+            disposables?: Disposable[]
+        ) => {
+            const disposable = { dispose: () => { /* noop */ } };
+            disposables?.push(disposable);
+            return disposable;
         });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between bb12b54 and 0198a06.

📒 Files selected for processing (2)
  • cspell.json (2 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.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 have the .unit.test.ts 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/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
src/kernels/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)

src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.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 then property to avoid hanging tests

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
🧬 Code graph analysis (1)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (4)
src/kernels/deepnote/types.ts (6)
  • IDeepnoteEnvironmentManager (207-207)
  • IDeepnoteEnvironmentManager (208-293)
  • IDeepnoteKernelAutoSelector (181-181)
  • IDeepnoteKernelAutoSelector (182-205)
  • IDeepnoteNotebookEnvironmentMapper (305-305)
  • IDeepnoteNotebookEnvironmentMapper (306-333)
src/test/vscode-mock.ts (2)
  • resetVSCodeMocks (60-79)
  • mockedVSCodeNamespaces (17-17)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/test/mocks/vsc/index.ts (1)
  • CancellationToken (100-112)
🔇 Additional comments (1)
cspell.json (1)

44-44: LGTM. Three Python library names appropriately added to spell-check exceptions, maintaining alphabetical order. These are expected in Deepnote environments context.

Also applies to: 53-54

…View

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (2)

39-46: Event mock signature still incomplete.

The mock partially addresses the past comment but doesn't fully match Event<T>. Add thisArgs? and disposables? parameters, and if disposables is provided, push the returned Disposable into it.

Apply this diff:

-        when(mockConfigManager.onDidChangeEnvironments).thenReturn((_listener: () => void) => {
-            return {
-                dispose: () => {
-                    /* noop */
-                }
-            };
+        when(mockConfigManager.onDidChangeEnvironments).thenReturn(((listener: () => void, thisArgs?: any, disposables?: Disposable[]) => {
+            const disposable = { dispose: () => {/* noop */} };
+            if (disposables) {
+                disposables.push(disposable);
+            }
+            return disposable;
+        }) as any);
-        });

736-758: Reduce duplication in server lifecycle tests.

The three tests for startServer, stopServer, and restartServer are nearly identical. Consider a parameterized approach or shared helper as suggested in past comments.

Example:

['startServer', 'stopServer', 'restartServer'].forEach((methodName) => {
    test(`should call environmentManager.${methodName}`, async () => {
        when((mockConfigManager as any)[methodName](testEnvironmentId, anything())).thenResolve();
        
        await (view as any)[methodName](testEnvironmentId);
        
        verify((mockConfigManager as any)[methodName](testEnvironmentId, anything())).once();
    });
});
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0198a06 and 02633fe.

📒 Files selected for processing (1)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.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 have the .unit.test.ts 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/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
src/kernels/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)

src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.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 then property to avoid hanging tests

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
🧬 Code graph analysis (1)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (4)
src/kernels/deepnote/types.ts (6)
  • IDeepnoteEnvironmentManager (207-207)
  • IDeepnoteEnvironmentManager (208-293)
  • IDeepnoteKernelAutoSelector (181-181)
  • IDeepnoteKernelAutoSelector (182-205)
  • IDeepnoteNotebookEnvironmentMapper (305-305)
  • IDeepnoteNotebookEnvironmentMapper (306-333)
src/test/vscode-mock.ts (2)
  • resetVSCodeMocks (60-79)
  • mockedVSCodeNamespaces (17-17)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/test/mocks/vsc/index.ts (1)
  • CancellationToken (100-112)
🔇 Additional comments (1)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1)

16-809: Comprehensive test coverage for environment view.

The test suite thoroughly covers the DeepnoteEnvironmentsView component with good coverage of happy paths, error cases, validation, and integration points. Mock setup is solid and tests verify expected interactions with collaborators.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 31, 2025
…e-deepnote-kernels

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Copy link
Member

@saltenasl saltenasl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this is good.

As discussed over DMs - the first time experience is confusing (it displays a loader while saying "Select kernel" but it's unclear where the kernel can be selected).

Also, when environment is selected in the topbar of the notebook I still see both "Select Environment for Notebook" as well as "Deepnote: env" buttons. I think the former is unnecessary after environment is already selected.

// Step 3: Enter packages (optional)
const packagesInput = await window.showInputBox({
prompt: l10n.t('Enter additional packages to install (comma-separated, optional)'),
placeHolder: l10n.t('e.g., pandas, numpy, matplotlib'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toolit already install pandas (and has a strict requirement on its version https://github.com/deepnote/deepnote-toolkit/blob/48b0a540532152377886dcc7a76200ebd5849833/pyproject.toml#L48) so installing it at this step may actually break the environment. I suggest just to keep matplotlib and maybe add tensorflow as examples.

…Picker and DeepnoteEnvironmentsView

- Removed redundant checks for existing environments and streamlined the user prompts for creating new environments.
- Updated the createEnvironmentCommand to return the created environment for better handling in the environment selection process.
- Enhanced logging for environment switching and user interactions.

This refactor improves the clarity and efficiency of the environment management workflow.

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

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

⚠️ Outside diff range comments (3)
src/kernels/deepnote/deepnoteSharedToolkitInstaller.node.ts (2)

89-96: Fix Windows path escaping when embedding fsPath into Python code.

Backslashes and quotes in this.sharedInstallationPath.fsPath break the Python string on Windows, causing import to fail. Use a pre‑escaped literal and insert it without quotes.

Apply:

-            const result = await processService.exec(
+            const sharedPathLiteral = JSON.stringify(this.sharedInstallationPath.fsPath);
+            const result = await processService.exec(
                 interpreter.uri.fsPath,
                 [
                     '-c',
-                    `import sys; sys.path.insert(0, '${this.sharedInstallationPath.fsPath}'); import deepnote_toolkit; print('shared import successful')`
+                    `import sys; sys.path.insert(0, ${sharedPathLiteral}); import deepnote_toolkit; print('shared import successful')`
                 ],
                 { throwOnStdErr: false }
             );

124-131: Harden site‑packages discovery (venv/platform variance).

site.getsitepackages()[0] can be missing/wrong on some venvs. Prefer sysconfig fallback.

-            ['-c', 'import site; print(site.getsitepackages()[0])'],
+            [
+                '-c',
+                "import sysconfig, site; " +
+                "p=None\n" +
+                "try:\n p=site.getsitepackages()[0]\n" +
+                "except Exception:\n pass\n" +
+                "print(p or sysconfig.get_paths().get('purelib') or sysconfig.get_paths().get('platlib'))"
+            ],
package.json (1)

50-57: Add activation events for the new view and commands.

Ensures palette/menu invocations activate the extension.

   "activationEvents": [
     "onLanguage:python",
     "onLanguageModelTool:configure_notebook",
     "onLanguageModelTool:notebook_install_packages",
     "onLanguageModelTool:notebook_list_packages",
     "onNotebook:deepnote",
-    "onNotebook:interactive"
+    "onNotebook:interactive",
+    "onView:deepnoteEnvironments",
+    "onCommand:deepnote.environments.create",
+    "onCommand:deepnote.environments.start",
+    "onCommand:deepnote.environments.stop",
+    "onCommand:deepnote.environments.restart",
+    "onCommand:deepnote.environments.delete",
+    "onCommand:deepnote.environments.managePackages",
+    "onCommand:deepnote.environments.editName",
+    "onCommand:deepnote.environments.refresh",
+    "onCommand:deepnote.environments.selectForNotebook"
   ],
♻️ Duplicate comments (3)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (1)

117-121: Prevent near‑duplicate names (trim + case‑insensitive).

Avoid confusing duplicates.

-            const existingEnvironments = this.environmentManager.listEnvironments();
-            if (existingEnvironments.some((env) => env.name === name)) {
+            const existingEnvironments = this.environmentManager.listEnvironments();
+            const entered = name.trim().toLowerCase();
+            if (existingEnvironments.some((env) => env.name.trim().toLowerCase() === entered)) {
                 void window.showErrorMessage(l10n.t('An environment with this name already exists'));
                 return;
             }
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)

160-191: Use typed errors and scrub the failure message.

Bare Error violates the typed-error requirement, and dumping {ex} into the output channel leaks raw details to users. Suggested fix:

-import { DeepnoteToolkitInstallError, DeepnoteVenvCreationError } from '../../platform/errors/deepnoteKernelErrors';
+import {
+    DeepnoteToolkitInstallError,
+    DeepnoteVenvCreationError,
+    DeepnoteVenvNotFoundError
+} from '../../platform/errors/deepnoteKernelErrors';
@@
-        if (!venvInterpreter) {
-            throw new Error(`Venv not found at ${venvPath.fsPath}`);
+        if (!venvInterpreter) {
+            throw new DeepnoteVenvNotFoundError(venvPath.fsPath);
@@
-            this.outputChannel.appendLine(l10n.t('✗ Failed to install packages: {0}', ex));
+            this.outputChannel.appendLine(l10n.t('✗ Failed to install packages; see logs for details'));
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (1)

377-624: Localize the remaining progress strings.

Raw literals still leak into the UI; per guidelines these need l10n.t() wrappers (with placeholders for dynamic bits). Suggested diff:

-                        progress.report({ message: 'Select kernel configuration...' });
+                        progress.report({ message: l10n.t('Select kernel configuration…') });
@@
-        progress.report({ message: `Using ${configuration.name}...` });
+        progress.report({ message: l10n.t('Using {0}…', configuration.name) });
@@
-        progress.report({ message: 'Starting Deepnote server...' });
+        progress.report({ message: l10n.t('Starting Deepnote server…') });
@@
-        progress.report({ message: 'Connecting to kernel...' });
+        progress.report({ message: l10n.t('Connecting to kernel…') });
@@
-        progress.report({ message: 'Finalizing kernel setup...' });
+        progress.report({ message: l10n.t('Finalizing kernel setup…') });
@@
-            progress.report({ message: 'Creating requirements.txt...' });
+            progress.report({ message: l10n.t('Creating requirements.txt…') });
@@
-        progress.report({ message: 'Kernel ready!' });
+        progress.report({ message: l10n.t('Kernel ready!') });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 02633fe and a09ba40.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (7)
  • package.json (7 hunks)
  • src/kernels/deepnote/deepnoteSharedToolkitInstaller.node.ts (5 hunks)
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (13 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts (1 hunks)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (1 hunks)
  • src/kernels/deepnote/types.ts (8 hunks)
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (6 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.node.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/kernels/deepnote/deepnoteSharedToolkitInstaller.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/kernels/deepnote/deepnoteSharedToolkitInstaller.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
  • src/kernels/deepnote/types.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
src/kernels/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)

src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations

Files:

  • src/kernels/deepnote/deepnoteSharedToolkitInstaller.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
  • src/kernels/deepnote/types.ts
  • src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts
  • src/kernels/deepnote/types.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
🧬 Code graph analysis (6)
src/kernels/deepnote/deepnoteSharedToolkitInstaller.node.ts (1)
src/platform/logging/index.ts (1)
  • logger (35-48)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (4)
src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts (1)
  • injectable (12-84)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1)
  • injectable (22-318)
src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts (1)
  • injectable (13-91)
src/platform/interpreter/helpers.ts (3)
  • resolvedPythonEnvToJupyterEnv (229-254)
  • getCachedEnvironment (148-162)
  • getPythonEnvironmentName (65-74)
src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts (3)
src/kernels/deepnote/types.ts (2)
  • IDeepnoteEnvironmentManager (207-207)
  • IDeepnoteEnvironmentManager (208-293)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/kernels/deepnote/environments/deepnoteEnvironmentUi.ts (1)
  • getDeepnoteEnvironmentStatusVisual (5-49)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (6)
src/kernels/deepnote/types.ts (7)
  • IDeepnoteEnvironmentManager (207-207)
  • IDeepnoteEnvironmentManager (208-293)
  • IDeepnoteEnvironmentPicker (295-295)
  • IDeepnoteEnvironmentPicker (296-303)
  • IDeepnoteNotebookEnvironmentMapper (305-305)
  • IDeepnoteNotebookEnvironmentMapper (306-333)
  • DeepnoteKernelConnectionMetadata (25-80)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
  • DeepnoteEnvironment (9-60)
src/platform/common/constants.ts (1)
  • JVSC_EXTENSION_ID (24-24)
src/kernels/jupyter/jupyterUtils.ts (1)
  • createJupyterConnectionInfo (83-179)
src/platform/common/utils.ts (1)
  • disposeAsync (458-465)
src/kernels/types.ts (1)
  • IJupyterKernelSpec (626-698)
src/kernels/deepnote/types.ts (1)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (3)
  • CreateDeepnoteEnvironmentOptions (84-89)
  • DeepnoteEnvironment (9-60)
  • DeepnoteEnvironmentWithStatus (119-122)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)
src/kernels/deepnote/types.ts (2)
  • VenvAndToolkitInstallation (16-19)
  • DEEPNOTE_TOOLKIT_VERSION (335-335)
⏰ 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: Build & Test

Comment on lines +31 to +34
const envWithStatus = this.environmentManager.getEnvironmentWithStatus(env.id);
const { icon, text } = getDeepnoteEnvironmentStatusVisual(
envWithStatus?.status || EnvironmentStatus.Stopped
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use nullish coalescing, not logical OR, with enums.

|| treats 0 as falsy; if EnvironmentStatus.Running === 0, you’ll show Stopped. Use ??.

-            const { icon, text } = getDeepnoteEnvironmentStatusVisual(
-                envWithStatus?.status || EnvironmentStatus.Stopped
-            );
+            const { icon, text } = getDeepnoteEnvironmentStatusVisual(
+                envWithStatus?.status ?? EnvironmentStatus.Stopped
+            );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const envWithStatus = this.environmentManager.getEnvironmentWithStatus(env.id);
const { icon, text } = getDeepnoteEnvironmentStatusVisual(
envWithStatus?.status || EnvironmentStatus.Stopped
);
const envWithStatus = this.environmentManager.getEnvironmentWithStatus(env.id);
const { icon, text } = getDeepnoteEnvironmentStatusVisual(
envWithStatus?.status ?? EnvironmentStatus.Stopped
);
🤖 Prompt for AI Agents
In src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts around lines
31 to 34, the code uses the logical OR operator (||) when falling back to
EnvironmentStatus.Stopped, which treats numeric enum values like 0 as falsy and
can incorrectly map Running (if 0) to Stopped; replace the || fallback with the
nullish coalescing operator (??) so that only null or undefined fall back to
EnvironmentStatus.Stopped, preserving valid enum values like 0.

Comment on lines +44 to +59
// Create tree data provider

// Create tree view
this.treeView = window.createTreeView('deepnoteEnvironments', {
treeDataProvider: this.treeDataProvider,
showCollapseAll: true
});

this.disposables.push(this.treeView);

// Register commands
this.registerCommands();

// Register for disposal
disposableRegistry.push(this);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Ensure the environment manager initializes on activation.

If nothing else calls environmentManager.activate(), the view/picker may see empty/disappearing items after reload. Initialize here or verify global activation wiring.

     ) {
-        // Create tree data provider
+        // Create tree data provider
+
+        // Ensure environments are loaded on activation if not initialized elsewhere
+        try { this.environmentManager.activate(); } catch { /* no-op if already activated */ }

To verify container wiring, run:


🏁 Script executed:

#!/bin/bash
# Check that DeepnoteEnvironmentManager is bound as an activation service and activate() is called.
rg -nP "class\\s+DeepnoteEnvironmentManager\\b" -n
rg -nP "addSingleton<[^>]*IExtensionSyncActivationService" -n src | sed -n '1,200p'
rg -nP "\\bactivate\\s*\\(" -n src/kernels/deepnote/environments
rg -nP "IDeepnoteEnvironmentManager[^\\n]*=\\s*Symbol" -n src
rg -nP "container\\.|serviceManager\\." -n src/notebooks | head -n 50

Length of output: 22124



---

🏁 Script executed:

```shell
#!/bin/bash
# Check DeepnoteEnvironmentsView activate() and if it calls environmentManager.activate()
cd $(find . -name "deepnoteEnvironmentsView.node.ts" -type f | head -1 | xargs dirname)
echo "=== DeepnoteEnvironmentsView activate() method ==="
rg -nP "public\s+activate\s*\(\s*\)\s*:" -A 20 src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
echo ""
echo "=== Check if DeepnoteEnvironmentManager is registered as IExtensionSyncActivationService ==="
rg -nP "DeepnoteEnvironmentManager" src/notebooks/serviceRegistry.node.ts -B 2 -A 2
echo ""
echo "=== Check for environmentManager.activate() calls ==="
rg -nP "environmentManager\.activate\s*\(" src/kernels/deepnote/environments/

Length of output: 459



---

🏁 Script executed:

```shell
#!/bin/bash
echo "=== DeepnoteEnvironmentsView activate() method ==="
rg -nP "public\s+activate\s*\(\s*\)\s*:" -A 20 "deepnoteEnvironmentsView"

echo ""
echo "=== DeepnoteEnvironmentManager registration ==="
rg "DeepnoteEnvironmentManager" "serviceRegistry.node.ts" -B 2 -A 2

echo ""
echo "=== environmentManager.activate() calls across codebase ==="
rg "environmentManager\.activate\s*\(" -n

echo ""
echo "=== DeepnoteEnvironmentsView constructor and fields ==="
rg -A 40 "class DeepnoteEnvironmentsView" "deepnoteEnvironmentsView" | head -60

Length of output: 411


🏁 Script executed:

#!/bin/bash
# Search entire repo without path constraints
rg "public\s+activate.*\(\s*\)\s*:" --type=ts -A 15 | grep -A 15 "DeepnoteEnvironmentsView"
echo "---"
rg "environmentManager" --type=ts | grep -i activate

Length of output: 3


Ensure DeepnoteEnvironmentManager activation on extension startup.

DeepnoteEnvironmentManager implements IExtensionSyncActivationService, but is registered only as IDeepnoteEnvironmentManager in the service container, so the activation system won't auto-call its activate() method. No calls to environmentManager.activate() exist in the codebase. Add the activation call in the view's constructor:

     ) {
         // Create tree data provider
+
+        // Ensure environments are loaded on activation if not initialized elsewhere
+        this.environmentManager.activate();
🤖 Prompt for AI Agents
In src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts around
lines 44 to 59, the DeepnoteEnvironmentManager (which implements
IExtensionSyncActivationService) is only registered as
IDeepnoteEnvironmentManager so its activate() is never invoked on extension
startup; after creating the tree view and before pushing this into
disposableRegistry, call the environment manager's activate() (await if it
returns a Promise) to ensure it runs on startup, and wrap the call in a
try/catch or log any errors so activation failures don't crash the constructor.

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Copy link
Member

@saltenasl saltenasl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working pretty well now! lets merge and improve forward if we find anything

coderabbitai[bot]
coderabbitai bot previously requested changes Oct 31, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (2)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (2)

117-122: Harden duplicate-name check (case-insensitive + trim).

Prevents near-duplicates like "Env1" vs "env1" or "Env1 ".

         // Check if name is already in use
         const existingEnvironments = this.environmentManager.listEnvironments();
-        if (existingEnvironments.some((env) => env.name === name)) {
+        const normalizedInput = name.trim().toLowerCase();
+        if (existingEnvironments.some((env) => env.name.trim().toLowerCase() === normalizedInput)) {
             void window.showErrorMessage(l10n.t('An environment with this name already exists'));
             return;
         }

492-494: Fix misleading comment: old controller is not disposed.

Per past review, implementation intentionally keeps the old controller alive to avoid "DISPOSED" errors on queued executions.

-                    // Force rebuild the controller with the new environment
-                    // This clears cached metadata and creates a fresh controller.
+                    // Rebuild the controller with the new environment.
+                    // Clears cached metadata and creates a fresh controller; old controller intentionally not disposed.
                     await this.kernelAutoSelector.rebuildController(activeNotebook);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a09ba40 and 09742c9.

📒 Files selected for processing (1)
  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.node.ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use *.node.ts for Desktop-specific implementations that require full file system access and Python environments

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
src/kernels/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)

src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations

Files:

  • src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
🧬 Code graph analysis (1)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (5)
src/kernels/deepnote/environments/deepnoteEnvironmentPicker.ts (1)
  • injectable (12-84)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1)
  • injectable (22-318)
src/kernels/deepnote/environments/deepnoteNotebookEnvironmentMapper.node.ts (1)
  • injectable (13-91)
src/platform/interpreter/helpers.ts (3)
  • resolvedPythonEnvToJupyterEnv (229-254)
  • getCachedEnvironment (148-162)
  • getPythonEnvironmentName (65-74)
src/kernels/deepnote/environments/deepnoteEnvironmentUi.ts (1)
  • getDeepnoteEnvironmentStatusVisual (5-49)

@saltenasl saltenasl dismissed coderabbitai[bot]’s stale review October 31, 2025 13:29

resolved or irrelevant

@saltenasl saltenasl merged commit dee6066 into main Oct 31, 2025
12 of 13 checks passed
@saltenasl saltenasl deleted the hannes/grn-4913-manage-deepnote-kernels branch October 31, 2025 13:30
tkislan added a commit that referenced this pull request Oct 31, 2025
saltenasl pushed a commit that referenced this pull request Oct 31, 2025
@tkislan tkislan restored the hannes/grn-4913-manage-deepnote-kernels branch October 31, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants