Skip to content

Conversation

@Artmann
Copy link
Contributor

@Artmann Artmann commented Dec 9, 2025

Adding code blocks is handled by VS Code, so we can't modify that behaviour, so instead we check that the block has the correct settings after it's created.

Fixes #227

Summary by CodeRabbit

  • New Features

    • Deepnote notebooks now default newly added empty code cells to Python for a smoother editing experience.
  • Tests

    • Added unit tests covering language defaults, metadata exceptions, whitespace-only cells, multi-cell scenarios, and selective language conversion.
  • Chores

    • Registered the new activation service and updated test utilities/enums to support the new functionality.

✏️ Tip: You can customize this high-level summary in your review settings.

Adding code blocks is handled by the VS Code so we can't modify that behaviour, so instead we check that the block has the correct settings after it's created.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 9, 2025

📝 Walkthrough

Walkthrough

New Deepnote notebook service DeepnoteNewCellLanguageService was added and exported; it implements IExtensionSyncActivationService and registers on activation a workspace.onDidChangeNotebookDocument listener. The listener examines added cells in Deepnote notebooks, ignores non-code/markup or non-empty cells, preserves cells with __deepnotePocket metadata, and resets empty code cells that inherited a non‑Python language to Python via languages.setTextDocumentLanguage. The service is registered as a singleton activation service in both node and web service registries. Unit tests were added to validate activation, listener behavior, and language-change scenarios. A test mock enum member Markup = 1 was also introduced.

Sequence Diagram

sequenceDiagram
    participant ExtensionHost as Extension Host
    participant Service as DeepnoteNewCellLanguageService
    participant Workspace as VS Code Workspace
    participant NotebookDoc as Notebook Document
    participant Languages as VS Code languages API

    ExtensionHost->>Service: activate()
    Service->>Workspace: workspace.onDidChangeNotebookDocument(listener)
    Workspace-->>Service: listener registered

    Note over NotebookDoc: User adds one or more cells
    NotebookDoc->>Workspace: onDidChangeNotebookDocument(event)
    Workspace->>Service: invoke listener(event)

    Service->>Service: for each added cell: is Deepnote notebook?
    alt not Deepnote
        Service-->>Workspace: ignore
    else Deepnote
        Service->>Service: is cell Code and empty/whitespace-only?
        alt not Code or non-empty
            Service-->>Workspace: ignore
        else candidate
            Service->>Service: has __deepnotePocket metadata?
            alt has pocket (intentional SQL/JSON/Input)
                Service-->>Workspace: preserve language
            else no pocket
                Service->>Languages: setTextDocumentLanguage(cell.document, "python")
                Languages-->>Service: success / noop / error
            end
        end
    end
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 (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main change: ensuring code cells default to Python language instead of SQL.
Linked Issues check ✅ Passed The PR implementation correctly addresses #227 by adding post-creation checks that convert SQL/JSON cells to Python when intentional blocks aren't marked with __deepnotePocket metadata.
Out of Scope Changes check ✅ Passed All changes directly support the objective. The NotebookCellKind enum addition appears incidental to mock updates but is aligned with the service implementation.

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

@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0%. Comparing base (bb8c843) to head (c02ae82).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff     @@
##   main   #244   +/-   ##
===========================
===========================
🚀 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 1851599 and 4e4b181.

📒 Files selected for processing (5)
  • src/notebooks/deepnote/deepnoteNewCellLanguageService.ts (1 hunks)
  • src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts (1 hunks)
  • src/notebooks/serviceRegistry.node.ts (2 hunks)
  • src/notebooks/serviceRegistry.web.ts (2 hunks)
  • src/test/mocks/vsc/extHostedTypes.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}

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

**/*.{ts,tsx}: Use l10n.t() for all user-facing strings in TypeScript files
Use typed error classes from src/platform/errors/ instead of generic errors
Use ILogger service instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationToken

Order method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order

Files:

  • src/notebooks/deepnote/deepnoteNewCellLanguageService.ts
  • src/notebooks/serviceRegistry.web.ts
  • src/notebooks/serviceRegistry.node.ts
  • src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts
  • src/test/mocks/vsc/extHostedTypes.ts
**/*.ts

📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)

**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS run npm run format-fix before committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Use npm run format-fix to auto-fix TypeScript formatting issues before committing
Use npm run lint to check for linter issues in TypeScript files and attempt to fix before committing

Files:

  • src/notebooks/deepnote/deepnoteNewCellLanguageService.ts
  • src/notebooks/serviceRegistry.web.ts
  • src/notebooks/serviceRegistry.node.ts
  • src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts
  • src/test/mocks/vsc/extHostedTypes.ts
src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
Use Uri.joinPath() for constructing file paths instead of string concatenation with / to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., use import { generateUuid } from '../platform/common/uuid' instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports

Files:

  • src/notebooks/deepnote/deepnoteNewCellLanguageService.ts
  • src/notebooks/serviceRegistry.web.ts
  • src/notebooks/serviceRegistry.node.ts
  • src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts
  • src/test/mocks/vsc/extHostedTypes.ts
src/notebooks/deepnote/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration located in src/notebooks/deepnote/ with refactored architecture: deepnoteTypes.ts (type definitions), deepnoteNotebookManager.ts (state management), deepnoteNotebookSelector.ts (UI selection logic), deepnoteDataConverter.ts (data transformations), deepnoteSerializer.ts (main serializer/orchestration), deepnoteActivationService.ts (VSCode activation)

Files:

  • src/notebooks/deepnote/deepnoteNewCellLanguageService.ts
  • src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts
**/*.web.ts

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

Platform Implementation - Web: Use .web.ts files for browser-compatible APIs

Files:

  • src/notebooks/serviceRegistry.web.ts
**/*.node.ts

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

Platform Implementation - Desktop: Use .node.ts files for full file system access and Python environments

Files:

  • src/notebooks/serviceRegistry.node.ts
**/*.unit.test.ts

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

Create unit tests in *.unit.test.ts files

**/*.unit.test.ts: Unit tests use Mocha/Chai framework with .unit.test.ts extension
Test files should be placed alongside the source files they test
Use assert.deepStrictEqual() for object comparisons instead of checking individual properties

Files:

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

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

Create integration tests in *.test.ts files (not *.unit.test.ts)

**/*.test.ts: ALWAYS check the 'Unittest - Build' task output for TypeScript compilation errors before running any script or declaring work complete
When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefined then property to avoid hanging tests
Use npm run test:unittests for TypeScript unit tests before committing changes

Files:

  • src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteNewCellLanguageService.ts (2)
src/kernels/deepnote/types.ts (1)
  • DEEPNOTE_NOTEBOOK_TYPE (352-352)
src/platform/common/constants.ts (1)
  • PYTHON_LANGUAGE (4-4)
src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
  • Disposable (146-174)
src/test/vscode-mock.ts (2)
  • resetVSCodeMocks (57-305)
  • mockedVSCodeNamespaces (15-15)
🔇 Additional comments (5)
src/test/mocks/vsc/extHostedTypes.ts (1)

104-108: Enum alias for NotebookCellKind.Markup looks safe

Having both Markdown and Markup share the same numeric value keeps older tests working while allowing newer code to use Markup, with no behavioral change for enum comparisons.

src/notebooks/serviceRegistry.web.ts (1)

54-55: Web DI wiring for DeepnoteNewCellLanguageService is consistent

The new import and addSingleton<IExtensionSyncActivationService>(..., DeepnoteNewCellLanguageService) follow the existing activation-service pattern and mirror the node registration; nothing else needed here.

Also applies to: 127-130

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

16-22: Activation and disposable management are correct

Injecting IDisposableRegistry and pushing the workspace.onDidChangeNotebookDocument subscription into it ensures the handler is cleaned up with the rest of the extension disposables.

src/notebooks/serviceRegistry.node.ts (1)

85-86: Node DI registration for DeepnoteNewCellLanguageService is correctly placed

Importing the service and registering it as an IExtensionSyncActivationService alongside other Deepnote kernel services keeps the activation lifecycle consistent across node and web.

Also applies to: 217-220

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

100-305: Test coverage for new-cell language behavior is thorough

The suite exercises all key branches: notebook type filtering, cell kind/content checks, __deepnotePocket metadata, multiple added cells and content changes, and the “no added cells” path, giving high confidence in the service logic.

@Artmann Artmann marked this pull request as ready for review December 9, 2025 11:18
@Artmann Artmann requested a review from a team as a code owner December 9, 2025 11:18
coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 9, 2025
coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 9, 2025
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 ea540d3 and 27071c5.

📒 Files selected for processing (1)
  • src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.unit.test.ts

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

Create unit tests in *.unit.test.ts files

**/*.unit.test.ts: Unit tests use Mocha/Chai framework with .unit.test.ts extension
Test files should be placed alongside the source files they test
Use assert.deepStrictEqual() for object comparisons instead of checking individual properties

