-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Add commands to create Deepnote block cells #75
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
feat: Add commands to create Deepnote block cells #75
Conversation
…tomaskislan/grn-4762-support-big-number-blocks
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…maskislan/grn-4776-support-input-blocks
…maskislan/grn-4776-support-input-blocks
…tomaskislan/grn-4762-support-big-number-blocks
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
♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (1)
178-317: Private method ordering.Per guidelines, private methods should be alphabetical:
addBigNumberChartBlock,addInputBlock,addSqlBlock,registerCommands.As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.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/deepnoteNotebookCommandListener.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/deepnoteNotebookCommandListener.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (3)
src/notebooks/deepnote/deepnoteSchemas.ts (2)
DeepnoteTextInputMetadataSchema(77-86)DeepnoteBigNumberMetadataSchema(11-44)src/kernels/execution/notebookUpdater.ts (1)
chainWithPendingUpdates(24-52)src/test/mocks/vsc/extHostedTypes.ts (3)
NotebookCellData(2523-2549)NotebookEdit(2444-2507)NotebookRange(2413-2443)
⏰ 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). (3)
- GitHub Check: Build & Test
- GitHub Check: Build & Package Extension
- GitHub Check: Lint & Format
…er.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.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/deepnoteNotebookCommandListener.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/deepnoteNotebookCommandListener.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
⏰ 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). (3)
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
- GitHub Check: Build & Package Extension
🔇 Additional comments (6)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (6)
75-83: Clean type validation with Zod safeParse.The function correctly validates the parsed JSON value as a string type before returning.
85-128: Variable name generation logic is sound.The function correctly collects existing variable names from multiple metadata sources and computes the next sequential name.
144-176: Command registration follows standard pattern.All commands properly registered with disposable tracking.
178-224: SQL block insertion logic is correct.The method properly computes the insert position, creates appropriate metadata, and handles the cell insertion with proper timing for selection updates.
226-269: Big number block insertion mirrors SQL block pattern.Consistent implementation with proper metadata initialization and selection handling.
271-317: Input block insertion generalizes across block types.The parameterized approach cleanly handles all input block variants using
getInputBlockMetadata.
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.
thanks!
Artmann
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.
🚀
…expand-add-new-blocks-functionality-to-support-our-new
…erations and improve error handling - Changed private methods to public async methods for adding SQL, big number chart, and input blocks, enhancing the ability to handle asynchronous operations. - Improved error handling by throwing errors when no active notebook editor is found or when cell insertion fails. - Updated the logic for cell insertion to ensure proper selection and editing of newly created cells. - Enhanced unit tests to cover new async behavior and error scenarios. Signed-off-by: Tomas Kislan <tomas@kislan.sk>
efeabe4
Signed-off-by: Tomas Kislan <tomas@kislan.sk>
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: 6
♻️ Duplicate comments (1)
src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts (1)
67-86: Add idempotence test on same instance or rename test.This test compares two instances. Either add a test that calls activate() twice on the same instance (ensuring no duplicate registrations), or rename to reflect current scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
cspell.json(1 hunks)src/notebooks/deepnote/deepnoteNotebookCommandListener.ts(1 hunks)src/notebooks/deepnote/deepnoteNotebookCommandListener.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
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.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/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteNotebookCommandListener.tssrc/notebooks/deepnote/deepnoteNotebookCommandListener.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/deepnoteNotebookCommandListener.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/deepnoteNotebookCommandListener.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (4)
src/notebooks/deepnote/deepnoteSchemas.ts (1)
DeepnoteBigNumberMetadataSchema(11-44)src/platform/logging/index.ts (1)
logger(35-48)src/kernels/execution/notebookUpdater.ts (1)
chainWithPendingUpdates(24-52)src/test/mocks/vsc/extHostedTypes.ts (3)
NotebookCellData(2523-2549)NotebookEdit(2444-2507)NotebookRange(2413-2443)
src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts (2)
src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (2)
getNextDeepnoteVariableName(85-128)InputBlockType(33-42)src/test/datascience/editor-integration/helpers.ts (1)
createMockedNotebookDocument(159-204)
🔇 Additional comments (5)
cspell.json (1)
27-27: No issues found. "datascience" is a legitimate codebase identifier.The entry correctly adds "datascience" to the spell-check allowlist. It's used throughout the codebase as a directory name (
src/test/datascience/) and technical identifier (vscode_datascience_helpers), not as an English prose term. Alphabetical ordering is correct (between "dataframe" and "deepnote").src/notebooks/deepnote/deepnoteNotebookCommandListener.ts (3)
75-83: Good defensive parsing and logging.The zod-based safeParse avoids type leaks and noisy errors. Looks solid.
201-207: Metadata placement consistency.SQL block sets defaults both at top‑level and under __deepnotePocket; input and big‑number set only __deepnotePocket. Confirm spec—should input/big‑number also duplicate top‑level keys, or should SQL drop the top‑level copy?
Also applies to: 236-241, 279-284
188-193: Verify sql_integration_id policy.Constant 'deepnote-dataframe-sql' is hardcoded. If workspace/env may vary, consider resolving from settings or integrations service; otherwise, add a comment explaining why it’s fixed.
src/notebooks/deepnote/deepnoteNotebookCommandListener.unit.test.ts (1)
101-313: Thorough variable‑name tests.Great coverage across prefixes, mixed sources, invalid JSON, and large numbers.
Signed-off-by: Tomas Kislan <tomas@kislan.sk>
- Updated error handling in the DeepnoteNotebookCommandListener to use localized messages for better user experience. - Adjusted logic for variable name suffix checks to ensure correct prefix usage. Signed-off-by: Tomas Kislan <tomas@kislan.sk>
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.
Thanks for taking the time to add tests! 🚀

Summary by CodeRabbit
New Features
Other Changes
Tests