feat: Snapshot mode #256
feat: Snapshot mode #256
Conversation
📝 WalkthroughWalkthroughThis change introduces snapshot functionality for Deepnote notebooks. Two new VS Code commands allow users to enable or disable snapshot mode. When enabled, a new configuration flag activates snapshot-aware serialization and deserialization. Outputs are persisted to separate snapshot files in a snapshots folder, while the main notebook file stores only cell content. The SnapshotFileService manages snapshot file I/O, including reading latest snapshots, detecting changes, and writing timestamped snapshots. Sequence Diagram(s)sequenceDiagram
participant User
participant CommandListener as Command Listener
participant ConfigService as Configuration Service
participant UserNotification as VS Code UI
User->>CommandListener: Execute enableSnapshots/disableSnapshots command
CommandListener->>ConfigService: updateSetting(snapshots.enabled, true/false)
ConfigService->>ConfigService: Persist setting to workspace
CommandListener->>UserNotification: Show info message
UserNotification->>User: Confirm snapshots enabled/disabled
sequenceDiagram
participant Serializer as Notebook Serializer
participant SnapshotFileService as Snapshot File Service
participant FileSystem
rect rgb(200, 220, 240)
Note over Serializer,FileSystem: Deserialization (Snapshots Enabled)
Serializer->>SnapshotFileService: readSnapshot(projectId)
SnapshotFileService->>FileSystem: Read latest snapshot file
FileSystem-->>SnapshotFileService: YAML content
SnapshotFileService->>SnapshotFileService: Parse outputs into Map
SnapshotFileService-->>Serializer: Map<blockId, outputs>
Serializer->>Serializer: Merge snapshot outputs into cells
end
sequenceDiagram
participant Serializer as Notebook Serializer
participant SnapshotFileService as Snapshot File Service
participant FileSystem
rect rgb(240, 220, 200)
Note over Serializer,FileSystem: Serialization (Snapshots Enabled)
Serializer->>SnapshotFileService: createSnapshot(projectUri, projectId, projectName, projectData)
SnapshotFileService->>FileSystem: Check latest snapshot exists
SnapshotFileService->>SnapshotFileService: Compare current vs latest (exclude metadata)
alt Changes Detected
SnapshotFileService->>FileSystem: Write timestamped snapshot file
SnapshotFileService->>FileSystem: Update latest snapshot file
end
SnapshotFileService-->>Serializer: Snapshot Uri or undefined
Serializer->>Serializer: Strip outputs from main notebook blocks
end
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
| { | ||
| "command": "deepnote.enableSnapshots", | ||
| "title": "Enable Snapshots", | ||
| "category": "Deepnote" | ||
| }, | ||
| { | ||
| "command": "deepnote.disableSnapshots", | ||
| "title": "Disable Snapshots", | ||
| "category": "Deepnote" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing l10n placeholders for command titles.
Other commands use %deepnote.commands.xxx.title% placeholders. These hardcoded strings break localization consistency.
{
"command": "deepnote.enableSnapshots",
- "title": "Enable Snapshots",
+ "title": "%deepnote.commands.enableSnapshots.title%",
"category": "Deepnote"
},
{
"command": "deepnote.disableSnapshots",
- "title": "Disable Snapshots",
+ "title": "%deepnote.commands.disableSnapshots.title%",
"category": "Deepnote"
}📝 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.
| { | |
| "command": "deepnote.enableSnapshots", | |
| "title": "Enable Snapshots", | |
| "category": "Deepnote" | |
| }, | |
| { | |
| "command": "deepnote.disableSnapshots", | |
| "title": "Disable Snapshots", | |
| "category": "Deepnote" | |
| }, | |
| { | |
| "command": "deepnote.enableSnapshots", | |
| "title": "%deepnote.commands.enableSnapshots.title%", | |
| "category": "Deepnote" | |
| }, | |
| { | |
| "command": "deepnote.disableSnapshots", | |
| "title": "%deepnote.commands.disableSnapshots.title%", | |
| "category": "Deepnote" | |
| }, |
🤖 Prompt for AI Agents
In package.json around lines 102 to 111, the command titles are hardcoded
("Enable Snapshots" / "Disable Snapshots") which breaks localization; replace
those title values with the existing localization placeholders (e.g.
"%deepnote.commands.enableSnapshots.title%" and
"%deepnote.commands.disableSnapshots.title%") and ensure corresponding keys are
present in the package.nls.json (add them if missing) so the commands use l10n
strings consistently across the project.
| import { IIntegrationManager } from './integrations/types'; | ||
| import { DeepnoteInputBlockEditProtection } from './deepnoteInputBlockEditProtection'; | ||
| import { ISnapshotMetadataService, ISnapshotMetadataServiceFull } from './snapshotMetadataService'; | ||
| import { ISnapshotFileService } from './snapshotFileService.node'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider importing interface from types file.
ISnapshotFileService is defined in snapshotFileServiceTypes.ts. Importing the interface directly from its definition file improves clarity and follows the separation between types and implementation.
🔎 Suggested change
-import { ISnapshotFileService } from './snapshotFileService.node';
+import { ISnapshotFileService } from './snapshotFileServiceTypes';📝 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.
| import { ISnapshotFileService } from './snapshotFileService.node'; | |
| import { ISnapshotFileService } from './snapshotFileServiceTypes'; |
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteActivationService.ts around line 13, the code
imports ISnapshotFileService from './snapshotFileService.node' but the interface
is defined in snapshotFileServiceTypes.ts; change the import to reference the
types file (e.g., './snapshotFileServiceTypes') so the file imports the
interface from its canonical types definition, update any import statements
accordingly and remove or keep the implementation import only where concrete
implementations are needed.
| private async disableSnapshots(): Promise<void> { | ||
| await this.configurationService.updateSetting( | ||
| 'snapshots.enabled', | ||
| false, | ||
| undefined, | ||
| ConfigurationTarget.Workspace | ||
| ); | ||
| void window.showInformationMessage(l10n.t('Snapshots disabled for this workspace.')); | ||
| } | ||
|
|
||
| private async enableSnapshots(): Promise<void> { | ||
| await this.configurationService.updateSetting( | ||
| 'snapshots.enabled', | ||
| true, | ||
| undefined, | ||
| ConfigurationTarget.Workspace | ||
| ); | ||
| void window.showInformationMessage(l10n.t('Snapshots enabled for this workspace.')); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider error handling for setting update.
updateSetting can fail. Silent failure may leave user confused.
Suggested improvement
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.'));
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteNotebookCommandListener.ts around lines 380 to
398, the calls to this.configurationService.updateSetting(...) are unguarded and
can fail silently; wrap each updateSetting call in a try/catch, await the call
inside the try, and on error call window.showErrorMessage with a user-friendly
message that includes the error (e.g., "Failed to enable/disable snapshots:
<error.message>") and log the error via the class logger/telemetry before
returning so the user is informed and the failure is recorded.
| // Merge outputs from snapshot if snapshots are enabled | ||
| if (this.snapshotFileService?.isSnapshotsEnabled()) { | ||
| const snapshotOutputs = await this.snapshotFileService.readSnapshot(projectId); | ||
|
|
||
| if (snapshotOutputs) { | ||
| logger.debug(`DeepnoteSerializer: Merging ${snapshotOutputs.size} outputs from snapshot`); | ||
| this.snapshotFileService.mergeOutputsIntoBlocks(selectedNotebook.blocks ?? [], snapshotOutputs); | ||
|
|
||
| cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []); | ||
| } | ||
| } |
There was a problem hiding this comment.
Mutates original notebook blocks in place.
Line 129 modifies selectedNotebook.blocks directly. Since storeOriginalProject is called after (line 135), the "original" project now contains merged snapshot outputs, corrupting the baseline for change detection.
Suggested fix
if (this.snapshotFileService?.isSnapshotsEnabled()) {
const snapshotOutputs = await this.snapshotFileService.readSnapshot(projectId);
if (snapshotOutputs) {
logger.debug(`DeepnoteSerializer: Merging ${snapshotOutputs.size} outputs from snapshot`);
- this.snapshotFileService.mergeOutputsIntoBlocks(selectedNotebook.blocks ?? [], snapshotOutputs);
-
- cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []);
+ const blocksWithOutputs = structuredClone(selectedNotebook.blocks ?? []);
+ this.snapshotFileService.mergeOutputsIntoBlocks(blocksWithOutputs, snapshotOutputs);
+ cells = this.converter.convertBlocksToCells(blocksWithOutputs);
}
}📝 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.
| // Merge outputs from snapshot if snapshots are enabled | |
| if (this.snapshotFileService?.isSnapshotsEnabled()) { | |
| const snapshotOutputs = await this.snapshotFileService.readSnapshot(projectId); | |
| if (snapshotOutputs) { | |
| logger.debug(`DeepnoteSerializer: Merging ${snapshotOutputs.size} outputs from snapshot`); | |
| this.snapshotFileService.mergeOutputsIntoBlocks(selectedNotebook.blocks ?? [], snapshotOutputs); | |
| cells = this.converter.convertBlocksToCells(selectedNotebook.blocks ?? []); | |
| } | |
| } | |
| // Merge outputs from snapshot if snapshots are enabled | |
| if (this.snapshotFileService?.isSnapshotsEnabled()) { | |
| const snapshotOutputs = await this.snapshotFileService.readSnapshot(projectId); | |
| if (snapshotOutputs) { | |
| logger.debug(`DeepnoteSerializer: Merging ${snapshotOutputs.size} outputs from snapshot`); | |
| const blocksWithOutputs = structuredClone(selectedNotebook.blocks ?? []); | |
| this.snapshotFileService.mergeOutputsIntoBlocks(blocksWithOutputs, snapshotOutputs); | |
| cells = this.converter.convertBlocksToCells(blocksWithOutputs); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/notebooks/deepnote/deepnoteSerializer.ts around lines 123-133, the code
calls mergeOutputsIntoBlocks which mutates selectedNotebook.blocks in place,
corrupting the original project before storeOriginalProject runs; to fix, create
a shallow/deep copy of selectedNotebook.blocks (e.g., clone array and block
objects) and pass that copy into mergeOutputsIntoBlocks and convertBlocksToCells
so the original selectedNotebook remains unchanged, then proceed using the
copied/merged blocks for conversion and downstream work.
| 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.
| .sort((a, b) => { | ||
| // Sort by filename descending (timestamps sort lexicographically) | ||
| const nameA = a.fsPath.split('/').pop() || ''; | ||
| const nameB = b.fsPath.split('/').pop() || ''; | ||
|
|
||
| return nameB.localeCompare(nameA); | ||
| }); |
There was a problem hiding this comment.
Path splitting not cross-platform.
split('/') fails on Windows. Use path.basename() or Uri.Utils.basename().
+import { Utils } from 'vscode-uri';
+
const sortedFiles = timestampedFiles
.filter((uri) => !uri.fsPath.includes('_latest.'))
.sort((a, b) => {
- const nameA = a.fsPath.split('/').pop() || '';
- const nameB = b.fsPath.split('/').pop() || '';
+ const nameA = Utils.basename(a);
+ const nameB = Utils.basename(b);
return nameB.localeCompare(nameA);
});📝 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.
| .sort((a, b) => { | |
| // Sort by filename descending (timestamps sort lexicographically) | |
| const nameA = a.fsPath.split('/').pop() || ''; | |
| const nameB = b.fsPath.split('/').pop() || ''; | |
| return nameB.localeCompare(nameA); | |
| }); | |
| import { basename } from 'path'; | |
| .sort((a, b) => { | |
| // Sort by filename descending (timestamps sort lexicographically) | |
| const nameA = basename(a.fsPath); | |
| const nameB = basename(b.fsPath); | |
| return nameB.localeCompare(nameA); | |
| }); |
🤖 Prompt for AI Agents
In src/notebooks/deepnote/snapshotFileService.node.ts around lines 94 to 100,
the code uses string.split('/') to get the filename which breaks on Windows;
replace that logic with a cross-platform basename call (e.g. use Node's
path.basename or the project's Uri.Utils.basename). Update the two occurrences
to compute nameA/nameB via path.basename(a.fsPath) and path.basename(b.fsPath)
(or the equivalent Uri util), and add the necessary import (import path from
'path' or the appropriate Uri utils import) so sorting remains by filename
descending.
| 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.
| mergeOutputsIntoBlocks(blocks: DeepnoteBlock[], outputs: Map<string, DeepnoteOutput[]>): void { | ||
| for (const block of blocks) { | ||
| if (block.id && outputs.has(block.id)) { | ||
| block.outputs = outputs.get(block.id); | ||
| } | ||
| } | ||
|
|
||
| logger.debug(`[SnapshotFileService] Merged outputs into ${blocks.length} blocks`); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Log message misleading.
"Merged outputs into X blocks" logs total block count, not how many actually received outputs.
+let mergedCount = 0;
for (const block of blocks) {
if (block.id && outputs.has(block.id)) {
block.outputs = outputs.get(block.id);
+ mergedCount++;
}
}
-logger.debug(`[SnapshotFileService] Merged outputs into ${blocks.length} blocks`);
+logger.debug(`[SnapshotFileService] Merged outputs into ${mergedCount}/${blocks.length} blocks`);🤖 Prompt for AI Agents
In src/notebooks/deepnote/snapshotFileService.node.ts around lines 243 to 251,
the debug message currently logs the total number of blocks instead of the
number that actually had outputs merged; change the implementation to count how
many blocks were updated (e.g., increment a counter or collect matches when
outputs.has(block.id) and assignment occurs) and then log the actual merged
count (optionally include the total blocks for context), making sure to handle
outputs.get(block.id) safely when assigning.
| stripOutputsFromBlocks(blocks: DeepnoteBlock[]): DeepnoteBlock[] { | ||
| return blocks.map((block) => { | ||
| const strippedBlock = { ...block }; | ||
|
|
||
| delete strippedBlock.outputs; | ||
|
|
||
| return strippedBlock; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Shallow copy keeps nested references.
If original block has outputs and other nested objects, those remain shared. Use deep clone or explicitly handle.
stripOutputsFromBlocks(blocks: DeepnoteBlock[]): DeepnoteBlock[] {
return blocks.map((block) => {
- const strippedBlock = { ...block };
+ const { outputs, ...strippedBlock } = block;
- delete strippedBlock.outputs;
return strippedBlock;
});
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/notebooks/deepnote/snapshotFileService.node.ts around lines 257 to 265,
the current implementation uses a shallow spread to copy blocks so nested
objects (including outputs) remain shared; change to create a deep clone of each
block (for example using structuredClone if available,
JSON.parse(JSON.stringify(block)) where acceptable, or lodash.cloneDeep) then
delete the outputs property from the cloned object before returning it,
preserving typings and handling potential clone errors or non-serializable
values as needed.
| 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
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)