Files:

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

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

Create integration tests in *.test.ts files (not *.unit.test.ts)

**/*.test.ts: ALWAYS check the 'Unittest - Build' task output for TypeScript compilation errors before running any script or declaring work complete
When a mock is returned from a promise in unit tests, ensure the mocked instance has an undefined then property to avoid hanging tests
Use npm run test:unittests for TypeScript unit tests before committing changes

Files:

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

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

**/*.{ts,tsx}: Use l10n.t() for all user-facing strings in TypeScript files
Use typed error classes from src/platform/errors/ instead of generic errors
Use ILogger service instead of console.log for logging
Preserve error details in error messages while scrubbing personally identifiable information (PII)
Prefer async/await over promise chains
Handle cancellation with CancellationToken

Order method, fields and properties first by accessibility (public/private/protected) and then by alphabetical order

Files:

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

📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)

**/*.ts: ALWAYS check the 'Core - Build' task output for TypeScript compilation errors before running any script or declaring work complete
ALWAYS run npm run format-fix before committing changes to ensure proper code formatting
FIX all TypeScript compilation errors before moving forward with development
Use npm run format-fix to auto-fix TypeScript formatting issues before committing
Use npm run lint to check for linter issues in TypeScript files and attempt to fix before committing

Files:

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

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{ts,tsx}: Don't add the Microsoft copyright header to new files
Use Uri.joinPath() for constructing file paths instead of string concatenation with / to ensure platform-correct path separators
Follow established patterns when importing new packages, using helper imports rather than direct imports (e.g., use import { generateUuid } from '../platform/common/uuid' instead of importing uuid directly)
Add blank lines after const groups and before return statements for readability
Separate third-party and local file imports

Files:

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

📄 CodeRabbit inference engine (CLAUDE.md)

Deepnote integration located in src/notebooks/deepnote/ with refactored architecture: deepnoteTypes.ts (type definitions), deepnoteNotebookManager.ts (state management), deepnoteNotebookSelector.ts (UI selection logic), deepnoteDataConverter.ts (data transformations), deepnoteSerializer.ts (main serializer/orchestration), deepnoteActivationService.ts (VSCode activation)

Files:

  • src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
  • Disposable (146-174)
src/test/vscode-mock.ts (2)
  • resetVSCodeMocks (57-305)
  • mockedVSCodeNamespaces (15-15)
⏰ 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 (1)
src/notebooks/deepnote/deepnoteNewCellLanguageService.unit.test.ts (1)

1-78: Well-structured test setup and helpers.

Mock wiring correctly captures the change handler, and activation tests verify listener registration and disposable tracking.

@Artmann Artmann merged commit 722b206 into main Dec 10, 2025
14 checks passed
@Artmann Artmann deleted the chris/fix-code-blocks-as-sql branch December 10, 2025 10:22
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.

Sometimes add SQL blocks instead of Code blocks

3 participants