-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Create snapshots when running notebooks #256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2fc0363
e4ee510
dd98a9d
d990d31
8f50413
a1e62fc
b284191
6075e3c
aed189f
116d95e
602d01f
ddb5640
51006bd
4f66e11
fda8eb3
c8761e4
9de7811
19d530f
fafbabc
f8b7c79
15a1de5
990ba07
fb2bd10
7536771
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,8 @@ import { CodeExecution } from './codeExecution'; | |
| import { once } from '../../platform/common/utils/events'; | ||
| import { getCellMetadata } from '../../platform/common/utils'; | ||
| import { NotebookCellExecutionState, notebookCellExecutions } from '../../platform/notebooks/cellExecutionStateService'; | ||
| import { ISnapshotMetadataService } from '../../platform/notebooks/deepnote/types'; | ||
| // eslint-disable-next-line import/no-restricted-paths | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a question: why is this a restricted path?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These rules are from the original repo and I'm not sure why its restricted. |
||
| import { ISnapshotMetadataService } from '../../notebooks/deepnote/snapshots/snapshotService'; | ||
Artmann marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * A queue responsible for execution of cells. | ||
|
|
@@ -324,5 +325,10 @@ export class CellExecutionQueue implements Disposable { | |
| break; | ||
| } | ||
| } | ||
|
|
||
| // Notify listeners that execution queue is complete | ||
| if (this.notebook) { | ||
| notebookCellExecutions.notifyQueueComplete(this.notebook.uri.toString()); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,8 +11,8 @@ import type { DeepnoteProject } from '../../platform/deepnote/deepnoteTypes'; | |
| export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { | ||
| private readonly currentNotebookId = new Map<string, string>(); | ||
| private readonly originalProjects = new Map<string, DeepnoteProject>(); | ||
| private readonly selectedNotebookByProject = new Map<string, string>(); | ||
| private readonly projectsWithInitNotebookRun = new Set<string>(); | ||
| private readonly selectedNotebookByProject = new Map<string, string>(); | ||
|
|
||
| /** | ||
| * Gets the currently selected notebook ID for a project. | ||
|
|
@@ -61,7 +61,13 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { | |
| * @param notebookId Initial notebook ID to set as current | ||
| */ | ||
| storeOriginalProject(projectId: string, project: DeepnoteProject, notebookId: string): void { | ||
| this.originalProjects.set(projectId, project); | ||
| // Deep clone to prevent mutations from affecting stored state | ||
| // This is critical for multi-notebook projects where multiple notebooks | ||
| // share the same stored project reference | ||
| // Using structuredClone to handle circular references (e.g., in output metadata) | ||
| const clonedProject = structuredClone(project); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change unrelated to snapshots? Or is it to fix issues that arose while introducing them?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was an existing bug that I found while testing the snapshots. |
||
|
|
||
| this.originalProjects.set(projectId, clonedProject); | ||
| this.currentNotebookId.set(projectId, notebookId); | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
@@ -90,7 +96,7 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { | |
| return false; | ||
| } | ||
|
|
||
| const updatedProject = JSON.parse(JSON.stringify(project)) as DeepnoteProject; | ||
| const updatedProject = structuredClone(project); | ||
| updatedProject.project.integrations = integrations; | ||
|
|
||
| const currentNotebookId = this.currentNotebookId.get(projectId); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.