Skip to content

Conversation

@jankuca
Copy link
Contributor

@jankuca jankuca commented Oct 16, 2025

Resolves GRN-4954

Screenshot 2025-10-16 at 14 38 51

Summary by CodeRabbit

  • New Features
    • Deepnote notebook integration now recognizes and renders SQL cells as SQL code, preserving content and metadata through edits and conversions.
  • Tests
    • Added unit tests to validate SQL block conversion, language assignment, multi-line content handling, and round‑trip preservation of content and metadata.

@linear
Copy link

linear bot commented Oct 16, 2025

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 16, 2025

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.
📝 Walkthrough

Walkthrough

Adds SQL block support to the Deepnote notebook conversion pipeline. A new SqlBlockConverter implements BlockConverter: it recognizes blocks with type "sql" (case-insensitive), converts them into Notebook code cells with languageId "sql" and content from block.content (or empty string when undefined), and writes edits back from a NotebookCellData to block.content. SqlBlockConverter is registered in DeepnoteDataConverter (registration order: CodeBlockConverter, MarkdownBlockConverter, SqlBlockConverter, TextBlockConverter). Unit tests cover detection, conversion (including multiline/empty/undefined content), metadata preservation, and applyChangesToBlock behavior.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant DD as DeepnoteDataConverter
  participant SC as SqlBlockConverter
  participant OC as OtherConverters
  participant B as DeepnoteBlock
  participant C as NotebookCellData

  rect rgba(220,235,255,0.6)
    note over DD: Registration sequence
    DD->>DD: register(CodeBlockConverter)
    DD->>DD: register(MarkdownBlockConverter)
    DD->>SC: register(SqlBlockConverter)
    DD->>DD: register(TextBlockConverter)
  end

  note over B,DD: Convert blocks -> cells
  DD->>SC: canConvert(B.type?)
  alt B.type is "sql"
    SC-->>DD: true
    DD->>SC: convertToCell(B)
    SC-->>DD: C (kind: Code, languageId: "sql", value: B.content || '')
    DD->>C: attach __deepnotePocket metadata (id/type/sortingKey) and preserve other metadata
  else other type
    DD->>OC: delegate conversion
  end

  note over C,SC: Edit round-trip (cell -> block)
  C->>DD: applyChanges request
  DD->>SC: applyChangesToBlock(B, C)
  SC-->>DD: B.content = C.value || ''
Loading

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning While the pull request adds support for rendering SQL blocks as code editors, it omits any implementation of executing SQL block contents via the required dntk.execute_sql() call, failing to address the execution aspect of the linked issue. Add integration with dntk.execute_sql() to invoke SQL execution for SQL blocks and include corresponding tests to satisfy the linked issue requirements.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title concisely and accurately summarizes the primary change of introducing SQL block rendering in the editor, following conventional commit style and avoiding irrelevant details.
Out of Scope Changes Check ✅ Passed All modifications focus on adding support for SQL block conversion and registration without introducing unrelated features or modifications, so no out-of-scope changes are present.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d28645c and 738a2b8.

📒 Files selected for processing (4)
  • src/notebooks/deepnote/converters/sqlBlockConverter.ts (1 hunks)
  • src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts (1 hunks)
  • src/notebooks/deepnote/deepnoteDataConverter.ts (2 hunks)
  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.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
  • src/notebooks/deepnote/converters/sqlBlockConverter.ts
  • src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts
  • 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.unit.test.ts
  • src/notebooks/deepnote/converters/sqlBlockConverter.ts
  • src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts
  • src/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: 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
  • src/notebooks/deepnote/converters/sqlBlockConverter.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/converters/sqlBlockConverter.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
  • src/notebooks/deepnote/converters/sqlBlockConverter.ts
  • src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts
  • 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
🧠 Learnings (4)
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs Deepnote data transformations

