Skip to content

Conversation

@tkislan
Copy link
Member

@tkislan tkislan commented Oct 17, 2025

image

Summary by CodeRabbit

  • New Features

    • New Deepnote commands to insert blocks: Add SQL, Big Number Chart, Input Text, Input Textarea, Input Select, Input Slider, Input Checkbox, Input Date, Input Date Range, Input File, and Button. SQL block added to the notebook toolbar for Deepnote notebooks.
    • Localization strings added for all new commands.
  • Other Changes

    • Big Number chart rendering now respects configured formatting.
    • Spellcheck vocabulary and test mocks updated to support new reveal modes.
  • Tests

    • Unit tests added covering command activation, block insertion, and variable-naming logic.

Artmann and others added 30 commits October 8, 2025 16:07
…tomaskislan/grn-4762-support-big-number-blocks
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…tomaskislan/grn-4762-support-big-number-blocks
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

♻️ 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

📥 Commits

Reviewing files that changed from the base of the PR and between 88c4b00 and 7439292.

📒 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 .ts files (not suffixed with .node or .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
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/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>
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 7439292 and 2b7a8fe.

📒 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 .ts files (not suffixed with .node or .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
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/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.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 25, 2025
@tkislan tkislan enabled auto-merge (squash) October 25, 2025 11:27
@tkislan
Copy link
Member Author

tkislan commented Oct 25, 2025

image Automatically sets integration id to dataframe sql now doesn't actually work right now .. but that is most likely out of scope of this task

@tkislan tkislan requested a review from saltenasl October 25, 2025 11:29
saltenasl
saltenasl previously approved these changes Oct 27, 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.

thanks!

Artmann
Artmann previously approved these changes Oct 27, 2025
Copy link
Member

@Artmann Artmann left a 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>
@tkislan tkislan dismissed stale reviews from Artmann, saltenasl, and coderabbitai[bot] via efeabe4 October 27, 2025 13:33
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: 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2b7a8fe and 80b7492.

📒 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 .ts files (not suffixed with .node or .web)

Files:

  • src/notebooks/deepnote/deepnoteNotebookCommandListener.ts
  • src/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
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/deepnoteNotebookCommandListener.ts
  • src/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.ts
  • src/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 then property 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>
@tkislan tkislan requested a review from saltenasl October 27, 2025 14:44
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.

Thanks for taking the time to add tests! 🚀

@saltenasl saltenasl disabled auto-merge October 28, 2025 08:21
@saltenasl saltenasl merged commit 71dffde into main Oct 28, 2025
13 checks passed
@saltenasl saltenasl deleted the tomaskislan/grn-4784-expand-add-new-blocks-functionality-to-support-our-new branch October 28, 2025 08:21
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.

5 participants