feat: Create snapshots when running notebooks #256
Conversation
📝 WalkthroughWalkthroughThis PR adds snapshot support for Deepnote notebooks: a resource-scoped config Sequence Diagram(s)sequenceDiagram
participant User
participant CellExecutionQueue
participant SnapshotMetadataService
participant DeepnoteDataConverter
participant SnapshotFileService
participant FileSystem
User->>CellExecutionQueue: Execute notebook cells
Note over CellExecutionQueue: Execution completes
CellExecutionQueue->>SnapshotMetadataService: onExecutionComplete(notebookUri)
SnapshotMetadataService->>DeepnoteDataConverter: convert notebook cells -> blocks
DeepnoteDataConverter-->>SnapshotMetadataService: Deepnote blocks
alt Run-All mode
SnapshotMetadataService->>SnapshotFileService: createSnapshot(projectUri, projectData)
else Partial run
SnapshotMetadataService->>SnapshotFileService: updateLatestSnapshot(projectUri, projectData)
end
SnapshotFileService->>FileSystem: write snapshot files (timestamped + latest / latest)
FileSystem-->>SnapshotFileService: write result
SnapshotFileService-->>SnapshotMetadataService: return Uri
sequenceDiagram
participant User
participant DeepnoteNotebookSerializer
participant SnapshotFileService
participant FileSystem
participant Notebook
User->>DeepnoteNotebookSerializer: Open/load notebook file
DeepnoteNotebookSerializer->>SnapshotFileService: readSnapshot(projectId)
SnapshotFileService->>FileSystem: read latest snapshot file
FileSystem-->>SnapshotFileService: snapshot content (YAML)
SnapshotFileService-->>DeepnoteNotebookSerializer: Map<blockId, outputs>
DeepnoteNotebookSerializer->>DeepnoteNotebookSerializer: mergeOutputsIntoBlocks(blocks, outputs)
DeepnoteNotebookSerializer->>Notebook: provide cells with merged outputs
Notebook-->>User: display notebook
🚥 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.
| const snapshotProject = structuredClone(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
Inconsistent cloning approach.
Line 224 uses structuredClone, line 230 uses cloneWithoutCircularRefs, line 251/255 also uses cloneWithoutCircularRefs. Consider consistent approach—cloneWithoutCircularRefs exists to handle VS Code's circular refs in outputs.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteSerializer.ts around lines 224 to 256, the
code inconsistently uses structuredClone at line 224 but uses
cloneWithoutCircularRefs at lines 230, 251 and 255; replace the structuredClone
call with cloneWithoutCircularRefs when creating snapshotProject so the snapshot
and all subsequent notebook.blocks assignments consistently use
cloneWithoutCircularRefs (ensuring circular refs in VS Code outputs are
handled), and keep the existing uses at the fallback and default branches
unchanged.
| 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.
| suite('SnapshotFileService', () => { | ||
| let service: SnapshotFileService; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| let serviceAny: any; | ||
|
|
||
| setup(() => { | ||
| service = new SnapshotFileService(); | ||
| serviceAny = service; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing tests for async methods.
No coverage for isSnapshotsEnabled(), readSnapshot(), or createSnapshot(). These are core public methods that handle I/O and configuration.
Want me to generate test stubs for these async methods?
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.
| /** | ||
| * Slugifies a project name for use in filenames. | ||
| * Converts to lowercase, replaces spaces with hyphens, removes non-alphanumeric chars. | ||
| * @throws Error if the result is empty after transformation | ||
| */ | ||
| function slugifyProjectName(name: string): string { | ||
| if (typeof name !== 'string' || !name.trim()) { | ||
| throw new InvalidProjectNameError(); | ||
| } | ||
|
|
||
| const slug = name | ||
| .toLowerCase() | ||
| .replace(/\s+/g, '-') | ||
| .replace(/[^a-z0-9-]/g, '') | ||
| .replace(/-+/g, '-') | ||
| .replace(/^-|-$/g, ''); | ||
|
|
||
| if (!slug) { | ||
| throw new InvalidProjectNameError(); | ||
| } | ||
|
|
||
| return slug; | ||
| } |
There was a problem hiding this comment.
Blocker: InvalidProjectNameError can escape and break snapshot/save flows.
Slugification failures should be handled locally (log + skip), per the error’s docstring intent.
Proposed diff
private async prepareSnapshotData(
projectUri: Uri,
projectId: string,
projectName: string,
projectData: DeepnoteFile
): Promise<{ latestPath: Uri; content: Uint8Array } | undefined> {
- const latestPath = this.buildSnapshotPath(projectUri, projectId, projectName, 'latest');
+ let latestPath: Uri;
+ try {
+ latestPath = this.buildSnapshotPath(projectUri, projectId, projectName, 'latest');
+ } catch (error) {
+ if (error instanceof InvalidProjectNameError) {
+ logger.warn('[SnapshotFileService] Skipping snapshots due to invalid project name', error);
+ return undefined;
+ }
+ throw error;
+ }
const snapshotsDir = Uri.joinPath(latestPath, '..');Also applies to: 69-80, 253-292, 306-387
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshotFileService.node.ts around lines 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.
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.
| const files = await workspace.findFiles(timestampedPattern, null, 100); | ||
|
|
||
| allTimestampedFiles = allTimestampedFiles.concat(files); | ||
| } | ||
|
|
||
| // Filter out 'latest' files (in case glob matched them) and sort by filename descending | ||
| const sortedFiles = allTimestampedFiles | ||
| .filter((uri) => !uri.fsPath.includes('_latest.')) | ||
| .sort((a, b) => { | ||
| // Sort by filename descending (timestamps sort lexicographically) | ||
| const nameA = Utils.basename(a); | ||
| const nameB = Utils.basename(b); | ||
|
|
||
| return nameB.localeCompare(nameA); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider performance implications of searching and sorting many snapshots.
The current limit of 100 timestamped snapshots per workspace folder may be insufficient for long-running projects. Additionally, sorting all discovered snapshots could be slow if many exist across workspace folders.
Consider whether a smaller limit or pagination is appropriate, or document expected snapshot retention policies.
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshotFileService.node.ts around lines 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.
| const sortedFiles = allTimestampedFiles | ||
| .filter((uri) => !uri.fsPath.includes('_latest.')) | ||
| .sort((a, b) => { | ||
| // Sort by filename descending (timestamps sort lexicographically) | ||
| const nameA = Utils.basename(a); | ||
| const nameB = Utils.basename(b); | ||
|
|
||
| return nameB.localeCompare(nameA); | ||
| }); |
There was a problem hiding this comment.
Fix fragile filter logic.
The filter !uri.fsPath.includes('_latest.') will incorrectly exclude snapshots for projects whose slugified name contains "_latest". For example, a project named "my_latest_analysis" would be filtered out.
Check the variant portion of the filename instead of using a simple string search.
🐛 Proposed fix
const sortedFiles = allTimestampedFiles
- .filter((uri) => !uri.fsPath.includes('_latest.'))
+ .filter((uri) => {
+ const basename = Utils.basename(uri);
+ // Match: {slug}_{projectId}_{variant}.snapshot.deepnote
+ // Exclude if variant is 'latest'
+ return !basename.endsWith('_latest.snapshot.deepnote');
+ })
.sort((a, b) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sortedFiles = allTimestampedFiles | |
| .filter((uri) => !uri.fsPath.includes('_latest.')) | |
| .sort((a, b) => { | |
| // Sort by filename descending (timestamps sort lexicographically) | |
| const nameA = Utils.basename(a); | |
| const nameB = Utils.basename(b); | |
| return nameB.localeCompare(nameA); | |
| }); | |
| const sortedFiles = allTimestampedFiles | |
| .filter((uri) => { | |
| const basename = Utils.basename(uri); | |
| // Match: {slug}_{projectId}_{variant}.snapshot.deepnote | |
| // Exclude if variant is 'latest' | |
| return !basename.endsWith('_latest.snapshot.deepnote'); | |
| }) | |
| .sort((a, b) => { | |
| // Sort by filename descending (timestamps sort lexicographically) | |
| const nameA = Utils.basename(a); | |
| const nameB = Utils.basename(b); | |
| return nameB.localeCompare(nameA); | |
| }); |
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshotFileService.node.ts around lines 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.
| for (const block of blocks) { | ||
| if ( | ||
| typeof block === 'object' && | ||
| block !== null && | ||
| typeof block.id === 'string' && | ||
| Array.isArray(block.outputs) | ||
| ) { | ||
| outputsMap.set(block.id, block.outputs as DeepnoteOutput[]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate output structure before casting.
The code verifies Array.isArray(block.outputs) but casts to DeepnoteOutput[] without validating the array contents. Corrupted snapshot files could contain malformed outputs, causing runtime errors downstream.
Consider adding validation or at minimum document the trust assumption.
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshotFileService.node.ts around lines 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.
| snapshotData.metadata = snapshotData.metadata || { createdAt: new Date().toISOString() }; | ||
| snapshotData.metadata.modifiedAt = new Date().toISOString(); |
There was a problem hiding this comment.
Ensure consistent metadata initialization.
If metadata already exists but lacks createdAt, it won't be initialized. This could lead to inconsistent metadata across snapshots.
🔧 Suggested fix
snapshotData.metadata = snapshotData.metadata || { createdAt: new Date().toISOString() };
+if (!snapshotData.metadata.createdAt) {
+ snapshotData.metadata.createdAt = new Date().toISOString();
+}
snapshotData.metadata.modifiedAt = new Date().toISOString();🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshotFileService.node.ts around lines 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.
| private async hasSnapshotChanges(latestPath: Uri, projectData: DeepnoteFile): Promise<boolean> { | ||
| try { | ||
| const existingContent = await workspace.fs.readFile(latestPath); | ||
| const existingString = new TextDecoder('utf-8').decode(existingContent); | ||
| const existingData = yaml.load(existingString) as DeepnoteFile; | ||
|
|
||
| // Compare the project content (notebooks and their blocks) | ||
| // We exclude metadata since timestamps always change | ||
| const existingProject = this.getComparableProjectContent(existingData); | ||
| const newProject = this.getComparableProjectContent(projectData); | ||
|
|
||
| return !fastDeepEqual(existingProject, newProject); | ||
| } catch { | ||
| // If we can't read the existing snapshot, treat it as having changes | ||
| logger.debug(`[SnapshotFileService] No existing snapshot found, treating as changed`); | ||
|
|
||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling in change detection.
The catch block treats all errors (file not found, permission denied, parsing failures) as "has changes". This masks real errors that should be visible.
Log errors at warn/error level, and consider differentiating between "file doesn't exist" (expected) vs other failures (unexpected).
🔧 Suggested improvement
} catch {
- // If we can't read the existing snapshot, treat it as having changes
- logger.debug(`[SnapshotFileService] No existing snapshot found, treating as changed`);
+ } catch (error) {
+ if (error && typeof error === 'object' && 'code' in error && error.code === 'FileNotFound') {
+ logger.debug(`[SnapshotFileService] No existing snapshot found, treating as changed`);
+ } else {
+ logger.warn(`[SnapshotFileService] Error reading existing snapshot, treating as changed`, error);
+ }
return true;
}Committable suggestion skipped: line range outside the PR's diff.
| mergeOutputsIntoBlocks(blocks: DeepnoteBlock[], outputs: Map<string, DeepnoteOutput[]>): void { | ||
| let mergedCount = 0; | ||
|
|
||
| for (const block of blocks) { | ||
| if (!block.id) { | ||
| continue; | ||
| } | ||
|
|
||
| const blockOutputs = outputs.get(block.id); | ||
|
|
||
| if (blockOutputs !== undefined) { | ||
| block.outputs = blockOutputs; | ||
| mergedCount++; | ||
| } | ||
| } | ||
|
|
||
| logger.debug(`[SnapshotFileService] Merged outputs into ${mergedCount}/${blocks.length} blocks`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider immutable approach or clearer mutation signaling.
The method mutates blocks in place, which is documented but could surprise callers since it's part of the public API. Consider either:
- Returning a new array (consistent with
stripOutputsFromBlocks) - Renaming to signal mutation (e.g.,
mergeOutputsIntoBlocksInPlace) - Adding a JSDoc
@mutatesannotation
| extractOutputsFromBlocks(blocks: DeepnoteBlock[]): Map<string, DeepnoteOutput[]> { | ||
| const outputsMap = new Map<string, DeepnoteOutput[]>(); | ||
|
|
||
| for (const block of blocks) { | ||
| if (block.id && block.outputs) { | ||
| outputsMap.set(block.id, block.outputs as DeepnoteOutput[]); | ||
| } | ||
| } | ||
|
|
||
| return outputsMap; | ||
| } |
There was a problem hiding this comment.
Validate output types before casting.
Similar to the issue in parseSnapshotFile, this casts block.outputs to DeepnoteOutput[] without validation. Malformed block data could cause runtime errors.
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/snapshotFileService.node.ts around lines 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).
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.
| private async disableSnapshots(): Promise<void> { | ||
| try { | ||
| await this.configurationService.updateSetting( | ||
| 'snapshots.enabled', | ||
| false, | ||
| undefined, | ||
| ConfigurationTarget.Workspace | ||
| ); | ||
| void window.showInformationMessage(l10n.t('Snapshots disabled for this workspace.')); | ||
| } catch (error) { | ||
| logger.error('Failed to disable snapshots', error); | ||
| void window.showErrorMessage(l10n.t('Failed to disable snapshots.')); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Setting key mismatch.
Setting key 'snapshots.enabled' on line 501 doesn't match the configuration property 'deepnote.snapshots.enabled' defined in package.json (line 1677). This will fail to update the correct setting.
🐛 Proposed fix
await this.configurationService.updateSetting(
- 'snapshots.enabled',
+ 'deepnote.snapshots.enabled',
false,
undefined,
ConfigurationTarget.Workspace
);📝 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.
| private async disableSnapshots(): Promise<void> { | |
| try { | |
| await this.configurationService.updateSetting( | |
| 'snapshots.enabled', | |
| false, | |
| undefined, | |
| ConfigurationTarget.Workspace | |
| ); | |
| void window.showInformationMessage(l10n.t('Snapshots disabled for this workspace.')); | |
| } catch (error) { | |
| logger.error('Failed to disable snapshots', error); | |
| void window.showErrorMessage(l10n.t('Failed to disable snapshots.')); | |
| } | |
| } | |
| private async disableSnapshots(): Promise<void> { | |
| try { | |
| await this.configurationService.updateSetting( | |
| 'deepnote.snapshots.enabled', | |
| false, | |
| undefined, | |
| ConfigurationTarget.Workspace | |
| ); | |
| void window.showInformationMessage(l10n.t('Snapshots disabled for this workspace.')); | |
| } catch (error) { | |
| logger.error('Failed to disable snapshots', error); | |
| void window.showErrorMessage(l10n.t('Failed to disable snapshots.')); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/deepnoteNotebookCommandListener.ts around lines 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.
| private async enableSnapshots(): Promise<void> { | ||
| try { | ||
| await this.configurationService.updateSetting( | ||
| 'snapshots.enabled', | ||
| true, | ||
| undefined, | ||
| ConfigurationTarget.Workspace | ||
| ); | ||
| void window.showInformationMessage(l10n.t('Snapshots enabled for this workspace.')); | ||
| } catch (error) { | ||
| logger.error('Failed to enable snapshots', error); | ||
| void window.showErrorMessage(l10n.t('Failed to enable snapshots.')); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Setting key mismatch.
Same issue as disableSnapshots: setting key 'snapshots.enabled' on line 516 should be 'deepnote.snapshots.enabled' to match package.json configuration.
🐛 Proposed fix
await this.configurationService.updateSetting(
- 'snapshots.enabled',
+ 'deepnote.snapshots.enabled',
true,
undefined,
ConfigurationTarget.Workspace
);📝 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.
| private async enableSnapshots(): Promise<void> { | |
| try { | |
| await this.configurationService.updateSetting( | |
| 'snapshots.enabled', | |
| true, | |
| undefined, | |
| ConfigurationTarget.Workspace | |
| ); | |
| void window.showInformationMessage(l10n.t('Snapshots enabled for this workspace.')); | |
| } catch (error) { | |
| logger.error('Failed to enable snapshots', error); | |
| void window.showErrorMessage(l10n.t('Failed to enable snapshots.')); | |
| } | |
| } | |
| private async enableSnapshots(): Promise<void> { | |
| try { | |
| await this.configurationService.updateSetting( | |
| 'deepnote.snapshots.enabled', | |
| true, | |
| undefined, | |
| ConfigurationTarget.Workspace | |
| ); | |
| void window.showInformationMessage(l10n.t('Snapshots enabled for this workspace.')); | |
| } catch (error) { | |
| logger.error('Failed to enable snapshots', error); | |
| void window.showErrorMessage(l10n.t('Failed to enable snapshots.')); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/notebooks/deepnote/deepnoteNotebookCommandListener.ts around lines 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.
Adds snapshot mode for Deepnote notebooks, which separates cell outputs from notebook content for cleaner version control.
When snapshots are enabled:
snapshots/{project}_{id}_latest.snapshot.deepnote_2025-01-02T14-30-00.snapshot.deepnote)Benefits:
.deepnotefile only changes when you modify code or markdown—not when outputs changeUsage
Deepnote: Enable Snapshotsfrom the command paletteDeepnote: Disable Snapshotsdeepnote.snapshots.enabled(workspace-scoped)Summary by CodeRabbit
New Features
Behavior
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.