feat: Track the environment information when executing blocks #254
Conversation
📝 WalkthroughWalkthroughThis PR introduces snapshot metadata capture for Deepnote notebook execution. New modules handle environment detection (Python version, packages, platform), timing, and content hashing. The snapshot service integrates into the kernel execution pipeline to record cell state changes and environment details before execution begins. Serialization logic is updated to persist these metadata fields. Service registration wires the new components through dependency injection. Type definitions are extended to carry execution metadata through the Deepnote block hierarchy. Sequence Diagram(s)sequenceDiagram
participant CellExecutionQueue
participant SnapshotMetadataService
participant EnvironmentCapture
participant Kernel as Kernel<br/>(execute cells)
participant DeepnoteSerializer
participant Storage
CellExecutionQueue->>SnapshotMetadataService: captureEnvironmentBeforeExecution(notebookUri)
activate SnapshotMetadataService
SnapshotMetadataService->>EnvironmentCapture: captureEnvironment(notebookUri)
activate EnvironmentCapture
EnvironmentCapture-->>SnapshotMetadataService: Environment {hash, packages, platform, python}
deactivate EnvironmentCapture
SnapshotMetadataService-->>CellExecutionQueue: environment captured & cached
deactivate SnapshotMetadataService
rect rgb(200, 220, 240)
Note over CellExecutionQueue,Kernel: Cell Execution Loop
loop per queued cell
CellExecutionQueue->>SnapshotMetadataService: recordCellExecutionStart(cellId, startTime)
CellExecutionQueue->>Kernel: execute cell
Kernel->>Kernel: cell runs
CellExecutionQueue->>SnapshotMetadataService: recordCellExecutionEnd(cellId, endTime, success)
CellExecutionQueue->>SnapshotMetadataService: updateContentHash(cellId, hash)
end
end
CellExecutionQueue->>DeepnoteSerializer: serialize notebook
activate DeepnoteSerializer
DeepnoteSerializer->>SnapshotMetadataService: getBlockExecutionMetadata(cellId)
SnapshotMetadataService-->>DeepnoteSerializer: {contentHash, executionStartedAt, executionFinishedAt}
DeepnoteSerializer->>SnapshotMetadataService: getEnvironmentMetadata()
SnapshotMetadataService-->>DeepnoteSerializer: Environment details
DeepnoteSerializer->>Storage: write notebook with metadata
deactivate DeepnoteSerializer
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
Files:
src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/notebooks/deepnote/**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)src/notebooks/deepnote/snapshotMetadataService.ts (2)
⏰ 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)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
===========================
===========================
🚀 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: 15
📜 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 ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
build/esbuild/build.tspackage.jsonsrc/kernels/execution/cellExecutionQueue.tssrc/kernels/kernelExecution.tssrc/kernels/kernelProvider.node.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/environmentCapture.unit.test.tssrc/notebooks/deepnote/pipFileParser.tssrc/notebooks/deepnote/pipFileParser.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/notebooks/serviceRegistry.node.tssrc/platform/deepnote/deepnoteTypes.tssrc/platform/deepnote/pocket.tssrc/platform/interpreter/helpers.tssrc/platform/notebooks/cellExecutionStateService.tssrc/platform/notebooks/deepnote/types.tstsconfig.base.json
🧰 Additional context used
📓 Path-based instructions (18)
**/*.{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:
build/esbuild/build.tssrc/notebooks/deepnote/pipFileParser.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/environmentCapture.unit.test.tssrc/kernels/kernelExecution.tssrc/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/pipFileParser.tssrc/platform/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/platform/deepnote/pocket.tssrc/platform/notebooks/cellExecutionStateService.tssrc/kernels/kernelProvider.node.tssrc/notebooks/deepnote/snapshotMetadataService.tssrc/platform/interpreter/helpers.tssrc/notebooks/serviceRegistry.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:
build/esbuild/build.tssrc/notebooks/deepnote/pipFileParser.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/environmentCapture.unit.test.tssrc/kernels/kernelExecution.tssrc/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/pipFileParser.tssrc/platform/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/platform/deepnote/pocket.tssrc/platform/notebooks/cellExecutionStateService.tssrc/kernels/kernelProvider.node.tssrc/notebooks/deepnote/snapshotMetadataService.tssrc/platform/interpreter/helpers.tssrc/notebooks/serviceRegistry.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/notebooks/deepnote/pipFileParser.unit.test.tssrc/notebooks/deepnote/environmentCapture.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.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/pipFileParser.unit.test.tssrc/notebooks/deepnote/environmentCapture.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.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/notebooks/deepnote/pipFileParser.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/environmentCapture.unit.test.tssrc/kernels/kernelExecution.tssrc/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/pipFileParser.tssrc/platform/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/platform/deepnote/pocket.tssrc/platform/notebooks/cellExecutionStateService.tssrc/kernels/kernelProvider.node.tssrc/notebooks/deepnote/snapshotMetadataService.tssrc/platform/interpreter/helpers.tssrc/notebooks/serviceRegistry.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/pipFileParser.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/environmentCapture.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/pipFileParser.tssrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.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/notebooks/deepnote/types.tssrc/platform/deepnote/deepnoteTypes.tssrc/platform/deepnote/pocket.tssrc/platform/notebooks/cellExecutionStateService.tssrc/platform/interpreter/helpers.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/kernelExecution.tssrc/kernels/execution/cellExecutionQueue.tssrc/kernels/kernelProvider.node.ts
src/kernels/{kernel.ts,kernelProvider.*.ts,execution/*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use event-driven updates via
EventEmitterfor kernel state changes and lifecycle events (willRestart, willInterrupt, onDidChangeKernels, etc.)
Files:
src/kernels/execution/cellExecutionQueue.tssrc/kernels/kernelProvider.node.ts
src/kernels/{kernelProvider.*.ts,execution/*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use weak references or proper cleanup patterns to avoid memory leaks with notebook references in kernel tracking and lifecycle management
Files:
src/kernels/execution/cellExecutionQueue.tssrc/kernels/kernelProvider.node.ts
src/kernels/execution/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Coordinate notebook-level execution through
NotebookKernelExecutionwhich managesCellExecutionQueuefor sequential cell execution and usesCellExecutionMessageHandlerto process kernel messages
Files:
src/kernels/execution/cellExecutionQueue.ts
src/kernels/{kernel.ts,kernelFinder.ts,execution/*.ts,**/launcher*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Add telemetry tracking for all kernel operations (discovery, startup, execution, errors) to enable usage analytics
Files:
src/kernels/execution/cellExecutionQueue.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/notebooks/deepnote/environmentCapture.node.tssrc/kernels/kernelProvider.node.tssrc/notebooks/serviceRegistry.node.ts
src/kernels/{kernelProvider.node.ts,kernelProvider.web.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Implement platform-specific variants for
KernelProviderand session communication:kernelProvider.node.tsfor Node.js environment with raw kernel support and ZeroMQ,kernelProvider.web.tsfor web environment with remote kernels only
Files:
src/kernels/kernelProvider.node.ts
src/kernels/{serviceRegistry.*.ts,kernelProvider.*.ts,common/base*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use dependency injection for all kernel system components to ensure testability and loose coupling between modules
Files:
src/kernels/kernelProvider.node.ts
src/kernels/{serviceRegistry.*.ts,kernelProvider.*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Implement lazy loading for finder activation, session creation, and Python extension feature integration to optimize performance
Files:
src/kernels/kernelProvider.node.ts
src/kernels/{kernelFinder.ts,kernelProvider.*.ts,**/session*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Implement caching strategies for kernel specs, connection metadata, and session reuse to reduce file system scanning and avoid unnecessary kernel restarts
Files:
src/kernels/kernelProvider.node.ts
src/kernels/{kernel.ts,kernelProvider.*.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use monitoring components (
KernelCrashMonitor,KernelAutoReConnectMonitor,KernelAutoRestartMonitor) to detect kernel failures and implement transparent recovery mechanisms
Files:
src/kernels/kernelProvider.node.ts
🧬 Code graph analysis (14)
src/notebooks/deepnote/pipFileParser.unit.test.ts (2)
src/notebooks/deepnote/environmentCapture.node.ts (2)
parsePipFreezeFile(46-46)parsePipFreezeFile(46-46)src/notebooks/deepnote/pipFileParser.ts (1)
parsePipFreezeFile(11-44)
src/platform/notebooks/deepnote/types.ts (1)
src/notebooks/deepnote/snapshotMetadataService.ts (1)
ISnapshotMetadataService(78-78)
src/kernels/kernelExecution.ts (1)
src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-138)
src/kernels/execution/cellExecutionQueue.ts (1)
src/platform/common/types.ts (1)
Resource(19-19)
src/notebooks/deepnote/deepnoteSerializer.ts (5)
src/notebooks/types.ts (2)
IDeepnoteNotebookManager(38-38)IDeepnoteNotebookManager(39-59)src/notebooks/deepnote/snapshotMetadataService.ts (2)
ISnapshotMetadataService(78-78)ISnapshotMetadataServiceFull(80-112)src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-138)src/platform/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(12-21)src/notebooks/deepnote/environmentCapture.node.ts (1)
computeHash(114-130)
src/notebooks/deepnote/deepnoteActivationService.ts (2)
src/notebooks/deepnote/snapshotMetadataService.ts (2)
ISnapshotMetadataService(78-78)ISnapshotMetadataServiceFull(80-112)src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-138)
src/notebooks/deepnote/pipFileParser.ts (1)
src/notebooks/deepnote/environmentCapture.node.ts (2)
parsePipFreezeFile(46-46)parsePipFreezeFile(46-46)
src/platform/deepnote/deepnoteTypes.ts (1)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
DeepnoteBlock(12-12)DeepnoteFile(12-12)
src/notebooks/deepnote/environmentCapture.node.ts (2)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
DeepnoteEnvironment(9-60)src/notebooks/deepnote/pipFileParser.ts (1)
parsePipFreezeFile(11-44)
src/notebooks/deepnote/snapshotMetadataService.unit.test.ts (1)
src/notebooks/deepnote/environmentCapture.node.ts (2)
IEnvironmentCapture(48-48)IEnvironmentCapture(50-55)
src/platform/notebooks/cellExecutionStateService.ts (4)
src/kernels/execution/cellExecutionCreator.ts (3)
cell(38-40)executionOrder(44-46)executionOrder(47-52)src/platform/logging/index.ts (1)
logger(35-48)src/kernels/execution/cellExecution.ts (1)
executionOrder(93-95)src/platform/common/utils/lifecycle.ts (1)
trackDisposable(26-29)
src/kernels/kernelProvider.node.ts (3)
src/kernels/raw/types.ts (2)
IRawNotebookSupportedService(59-59)IRawNotebookSupportedService(60-62)src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-138)src/kernels/kernelExecution.ts (1)
NotebookKernelExecution(53-388)
src/notebooks/deepnote/snapshotMetadataService.ts (3)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
injectable(51-381)src/notebooks/deepnote/environmentCapture.node.ts (3)
injectable(59-244)IEnvironmentCapture(48-48)IEnvironmentCapture(50-55)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/serviceRegistry.node.ts (2)
src/notebooks/deepnote/environmentCapture.node.ts (2)
IEnvironmentCapture(48-48)IEnvironmentCapture(50-55)src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-138)
🪛 GitHub Check: Lint & Format
src/notebooks/deepnote/environmentCapture.unit.test.ts
[warning] 3-3:
'Uri' is defined but never used. Allowed unused vars must match /_\w*/u
[warning] 2-2:
'sinon' is defined but never used. Allowed unused vars must match /_\w*/u
⏰ 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 (27)
src/platform/interpreter/helpers.ts (1)
136-148: Formatting improvement approved.The added blank lines at lines 140 and 146 improve readability by separating guard clauses from subsequent logic, with no impact to correctness. The function logic remains unchanged and sound.
src/platform/notebooks/cellExecutionStateService.ts (1)
6-6: Logging approach follows guidelines.Correctly uses
ILoggerservice and adds useful observability without behavior changes.Also applies to: 52-58, 61-61, 66-66, 74-74
build/esbuild/build.ts (1)
62-67: LGTM! Node.js built-ins correctly externalized.The additions align with environment capture functionality that requires process spawning and utility functions.
src/notebooks/deepnote/pipFileParser.ts (1)
11-44: LGTM! Pip freeze parser handles common formats correctly.The function properly handles standard (==), file URL (@), and editable (-e) formats, plus normalizes package names to lowercase.
src/platform/deepnote/deepnoteTypes.ts (1)
1-37: LGTM! Type extensions are backward compatible.The optional fields (contentHash, executionStartedAt, executionFinishedAt) preserve compatibility with existing code while enabling snapshot metadata tracking.
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
94-106: LGTM! Conditional metadata inclusion is correct.The spread syntax properly includes snapshot fields only when defined, maintaining backward compatibility.
src/platform/notebooks/deepnote/types.ts (1)
121-138: LGTM! Interface is well-defined.The blocking nature of
captureEnvironmentBeforeExecutionis properly documented, which is important for callers to understand the async behavior.package.json (1)
2503-2503: No public breaking changes documentation available for @deepnote/blocks 1.4.0.@deepnote/blocks is a private Deepnote package with no public npm release notes. The version bump from 1.3.5 to 1.4.0 was merged in commit 02816e1 with message "feat: Track the environment information when executing blocks", suggesting a feature addition. Verify API compatibility through internal Deepnote documentation or manual testing if @deepnote/blocks internals were used directly.
src/notebooks/deepnote/pipFileParser.unit.test.ts (1)
1-159: LGTM!Comprehensive test coverage for
parsePipFreezeFile. Correctly usesassert.deepStrictEqual()for object comparisons per guidelines.src/kernels/kernelProvider.node.ts (2)
55-57: LGTM!Optional DI wiring is correct. Service gracefully degrades when unavailable.
115-118: LGTM!Snapshot service correctly propagated to
NotebookKernelExecution.src/notebooks/serviceRegistry.node.ts (1)
245-248: LGTM!Correct singleton registration pattern. Binding to
IExtensionSyncActivationServiceensures lifecycle integration.src/notebooks/deepnote/snapshotMetadataService.unit.test.ts (1)
1-285: LGTM!Thorough coverage of state management methods. Correctly isolates per-notebook state and handles edge cases.
src/notebooks/deepnote/deepnoteSerializer.ts (2)
55-58: LGTM!Optional injection pattern correctly applied.
239-241: Index alignment is guaranteed by design.
convertCellsToBlocks()uses a simplemap()operation without filtering or reordering, soblocks[i]always corresponds todata.cells[i].src/kernels/execution/cellExecutionQueue.ts (1)
53-56: LGTM!Optional parameters cleanly extend the constructor without breaking existing callers.
src/notebooks/deepnote/deepnoteActivationService.ts (2)
43-43: LGTM!Serializer correctly receives the snapshot service.
32-33: No changes needed. The pattern of injecting by base interface symbol (ISnapshotMetadataService) while typing as the full interface (ISnapshotMetadataServiceFull) is correct—ISnapshotMetadataServiceFullextends the base interface, so the type hierarchy is valid. This is standard DI practice: inject by contract, type-hint actual implementation.Likely an incorrect or invalid review comment.
src/kernels/kernelExecution.ts (2)
69-70: LGTM!Clean optional parameter addition.
362-369: LGTM!Snapshot service correctly wired through to
CellExecutionQueue.src/platform/deepnote/pocket.ts (3)
10-18: LGTM!Array alphabetically sorted. New fields properly included.
22-33: LGTM!Good JSDoc comments for new optional fields.
95-110: LGTM!Consistent pattern with existing
executionCounthandling.src/notebooks/deepnote/snapshotMetadataService.ts (4)
24-33:contentHashis required but initialized empty elsewhere.At line 297,
contentHash: ''is used. If it's optional, make it so. If required, ensure it's always set.
131-150: LGTM!Clean activation pattern with proper disposal registration.
380-398: LGTM!State initialization is correct with proper defaults.
400-426: LGTM!Good use of VSCode's timing when available, sensible fallback.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (7)
cspell.jsonsrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/environmentCapture.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/platform/notebooks/cellExecutionStateService.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.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/environmentCapture.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.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/environmentCapture.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.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/notebooks/deepnote/environmentCapture.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/snapshotMetadataService.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/notebooks/deepnote/environmentCapture.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/snapshotMetadataService.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/notebooks/deepnote/environmentCapture.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/snapshotMetadataService.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/environmentCapture.unit.test.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotMetadataService.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/notebooks/deepnote/environmentCapture.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/notebooks/cellExecutionStateService.ts
🧬 Code graph analysis (5)
src/notebooks/deepnote/snapshotMetadataService.unit.test.ts (1)
src/notebooks/deepnote/environmentCapture.node.ts (2)
IEnvironmentCapture(47-47)IEnvironmentCapture(49-54)
src/notebooks/deepnote/environmentCapture.node.ts (2)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
DeepnoteEnvironment(9-60)src/notebooks/deepnote/pipFileParser.ts (1)
parsePipFreezeFile(11-44)
src/notebooks/deepnote/deepnoteSerializer.ts (3)
src/notebooks/types.ts (2)
IDeepnoteNotebookManager(38-38)IDeepnoteNotebookManager(39-59)src/notebooks/deepnote/snapshotMetadataService.ts (2)
ISnapshotMetadataService(77-77)ISnapshotMetadataServiceFull(79-111)src/platform/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(12-21)
src/platform/notebooks/cellExecutionStateService.ts (4)
src/kernels/execution/cellExecutionCreator.ts (3)
cell(38-40)executionOrder(44-46)executionOrder(47-52)src/platform/logging/index.ts (1)
logger(35-48)src/kernels/execution/cellExecution.ts (1)
executionOrder(93-95)src/platform/common/utils/lifecycle.ts (1)
trackDisposable(26-29)
src/notebooks/deepnote/snapshotMetadataService.ts (1)
src/notebooks/deepnote/environmentCapture.node.ts (2)
IEnvironmentCapture(47-47)IEnvironmentCapture(49-54)
⏰ 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 (14)
src/notebooks/deepnote/environmentCapture.unit.test.ts (1)
1-132: Well-structured unit tests for parsePipFreeze.Good coverage of edge cases: editable installs, local packages, CRLF, complex versions.
cspell.json (1)
29-71: Dictionary additions are appropriate.Terms align with environment capture functionality.
src/platform/notebooks/cellExecutionStateService.ts (1)
41-45: Prior feedback addressed.STATE_NAMES moved to module-level. Logging now uses
debuglevel.Also applies to: 57-79
src/notebooks/deepnote/snapshotMetadataService.unit.test.ts (1)
1-278: Solid test coverage for metadata service.Covers execution lifecycle, multi-notebook isolation, state clearing. Environment capture properly mocked.
src/notebooks/deepnote/deepnoteSerializer.ts (3)
55-58: Constructor extension looks correct.Optional injection with
@optional()handles missing snapshotService gracefully.
197-207: Serialization flow properly integrates snapshot metadata.Metadata added before cloning blocks, environment/execution added before YAML dump.
239-266: No issue here.convertCellsToBlocksuses.map()which preserves a strict 1:1 correspondence between input cells and output blocks at matching indices—no filtering or reordering occurs.Likely an incorrect or invalid review comment.
src/notebooks/deepnote/environmentCapture.node.ts (3)
23-42:withTimeoututility is well-implemented.Proper cleanup of timeout handle in finally block.
76-78: Windows pip path handling implemented.Addresses prior feedback about cross-platform support.
215-241: Package listing with timeout is robust.Returns empty object on failure, logs appropriately.
src/notebooks/deepnote/snapshotMetadataService.ts (4)
199-209: Timeout handling fixed correctly.Prior critical issue about
throwinsetTimeoutresolved withPromise.race.
292-306:recordCellExecutionStartcorrectly initializes cell metadata.Clears
executionFinishedAton re-execution.
400-426:handleCellExecutionStateChangehandles timing correctly.Uses VSCode's timing when available, defaults to
Date.now(). Success defaults totruewhen undefined.
120-127: Constructor is clean.Prior feedback about unused
interpreterServiceaddressed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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)
cspell.jsonsrc/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/snapshotMetadataService.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/notebooks/deepnote/environmentCapture.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/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/snapshotMetadataService.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/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/snapshotMetadataService.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/notebooks/deepnote/environmentCapture.node.tssrc/notebooks/deepnote/snapshotMetadataService.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/environmentCapture.node.tssrc/notebooks/deepnote/snapshotMetadataService.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/environmentCapture.node.ts (2)
src/kernels/deepnote/environments/deepnoteEnvironment.ts (1)
DeepnoteEnvironment(9-60)src/notebooks/deepnote/pipFileParser.ts (1)
parsePipFreezeFile(11-44)
🔇 Additional comments (2)
cspell.json (1)
26-81: LGTM!Package names align with environment capture scope. Alphabetical order maintained.
src/notebooks/deepnote/environmentCapture.node.ts (1)
1-247: LGTM! Past review issues have been addressed.All previously flagged issues have been resolved:
- Platform-aware pip path handling for Windows/Unix (lines 76-78)
- Proper return type with undefined handling (lines 90-94, 140-154)
- Timeout wrapper with cleanup (lines 38-40)
- Correct variable logging (line 229)
- Explicit return statements (line 183)
The environment capture implementation is solid with good error handling and cross-platform support.
|
@Artmann is there an issue for this PR? Is this in relation to deepnote/deepnote#187? |
dinohamzic
left a comment
dinohamzic
left a comment
There was a problem hiding this comment.
Mainly nits and questions, will test soon. 👀
| async function withTimeout<T>(promise: Promise<T>, timeoutMs: number, fallback: T, operationName: string): Promise<T> { | ||
| let timeoutId: NodeJS.Timeout | undefined; | ||
|
|
||
| const timeoutPromise = new Promise<T>((resolve) => { | ||
| timeoutId = setTimeout(() => { | ||
| logger.warn(`[EnvironmentCapture] ${operationName} timed out after ${timeoutMs}ms`); | ||
| resolve(fallback); | ||
| }, timeoutMs); | ||
| }); | ||
|
|
||
| try { | ||
| const result = await Promise.race([promise, timeoutPromise]); | ||
|
|
||
| return result; | ||
| } finally { | ||
| if (timeoutId) { | ||
| clearTimeout(timeoutId); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: might be worth extracting to a shared helper? Have you also checked in the codebase if raceTimeout is doing something similar?
| export const onDidChangeNotebookCellExecutionState = eventEmitter.event; | ||
|
|
||
| export function changeCellState(cell: NotebookCell, state: NotebookCellExecutionState, executionOrder?: number) { | ||
| const cellId = cell.metadata?.id as string | undefined; |
There was a problem hiding this comment.
Is the cast here necessary? 👀
| return; | ||
| } | ||
| // Wait for VS Code to update the cell execution state before firing the event. | ||
| logger.debug(`[CellExecState] Waiting for VS Code to update cell before firing Idle event`); |
There was a problem hiding this comment.
micro nit: should we be editor agnostic? that is, avoiding using the "VS Code" name in favor of a more generic "Editor"?
| export interface DeepnoteBlock extends BaseDeepnoteBlock { | ||
| /** SHA-256 hash of block content (prefixed with "sha256:") */ | ||
| contentHash?: string; | ||
|
|
||
| /** ISO 8601 timestamp when block execution started */ | ||
| executionStartedAt?: string; | ||
|
|
||
| /** ISO 8601 timestamp when block execution finished */ | ||
| executionFinishedAt?: string; | ||
| } |
There was a problem hiding this comment.
Just a question: what's the reason for not having these optional fields directly in DeepnoteBlock?
Example:
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.