-
Notifications
You must be signed in to change notification settings - Fork 0
fix: re-add notebook switching #5
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
Conversation
Signed-off-by: Andy Jakubowski <[email protected]>
Signed-off-by: Andy Jakubowski <[email protected]>
GRN-4910 Re-add notebook switching
Need to redo this after switching from Python to TypeScript conversion of .deepnote <> .ipynb |
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant NP as NotebookPicker
participant Z as deepnoteMetadataSchema
participant M as Notebook Model
U->>NP: Select notebook (handleChange)
NP->>Z: safeParse(metadata.deepnote)
alt Metadata invalid
Z-->>NP: failure
NP->>NP: log error, abort
else Metadata valid
Z-->>NP: success (notebooks map)
NP->>NP: lookup selected in notebooks
alt Selected exists
NP->>M: fromJSON({ cells, nbformat, nbformat_minor, metadata.deepnote.notebooks })
else No selection
NP->>NP: no-op
end
end
sequenceDiagram
autonumber
participant NP as NotebookPicker (render)
participant Z as deepnoteMetadataSchema
NP->>Z: safeParse(metadata.deepnote)
alt Valid
Z-->>NP: notebooks map
NP->>NP: derive names for UI list
else Invalid
Z-->>NP: failure
NP->>NP: render empty list
end
sequenceDiagram
autonumber
participant S as Server (Deepnote YAML)
participant CP as deepnote-content-provider
participant SZ as deepnoteFileFromServerSchema
participant T as transform-deepnote-yaml-to-notebook-content
participant M as Notebook Model
S-->>CP: content (YAML-derived JSON)
CP->>SZ: safeParse(content)
alt Invalid content
SZ-->>CP: failure
CP->>M: set model.content.cells = []
CP-->>M: return model
else Valid content
SZ-->>CP: validatedModelContent
CP->>T: transform(validatedModelContent)
T-->>CP: { cells, metadata.deepnote.notebooks, nbformat, nbformat_minor }
CP->>M: update model.content
end
sequenceDiagram
autonumber
participant T as Transformer
participant F as deepnoteFile
participant C as Converter
T->>F: read project.notebooks[]
loop Build notebooks map
T->>C: convertDeepnoteBlockToJupyterCell(block)
C-->>T: cell
T->>T: accumulate { id, name, cells[] } by notebook.id
end
alt Selected notebook found
T-->>T: prepare cells from selected
T-->>Caller: { cells, metadata.deepnote.notebooks, nbformat, nbformat_minor }
else None found
T-->>Caller: { cells:[code cell "No notebooks found"], metadata.deepnote.notebooks, nbformat, nbformat_minor }
end
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/components/NotebookPicker.tsx(3 hunks)src/deepnote-content-provider.ts(3 hunks)src/fallback-data.ts(1 hunks)src/transform-deepnote-yaml-to-notebook-content.ts(3 hunks)src/types.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-09T11:21:57.482Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.482Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs Deepnote data transformations
Applied to files:
src/transform-deepnote-yaml-to-notebook-content.tssrc/deepnote-content-provider.tssrc/types.tssrc/fallback-data.ts
📚 Learning: 2025-10-09T11:21:57.482Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.482Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles Deepnote notebook state management
Applied to files:
src/transform-deepnote-yaml-to-notebook-content.tssrc/components/NotebookPicker.tsx
📚 Learning: 2025-10-09T11:21:57.482Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.482Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts contains Deepnote-related type definitions
Applied to files:
src/transform-deepnote-yaml-to-notebook-content.tssrc/deepnote-content-provider.tssrc/types.tssrc/fallback-data.ts
📚 Learning: 2025-10-09T11:21:57.482Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.482Z
Learning: Applies to src/notebooks/deepnote/deepnoteSerializer.ts : deepnoteSerializer.ts is the main serializer orchestrating Deepnote integration
Applied to files:
src/deepnote-content-provider.tssrc/types.ts
📚 Learning: 2025-10-09T11:21:57.482Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.482Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookSelector.ts : deepnoteNotebookSelector.ts implements UI selection logic for Deepnote notebooks
Applied to files:
src/components/NotebookPicker.tsx
🧬 Code graph analysis (2)
src/transform-deepnote-yaml-to-notebook-content.ts (2)
src/convert-deepnote-block-to-jupyter-cell.ts (1)
convertDeepnoteBlockToJupyterCell(10-44)src/types.ts (1)
IDeepnoteNotebookMetadata(21-32)
src/components/NotebookPicker.tsx (1)
src/types.ts (1)
deepnoteMetadataSchema(10-19)
⏰ 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
- GitHub Check: check_release
🔇 Additional comments (11)
src/fallback-data.ts (1)
24-24: LGTM!The shift to a notebooks map cleanly replaces the previous null placeholders.
src/deepnote-content-provider.ts (3)
7-16: LGTM!The renamed schema better describes its purpose (validating server response structure).
32-34: LGTM!Validation call correctly uses the renamed schema.
46-50: LGTM!Comment clarifies the transformation step.
src/transform-deepnote-yaml-to-notebook-content.ts (2)
43-50: LGTM!The return structure correctly includes cells, metadata with notebooks map, and nbformat fields, matching the IDeepnoteNotebookContent interface.
12-22: Verify notebook.name uniqueness or switch to id key
Unable to locate theDeepnoteFiledefinition locally; ensurenotebook.nameis guaranteed unique indeepnoteFile.project.notebooks. If not, usenotebook.idas the map key to avoid collisions.src/components/NotebookPicker.tsx (3)
35-44: LGTM!Validation with early return prevents accessing invalid metadata.
69-74: Verify that Object.values preserves notebook order.If notebook order matters for the dropdown, note that
Object.values()iteration order depends on key insertion order. With string keys, this should be stable in modern JS, but consider whether the dropdown should display notebooks in a specific order (e.g., by ID or creation time).
46-60: Verify notebook key uniqueness and fromJSON metadata support
- Ensure
deepnoteMetadataValidated.data.notebooksis keyed by a unique ID (not justnotebook.name) to avoid selecting the wrong notebook.- Manually confirm that
NotebookPanel.model.fromJSONaccepts the metadata shape{ deepnote: { notebooks } }and adjust if it doesn’t.src/types.ts (2)
21-32: LGTM!The interface correctly defines the notebooks map structure with typed cells.
34-37: LGTM!The IDeepnoteNotebookContent interface properly extends INotebookContent with the custom metadata type.
| export const deepnoteMetadataSchema = z.object({ | ||
| notebooks: z.record( | ||
| z.string(), | ||
| z.object({ | ||
| id: z.string(), | ||
| name: z.string(), | ||
| cells: z.array(z.any()) | ||
| }) | ||
| ) | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Tighten schema cell typing to match the interface.
The schema uses z.array(z.any()) for cells (line 16), while the interface specifies Array<ICodeCell | IMarkdownCell> (line 28). This inconsistency allows invalid cells to pass validation.
Apply this diff to align the schema with the interface:
+const cellSchema = z.object({
+ cell_type: z.enum(['code', 'markdown']),
+ source: z.union([z.string(), z.array(z.string())]),
+ metadata: z.record(z.any())
+});
+
export const deepnoteMetadataSchema = z.object({
notebooks: z.record(
z.string(),
z.object({
id: z.string(),
name: z.string(),
- cells: z.array(z.any())
+ cells: z.array(cellSchema)
})
)
});🤖 Prompt for AI Agents
In src/types.ts around lines 10 to 19, the deepnoteMetadataSchema currently uses
z.array(z.any()) for cells which is too permissive; change it to validate
against the two cell shapes declared by the interface (ICodeCell and
IMarkdownCell). Replace z.array(z.any()) with a z.array(z.union([<ICodeCell
schema>, <IMarkdownCell schema>])) — or better, if the cell objects have a
distinguishing "type" field, use z.discriminatedUnion('type', [{ type:
z.literal('code'), ...code cell fields schema }, { type: z.literal('markdown'),
...markdown cell fields schema }]) — ensuring each schema includes the exact
fields and types from the ICodeCell and IMarkdownCell interfaces so invalid
cells are rejected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don’t want to recreate schemas for ICodeCell and IMarkdownCell types. This is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andyjakubowski, understood. I'll leave the schema as is.
🧠 Learnings used
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.482Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts contains Deepnote-related type definitions
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.482Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs Deepnote data transformations
Signed-off-by: Andy Jakubowski [email protected]
Summary by CodeRabbit
Bug Fixes
Improvements