-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add SQL block rendering as editor #72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
📝 WalkthroughWalkthroughAdds 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 || ''
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (6)**/!(*.node|*.web).ts📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.unit.test.ts📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{test,spec}.ts📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
Files:
src/notebooks/deepnote/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/notebooks/deepnote/deepnoteDataConverter.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (4)📚 Learning: 2025-10-09T11:21:57.494ZApplied to files:
📚 Learning: 2025-09-03T12:59:14.489ZApplied to files:
📚 Learning: 2025-10-09T11:21:57.494ZApplied to files:
📚 Learning: 2025-09-16T13:05:15.049ZApplied to files:
🧬 Code graph analysis (2)src/notebooks/deepnote/deepnoteDataConverter.unit.test.ts (1)
src/notebooks/deepnote/deepnoteDataConverter.ts (2)
⏰ 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 (4)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 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
🚀 New features to boost your workflow:
|
There was a problem hiding this 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
📒 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
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/notebooks/deepnote/converters/sqlBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/sqlBlockConverter.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 (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.tssrc/notebooks/deepnote/converters/sqlBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/sqlBlockConverter.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: 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.tssrc/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
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/deepnoteDataConverter.unit.test.tssrc/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.tssrc/notebooks/deepnote/converters/sqlBlockConverter.unit.test.tssrc/notebooks/deepnote/converters/sqlBlockConverter.tssrc/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.tssrc/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.tssrc/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
createPythonCodefrom@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.
src/notebooks/deepnote/converters/sqlBlockConverter.unit.test.ts
Outdated
Show resolved
Hide resolved
fb6350c to
d28645c
Compare
saltenasl
left a comment
There was a problem hiding this 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.
| 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`; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
738a2b8
d28645c to
738a2b8
Compare
|
@coderabbitai pause |
|
@coderabbitai review |
✅ Actions performedReviews paused. |
✅ Actions performedReview triggered.
|
Resolves GRN-4954
Summary by CodeRabbit