Skip to content

Conversation

@jankuca
Copy link
Contributor

@jankuca jankuca commented Oct 27, 2025

Summary by CodeRabbit

  • Bug Fixes
    • DuckDB dataframe integration is now properly configured and available for use in notebook environments.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 27, 2025

📝 Walkthrough

Walkthrough

The implementation adds explicit handling for the internal DuckDB dataframe integration when generating environment variables. When the integration ID matches the internal dataframe SQL integration, a special environment variable is created with a hardcoded DuckDB in-memory configuration. Other integrations continue using the existing config-to-JSON conversion approach. Internal DuckDB integration scanning is now enabled in cell traversal, and debug logging replaces info-level logging for environment variable assignments.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Provider as SqlIntegrationEnvironmentVariablesProvider
    participant NotebookScanner
    participant IntegrationConfig

    Caller->>Provider: getEnvironmentVariables()
    Provider->>NotebookScanner: scanCellsForIntegrationIds()
    NotebookScanner-->>Provider: integrationIds (including DATAFRAME_SQL_INTEGRATION_ID)
    
    loop For each integrationId
        alt Internal DuckDB Integration
            Provider->>Provider: Create env var with hardcoded<br/>deepnote+duckdb:///:memory: config
            Provider->>Provider: debug log assignment
        else Other Integration
            Provider->>IntegrationConfig: fetchConfig(integrationId)
            IntegrationConfig-->>Provider: config
            Provider->>Provider: convertIntegrationConfigToJson()
            Provider->>Provider: debug log assignment
        end
    end
    
    Provider-->>Caller: environment variables object
Loading

Possibly related PRs

Suggested reviewers

  • jamesbhobbs
  • Artmann
  • saltenasl

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 (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix: Dataframe SQL block support" is fully related to the main change in the changeset. The PR enables support for the internal DuckDB dataframe integration by adding proper environment variable handling instead of skipping it, which directly addresses fixing Dataframe SQL block support. The title is concise, clear, and accurately captures the primary objective without being vague or misleading.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e2fa9b5 and d94fc7b.

📒 Files selected for processing (2)
  • src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2 hunks)
  • src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (2 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/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
  • src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
  • src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
src/platform/**/*.ts

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

src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging

Files:

  • src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
  • src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.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 & Package Extension
  • GitHub Check: Lint & Format
  • GitHub Check: Build & Test
🔇 Additional comments (3)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2)

152-166: LGTM!

Special handling for internal DuckDB integration is well-structured. The hardcoded in-memory configuration is appropriate for the dataframe SQL use case.


181-181: LGTM!

Logging level adjustment to debug is appropriate and maintains consistency with the new internal integration logging at line 162.

src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (1)

67-85: LGTM!

Test correctly validates the internal DuckDB integration environment variable creation. Assertions comprehensively verify the JSON structure.


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

@codecov
Copy link

codecov bot commented Oct 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72%. Comparing base (e2fa9b5) to head (d94fc7b).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #118   +/-   ##
=====================================
  Coverage     72%     72%           
=====================================
  Files        536     536           
  Lines      40829   40837    +8     
  Branches    4990    4990           
=====================================
+ Hits       29588   29596    +8     
  Misses      9578    9578           
  Partials    1663    1663           
Files with missing lines Coverage Δ
...note/sqlIntegrationEnvironmentVariablesProvider.ts 86% <100%> (+<1%) ⬆️
...tegrationEnvironmentVariablesProvider.unit.test.ts 98% <100%> (+<1%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jankuca jankuca marked this pull request as ready for review October 27, 2025 10:30
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.

🚀

@saltenasl saltenasl merged commit 4382e25 into main Oct 27, 2025
12 checks passed
@saltenasl saltenasl deleted the jk/fix/df-sql-url branch October 27, 2025 10:47
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.

4 participants