refactor: Change how we handle blocks to make it possible to support more Deepnote ones. #17
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR removes legacy MIME/output processing and stream/error/rich handlers, plus several data conversion utilities and the DeepnoteVirtualDocumentProvider. It introduces a converter-based serialization system: a BlockConverter interface, Code/Markdown/Text converters, and a ConverterRegistry. DeepnoteDataConverter is reworked to use the registry and a pocketed metadata shape (__deepnotePocket), with fallback conversions and bidirectional output transform helpers. Deepnote types were updated (block.type → string; DeepnoteOutput → union). New pocket utilities, unit tests for converters/pocket/DeepnoteDataConverter, and a serialization guide were added. Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as VS Code/Caller
participant DDC as DeepnoteDataConverter
participant CR as ConverterRegistry
participant Conv as BlockConverter
participant Pocket as pocket.ts
participant Out as OutputTransformer
Client->>DDC: convertBlocksToCells(blocks)
loop each block
DDC->>CR: findConverter(block.type)
alt converter found
CR-->>DDC: Conv
DDC->>Conv: convertToCell(block)
Conv-->>DDC: NotebookCellData
else fallback
DDC-->>DDC: createFallbackCell(block)
end
DDC->>Pocket: addPocketToCellMetadata(cell)
DDC->>Out: transformOutputsForVsCode(block.outputs)
Out-->>DDC: NotebookCellOutput[]
end
DDC-->>Client: cells
sequenceDiagram
autonumber
actor Client as VS Code/Caller
participant DDC as DeepnoteDataConverter
participant CR as ConverterRegistry
participant Conv as BlockConverter
participant Pocket as pocket.ts
participant Out as OutputTransformer
Client->>DDC: convertCellsToBlocks(cells)
loop each cell with index
DDC->>Pocket: createBlockFromPocket(cell, index)
Pocket-->>DDC: DeepnoteBlock
DDC->>CR: findConverter(block.type)
alt converter found
CR-->>DDC: Conv
DDC->>Conv: applyChangesToBlock(block, cell)
else fallback
DDC-->>DDC: createFallbackBlock(cell)
end
DDC->>Out: transformOutputsForDeepnote(cell.outputs)
Out-->>DDC: DeepnoteOutput[]
end
DDC-->>Client: blocks
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/!(*.node|*.web).ts📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
src/notebooks/deepnote/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/notebooks/deepnote/**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-08-27T11:02:24.731ZApplied to files:
🧬 Code graph analysis (1)src/notebooks/deepnote/pocket.ts (3)
⏰ 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)
🔇 Additional comments (7)
Comment |
Artmann
force-pushed
the
artmann/oss-59-support-deepnotes-text-blocks
branch
3 times, most recently
from
a46b027 to
53bf03c
Compare
…more Deepnote ones. This adds partial support for Deepnote text blocks as a proof of concept.
Artmann
force-pushed
the
artmann/oss-59-support-deepnotes-text-blocks
branch
from
53bf03c to
ff9a9f8
Compare
There was a problem hiding this comment.
Pull Request Overview
This refactor introduces a converter pattern-based architecture for handling different Deepnote block types, replacing the previous MIME-type focused approach. The changes enable extensible support for various Deepnote block formats while maintaining perfect round-trip conversion.
Key changes:
- Replaces legacy MIME/output handlers with a registry-driven converter system
- Introduces the "pocket" metadata system for preserving Deepnote-specific fields
- Adds partial support for Deepnote text blocks (h1, h2, h3, paragraph) as a proof of concept
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/notebooks/deepnote/serialization.md |
Comprehensive documentation explaining the new converter architecture and pocket system |
src/notebooks/deepnote/pocket.unit.test.ts |
Unit tests for pocket metadata management functions |
src/notebooks/deepnote/pocket.ts |
Core pocket system implementation for storing Deepnote-specific metadata |
src/notebooks/deepnote/outputHandlers/*.ts |
Removal of legacy output handler classes |
src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts |
Removal of virtual document provider |
src/notebooks/deepnote/deepnoteTypes.ts |
Updated type definitions with more flexible output types |
src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts |
Updated tests to use new pocket-based system |
src/notebooks/deepnote/deepnoteDataConverter.ts |
Complete refactor to use converter registry pattern |
src/notebooks/deepnote/dataConversionUtils.ts |
Simplified to only contain ID/sorting key generation |
src/notebooks/deepnote/converters/*.ts |
New converter implementations for different block types |
src/notebooks/deepnote/OutputTypeDetector.ts |
Removal of legacy output type detection |
src/notebooks/deepnote/MimeTypeProcessor.ts |
Removal of legacy MIME type processing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1)
228-229: Avoid env‑specific reliance on global TextDecoderSome Node test runners don’t expose TextDecoder globally. Import from ‘util’ or use Buffer for decoding to prevent flakes.
Option 1:
+import { TextDecoder } from 'node:util';Option 2:
-assert.strictEqual(new TextDecoder().decode(outputs[0].items[0].data), 'Hello world\n'); +assert.strictEqual(Buffer.from(outputs[0].items[0].data).toString('utf8'), 'Hello world\n');Please confirm CI’s Node version guarantees a global TextDecoder.
Also applies to: 341-342, 391-393
src/notebooks/deepnote/dataConversionUtils.ts (2)
8-15: Consider crypto.randomUUID() or equivalent for stronger uniqueness guarantees.Math.random() suffices for typical cases but risks collisions under high load. For block IDs in a collaborative system, prefer crypto.randomUUID() (Node 15+) or a library like
uuid.Example with crypto.randomUUID():
export function generateBlockId(): string { - const chars = '0123456789abcdef'; - let id = ''; - for (let i = 0; i < 32; i++) { - id += chars[Math.floor(Math.random() * chars.length)]; - } - return id; + return crypto.randomUUID().replace(/-/g, ''); }
20-26: Sorting ambiguity beyond index 2599.Clamping to 'z' when
letterIndex >= 26causes duplicate keys for indices ≥2600 (z0 repeats at 2600, 2700, etc.). If >2600 blocks is possible, consider extending the key scheme (e.g., multi-letter prefixes: aa0, ab0) or enforce a hard limit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (21)
src/notebooks/deepnote/MimeTypeProcessor.ts(0 hunks)src/notebooks/deepnote/OutputTypeDetector.ts(0 hunks)src/notebooks/deepnote/converters/blockConverter.ts(1 hunks)src/notebooks/deepnote/converters/codeBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts(1 hunks)src/notebooks/deepnote/converters/converterRegistry.ts(1 hunks)src/notebooks/deepnote/converters/markdownBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts(1 hunks)src/notebooks/deepnote/converters/textBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts(1 hunks)src/notebooks/deepnote/dataConversionUtils.ts(2 hunks)src/notebooks/deepnote/deepnoteDataConverter.ts(3 hunks)src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts(7 hunks)src/notebooks/deepnote/deepnoteTypes.ts(1 hunks)src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts(0 hunks)src/notebooks/deepnote/outputHandlers/ErrorOutputHandler.ts(0 hunks)src/notebooks/deepnote/outputHandlers/RichOutputHandler.ts(0 hunks)src/notebooks/deepnote/outputHandlers/StreamOutputHandler.ts(0 hunks)src/notebooks/deepnote/pocket.ts(1 hunks)src/notebooks/deepnote/pocket.unit.test.ts(1 hunks)src/notebooks/deepnote/serialization.md(1 hunks)
💤 Files with no reviewable changes (6)
- src/notebooks/deepnote/deepnoteVirtualDocumentProvider.ts
- src/notebooks/deepnote/outputHandlers/ErrorOutputHandler.ts
- src/notebooks/deepnote/OutputTypeDetector.ts
- src/notebooks/deepnote/MimeTypeProcessor.ts
- src/notebooks/deepnote/outputHandlers/StreamOutputHandler.ts
- src/notebooks/deepnote/outputHandlers/RichOutputHandler.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/converters/converterRegistry.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/dataConversionUtils.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/blockConverter.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.tssrc/notebooks/deepnote/pocket.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility, then alphabetically
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/converters/converterRegistry.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/dataConversionUtils.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/blockConverter.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.tssrc/notebooks/deepnote/pocket.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/serialization.mdsrc/notebooks/deepnote/converters/converterRegistry.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/dataConversionUtils.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/blockConverter.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.tssrc/notebooks/deepnote/pocket.ts
src/notebooks/deepnote/deepnoteDataConverter.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteDataConverter.ts performs data transformations
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Add a blank line after const groups and before return statements (whitespace for readability)
Files:
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/converters/converterRegistry.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/dataConversionUtils.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/blockConverter.tssrc/notebooks/deepnote/pocket.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.tssrc/notebooks/deepnote/pocket.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Name unit test files with the .unit.test.ts extension and use Mocha/Chai
Place unit test files alongside the source files they test
Files:
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/pocket.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/pocket.unit.test.ts
{**/*.unit.test.ts,out/**/*.unit.test.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use assert.deepStrictEqual() for object comparisons in tests
Files:
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/pocket.unit.test.ts
src/notebooks/deepnote/deepnoteTypes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteTypes.ts should contain only type definitions
Files:
src/notebooks/deepnote/deepnoteTypes.ts
🧠 Learnings (4)
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs data transformations
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.tssrc/notebooks/deepnote/converters/markdownBlockConverter.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/dataConversionUtils.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/blockConverter.tssrc/notebooks/deepnote/converters/codeBlockConverter.ts
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/export/**/*.ts : Update FileConverter to handle the new export format
Applied to files:
src/notebooks/deepnote/converters/markdownBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/blockConverter.tssrc/notebooks/deepnote/converters/codeBlockConverter.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts should contain only type definitions
Applied to files:
src/notebooks/deepnote/deepnoteTypes.ts
📚 Learning: 2025-09-11T09:38:17.564Z
Learnt from: jankuca
PR: deepnote/deepnote#18381
File: apps/webapp/server/graphql/types/notebook/resolvers.test.ts:888-918
Timestamp: 2025-09-11T09:38:17.564Z
Learning: The getBlocksInNotebook function returns blocks already sorted by their sorting_key, so positional assertions in tests already verify correct ordering without needing explicit lexical comparisons.
Applied to files:
src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
🧬 Code graph analysis (12)
src/notebooks/deepnote/deepnoteDataConverter.ts (9)
src/notebooks/deepnote/converters/converterRegistry.ts (1)
ConverterRegistry(3-22)src/notebooks/deepnote/converters/codeBlockConverter.ts (1)
CodeBlockConverter(6-24)src/notebooks/deepnote/converters/textBlockConverter.ts (1)
TextBlockConverter(6-48)src/notebooks/deepnote/converters/markdownBlockConverter.ts (1)
MarkdownBlockConverter(6-24)src/notebooks/deepnote/deepnoteTypes.ts (2)
DeepnoteBlock(36-44)DeepnoteOutput(49-77)src/notebooks/deepnote/deepnoteSerializer.ts (2)
DeepnoteBlock(9-9)DeepnoteOutput(9-9)src/notebooks/deepnote/pocket.ts (2)
addPocketToCellMetadata(18-35)createBlockFromPocket(41-77)src/test/mocks/vsc/extHostedTypes.ts (3)
bytes(47-49)NotebookCellOutput(71-103)NotebookCellOutputItem(16-69)src/notebooks/deepnote/dataConversionUtils.ts (2)
generateBlockId(8-15)generateSortingKey(20-26)
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts (3)
src/notebooks/deepnote/converters/codeBlockConverter.ts (1)
CodeBlockConverter(6-24)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-44)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/converters/markdownBlockConverter.ts (3)
src/notebooks/deepnote/converters/blockConverter.ts (1)
BlockConverter(5-10)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-44)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/deepnoteTypes.ts (2)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteOutput(9-9)gulpfile.js (1)
key(160-160)
src/notebooks/deepnote/converters/converterRegistry.ts (1)
src/notebooks/deepnote/converters/blockConverter.ts (1)
BlockConverter(5-10)
src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (3)
src/notebooks/deepnote/converters/textBlockConverter.ts (1)
TextBlockConverter(6-48)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-44)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts (3)
src/notebooks/deepnote/converters/markdownBlockConverter.ts (1)
MarkdownBlockConverter(6-24)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-44)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/converters/textBlockConverter.ts (3)
src/notebooks/deepnote/converters/blockConverter.ts (1)
BlockConverter(5-10)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-44)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/converters/blockConverter.ts (2)
src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-44)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/pocket.unit.test.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)src/notebooks/deepnote/pocket.ts (3)
addPocketToCellMetadata(18-35)extractPocketFromCellMetadata(37-39)createBlockFromPocket(41-77)
src/notebooks/deepnote/converters/codeBlockConverter.ts (3)
src/notebooks/deepnote/converters/blockConverter.ts (1)
BlockConverter(5-10)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-44)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/pocket.ts (3)
src/notebooks/deepnote/deepnoteTypes.ts (2)
DeepnoteOutput(49-77)DeepnoteBlock(36-44)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)src/notebooks/deepnote/dataConversionUtils.ts (2)
generateBlockId(8-15)generateSortingKey(20-26)
🪛 markdownlint-cli2 (0.18.1)
src/notebooks/deepnote/serialization.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
227-227: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
234-234: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (18)
src/notebooks/deepnote/deepnoteDataConverter.ts (6)
1-22: LGTM!Clean constructor pattern with registry-based converter registration.
69-88: LGTM!Registry-based conversion with proper fallback handling and conditional output transformation.
90-99: LGTM!Standard base64 decoding implementation.
101-121: LGTM!Fallback handling preserves metadata and maps cell kinds appropriately.
123-202: LGTM!Comprehensive output transformation with proper error handling and fallback. Image encoding uses
btoa, which is standard in Node.js 16+.
204-320: LGTM!Thorough output transformation with rich format prioritization, proper mime type mapping, and multiple fallback paths.
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts (1)
1-141: LGTM!Comprehensive test coverage with proper assertions. Follows Mocha/Chai conventions and uses
assert.deepStrictEqual()for object comparisons as per guidelines.src/notebooks/deepnote/converters/codeBlockConverter.ts (1)
1-24: LGTM!Clean implementation of BlockConverter. Hardcoding
languageIdto 'python' is reasonable for Deepnote's Python-focused context.src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (1)
1-228: LGTM!Thorough test coverage for text block converter with heading prefix handling. Follows guidelines for assertions and test structure.
src/notebooks/deepnote/converters/blockConverter.ts (1)
1-10: LGTM!Clean interface definition with type-only imports as per guidelines.
src/notebooks/deepnote/deepnoteTypes.ts (2)
43-43: LGTM!Widening
typetostringenables extensible block type support via converters.
49-77: LGTM!Discriminated union for
DeepnoteOutputimproves type safety. Catch-all variant handles unknown output types gracefully.src/notebooks/deepnote/converters/markdownBlockConverter.ts (1)
1-24: LGTM!Clean implementation of BlockConverter for markdown blocks.
src/notebooks/deepnote/converters/converterRegistry.ts (1)
1-1: Add required copyright headerSource files must include the Microsoft copyright header.
As per coding guidelines.
⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-12T12:52:09.726Z Learning: Do not add the Microsoft copyright header to new filessrc/notebooks/deepnote/dataConversionUtils.ts (1)
2-2: LGTM!Module description accurately reflects the narrowed scope.
src/notebooks/deepnote/pocket.ts (3)
8-16: LGTM!Pocket interface correctly defines optional Deepnote-specific fields.
18-35: LGTM!Function correctly collects and pockets Deepnote-specific fields from cell metadata.
37-39: LGTM!Simple and correct accessor for pocket metadata.
| findConverter(blockType: string): BlockConverter | undefined { | ||
| return this.typeToConverterMap.get(blockType); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make lookup case‑insensitive
Normalize the key to lower case to match converter types consistently.
Apply:
- findConverter(blockType: string): BlockConverter | undefined {
- return this.typeToConverterMap.get(blockType);
- }
+ findConverter(blockType: string): BlockConverter | undefined {
+ return this.typeToConverterMap.get(blockType.toLowerCase());
+ }📝 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.
| findConverter(blockType: string): BlockConverter | undefined { | |
| return this.typeToConverterMap.get(blockType); | |
| } | |
| findConverter(blockType: string): BlockConverter | undefined { | |
| return this.typeToConverterMap.get(blockType.toLowerCase()); | |
| } |
🤖 Prompt for AI Agents
In src/notebooks/deepnote/converters/converterRegistry.ts around lines 7 to 9,
the converter lookup is case-sensitive; normalize the incoming blockType to
lower case before using it to access typeToConverterMap (e.g., validate
blockType is a string and call blockType.toLowerCase()) so lookups are
consistent regardless of input casing; ensure any map keys are stored in the
same lower-case form elsewhere or adjust insertion accordingly.
| suite('canConvert', () => { | ||
| test('returns true for markdown type', () => { | ||
| assert.isTrue(converter.canConvert('markdown')); | ||
| }); | ||
|
|
||
| test('returns false for non-markdown types', () => { | ||
| assert.isFalse(converter.canConvert('code')); | ||
| assert.isFalse(converter.canConvert('text-cell-h1')); | ||
| assert.isFalse(converter.canConvert('unknown')); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider case‑insensitive canConvert + test for it
TextBlockConverter treats types case‑insensitively; align Markdown for consistency and sturdier registry usage.
Add:
test('canConvert ignores case', () => {
assert.isTrue(converter.canConvert('MARKDOWN'));
});And update converter (in markdownBlockConverter.ts):
canConvert(blockType: string): boolean {
return blockType.toLowerCase() === 'markdown';
}🤖 Prompt for AI Agents
In src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts around
lines 14-24, the unit tests and converter should handle block types
case-insensitively; add a test named "canConvert ignores case" that asserts
converter.canConvert('MARKDOWN') returns true, and update the implementation in
src/notebooks/deepnote/converters/markdownBlockConverter.ts so canConvert
normalizes the incoming blockType (e.g., toLowerCase()) before comparing to
'markdown' to ensure case-insensitive matching.
There was a problem hiding this comment.
Actionable comments posted: 3
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/deepnoteTypes.ts (2)
50-79: Make union fully discriminated (missingoutput_typeon “text” variant).Without a discriminant, narrowing on
output_typeis incomplete and error‑prone.Option A (preferred if valid for Deepnote data):
| { - name?: string; - text?: string; + output_type: 'text'; + name?: string; + text?: string; }If Deepnote truly omits
output_typefor this shape, document the intent and ensure all consumers guard for its absence. Based on learnings
1-1: Add required copyright header.Add the standard header at file top.
+/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/As per coding guidelines
♻️ Duplicate comments (5)
src/notebooks/deepnote/converters/converterRegistry.ts (2)
4-4:typeToConverterMapis actively used.Past comment incorrectly flagged this field as unused. It's read by
findConverter(line 7) andlistSupportedTypes(line 11).
14-17: Critical: Normalize keys inregisterto matchfindConverterlookup.
findConverterlowercases the input (line 7), butregisterstores keys as-is. IfgetSupportedTypes()returns mixed-case types, lookups fail.Apply this diff:
register(converter: BlockConverter): void { converter.getSupportedTypes().forEach((type) => { - this.typeToConverterMap.set(type, converter); + this.typeToConverterMap.set(type.toLowerCase(), converter); }); }src/notebooks/deepnote/converters/textBlockConverter.ts (2)
23-24: Case-insensitive matching already implemented.
canConvertusesblockType.toLowerCase()before checking againsttextBlockTypes(which are lowercase). Past comment about case sensitivity is resolved.
28-28: TODO lacks implementation details.Which library?
markdown-it,marked,unified/remark? When? Link to tracking issue?Enhance specificity:
- // TODO: Use the library to handle the markdown conversion here in the future. + // TODO: Use markdown-it or unified/remark for robust heading conversion. Track: #<issue-number>src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts (1)
19-23: Case-insensitive tests already present.Past review suggested adding case-insensitive canConvert tests. Lines 19-23 implement exactly that.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
src/notebooks/deepnote/converters/codeBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts(1 hunks)src/notebooks/deepnote/converters/converterRegistry.ts(1 hunks)src/notebooks/deepnote/converters/markdownBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts(1 hunks)src/notebooks/deepnote/converters/textBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteTypes.ts(1 hunks)src/notebooks/deepnote/pocket.ts(1 hunks)src/notebooks/deepnote/serialization.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/converters/codeBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/markdownBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/converterRegistry.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility, then alphabetically
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/converters/codeBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/markdownBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/converterRegistry.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/converters/codeBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/markdownBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/serialization.mdsrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/converterRegistry.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Add a blank line after const groups and before return statements (whitespace for readability)
Files:
src/notebooks/deepnote/converters/codeBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/markdownBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/converterRegistry.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Name unit test files with the .unit.test.ts extension and use Mocha/Chai
Place unit test files alongside the source files they test
Files:
src/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts
{**/*.unit.test.ts,out/**/*.unit.test.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use assert.deepStrictEqual() for object comparisons in tests
Files:
src/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts
src/notebooks/deepnote/deepnoteTypes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteTypes.ts should contain only type definitions
Files:
src/notebooks/deepnote/deepnoteTypes.ts
🧠 Learnings (5)
📚 Learning: 2025-09-03T12:59:14.489Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/notebooks.instructions.md:0-0
Timestamp: 2025-09-03T12:59:14.489Z
Learning: Applies to src/notebooks/export/**/*.ts : Update FileConverter to handle the new export format
Applied to files:
src/notebooks/deepnote/converters/codeBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/converters/markdownBlockConverter.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs data transformations
Applied to files:
src/notebooks/deepnote/converters/codeBlockConverter.tssrc/notebooks/deepnote/converters/textBlockConverter.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Do not add the Microsoft copyright header to new files
Applied to files:
src/notebooks/deepnote/converters/textBlockConverter.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts should contain only type definitions
Applied to files:
src/notebooks/deepnote/deepnoteTypes.ts
📚 Learning: 2025-08-27T11:02:24.731Z
Learnt from: mfranczel
PR: deepnote/deepnote#18265
File: libs/filesystem-node/src/lib/modules/darwin/dataset-client.ts:10-11
Timestamp: 2025-08-27T11:02:24.731Z
Learning: In libs/filesystem-node dataset client implementations, maintain consistency with existing patterns for discriminant properties (e.g., readonly type: StorageType = 'storageType') rather than optimizing for TypeScript narrowing with 'as const', to ensure uniformity across similar classes in the module.
Applied to files:
src/notebooks/deepnote/pocket.ts
🧬 Code graph analysis (7)
src/notebooks/deepnote/converters/codeBlockConverter.ts (3)
src/notebooks/deepnote/converters/blockConverter.ts (1)
BlockConverter(5-10)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-45)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/converters/textBlockConverter.ts (3)
src/notebooks/deepnote/converters/blockConverter.ts (1)
BlockConverter(5-10)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-45)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (3)
src/notebooks/deepnote/converters/textBlockConverter.ts (1)
TextBlockConverter(6-48)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-45)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts (3)
src/notebooks/deepnote/converters/codeBlockConverter.ts (1)
CodeBlockConverter(6-24)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-45)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
src/notebooks/deepnote/converters/converterRegistry.ts (1)
src/notebooks/deepnote/converters/blockConverter.ts (1)
BlockConverter(5-10)
src/notebooks/deepnote/pocket.ts (3)
src/notebooks/deepnote/deepnoteTypes.ts (2)
DeepnoteOutput(50-78)DeepnoteBlock(36-45)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)src/notebooks/deepnote/dataConversionUtils.ts (2)
generateBlockId(8-15)generateSortingKey(20-26)
src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts (3)
src/notebooks/deepnote/converters/markdownBlockConverter.ts (1)
MarkdownBlockConverter(6-24)src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-45)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
🔇 Additional comments (12)
src/notebooks/deepnote/converters/markdownBlockConverter.ts (1)
6-24: Converter logic looks solid.Correct kind, language, content mapping, and type handling.
src/notebooks/deepnote/pocket.ts (1)
41-77: Block creation looks correct.Pocket extraction, metadata sanitization, id/sortingKey fallbacks, and content mapping are sound.
src/notebooks/deepnote/serialization.md (1)
1-306: Docs read clean; lint nits addressed.Fenced languages and heading spacing look fixed. Content aligns with implementation.
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts (1)
1-147: Good coverage and assertions.Case handling, content variants, and non‑mutation checks are solid; uses deepStrictEqual per guidelines.
src/notebooks/deepnote/converters/codeBlockConverter.ts (3)
7-23: LGTM — methods are already alphabetically ordered.
applyChangesToBlock,canConvert,convertToCell,getSupportedTypesfollow alphabetical order per coding guidelines.
16-16: Ignore hard-codedpythonconcern. DeepnoteBlock has no language metadata and converters/tests rely on Python as the default.Likely an incorrect or invalid review comment.
7-23: Optional: Order methods alphabetically.Coding guidelines specify ordering class members alphabetically within the same accessibility level. Current order:
applyChangesToBlock,canConvert,convertToCell,getSupportedTypes.As per coding guidelines.
Apply this diff:
- applyChangesToBlock(block: DeepnoteBlock, cell: NotebookCellData): void { - block.content = cell.value || ''; - } - - canConvert(blockType: string): boolean { - return blockType.toLowerCase() === 'code'; - } - - convertToCell(block: DeepnoteBlock): NotebookCellData { - const cell = new NotebookCellData(NotebookCellKind.Code, block.content || '', 'python'); - - return cell; - } - - getSupportedTypes(): string[] { - return ['code']; - } + applyChangesToBlock(block: DeepnoteBlock, cell: NotebookCellData): void { + block.content = cell.value || ''; + } + + canConvert(blockType: string): boolean { + return blockType.toLowerCase() === 'code'; + } + + convertToCell(block: DeepnoteBlock): NotebookCellData { + const cell = new NotebookCellData(NotebookCellKind.Code, block.content || '', 'python'); + + return cell; + } + + getSupportedTypes(): string[] { + return ['code']; + }Note: Already alphabetical—this is a no-op. Ignore if guidelines are lenient for simple classes.
⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-12T12:52:09.726Z Learning: Applies to **/*.{ts,tsx} : Order class members (methods, fields, properties) first by accessibility, then alphabeticallyLearnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-12T12:52:09.726Z Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs data transformationssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (1)
1-270: LGTM — comprehensive test coverage.Tests exercise all TextBlockConverter methods with edge cases: case sensitivity, empty/missing content, whitespace handling, and property preservation. Uses
assert.deepStrictEqualfor object comparisons per guidelines.src/notebooks/deepnote/converters/converterRegistry.ts (1)
6-8: LGTM — case-insensitive lookup implemented.
findConverternormalizes input to lowercase before map lookup, addressing past review feedback.src/notebooks/deepnote/converters/textBlockConverter.ts (2)
12-18: Robust header stripping with leading whitespace handling.Regex patterns
/^\s*#\s+/correctly strip headings even with leading spaces, addressing past feedback.
45-46: Returns defensive copy of supported types.
[...textBlockTypes]prevents external mutation, addressing past review.src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts (1)
1-162: LGTM — thorough test suite.Covers all MarkdownBlockConverter methods with edge cases: case sensitivity, empty/missing content, complex markdown, and property preservation. Uses
assert.deepStrictEqualper guidelines.
jankuca
left a comment
jankuca
left a comment
There was a problem hiding this comment.
These changes and setup make sense, cool. I'm not seeing anything that would be obviously wrong.
I have just a single high-level question - how different is the Jupyter and VSCode notebook format? I see you are adding the "pocket" fields to the VSCode format but we are not doing anything like that for conversions from Deepnote to Jupyter - we just add the extra data to the cell objects and metadata. Was this deemed too unclean, or is it not possible in VSCode? I'm thinking about how many different formats we have and if we could consolidate a bit.
VS Codes notebook api is similar to Jupyter and they support code and markdown blocks. I used the 'pocket' just to make it a bit easier to pass back and forth. |
This adds partial support for Deepnote text blocks as a proof of concept.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests