Add support for creating environments with existing python virtual environments #257
Conversation
📝 WalkthroughWalkthroughAdds a managedVenv boolean across Deepnote environment types, storage, UI, and runtime flows to mark whether a virtual environment is extension-managed or external. EnvironmentManager uses a process service to detect interpreters running inside existing venvs and sets managedVenv accordingly; deletion only removes venv directories when managedVenv is true. The managedVenv flag is threaded through toolkit installation and server startup APIs. DeepnoteToolkitInstaller gains an external-venv installation path and throws DeepnoteToolkitMissingError when deepnote-toolkit is absent in non-managed venvs. Kernel auto-selector and EnvironmentsView handle toolkit-missing flows, allowing installing the toolkit into existing venvs or surfacing output to the user. Sequence Diagram(s)sequenceDiagram
participant User
participant View as DeepnoteEnvironmentsView
participant Mgr as DeepnoteEnvironmentManager
participant ProcFactory as IProcessServiceFactory
participant Proc as IProcessService
participant Storage as EnvironmentStorage
participant Toolkit as DeepnoteToolkitInstaller
participant AutoSel as DeepnoteKernelAutoSelector
participant Server as DeepnoteServerStarter
User->>View: create or switch environment (pythonUri)
View->>Mgr: createOrSelectEnvironment(pythonUri)
rect rgba(237,247,238,0.9)
Mgr->>ProcFactory: create(pythonUri)
ProcFactory->>Proc: run python -c "sys.prefix vs sys.base_prefix"
alt interpreter inside existing venv
Proc-->>Mgr: venvPath
Mgr->>Mgr: set managedVenv = false
else interpreter not in venv
Proc-->>Mgr: undefined
Mgr->>Mgr: create managed venv (venvPath)
Mgr->>Mgr: set managedVenv = true
end
end
Mgr->>Storage: saveEnvironment(env{managedVenv})
Storage-->>Mgr: saved
rect rgba(238,243,255,0.9)
Mgr->>Toolkit: ensureVenvAndToolkit(baseInterpreter, venvPath, managedVenv)
alt toolkit missing in external venv
Toolkit-->>Mgr: throw DeepnoteToolkitMissingError
Mgr->>AutoSel: propagate error
AutoSel->>View: handleKernelSelectionError(error, notebook)
View->>User: prompt install or show output
alt user chooses install
View->>Toolkit: installToolkitInExistingVenv(venvPath)
Toolkit-->>View: install complete
View->>Server: startServer(..., managedVenv, ...)
else user shows output
View->>View: show output channel
end
else toolkit present or installed
Toolkit-->>Mgr: success
Mgr->>Server: startServer(interpreter, venvPath, managedVenv, ...)
Server-->>View: server started
end
end
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #257 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/environments/deepnoteEnvironmentStorage.unit.test.ts (1)
122-161: Test expectations missingmanagedVenvfield.Line 128 includes
managedVenv: truein the test data, but the expected serialized state (lines 143-157) omits it. The test won't properly verify that the field is persisted.🔎 Add managedVenv to expected output
venvPath: 'file:///path/to/venv', + managedVenv: true, createdAt: '2025-01-01T00:00:00.000Z',Apply the same fix to the test at lines 163-190.
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1)
54-62: Fix compilation error: Missing outputChannel argument.Constructor now requires 8 arguments including
IOutputChannel(see line 55 in deepnoteEnvironmentsView.node.ts).🔎 Proposed fix
setup(() => { resetVSCodeMocks(); disposables.push(new Disposable(() => resetVSCodeMocks())); // Initialize Python API for helper functions pythonEnvironments = crateMockedPythonApi(disposables).environments; mockConfigManager = mock<IDeepnoteEnvironmentManager>(); mockTreeDataProvider = mock<DeepnoteEnvironmentTreeDataProvider>(); mockPythonApiProvider = mock<IPythonApiProvider>(); mockDisposableRegistry = mock<IDisposableRegistry>(); mockKernelAutoSelector = mock<IDeepnoteKernelAutoSelector>(); mockNotebookEnvironmentMapper = mock<IDeepnoteNotebookEnvironmentMapper>(); mockKernelProvider = mock<IKernelProvider>(); + const mockOutputChannel = mock<IOutputChannel>(); // Mock onDidChangeEnvironments to return a disposable event when(mockConfigManager.onDidChangeEnvironments).thenReturn((_listener: () => void) => { return { dispose: () => { /* noop */ } }; }); view = new DeepnoteEnvironmentsView( instance(mockConfigManager), instance(mockTreeDataProvider), instance(mockPythonApiProvider), instance(mockDisposableRegistry), instance(mockKernelAutoSelector), instance(mockNotebookEnvironmentMapper), - instance(mockKernelProvider) + instance(mockKernelProvider), + instance(mockOutputChannel) ); });src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (1)
84-106: Missing test coverage for external venvs.All tests use
managedVenv: true. Consider adding tests for:
- Creating/loading environments with
managedVenv: false- Verifying deletion skips venv directory cleanup for external venvs
- Verifying migration logic respects
managedVenvflag
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (17)
src/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/environments/deepnoteEnvironment.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.tssrc/kernels/deepnote/types.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.tssrc/platform/errors/deepnoteKernelErrors.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create unit tests in
*.unit.test.tsfiles
**/*.unit.test.ts: Unit tests use Mocha/Chai framework with.unit.test.tsextension
Test files should be placed alongside the source files they test
Useassert.deepStrictEqual()for object comparisons instead of checking individual properties
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create integration tests in
*.test.tsfiles (not*.unit.test.ts)
**/*.test.ts: ALWAYS check the 'Unittest - Build' task output for TypeScript compilation errors before running any script or declaring work complete
When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefinedthenproperty to avoid hanging tests
Usenpm run test:unittestsfor TypeScript unit tests before committing changes
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.tssrc/platform/errors/deepnoteKernelErrors.tssrc/kernels/deepnote/environments/deepnoteEnvironment.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.tssrc/kernels/deepnote/types.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: RespectCancellationTokenin all async operations across kernel execution, session communication, and finder operations
Implement proper error handling in files using custom error types fromerrors/directory (KernelDeadError,KernelConnectionTimeoutError,KernelInterruptTimeoutError,JupyterInvalidKernelError,KernelDependencyError)
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironment.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/deepnote/types.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.tssrc/platform/errors/deepnoteKernelErrors.tssrc/kernels/deepnote/environments/deepnoteEnvironment.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.tssrc/kernels/deepnote/types.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.tssrc/platform/errors/deepnoteKernelErrors.tssrc/kernels/deepnote/environments/deepnoteEnvironment.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.tssrc/kernels/deepnote/types.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify-based dependency injection with IServiceManager and IServiceContainer for managing service relationships and dependencies
Define service interfaces using Symbol-based exports (e.g.,export const IFileSystem = Symbol('IFileSystem')) paired with corresponding TypeScript interfaces
Register types using the patternserviceManager.addSingleton<IService>(IService, ServiceImplementation)in registerTypes functions
Use @Injectable() and @Inject() decorators from Inversify for class dependency injection configuration
Implement unified interfaces for cross-platform services (IFileSystem, IPlatformService, etc.) with graceful degradation for web environment limitations
Use structured logging with context via logger.info, logger.error, etc. rather than console.log
Provide platform capability checks and feature detection before using advanced platform-specific features
Register separate service implementations for desktop and web environments, allowing the DI container to resolve the correct platform implementation
Files:
src/platform/errors/deepnoteKernelErrors.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/kernels/deepnote/deepnoteServerStarter.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
🧬 Code graph analysis (7)
src/platform/errors/deepnoteKernelErrors.ts (1)
.vscode-test.mjs (1)
pythonPath(48-52)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (3)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteFile(13-13)src/platform/errors/deepnoteKernelErrors.ts (1)
DeepnoteToolkitMissingError(171-191)src/kernels/deepnote/types.ts (1)
DEEPNOTE_TOOLKIT_VERSION(360-360)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
DeepnoteEnvironment(9-67)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (1)
src/platform/common/process/types.node.ts (1)
IProcessService(47-51)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts (1)
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts (1)
DeepnoteEnvironmentTreeItem(26-126)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (1)
src/platform/errors/deepnoteKernelErrors.ts (1)
DeepnoteToolkitMissingError(171-191)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (3)
src/platform/common/constants.ts (1)
STANDARD_OUTPUT_CHANNEL(32-32)src/platform/errors/deepnoteKernelErrors.ts (1)
DeepnoteToolkitMissingError(171-191)src/platform/logging/index.ts (1)
logger(35-48)
🪛 GitHub Actions: CD
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
[error] 54-54: TS2554: Expected 8 arguments, but got 7.
🪛 GitHub Actions: CI
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
[error] 54-54: TS2554: Expected 8 arguments, but got 7.
🔇 Additional comments (31)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (2)
31-36: LGTM - clean interface extension.The
managedVenvfield is well-documented and properly typed. The distinction between extension-managed and external venvs is clear.
81-81: Optional field appropriate for backward compatibility.Making
managedVenvoptional in the state interface allows existing serialized environments to load without migration.src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.unit.test.ts (1)
19-19: Test fixture updated correctly.src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts (1)
1023-1023: Mock helper updated correctly.src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.node.ts (1)
96-99: UI change displays managed status clearly.Icon choices are appropriate:
shieldfor managed environments,link-externalfor external ones.src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1)
110-110: LGTM: Test fixtures updated correctly.Consistent addition of
managedVenv: trueto all test environment fixtures.Also applies to: 251-251, 387-387, 585-585, 595-595, 703-703
src/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.ts (1)
25-25: LGTM: Test fixtures updated correctly.Consistent addition of
managedVenv: truealigns with API changes.Also applies to: 35-35
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.node.ts (1)
72-72: LGTM: Proper serialization with backward compatibility.The
?? truefallback ensures existing stored environments withoutmanagedVenvare treated as managed.Also applies to: 98-98
src/kernels/deepnote/environments/deepnoteEnvironmentTreeItem.node.ts (2)
14-21: LGTM: Type extension for managed venv info item.Extends the info item ID union to support the new managed/external indicator.
71-75: LGTM: Clear UI distinction between managed and external environments.Properly localized labels for environment type display.
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (2)
54-56: LGTM: Constructor extended for output channel support.Enables showing output when environment operations fail. Note: Test file must be updated (already flagged).
488-505: LGTM: Proper error handling for toolkit missing scenarios.Distinguishes
DeepnoteToolkitMissingError(delegated to kernel auto selector) from other errors (shown with output option).src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (1)
59-64: Mock behavior simulates "not in venv" scenario correctly.The default mock returns
0|/usr/lib/python3, indicatingsys.prefix == sys.base_prefix(not in venv). This is appropriate for the default test case where new managed venvs are created.src/kernels/deepnote/deepnoteServerStarter.node.ts (2)
94-111: Clean parameter threading.The
managedVenvparameter is properly documented and threaded through the call chain.
237-259: Parameter propagation is consistent.The
managedVenvflag flows correctly fromstartServer→startServerForEnvironment→ensureVenvAndToolkit.src/kernels/deepnote/types.ts (2)
230-234: Consider error propagation strategy.
handleKernelSelectionErrorreturnsPromise<void>, suggesting fire-and-forget error handling. If this method can throw (e.g., showing UI dialogs), callers won't know. Verify this is intentional.
90-108: Breaking API change—properly handled.The new required
managedVenvparameter is consistently applied across the interface definition (types.ts), implementation (deepnoteToolkitInstaller.node.ts), and call site (deepnoteServerStarter.node.ts). All usages pass the parameter correctly.src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (4)
7-19: Import organization improved.Duplicate import removed and new error class properly imported.
81-101: Validation correctly gated on managedVenv.External venvs can exist outside extension storage, so the path prefix check only applies to managed venvs.
147-150: Correct error path for external venvs without toolkit.The check happens after verifying toolkit isn't already installed, so this only throws when an external venv lacks the toolkit and cannot auto-install.
369-384: Returnundefinedfor empty toolkit versions.The method now returns
undefinedinstead of empty string when the version output is empty. Both callers check!= null, ensuring compatibility. Change is correct.src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (6)
4-5: LGTM!Direct
fsimport appropriate for.node.tsfile per platform implementation guidelines.
27-27: LGTM!Version constant correctly imported for install command.
52-52: LGTM!Error type import follows established pattern.
75-78: LGTM!Type alignment with
DeepnoteFilecorrect.
550-558: LGTM!
managedVenvcorrectly propagated to server startup flow.
673-675: LGTM!Cast aligns with updated type signature.
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (4)
37-38: LGTM!Proper DI for file system and process operations.
127-144: LGTM!Clean detection logic. External venvs preserved; new ones managed.
217-233: LGTM!Safe deletion logic preserves external venvs. Error handling appropriate.
69-77: Handle legacy configs without managedVenv property for migration.Line 77 uses strict equality
=== true, so old configs with undefinedmanagedVenvwon't migrate. This is inconsistent with the deletion logic (line 219) which uses truthy check. Clarify intent: if old managed venvs should migrate, defaultundefinedtotrueduring load or upgrade path initialization. If treating undefined as external is intentional, document it.
There was a problem hiding this comment.
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 (6)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (3)
86-109: Missing assertion formanagedVenvproperty.Test verifies
idbut not the newly addedmanagedVenvfield from the fixture.Proposed fix
assert.strictEqual(configs.length, 1); assert.strictEqual(configs[0].id, 'existing-config'); + assert.strictEqual(configs[0].managedVenv, true);
112-131: Missing assertion formanagedVenvon newly created environments.
createEnvironmentshould setmanagedVenv. Verify the property is set correctly.Proposed fix
assert.ok(config.createdAt); assert.ok(config.lastUsedAt); + assert.strictEqual(config.managedVenv, true); verify(mockStorage.saveEnvironments(anything())).once();
265-287: Test only covers managed venv deletion.Per the AI summary, deletion should skip removing venv directories when
managedVenv: false. Add a test case for unmanaged venvs.Proposed test case
test('should not delete virtual environment directory for unmanaged venvs', async () => { const externalVenvPath = Uri.file(`${testGlobalStoragePath}/external-venv`); // Create the external venv directory fs.mkdirSync(externalVenvPath.fsPath, { recursive: true }); fs.writeFileSync(`${externalVenvPath.fsPath}/pyvenv.cfg`, 'test'); // Load an unmanaged environment const unmanagedConfig = { id: 'unmanaged-env', name: 'External Venv', pythonInterpreter: testInterpreter, venvPath: externalVenvPath, managedVenv: false, createdAt: new Date(), lastUsedAt: new Date() }; when(mockStorage.loadEnvironments()).thenResolve([unmanagedConfig]); manager.activate(); await manager.waitForInitialization(); await manager.deleteEnvironment('unmanaged-env'); // Directory should still exist assert.isTrue(fs.existsSync(externalVenvPath.fsPath), 'Unmanaged venv directory should not be deleted'); // Cleanup fs.rmSync(externalVenvPath.fsPath, { recursive: true, force: true }); });src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts (3)
185-185: Remove redundant mock setup.Line 29 already configures this mock in
setup().🔎 Proposed fix
- - when(mockGlobalState.update(anything(), anything())).thenResolve(); - await storage.saveEnvironments(configs);
44-75: Verify managedVenv defaults to true when absent from stored state.These tests load environments from stored states that lack
managedVenv, but don't assert the loaded configs havemanagedVenv: true. Add assertions to verify the defaulting behavior mentioned in the AI summary.🔎 Suggested test assertions
For lines 44-75:
assert.strictEqual(configs[0].toolkitVersion, '0.2.30'); assert.strictEqual(configs[0].description, 'Test environment'); + assert.strictEqual(configs[0].managedVenv, true); });For lines 77-111:
assert.strictEqual(configs.length, 2); assert.strictEqual(configs[0].id, 'config-1'); + assert.strictEqual(configs[0].managedVenv, true); assert.strictEqual(configs[1].id, 'config-2'); + assert.strictEqual(configs[1].managedVenv, true); });Also applies to: 77-111
123-207: Consider adding test coverage formanagedVenv: false.All save tests use
managedVenv: true. Add a test that saves and verifies serialization of an environment withmanagedVenv: falseto ensure external venvs are handled correctly.
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (3)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create unit tests in
*.unit.test.tsfiles
**/*.unit.test.ts: Unit tests use Mocha/Chai framework with.unit.test.tsextension
Test files should be placed alongside the source files they test
Useassert.deepStrictEqual()for object comparisons instead of checking individual properties
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create integration tests in
*.test.tsfiles (not*.unit.test.ts)
**/*.test.ts: ALWAYS check the 'Unittest - Build' task output for TypeScript compilation errors before running any script or declaring work complete
When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefinedthenproperty to avoid hanging tests
Usenpm run test:unittestsfor TypeScript unit tests before committing changes
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: RespectCancellationTokenin all async operations across kernel execution, session communication, and finder operations
Implement proper error handling in files using custom error types fromerrors/directory (KernelDeadError,KernelConnectionTimeoutError,KernelInterruptTimeoutError,JupyterInvalidKernelError,KernelDependencyError)
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
🧬 Code graph analysis (1)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (1)
src/platform/common/process/types.node.ts (1)
IProcessService(47-51)
⏰ 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
🔇 Additional comments (6)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1)
8-8: LGTM: DI wiring for output channel.The
IOutputChanneldependency is properly mocked and injected. No behavior configuration needed if the channel isn't used in these test paths.Also applies to: 27-27, 45-45, 63-64
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts (4)
1-16: LGTM on imports.Imports are properly organized with third-party first, local files second.
25-26: LGTM on process service mocks.Mock declarations and initialization follow the established pattern.
Also applies to: 41-42
318-350: LGTM on migration tests.Test fixtures updated with
managedVenv: true. Migration logic properly verified.
455-502: LGTM on batch migration test.All fixtures have
managedVenv: true. Assertions verify correct path migration.src/kernels/deepnote/environments/deepnoteEnvironmentStorage.unit.test.ts (1)
29-29: LGTM: Centralized mock setup.Moving this to
setup()reduces duplication across tests.
src/kernels/deepnote/environments/deepnoteEnvironmentManager.unit.test.ts
Show resolved
Hide resolved
- Added timeout to Python execution for virtual environment detection. - Implemented tests for renaming, deleting, and switching between managed and external environments. - Updated package management for external environments. - Improved error messages for missing deepnote-toolkit installation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (1)
606-744: Verify venv deletion is skipped for external environments.Tests confirm side effects (mapping removal, kernel disposal) but don't verify the key distinction: venv directory removal should be skipped when
managedVenv: false. Add assertions thatfileSystem.deleteis NOT called for external environments.src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1)
267-293: Strengthen output parsing.Timeout added (good), but parsing needs validation:
- Line 281: Verify
split('|')returns exactly 2 parts- Line 283: Check
prefixPathis non-empty string- Handle case where stdout has unexpected format
🔎 Suggested improvement
const output = result.stdout.trim(); -const [isVenv, prefixPath] = output.split('|'); +const parts = output.split('|'); -if (isVenv === '1' && prefixPath) { +if (parts.length === 2 && parts[0] === '1' && parts[1] && parts[1].length > 0) { + const prefixPath = parts[1]; logger.info(`Detected existing virtual environment at: ${prefixPath}`); return Uri.file(prefixPath); } +logger.info(`Python is not in a virtual environment (output: ${output})`); return undefined;
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (4)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/platform/errors/deepnoteKernelErrors.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/platform/errors/deepnoteKernelErrors.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify-based dependency injection with IServiceManager and IServiceContainer for managing service relationships and dependencies
Define service interfaces using Symbol-based exports (e.g.,export const IFileSystem = Symbol('IFileSystem')) paired with corresponding TypeScript interfaces
Register types using the patternserviceManager.addSingleton<IService>(IService, ServiceImplementation)in registerTypes functions
Use @Injectable() and @Inject() decorators from Inversify for class dependency injection configuration
Implement unified interfaces for cross-platform services (IFileSystem, IPlatformService, etc.) with graceful degradation for web environment limitations
Use structured logging with context via logger.info, logger.error, etc. rather than console.log
Provide platform capability checks and feature detection before using advanced platform-specific features
Register separate service implementations for desktop and web environments, allowing the DI container to resolve the correct platform implementation
Files:
src/platform/errors/deepnoteKernelErrors.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/platform/errors/deepnoteKernelErrors.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/platform/errors/deepnoteKernelErrors.tssrc/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: RespectCancellationTokenin all async operations across kernel execution, session communication, and finder operations
Implement proper error handling in files using custom error types fromerrors/directory (KernelDeadError,KernelConnectionTimeoutError,KernelInterruptTimeoutError,JupyterInvalidKernelError,KernelDependencyError)
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create unit tests in
*.unit.test.tsfiles
**/*.unit.test.ts: Unit tests use Mocha/Chai framework with.unit.test.tsextension
Test files should be placed alongside the source files they test
Useassert.deepStrictEqual()for object comparisons instead of checking individual properties
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create integration tests in
*.test.tsfiles (not*.unit.test.ts)
**/*.test.ts: ALWAYS check the 'Unittest - Build' task output for TypeScript compilation errors before running any script or declaring work complete
When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefinedthenproperty to avoid hanging tests
Usenpm run test:unittestsfor TypeScript unit tests before committing changes
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts
🧬 Code graph analysis (3)
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (1)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
DeepnoteEnvironment(9-67)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (3)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteFile(13-13)src/platform/errors/deepnoteKernelErrors.ts (1)
DeepnoteToolkitMissingError(171-195)src/kernels/deepnote/types.ts (1)
DEEPNOTE_TOOLKIT_VERSION(360-360)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (3)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
DeepnoteEnvironment(9-67)src/test/mocks/vsc/index.ts (1)
CancellationToken(100-112)src/platform/deepnote/deepnoteServerUtils.node.ts (1)
createDeepnoteServerConfigHandle(3-5)
⏰ 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
🔇 Additional comments (12)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.unit.test.ts (4)
8-8: LGTM! OutputChannel mock wired correctly.Clean addition of
IOutputChanneldependency following established test patterns.Also applies to: 27-27, 45-45, 63-64
113-126: LGTM! External environment rename test added.Correctly verifies that renaming works identically for both managed and external environments.
Also applies to: 251-265
890-1037: LGTM! Bidirectional environment switching tested.Correctly validates switching between managed and external environments in both directions.
1101-1124: LGTM! Package management for external environments covered.Validates that package updates work correctly for external environments.
src/kernels/deepnote/environments/deepnoteEnvironmentManager.node.ts (4)
8-9: LGTM! DI for filesystem and process services added.Clean injection of required dependencies.
Also applies to: 37-38
69-77: LGTM! Migration logic respects managedVenv flag.Correctly only migrates extension-managed venvs, preserving external venv paths.
128-138: LGTM! Environment creation reuses existing venvs.Correctly detects existing venvs and marks them as external (
managedVenv: false).
217-233: LGTM! Conditional deletion protects external venvs.Critical defensive code correctly preserves external venvs while cleaning up managed ones.
src/platform/errors/deepnoteKernelErrors.ts (1)
171-195: LGTM! Toolkit missing error with actionable guidance.Error class properly structured with helpful troubleshooting steps. Past review concern addressed.
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (3)
4-5: LGTM! Imports and types updated for DeepnoteFile.Clean alignment with updated type definitions.
Also applies to: 27-27, 52-52, 77-77
553-553: LGTM! managedVenv threaded to server startup.Correctly passes environment management flag to server starter.
Also applies to: 674-674
813-813: LGTM! Method visibility changed to public.Correctly made public to support external error handling calls.
…nments - Added a new method `installToolkitInExistingVenv` to the `DeepnoteToolkitInstaller` class for installing the toolkit in an existing external virtual environment. - Updated the `IDeepnoteToolkitInstaller` interface to include the new method. - Enhanced error handling and logging during the installation process. - Modified the `handleKernelSelectionError` method to provide user-friendly actions for missing toolkit installations. - Introduced a new private method `installToolkitAndNotify` in `DeepnoteKernelAutoSelector` to handle toolkit installation and notify users upon completion.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts (1)
1015-1040: Consider adding test coverage for external venvs.
createMockEnvironmentalways returnsmanagedVenv: true. Add an optional parameter or separate helper formanagedVenv: falsescenarios to test external venv code paths.🔎 Suggested change
-function createMockEnvironment(id: string, name: string, hasServer: boolean = false): DeepnoteEnvironment { +function createMockEnvironment(id: string, name: string, hasServer: boolean = false, managedVenv: boolean = true): DeepnoteEnvironment { const mockPythonInterpreter: PythonEnvironment = { id: `/usr/bin/python3`, uri: Uri.parse(`/usr/bin/python3`) }; return { id, name, description: `Test environment ${name}`, pythonInterpreter: mockPythonInterpreter, venvPath: Uri.file(`/test/venvs/${id}`), - managedVenv: true, + managedVenv, packages: [],
🤖 Fix all issues with AI Agents
In @src/kernels/deepnote/deepnoteToolkitInstaller.node.ts:
- Around line 223-236: installToolkitInExistingVenv lacks the concurrent-install
guard so multiple calls can trigger duplicate installs; wrap its work with the
same pendingInstallations logic used by ensureVenvAndToolkit: compute a unique
key (e.g., venvPath.fsPath or venvInterpreter.path), if
pendingInstallations.has(key) return the existing promise, otherwise create a
promise that calls installToolkitPackages(venvInterpreter, venvPath, token),
store it in pendingInstallations.set(key, promise), and ensure you remove the
key from pendingInstallations in finally (and propagate errors) so subsequent
calls behave correctly.
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (5)
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/kernels/deepnote/types.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/kernels/deepnote/types.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: RespectCancellationTokenin all async operations across kernel execution, session communication, and finder operations
Implement proper error handling in files using custom error types fromerrors/directory (KernelDeadError,KernelConnectionTimeoutError,KernelInterruptTimeoutError,JupyterInvalidKernelError,KernelDependencyError)
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/kernels/deepnote/types.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/kernels/deepnote/types.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.tssrc/kernels/deepnote/types.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create unit tests in
*.unit.test.tsfiles
**/*.unit.test.ts: Unit tests use Mocha/Chai framework with.unit.test.tsextension
Test files should be placed alongside the source files they test
Useassert.deepStrictEqual()for object comparisons instead of checking individual properties
Files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create integration tests in
*.test.tsfiles (not*.unit.test.ts)
**/*.test.ts: ALWAYS check the 'Unittest - Build' task output for TypeScript compilation errors before running any script or declaring work complete
When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefinedthenproperty to avoid hanging tests
Usenpm run test:unittestsfor TypeScript unit tests before committing changes
Files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
🧬 Code graph analysis (3)
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (3)
src/platform/common/constants.ts (1)
STANDARD_OUTPUT_CHANNEL(32-32)src/platform/errors/deepnoteKernelErrors.ts (1)
DeepnoteToolkitMissingError(171-195)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (3)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteFile(13-13)src/kernels/deepnote/types.ts (2)
IDeepnoteToolkitInstaller(91-91)IDeepnoteToolkitInstaller(92-148)src/platform/errors/deepnoteKernelErrors.ts (1)
DeepnoteToolkitMissingError(171-195)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts (1)
src/kernels/deepnote/types.ts (2)
IDeepnoteToolkitInstaller(91-91)IDeepnoteToolkitInstaller(92-148)
🔇 Additional comments (11)
src/kernels/deepnote/types.ts (2)
91-135: Interface additions look good.The
managedVenvparameter andinstallToolkitInExistingVenvmethod cleanly separate managed vs external venv flows. Documentation is thorough.
243-249: New public method signature is appropriate.
handleKernelSelectionErrornow acceptsnotebookparameter, enabling environment-specific error handling flows.src/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts (1)
45-75: Mocks properly added.Toolkit installer dependency wired correctly.
src/kernels/deepnote/environments/deepnoteEnvironmentsView.node.ts (2)
488-504: Error handling pattern is solid.Toolkit-missing errors correctly delegate to the auto-selector. Generic errors provide actionable "Show Output" option.
54-55: Dependency injection correct.Named injection for output channel follows established patterns.
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (3)
147-150: Correct behavior for external venvs.Throwing
DeepnoteToolkitMissingErrortriggers the user prompt flow upstream.
315-402: Good refactor—shared installation logic.
installToolkitPackageseliminates duplication between managed and external venv flows. Cancellation tokens properly checked.
294-312: Error handling improved.Known error types rethrown without wrapping. Generic errors get context via
DeepnoteToolkitInstallError.src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (3)
887-914: Installation flow handles errors gracefully.Progress notification, cancellation support, and controller rebuild after success. Error message properly sanitized.
101-102: Dependency injection correct.Toolkit installer wired properly.
554-554: managedVenv correctly passed to startServer.Ensures server startup respects environment type.
- Added logic to wait for any pending installations in the `DeepnoteToolkitInstaller` class, improving user experience during concurrent installation requests. - Updated the error message in `DeepnoteKernelAutoSelector` to inform users about the required deepnote-toolkit version for running Deepnote projects, enhancing clarity and guidance for users.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/kernels/deepnote/deepnoteToolkitInstaller.node.ts:
- Around line 223-245: installToolkitInExistingVenv has a race: it returns the
existing pending promise but doesn't add a new pending entry when starting an
installation, so concurrent callers can start duplicate installs; fix by
creating the installation promise from installToolkitPackages, immediately store
it in this.pendingInstallations keyed by venvKey, and ensure you remove the
entry in a finally handler (or on both resolve/reject) so the map is cleared
after completion; reference installToolkitInExistingVenv, pendingInstallations,
and installToolkitPackages when making the change.
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (2)
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: RespectCancellationTokenin all async operations across kernel execution, session communication, and finder operations
Implement proper error handling in files using custom error types fromerrors/directory (KernelDeadError,KernelConnectionTimeoutError,KernelInterruptTimeoutError,JupyterInvalidKernelError,KernelDependencyError)
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration located in
src/notebooks/deepnote/with refactored architecture:deepnoteTypes.ts(type definitions),deepnoteNotebookManager.ts(state management),deepnoteNotebookSelector.ts(UI selection logic),deepnoteDataConverter.ts(data transformations),deepnoteSerializer.ts(main serializer/orchestration),deepnoteActivationService.ts(VSCode activation)
Files:
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts
🧬 Code graph analysis (2)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)
src/platform/errors/deepnoteKernelErrors.ts (3)
DeepnoteToolkitMissingError(171-195)DeepnoteVenvCreationError(50-89)DeepnoteToolkitInstallError(94-169)src/kernels/deepnote/types.ts (2)
VenvAndToolkitInstallation(13-16)DEEPNOTE_TOOLKIT_VERSION(375-375)
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (3)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteFile(13-13)src/kernels/deepnote/types.ts (2)
IDeepnoteToolkitInstaller(91-91)IDeepnoteToolkitInstaller(92-148)src/platform/errors/deepnoteKernelErrors.ts (1)
DeepnoteToolkitMissingError(171-195)
🔇 Additional comments (13)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (6)
7-19: LGTM!Imports align with new error handling and cancellation patterns.
84-101: LGTM!Conditional validation for managed venvs is correct. External venvs properly skip the globalStorage path check.
147-150: Good error boundary.Correctly throws
DeepnoteToolkitMissingErrorfor external venvs missing toolkit, enabling upstream handling.
303-321: LGTM!Clean error propagation. Known errors pass through; unknown errors get wrapped with context.
328-411: LGTM!Well-structured helper. Proper cancellation checks, and kernel spec failure is non-fatal.
422-423: LGTM!Defensive check prevents empty string from being treated as a valid version.
src/notebooks/deepnote/deepnoteKernelAutoSelector.node.ts (7)
4-60: LGTM!Imports align with new toolkit handling and type updates.
76-106: LGTM!Type update and new dependency injection are consistent with the toolkit handling flow.
161-184: LGTM!Updated calls pass notebook context for toolkit installation flow.
552-560: LGTM!
managedVenvflag properly propagated to server starter.
675-677: LGTM!Cast to
DeepnoteFilealigns with updated type definition.
815-836: LGTM!Clean error UX. Modal dialog with actionable options (install/change environment). Properly localized.
891-918: LGTM!Good progress UX. Controller rebuild after install ensures kernel readiness. Error handling is clean.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/kernels/deepnote/deepnoteToolkitInstaller.node.ts:
- Line 148: Remove the dead commented-out line in
deepnoteToolkitInstaller.node.ts—delete the commented call to
this.outputChannel.appendLine(l10n.t('Error: deepnote-toolkit not installed in
external venv')); (located near the error handling in the
DeepnoteToolkitInstaller logic) so the file contains no leftover commented code.
- Around line 223-254: Before kicking off installToolkitPackages in
installToolkitInExistingVenv, check whether the deepnote-toolkit is already
present in the given venv and return early if so; mirror the behavior from
ensureVenvAndToolkit (lines 116-133) by using the same existence check (e.g., a
helper that inspects the venv interpreter or runs a lightweight pip show/list)
against the venvInterpreter and only proceed to create and store the
installation promise in pendingInstallations when the toolkit is not already
installed, so you avoid redundant pip installs while preserving the
pendingInstallations logic and the finally cleanup.
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (1)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: RespectCancellationTokenin all async operations across kernel execution, session communication, and finder operations
Implement proper error handling in files using custom error types fromerrors/directory (KernelDeadError,KernelConnectionTimeoutError,KernelInterruptTimeoutError,JupyterInvalidKernelError,KernelDependencyError)
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
🧬 Code graph analysis (1)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)
src/platform/errors/deepnoteKernelErrors.ts (3)
DeepnoteToolkitMissingError(171-195)DeepnoteVenvCreationError(50-89)DeepnoteToolkitInstallError(94-169)src/kernels/deepnote/types.ts (2)
VenvAndToolkitInstallation(13-16)DEEPNOTE_TOOLKIT_VERSION(375-375)
🔇 Additional comments (3)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (3)
310-331: Good refactoring.Extracted shared logic and improved error handling. Typed errors are preserved without wrapping.
333-420: Solid shared helper.Consolidates toolkit installation logic with proper cancellation checks, verification, and error handling.
431-432: Cleaner return semantics.Explicitly returns
undefinedfor empty version strings instead of returning them as-is.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/kernels/deepnote/deepnoteToolkitInstaller.node.ts:
- Around line 229-233: The code returns a pendingInstall directly which lets
errors propagate, inconsistent with ensureVenvAndToolkit that catches
pending-install errors and retries; change this to mirror that pattern by
awaiting pendingInstall and attaching a .catch handler that logs the error,
removes the failed entry from pendingInstallations (venvKey) and then
calls/returns ensureVenvAndToolkit with the same parameters to retry, or
alternatively explicitly document why propagation is intentional if you want
different behavior.
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (1)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Platform Implementation - Desktop: Use
.node.tsfiles for full file system access and Python environments
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Usel10n.t()for all user-facing strings in TypeScript files
Use typed error classes fromsrc/platform/errors/instead of generic errors
UseILoggerservice instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationTokenOrder method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: RespectCancellationTokenin all async operations across kernel execution, session communication, and finder operations
Implement proper error handling in files using custom error types fromerrors/directory (KernelDeadError,KernelConnectionTimeoutError,KernelInterruptTimeoutError,JupyterInvalidKernelError,KernelDependencyError)
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
**/*.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS runnpm run format-fixbefore committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Usenpm run format-fixto auto-fix TypeScript formatting issues before committing
Usenpm run lintto check for linter issues in TypeScript files and attempt to fix before committing
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
UseUri.joinPath()for constructing file paths instead of string concatenation with/to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., useimport { generateUuid } from '../platform/common/uuid'instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports
Files:
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts
🧬 Code graph analysis (1)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (2)
src/platform/errors/deepnoteKernelErrors.ts (3)
DeepnoteToolkitMissingError(171-195)DeepnoteVenvCreationError(50-89)DeepnoteToolkitInstallError(94-169)src/kernels/deepnote/types.ts (2)
VenvAndToolkitInstallation(13-16)DEEPNOTE_TOOLKIT_VERSION(375-375)
⏰ 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
🔇 Additional comments (5)
src/kernels/deepnote/deepnoteToolkitInstaller.node.ts (5)
7-7: LGTM!Imports correctly aligned with new error types and cancellation handling used throughout.
Also applies to: 12-16
84-84: Managed venv handling looks correct.Path mismatch checks skip external venvs. Missing toolkit in non-managed venvs throws the appropriate error with interpreter context.
Also applies to: 94-94, 147-149
309-330: LGTM!Error handling correctly avoids double-wrapping typed errors while preserving context for unexpected failures.
336-419: LGTM!Shared helper cleanly consolidates pip upgrade, package installation, verification, and kernel spec setup. Cancellation checks placed appropriately.
430-431: LGTM!Explicit check for empty version string prevents false positives.
deepnote/deepnote#180
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.