Applied to files:

  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts
  • src/notebooks/deepnote/converters/sqlBlockConverter.ts
  • src/notebooks/deepnote/deepnoteDataConverter.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/sqlBlockConverter.ts
  • src/notebooks/deepnote/deepnoteDataConverter.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to **/*.unit.test.ts : Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties

Applied to files:

  • src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts
📚 Learning: 2025-09-16T13:05:15.049Z
Learnt from: jankuca
PR: deepnote/deepnote#18415
File: libs/shared/src/deepnote-ai/ai-chat-schema.test.ts:1-5
Timestamp: 2025-09-16T13:05:15.049Z
Learning: In the deepnote project, while assert import is restricted in libs/shared/.eslintrc.json, jankuca confirms that assert is allowed in test files according to project conventions, overriding the ESLint restriction.

Applied to files:

  • src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1)
src/notebooks/deepnote/deepnoteTypes.ts (1)
  • DeepnoteBlock (6-6)
src/notebooks/deepnote/deepnoteDataConverter.ts (2)
src/notebooks/deepnote/converters/sqlBlockConverter.ts (1)
  • SqlBlockConverter (16-37)
src/notebooks/deepnote/converters/textBlockConverter.ts (1)
  • TextBlockConverter (7-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
🔇 Additional comments (4)
src/notebooks/deepnote/converters/sqlBlockConverter.ts (1)

16-37: LGTM! Clean implementation.

The converter correctly handles SQL blocks with case-insensitive type matching, proper cell creation with 'sql' languageId, and safe handling of undefined content.

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

62-87: LGTM! Comprehensive test coverage.

The test verifies SQL block conversion including content preservation, correct cell type/language, and metadata propagation (id, type, sortingKey, sql_integration_id).

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

9-10: LGTM! Appropriate registration order.

SqlBlockConverter is correctly registered before TextBlockConverter, ensuring SQL blocks are matched by their specific converter rather than falling through to the generic text converter.

Also applies to: 22-23

src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts (1)

1-199: LGTM! Thorough test coverage.

Comprehensive unit tests covering all SqlBlockConverter methods with edge cases (empty/undefined content, case-insensitive matching, multi-line SQL, metadata preservation). Uses Chai assertions and deepStrictEqual for object comparisons as per coding guidelines.


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

@codecov
Copy link

codecov bot commented Oct 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70%. Comparing base (635883e) to head (738a2b8).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff          @@
##            main     #72   +/-   ##
=====================================
  Coverage     70%     70%           
=====================================
  Files        516     518    +2     
  Lines      38178   38270   +92     
  Branches    4854    4856    +2     
=====================================
+ Hits       27068   27160   +92     
  Misses      9505    9505           
  Partials    1605    1605           
Files with missing lines Coverage Δ
...notebooks/deepnote/converters/sqlBlockConverter.ts 100% <100%> (ø)
...deepnote/converters/sqlBlockConverter.unit.test.ts 100% <100%> (ø)
src/notebooks/deepnote/deepnoteDataConverter.ts 56% <100%> (+<1%) ⬆️
...ebooks/deepnote/deepnoteDataConverter.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.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 635883e and 24ed4a7.

📒 Files selected for processing (4)
  • src/notebooks/deepnote/converters/sqlBlockConverter.ts (1 hunks)
  • src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts (1 hunks)
  • src/notebooks/deepnote/deepnoteDataConverter.ts (2 hunks)
  • src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.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
  • src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts
  • src/notebooks/deepnote/converters/sqlBlockConverter.ts
  • 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.unit.test.ts
  • src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts
  • src/notebooks/deepnote/converters/sqlBlockConverter.ts
  • src/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: 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
  • src/notebooks/deepnote/converters/sqlBlockConverter.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/converters/sqlBlockConverter.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
  • src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts
  • src/notebooks/deepnote/converters/sqlBlockConverter.ts
  • 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
🧠 Learnings (2)
📚 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/sqlBlockConverter.ts
  • src/notebooks/deepnote/deepnoteDataConverter.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteDataConverter.ts : deepnoteDataConverter.ts performs Deepnote data transformations

Applied to files:

  • src/notebooks/deepnote/converters/sqlBlockConverter.ts
  • src/notebooks/deepnote/deepnoteDataConverter.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1)
src/notebooks/deepnote/deepnoteTypes.ts (1)
  • DeepnoteBlock (6-6)
src/notebooks/deepnote/deepnoteDataConverter.ts (2)
src/notebooks/deepnote/converters/sqlBlockConverter.ts (1)
  • SqlBlockConverter (16-37)
src/notebooks/deepnote/converters/textBlockConverter.ts (1)
  • TextBlockConverter (7-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
🔇 Additional comments (2)
src/notebooks/deepnote/converters/sqlBlockConverter.ts (2)

6-15: Verify SQL execution implementation.

Documentation mentions execution via createPythonCode from @deepnote/blocks, and the PR objective states SQL blocks should "execute their contents by invoking dntk.execute_sql()". This converter handles rendering only.

Confirm the execution layer is implemented elsewhere or planned for a follow-up PR.


17-36: LGTM!

Clean implementation following the BlockConverter interface pattern. Handles bidirectional conversion correctly with case-insensitive type detection.

@jankuca jankuca force-pushed the jk/feat/sql-block-editor branch 2 times, most recently from fb6350c to d28645c Compare October 16, 2025 13:02
coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 16, 2025
@jankuca jankuca marked this pull request as ready for review October 16, 2025 13:06
saltenasl
saltenasl previously approved these changes Oct 16, 2025
Copy link
Member

@saltenasl saltenasl left a comment

Choose a reason for hiding this comment

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

Block is rendered nicely, the syntax is highlighted and I get the same error as in the PR description.

Comment on lines 87 to 96
const sqlQuery = `SELECT
u.name,
u.email,
COUNT(o.id) as order_count
FROM users u
LEFT JOIN orders o ON u.id = o.user_id
WHERE u.created_at > '2024-01-01'
GROUP BY u.id, u.name, u.email
ORDER BY order_count DESC
LIMIT 10`;
Copy link
Member

Choose a reason for hiding this comment

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

nit dedent would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated. i need a new approval it seems - is the setup really that any push after an approval requires another approval?

Copy link
Member

Choose a reason for hiding this comment

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

yes

@jankuca jankuca dismissed stale reviews from saltenasl and coderabbitai[bot] via 738a2b8 October 16, 2025 14:34
@jankuca jankuca force-pushed the jk/feat/sql-block-editor branch from d28645c to 738a2b8 Compare October 16, 2025 14:34
@jankuca
Copy link
Contributor Author

jankuca commented Oct 16, 2025

@coderabbitai pause

@jankuca jankuca enabled auto-merge (squash) October 16, 2025 14:35
@jankuca
Copy link
Contributor Author

jankuca commented Oct 16, 2025

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 16, 2025

✅ Actions performed

Reviews paused.

@jankuca jankuca merged commit 0ac510e into main Oct 16, 2025
10 checks passed
@jankuca jankuca deleted the jk/feat/sql-block-editor branch October 16, 2025 14:39
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 16, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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