Skip to content

Conversation

@tkislan
Copy link
Member

@tkislan tkislan commented Oct 31, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Preserve and correctly decode markdown outputs when converting notebook data so markdown appears alongside other output formats in the correct order.
  • Tests
    • Added coverage to verify conversion preserves both markdown and plain-text outputs within execute_result/display_data.

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

📝 Walkthrough

Walkthrough

The changes extend the Deepnote↔VS Code notebook conversion to preserve markdown outputs. transformOutputsForDeepnote now decodes items with mime type text/markdown and stores them under data['text/markdown']. transformOutputsForVsCode reconstructs a separate markdown output item from data['text/markdown'] when present, inserting it after image outputs and before text/plain items so markdown is preserved and correctly ordered during round-trip conversion.

Sequence Diagram(s)

sequenceDiagram
  participant VSCode
  participant Converter
  participant Deepnote

  VSCode->>Converter: send notebook outputs (may include\n- image/*\n- text/markdown\n- text/plain)
  Note right of Converter: transformOutputsForDeepnote
  Converter-->>Deepnote: store data fields\n(e.g. data['image/*'], data['text/markdown'], data['text/plain'])

  Deepnote->>Converter: return stored block data
  Note right of Converter: transformOutputsForVsCode\n(reconstruct outputs in order:\n1) images\n2) text/markdown\n3) text/plain)
  Converter-->>VSCode: reconstructed outputs (image, markdown, plain)
Loading

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix: Fix markdown outputs not rendering" directly aligns with the changeset's main objective. The PR adds support for decoding and propagating text/markdown mime-type in Deepnote data conversion, which addresses the exact issue described in the title. The title is specific and clear about the problem domain (markdown outputs), making it easy for reviewers to understand the primary change at a glance. While there's minor redundancy in the phrasing ("fix: Fix"), this follows common conventional patterns and doesn't detract from clarity.

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

@codecov
Copy link

codecov bot commented Oct 31, 2025

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72%. Comparing base (9f0d02b) to head (bc46670).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/notebooks/deepnote/deepnoteDataConverter.ts 50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #147   +/-   ##
=====================================
  Coverage     72%     72%           
=====================================
  Files        545     545           
  Lines      41777   41795   +18     
  Branches    5047    5049    +2     
=====================================
+ Hits       30431   30447   +16     
- Misses      9665    9666    +1     
- Partials    1681    1682    +1     
Files with missing lines Coverage Δ
...ebooks/deepnote/deepnoteDataConverter.unit.test.ts 100% <100%> (ø)
src/notebooks/deepnote/deepnoteDataConverter.ts 59% <50%> (-1%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 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 9f0d02b and 4527b4a.

📒 Files selected for processing (1)
  • src/notebooks/deepnote/deepnoteDataConverter.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.ts
src/notebooks/deepnote/**

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.ts
src/notebooks/deepnote/deepnoteDataConverter.ts

📄 CodeRabbit inference engine (CLAUDE.md)

deepnoteDataConverter.ts performs Deepnote data transformations

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteDataConverter.ts (1)
src/test/mocks/vsc/extHostedTypes.ts (1)
  • NotebookCellOutputItem (16-69)
⏰ 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

…verter

Signed-off-by: Tomas Kislan <tomas@kislan.sk>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

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 4527b4a and bc46670.

📒 Files selected for processing (1)
  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Place shared cross-platform logic in common .ts files (not suffixed with .node or .web)

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Use l10n.t() for user-facing strings
Use typed error classes from src/platform/errors/ when throwing or handling errors
Use the ILogger service instead of console.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 with CancellationToken

**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.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: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.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 then property to avoid hanging tests

Files:

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

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration code resides under src/notebooks/deepnote/

Files:

  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1)
src/platform/deepnote/deepnoteTypes.ts (1)
  • DeepnoteOutput (23-51)
⏰ 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: Build & Test

@tkislan tkislan marked this pull request as ready for review October 31, 2025 11:44
@tkislan tkislan requested a review from a team as a code owner October 31, 2025 11:44
@saltenasl saltenasl merged commit f564456 into main Oct 31, 2025
13 checks passed
@saltenasl saltenasl deleted the tk/fix-markdown-not-displaying branch October 31, 2025 11:49
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