feat: Create snapshots when running notebooks #256
Conversation
📝 WalkthroughWalkthroughThis PR introduces a snapshot feature for Deepnote notebooks that persistently stores cell execution outputs in separate Sequence DiagramssequenceDiagram
participant Cell as Cell Execution
participant Queue as Execution Queue
participant Monitor as Event Monitor
participant Service as SnapshotService
participant FS as File System
participant Config as Configuration
Cell->>Queue: Execute cells
Queue->>Monitor: notifyQueueComplete(notebookUri)
Monitor->>Service: onDidCompleteQueueExecution event
Service->>Config: isSnapshotsEnabled()?
alt Snapshots enabled
Service->>Service: Aggregate execution metadata
Service->>Service: Compute snapshot hash
Service->>FS: Check for changes vs latest
alt Changes detected
Service->>FS: Write timestamped snapshot
Service->>FS: Update latest symlink
end
end
sequenceDiagram
participant Deserializer as Deserializer
participant Service as SnapshotService
participant FS as File System
participant Blocks as Block Converter
Deserializer->>Service: isSnapshotsEnabled()?
alt Snapshots enabled
Deserializer->>Service: readSnapshot(projectId)
Service->>FS: Parse latest/timestamped YAML
Service->>Deserializer: Return outputs map
Deserializer->>Blocks: mergeOutputsIntoBlocks(blocks, outputs)
else Snapshots disabled
Deserializer->>Blocks: Use outputs from main file
end
Blocks->>Deserializer: Return hydrated blocks
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 #256 +/- ##
===========================
===========================
🚀 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: 10
📜 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 (11)
package.jsonsrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/serviceRegistry.node.tssrc/platform/common/constants.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/platform/common/constants.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/snapshotFileService.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/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/snapshotFileService.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/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.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/snapshotFileService.unit.test.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.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/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshotFileService.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/common/constants.ts
🧬 Code graph analysis (2)
src/notebooks/serviceRegistry.node.ts (2)
src/notebooks/deepnote/snapshotFileService.node.ts (1)
ISnapshotFileService(10-10)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-45)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-138)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-45)
⏰ 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 (16)
src/notebooks/deepnote/deepnoteNotebookManager.ts (1)
15-15: LGTM! Correct alphabetical ordering.Field reordering follows the coding guideline for private fields.
src/notebooks/serviceRegistry.node.ts (2)
92-92: LGTM.Import follows established pattern in this file.
250-250: LGTM.Correct DI registration for the snapshot file service.
src/platform/common/constants.ts (1)
227-228: LGTM.Command constants follow established patterns and naming conventions.
src/notebooks/deepnote/deepnoteActivationService.ts (1)
34-35: LGTM.Optional DI injection is correctly implemented, and serializer construction properly passes both snapshot services.
Also applies to: 45-49
src/notebooks/deepnote/snapshotFileServiceTypes.ts (1)
1-45: LGTM.Well-documented interface with clear method signatures. Follows established DI patterns with Symbol-based export.
src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts (1)
21-21: LGTM.Test scaffolding properly updated to accommodate IConfigurationService dependency. Mock implementation is appropriate and consistently applied.
Also applies to: 32-46, 91-91
package.json (1)
1648-1653: LGTM.Config property correctly defined with resource scope—appropriate for per-notebook snapshot settings.
src/notebooks/deepnote/snapshotFileService.unit.test.ts (3)
83-143: Good coverage of mergeOutputsIntoBlocks edge cases.Tests cover: ID matching, missing matches, empty maps, empty arrays.
145-226: Solid immutability testing.Line 194-208 correctly verifies original blocks aren't mutated—important for data integrity.
228-310: LGTM.Edge cases well-covered: missing IDs, missing outputs, complex output structures.
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (1)
143-146: LGTM.Constructor injection follows DI pattern correctly. Ordering alphabetical per guidelines.
src/notebooks/deepnote/deepnoteSerializer.ts (2)
60-62: LGTM.Optional injection with
@optional()decorator correctly used for gradual feature adoption.
394-405: LGTM.Helper correctly finds project URI and strips query params for clean path construction.
src/notebooks/deepnote/snapshotFileService.node.ts (2)
16-23: LGTM.Slugification handles edge cases: multiple spaces, leading/trailing hyphens.
175-178: May overwrite existing createdAt.Line 177 initializes metadata with new
createdAteven ifprojectData.metadataalready has one.-snapshotData.metadata = snapshotData.metadata || { createdAt: new Date().toISOString() }; +if (!snapshotData.metadata) { + snapshotData.metadata = { createdAt: new Date().toISOString() }; +}Likely an incorrect or invalid review comment.
| async createSnapshot( | ||
| projectUri: Uri, | ||
| projectId: string, | ||
| projectName: string, | ||
| projectData: DeepnoteFile | ||
| ): Promise<Uri | undefined> { | ||
| const latestPath = this.buildSnapshotPath(projectUri, projectId, projectName, 'latest'); | ||
| const snapshotsDir = Uri.joinPath(latestPath, '..'); | ||
|
|
||
| // Ensure snapshots directory exists | ||
| try { | ||
| await workspace.fs.stat(snapshotsDir); | ||
| } catch { | ||
| logger.debug(`[SnapshotFileService] Creating snapshots directory: ${snapshotsDir.fsPath}`); | ||
| await workspace.fs.createDirectory(snapshotsDir); | ||
| } | ||
|
|
||
| // Check if there are changes compared to the existing latest snapshot | ||
| const hasChanges = await this.hasSnapshotChanges(latestPath, projectData); | ||
|
|
||
| if (!hasChanges) { | ||
| logger.debug(`[SnapshotFileService] No changes detected, skipping snapshot creation`); | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| const timestamp = generateTimestamp(); | ||
| const timestampedPath = this.buildSnapshotPath(projectUri, projectId, projectName, timestamp); | ||
|
|
||
| // Prepare snapshot data with timestamp | ||
| const snapshotData = structuredClone(projectData); | ||
|
|
||
| snapshotData.metadata = snapshotData.metadata || { createdAt: new Date().toISOString() }; | ||
| snapshotData.metadata.modifiedAt = new Date().toISOString(); | ||
|
|
||
| const yamlString = yaml.dump(snapshotData, { | ||
| indent: 2, | ||
| lineWidth: -1, | ||
| noRefs: true, | ||
| sortKeys: false | ||
| }); | ||
|
|
||
| const content = new TextEncoder().encode(yamlString); | ||
|
|
||
| // Write to timestamped file first (safe - doesn't touch existing files) | ||
| await workspace.fs.writeFile(timestampedPath, content); | ||
| logger.debug(`[SnapshotFileService] Wrote timestamped snapshot: ${timestampedPath.fsPath}`); | ||
|
|
||
| // Copy to latest (only after timestamped write succeeded) | ||
| await workspace.fs.writeFile(latestPath, content); | ||
| logger.debug(`[SnapshotFileService] Updated latest snapshot: ${latestPath.fsPath}`); | ||
|
|
||
| return timestampedPath; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Race condition between timestamped and latest writes.
If process crashes after line 190 but before line 194, latest snapshot becomes stale. Consider using a temp file and atomic rename for the latest file.
Artmann
changed the title
Artmann
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteSerializer.ts:
- Around line 217-257: The fallback branch when findProjectUriFromId(projectId)
returns falsy silently assigns notebook.blocks but lacks logging; add a debug
log via logger.debug inside the else that mentions projectId and that snapshot
URI was unavailable (e.g., "SerializeNotebook: projectUri not found for
projectId={projectId}, writing full outputs to main file"), so the fallback path
in the snapshot handling around projectUri,
snapshotFileService.isSnapshotsEnabled(), and notebook.blocks is observable for
troubleshooting.
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 17-24: slugifyProjectName can return an empty string when the
input name contains only non-alphanumeric characters; update slugifyProjectName
to detect an empty result and return a sensible fallback (e.g., a default slug
like "untitled-project" or a derived fallback such as a truncated hash/UUID) so
callers never receive an empty slug. Locate the slugifyProjectName function and
after the existing chain of replacements, check if the resulting string is empty
and if so return the chosen fallback value; ensure the fallback is deterministic
or clearly documented depending on needs.
📜 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 (6)
package.jsonpackage.nls.jsonsrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.node.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/deepnoteActivationService.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.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/deepnoteActivationService.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.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/deepnoteActivationService.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.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/deepnoteActivationService.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.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/snapshotFileService.node.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteSerializer.ts (3)
src/notebooks/deepnote/snapshotMetadataService.ts (2)
ISnapshotMetadataService(77-77)ISnapshotMetadataServiceFull(79-111)src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-138)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-45)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
error(30-37)src/platform/logging/index.ts (1)
logger(35-48)
⏰ 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 (18)
package.nls.json (1)
253-254: LGTM!New i18n entries follow existing conventions.
package.json (2)
102-111: LGTM!Commands registered correctly with proper i18n references.
1648-1653: LGTM!Configuration uses
"scope": "resource"enabling per-workspace control.src/notebooks/deepnote/deepnoteActivationService.ts (2)
34-35: LGTM!Optional injection allows graceful degradation when snapshot services unavailable.
45-49: LGTM!Serializer correctly receives all three dependencies.
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (3)
143-146: LGTM!Constructor injects required services correctly.
188-191: LGTM!Commands registered and wired to handlers.
380-408: Setting key prefix is correct.updateSetting('snapshots.enabled', ...)correctly relies onIConfigurationServiceprepending'deepnote.'prefix—the implementation explicitly callsupdateSectionSetting('deepnote', setting, ...), resulting in the correctdeepnote.snapshots.enabledconfiguration key.src/notebooks/deepnote/deepnoteSerializer.ts (3)
60-61: LGTM!Optional dependency follows existing pattern.
119-134: LGTM!Reads snapshot outputs and merges before cell conversion.
structuredClonepreserves original blocks.
395-406: LGTM!Strips query params to get clean project URI for snapshot paths.
src/notebooks/deepnote/snapshotFileService.node.ts (7)
39-47: LGTM!Reads configuration with safe default.
56-67: LGTM!Uses
Uri.joinPathper coding guidelines.
75-114: LGTM!Fallback from latest to timestamped is sound. Lexicographic sort works for ISO timestamps.
146-199: LGTM!Atomic write pattern: timestamped first, then latest. Directory creation handles missing folder.
261-268: LGTM!Returns new array; original untouched.
244-255: In-place mutation is already properly handled.The method does mutate blocks in place, and the doc comment documents this. The only production caller in
deepnoteSerializer.tsalready passes a cloned array viastructuredClone()before calling the method. No action required.
205-226: Key ordering is deterministic here—no risk of false positives.
getComparableProjectContent()explicitly constructs an object with fixed key order (version→project→environment→execution), and both objects are processed identically. Modern JavaScript preserves insertion order, soJSON.stringify()will produce consistent output. Also, the method doesn't includenotebooksas mentioned in the concern; keys come from the explicit object literal.Likely an incorrect or invalid review comment.
| // Handle snapshot file logic if enabled | ||
| if (this.snapshotFileService?.isSnapshotsEnabled()) { | ||
| const projectUri = this.findProjectUriFromId(projectId); | ||
|
|
||
| if (projectUri) { | ||
| logger.debug('SerializeNotebook: Snapshots enabled, creating snapshot'); | ||
|
|
||
| // Create snapshot project with full outputs | ||
| const snapshotProject = cloneWithoutCircularRefs(originalProject) as DeepnoteFile; | ||
| const snapshotNotebook = snapshotProject.project.notebooks.find( | ||
| (nb: { id: string }) => nb.id === notebookId | ||
| ); | ||
|
|
||
| if (snapshotNotebook) { | ||
| snapshotNotebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | ||
| } | ||
|
|
||
| // Create snapshot if there are changes (writes timestamped first, then copies to latest) | ||
| const snapshotUri = await this.snapshotFileService.createSnapshot( | ||
| projectUri, | ||
| projectId, | ||
| originalProject.project.name, | ||
| snapshotProject | ||
| ); | ||
|
|
||
| // Strip outputs from main file blocks | ||
| notebook.blocks = this.snapshotFileService.stripOutputsFromBlocks(blocks); | ||
|
|
||
| if (snapshotUri) { | ||
| logger.debug('SerializeNotebook: Created snapshot and stripped outputs from main file'); | ||
| } else { | ||
| logger.debug('SerializeNotebook: No changes, skipped snapshot creation'); | ||
| } | ||
| } else { | ||
| // Fallback if we can't find the project URI | ||
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | ||
| } | ||
| } else { | ||
| // Default behavior: outputs in main file | ||
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging the fallback path.
Line 252: Silent fallback when projectUri not found. Adding a debug log would aid troubleshooting.
Suggested change
} else {
// Fallback if we can't find the project URI
+ logger.debug('SerializeNotebook: Could not find project URI, skipping snapshot');
notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle snapshot file logic if enabled | |
| if (this.snapshotFileService?.isSnapshotsEnabled()) { | |
| const projectUri = this.findProjectUriFromId(projectId); | |
| if (projectUri) { | |
| logger.debug('SerializeNotebook: Snapshots enabled, creating snapshot'); | |
| // Create snapshot project with full outputs | |
| const snapshotProject = cloneWithoutCircularRefs(originalProject) as DeepnoteFile; | |
| const snapshotNotebook = snapshotProject.project.notebooks.find( | |
| (nb: { id: string }) => nb.id === notebookId | |
| ); | |
| if (snapshotNotebook) { | |
| snapshotNotebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } | |
| // Create snapshot if there are changes (writes timestamped first, then copies to latest) | |
| const snapshotUri = await this.snapshotFileService.createSnapshot( | |
| projectUri, | |
| projectId, | |
| originalProject.project.name, | |
| snapshotProject | |
| ); | |
| // Strip outputs from main file blocks | |
| notebook.blocks = this.snapshotFileService.stripOutputsFromBlocks(blocks); | |
| if (snapshotUri) { | |
| logger.debug('SerializeNotebook: Created snapshot and stripped outputs from main file'); | |
| } else { | |
| logger.debug('SerializeNotebook: No changes, skipped snapshot creation'); | |
| } | |
| } else { | |
| // Fallback if we can't find the project URI | |
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } | |
| } else { | |
| // Default behavior: outputs in main file | |
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } | |
| // Handle snapshot file logic if enabled | |
| if (this.snapshotFileService?.isSnapshotsEnabled()) { | |
| const projectUri = this.findProjectUriFromId(projectId); | |
| if (projectUri) { | |
| logger.debug('SerializeNotebook: Snapshots enabled, creating snapshot'); | |
| // Create snapshot project with full outputs | |
| const snapshotProject = cloneWithoutCircularRefs(originalProject) as DeepnoteFile; | |
| const snapshotNotebook = snapshotProject.project.notebooks.find( | |
| (nb: { id: string }) => nb.id === notebookId | |
| ); | |
| if (snapshotNotebook) { | |
| snapshotNotebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } | |
| // Create snapshot if there are changes (writes timestamped first, then copies to latest) | |
| const snapshotUri = await this.snapshotFileService.createSnapshot( | |
| projectUri, | |
| projectId, | |
| originalProject.project.name, | |
| snapshotProject | |
| ); | |
| // Strip outputs from main file blocks | |
| notebook.blocks = this.snapshotFileService.stripOutputsFromBlocks(blocks); | |
| if (snapshotUri) { | |
| logger.debug('SerializeNotebook: Created snapshot and stripped outputs from main file'); | |
| } else { | |
| logger.debug('SerializeNotebook: No changes, skipped snapshot creation'); | |
| } | |
| } else { | |
| // Fallback if we can't find the project URI | |
| logger.debug('SerializeNotebook: Could not find project URI, skipping snapshot'); | |
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } | |
| } else { | |
| // Default behavior: outputs in main file | |
| notebook.blocks = cloneWithoutCircularRefs<DeepnoteBlock[]>(blocks); | |
| } |
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/deepnoteSerializer.ts around lines 217 - 257, The
fallback branch when findProjectUriFromId(projectId) returns falsy silently
assigns notebook.blocks but lacks logging; add a debug log via logger.debug
inside the else that mentions projectId and that snapshot URI was unavailable
(e.g., "SerializeNotebook: projectUri not found for projectId={projectId},
writing full outputs to main file"), so the fallback path in the snapshot
handling around projectUri, snapshotFileService.isSnapshotsEnabled(), and
notebook.blocks is observable for troubleshooting.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 212-233: The JSON.stringify comparison in hasSnapshotChanges is
brittle due to property order; replace the string comparison with a proper deep
equality check by importing and using a stable deep-equal function (e.g.,
lodash.isEqual or fast-deep-equal) to compare the results of
getComparableProjectContent(existingData) and
getComparableProjectContent(projectData); keep the existing try/catch and
behavior (return true on read error) but change the return to use
deepEqual(existingProject, newProject) negated instead of comparing JSON
strings.
📜 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/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotFileService.unit.test.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/snapshotFileService.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/snapshotFileService.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/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.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/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.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/snapshotFileService.node.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/snapshotFileService.unit.test.ts (1)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
DeepnoteBlock(14-14)DeepnoteOutput(15-15)
src/notebooks/deepnote/snapshotFileService.node.ts (2)
src/notebooks/deepnote/deepnoteSerializer.ts (4)
injectable(54-451)DeepnoteOutput(15-15)DeepnoteFile(14-14)DeepnoteBlock(14-14)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-45)
⏰ 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/notebooks/deepnote/snapshotFileService.unit.test.ts (1)
1-344: Comprehensive test coverage.The test suite thoroughly validates path building, output merging/stripping/extraction, edge cases, and error scenarios. Well-structured and follows coding guidelines.
src/notebooks/deepnote/snapshotFileService.node.ts (11)
18-31: LGTM.Slugification logic correctly handles spaces, special characters, and validates non-empty results.
37-39: LGTM.Timestamp generation is straightforward and produces filesystem-safe filenames.
50-54: LGTM.Configuration reading is straightforward.
63-74: LGTM.Path construction correctly uses
Uri.joinPath()per coding guidelines.
82-121: LGTM.Snapshot discovery logic correctly prioritizes latest snapshot, falls back to newest timestamped file, and handles sorting/filtering properly.
126-144: LGTM.Snapshot parsing safely uses optional chaining and validates block IDs and outputs.
153-206: LGTM.Atomic write pattern (timestamped first, then latest) ensures data integrity. Change detection prevents unnecessary writes.
238-245: LGTM.Correctly excludes timestamp metadata from comparison.
251-262: LGTM.In-place mutation correctly matches the interface contract and is well-tested.
268-275: LGTM.Immutable output stripping via destructuring is clean and correct.
281-291: LGTM.Extraction logic correctly validates block IDs and outputs.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 77-122: The readSnapshot method lacks error handling around
parseSnapshotFile and searches all workspace roots with an expensive recursive
glob; wrap both calls to this.parseSnapshotFile(latestFiles[0]) and
this.parseSnapshotFile(newestFile) in try-catch blocks that log the error (using
logger.error) and return undefined (or a well-documented failure value) instead
of letting exceptions bubble; scope workspace.findFiles by passing a
RelativePattern (or a workspaceFolder argument) instead of a bare glob so
results are limited to the intended project workspace, and tighten the glob
(e.g., remove the leading **/ or target the snapshots/ directory directly) to
reduce cost—update references: readSnapshot, latestPattern, timestampedPattern,
workspace.findFiles, parseSnapshotFile, and sortedFiles accordingly.
- Around line 124-145: parseSnapshotFile currently assumes yaml.load succeeds
and snapshotData has the expected shape, which can crash on malformed YAML or
unexpected structure; wrap the file read + yaml.load in a try/catch, log the
parsing error via logger.error including the caught error, and either rethrow a
descriptive Error or return an empty outputsMap; after parsing, validate that
snapshotData is an object and snapshotData.project?.notebooks is an array before
iterating (skip or log and return empty map if not), and ensure block and
block.outputs are type-checked before calling outputsMap.set so malformed
structures don't throw.
📜 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/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/snapshotFileService.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/snapshotFileService.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/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/snapshotFileService.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/snapshotFileService.node.ts
⏰ 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 (8)
src/notebooks/deepnote/snapshotFileService.node.ts (8)
14-32: LGTM - robust slugification.Handles edge cases and throws on empty results. Combined with projectId in filenames prevents collisions.
34-40: LGTM.
48-55: LGTM.
57-75: LGTM - follows Uri.joinPath guidelines.
209-231: LGTM - appropriate change detection.Correctly excludes metadata timestamps and treats read failures as changed state.
233-243: LGTM.
245-260: LGTM.
262-289: LGTM - clean utility methods.Both methods correctly handle their transformations without side effects.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
57-68: Add the Disable Snapshots command to the command palette table.The PR includes both
Deepnote: Enable SnapshotsandDeepnote: Disable Snapshotscommands, but the table documents only the enable command. Add a row for the disable command to maintain consistency with the feature's symmetric interface.📝 Proposed addition to the command table
| `Notebook: Change Cell Language` | Change the language of the cell currently in focus | +| `Deepnote: Disable Snapshots` | Disable snapshot mode for the current workspace | | `Deepnote: Enable Snapshots` | Enable snapshot mode for the current workspace |
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteSerializer.ts:
- Around line 119-134: Wrap the snapshot read/merge block in a try/catch so
failures in snapshotFileService.readSnapshot or
snapshotFileService.mergeOutputsIntoBlocks don't crash deserialization: catch
errors around the existing snapshot logic (the readSnapshot call and
mergeOutputsIntoBlocks invocation), log the exception via logger with context
(e.g., include projectId and that merging failed), and fall back to the original
cells computed from this.converter.convertBlocksToCells(selectedNotebook.blocks
?? []) so the function returns the baseline cells when snapshot handling fails.
In @src/notebooks/deepnote/deepnoteTreeDataProvider.ts:
- Line 206: Extract the repeated string '.snapshot.deepnote' into a single
constant (e.g. SNAPSHOT_FILE_SUFFIX) declared near the top of the file, then
replace all literal occurrences with that constant—specifically update the
projectFiles filter (projectFiles = files.filter(...)) and the three
file-watcher guard checks so they use SNAPSHOT_FILE_SUFFIX.endsWith/endsWith
comparisons instead of the repeated literal.
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 318-380: updateLatestSnapshot duplicates directory creation,
change detection, metadata handling and YAML serialization found in
createSnapshot; extract the shared logic into a helper (e.g.
prepareSnapshotData) that takes (projectUri, projectId, projectName,
projectData) and returns { latestPath, content } or undefined when no changes,
move the directory creation into an ensureDirectoryExists helper used by both,
reuse hasSnapshotChanges and buildSnapshotPath inside the helper, and then have
updateLatestSnapshot call the helper and only perform the final
workspace.fs.writeFile(latestPath, content) and logging.
- Around line 19-32: Replace the generic Error thrown in slugifyProjectName with
a typed error from src/platform/errors: import the appropriate class (e.g.,
ValidationError or InvalidInputError) and throw that (e.g., throw new
ValidationError('Project name cannot be empty or contain only special
characters')) so callers can programmatically detect this validation failure;
update the import list at the top of the file and keep the message intact.
📜 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 (9)
README.mdsrc/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotMetadataService.tssrc/notebooks/notebookCommandListener.tssrc/platform/notebooks/deepnote/types.ts
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{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/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/notebookCommandListener.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotMetadataService.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.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/execution/cellExecutionQueue.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.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
**/*.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/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/notebookCommandListener.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/snapshotFileService.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/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/notebookCommandListener.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/snapshotFileService.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/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/snapshotFileService.node.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.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/snapshotFileService.node.ts
🧬 Code graph analysis (5)
src/kernels/execution/cellExecutionQueue.ts (2)
src/platform/logging/outputChannelLogger.ts (1)
error(33-35)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)
src/notebooks/notebookCommandListener.ts (2)
src/kernels/kernelStatusProvider.ts (2)
IKernelStatusProvider(14-14)IKernelStatusProvider(15-17)src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-157)
src/notebooks/deepnote/snapshotFileService.node.ts (1)
src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)
src/notebooks/deepnote/snapshotMetadataService.ts (3)
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
DeepnoteDataConverter(37-560)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)src/notebooks/deepnote/deepnoteSerializer.ts (2)
DeepnoteFile(14-14)DeepnoteBlock(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (19)
README.md (1)
92-108: Snapshot mode documentation is clear and well-structured.The explanation of how snapshots work, the benefits for Git workflows, and the enablement steps are clear and align with the feature's design.
src/platform/notebooks/deepnote/types.ts (1)
139-156: LGTM: Clear interface additions for Run All snapshot workflow.The three new methods properly extend the service interface with clear documentation. Using string-based URIs maintains consistency with the existing
captureEnvironmentBeforeExecutionmethod.src/kernels/execution/cellExecutionQueue.ts (1)
328-335: LGTM: Post-execution hook properly integrated.The hook correctly notifies the snapshot service after cell execution completes. Error handling with
logger.warnis appropriate, and the null checks prevent issues when snapshot service isn't available.src/notebooks/notebookCommandListener.ts (2)
58-59: Verify: Required injection may cause issues if service unavailable.The
ISnapshotMetadataServiceis injected as a required dependency (no@optional()decorator). This means the service must always be registered, or DI will fail. Consider making it optional if snapshots are an optional feature.Based on coding guidelines, use
@optional()for optional platform features:@inject(ISnapshotMetadataService) @optional() private snapshotMetadataService?: ISnapshotMetadataServiceThen add a null check before usage at Line 121.
119-121: LGTM: Run All mode set before execution.The snapshot service is correctly notified before executing the notebook. The synchronous call is appropriate since
setRunAllModeis a void method per the interface.src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
206-208: LGTM: Snapshot files filtered from tree view.The filtering correctly excludes snapshot files from the project tree, and Line 208 properly iterates over the filtered set rather than all files.
src/notebooks/deepnote/deepnoteSerializer.ts (2)
218-225: LGTM: Conditional output handling for snapshot mode.The serializer correctly strips outputs when snapshots are enabled, preserving the original behavior otherwise. The comment clarifies that snapshots are created during execution, not on save.
60-61: LGTM: Optional dependency injection pattern.Using
@optional()forISnapshotFileServicecorrectly handles the case where snapshots may not be available. This follows the established pattern in Line 60 forsnapshotService.src/notebooks/deepnote/snapshotFileServiceTypes.ts (1)
1-57: Well-designed interface.Clean separation of concerns. Method signatures correctly distinguish between full snapshots (timestamped + latest) and partial updates (latest only). JSDoc comments are helpful.
src/notebooks/deepnote/snapshotMetadataService.ts (5)
126-128: LGTM.Direct instantiation of
DeepnoteDataConverteris fine for a stateless utility class.
130-135: Optional injection pattern looks correct.Graceful degradation when snapshot service is unavailable.
547-553: Works only for open notebooks.This searches
workspace.notebookDocuments, so it only finds URIs for currently open notebooks. Fine here since it's called during active execution.
276-310: Snapshot branching logic is sound.Run All → full snapshot with history. Partial run → latest only. Silent failures with logging are acceptable for this non-critical feature.
259-266: No issues found. TheconvertCellsToBlocksmethod exists onDeepnoteDataConverterand is correctly used at line 266.src/notebooks/deepnote/snapshotFileService.node.ts (5)
83-153: Snapshot discovery logic is correct.Latest first, fallback to newest timestamped. The 100-file limit is reasonable.
386-416: Change detection is correct.Using
fast-deep-equalwith metadata exclusion ensures timestamps don't trigger false positives.
422-433: LGTM.In-place mutation is documented. Correct merge logic.
439-446: Clean output stripping.Immutable operation via rest spread. Type assertion is necessary here.
452-462: LGTM.Correctly filters blocks without id or outputs.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteTreeDataProvider.ts:
- Around line 323-325: Duplicate snapshot-file guard logic across the file
watcher handlers (onDidChange, onDidCreate, onDidDelete) should be extracted
into a private helper to reduce repetition: add a method (e.g., private
isSnapshotFile(uri: Uri): boolean) that checks
uri.path.endsWith(SNAPSHOT_FILE_SUFFIX) and replace the inline checks in the
three handlers with calls to this helper, returning early when true; update
references in deepnoteTreeDataProvider.ts to use isSnapshotFile instead of
repeating the endsWith check.
In @src/notebooks/deepnote/errors.ts:
- Around line 6-11: The InvalidProjectNameError class is defined as a plain
Error with a hardcoded user-facing message but is used across modules; move the
InvalidProjectNameError definition into the shared platform errors module where
other BaseError subclasses live, change it to extend BaseError, wrap the default
message with l10n.t('Project name cannot be empty or contain only special
characters'), and call super({ message }) in the constructor while retaining
this.name = 'InvalidProjectNameError' so existing imports (e.g. usages in
snapshotFileService.node.ts) keep working.
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 20-33: slugifyProjectName currently assumes name is a non-empty
string and will throw or behave unpredictably for null/undefined or non-string
inputs; add an input guard at the start of slugifyProjectName that checks typeof
name === 'string' and that name.trim() is non-empty, and if not, throw
InvalidProjectNameError (or coerce to string only after validation); then
continue using the trimmed value for the subsequent transformations to avoid
runtime errors on invalid inputs.
📜 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)
cspell.jsonsrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/errors.tssrc/notebooks/deepnote/snapshotFileService.node.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/errors.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/errors.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/errors.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.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/errors.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.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/snapshotFileService.node.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)
src/notebooks/deepnote/snapshotFileService.node.ts (3)
src/notebooks/deepnote/errors.ts (1)
InvalidProjectNameError(6-11)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint & Format
- GitHub Check: Build & Test
🔇 Additional comments (20)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (2)
21-22: LGTM: Clear constant definition.Constant naming and documentation are appropriate for filtering snapshot files from the explorer view.
209-211: LGTM: Filtering logic is correct.Snapshot files are properly excluded from project file discovery, and the loop correctly iterates over the filtered list.
cspell.json (1)
81-84: LGTM!Spell-check vocabulary expansion aligns with new slugification utilities.
src/notebooks/deepnote/deepnoteSerializer.ts (4)
11-11: LGTM!Import follows established patterns for local file imports.
60-61: LGTM!Optional injection enables graceful degradation when snapshot feature is disabled.
224-233: LGTM!Snapshot mode correctly strips outputs from main file. Note: different cloning strategies (structuredClone vs. cloneWithoutCircularRefs) are appropriate for their respective contexts.
119-142: Use availability check forstructuredClonelike deepnoteExplorerView.ts does.Line 130 uses
structuredClonewithout verifying availability. The same module'sdeepnoteExplorerView.tsalready implements a safe pattern—checktypeof structuredClone !== 'undefined'and fall back toJSON.parse(JSON.stringify())for broader Node.js compatibility.src/notebooks/deepnote/snapshotFileService.node.ts (13)
52-56: LGTM!Configuration check is straightforward and correct.
65-76: LGTM!Path construction follows coding guidelines using
Uri.joinPath().
84-154: LGTM!Fallback logic (latest → newest timestamped) is well-structured with appropriate error handling. Note: 100-file limit at line 121 is reasonable but could be reached in long-lived projects.
221-242: LGTM!Directory creation with proper error handling and boolean success indicator.
249-288: LGTM!Snapshot preparation logic is sound. Note:
structuredCloneat line 273 requires Node 17+ (same consideration as deepnoteSerializer.ts).
302-348: LGTM!Two-write strategy (timestamped first, then latest) provides good safety guarantees. Fallback logic in
readSnapshotcorrectly handles stale 'latest' pointers.
356-383: LGTM!Latest-only update for partial execution is correctly implemented.
389-407: LGTM!Change detection using
fastDeepEqualwith timestamp-stripped comparison is appropriate.
412-419: LGTM!Strips metadata to prevent timestamp-driven false positives.
425-436: LGTM!In-place output merging is correctly implemented with debug logging.
442-449: LGTM!Output stripping via destructuring is clean and non-mutating.
455-465: LGTM!Output extraction into Map is straightforward and correct.
160-215: This recommendation is based on outdated js-yaml patterns. In v4 (which the project uses),yaml.load()is safe by default—safeLoad()was removed. The current code already follows the correct approach.Likely an incorrect or invalid review comment.
src/notebooks/deepnote/errors.ts
Outdated
| export class InvalidProjectNameError extends Error { | ||
| constructor(message: string = 'Project name cannot be empty or contain only special characters') { | ||
| super(message); | ||
| this.name = 'InvalidProjectNameError'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find other custom error classes to understand the pattern
echo "=== Custom error classes in codebase ==="
rg -n "class \w+Error extends Error" --type ts -C 2 | head -100
echo -e "\n=== Errors using l10n.t() ==="
rg -n "l10n\.t\(" --type ts -g "*error*.ts" -C 2
echo -e "\n=== Check src/platform/errors/ structure ==="
find src/platform/errors -name "*.ts" 2>/dev/null | head -20
echo -e "\n=== Check for InvalidProjectNameError references ==="
rg -n "InvalidProjectNameError" --type tsRepository: deepnote/vscode-deepnote
Length of output: 6968
Move error to src/platform/errors/ and localize the message.
This exported error is used across modules (src/notebooks/deepnote/snapshotFileService.node.ts) so it belongs in src/platform/errors/ per coding guidelines. The default message is user-facing and should be wrapped in l10n.t():
export class InvalidProjectNameError extends BaseError {
constructor(message: string = l10n.t('Project name cannot be empty or contain only special characters')) {
super({ message });
this.name = 'InvalidProjectNameError';
}
}🤖 Prompt for AI Agents
In @src/notebooks/deepnote/errors.ts around lines 6 - 11, The
InvalidProjectNameError class is defined as a plain Error with a hardcoded
user-facing message but is used across modules; move the InvalidProjectNameError
definition into the shared platform errors module where other BaseError
subclasses live, change it to extend BaseError, wrap the default message with
l10n.t('Project name cannot be empty or contain only special characters'), and
call super({ message }) in the constructor while retaining this.name =
'InvalidProjectNameError' so existing imports (e.g. usages in
snapshotFileService.node.ts) keep working.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (2)
210-259: Excludesnapshots/infindFiles(don’t filter after the fact).This avoids scanning snapshot files at all and keeps the intent centralized.
Proposed diff
for (const workspaceFolder of workspace.workspaceFolders || []) { const pattern = new RelativePattern(workspaceFolder, '**/*.deepnote'); - const files = await workspace.findFiles(pattern); - const projectFiles = files.filter((file) => !file.path.endsWith(SNAPSHOT_FILE_SUFFIX)); + const files = await workspace.findFiles(pattern, '**/snapshots/**'); + const projectFiles = files.filter((file) => !isSnapshotFile(file)); for (const file of projectFiles) { try {
329-353: Watcher no-op for snapshot files is the right behavior.Optional: if VS Code glob negation is reliable in this extension’s supported versions, consider excluding
**/snapshots/**at the watcher level too (less event churn).
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 306-352: In createSnapshot, instead of writing directly to
latestPath (which risks partial-write), write the snapshot content to a
temporary file in the same directory (e.g., latestPath + '.tmp' or include a
unique suffix using timestamp) using workspace.fs.writeFile, then atomically
rename/move that temp file to latestPath using workspace.fs.rename; on rename
failure log a warning (including error and timestampedPath) and ensure any
leftover temp file is cleaned up if possible; keep returning timestampedPath
even if latest update fails and preserve the existing timestamped write/err
handling around the timestampedPath and latestPath operations.
- Around line 15-37: slugifyProjectName currently throws InvalidProjectNameError
which can bubble up and abort snapshot/save flows; change slugifyProjectName to
NOT throw: validate input and on failure log a warning and return null (or
undefined) instead of throwing, and then update all callers that assume a string
(the other slugify usages in this file) to check the return value and skip/log
and continue the snapshot/save flow when slug is null; leave the
InvalidProjectNameError class for other contexts but stop using it here so
failures are handled locally.
- Around line 225-246: The current ensureSnapshotsDirectory uses
workspace.fs.stat(snapshotsDir) and treats any success as the directory
existing, but if stat points to a non-directory file later writes will fail;
change ensureSnapshotsDirectory to call workspace.fs.stat(snapshotsDir), inspect
the returned FileStat.type (using the vscode FileType enum) and only return true
when the type includes FileType.Directory; if the path exists but is not a
directory, log a clear error and either remove/replace it with a directory (or
return false) before attempting workspace.fs.createDirectory(snapshotsDir); keep
existing error logging paths (logger.debug/logger.error) and references to
snapshotsDir and ensureSnapshotsDirectory.
- Around line 429-440: The assignment block.outputs = outputs.get(block.id) can
break strict TypeScript because Map.get returns T | undefined even after
outputs.has(block.id) — change the code in mergeOutputsIntoBlocks to first fetch
into a local const (e.g., const outs = outputs.get(block.id)) and then assign
only after a runtime check (if (outs) block.outputs = outs), or use a non-null
assertion (outputs.get(block.id)!) if you prefer; reference the
mergeOutputsIntoBlocks function, the outputs Map, and the block.outputs property
when making the change.
- Around line 88-158: The readSnapshot method logs absolute paths (fsPath) which
can contain PII; update every logger.debug and logger.error in readSnapshot to
avoid printing uri.fsPath directly and instead log a non-sensitive identifier
such as Utils.basename(uri) or a redacted form (e.g.,
`<redacted>/${Utils.basename(uri)}`) for latestFiles[0], newestFile and any uri
used in messages, and include the error object (when present) without exposing
the full path; adjust messages around the latest snapshot detection, timestamped
snapshot selection, and parse failures (references: readSnapshot, latestFiles,
latestPattern, newestFile, allTimestampedFiles, parseSnapshotFile).
- Around line 164-219: The YAML parsing in parseSnapshotFile is using yaml.load
without a safe schema or size checks; update parseSnapshotFile to first enforce
a maximum content size (e.g., reject/return if contentString.length or the raw
byte length exceeds a reasonable limit such as 1MB) and then call yaml.load with
an explicit safe schema option (e.g., { schema: yaml.SAFE_SCHEMA }) to prevent
arbitrary code execution; apply the same changes to the hasSnapshotChanges
parsing call around line 397 and any other YAML.parse/load usages to standardize
safe parsing and consistent size limits, and log a clear error when the document
is too large or fails safe parsing.
📜 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/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/platform/errors/invalidProjectNameError.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/invalidProjectNameError.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.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/invalidProjectNameError.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/invalidProjectNameError.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.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/platform/errors/invalidProjectNameError.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.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/deepnoteTreeDataProvider.tssrc/notebooks/deepnote/snapshotFileService.node.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/snapshotFileService.node.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/snapshotFileService.node.ts (3)
src/platform/errors/invalidProjectNameError.ts (1)
InvalidProjectNameError(17-22)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)src/platform/logging/index.ts (1)
logger(35-48)
⏰ 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 (3)
src/notebooks/deepnote/deepnoteTreeDataProvider.ts (1)
21-29: Snapshot detection helper looks fine.src/notebooks/deepnote/snapshotFileService.node.ts (1)
253-293:structuredClone()runtime support needs confirming.src/platform/errors/invalidProjectNameError.ts (1)
17-21: Use a specific error category instead of'unknown', but the proposed change is incomplete.The first parameter to
BaseErroriscategory(notcode), and it must be one of theErrorCategoryunion values defined intypes.ts. The suggested value'invalidProjectName'is not in that union. To implement this fix, add'invalidProjectName'to theErrorCategorytype insrc/platform/errors/types.ts, then update the error class accordingly.
Resolve conflict in deepnoteNotebookCommandListener.ts: - Keep text block commands from main - Keep snapshot enable/disable commands from branch
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 494-504: In extractOutputsFromBlocks, don't blindly cast
block.outputs to DeepnoteOutput[]; instead validate that block.outputs is an
array (Array.isArray) and that each element has the expected shape (e.g., is an
object and contains the required output fields used elsewhere), filter/transform
only the valid entries into a DeepnoteOutput[] before calling
outputsMap.set(block.id, ...), and skip or log blocks with malformed outputs so
you never insert invalid types into the map (refer to extractOutputsFromBlocks
and block.outputs when implementing the checks).
- Around line 207-216: The loop that sets outputsMap currently checks
Array.isArray(block.outputs) then casts to DeepnoteOutput[] without validating
items; add a runtime type guard (e.g., an isDeepnoteOutput(item): boolean) and
only include items that pass the guard (or skip the whole block if the outputs
array contains invalid entries), then set outputsMap.set(block.id,
validatedOutputs) instead of blind casting; alternatively, if you intentionally
trust snapshots, add an inline comment documenting that assumption and add a
defensive log/error when malformed outputs are encountered so corruption is
detectable.
- Around line 128-142: The code currently concatenates all results from
workspace.findFiles into allTimestampedFiles and then sorts them, which can be
slow and memory-heavy for many snapshots; instead introduce a configurable
maxSnapshots (or maxSnapshotsPerFolder) and when iterating results from
workspace.findFiles(timestampedPattern) maintain a fixed-size min-heap (priority
queue) keyed by Utils.basename(filename) to keep only the top N most-recent
timestamped files while filtering out URIs with '_latest.'; after processing all
folders extract and reverse the heap to produce sortedFiles; alternatively
expose the limit via config and document snapshot retention policy if you prefer
increasing the limit rather than streaming with a heap.
- Around line 304-305: Initialize createdAt when missing instead of only when
metadata is falsy: ensure snapshotData.metadata exists (leave existing object
intact), then if snapshotData.metadata.createdAt is undefined set it to new
Date().toISOString(), and always set snapshotData.metadata.modifiedAt to new
Date().toISOString(); update the code around snapshotData.metadata
initialization in the snapshot handling logic so you don't overwrite an existing
metadata object but still populate a missing createdAt and update modifiedAt.
- Around line 134-142: The filter on allTimestampedFiles is too broad because it
checks uri.fsPath.includes('_latest.') and will exclude files when the project
slug contains "_latest"; instead inspect the filename/variant portion only: use
Utils.basename(uri) (as already used later) to extract the filename and test
whether the filename's variant segment equals or contains "_latest" (e.g., parse
the basename into name/variant/timestamp parts) and filter out only when that
variant indicates the latest snapshot; update the filter in the sortedFiles
creation to operate on Utils.basename(uri) rather than uri.fsPath so only true
"_latest" snapshot variants are excluded.
📜 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/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/snapshotFileService.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/snapshotFileService.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/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/snapshotFileService.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/snapshotFileService.node.ts
🔇 Additional comments (8)
src/notebooks/deepnote/snapshotFileService.node.ts (8)
15-45: LGTM!Utility functions are well-implemented with proper validation and error handling.
53-60: LGTM!Configuration reading is straightforward.
69-80: LGTM!Path construction follows guidelines with proper use of
Uri.joinPath.
228-259: LGTM!Directory creation with proper validation and error handling.
331-378: LGTM!Snapshot write logic is well-structured with proper error recovery. The two-phase write strategy and its limitations are clearly documented.
Also applies to: 386-416
445-452: LGTM!Proper content extraction for change detection.
481-488: LGTM!Clean immutable implementation using destructuring.
302-302: No compatibility issue withstructuredCloneat this version.The minimum VSCode version is 1.95.0, which uses Electron 30+ (Chromium 124+).
structuredCloneis fully supported in VSCode 1.67+ (Electron 18+, available since 2022). The code is safe.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:
- Around line 498-511: The disableSnapshots() method is updating the wrong
setting key ('snapshots.enabled'); change the key passed to
configurationService.updateSetting to the full registered name
'deepnote.snapshots.enabled' so the ConfigurationTarget.Workspace update affects
the actual setting defined in package.json; leave the rest of the try/catch and
messages intact and ensure the call in disableSnapshots() uses the updated key
string.
- Around line 513-526: The enableSnapshots function uses the wrong configuration
key ('snapshots.enabled') when calling configurationService.updateSetting;
change the key to 'deepnote.snapshots.enabled' to match package.json and be
consistent with disableSnapshots; update the string passed to
configurationService.updateSetting in enableSnapshots (and verify
disableSnapshots uses the same 'deepnote.snapshots.enabled' key) so the setting
is stored under the correct namespace.
📜 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)
package.jsonpackage.nls.jsonsrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/platform/common/constants.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/deepnoteNotebookCommandListener.tssrc/platform/common/constants.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/deepnoteNotebookCommandListener.tssrc/platform/common/constants.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/deepnoteNotebookCommandListener.tssrc/platform/common/constants.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/deepnoteNotebookCommandListener.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/common/constants.ts
🔇 Additional comments (7)
package.json (2)
96-105: LGTM: Command definitions follow established patterns.The new snapshot commands are properly structured and consistently placed alongside related explorer commands.
1677-1682: LGTM: Configuration is properly scoped and documented.The resource-scoped boolean with opt-in default (false) is the correct choice for a workspace-level feature toggle.
src/platform/common/constants.ts (1)
226-227: LGTM: Command constants properly defined.Values match package.json command IDs exactly. Placement is logical.
package.nls.json (1)
253-254: LGTM: Localization entries are clear and properly referenced.Titles match package.json references and follow established naming patterns.
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (3)
4-4: LGTM: Necessary imports added.ConfigurationTarget and IConfigurationService properly imported for snapshot configuration management.
Also applies to: 20-20
150-153: LGTM: Dependency injection properly implemented.IConfigurationService correctly injected and positioned before existing dependency.
218-221: LGTM: Command registration follows established patterns.Both commands properly registered with disposable tracking and bound to private handler methods.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @package.json:
- Around line 1653-1658: The configuration description for the
deepnote.snapshots.enabled property is not localized; replace the hard-coded
"description" value with a localization key following the project's pattern
(e.g., "%deepnote.configuration.snapshotsEnabled.description%") in the
package.json entry for deepnote.snapshots.enabled, and add the corresponding key
and translated string to the repository's localization file (e.g.,
package.nls.json) so it matches other properties that use
"%deepnote.configuration...%" keys.
In @src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:
- Around line 549-577: The two methods disableSnapshots and enableSnapshots are
nearly identical; extract a shared helper (e.g., setSnapshotsEnabled(enabled:
boolean)) that takes a boolean, calls
this.configurationService.updateSetting('snapshots.enabled', enabled, undefined,
ConfigurationTarget.Workspace), shows the corresponding info message (use
conditional text based on enabled) and logs on error with the same error
handling; then replace disableSnapshots and enableSnapshots to call
setSnapshotsEnabled(false) and setSnapshotsEnabled(true) respectively to remove
duplication.
📜 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)
package.jsonpackage.nls.jsonsrc/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/platform/common/constants.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/common/constants.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.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/common/constants.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/common/constants.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.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/common/constants.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.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/deepnoteNotebookCommandListener.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
error(30-37)src/platform/logging/index.ts (1)
logger(35-48)
⏰ 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/platform/common/constants.ts (1)
226-227: LGTM!Constants align with package.json commands and follow existing conventions.
package.nls.json (1)
253-254: LGTM!Localization keys follow established patterns.
package.json (1)
96-105: LGTM!Commands properly structured with localized titles.
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)
153-156: LGTM!DI wiring follows Inversify patterns correctly.
224-227: LGTM!Command registration consistent with existing patterns.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteSerializer.ts:
- Around line 227-235: The snapshot branch uses
snapshotFileService.stripOutputsFromBlocks which only removes outputs but does
not break circular references, causing yaml.dump to fail; update the logic so
that when snapshotFileService.isSnapshotsEnabled() is true you first strip
outputs (via stripOutputsFromBlocks) and then protect against cycles by passing
the result through cloneWithoutCircularRefs (or modify stripOutputsFromBlocks to
perform an equivalent deep clone-without-cycles), ensuring notebook.blocks is
assigned a cycle-safe structure before serialization (references:
stripOutputsFromBlocks, cloneWithoutCircularRefs,
snapshotFileService.isSnapshotsEnabled, notebook.blocks).
In @src/notebooks/deepnote/snapshotFileService.node.ts:
- Around line 43-45: The generateTimestamp function currently uses
toISOString().replace(/:/g, '-').slice(0, 19) which intentionally truncates
milliseconds for a filename-safe timestamp; add a concise comment above the
generateTimestamp function stating that milliseconds are intentionally removed
for filename safety/consistency and that the slice(0, 19) is deliberate, or if
milliseconds are required change the slice to include the fractional seconds and
update the replacement logic accordingly—refer to generateTimestamp to locate
where to add the comment or adjust the slice.
📜 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/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.node.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileServiceTypes.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/snapshotFileServiceTypes.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.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/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.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/snapshotFileServiceTypes.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.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/snapshotFileService.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/snapshotFileService.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/notebooks/deepnote/snapshotFileService.node.ts
🧠 Learnings (1)
📚 Learning: 2026-01-12T09:12:12.830Z
Learnt from: Artmann
Repo: deepnote/vscode-deepnote PR: 256
File: src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:549-562
Timestamp: 2026-01-12T09:12:12.830Z
Learning: IConfigurationService.updateSetting() automatically prefixes the key with the 'deepnote' namespace. When calling updateSetting, always pass the unprefixed key (e.g., use 'snapshots.enabled' instead of 'deepnote.snapshots.enabled'). This guideline applies to all TypeScript source files under src where updateSetting is used.
Applied to files:
src/notebooks/deepnote/snapshotFileServiceTypes.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotFileService.unit.test.tssrc/notebooks/deepnote/snapshotFileService.node.ts
🧬 Code graph analysis (3)
src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
src/notebooks/deepnote/snapshotFileService.node.ts (1)
ISnapshotFileService(13-13)src/notebooks/deepnote/deepnoteSerializer.ts (3)
DeepnoteOutput(15-15)DeepnoteFile(14-14)DeepnoteBlock(14-14)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)
src/notebooks/deepnote/snapshotFileService.node.ts (3)
src/platform/errors/invalidProjectNameError.ts (1)
InvalidProjectNameError(17-22)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (16)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
58-62: Constructor injection looks correct.Optional DI for
ISnapshotFileServiceproperly wired with@optional()decorator.
119-144: Snapshot merge flow is sound.Graceful fallback to baseline cells on error. The
cellsreassignment pattern is clear.One minor note: consider extracting the merge block into a private method for readability if this grows.
src/notebooks/deepnote/snapshotFileServiceTypes.ts (1)
1-57: Interface well-defined.Clean separation of types. Symbol token pattern correct for inversify DI.
src/notebooks/deepnote/snapshotFileService.unit.test.ts (4)
11-20: Test setup is clean.Using
serviceAnyfor private method access is pragmatic. Consider adding a comment explaining this pattern for future maintainers.
361-391: Config mock pattern could cause hanging tests.Per coding guidelines: "When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests."The
WorkspaceConfigurationmock is returned synchronously fromgetConfiguration, so this specific case is fine. But worth verifying if any mocks are returned from promises elsewhere.
571-597: Helper function reduces duplication.
createProjectData()is reused well across test cases.
685-709: Good resilience test.Verifying timestamped path returned even when latest write fails confirms the recovery behavior documented in the implementation.
src/notebooks/deepnote/snapshotFileService.node.ts (9)
20-37: Slug function is robust.Handles edge cases: empty strings, whitespace-only, special-chars-only. Throws typed error for invalid names.
56-60: Config key is unprefixed.Correctly using
'snapshots.enabled'without the'deepnote.'prefix sincegetConfiguration('deepnote')already scopes it. Based on learnings.
88-162: readSnapshot searches all workspace folders.Logic is correct: tries latest first, falls back to most recent timestamped. The
limit: 100on timestamped search is reasonable.One edge case: if two workspace folders contain snapshots for the same projectId, the first folder's latest wins. This matches expected behavior but could be documented.
168-223: parseSnapshotFile handles malformed YAML gracefully.Returns empty Map on parse errors. Good defensive coding.
303-310: Metadata mutation on clone.
structuredClonecreates a deep copy, so mutatingsnapshotData.metadatais safe. The pattern is correct.
336-383: Two-step write strategy is well-documented.Comment explains the tradeoff: not atomic, but provides recovery if latest write fails. Good.
427-445: Change detection excludes metadata.Uses
getComparableProjectContentto ignore timestamps.fastDeepEqualis efficient for deep comparison.Note: If read fails (catch block), defaults to
true(has changes). This is correct behavior.
491-498: stripOutputsFromBlocks uses object rest spread.The eslint-disable for unused
outputsis fine—destructuring to exclude is idiomatic.
504-514: extractOutputsFromBlocks is straightforward.Returns Map keyed by block ID. Skips blocks without ID or outputs.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteSerializer.ts:
- Around line 58-62: The constructor parameter order in DeepnoteSerializer is
non-alphabetical and could break manual instantiation: swap the positions of
snapshotFileService and snapshotService so that @inject(ISnapshotFileService)
@optional() private readonly snapshotFileService?: ISnapshotFileService appears
before @inject(ISnapshotMetadataService) @optional() private readonly
snapshotService?: ISnapshotMetadataServiceFull in the constructor signature;
preserve the existing decorators, types, access modifiers, and
IDeepnoteNotebookManager parameter, and update any manual instantiation sites or
tests that construct this class to use the new parameter order.
- Around line 119-144: Currently convertBlocksToCells is called immediately into
cells and then called again after merging snapshot outputs, causing double work;
modify DeepnoteSerializer logic to determine the source blocks first (use a
blocksToConvert variable: if snapshotFileService?.isSnapshotsEnabled() then try
to read snapshotOutputs and, if present, call
mergeOutputsIntoBlocks(selectedNotebook.blocks ?? [], snapshotOutputs) else fall
back to selectedNotebook.blocks ?? []), and only after resolving blocksToConvert
call this.converter.convertBlocksToCells(blocksToConvert) once; ensure existing
try/catch around readSnapshot is preserved and that logger.debug messages are
updated to reflect single conversion.
📜 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/notebooks/deepnote/deepnoteSerializer.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/deepnoteSerializer.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/deepnoteSerializer.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/deepnoteSerializer.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/deepnoteSerializer.ts
🧠 Learnings (1)
📚 Learning: 2026-01-12T09:12:12.830Z
Learnt from: Artmann
Repo: deepnote/vscode-deepnote PR: 256
File: src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:549-562
Timestamp: 2026-01-12T09:12:12.830Z
Learning: IConfigurationService.updateSetting() automatically prefixes the key with the 'deepnote' namespace. When calling updateSetting, always pass the unprefixed key (e.g., use 'snapshots.enabled' instead of 'deepnote.snapshots.enabled'). This guideline applies to all TypeScript source files under src where updateSetting is used.
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
src/notebooks/deepnote/snapshotMetadataService.ts (2)
ISnapshotMetadataService(81-81)ISnapshotMetadataServiceFull(83-115)src/notebooks/deepnote/snapshotFileServiceTypes.ts (2)
ISnapshotFileService(6-6)ISnapshotFileService(12-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (1)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
227-237: LGTM.Clean branching. Outputs stripped before clone; fallback retains default behavior.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @src/notebooks/deepnote/deepnoteSerializer.ts:
- Around line 121-142: DeepnoteSerializer is converting blocks to cells twice
when snapshots exist; defer conversion so convertBlocksToCells is only called
once after any snapshot-merge. Change the flow in the block that checks
snapshotService.isSnapshotsEnabled(): read snapshotOutputs via readSnapshot and,
if present, call mergeOutputsIntoBlocks on selectedNotebook.blocks to produce
blocksWithOutputs and then call
converter.convertBlocksToCells(blocksWithOutputs) to set cells; if no
snapshotOutputs, convert the original selectedNotebook.blocks once. Ensure any
early call to converter.convertBlocksToCells (the initial conversion) is removed
or moved so conversion happens only after the snapshot merge logic.
In @src/notebooks/deepnote/snapshotService.ts:
- Around line 517-565: The captureEnvironmentForNotebook method is using
logger.info for routine tracing; switch non-significant, verbose messages to
logger.debug (e.g., the initial call log, "Skipping capture: no state found",
"Skipping capture: already captured", "Could not find notebook document for
...", the list of available notebooks, "Found notebook, getting interpreter...",
and "environmentCapture returned undefined") while keeping important
success/failure logs (e.g., captured successfully and the error log in the
catch) as logger.info/logger.error; update references in this function
(captureEnvironmentForNotebook, environmentCaptured, environment, and the call
to environmentCapture.captureEnvironment) accordingly to reduce noise and match
getEnvironmentMetadata's logging level usage.
- Around line 150-160: SnapshotService directly instantiates
DeepnoteDataConverter via the private readonly converter = new
DeepnoteDataConverter(), which makes onExecutionComplete hard to unit-test;
change SnapshotService to accept a DeepnoteDataConverter (or an
interface/IDeepnoteDataConverter) via constructor injection (add a constructor
parameter with @inject for DeepnoteDataConverter), assign it to the existing
converter property instead of new-ing it, and update DI bindings to provide the
converter so tests can supply a mock/fake for onExecutionComplete.
- Around line 286-319: The getEnvironmentMetadata method is using logger.info
for routine/internal state checks; change those to a lower verbosity level
(e.g., logger.debug or logger.trace) to follow logging guidelines: replace the
logger.info calls that log existence of state, waiting for capture, timeout
start/finish, and the final state.environment/state.environmentCaptured checks
in getEnvironmentMetadata (references: method getEnvironmentMetadata, properties
state.capturePromise and state.environmentCaptured) with logger.debug (or
logger.trace if you prefer more verbosity) so only meaningful user-facing events
remain at info level.
- Around line 196-211: The method captureEnvironmentBeforeExecution sets
state.capturePromise = this.captureEnvironmentForNotebook(notebookUri) but
doesn't await it; modify captureEnvironmentBeforeExecution to await
state.capturePromise so callers block until the capture completes (i.e., replace
the assignment-only path with: assign the promise, await it), and after awaiting
clear state.capturePromise (set to undefined/null) to avoid leaving a stale
promise; reference captureEnvironmentBeforeExecution, state.capturePromise,
captureEnvironmentForNotebook and getOrCreateExecutionState when making the
change.
In @src/notebooks/deepnote/snapshotService.unit.test.ts:
- Around line 1066-1079: Tests only assert no-throw/undefined for environment
capture; add coverage that mocks IEnvironmentCapture.captureEnvironment to
assert the service state is updated and returned by getEnvironmentMetadata.
Update the unit tests for captureEnvironmentBeforeExecution to stub/mocks the
IEnvironmentCapture.captureEnvironment (or the concrete capture provider used by
service) to return a sample EnvironmentMetadata, call
service.captureEnvironmentBeforeExecution(notebookUri), then call
service.getEnvironmentMetadata(notebookUri) and assert the returned metadata
equals the mocked EnvironmentMetadata and any internal flags/state changed.
Ensure you reference the service instance used in the suite and the
IEnvironmentCapture.captureEnvironment method in your test doubles so the
behavior is validated end-to-end.
📜 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 (10)
src/kernels/execution/cellExecutionQueue.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshotMetadataService.tssrc/notebooks/deepnote/snapshotMetadataService.unit.test.tssrc/notebooks/deepnote/snapshotService.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/serviceRegistry.node.tssrc/platform/notebooks/cellExecutionStateService.tssrc/platform/notebooks/deepnote/types.ts
💤 Files with no reviewable changes (2)
- src/notebooks/deepnote/snapshotMetadataService.ts
- src/notebooks/deepnote/snapshotMetadataService.unit.test.ts
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{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/execution/cellExecutionQueue.tssrc/platform/notebooks/deepnote/types.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/deepnote/snapshotService.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.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/execution/cellExecutionQueue.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.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
**/*.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/execution/cellExecutionQueue.tssrc/platform/notebooks/deepnote/types.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/deepnote/snapshotService.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/execution/cellExecutionQueue.tssrc/platform/notebooks/deepnote/types.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/deepnote/snapshotService.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/notebooks/cellExecutionStateService.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/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/deepnote/snapshotService.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/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/snapshotService.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/snapshotService.unit.test.ts
🧠 Learnings (1)
📚 Learning: 2026-01-12T09:12:12.830Z
Learnt from: Artmann
Repo: deepnote/vscode-deepnote PR: 256
File: src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:549-562
Timestamp: 2026-01-12T09:12:12.830Z
Learning: IConfigurationService.updateSetting() automatically prefixes the key with the 'deepnote' namespace. When calling updateSetting, always pass the unprefixed key (e.g., use 'snapshots.enabled' instead of 'deepnote.snapshots.enabled'). This guideline applies to all TypeScript source files under src where updateSetting is used.
Applied to files:
src/kernels/execution/cellExecutionQueue.tssrc/platform/notebooks/deepnote/types.tssrc/platform/notebooks/cellExecutionStateService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshotService.unit.test.tssrc/notebooks/deepnote/snapshotService.ts
🧬 Code graph analysis (5)
src/platform/notebooks/cellExecutionStateService.ts (3)
src/platform/common/utils/lifecycle.ts (1)
trackDisposable(26-29)src/test/mocks/vsc/index.ts (1)
EventEmitter(65-89)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/notebooks/deepnote/snapshotService.ts (2)
ISnapshotService(113-113)ISnapshotService(119-143)
src/notebooks/serviceRegistry.node.ts (3)
src/notebooks/deepnote/environmentCapture.node.ts (2)
IEnvironmentCapture(23-23)IEnvironmentCapture(25-30)src/notebooks/deepnote/snapshotService.ts (3)
ISnapshotService(113-113)ISnapshotService(119-143)ISnapshotMetadataService(20-20)src/platform/notebooks/deepnote/types.ts (2)
ISnapshotMetadataService(125-125)ISnapshotMetadataService(126-149)
src/notebooks/deepnote/snapshotService.unit.test.ts (2)
src/test/vscode-mock.ts (2)
resetVSCodeMocks(57-305)mockedVSCodeNamespaces(15-15)src/notebooks/deepnote/deepnoteSerializer.ts (3)
DeepnoteFile(13-13)DeepnoteBlock(13-13)DeepnoteOutput(14-14)
src/notebooks/deepnote/snapshotService.ts (3)
src/platform/errors/invalidProjectNameError.ts (1)
InvalidProjectNameError(17-22)src/notebooks/deepnote/deepnoteDataConverter.ts (1)
DeepnoteDataConverter(37-560)src/platform/logging/index.ts (1)
logger(35-48)
⏰ 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 (31)
src/platform/notebooks/cellExecutionStateService.ts (3)
41-49: LGTM!Interface is clean and follows existing patterns.
59-72: LGTM!Emitter setup mirrors existing pattern. Tracked for disposal.
74-81: LGTM!Simple notify function. Logging is appropriate.
src/kernels/execution/cellExecutionQueue.ts (1)
327-331: LGTM!Notification fires after queue drains, regardless of success/failure. Ensures listeners can react to completion.
src/notebooks/deepnote/deepnoteActivationService.ts (2)
12-12: LGTM!Import updated to new consolidated service.
33-33: LGTM!Optional injection appropriate for opt-in feature.
src/notebooks/deepnote/deepnoteSerializer.ts (3)
10-10: LGTM!Import updated to consolidated service.
225-235: LGTM!Output stripping conditional on snapshot mode. Circular ref handling applied consistently.
274-276: Index alignment is safe.
convertCellsToBlocksuses.map()on the cells array (deepnoteDataConverter.ts:167-169), guaranteeing a 1:1 correspondence. Since blocks are created directly fromdata.cellsimmediately before this loop, and the loop bounds onblocks.length, there is no risk of misalignment. The code also includes a null-check guard on line 288.src/platform/notebooks/deepnote/types.ts (1)
138-148: No action needed.
clearExecutionStatealready resets Run All mode viathis.runAllModeNotebooks.delete(notebookUri). The interface and implementation are correct as-is.src/notebooks/serviceRegistry.node.ts (2)
91-92: LGTM!Import structure cleanly separates the new service interface/implementation from the platform type re-export.
246-250: LGTM!Service registration follows established patterns. Binding to both
IExtensionSyncActivationService(for lifecycle) andISnapshotMetadataService(for backward compatibility) is correct.src/notebooks/deepnote/snapshotService.unit.test.ts (5)
1-26: LGTM!Test setup is clean. The
serviceAnypattern for testing private methods is reasonable here.resetVSCodeMocks()ensures isolation.
56-151: LGTM!Good coverage of path building: variants, slugification, and error cases.
153-393: LGTM!Immutability tests for all transform methods. Edge cases well covered.
417-588: LGTM!Fallback logic from latest to timestamped snapshots is well tested. Error handling behavior verified.
590-799: LGTM!Covers success, failure, and partial success. The test at lines 681-705 correctly verifies graceful degradation when latest write fails.
src/notebooks/deepnote/snapshotService.ts (14)
1-17: LGTM!Imports organized with third-party first, then local. Types correctly imported with
import typewhere applicable.
22-72: LGTM!Interfaces well-documented. NotebookExecutionState tracks all necessary state for snapshot metadata.
74-79: LGTM!Simple timeout error for environment capture. Correctly sets
name.
86-103: LGTM!Slugification handles edge cases and throws appropriate error for invalid names.
109-111: LGTM!Filename-safe timestamp format.
113-143: LGTM!Interface extends platform type and adds file I/O methods. Clean separation of concerns.
166-194: LGTM!Event subscriptions correctly registered with disposables for cleanup.
220-262: LGTM!Write order prioritizes timestamped (immutable) before latest. Graceful degradation on partial failure.
386-455: LGTM!Fallback from latest to timestamped snapshots is correct. Filename-based sorting works due to ISO timestamp format.
463-470: LGTM!Immutable stripping of outputs.
632-652: LGTM!State change handling is correct. Treating undefined success as true is reasonable fallback.
671-782: LGTM!Comprehensive validation with proper cleanup on all exit paths.
structuredCloneensures safe mutation.
839-889: LGTM!Proper error handling, deep cloning, and timestamp management.
934-946: No issue — contentHash is populated by design.
contentHashis intentionally initialized as an empty string during execution tracking. It's computed later indeepnoteSerializer.tsduring the serialization phase when callingaddSnapshotMetadataToBlocks(), which computes the SHA-256 hash (line 282). This two-phase approach separates execution timing concerns from content hashing.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @package.json:
- Around line 96-105: The package registers commands deepnote.enableSnapshots
and deepnote.disableSnapshots but doesn't expose them in the commandPalette;
update the commandPalette array to include entries for
"deepnote.enableSnapshots" and "deepnote.disableSnapshots" with an appropriate
when clause (e.g., "notebookType == 'deepnote'") so they appear in the Command
Palette.
- Around line 1653-1658: Add a localization entry for the configuration
description: create a key "deepnote.snapshots.enabled.description" in
package.nls.json with the value "When enabled, outputs are saved to separate
snapshot files in a 'snapshots' folder instead of the main .deepnote file." and
replace the hard-coded description in the package.json
"deepnote.snapshots.enabled" block to reference this localized key so the config
uses localized text.
📜 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)
package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint & Format
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/notebookCommandListener.ts (1)
37-60: MakeISnapshotMetadataServiceinjection optional (likely required for non-Deepnote contexts).
A global command listener shouldn’t hard-require a Deepnote-only service unless every extension variant guarantees the binding. Suggest@optional()+ guard.Proposed fix
-import { ISnapshotMetadataService } from './deepnote/snapshots/snapshotService'; +import { ISnapshotMetadataService } from './deepnote/snapshots/snapshotService';- @inject(IKernelStatusProvider) private kernelStatusProvider: IKernelStatusProvider, - @inject(ISnapshotMetadataService) private snapshotMetadataService: ISnapshotMetadataService + @inject(IKernelStatusProvider) private kernelStatusProvider: IKernelStatusProvider, + @inject(ISnapshotMetadataService) @optional() + private snapshotMetadataService?: ISnapshotMetadataService ) {}
🤖 Fix all issues with AI agents
In @src/kernels/execution/cellExecutionQueue.ts:
- Around line 17-18: Move the ISnapshotMetadataService interface out of
notebooks/deepnote/snapshots/snapshotService and into the shared platform types
module (e.g., src/platform/common/types), then update all kernel imports that
currently bypass the rule—replace the import in cellExecutionQueue.ts,
kernelProvider.node.ts, and kernelExecution.ts to import
ISnapshotMetadataService from the new shared types module and remove the
eslint-disable-next-line import/no-restricted-paths comments; ensure the
exported name and shape remain unchanged so callers compile without other
changes.
In @src/notebooks/deepnote/snapshots/environmentCapture.node.ts:
- Around line 8-16: determinePythonVersionFromRunningTheInterpreter currently
only reads stdout and can miss versions printed to stderr; update it to inspect
stderr when stdout is empty (or combine stdout+stderr), trim and run the same
version regex against the combined output to reliably extract the version. Also
replace uses of path.resolve(...) when constructing the pip binary path with
Uri.joinPath(deepnoteEnvironment.venvPath, pipDir, pipBinary).fsPath to follow
the Uri-based path conventions (look for usages around DeepnoteEnvironment /
pipBinary construction).
In @src/notebooks/deepnote/snapshots/snapshotService.ts:
- Around line 189-205: captureEnvironmentBeforeExecution currently sets
state.capturePromise and returns immediately, so callers awaiting it don't
actually wait for capture to finish; fix by awaiting state.capturePromise (the
promise returned from captureEnvironmentForNotebook) before returning, and
ensure you clear state.capturePromise when the capture completes or fails.
Specifically, in captureEnvironmentBeforeExecution (which uses
getOrCreateExecutionState and state.capturePromise and calls
captureEnvironmentForNotebook), assign state.capturePromise =
this.captureEnvironmentForNotebook(notebookUri), then await it inside a
try/finally and in finally set state.capturePromise = undefined (and rethrow
errors if you want propagation) so callers properly block until capture
completes.
- Around line 543-568: The directory check in ensureSnapshotsDirectory
incorrectly uses strict equality against FileType.Directory which can miss
directory symlinks because FileType is a bitmask; replace the equality check
with a bitmask test (e.g., check that stat.type has the FileType.Directory bit
set) when validating the result of workspace.fs.stat in
ensureSnapshotsDirectory, so the branch that logs "[Snapshot] Snapshots path
exists but is not a directory: ${Utils.basename(snapshotsDir)}" only executes
when the Directory bit is absent; leave the surrounding logging and error
handling intact.
- Around line 290-323: The method getEnvironmentMetadata creates a timeout via
setTimeout that is never cleared and emits many info-level logs; update
getEnvironmentMetadata to store the timer id and call clearTimeout(timer) when
the capturePromise resolves or the race finishes to avoid leaking timers, reduce
noisy logger.info calls to logger.debug or logger.trace (e.g., the lines logging
state existence, waiting for capture, environment presence, and
environmentCaptured), and keep the TimeoutError logic unchanged (use the
existing TimeoutError class and keep the same warn on timeout); reference the
symbols getEnvironmentMetadata, state.capturePromise, timeoutPromise,
TimeoutError, and logger when making the changes.
In @src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts:
- Around line 1-988: Replace brittle string-equality assertions and non-strict
deep equality checks: in tests that expect project name validation failures
(tests invoking serviceAny.buildSnapshotPath and service.createSnapshot for
invalid names), assert the thrown error is an instance of
InvalidProjectNameError instead of matching the exact message; and replace all
assert.deepEqual usages (e.g., in mergeOutputsIntoBlocks,
extractOutputsFromBlocks, readSnapshot tests) with assert.deepStrictEqual to
follow strict object comparison guidelines and ensure exact structural equality.
In @src/notebooks/notebookCommandListener.ts:
- Around line 117-123: The runAllCells method calls
this.snapshotMetadataService.setRunAllMode(...) without checking for a possibly
missing service; update runAllCells to guard that call by verifying
this.snapshotMetadataService is non-null/undefined before invoking setRunAllMode
(e.g., if (this.snapshotMetadataService)
this.snapshotMetadataService.setRunAllMode(notebookUri)); ensure the rest of
runAllCells still executes commands.executeCommand('notebook.execute') even if
the service is absent.
In @src/notebooks/serviceRegistry.node.ts:
- Around line 91-93: The web service registry is missing the
ISnapshotMetadataService binding required by NotebookCommandListener; update the
web registry to import SnapshotService and EnvironmentCapture and register them:
bind SnapshotService to the ISnapshotMetadataService interface (same way the
node registry does) and register EnvironmentCapture for IEnvironmentCapture so
the DI container can resolve those dependencies when instantiating
NotebookCommandListener. Ensure you use the same class names SnapshotService and
EnvironmentCapture and the interface token ISnapshotMetadataService (and
IEnvironmentCapture) as in the node registry.
📜 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 (14)
src/kernels/execution/cellExecutionQueue.tssrc/kernels/kernelExecution.tssrc/kernels/kernelProvider.node.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/environmentCapture.node.tssrc/notebooks/deepnote/snapshots/environmentCapture.unit.test.tssrc/notebooks/deepnote/snapshots/pipFileParser.tssrc/notebooks/deepnote/snapshots/pipFileParser.unit.test.tssrc/notebooks/deepnote/snapshots/snapshotService.tssrc/notebooks/deepnote/snapshots/snapshotService.unit.test.tssrc/notebooks/notebookCommandListener.tssrc/notebooks/serviceRegistry.node.tssrc/platform/notebooks/deepnote/types.ts
💤 Files with no reviewable changes (1)
- src/platform/notebooks/deepnote/types.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Order method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/kernels/execution/cellExecutionQueue.tssrc/notebooks/notebookCommandListener.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/environmentCapture.node.tssrc/notebooks/deepnote/snapshots/snapshotService.unit.test.tssrc/kernels/kernelProvider.node.tssrc/kernels/kernelExecution.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshots/snapshotService.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/execution/cellExecutionQueue.tssrc/notebooks/notebookCommandListener.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/environmentCapture.node.tssrc/notebooks/deepnote/snapshots/snapshotService.unit.test.tssrc/kernels/kernelProvider.node.tssrc/kernels/kernelExecution.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshots/snapshotService.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/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/environmentCapture.node.tssrc/notebooks/deepnote/snapshots/snapshotService.unit.test.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/snapshots/snapshotService.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.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/snapshots/snapshotService.unit.test.ts
🧠 Learnings (1)
📚 Learning: 2026-01-12T09:12:12.830Z
Learnt from: Artmann
Repo: deepnote/vscode-deepnote PR: 256
File: src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:549-562
Timestamp: 2026-01-12T09:12:12.830Z
Learning: IConfigurationService.updateSetting() automatically prefixes the key with the 'deepnote' namespace. When calling updateSetting, always pass the unprefixed key (e.g., use 'snapshots.enabled' instead of 'deepnote.snapshots.enabled'). This guideline applies to all TypeScript source files under src where updateSetting is used.
Applied to files:
src/kernels/execution/cellExecutionQueue.tssrc/notebooks/notebookCommandListener.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/environmentCapture.node.tssrc/notebooks/deepnote/snapshots/snapshotService.unit.test.tssrc/kernels/kernelProvider.node.tssrc/kernels/kernelExecution.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/snapshots/snapshotService.ts
🧬 Code graph analysis (4)
src/notebooks/notebookCommandListener.ts (2)
src/kernels/kernelStatusProvider.ts (2)
IKernelStatusProvider(14-14)IKernelStatusProvider(15-17)src/notebooks/deepnote/snapshots/snapshotService.ts (2)
ISnapshotMetadataService(25-25)ISnapshotMetadataService(26-49)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts (3)
src/notebooks/deepnote/snapshots/environmentCapture.node.ts (2)
IEnvironmentCapture(23-23)IEnvironmentCapture(25-30)src/test/vscode-mock.ts (2)
resetVSCodeMocks(57-305)mockedVSCodeNamespaces(15-15)src/notebooks/deepnote/deepnoteSerializer.ts (3)
DeepnoteFile(13-13)DeepnoteBlock(13-13)DeepnoteOutput(14-14)
src/notebooks/serviceRegistry.node.ts (2)
src/notebooks/deepnote/snapshots/environmentCapture.node.ts (2)
IEnvironmentCapture(23-23)IEnvironmentCapture(25-30)src/notebooks/deepnote/snapshots/snapshotService.ts (2)
ISnapshotMetadataService(25-25)ISnapshotMetadataService(26-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (9)
src/kernels/execution/cellExecutionQueue.ts (2)
198-208: Pre-execution env capture is good, but currently doesn’t guarantee capture completes.
This block is correct assumingcaptureEnvironmentBeforeExecution()truly blocks; with the current SnapshotService implementation it won’t. Once SnapshotService is fixed to await capture, this looks good.
328-332: Queue-complete notification: verify desired behavior on cancel/failure.
notifyQueueComplete()fires whenever the loop ends, including cancellations and failures. Snapshotting “latest” after an interrupted run might be unwanted (or might be exactly what you want). Please confirm product intent and gate accordingly if needed (e.g., only if at least one cell executed and queue wasn’t cancelled).src/kernels/kernelProvider.node.ts (1)
35-37: Optional injection adequately addresses the layering concern.The implementation is sound. Optional injection (via
@optional()decorator) ensures the kernel layer can function independently of the snapshot service, mitigating the cross-layer import. No circular dependency exists between kernels and notebooks.src/notebooks/deepnote/snapshots/snapshotService.ts (1)
480-491: No changes needed;Uri.joinPath(projectUri, '..')is the idiomatic VS Code API pattern.VS Code's
Uri.joinPathcorrectly normalizes ".." segments to return the parent directory for file URIs. There is noUtils.dirnamemethod in the VS Code API, and the current implementation follows the established pattern in the codebase (e.g.,src/test/web/customReporter.tsuses the same approach with multiple ".." segments).Likely an incorrect or invalid review comment.
src/kernels/kernelExecution.ts (1)
48-48: LGTM. Import path updated to match newSnapshotServicelocation.src/notebooks/deepnote/deepnoteSerializer.ts (3)
10-10: LGTM. Import and DI updated toSnapshotService.Also applies to: 59-59
117-142: LGTM. Graceful fallback on snapshot read failure.
225-235: LGTM. Outputs stripped only when snapshots enabled; both paths apply circular-ref cleanup.src/notebooks/deepnote/deepnoteActivationService.ts (1)
12-12: Verified:SnapshotServicebound in IoC container atserviceRegistry.node.ts:248-250.
| // eslint-disable-next-line import/no-restricted-paths | ||
| import { ISnapshotMetadataService } from '../../notebooks/deepnote/snapshots/snapshotService'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check eslint configuration for restricted-paths rule
fd -t f "eslintrc\|.eslintrc\|eslint\.config" . | head -5Repository: deepnote/vscode-deepnote
Length of output: 50
🏁 Script executed:
# Search for the restricted-paths configuration and understand what paths are restricted
rg -l "import/no-restricted-paths" . --type js --type json 2>/dev/null | head -5Repository: deepnote/vscode-deepnote
Length of output: 50
🏁 Script executed:
# Look at the actual file and surrounding imports to understand context
head -40 src/kernels/execution/cellExecutionQueue.tsRepository: deepnote/vscode-deepnote
Length of output: 2136
🏁 Script executed:
# Find all imports of ISnapshotMetadataService in the codebase
rg "ISnapshotMetadataService" --type ts --type tsxRepository: deepnote/vscode-deepnote
Length of output: 95
🏁 Script executed:
# Check where ISnapshotMetadataService is exported from
rg "export.*ISnapshotMetadataService|export.*interface ISnapshotMetadataService" --type ts --type tsxRepository: deepnote/vscode-deepnote
Length of output: 95
🏁 Script executed:
# Search for ISnapshotMetadataService usage without type specification
rg "ISnapshotMetadataService" --type tsRepository: deepnote/vscode-deepnote
Length of output: 1703
🏁 Script executed:
# Check eslint config more broadly
find . -name "*eslint*" -type f 2>/dev/null | head -10Repository: deepnote/vscode-deepnote
Length of output: 99
🏁 Script executed:
# Look for restricted-paths configuration
rg "restricted.*path|snapshotService" --type json | head -20Repository: deepnote/vscode-deepnote
Length of output: 50
🏁 Script executed:
# Check if there's a common/shared types directory where interfaces are typically placed
find src -type d -name "common" -o -name "types" -o -name "platform" | head -10Repository: deepnote/vscode-deepnote
Length of output: 294
🏁 Script executed:
# Examine the snapshotService file to understand the interface
cat src/notebooks/deepnote/snapshots/snapshotService.ts 2>/dev/null || echo "File not found, checking alternatives..."Repository: deepnote/vscode-deepnote
Length of output: 32299
Multiple kernel modules import from notebooks—move ISnapshotMetadataService to a shared platform types module.
Three kernel files (kernelProvider.node.ts, kernelExecution.ts, cellExecutionQueue.ts) bypass the import restriction to access ISnapshotMetadataService from notebooks/deepnote/snapshots/snapshotService.ts. While the interface is documented as "platform-layer" for kernel use, its location in notebooks creates an unwanted dependency direction. Relocate the interface to src/platform/common/types (or similar shared location) and update all three imports to eliminate the eslint overrides.
🤖 Prompt for AI Agents
In @src/kernels/execution/cellExecutionQueue.ts around lines 17 - 18, Move the
ISnapshotMetadataService interface out of
notebooks/deepnote/snapshots/snapshotService and into the shared platform types
module (e.g., src/platform/common/types), then update all kernel imports that
currently bypass the rule—replace the import in cellExecutionQueue.ts,
kernelProvider.node.ts, and kernelExecution.ts to import
ISnapshotMetadataService from the new shared types module and remove the
eslint-disable-next-line import/no-restricted-paths comments; ensure the
exported name and shape remain unchanged so callers compile without other
changes.
| import { PythonEnvironment } from '../../../platform/pythonEnvironments/info'; | ||
| import { computeHash } from '../../../platform/common/crypto'; | ||
| import { raceTimeout } from '../../../platform/common/utils/async'; | ||
| import { logger } from '../../../platform/logging'; | ||
| import { parsePipFreezeFile } from './pipFileParser'; | ||
| import { IDeepnoteEnvironmentManager, IDeepnoteNotebookEnvironmentMapper } from '../../kernels/deepnote/types'; | ||
| import { IDeepnoteEnvironmentManager, IDeepnoteNotebookEnvironmentMapper } from '../../../kernels/deepnote/types'; | ||
| import { Uri } from 'vscode'; | ||
| import { DeepnoteEnvironment } from '../../kernels/deepnote/environments/deepnoteEnvironment'; | ||
| import * as path from '../../platform/vscode-path/path'; | ||
| import { DeepnoteEnvironment } from '../../../kernels/deepnote/environments/deepnoteEnvironment'; | ||
| import * as path from '../../../platform/vscode-path/path'; |
There was a problem hiding this comment.
Imports refactor OK; watch python --version stderr + prefer Uri.joinPath where possible.
Even though this hunk is just import rewiring, determinePythonVersionFromRunningTheInterpreter() currently parses only stdout—some Python builds print the version to stderr, which can make env capture flaky. Also consider Uri.joinPath(deepnoteEnvironment.venvPath, pipDir, pipBinary).fsPath instead of path.resolve(...) to stay consistent with path guidelines.
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshots/environmentCapture.node.ts around lines 8 -
16, determinePythonVersionFromRunningTheInterpreter currently only reads stdout
and can miss versions printed to stderr; update it to inspect stderr when stdout
is empty (or combine stdout+stderr), trim and run the same version regex against
the combined output to reliably extract the version. Also replace uses of
path.resolve(...) when constructing the pip binary path with
Uri.joinPath(deepnoteEnvironment.venvPath, pipDir, pipBinary).fsPath to follow
the Uri-based path conventions (look for usages around DeepnoteEnvironment /
pipBinary construction).
| async captureEnvironmentBeforeExecution(notebookUri: string): Promise<void> { | ||
| logger.info(`[Snapshot] captureEnvironmentBeforeExecution called for ${notebookUri}`); | ||
|
|
||
| const state = this.getOrCreateExecutionState(notebookUri, Date.now()); | ||
|
|
||
| // If capture is already in progress, wait for it | ||
| if (state.capturePromise) { | ||
| logger.info(`[Snapshot] Capture already in progress, waiting...`); | ||
| await state.capturePromise; | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| // Start capture and store the promise so other callers can wait for it | ||
| state.capturePromise = this.captureEnvironmentForNotebook(notebookUri); | ||
| } | ||
|
|
There was a problem hiding this comment.
captureEnvironmentBeforeExecution() is not actually blocking (spec mismatch).
Callers await this expecting env capture to complete before execution, but the method returns immediately after setting state.capturePromise. This breaks the contract in the interface docs and will cause missing/late environment data.
Proposed fix
async captureEnvironmentBeforeExecution(notebookUri: string): Promise<void> {
logger.info(`[Snapshot] captureEnvironmentBeforeExecution called for ${notebookUri}`);
const state = this.getOrCreateExecutionState(notebookUri, Date.now());
// If capture is already in progress, wait for it
if (state.capturePromise) {
logger.info(`[Snapshot] Capture already in progress, waiting...`);
await state.capturePromise;
return;
}
// Start capture and store the promise so other callers can wait for it
- state.capturePromise = this.captureEnvironmentForNotebook(notebookUri);
+ state.capturePromise = this.captureEnvironmentForNotebook(notebookUri);
+ await state.capturePromise;
}🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshots/snapshotService.ts around lines 189 - 205,
captureEnvironmentBeforeExecution currently sets state.capturePromise and
returns immediately, so callers awaiting it don't actually wait for capture to
finish; fix by awaiting state.capturePromise (the promise returned from
captureEnvironmentForNotebook) before returning, and ensure you clear
state.capturePromise when the capture completes or fails. Specifically, in
captureEnvironmentBeforeExecution (which uses getOrCreateExecutionState and
state.capturePromise and calls captureEnvironmentForNotebook), assign
state.capturePromise = this.captureEnvironmentForNotebook(notebookUri), then
await it inside a try/finally and in finally set state.capturePromise =
undefined (and rethrow errors if you want propagation) so callers properly block
until capture completes.
| async getEnvironmentMetadata(notebookUri: string): Promise<Environment | undefined> { | ||
| const state = this.executionStates.get(notebookUri); | ||
|
|
||
| logger.info(`[Snapshot] getEnvironmentMetadata for ${notebookUri}`); | ||
| logger.info(Boolean(state) ? '[Snapshot] State exists.' : '[Snapshot] No state found.'); | ||
|
|
||
| if (!state) { | ||
| logger.info(`[Snapshot] Available URIs: ${Array.from(this.executionStates.keys()).join(', ')}`); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| // If capture is in progress, wait for it to complete | ||
| if (state.capturePromise && !state.environmentCaptured) { | ||
| logger.info(`[Snapshot] Waiting for capture to complete before returning metadata.`); | ||
|
|
||
| const timeoutPromise = new Promise<void>((_, reject) => { | ||
| setTimeout(() => reject(new TimeoutError('Timeout waiting for environment capture.')), 10_000); | ||
| }); | ||
|
|
||
| await Promise.race([state.capturePromise, timeoutPromise]).catch((error) => { | ||
| if (error instanceof TimeoutError) { | ||
| logger.warn('[Snapshot] Timed out waiting for environment capture'); | ||
| } else { | ||
| throw error; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| logger.info(`[Snapshot] state.environment exists: ${!!state.environment}`); | ||
| logger.info(`[Snapshot] state.environmentCaptured: ${state.environmentCaptured}`); | ||
|
|
||
| return state.environment; | ||
| } |
There was a problem hiding this comment.
Timeout race leaks timers + log level is too loud.
getEnvironmentMetadata() creates a setTimeout but never clears it, and the method logs multiple info lines per call (likely noisy). Use clearTimeout() and downgrade most of these to debug/trace.
Proposed fix
- const timeoutPromise = new Promise<void>((_, reject) => {
- setTimeout(() => reject(new TimeoutError('Timeout waiting for environment capture.')), 10_000);
- });
+ let timeoutHandle: ReturnType<typeof setTimeout> | undefined;
+ const timeoutPromise = new Promise<void>((_, reject) => {
+ timeoutHandle = setTimeout(
+ () => reject(new TimeoutError('Timeout waiting for environment capture.')),
+ 10_000
+ );
+ });
- await Promise.race([state.capturePromise, timeoutPromise]).catch((error) => {
+ await Promise.race([state.capturePromise, timeoutPromise])
+ .catch((error) => {
if (error instanceof TimeoutError) {
logger.warn('[Snapshot] Timed out waiting for environment capture');
} else {
throw error;
}
- });
+ })
+ .finally(() => {
+ if (timeoutHandle) {
+ clearTimeout(timeoutHandle);
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getEnvironmentMetadata(notebookUri: string): Promise<Environment | undefined> { | |
| const state = this.executionStates.get(notebookUri); | |
| logger.info(`[Snapshot] getEnvironmentMetadata for ${notebookUri}`); | |
| logger.info(Boolean(state) ? '[Snapshot] State exists.' : '[Snapshot] No state found.'); | |
| if (!state) { | |
| logger.info(`[Snapshot] Available URIs: ${Array.from(this.executionStates.keys()).join(', ')}`); | |
| return; | |
| } | |
| // If capture is in progress, wait for it to complete | |
| if (state.capturePromise && !state.environmentCaptured) { | |
| logger.info(`[Snapshot] Waiting for capture to complete before returning metadata.`); | |
| const timeoutPromise = new Promise<void>((_, reject) => { | |
| setTimeout(() => reject(new TimeoutError('Timeout waiting for environment capture.')), 10_000); | |
| }); | |
| await Promise.race([state.capturePromise, timeoutPromise]).catch((error) => { | |
| if (error instanceof TimeoutError) { | |
| logger.warn('[Snapshot] Timed out waiting for environment capture'); | |
| } else { | |
| throw error; | |
| } | |
| }); | |
| } | |
| logger.info(`[Snapshot] state.environment exists: ${!!state.environment}`); | |
| logger.info(`[Snapshot] state.environmentCaptured: ${state.environmentCaptured}`); | |
| return state.environment; | |
| } | |
| async getEnvironmentMetadata(notebookUri: string): Promise<Environment | undefined> { | |
| const state = this.executionStates.get(notebookUri); | |
| logger.info(`[Snapshot] getEnvironmentMetadata for ${notebookUri}`); | |
| logger.info(Boolean(state) ? '[Snapshot] State exists.' : '[Snapshot] No state found.'); | |
| if (!state) { | |
| logger.info(`[Snapshot] Available URIs: ${Array.from(this.executionStates.keys()).join(', ')}`); | |
| return; | |
| } | |
| // If capture is in progress, wait for it to complete | |
| if (state.capturePromise && !state.environmentCaptured) { | |
| logger.info(`[Snapshot] Waiting for capture to complete before returning metadata.`); | |
| let timeoutHandle: ReturnType<typeof setTimeout> | undefined; | |
| const timeoutPromise = new Promise<void>((_, reject) => { | |
| timeoutHandle = setTimeout( | |
| () => reject(new TimeoutError('Timeout waiting for environment capture.')), | |
| 10_000 | |
| ); | |
| }); | |
| await Promise.race([state.capturePromise, timeoutPromise]) | |
| .catch((error) => { | |
| if (error instanceof TimeoutError) { | |
| logger.warn('[Snapshot] Timed out waiting for environment capture'); | |
| } else { | |
| throw error; | |
| } | |
| }) | |
| .finally(() => { | |
| if (timeoutHandle) { | |
| clearTimeout(timeoutHandle); | |
| } | |
| }); | |
| } | |
| logger.info(`[Snapshot] state.environment exists: ${!!state.environment}`); | |
| logger.info(`[Snapshot] state.environmentCaptured: ${state.environmentCaptured}`); | |
| return state.environment; | |
| } |
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshots/snapshotService.ts around lines 290 - 323,
The method getEnvironmentMetadata creates a timeout via setTimeout that is never
cleared and emits many info-level logs; update getEnvironmentMetadata to store
the timer id and call clearTimeout(timer) when the capturePromise resolves or
the race finishes to avoid leaking timers, reduce noisy logger.info calls to
logger.debug or logger.trace (e.g., the lines logging state existence, waiting
for capture, environment presence, and environmentCaptured), and keep the
TimeoutError logic unchanged (use the existing TimeoutError class and keep the
same warn on timeout); reference the symbols getEnvironmentMetadata,
state.capturePromise, timeoutPromise, TimeoutError, and logger when making the
changes.
| private async ensureSnapshotsDirectory(snapshotsDir: Uri): Promise<boolean> { | ||
| try { | ||
| const stat = await workspace.fs.stat(snapshotsDir); | ||
|
|
||
| if (stat.type !== FileType.Directory) { | ||
| logger.error( | ||
| `[Snapshot] Snapshots path exists but is not a directory: ${Utils.basename(snapshotsDir)}` | ||
| ); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } catch { | ||
| logger.debug(`[Snapshot] Creating snapshots directory: ${Utils.basename(snapshotsDir)}`); | ||
|
|
||
| try { | ||
| await workspace.fs.createDirectory(snapshotsDir); | ||
|
|
||
| return true; | ||
| } catch (error) { | ||
| logger.error(`[Snapshot] Failed to create snapshots directory: ${Utils.basename(snapshotsDir)}`, error); | ||
|
|
||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
FileType is a bitmask; strict equality can mis-detect directories.
stat.type !== FileType.Directory will fail for (e.g.) directory symlinks. Prefer a bitmask check.
Proposed fix
- if (stat.type !== FileType.Directory) {
+ if ((stat.type & FileType.Directory) === 0) {
logger.error(
`[Snapshot] Snapshots path exists but is not a directory: ${Utils.basename(snapshotsDir)}`
);🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshots/snapshotService.ts around lines 543 - 568,
The directory check in ensureSnapshotsDirectory incorrectly uses strict equality
against FileType.Directory which can miss directory symlinks because FileType is
a bitmask; replace the equality check with a bitmask test (e.g., check that
stat.type has the FileType.Directory bit set) when validating the result of
workspace.fs.stat in ensureSnapshotsDirectory, so the branch that logs
"[Snapshot] Snapshots path exists but is not a directory:
${Utils.basename(snapshotsDir)}" only executes when the Directory bit is absent;
leave the surrounding logging and error handling intact.
| private recordCellExecutionStart(notebookUri: string, cellId: string, startTime: number): void { | ||
| const state = this.getOrCreateExecutionState(notebookUri, startTime); | ||
| const isoTimestamp = new Date(startTime).toISOString(); | ||
| const cellMetadata = state.cellMetadata.get(cellId) || { contentHash: '' }; | ||
|
|
||
| cellMetadata.executionStartedAt = isoTimestamp; | ||
|
|
||
| delete cellMetadata.executionFinishedAt; | ||
|
|
||
| state.cellMetadata.set(cellId, cellMetadata); | ||
|
|
||
| logger.trace(`[Snapshot] Cell ${cellId} execution started at ${isoTimestamp}`); | ||
| } |
There was a problem hiding this comment.
BlockExecutionMetadata.contentHash is required but never computed.
Right now you store { contentHash: '' }, which makes the metadata misleading at best. If anything relies on contentHash for reproducibility/diffing, this is effectively broken. Consider computing a hash from the cell source (and updating it on each start).
| import { assert } from 'chai'; | ||
| import { anything, instance, mock, when } from 'ts-mockito'; | ||
| import { FileType, Uri, WorkspaceConfiguration, WorkspaceFolder } from 'vscode'; | ||
|
|
||
| import type { DeepnoteBlock, DeepnoteFile } from '@deepnote/blocks'; | ||
|
|
||
| import { IEnvironmentCapture } from './environmentCapture.node'; | ||
| import { SnapshotService } from './snapshotService'; | ||
| import type { DeepnoteOutput } from '../../../platform/deepnote/deepnoteTypes'; | ||
| import { IDisposableRegistry } from '../../../platform/common/types'; | ||
| import { mockedVSCodeNamespaces, resetVSCodeMocks } from '../../../test/vscode-mock'; | ||
|
|
||
| suite('SnapshotService', () => { | ||
| let service: SnapshotService; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| let serviceAny: any; | ||
| let mockEnvironmentCapture: IEnvironmentCapture; | ||
| let mockDisposables: IDisposableRegistry; | ||
|
|
||
| setup(() => { | ||
| resetVSCodeMocks(); | ||
| mockEnvironmentCapture = mock<IEnvironmentCapture>(); | ||
| mockDisposables = []; | ||
| service = new SnapshotService(instance(mockEnvironmentCapture), mockDisposables); | ||
| serviceAny = service; | ||
| }); | ||
|
|
||
| function createProjectData(projectId = 'test-project-id-123', projectName = 'My Project'): DeepnoteFile { | ||
| return { | ||
| metadata: { | ||
| createdAt: '2025-01-01T00:00:00Z' | ||
| }, | ||
| version: '1.0', | ||
| project: { | ||
| id: projectId, | ||
| name: projectName, | ||
| notebooks: [ | ||
| { | ||
| id: 'notebook-1', | ||
| name: 'Notebook 1', | ||
| blocks: [ | ||
| { | ||
| id: 'block-1', | ||
| type: 'code', | ||
| sortingKey: 'a0', | ||
| content: 'print(1)', | ||
| outputs: [{ output_type: 'stream', text: '1' }] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| suite('buildSnapshotPath', () => { | ||
| test('should build correct path for latest variant', () => { | ||
| const projectUri = Uri.file('/path/to/my-project.deepnote'); | ||
| const projectId = 'e132b172-b114-410e-8331-011517db664f'; | ||
| const projectName = 'My Project'; | ||
|
|
||
| const result = serviceAny.buildSnapshotPath(projectUri, projectId, projectName, 'latest'); | ||
|
|
||
| assert.include(result.fsPath, 'snapshots'); | ||
| assert.include(result.fsPath, 'my-project'); | ||
| assert.include(result.fsPath, projectId); | ||
| assert.include(result.fsPath, 'latest'); | ||
| assert.include(result.fsPath, '.snapshot.deepnote'); | ||
| }); | ||
|
|
||
| test('should build correct path for timestamped variant', () => { | ||
| const projectUri = Uri.file('/path/to/my-project.deepnote'); | ||
| const projectId = 'e132b172-b114-410e-8331-011517db664f'; | ||
| const projectName = 'My Project'; | ||
| const timestamp = '2025-12-11T10-31-48'; | ||
|
|
||
| const result = serviceAny.buildSnapshotPath(projectUri, projectId, projectName, timestamp); | ||
|
|
||
| assert.include(result.fsPath, 'snapshots'); | ||
| assert.include(result.fsPath, 'my-project'); | ||
| assert.include(result.fsPath, projectId); | ||
| assert.include(result.fsPath, timestamp); | ||
| assert.include(result.fsPath, '.snapshot.deepnote'); | ||
| }); | ||
|
|
||
| test('should slugify project name correctly', () => { | ||
| const projectUri = Uri.file('/path/to/file.deepnote'); | ||
| const projectId = 'abc-123'; | ||
| const projectName = 'Customer Churn ML Playbook!'; | ||
|
|
||
| const result = serviceAny.buildSnapshotPath(projectUri, projectId, projectName, 'latest'); | ||
|
|
||
| assert.include(result.fsPath, 'customer-churn-ml-playbook'); | ||
| assert.notInclude(result.fsPath, '!'); | ||
| assert.notInclude(result.fsPath, ' '); | ||
| }); | ||
|
|
||
| test('should handle project names with special characters', () => { | ||
| const projectUri = Uri.file('/path/to/file.deepnote'); | ||
| const projectId = 'abc-123'; | ||
| const projectName = 'Test@#$%Project'; | ||
|
|
||
| const result = serviceAny.buildSnapshotPath(projectUri, projectId, projectName, 'latest'); | ||
|
|
||
| assert.include(result.fsPath, 'testproject'); | ||
| }); | ||
|
|
||
| test('should handle project names with multiple spaces', () => { | ||
| const projectUri = Uri.file('/path/to/file.deepnote'); | ||
| const projectId = 'abc-123'; | ||
| const projectName = 'My Project Name'; | ||
|
|
||
| const result = serviceAny.buildSnapshotPath(projectUri, projectId, projectName, 'latest'); | ||
|
|
||
| assert.include(result.fsPath, 'my-project-name'); | ||
| assert.notInclude(result.fsPath, '--'); | ||
| }); | ||
|
|
||
| test('should throw error for empty project name', () => { | ||
| const projectUri = Uri.file('/path/to/file.deepnote'); | ||
| const projectId = 'abc-123'; | ||
| const projectName = ''; | ||
|
|
||
| assert.throws( | ||
| () => serviceAny.buildSnapshotPath(projectUri, projectId, projectName, 'latest'), | ||
| 'Project name cannot be empty or contain only special characters' | ||
| ); | ||
| }); | ||
|
|
||
| test('should throw error for project name with only special characters', () => { | ||
| const projectUri = Uri.file('/path/to/file.deepnote'); | ||
| const projectId = 'abc-123'; | ||
| const projectName = '@#$%^&*()'; | ||
|
|
||
| assert.throws( | ||
| () => serviceAny.buildSnapshotPath(projectUri, projectId, projectName, 'latest'), | ||
| 'Project name cannot be empty or contain only special characters' | ||
| ); | ||
| }); | ||
|
|
||
| test('should throw error for project name with only whitespace', () => { | ||
| const projectUri = Uri.file('/path/to/file.deepnote'); | ||
| const projectId = 'abc-123'; | ||
| const projectName = ' '; | ||
|
|
||
| assert.throws( | ||
| () => serviceAny.buildSnapshotPath(projectUri, projectId, projectName, 'latest'), | ||
| 'Project name cannot be empty or contain only special characters' | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| suite('mergeOutputsIntoBlocks', () => { | ||
| test('should merge outputs into blocks by ID', () => { | ||
| const blocks: DeepnoteBlock[] = [ | ||
| { id: 'block-1', type: 'code', sortingKey: 'a0', content: 'print(1)' }, | ||
| { id: 'block-2', type: 'code', sortingKey: 'a1', content: 'print(2)' }, | ||
| { id: 'block-3', type: 'markdown', sortingKey: 'a2', content: '# Hello' } | ||
| ]; | ||
|
|
||
| const outputs = new Map<string, DeepnoteOutput[]>(); | ||
|
|
||
| outputs.set('block-1', [{ output_type: 'stream', name: 'stdout', text: '1\n' }]); | ||
| outputs.set('block-2', [{ output_type: 'stream', name: 'stdout', text: '2\n' }]); | ||
|
|
||
| const result = service.mergeOutputsIntoBlocks(blocks, outputs); | ||
|
|
||
| assert.deepEqual(result[0].outputs, [{ output_type: 'stream', name: 'stdout', text: '1\n' }]); | ||
| assert.deepEqual(result[1].outputs, [{ output_type: 'stream', name: 'stdout', text: '2\n' }]); | ||
| assert.isUndefined(result[2].outputs); | ||
| }); | ||
|
|
||
| test('should not modify original blocks', () => { | ||
| const blocks: DeepnoteBlock[] = [{ id: 'block-1', type: 'code', sortingKey: 'a0', content: 'print(1)' }]; | ||
|
|
||
| const outputs = new Map<string, DeepnoteOutput[]>(); | ||
|
|
||
| outputs.set('block-1', [{ output_type: 'stream', text: 'new' }]); | ||
|
|
||
| service.mergeOutputsIntoBlocks(blocks, outputs); | ||
|
|
||
| assert.isUndefined(blocks[0].outputs); | ||
| }); | ||
|
|
||
| test('should preserve blocks without matching outputs', () => { | ||
| const blocks: DeepnoteBlock[] = [ | ||
| { | ||
| id: 'block-1', | ||
| type: 'code', | ||
| sortingKey: 'a0', | ||
| content: 'print(1)', | ||
| outputs: [{ output_type: 'stream', text: 'old' }] | ||
| } | ||
| ]; | ||
|
|
||
| const outputs = new Map<string, DeepnoteOutput[]>(); | ||
|
|
||
| outputs.set('block-2', [{ output_type: 'stream', text: 'new' }]); | ||
|
|
||
| const result = service.mergeOutputsIntoBlocks(blocks, outputs); | ||
|
|
||
| assert.deepEqual(result[0].outputs, [{ output_type: 'stream', text: 'old' }]); | ||
| }); | ||
|
|
||
| test('should handle empty outputs map', () => { | ||
| const blocks: DeepnoteBlock[] = [{ id: 'block-1', type: 'code', sortingKey: 'a0', content: 'print(1)' }]; | ||
|
|
||
| const outputs = new Map<string, DeepnoteOutput[]>(); | ||
|
|
||
| const result = service.mergeOutputsIntoBlocks(blocks, outputs); | ||
|
|
||
| assert.lengthOf(result, 1); | ||
| assert.isUndefined(result[0].outputs); | ||
| }); | ||
|
|
||
| test('should handle empty blocks array', () => { | ||
| const blocks: DeepnoteBlock[] = []; | ||
| const outputs = new Map<string, DeepnoteOutput[]>(); | ||
|
|
||
| outputs.set('block-1', [{ output_type: 'stream', text: 'test' }]); | ||
|
|
||
| const result = service.mergeOutputsIntoBlocks(blocks, outputs); | ||
|
|
||
| assert.lengthOf(result, 0); | ||
| }); | ||
| }); | ||
|
|
||
| suite('stripOutputsFromBlocks', () => { | ||
| test('should remove outputs from all blocks', () => { | ||
| const blocks: DeepnoteBlock[] = [ | ||
| { | ||
| id: 'block-1', | ||
| type: 'code', | ||
| sortingKey: 'a0', | ||
| content: 'print(1)', | ||
| outputs: [{ output_type: 'stream', text: '1' }] | ||
| }, | ||
| { | ||
| id: 'block-2', | ||
| type: 'code', | ||
| sortingKey: 'a1', | ||
| content: 'print(2)', | ||
| outputs: [{ output_type: 'stream', text: '2' }] | ||
| } | ||
| ]; | ||
|
|
||
| const result = service.stripOutputsFromBlocks(blocks); | ||
|
|
||
| assert.lengthOf(result, 2); | ||
| assert.isUndefined(result[0].outputs); | ||
| assert.isUndefined(result[1].outputs); | ||
| }); | ||
|
|
||
| test('should preserve other block properties', () => { | ||
| const blocks: DeepnoteBlock[] = [ | ||
| { | ||
| id: 'block-1', | ||
| type: 'code', | ||
| sortingKey: 'a0', | ||
| content: 'print(1)', | ||
| contentHash: 'sha256:abc123', | ||
| executionStartedAt: '2025-01-01T00:00:00Z', | ||
| outputs: [{ output_type: 'stream', text: '1' }] | ||
| } | ||
| ]; | ||
|
|
||
| const result = service.stripOutputsFromBlocks(blocks); | ||
|
|
||
| assert.strictEqual(result[0].id, 'block-1'); | ||
| assert.strictEqual(result[0].type, 'code'); | ||
| assert.strictEqual(result[0].content, 'print(1)'); | ||
| assert.strictEqual(result[0].contentHash, 'sha256:abc123'); | ||
| assert.strictEqual(result[0].executionStartedAt, '2025-01-01T00:00:00Z'); | ||
| assert.isUndefined(result[0].outputs); | ||
| }); | ||
|
|
||
| test('should not modify original blocks', () => { | ||
| const blocks: DeepnoteBlock[] = [ | ||
| { | ||
| id: 'block-1', | ||
| type: 'code', | ||
| sortingKey: 'a0', | ||
| content: 'print(1)', | ||
| outputs: [{ output_type: 'stream', text: '1' }] | ||
| } | ||
| ]; | ||
|
|
||
| service.stripOutputsFromBlocks(blocks); | ||
|
|
||
| assert.isDefined(blocks[0].outputs); | ||
| }); | ||
|
|
||
| test('should handle blocks without outputs', () => { | ||
| const blocks: DeepnoteBlock[] = [{ id: 'block-1', type: 'code', sortingKey: 'a0', content: 'print(1)' }]; | ||
|
|
||
| const result = service.stripOutputsFromBlocks(blocks); | ||
|
|
||
| assert.lengthOf(result, 1); | ||
| assert.isUndefined(result[0].outputs); | ||
| }); | ||
|
|
||
| test('should handle empty array', () => { | ||
| const blocks: DeepnoteBlock[] = []; | ||
|
|
||
| const result = service.stripOutputsFromBlocks(blocks); | ||
|
|
||
| assert.lengthOf(result, 0); | ||
| }); | ||
| }); | ||
|
|
||
| suite('extractOutputsFromBlocks', () => { | ||
| test('should extract outputs into a map', () => { | ||
| const blocks: DeepnoteBlock[] = [ | ||
| { | ||
| id: 'block-1', | ||
| type: 'code', | ||
| sortingKey: 'a0', | ||
| content: 'print(1)', | ||
| outputs: [{ output_type: 'stream', text: '1' }] | ||
| }, | ||
| { | ||
| id: 'block-2', | ||
| type: 'code', | ||
| sortingKey: 'a1', | ||
| content: 'print(2)', | ||
| outputs: [{ output_type: 'stream', text: '2' }] | ||
| } | ||
| ]; | ||
|
|
||
| const result = service.extractOutputsFromBlocks(blocks); | ||
|
|
||
| assert.strictEqual(result.size, 2); | ||
| assert.deepEqual(result.get('block-1'), [{ output_type: 'stream', text: '1' }]); | ||
| assert.deepEqual(result.get('block-2'), [{ output_type: 'stream', text: '2' }]); | ||
| }); | ||
|
|
||
| test('should skip blocks without outputs', () => { | ||
| const blocks: DeepnoteBlock[] = [ | ||
| { | ||
| id: 'block-1', | ||
| type: 'code', | ||
| sortingKey: 'a0', | ||
| content: 'print(1)', | ||
| outputs: [{ output_type: 'stream', text: '1' }] | ||
| }, | ||
| { id: 'block-2', type: 'code', sortingKey: 'a1', content: 'print(2)' } | ||
| ]; | ||
|
|
||
| const result = service.extractOutputsFromBlocks(blocks); | ||
|
|
||
| assert.strictEqual(result.size, 1); | ||
| assert.isTrue(result.has('block-1')); | ||
| assert.isFalse(result.has('block-2')); | ||
| }); | ||
|
|
||
| test('should skip blocks without ID', () => { | ||
| const blocks = [ | ||
| { type: 'code', sortingKey: 'a0', content: 'print(1)', outputs: [{ output_type: 'stream', text: '1' }] } | ||
| ] as unknown as DeepnoteBlock[]; | ||
|
|
||
| const result = service.extractOutputsFromBlocks(blocks); | ||
|
|
||
| assert.strictEqual(result.size, 0); | ||
| }); | ||
|
|
||
| test('should handle empty array', () => { | ||
| const blocks: DeepnoteBlock[] = []; | ||
|
|
||
| const result = service.extractOutputsFromBlocks(blocks); | ||
|
|
||
| assert.strictEqual(result.size, 0); | ||
| }); | ||
|
|
||
| test('should handle complex outputs', () => { | ||
| const complexOutput: DeepnoteOutput = { | ||
| output_type: 'execute_result', | ||
| execution_count: 1, | ||
| data: { | ||
| 'text/html': '<table>...</table>', | ||
| 'text/plain': 'DataFrame...' | ||
| }, | ||
| metadata: { table_state_spec: '{}' } | ||
| }; | ||
|
|
||
| const blocks: DeepnoteBlock[] = [ | ||
| { id: 'block-1', type: 'code', sortingKey: 'a0', content: 'df', outputs: [complexOutput] } | ||
| ]; | ||
|
|
||
| const result = service.extractOutputsFromBlocks(blocks); | ||
|
|
||
| assert.deepEqual(result.get('block-1'), [complexOutput]); | ||
| }); | ||
| }); | ||
|
|
||
| suite('isSnapshotsEnabled', () => { | ||
| test('should return true when snapshots.enabled is true', () => { | ||
| const mockConfig = mock<WorkspaceConfiguration>(); | ||
| when(mockConfig.get<boolean>('snapshots.enabled', false)).thenReturn(true); | ||
| when(mockedVSCodeNamespaces.workspace.getConfiguration('deepnote')).thenReturn(instance(mockConfig)); | ||
|
|
||
| const result = service.isSnapshotsEnabled(); | ||
|
|
||
| assert.isTrue(result); | ||
| }); | ||
|
|
||
| test('should return false when snapshots.enabled is false', () => { | ||
| const mockConfig = mock<WorkspaceConfiguration>(); | ||
| when(mockConfig.get<boolean>('snapshots.enabled', false)).thenReturn(false); | ||
| when(mockedVSCodeNamespaces.workspace.getConfiguration('deepnote')).thenReturn(instance(mockConfig)); | ||
|
|
||
| const result = service.isSnapshotsEnabled(); | ||
|
|
||
| assert.isFalse(result); | ||
| }); | ||
| }); | ||
|
|
||
| suite('readSnapshot', () => { | ||
| const projectId = 'test-project-id-123'; | ||
|
|
||
| test('should return undefined when no workspace folders exist', async () => { | ||
| when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn(undefined); | ||
|
|
||
| const result = await service.readSnapshot(projectId); | ||
|
|
||
| assert.isUndefined(result); | ||
| }); | ||
|
|
||
| test('should return undefined when workspace folders array is empty', async () => { | ||
| when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn([]); | ||
|
|
||
| const result = await service.readSnapshot(projectId); | ||
|
|
||
| assert.isUndefined(result); | ||
| }); | ||
|
|
||
| test('should find and parse latest snapshot file', async () => { | ||
| const workspaceFolder: WorkspaceFolder = { | ||
| uri: Uri.file('/workspace'), | ||
| name: 'workspace', | ||
| index: 0 | ||
| }; | ||
| when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn([workspaceFolder]); | ||
|
|
||
| const snapshotUri = Uri.file('/workspace/snapshots/project_test-project-id-123_latest.snapshot.deepnote'); | ||
| when(mockedVSCodeNamespaces.workspace.findFiles(anything(), anything(), anything())).thenResolve([ | ||
| snapshotUri | ||
| ] as any); | ||
|
|
||
| const snapshotYaml = ` | ||
| version: '1.0' | ||
| project: | ||
| id: test-project-id-123 | ||
| name: Test Project | ||
| notebooks: | ||
| - id: notebook-1 | ||
| name: Notebook 1 | ||
| blocks: | ||
| - id: block-1 | ||
| type: code | ||
| content: print(1) | ||
| outputs: | ||
| - output_type: stream | ||
| name: stdout | ||
| text: '1' | ||
| - id: block-2 | ||
| type: markdown | ||
| content: '# Hello' | ||
| `; | ||
| const mockFs = mock<typeof import('vscode').workspace.fs>(); | ||
| when(mockFs.readFile(anything())).thenResolve(new TextEncoder().encode(snapshotYaml) as any); | ||
| when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); | ||
|
|
||
| const result = await service.readSnapshot(projectId); | ||
|
|
||
| assert.isDefined(result); | ||
| assert.strictEqual(result!.size, 1); | ||
| assert.deepEqual(result!.get('block-1'), [{ output_type: 'stream', name: 'stdout', text: '1' }]); | ||
| }); | ||
|
|
||
| test('should return undefined when no snapshot files found', async () => { | ||
| const workspaceFolder: WorkspaceFolder = { | ||
| uri: Uri.file('/workspace'), | ||
| name: 'workspace', | ||
| index: 0 | ||
| }; | ||
| when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn([workspaceFolder]); | ||
| when(mockedVSCodeNamespaces.workspace.findFiles(anything(), anything(), anything())).thenResolve([] as any); | ||
|
|
||
| const result = await service.readSnapshot(projectId); | ||
|
|
||
| assert.isUndefined(result); | ||
| }); | ||
|
|
||
| test('should fall back to most recent timestamped snapshot when no latest exists', async () => { | ||
| const workspaceFolder: WorkspaceFolder = { | ||
| uri: Uri.file('/workspace'), | ||
| name: 'workspace', | ||
| index: 0 | ||
| }; | ||
| when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn([workspaceFolder]); | ||
|
|
||
| // First call for latest - returns empty | ||
| // Second call for timestamped - returns files | ||
| const timestampedUri1 = Uri.file( | ||
| '/workspace/snapshots/project_test-project-id-123_2025-01-01T10-00-00.snapshot.deepnote' | ||
| ); | ||
| const timestampedUri2 = Uri.file( | ||
| '/workspace/snapshots/project_test-project-id-123_2025-01-02T10-00-00.snapshot.deepnote' | ||
| ); | ||
|
|
||
| let callCount = 0; | ||
| when(mockedVSCodeNamespaces.workspace.findFiles(anything(), anything(), anything())).thenCall(() => { | ||
| callCount++; | ||
| if (callCount === 1) { | ||
| return Promise.resolve([]); | ||
| } | ||
|
|
||
| return Promise.resolve([timestampedUri1, timestampedUri2]); | ||
| }); | ||
|
|
||
| const snapshotYaml = ` | ||
| version: '1.0' | ||
| project: | ||
| notebooks: | ||
| - blocks: | ||
| - id: block-1 | ||
| outputs: | ||
| - output_type: stream | ||
| text: 'from timestamped' | ||
| `; | ||
| const mockFs = mock<typeof import('vscode').workspace.fs>(); | ||
| when(mockFs.readFile(anything())).thenResolve(new TextEncoder().encode(snapshotYaml) as any); | ||
| when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); | ||
|
|
||
| const result = await service.readSnapshot(projectId); | ||
|
|
||
| assert.isDefined(result); | ||
| assert.strictEqual(result!.size, 1); | ||
| }); | ||
|
|
||
| test('should return empty map when snapshot file read fails', async () => { | ||
| const workspaceFolder: WorkspaceFolder = { | ||
| uri: Uri.file('/workspace'), | ||
| name: 'workspace', | ||
| index: 0 | ||
| }; | ||
| when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn([workspaceFolder]); | ||
|
|
||
| const snapshotUri = Uri.file('/workspace/snapshots/project_test-project-id-123_latest.snapshot.deepnote'); | ||
| when(mockedVSCodeNamespaces.workspace.findFiles(anything(), anything(), anything())).thenResolve([ | ||
| snapshotUri | ||
| ] as any); | ||
|
|
||
| const mockFs = mock<typeof import('vscode').workspace.fs>(); | ||
| when(mockFs.readFile(anything())).thenReject(new Error('File read error')); | ||
| when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); | ||
|
|
||
| const result = await service.readSnapshot(projectId); | ||
|
|
||
| // parseSnapshotFile catches read errors and returns empty map | ||
| assert.isDefined(result); | ||
| assert.strictEqual(result!.size, 0); | ||
| }); | ||
|
|
||
| test('should return empty map when snapshot has invalid structure', async () => { | ||
| const workspaceFolder: WorkspaceFolder = { | ||
| uri: Uri.file('/workspace'), | ||
| name: 'workspace', | ||
| index: 0 | ||
| }; | ||
| when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn([workspaceFolder]); | ||
|
|
||
| const snapshotUri = Uri.file('/workspace/snapshots/project_test-project-id-123_latest.snapshot.deepnote'); | ||
| when(mockedVSCodeNamespaces.workspace.findFiles(anything(), anything(), anything())).thenResolve([ | ||
| snapshotUri | ||
| ] as any); | ||
|
|
||
| const invalidYaml = 'not_an_object'; | ||
| const mockFs = mock<typeof import('vscode').workspace.fs>(); | ||
| when(mockFs.readFile(anything())).thenResolve(new TextEncoder().encode(invalidYaml) as any); | ||
| when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); | ||
|
|
||
| const result = await service.readSnapshot(projectId); | ||
|
|
||
| assert.isDefined(result); | ||
| assert.strictEqual(result!.size, 0); | ||
| }); | ||
| }); | ||
|
|
||
| suite('createSnapshot', () => { | ||
| const projectUri = Uri.file('/workspace/my-project.deepnote'); | ||
| const projectId = 'test-project-id-123'; | ||
| const projectName = 'My Project'; | ||
|
|
||
| test('should create snapshot files when there are changes', async () => { | ||
| const projectData = createProjectData(); | ||
|
|
||
| const mockFs = mock<typeof import('vscode').workspace.fs>(); | ||
| // Directory doesn't exist - stat throws | ||
| when(mockFs.stat(anything())).thenReject(new Error('ENOENT')); | ||
| when(mockFs.createDirectory(anything())).thenResolve(); | ||
| when(mockFs.readFile(anything())).thenReject(new Error('ENOENT')); | ||
| when(mockFs.writeFile(anything(), anything())).thenResolve(); | ||
| when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); | ||
|
|
||
| const result = await service.createSnapshot(projectUri, projectId, projectName, projectData); | ||
|
|
||
| assert.isDefined(result); | ||
| assert.include(result!.fsPath, 'snapshot.deepnote'); | ||
| }); | ||
|
|
||
| test('should return undefined when project name is invalid', async () => { | ||
| const projectData = createProjectData(); | ||
|
|
||
| const result = await service.createSnapshot(projectUri, projectId, '', projectData); | ||
|
|
||
| assert.isUndefined(result); | ||
| }); | ||
|
|
||
| test('should return undefined when directory creation fails', async () => { | ||
| const projectData = createProjectData(); | ||
|
|
||
| const mockFs = mock<typeof import('vscode').workspace.fs>(); | ||
| when(mockFs.stat(anything())).thenReject(new Error('ENOENT')); | ||
| when(mockFs.createDirectory(anything())).thenReject(new Error('Permission denied')); | ||
| when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); | ||
|
|
||
| const result = await service.createSnapshot(projectUri, projectId, projectName, projectData); | ||
|
|
||
| assert.isUndefined(result); | ||
| }); | ||
|
|
||
| test('should skip snapshot creation when no changes detected', async () => { | ||
| const projectData = createProjectData(); | ||
|
|
||
| const mockFs = mock<typeof import('vscode').workspace.fs>(); | ||
| // Directory exists | ||
| when(mockFs.stat(anything())).thenResolve({ type: FileType.Directory } as any); | ||
| // Return same content as existing | ||
| const existingYaml = ` | ||
| metadata: | ||
| createdAt: '2025-01-01T00:00:00Z' | ||
| version: '1.0' | ||
| project: | ||
| id: test-project-id-123 | ||
| name: My Project | ||
| notebooks: | ||
| - id: notebook-1 | ||
| name: Notebook 1 | ||
| blocks: | ||
| - id: block-1 | ||
| type: code | ||
| sortingKey: a0 | ||
| content: print(1) | ||
| outputs: | ||
| - output_type: stream | ||
| text: '1' | ||
| `; | ||
| when(mockFs.readFile(anything())).thenResolve(new TextEncoder().encode(existingYaml) as any); | ||
| when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); | ||
|
|
||
| const result = await service.createSnapshot(projectUri, projectId, projectName, projectData); | ||
|
|
||
| assert.isUndefined(result); | ||
| }); | ||
|
|
||
| test('should return undefined when timestamped file write fails', async () => { | ||
| const projectData = createProjectData(); | ||
|
|
||
| const mockFs = mock<typeof import('vscode').workspace.fs>(); | ||
| when(mockFs.stat(anything())).thenResolve({ type: FileType.Directory } as any); | ||
| when(mockFs.readFile(anything())).thenReject(new Error('ENOENT')); | ||
| when(mockFs.writeFile(anything(), anything())).thenReject(new Error('Write failed')); | ||
| when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); | ||
|
|
||
| const result = await service.createSnapshot(projectUri, projectId, projectName, projectData); | ||
|
|
||
| assert.isUndefined(result); | ||
| }); | ||
|
|
||
| test('should return timestamped path even if latest write fails', async () => { | ||
| const projectData = createProjectData(); | ||
|
|
||
| const mockFs = mock<typeof import('vscode').workspace.fs>(); | ||
| when(mockFs.stat(anything())).thenResolve({ type: FileType.Directory } as any); | ||
| when(mockFs.readFile(anything())).thenReject(new Error('ENOENT')); | ||
|
|
||
| let writeCallCount = 0; | ||
| when(mockFs.writeFile(anything(), anything())).thenCall(() => { | ||
| writeCallCount++; | ||
| if (writeCallCount === 1) { | ||
| // First write (timestamped) succeeds | ||
| return Promise.resolve(); | ||
| } | ||
| // Second write (latest) fails | ||
| return Promise.reject(new Error('Write failed')); | ||
| }); | ||
| when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); | ||
|
|
||
| const result = await service.createSnapshot(projectUri, projectId, projectName, projectData); | ||
|
|
||
| assert.isDefined(result); | ||
| assert.include(result!.fsPath, 'snapshot.deepnote'); | ||
| assert.notInclude(result!.fsPath, 'latest'); | ||
| }); | ||
| }); | ||
|
|
||
| // Metadata tracking tests (now using serviceAny for private methods) | ||
| suite('execution metadata tracking', () => { | ||
| const notebookUri = 'file:///path/to/notebook.deepnote'; | ||
| const cellId = 'cell-123'; | ||
|
|
||
| suite('recordCellExecutionStart (private)', () => { | ||
| test('should record cell execution start time', () => { | ||
| const startTime = Date.now(); | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
|
|
||
| const metadata = service.getBlockExecutionMetadata(notebookUri, cellId); | ||
| assert.isDefined(metadata); | ||
| assert.isDefined(metadata!.executionStartedAt); | ||
| assert.isUndefined(metadata!.executionFinishedAt); | ||
| }); | ||
|
|
||
| test('should initialize notebook execution state', () => { | ||
| const startTime = Date.now(); | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
|
|
||
| const executionMetadata = service.getExecutionMetadata(notebookUri); | ||
| // Should not have execution metadata yet since no cells have completed | ||
| assert.isUndefined(executionMetadata); | ||
| }); | ||
|
|
||
| test('should handle multiple cells in same notebook', () => { | ||
| const startTime = Date.now(); | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, 'cell-1', startTime); | ||
| serviceAny.recordCellExecutionStart(notebookUri, 'cell-2', startTime + 1000); | ||
|
|
||
| const metadata1 = service.getBlockExecutionMetadata(notebookUri, 'cell-1'); | ||
| const metadata2 = service.getBlockExecutionMetadata(notebookUri, 'cell-2'); | ||
|
|
||
| assert.isDefined(metadata1); | ||
| assert.isDefined(metadata2); | ||
| assert.notStrictEqual(metadata1!.executionStartedAt, metadata2!.executionStartedAt); | ||
| }); | ||
| }); | ||
|
|
||
| suite('recordCellExecutionEnd (private)', () => { | ||
| test('should record successful cell execution end', () => { | ||
| const startTime = Date.now(); | ||
| const endTime = startTime + 1000; | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, cellId, endTime, true); | ||
|
|
||
| const metadata = service.getBlockExecutionMetadata(notebookUri, cellId); | ||
| assert.isDefined(metadata); | ||
| assert.isDefined(metadata!.executionStartedAt); | ||
| assert.isDefined(metadata!.executionFinishedAt); | ||
| }); | ||
|
|
||
| test('should update execution summary on success', () => { | ||
| const startTime = Date.now(); | ||
| const endTime = startTime + 1000; | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, cellId, endTime, true); | ||
|
|
||
| const executionMetadata = service.getExecutionMetadata(notebookUri); | ||
| assert.isDefined(executionMetadata); | ||
| assert.isDefined(executionMetadata!.summary); | ||
| assert.strictEqual(executionMetadata!.summary!.blocksExecuted, 1); | ||
| assert.strictEqual(executionMetadata!.summary!.blocksSucceeded, 1); | ||
| assert.strictEqual(executionMetadata!.summary!.blocksFailed, 0); | ||
| }); | ||
|
|
||
| test('should update execution summary on failure', () => { | ||
| const startTime = Date.now(); | ||
| const endTime = startTime + 1000; | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, cellId, endTime, false); | ||
|
|
||
| const executionMetadata = service.getExecutionMetadata(notebookUri); | ||
| assert.isDefined(executionMetadata); | ||
| assert.isDefined(executionMetadata!.summary); | ||
| assert.strictEqual(executionMetadata!.summary!.blocksExecuted, 1); | ||
| assert.strictEqual(executionMetadata!.summary!.blocksSucceeded, 0); | ||
| assert.strictEqual(executionMetadata!.summary!.blocksFailed, 1); | ||
| }); | ||
|
|
||
| test('should record error details on failure', () => { | ||
| const startTime = Date.now(); | ||
| const endTime = startTime + 1000; | ||
| const error = { name: 'TypeError', message: 'undefined is not a function' }; | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, cellId, endTime, false, error); | ||
|
|
||
| const executionMetadata = service.getExecutionMetadata(notebookUri); | ||
| assert.isDefined(executionMetadata); | ||
| assert.isDefined(executionMetadata!.error); | ||
| assert.strictEqual(executionMetadata!.error!.name, 'TypeError'); | ||
| assert.strictEqual(executionMetadata!.error!.message, 'undefined is not a function'); | ||
| }); | ||
|
|
||
| test('should accumulate multiple cell executions', () => { | ||
| const startTime = Date.now(); | ||
|
|
||
| // Execute 3 cells: 2 successful, 1 failed | ||
| serviceAny.recordCellExecutionStart(notebookUri, 'cell-1', startTime); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, 'cell-1', startTime + 100, true); | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, 'cell-2', startTime + 200); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, 'cell-2', startTime + 300, true); | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, 'cell-3', startTime + 400); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, 'cell-3', startTime + 500, false); | ||
|
|
||
| const executionMetadata = service.getExecutionMetadata(notebookUri); | ||
| assert.isDefined(executionMetadata); | ||
| assert.isDefined(executionMetadata!.summary); | ||
| assert.strictEqual(executionMetadata!.summary!.blocksExecuted, 3); | ||
| assert.strictEqual(executionMetadata!.summary!.blocksSucceeded, 2); | ||
| assert.strictEqual(executionMetadata!.summary!.blocksFailed, 1); | ||
| }); | ||
|
|
||
| test('should calculate total duration', () => { | ||
| const startTime = Date.now(); | ||
| const endTime = startTime + 5000; | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, cellId, endTime, true); | ||
|
|
||
| const executionMetadata = service.getExecutionMetadata(notebookUri); | ||
| assert.isDefined(executionMetadata); | ||
| assert.isDefined(executionMetadata!.summary); | ||
| assert.strictEqual(executionMetadata!.summary!.totalDurationMs, 5000); | ||
| }); | ||
| }); | ||
|
|
||
| suite('getExecutionMetadata', () => { | ||
| test('should return undefined for unknown notebook', () => { | ||
| const metadata = service.getExecutionMetadata('unknown-notebook'); | ||
| assert.isUndefined(metadata); | ||
| }); | ||
|
|
||
| test('should return undefined if no cells have been executed', () => { | ||
| const startTime = Date.now(); | ||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
|
|
||
| const metadata = service.getExecutionMetadata(notebookUri); | ||
| assert.isUndefined(metadata); | ||
| }); | ||
|
|
||
| test('should include ISO timestamps', () => { | ||
| const startTime = Date.now(); | ||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, cellId, startTime + 1000, true); | ||
|
|
||
| const metadata = service.getExecutionMetadata(notebookUri); | ||
| assert.isDefined(metadata); | ||
| assert.isDefined(metadata!.startedAt); | ||
| assert.isDefined(metadata!.finishedAt); | ||
| // Should be valid ISO date strings | ||
| assert.doesNotThrow(() => new Date(metadata!.startedAt!)); | ||
| assert.doesNotThrow(() => new Date(metadata!.finishedAt!)); | ||
| }); | ||
| }); | ||
|
|
||
| suite('getBlockExecutionMetadata', () => { | ||
| test('should return undefined for unknown notebook', () => { | ||
| const metadata = service.getBlockExecutionMetadata('unknown-notebook', cellId); | ||
| assert.isUndefined(metadata); | ||
| }); | ||
|
|
||
| test('should return undefined for unknown cell', () => { | ||
| const startTime = Date.now(); | ||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
|
|
||
| const metadata = service.getBlockExecutionMetadata(notebookUri, 'unknown-cell'); | ||
| assert.isUndefined(metadata); | ||
| }); | ||
| }); | ||
|
|
||
| suite('clearExecutionState', () => { | ||
| test('should clear all state for a notebook', () => { | ||
| const startTime = Date.now(); | ||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, cellId, startTime + 1000, true); | ||
|
|
||
| service.clearExecutionState(notebookUri); | ||
|
|
||
| const executionMetadata = service.getExecutionMetadata(notebookUri); | ||
| const blockMetadata = service.getBlockExecutionMetadata(notebookUri, cellId); | ||
|
|
||
| assert.isUndefined(executionMetadata); | ||
| assert.isUndefined(blockMetadata); | ||
| }); | ||
|
|
||
| test('should only clear state for specified notebook', () => { | ||
| const startTime = Date.now(); | ||
| const otherNotebookUri = 'file:///other/notebook.deepnote'; | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebookUri, cellId, startTime); | ||
| serviceAny.recordCellExecutionEnd(notebookUri, cellId, startTime + 1000, true); | ||
|
|
||
| serviceAny.recordCellExecutionStart(otherNotebookUri, 'other-cell', startTime); | ||
| serviceAny.recordCellExecutionEnd(otherNotebookUri, 'other-cell', startTime + 1000, true); | ||
|
|
||
| service.clearExecutionState(notebookUri); | ||
|
|
||
| // First notebook should be cleared | ||
| assert.isUndefined(service.getExecutionMetadata(notebookUri)); | ||
|
|
||
| // Second notebook should still have state | ||
| assert.isDefined(service.getExecutionMetadata(otherNotebookUri)); | ||
| }); | ||
| }); | ||
|
|
||
| suite('multiple notebooks', () => { | ||
| test('should track state independently for different notebooks', () => { | ||
| const notebook1 = 'file:///notebook1.deepnote'; | ||
| const notebook2 = 'file:///notebook2.deepnote'; | ||
| const startTime = Date.now(); | ||
|
|
||
| // Execute cells in different notebooks | ||
| serviceAny.recordCellExecutionStart(notebook1, 'cell-1', startTime); | ||
| serviceAny.recordCellExecutionEnd(notebook1, 'cell-1', startTime + 100, true); | ||
|
|
||
| serviceAny.recordCellExecutionStart(notebook2, 'cell-2', startTime); | ||
| serviceAny.recordCellExecutionEnd(notebook2, 'cell-2', startTime + 200, false); | ||
|
|
||
| const metadata1 = service.getExecutionMetadata(notebook1); | ||
| const metadata2 = service.getExecutionMetadata(notebook2); | ||
|
|
||
| assert.isDefined(metadata1); | ||
| assert.isDefined(metadata1!.summary); | ||
| assert.strictEqual(metadata1!.summary!.blocksSucceeded, 1); | ||
| assert.strictEqual(metadata1!.summary!.blocksFailed, 0); | ||
|
|
||
| assert.isDefined(metadata2); | ||
| assert.isDefined(metadata2!.summary); | ||
| assert.strictEqual(metadata2!.summary!.blocksSucceeded, 0); | ||
| assert.strictEqual(metadata2!.summary!.blocksFailed, 1); | ||
| }); | ||
| }); | ||
|
|
||
| suite('setRunAllMode / isRunAllMode', () => { | ||
| test('should return false by default', () => { | ||
| assert.isFalse(service.isRunAllMode(notebookUri)); | ||
| }); | ||
|
|
||
| test('should return true after setting run all mode', () => { | ||
| service.setRunAllMode(notebookUri); | ||
|
|
||
| assert.isTrue(service.isRunAllMode(notebookUri)); | ||
| }); | ||
|
|
||
| test('should track run all mode per notebook', () => { | ||
| const notebook1 = 'file:///notebook1.deepnote'; | ||
| const notebook2 = 'file:///notebook2.deepnote'; | ||
|
|
||
| service.setRunAllMode(notebook1); | ||
|
|
||
| assert.isTrue(service.isRunAllMode(notebook1)); | ||
| assert.isFalse(service.isRunAllMode(notebook2)); | ||
| }); | ||
| }); | ||
|
|
||
| suite('captureEnvironmentBeforeExecution', () => { | ||
| test('should not throw for valid notebook URI', async () => { | ||
| await service.captureEnvironmentBeforeExecution(notebookUri); | ||
| // Should complete without error | ||
| }); | ||
| }); | ||
|
|
||
| suite('getEnvironmentMetadata', () => { | ||
| test('should return undefined when no environment captured', async () => { | ||
| const result = await service.getEnvironmentMetadata(notebookUri); | ||
|
|
||
| assert.isUndefined(result); | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests are solid, but a few assertions are brittle / guideline-misaligned.
- Prefer asserting
InvalidProjectNameError(type) over exact message text (can change without behavior change). - Consider switching
assert.deepEqual→assert.deepStrictEqualfor object comparisons (as per test guidelines).
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts around lines 1
- 988, Replace brittle string-equality assertions and non-strict deep equality
checks: in tests that expect project name validation failures (tests invoking
serviceAny.buildSnapshotPath and service.createSnapshot for invalid names),
assert the thrown error is an instance of InvalidProjectNameError instead of
matching the exact message; and replace all assert.deepEqual usages (e.g., in
mergeOutputsIntoBlocks, extractOutputsFromBlocks, readSnapshot tests) with
assert.deepStrictEqual to follow strict object comparison guidelines and ensure
exact structural equality.
| import { ISnapshotMetadataService, SnapshotService } from './deepnote/snapshots/snapshotService'; | ||
| import { EnvironmentCapture, IEnvironmentCapture } from './deepnote/snapshots/environmentCapture.node'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all service registry files
fd "serviceRegistry" src --type fRepository: deepnote/vscode-deepnote
Length of output: 1747
🏁 Script executed:
# Check if NotebookCommandListener uses non-optional injection of ISnapshotMetadataService
rg "NotebookCommandListener" src --type ts -A 20 | head -100Repository: deepnote/vscode-deepnote
Length of output: 10020
🏁 Script executed:
# Search for all bindings of ISnapshotMetadataService across registries
rg "ISnapshotMetadataService" src --type tsRepository: deepnote/vscode-deepnote
Length of output: 1703
🏁 Script executed:
# Check if NotebookCommandListener is registered in serviceRegistry.web.ts
grep -n "NotebookCommandListener\|ISnapshotMetadataService" src/notebooks/serviceRegistry.web.tsRepository: deepnote/vscode-deepnote
Length of output: 416
🏁 Script executed:
# Check the full content of serviceRegistry.web.ts to see all bindings
cat -n src/notebooks/serviceRegistry.web.ts | head -150Repository: deepnote/vscode-deepnote
Length of output: 9159
🏁 Script executed:
# Verify that SnapshotService and related imports don't exist in web registry
rg "SnapshotService|EnvironmentCapture" src/notebooks/serviceRegistry.web.tsRepository: deepnote/vscode-deepnote
Length of output: 50
🏁 Script executed:
# Check if test registries exist and register NotebookCommandListener
grep -l "NotebookCommandListener" src/**/*serviceRegistry*.tsRepository: deepnote/vscode-deepnote
Length of output: 142
🏁 Script executed:
# Check what lines 245-249 contain in serviceRegistry.node.ts
sed -n '240,260p' src/notebooks/serviceRegistry.node.tsRepository: deepnote/vscode-deepnote
Length of output: 1104
Web registry missing ISnapshotMetadataService binding breaks DI for NotebookCommandListener.
NotebookCommandListener has a non-optional ISnapshotMetadataService dependency, but serviceRegistry.web.ts registers the class without binding the service. The node registry binds it; the web registry does not. This causes a DI failure when instantiating the web extension.
Add to src/notebooks/serviceRegistry.web.ts:
- Import and register
SnapshotServiceandEnvironmentCapture - Bind
SnapshotServicetoISnapshotMetadataService
🤖 Prompt for AI Agents
In @src/notebooks/serviceRegistry.node.ts around lines 91 - 93, The web service
registry is missing the ISnapshotMetadataService binding required by
NotebookCommandListener; update the web registry to import SnapshotService and
EnvironmentCapture and register them: bind SnapshotService to the
ISnapshotMetadataService interface (same way the node registry does) and
register EnvironmentCapture for IEnvironmentCapture so the DI container can
resolve those dependencies when instantiating NotebookCommandListener. Ensure
you use the same class names SnapshotService and EnvironmentCapture and the
interface token ISnapshotMetadataService (and IEnvironmentCapture) as in the
node registry.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/notebooks/deepnote/deepnoteNotebookManager.ts`:
- Around line 63-72: The class mixes cloning strategies: storeOriginalProject
uses structuredClone but elsewhere in this class a
JSON.parse(JSON.stringify(...)) is used (line ~99) which breaks on circular
data; replace that JSON.parse(JSON.stringify(...)) call with
structuredClone(...) so all stored project copies use structuredClone to
preserve circular references and prevent serialization errors (update the method
in this class that currently performs JSON.parse(JSON.stringify(...))
accordingly).
In `@src/notebooks/deepnote/deepnoteSerializer.ts`:
- Around line 117-130: The per-block and per-cell iteration logs in
DeepnoteSerializer (inside the DeserializeNotebook method) are too verbose for
production; change the logger.debug calls inside the two for-loops that log
block IDs and cell metadata IDs to logger.trace instead (leave the higher-level
logger.debug that reports total converted cells around convertBlocksToCells
intact) so that convertBlocksToCells, the block iteration loop, and the cell
iteration loop still log but only at trace level in production.
- Around line 232-255: The per-item logger.debug calls in deepnoteSerializer
(the two for-loops that log cell metadata and the loop that logs block ids after
recoverBlockIdsFromOriginal) are too noisy; change them to logger.trace or wrap
them behind a level check (e.g., if logger.isTraceEnabled() or logger.level ===
'trace') so detailed per-item output only appears at trace level, leaving the
higher-level logger.debug calls (like the Converted to X blocks) unchanged.
In `@src/notebooks/deepnote/snapshots/snapshotService.ts`:
- Around line 635-638: Replace the fragile fixed 50ms sleep with an event-driven
wait: instead of using new Promise(resolve => setTimeout(...)), subscribe to the
cell/state change completion event emitted by the runtime (or the queue
completion event) and resolve only when the specific cell's state transitions to
Idle (or when the queue reports the last cell processed). Modify the logic in
snapshotService.ts (the block around the awaiting of pending cell state change
events) to register a one-time listener for the cellStateChange/queueComplete
event for the affected cell(s) and await that promise, with a short fallback
timeout as a last resort.
In `@src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts`:
- Around line 355-356: Replace all uses of assert.deepEqual with
assert.deepStrictEqual in the unit tests (e.g., the assertions comparing
result.get('block-1') and result.get('block-2') outputs), updating every
occurrence in this test file so comparisons use strict deep equality; keep the
asserted values unchanged but call assert.deepStrictEqual(...) instead of
assert.deepEqual(...).
In `@src/platform/deepnote/pocket.ts`:
- Around line 62-65: The log string in logger.debug inside
addPocketToCellMetadata contains an extra closing brace (`}}`) at the end of the
formatted message; remove the stray `}` so the message ends with a single `}`.
Locate the logger.debug call that formats `[Pocket] addPocketToCellMetadata:
output id=${cell.metadata.id}, pocket keys=${Object.keys(pocket).join(',')}}`
and change it to end with `...join(',')}` (i.e., drop the extra `}` after the
join).
♻️ Duplicate comments (10)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
123-153: Double conversion remains.Cells converted at 123, potentially again at 144. Consider deferring initial conversion.
src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts (3)
120-151: Error assertions use message strings.Per past review, consider asserting error type (
InvalidProjectNameError) instead of message text for resilience.
169-171: Useassert.deepStrictEqualfor object comparisons.Per test guidelines, prefer strict equality.
Suggested fix
- assert.deepEqual(result[0].outputs, [{ output_type: 'stream', name: 'stdout', text: '1\n' }]); - assert.deepEqual(result[1].outputs, [{ output_type: 'stream', name: 'stdout', text: '2\n' }]); + assert.deepStrictEqual(result[0].outputs, [{ output_type: 'stream', name: 'stdout', text: '1\n' }]); + assert.deepStrictEqual(result[1].outputs, [{ output_type: 'stream', name: 'stdout', text: '2\n' }]);
1101-1114: Thin environment capture coverage.Consider mocking
IEnvironmentCapture.captureEnvironmentto test state updates.src/notebooks/deepnote/snapshots/snapshotService.ts (6)
184-199:captureEnvironmentBeforeExecutiondoesn't await the capture.Line 198 starts capture but doesn't await. Callers expecting blocking behavior won't get it.
Fix
// Start capture and store the promise so other callers can wait for it state.capturePromise = this.captureEnvironmentForNotebook(notebookUri); + await state.capturePromise; }
301-311: Timer leak in timeout race.
setTimeouthandle never cleared. AddclearTimeoutin finally.Fix
+ let timeoutHandle: ReturnType<typeof setTimeout> | undefined; const timeoutPromise = new Promise<void>((_, reject) => { - setTimeout(() => reject(new TimeoutError('Timeout waiting for environment capture.')), 10_000); + timeoutHandle = setTimeout(() => reject(new TimeoutError('Timeout waiting for environment capture.')), 10_000); }); - await Promise.race([state.capturePromise, timeoutPromise]).catch((error) => { + try { + await Promise.race([state.capturePromise, timeoutPromise]); + } catch (error) { if (error instanceof TimeoutError) { logger.warn('[Snapshot] Timed out waiting for environment capture'); } else { throw error; } - }); + } finally { + if (timeoutHandle) { + clearTimeout(timeoutHandle); + } + }
532-538: FileType is a bitmask.Strict equality misses directory symlinks. Use
(stat.type & FileType.Directory) === 0.Fix
- if (stat.type !== FileType.Directory) { + if ((stat.type & FileType.Directory) === 0) {
902-902:contentHashis always empty.Placeholder
''doesn't match documented "SHA-256 hash" behavior. Compute from cell source or remove field.
288-315: Verbose info logging.Routine tracing should use
logger.debugorlogger.trace. Reserveinfofor significant events.
144-152: Member ordering doesn't follow guidelines.Per coding guidelines: order by accessibility (public/private) then alphabetically. Consider reorganizing.
📜 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)
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/snapshots/snapshotService.tssrc/notebooks/deepnote/snapshots/snapshotService.unit.test.tssrc/platform/deepnote/pocket.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Order method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order
Files:
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/platform/deepnote/pocket.tssrc/notebooks/deepnote/snapshots/snapshotService.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/snapshotService.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/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/platform/deepnote/pocket.tssrc/notebooks/deepnote/snapshots/snapshotService.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/snapshotService.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/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/snapshots/snapshotService.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/snapshotService.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.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/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/snapshots/snapshotService.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/deepnote/pocket.ts
🧠 Learnings (1)
📚 Learning: 2026-01-12T09:12:12.830Z
Learnt from: Artmann
Repo: deepnote/vscode-deepnote PR: 256
File: src/notebooks/deepnote/deepnoteNotebookCommandListener.ts:549-562
Timestamp: 2026-01-12T09:12:12.830Z
Learning: IConfigurationService.updateSetting() automatically prefixes the key with the 'deepnote' namespace. When calling updateSetting, always pass the unprefixed key (e.g., use 'snapshots.enabled' instead of 'deepnote.snapshots.enabled'). This guideline applies to all TypeScript source files under src where updateSetting is used.
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/platform/deepnote/pocket.tssrc/notebooks/deepnote/snapshots/snapshotService.tssrc/notebooks/deepnote/deepnoteNotebookManager.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/snapshots/snapshotService.unit.test.ts
🧬 Code graph analysis (4)
src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteFile(13-13)
src/platform/deepnote/pocket.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/notebooks/deepnote/snapshots/environmentCapture.node.ts (1)
computeHash(92-108)
src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts (3)
src/notebooks/deepnote/snapshots/environmentCapture.node.ts (2)
IEnvironmentCapture(23-23)IEnvironmentCapture(25-30)src/test/vscode-mock.ts (2)
resetVSCodeMocks(57-305)mockedVSCodeNamespaces(15-15)src/notebooks/deepnote/deepnoteSerializer.ts (3)
DeepnoteFile(13-13)DeepnoteBlock(13-13)DeepnoteOutput(14-14)
⏰ 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 (23)
src/platform/deepnote/pocket.ts (4)
3-6: LGTM. Logger import follows established patterns.
41-42: LGTM. Entry trace aids debugging.
52-56: LGTM. Clear early-exit trace.
77-96: LGTM. Defensive fallback for ID resolution with proper cleanup of both fields.src/notebooks/deepnote/deepnoteNotebookManager.ts (1)
15-15: LGTM!Field now in correct alphabetical position among privates.
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
98-99: Solid defensive measure.Backup key guards against VS Code metadata quirks. Consistent with recovery logic in serializer.
src/notebooks/deepnote/deepnoteSerializer.ts (4)
202-213: Good isolation of project state.Clone before mutation prevents cross-notebook corruption. Comment clarifies intent.
260-278: Circular ref handling now consistent.Strip + clone combo addresses prior asymmetry concern.
312-390: Content-based recovery is reasonable fallback.Edge case: duplicate content cells may recover wrong metadata. Acceptable given it's a last-resort mechanism when VS Code loses metadata.
523-556: Deterministic hash construction.Sorted arrays and consistent object shape ensure reproducible hashes.
src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (6)
400-492: Good coverage for ID preservation with metadata.Tests the happy path where VS Code preserves cell metadata correctly.
494-563: Thorough recovery test.Validates all key metadata fields (id, sortingKey, blockGroup) are recovered via content matching.
565-628: Covers the "no match" case.Ensures new blocks get fresh IDs rather than incorrectly inheriting originals.
887-1001: Solid determinism tests.Multiple serializations verify hash stability. State isolation via structuredClone is correct.
1004-1099: Good multi-iteration consistency test.5 iterations with complex project structure validates hash reproducibility.
1101-1369: Comprehensive change detection tests.Covers content, version, integrations, and environment hash changes. All critical factors tested.
src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts (5)
229-331: Comprehensive strip tests.Covers output removal, timestamp stripping, immutability, and edge cases.
439-622: Solid readSnapshot coverage.Tests workspace edge cases, fallback logic, and error handling.
624-740: Good createSnapshot error coverage.Tests directory creation, write failures, and partial success scenarios.
742-983: Thorough metadata tracking tests.Covers start/end recording, summaries, errors, duration, multi-notebook isolation.
985-1098: Good Run All detection test.Verifies full snapshot created when all code cells execute.
src/notebooks/deepnote/snapshots/snapshotService.ts (2)
749-786: Good schema validation.Using zod
deepnoteFileSchema.safeParseprovides type-safe parsing with clear error handling.
1-27: Well-structured imports and type definitions.Clean separation of concerns with imported types and local interfaces.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| storeOriginalProject(projectId: string, project: DeepnoteProject, notebookId: string): void { | ||
| this.originalProjects.set(projectId, project); | ||
| // Deep clone to prevent mutations from affecting stored state | ||
| // This is critical for multi-notebook projects where multiple notebooks | ||
| // share the same stored project reference | ||
| // Using structuredClone to handle circular references (e.g., in output metadata) | ||
| const clonedProject = structuredClone(project); | ||
|
|
||
| this.originalProjects.set(projectId, clonedProject); | ||
| this.currentNotebookId.set(projectId, notebookId); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the deepnoteNotebookManager.ts file
fd -p "deepnoteNotebookManager.ts" --exec cat -n {} \;Repository: deepnote/vscode-deepnote
Length of output: 6222
🌐 Web query:
When was structuredClone introduced in Node.js
💡 Result:
structuredClone was added in Node.js v17.0.0 (Node.js v17, first released Oct 19, 2021). [1][2]
Sources:
- Node.js releases (v17 first released Oct 19, 2021). [1]
- Coverage/history noting structuredClone introduced in Node.js 17.0.0. [2]
Unify cloning approaches in state management.
Line 68 uses structuredClone() to handle circular references, but line 99 uses JSON.parse(JSON.stringify()) which will fail on circular data. Replace with structuredClone() for consistency:
Fix
- const updatedProject = JSON.parse(JSON.stringify(project)) as DeepnoteProject;
+ const updatedProject = structuredClone(project);🤖 Prompt for AI Agents
In `@src/notebooks/deepnote/deepnoteNotebookManager.ts` around lines 63 - 72, The
class mixes cloning strategies: storeOriginalProject uses structuredClone but
elsewhere in this class a JSON.parse(JSON.stringify(...)) is used (line ~99)
which breaks on circular data; replace that JSON.parse(JSON.stringify(...)) call
with structuredClone(...) so all stored project copies use structuredClone to
preserve circular references and prevent serialization errors (update the method
in this class that currently performs JSON.parse(JSON.stringify(...))
accordingly).
| // Log block IDs from source file | ||
| for (let i = 0; i < (selectedNotebook.blocks ?? []).length; i++) { | ||
| const block = selectedNotebook.blocks![i]; | ||
| logger.debug(`DeserializeNotebook: block[${i}] id=${block.id} from source file`); | ||
| } | ||
|
|
||
| let cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []); | ||
|
|
||
| logger.debug(`DeepnoteSerializer: Converted ${cells.length} cells from notebook blocks`); | ||
|
|
||
| // Log cell metadata.id after conversion | ||
| for (let i = 0; i < cells.length; i++) { | ||
| logger.debug(`DeserializeNotebook: cell[${i}] metadata.id=${cells[i].metadata?.id} after conversion`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Debug logging looks verbose for production.
Consider logger.trace instead of logger.debug for per-block iteration logging. Could flood logs in large notebooks.
🤖 Prompt for AI Agents
In `@src/notebooks/deepnote/deepnoteSerializer.ts` around lines 117 - 130, The
per-block and per-cell iteration logs in DeepnoteSerializer (inside the
DeserializeNotebook method) are too verbose for production; change the
logger.debug calls inside the two for-loops that log block IDs and cell metadata
IDs to logger.trace instead (leave the higher-level logger.debug that reports
total converted cells around convertBlocksToCells intact) so that
convertBlocksToCells, the block iteration loop, and the cell iteration loop
still log but only at trace level in production.
| // Log cell metadata IDs before conversion | ||
| for (let i = 0; i < data.cells.length; i++) { | ||
| const cell = data.cells[i]; | ||
| logger.debug( | ||
| `SerializeNotebook: cell[${i}] metadata.id=${cell.metadata?.id}, metadata keys=${ | ||
| cell.metadata ? Object.keys(cell.metadata).join(',') : 'none' | ||
| }` | ||
| ); | ||
| } | ||
|
|
||
| // Clone blocks while removing circular references that may have been | ||
| // introduced by VS Code's notebook cell/output handling | ||
| const blocks = this.converter.convertCellsToBlocks(data.cells); | ||
|
|
||
| logger.debug(`SerializeNotebook: Converted to ${blocks.length} blocks, now cloning without circular refs`); | ||
| logger.debug(`SerializeNotebook: Converted to ${blocks.length} blocks`); | ||
|
|
||
| // Try to recover block IDs from original blocks when VS Code fails to preserve metadata | ||
| // This uses content-based matching as a fallback when metadata.id is missing | ||
| this.recoverBlockIdsFromOriginal(blocks, notebook.blocks ?? []); | ||
|
|
||
| // Log block IDs after conversion and recovery | ||
| for (let i = 0; i < blocks.length; i++) { | ||
| logger.debug(`SerializeNotebook: block[${i}] id=${blocks[i].id}`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Per-item logging is noisy.
Same as deserialize path—consider logger.trace or conditional logging.
🤖 Prompt for AI Agents
In `@src/notebooks/deepnote/deepnoteSerializer.ts` around lines 232 - 255, The
per-item logger.debug calls in deepnoteSerializer (the two for-loops that log
cell metadata and the loop that logs block ids after
recoverBlockIdsFromOriginal) are too noisy; change them to logger.trace or wrap
them behind a level check (e.g., if logger.isTraceEnabled() or logger.level ===
'trace') so detailed per-item output only appears at trace level, leaving the
higher-level logger.debug calls (like the Converted to X blocks) unchanged.
| // Wait briefly for any pending cell state change events to be processed. | ||
| // This is needed because the queue completion event can fire before the | ||
| // last cell's Idle state change event has been processed (race condition). | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fixed delay is fragile.
50ms wait for pending events is a race condition workaround. Consider event-driven approach if issues arise.
🤖 Prompt for AI Agents
In `@src/notebooks/deepnote/snapshots/snapshotService.ts` around lines 635 - 638,
Replace the fragile fixed 50ms sleep with an event-driven wait: instead of using
new Promise(resolve => setTimeout(...)), subscribe to the cell/state change
completion event emitted by the runtime (or the queue completion event) and
resolve only when the specific cell's state transitions to Idle (or when the
queue reports the last cell processed). Modify the logic in snapshotService.ts
(the block around the awaiting of pending cell state change events) to register
a one-time listener for the cellStateChange/queueComplete event for the affected
cell(s) and await that promise, with a short fallback timeout as a last resort.
| assert.deepEqual(result.get('block-1'), [{ output_type: 'stream', text: '1' }]); | ||
| assert.deepEqual(result.get('block-2'), [{ output_type: 'stream', text: '2' }]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same deepStrictEqual note.
Apply throughout file for consistency.
🤖 Prompt for AI Agents
In `@src/notebooks/deepnote/snapshots/snapshotService.unit.test.ts` around lines
355 - 356, Replace all uses of assert.deepEqual with assert.deepStrictEqual in
the unit tests (e.g., the assertions comparing result.get('block-1') and
result.get('block-2') outputs), updating every occurrence in this test file so
comparisons use strict deep equality; keep the asserted values unchanged but
call assert.deepStrictEqual(...) instead of assert.deepEqual(...).
|
|
||
| logger.debug( | ||
| `[Pocket] addPocketToCellMetadata: output id=${cell.metadata.id}, pocket keys=${Object.keys(pocket).join(',')}}` | ||
| ); |
There was a problem hiding this comment.
Extra } in log message.
Line 64 ends with }} but should be }.
Proposed fix
logger.debug(
- `[Pocket] addPocketToCellMetadata: output id=${cell.metadata.id}, pocket keys=${Object.keys(pocket).join(',')}}`
+ `[Pocket] addPocketToCellMetadata: output id=${cell.metadata.id}, pocket keys=${Object.keys(pocket).join(',')}`
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.debug( | |
| `[Pocket] addPocketToCellMetadata: output id=${cell.metadata.id}, pocket keys=${Object.keys(pocket).join(',')}}` | |
| ); | |
| logger.debug( | |
| `[Pocket] addPocketToCellMetadata: output id=${cell.metadata.id}, pocket keys=${Object.keys(pocket).join(',')}` | |
| ); |
🤖 Prompt for AI Agents
In `@src/platform/deepnote/pocket.ts` around lines 62 - 65, The log string in
logger.debug inside addPocketToCellMetadata contains an extra closing brace
(`}}`) at the end of the formatted message; remove the stray `}` so the message
ends with a single `}`. Locate the logger.debug call that formats `[Pocket]
addPocketToCellMetadata: output id=${cell.metadata.id}, pocket
keys=${Object.keys(pocket).join(',')}}` and change it to end with
`...join(',')}` (i.e., drop the extra `}` after the join).
Adds snapshot mode for Deepnote notebooks, which separates cell outputs from notebook content for cleaner version control.
When snapshots are enabled (deepnote.snapshots.enabled):
Benefits:
Usage:
Implementation
snapshotService.ts: Unified service handling both file I/O (read/write snapshots, merge/strip outputs) and execution metadata tracking (timing, environment capture). Listens to cell execution events to automatically create snapshots on completion.environmentCapture.node.ts: Captures Python environment metadata (packages, versions) before execution starts for inclusion in snapshots.deepnoteSerializer.ts: Updated to strip outputs on save when snapshots are enabled, and merge outputs from snapshots when opening notebooks.Summary by CodeRabbit
Release Notes
New Features
deepnote.snapshots.enabledconfiguration option to control snapshot mode.Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.