Skip to content

Conversation

@jankuca
Copy link
Member

@jankuca jankuca commented Nov 4, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Notebook creation now gracefully handles optional runtime features, avoiding errors when certain integration points are unavailable and improving stability during notebook setup.
  • Tests

    • Added unit tests covering controller creation across scenarios: feature present, feature absent, and feature-attachment failures to ensure silent, safe degradation.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 4, 2025

📝 Walkthrough

Walkthrough

A guard is added around attaching the Jupyter variableProvider to the notebook controller: the code checks at runtime whether controller.variableProvider exists before assigning and performs the assignment inside a try/catch that logs a warning on failure; if absent the assignment is skipped. Comments document the API's proposed status and the opt-in flag requirement. Unit tests were added covering API-available, API-unavailable, and assignment-throws scenarios. An exported type IConnectionDisplayData was added to src/notebooks/controllers/types.

Sequence Diagram(s)

mermaid
sequenceDiagram
autonumber
participant Creator as ControllerFactory
participant VSCode as NotebookController (runtime)
participant Logger as Logger

Creator->>VSCode: create controller
Note right of VSCode #DDEBF7: Runtime presence check for variableProvider
VSCode-->>VSCode: if ('variableProvider' in controller)
alt variableProvider exists
VSCode->>VSCode: try assign variableProvider
VSCode-->>Logger: on error -> warn("assignment failed")
Note right of VSCode #E8F8E0: assignment inside try/catch
else not present
Note right of VSCode #F7F2F2: skip assignment (graceful degradation)
end
Creator->>VSCode: return controller

mermaid
sequenceDiagram
autonumber
participant Creator as ControllerFactory
participant VSCode as NotebookController (old)
participant Logger as Logger

Creator->>VSCode: create controller
Note right of VSCode #FFF7E6: Previous unconditional assignment
VSCode->>VSCode: try assign variableProvider (unconditionally)
VSCode-->>Logger: on error -> warn("assignment failed")
Creator->>VSCode: return controller

Pre-merge checks

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title accurately describes the main change: adding defensive checks around VS Code API proposal usage in the variable provider attachment logic.

📜 Recent 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 d7b31e8 and c3e9a64.

📒 Files selected for processing (1)
  • src/notebooks/controllers/vscodeNotebookController.unit.test.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/controllers/vscodeNotebookController.unit.test.ts
🧬 Code graph analysis (1)
src/notebooks/controllers/vscodeNotebookController.unit.test.ts (3)
src/notebooks/controllers/types.ts (3)
  • IConnectionDisplayDataProvider (135-135)
  • IConnectionDisplayDataProvider (136-138)
  • IConnectionDisplayData (126-133)
src/kernels/variables/types.ts (2)
  • IJupyterVariablesProvider (149-149)
  • IJupyterVariablesProvider (150-150)
src/notebooks/controllers/vscodeNotebookController.ts (1)
  • VSCodeNotebookController (96-820)
🔇 Additional comments (3)
src/notebooks/controllers/vscodeNotebookController.unit.test.ts (3)

40-40: LGTM - Imports are correct.

The new imports support the variable provider tests.

Also applies to: 45-45


549-609: LGTM - Test suite setup is comprehensive.

All dependencies properly mocked and disposed.


611-758: LGTM - Test cases cover all scenarios.

The three tests validate:

  • Variable provider attached when API available
  • Graceful degradation when API missing
  • Error handling when assignment throws

Each test uses an appropriate mocking strategy for its scenario.


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

@jankuca jankuca force-pushed the jk/fix/variable-provider-error branch from cb95c8e to e10370b Compare November 4, 2025 13:29
@codecov
Copy link

codecov bot commented Nov 4, 2025

Codecov Report

❌ Patch coverage is 92.45283% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 73%. Comparing base (b47df83) to head (c3e9a64).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../controllers/vscodeNotebookController.unit.test.ts 91% 4 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #174   +/-   ##
=====================================
  Coverage     73%     73%           
=====================================
  Files        574     574           
  Lines      46813   46863   +50     
  Branches    5519    5520    +1     
=====================================
+ Hits       34358   34410   +52     
+ Misses     10637   10635    -2     
  Partials    1818    1818           
Files with missing lines Coverage Δ
.../notebooks/controllers/vscodeNotebookController.ts 36% <100%> (+1%) ⬆️
.../controllers/vscodeNotebookController.unit.test.ts 95% <91%> (-1%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai[bot]
coderabbitai bot previously approved these changes Nov 4, 2025
@jankuca jankuca force-pushed the jk/fix/variable-provider-error branch from 5719f60 to d7b31e8 Compare November 4, 2025 13:42
coderabbitai[bot]
coderabbitai bot previously approved these changes Nov 4, 2025
@jankuca jankuca marked this pull request as ready for review November 4, 2025 14:11
@jankuca jankuca requested a review from a team as a code owner November 4, 2025 14:11
@saltenasl saltenasl merged commit 2910974 into main Nov 4, 2025
13 checks passed
@saltenasl saltenasl deleted the jk/fix/variable-provider-error branch November 4, 2025 14:39
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