feat: Add support for all text blocks (using @deepnote/blocks). #22
Conversation
📝 WalkthroughWalkthroughAdds the @deepnote/blocks dependency and re-exports Deepnote types from it. Extends TextBlockConverter to support bullet, todo, callout, and separator blocks using createMarkdown/stripMarkdown and exposes getSupportedTypes(). Introduces an optional blockGroup field on Deepnote blocks and populates it in fallback and pocket flows and tests. Updates .npmrc and CI workflows to use the @deepnote GitHub Packages registry and inject NODE_AUTH_TOKEN for authenticated installs. Removes DeepnoteBlock from DeepnoteSerializer exports; other runtime behavior unchanged. Sequence Diagram(s)%%{init: {"themeVariables": {"actorBorder":"#1f6feb","actorBackground":"#e6f0ff","noteFill":"#fbfdff","sequenceNumber":"#6b7280"}}}%%
sequenceDiagram
autonumber
actor User as User
participant VSCode as "Editor / VS Code"
participant Serializer as "DeepnoteSerializer / Data layer"
participant Converter as "TextBlockConverter"
participant BlocksLib as "@deepnote/blocks"
User->>VSCode: Open notebook
VSCode->>Serializer: fetchNotebook()
Serializer-->>VSCode: notebook(blocks[] with blockGroup, executionMode:'block')
VSCode->>Converter: convertToCell(block{type,content,blockGroup})
alt block.type == "separator"
Converter-->>VSCode: NotebookCell(markdown:"<hr>")
else
Converter->>BlocksLib: createMarkdown(block)
BlocksLib-->>Converter: markdown
Converter-->>VSCode: NotebookCell(markdown:markdown)
end
User->>VSCode: Edit cell
VSCode->>Converter: applyChangesToBlock(cell, block)
alt block.type == "separator"
Converter->>block: block.content = ""
else
Converter->>BlocksLib: stripMarkdown(cell.content)
BlocksLib-->>Converter: plainText
Converter->>block: block.content = plainText
end
Converter->>Serializer: persistBlockUpdate(block with blockGroup)
Serializer-->>VSCode: updated block
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (1)
1-3: Use Node’s deepStrictEqual; keep Chai for other asserts.chai.assert may not expose deepStrictEqual. Import Node assert for that one check.
As per coding guidelines-import { assert } from 'chai'; +import { assert } from 'chai'; +import { strict as nodeAssert } from 'assert'; @@ - assert.deepStrictEqual(types, [ + nodeAssert.deepStrictEqual(types, [ 'text-cell-h1', 'text-cell-h2', 'text-cell-h3', 'text-cell-p', 'text-cell-bullet', 'text-cell-todo', 'text-cell-callout', 'separator' ]);Also applies to: 44-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
CONTRIBUTING.md(1 hunks)package.json(1 hunks)src/notebooks/deepnote/converters/textBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts(16 hunks)src/notebooks/deepnote/deepnoteTypes.ts(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/deepnoteTypes.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.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/deepnoteTypes.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.ts
src/notebooks/deepnote/deepnoteTypes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteTypes.ts should contain only type definitions
Files:
src/notebooks/deepnote/deepnoteTypes.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/deepnoteTypes.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.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.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.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.ts
🧠 Learnings (1)
📚 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/textBlockConverter.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (2)
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)
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
38-38: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
48-48: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
55-55: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (4)
src/notebooks/deepnote/converters/textBlockConverter.ts (3)
8-17: Supported type list expansion looks good.Clear, exhaustive, and case-insensitive via canConvert().
41-44: LGTM: centralized Markdown generation.Using createMarkdown() and Markup kind is correct.
1-1: Ignore Microsoft header suggestion. This new Deepnote-authored file does not require a Microsoft copyright banner.Likely an incorrect or invalid review comment.
src/notebooks/deepnote/deepnoteTypes.ts (1)
1-1: Ignore request to add Microsoft copyright header. New files should not include that header per project guidelines (CLAUDE.md).Likely an incorrect or invalid review comment.
Artmann
force-pushed
the
chris/convert-text-blocks-to-markdown
branch
from
5a2607f to
f863fe2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
package.json (1)
2316-2316: packageManager integrity should use sha512, not sha1.Corepack expects sha512 integrity. Either update to correct hash or remove suffix.
- "packageManager": "pnpm@8.15.4+sha1.c85a4305534f76d461407b59277b954bac97b5c4" + "packageManager": "pnpm@8.15.4"src/notebooks/deepnote/deepnoteTypes.ts (1)
37-37: Verify all DeepnoteBlock constructions include blockGroup.Making blockGroup required is a breaking change. Previous review found 15 test fixtures and one code literal lacking this field.
CONTRIBUTING.md (1)
35-41: Fix markdown formatting for code fence.Add blank lines around fence and specify language identifier.
#### Install Recommended Extensions First, install all the recommended VS Code extensions. Open the Command Palette (Ctrl+Shift+P or Cmd+Shift+P) and run: + -``` +```text Extensions: Show Recommended Extensions
Then install all the extensions listed under "Workspace Recommendations".
</blockquote></details> <details> <summary>src/notebooks/deepnote/converters/textBlockConverter.ts (1)</summary><blockquote> `27-33`: **Consider normalizing leading whitespace before stripping markdown.** Leading spaces/tabs prevent stripMarkdown from removing heading markers, potentially causing duplicate '#' on roundtrip. ```diff - // Update block content with cell value first - block.content = cell.value || ''; - - // Then strip the markdown formatting to get plain text - const textValue = stripMarkdown(block); - - block.content = textValue; + const raw = cell.value ?? ''; + const normalized = ['text-cell-h1', 'text-cell-h2', 'text-cell-h3'].includes(block.type) + ? raw.replace(/^\s+/, '') + : raw; + + const textValue = stripMarkdown({ ...block, content: normalized }); + block.content = textValue;src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (3)
241-243: Update expectation if normalization is implemented.After normalization, content should be plain text without heading marker.
- // stripMarkdown doesn't handle leading whitespace, so it stays in the content - assert.strictEqual(block.content, '# New Title'); + // Leading whitespace normalized; heading marker stripped + assert.strictEqual(block.content, 'New Title');
272-274: Update expectation if normalization is implemented.- // stripMarkdown doesn't handle leading whitespace, so it stays in the content - assert.strictEqual(block.content, '## New Section'); + // Leading whitespace normalized; heading marker stripped + assert.strictEqual(block.content, 'New Section');
303-305: Update expectation if normalization is implemented.- // stripMarkdown doesn't handle leading whitespace, so it stays in the content - assert.strictEqual(block.content, '### New Subsection'); + // Leading whitespace normalized; heading marker stripped + assert.strictEqual(block.content, 'New Subsection');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.npmrc(1 hunks)CONTRIBUTING.md(1 hunks)package.json(2 hunks)src/notebooks/deepnote/converters/textBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts(16 hunks)src/notebooks/deepnote/deepnoteTypes.ts(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/deepnoteTypes.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.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/deepnoteTypes.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.ts
src/notebooks/deepnote/deepnoteTypes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteTypes.ts should contain only type definitions
Files:
src/notebooks/deepnote/deepnoteTypes.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/deepnoteTypes.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.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.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.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.ts
🧠 Learnings (1)
📚 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/textBlockConverter.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (2)
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)
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
38-38: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
40-40: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
48-48: Bare URL used
(MD034, no-bare-urls)
54-54: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
56-56: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (10)
.npmrc (1)
1-1: LGTM!Registry configuration correctly points @deepnote scope to GitHub Packages.
package.json (1)
2101-2101: LGTM!Dependency properly declared and used in text block converter.
src/notebooks/deepnote/converters/textBlockConverter.ts (4)
1-1: LGTM!Correctly imports markdown utilities from @deepnote/blocks.
8-17: LGTM!Correctly expands support to include bullet, todo, callout, and separator block types.
20-25: LGTM!Separator correctly handled with empty content and early return.
40-46: LGTM!Correctly delegates markdown generation to createMarkdown library function.
src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (4)
20-31: LGTM!Comprehensive canConvert tests for new block types with case insensitivity checks.
44-54: LGTM!getSupportedTypes test correctly includes all new block types.
116-196: LGTM!Comprehensive convertToCell tests for new block types with correct markdown expectations.
372-449: LGTM!Comprehensive applyChangesToBlock tests for new types validating prefix stripping and special separator handling.
Artmann
force-pushed
the
chris/convert-text-blocks-to-markdown
branch
2 times, most recently
from
2057fa5 to
641fd51
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/copilot-setup-steps.yml (1)
14-21: Nit: Unused NPM_VERSION envNPM_VERSION is set but not used. Trim unless you pin npm explicitly.
.github/workflows/deps.yml (1)
60-68: Second job will fail to install private packagescheck-deps lacks registry/auth config; npm ci will 401 on @deepnote/blocks.
Mirror the audit job:
- name: Setup Node.js uses: actions/setup-node@v5 with: - node-version: ${{ env.NODE_VERSION }} + node-version: ${{ env.NODE_VERSION }} cache: 'npm' + registry-url: 'https://npm.pkg.github.com' + scope: '@deepnote' - name: Install dependencies - run: npm ci --prefer-offline --no-audit + run: npm ci --prefer-offline --no-audit + env: + NODE_AUTH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(3 hunks).github/workflows/copilot-setup-steps.yml(2 hunks).github/workflows/deps.yml(1 hunks)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
15-17: Good: minimal permissions incl. packages: readThis enables GH Packages access while staying least-privileged.
41-42: Token usage check for forked PRsUsing GITHUB_TOKEN is correct; confirm it has packages: read for forked PRs per repo/org settings to avoid 401s.
Run in repo settings: verify Actions > Workflow permissions allow GITHUB_TOKEN to read packages on PRs from forks.
Also applies to: 67-68
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts(6 hunks)src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts(7 hunks)src/notebooks/deepnote/deepnoteDataConverter.ts(1 hunks)src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts(13 hunks)src/notebooks/deepnote/deepnoteSerializer.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts(2 hunks)src/notebooks/deepnote/pocket.ts(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/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.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/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.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/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.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/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.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/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/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/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.ts
src/notebooks/deepnote/deepnoteDataConverter.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteDataConverter.ts performs data transformations
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts
🧠 Learnings (4)
📚 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/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.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/**/*.ts : Add a blank line after const groups and before return statements (whitespace for readability)
Applied to files:
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.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/deepnoteSerializer.ts : deepnoteSerializer.ts acts as the main serializer/orchestrator
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.unit.test.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/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.ts
🔇 Additional comments (7)
src/notebooks/deepnote/pocket.ts (1)
61-61: LGTM!Proper handling of required blockGroup field with sensible fallback.
src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (1)
33-33: LGTM!Test data properly updated with required blockGroup field.
Also applies to: 247-274
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts (1)
43-43: LGTM!Test blocks correctly include required blockGroup field.
Also applies to: 59-59, 85-85, 101-101, 116-116, 131-131
src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts (1)
43-43: LGTM!Test blocks correctly include required blockGroup field.
Also applies to: 59-59, 85-85, 104-104, 119-119, 134-134, 154-154
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (1)
22-30: LGTM!Test data properly updated with required blockGroup field in block arrays.
Also applies to: 37-45
src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
24-32: LGTM!Test data properly updated with required blockGroup field.
Also applies to: 39-47
src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1)
18-18: LGTM!Comprehensive test data updates with required blockGroup field across all conversion scenarios.
Also applies to: 42-42, 63-63, 80-80, 101-101, 116-116, 131-131, 218-218, 246-246, 279-279, 309-309, 335-335, 362-362, 388-388, 411-411, 427-427
Artmann
force-pushed
the
chris/convert-text-blocks-to-markdown
branch
from
d1bce63 to
1cef943
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
42-49: Consider removing defensive optional handling.Lines 42-43 treat
blockGroupas optional even though the type now requires it:const blockWithOptionalFields = block as DeepnoteBlock & { blockGroup?: string };If this is for legacy data compatibility, consider documenting it. Otherwise, simplify:
-const blockWithOptionalFields = block as DeepnoteBlock & { blockGroup?: string }; - cell.metadata = { ...block.metadata, id: block.id, type: block.type, sortingKey: block.sortingKey, - ...(blockWithOptionalFields.blockGroup && { blockGroup: blockWithOptionalFields.blockGroup }), + blockGroup: block.blockGroup, ...(block.executionCount !== undefined && { executionCount: block.executionCount }), ...(block.outputs !== undefined && { outputs: block.outputs }) };
♻️ Duplicate comments (4)
CONTRIBUTING.md (1)
50-50: Fix bare URL (MD034).Wrap the URL in angle brackets or use a markdown link.
Apply this diff:
-- Go to https://github.com/settings/tokens +- Go to <https://github.com/settings/tokens>src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (3)
241-243: Align expectation with normalized heading stripping.After normalization, content should be plain text.
- // stripMarkdown doesn't handle leading whitespace, so it stays in the content - assert.strictEqual(block.content, '# New Title'); + // Leading whitespace is normalized; heading marker is stripped + assert.strictEqual(block.content, 'New Title');
272-274: Align expectation with normalized heading stripping (h2).- // stripMarkdown doesn't handle leading whitespace, so it stays in the content - assert.strictEqual(block.content, '## New Section'); + // Leading whitespace is normalized; heading marker is stripped + assert.strictEqual(block.content, 'New Section');
303-305: Align expectation with normalized heading stripping (h3).- // stripMarkdown doesn't handle leading whitespace, so it stays in the content - assert.strictEqual(block.content, '### New Subsection'); + // Leading whitespace is normalized; heading marker is stripped + assert.strictEqual(block.content, 'New Subsection');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.github/workflows/ci.yml(3 hunks).github/workflows/copilot-setup-steps.yml(3 hunks).github/workflows/deps.yml(2 hunks).npmrc(1 hunks)CONTRIBUTING.md(1 hunks)package.json(1 hunks)src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts(6 hunks)src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts(7 hunks)src/notebooks/deepnote/converters/textBlockConverter.ts(1 hunks)src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts(16 hunks)src/notebooks/deepnote/deepnoteDataConverter.ts(1 hunks)src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts(13 hunks)src/notebooks/deepnote/deepnoteSerializer.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts(1 hunks)src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts(2 hunks)src/notebooks/deepnote/deepnoteTypes.ts(1 hunks)src/notebooks/deepnote/pocket.ts(1 hunks)
🧰 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/pocket.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.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/pocket.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/pocket.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/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/pocket.tssrc/notebooks/deepnote/converters/codeBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.tssrc/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.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/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.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/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.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/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/converters/textBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/markdownBlockConverter.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.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
src/notebooks/deepnote/deepnoteDataConverter.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteDataConverter.ts performs data transformations
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts
🧠 Learnings (6)
📚 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/**/*.ts : Add a blank line after const groups and before return statements (whitespace for readability)
Applied to files:
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.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/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/deepnoteSerializer.ts : deepnoteSerializer.ts acts as the main serializer/orchestrator
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.unit.test.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/deepnoteSerializer.unit.test.tssrc/notebooks/deepnote/deepnoteTreeItem.unit.test.tssrc/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.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/deepnoteTreeItem.unit.test.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/deepnoteDataConverter.ts
🧬 Code graph analysis (2)
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 (2)
src/notebooks/deepnote/deepnoteTypes.ts (1)
DeepnoteBlock(36-45)src/test/mocks/vsc/extHostedTypes.ts (1)
NotebookCellData(2523-2549)
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
50-50: Bare URL used
(MD034, no-bare-urls)
56-56: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
58-58: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (33)
.npmrc (1)
1-1: LGTM! Correctly scoped registry.The scoped registry configuration ensures only
@deepnotepackages use GitHub Packages, leaving public packages to resolve from the default registry.package.json (1)
2102-2102: LGTM! Dependency addition is clean.The
@deepnote/blockspackage is properly added with semantic versioning. The registry configuration in.npmrcensures it resolves from GitHub Packages..github/workflows/copilot-setup-steps.yml (3)
34-34: Necessary permission for GitHub Packages.The
packages: readpermission allows the workflow to authenticate and fetch packages from GitHub Packages.
46-47: Correctly scoped registry configuration.Using both
registry-urlandscopeensures the GitHub Packages registry applies only to@deepnotepackages, leaving public packages to resolve normally.
65-66: Required authentication for private packages.The
NODE_AUTH_TOKENusingGITHUB_TOKENenables npm to authenticate when installing@deepnotescoped packages from GitHub Packages.src/notebooks/deepnote/deepnoteTypes.ts (1)
37-37: Breaking change:blockGroupnow required.Making
blockGrouprequired ensures consistent metadata across all blocks. Based on the AI summary, related converters, tests, and data providers have been updated to populate this field.src/notebooks/deepnote/deepnoteDataConverter.ts (1)
103-103: LGTM! Consistent default value.Using
'default-group'aligns with the fallback used inpocket.ts.CONTRIBUTING.md (1)
35-62: Clear documentation for @deepnote/blocks setup.The new sections effectively guide developers through:
- Installing workspace extensions
- Authenticating with GitHub Packages
- Handling Apple Silicon dependencies
This addresses a common setup hurdle.
.github/workflows/deps.yml (3)
14-15: Necessary permission for GitHub Packages access.The
packages: readpermission enables the workflow to fetch@deepnotescoped packages.
33-35: Correctly scoped registry configuration.The
scope: '@deepnote'parameter ensures only@deepnotepackages use GitHub Packages, avoiding conflicts with public npm packages.
39-40: Required authentication for private packages.The
NODE_AUTH_TOKENenables npm to authenticate when installing from GitHub Packages..github/workflows/ci.yml (5)
15-16: Necessary permission for GitHub Packages access.Adding
packages: readallows both jobs to fetch@deepnotescoped packages from GitHub Packages.
35-37: Correctly scoped registry configuration (lint job).The scoped registry setup ensures only
@deepnotepackages resolve from GitHub Packages.
41-42: Required authentication (lint job).The
NODE_AUTH_TOKENenables npm authentication for private package installs.
62-64: Correctly scoped registry configuration (build job).Consistent with the lint job, the registry is properly scoped to
@deepnote.
68-69: Required authentication (build job).Consistent authentication setup across both jobs ensures reliable package installs.
src/notebooks/deepnote/converters/codeBlockConverter.unit.test.ts (1)
43-43: LGTM! Test data updated for required blockGroup field.Consistently adds
blockGroup: 'test-group'to all test fixtures.Also applies to: 59-59, 85-85, 101-101, 116-116, 131-131
src/notebooks/deepnote/deepnoteTreeItem.unit.test.ts (1)
33-33: LGTM! Test fixtures properly include blockGroup.Mock data consistently uses
blockGroup: 'group-123'across single and multi-block scenarios.Also applies to: 247-274
src/notebooks/deepnote/deepnoteTreeDataProvider.unit.test.ts (1)
22-30: LGTM! Mock data structure updated correctly.Blocks now in array form with
blockGroup: 'group-123'included.Also applies to: 37-45
src/notebooks/deepnote/deepnoteSerializer.unit.test.ts (1)
24-32: LGTM! Test data aligned with required blockGroup.Consistent with other test updates - blocks wrapped in arrays with
blockGroup: 'group-123'.Also applies to: 39-47
src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1)
18-18: LGTM! Comprehensive test data update.All DeepnoteBlock fixtures consistently include
blockGroup: 'test-group'.Also applies to: 42-42, 63-63, 80-80, 218-218, 246-246, 279-279, 309-309, 335-335, 362-362, 388-388, 411-411, 427-427
src/notebooks/deepnote/converters/markdownBlockConverter.unit.test.ts (1)
43-43: LGTM! Markdown converter tests updated consistently.Test fixtures properly include
blockGroup: 'test-group'matching other converter tests.Also applies to: 59-59, 85-85, 104-104, 119-119, 134-134, 154-154
src/notebooks/deepnote/pocket.ts (1)
61-61: LGTM! Required field now always set.Ensures
blockGroupis present with sensible'default-group'fallback when pocket data is missing.src/notebooks/deepnote/converters/textBlockConverter.ts (6)
1-1: LGTM! Integration with @deepnote/blocks.Imports
createMarkdownandstripMarkdownfor standardized text block handling.
8-17: LGTM! Expanded text block support.Added bullet, todo, callout, and separator types. Aligns with PR objective to support all Deepnote text blocks.
19-25: LGTM! Separator handled correctly.Early return with empty content is appropriate for separator blocks.
27-34: LGTM! Uses stripMarkdown for formatting removal.Sets cell value then strips markdown formatting to get plain text. Delegates formatting logic to @deepnote/blocks.
40-46: LGTM! Delegates markdown generation to library.Replaced manual prefix assembly with
createMarkdown(block). Cleaner and more maintainable.
48-50: LGTM! Exposes supported types.Implements
getSupportedTypes()fromBlockConverterinterface. Returns defensive copy of supported types array.src/notebooks/deepnote/converters/textBlockConverter.unit.test.ts (4)
20-30: LGTM!New block type coverage is thorough and consistent.
44-53: LGTM!Correctly reflects all supported block types.
116-196: LGTM!New block type conversions correctly validate Markdown generation. Separator uses
<hr>consistently.
372-449: LGTM!New prefix-stripping tests are comprehensive. Separator correctly expects empty content.
andyjakubowski
left a comment
andyjakubowski
left a comment
There was a problem hiding this comment.
Left a few comments about reusing types from @deepnote/blocks rather than repeating them here.
| protected static readonly textBlockTypes = [ | ||
| 'text-cell-h1', | ||
| 'text-cell-h2', | ||
| 'text-cell-h3', | ||
| 'text-cell-p', | ||
| 'text-cell-bullet', | ||
| 'text-cell-todo', | ||
| 'text-cell-callout', | ||
| 'separator' | ||
| ]; |
There was a problem hiding this comment.
Are we cool with hardcoding these?
There was a problem hiding this comment.
Yeah, I think that will be okay.
| export interface DeepnoteBlock { | ||
| blockGroup?: string; | ||
| blockGroup: string; | ||
| content: string; | ||
| executionCount?: number; | ||
| id: string; |
There was a problem hiding this comment.
We export this from @deepnote/blocks, it’s a type derived from a Zod schema.
There was a problem hiding this comment.
I've updated it to use/derive the types from @deepnote/blocks.
| /** | ||
| * Represents a single block (cell) within a Deepnote notebook. | ||
| * Can be either a code block or a markdown block. | ||
| */ |
There was a problem hiding this comment.
Actually, the DeepnoteProject and DeepnoteNotebook types could also be derived from the Zod schema in @deepnote/blocks?
There was a problem hiding this comment.
I've updated it to use/derive the types from @deepnote/blocks.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
77-83: Avoid running the same license check twice.
npm run check-licensesstill runs inside the build job even though the newcheck_licensesjob covers it. You can drop this step to keep the pipeline lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
.github/workflows/ci.yml
[warning] 1-1: Code style issues found in .github/workflows/ci.yml. Run Prettier to fix.
[error] 1-1: Prettier formatting check failed during 'npm run format' (exit code 1). Run 'prettier --write' to fix code style issues.
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 106-106: too many blank lines (1 > 0)
(empty-lines)
⏰ 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: copilot-setup-steps
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Refactor