Skip to content

Conversation

@jankuca
Copy link
Member

@jankuca jankuca commented Nov 4, 2025

Problem

Markdown blocks in .deepnote files were being saved with type=code instead of retaining their correct type=markdown. This was causing data corruption in the serialization/deserialization process.

Root Cause

The createBlockFromPocket function in src/platform/deepnote/pocket.ts had a hardcoded fallback to type='code' when the pocket metadata was missing or didn't contain a type field. This happened because VSCode may not preserve the __deepnotePocket metadata when cells are edited.

Solution

Modified createBlockFromPocket to infer the block type from the cell's kind property when the pocket metadata doesn't have a type:

  • If cell.kind === NotebookCellKind.Code, default to type='code'
  • If cell.kind === NotebookCellKind.Markup, default to type='markdown'

Changes

  • Modified src/platform/deepnote/pocket.ts to use cell kind as fallback for block type
  • Added unit tests in src/platform/deepnote/pocket.unit.test.ts to verify the fix
  • Added integration test in src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts

Testing

✅ All 1932 unit tests pass
✅ New tests specifically verify markdown cells without pocket metadata retain type=markdown
✅ Ensures round-trip fidelity for markdown blocks


Pull Request opened by Augment Code with guidance from the PR author

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of markdown cells to ensure proper conversion to markdown blocks based on cell type when metadata lacks explicit type information.
  • Tests

    • Added test coverage for markdown cell conversion scenarios, including cases with and without metadata.

- Modified createBlockFromPocket to infer block type from cell kind when pocket metadata is missing
- Markdown cells (NotebookCellKind.Markup) now correctly default to type='markdown' instead of type='code'
- Added unit tests to verify markdown type preservation
- Ensures round-trip fidelity for markdown blocks even when __deepnotePocket metadata is not preserved
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

📝 Walkthrough

Walkthrough

These changes extend the Deepnote converter to properly handle markdown cells without pocket metadata. The production code now infers a default block type—markdown for markup cells, code otherwise—when pocket metadata lacks an explicit type. Three new unit tests verify this behavior across different scenarios: markdown conversion without pocket data, and markdown blocks with partial pocket metadata.

Suggested reviewers

  • andyjakubowski

Pre-merge checks

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title accurately describes the main fix: markdown blocks incorrectly saved as type=code are now corrected to type=markdown.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b47df83 and 126d30a.

📒 Files selected for processing (3)
  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1 hunks)
  • src/platform/deepnote/pocket.ts (2 hunks)
  • src/platform/deepnote/pocket.unit.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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 then property to avoid hanging tests

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
  • src/platform/deepnote/pocket.unit.test.ts
src/platform/**/*.ts

📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)

src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging

Files:

  • src/platform/deepnote/pocket.unit.test.ts
  • src/platform/deepnote/pocket.ts
🧬 Code graph analysis (3)
src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (1)
  • NotebookCellData (2546-2572)
src/platform/deepnote/pocket.unit.test.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
  • NotebookCellData (2546-2572)
src/platform/deepnote/pocket.ts (1)
  • createBlockFromPocket (49-86)
src/platform/deepnote/pocket.ts (1)
src/notebooks/deepnote/dataConversionUtils.ts (2)
  • generateBlockId (16-23)
  • generateSortingKey (28-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: codecov/project
  • GitHub Check: Build & Test
🔇 Additional comments (6)
src/platform/deepnote/pocket.ts (3)

1-1: LGTM—needed for the cell.kind check.


67-70: Logic is sound—handles both cell kinds correctly.

The ternary correctly maps Code→'code' and Markup→'markdown'. VSCode currently has only these two cell kinds, so the fallback is safe.


78-78: Core fix looks correct—uses pocket.type when present, otherwise infers from cell.kind.

src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1)

179-192: Test coverage is solid—verifies markdown cells without pocket metadata convert correctly.

Confirms the fix works through the full conversion flow.

src/platform/deepnote/pocket.unit.test.ts (2)

193-199: LGTM—unit test confirms markdown inference when pocket is absent.


201-216: Excellent coverage—tests the partial pocket scenario where type is missing.

Confirms markdown inference while preserving other pocket fields (id, sortingKey).


Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Nov 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73%. Comparing base (2910974) to head (36cc6f5).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #176   +/-   ##
=====================================
  Coverage     73%     73%           
=====================================
  Files        574     574           
  Lines      46863   46881   +18     
  Branches    5520    5521    +1     
=====================================
+ Hits       34410   34428   +18     
  Misses     10635   10635           
  Partials    1818    1818           
Files with missing lines Coverage Δ
...ebooks/deepnote/deepnoteDataConverter.unit.test.ts 100% <100%> (ø)
src/platform/deepnote/pocket.ts 100% <100%> (ø)
src/platform/deepnote/pocket.unit.test.ts 100% <100%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jankuca
Copy link
Member Author

jankuca commented Nov 4, 2025

I can also confirm it is now saving the blocks as markdown to the .deepnote file:

Screenshot 2025-11-04 at 15 20 31

@jankuca jankuca changed the title Fix markdown blocks being saved as type=code in .deepnote files fix: markdown blocks being saved as type=code in .deepnote files Nov 4, 2025
@jankuca jankuca marked this pull request as ready for review November 4, 2025 14:23
@jankuca jankuca requested a review from a team as a code owner November 4, 2025 14:23
@saltenasl saltenasl enabled auto-merge (squash) November 4, 2025 14:43
@saltenasl saltenasl merged commit 3e42395 into main Nov 4, 2025
13 checks passed
@saltenasl saltenasl deleted the jk/fix/markdown-save branch November 4, 2025 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants