fix: Fix SQL dataframe integration

[tkislan]

duckdb integration was not set up when there were no other integrations in deepnote file

Summary by CodeRabbit

• Refactor

  • Initialization changed so the internal dataframe SQL integration is created once up front instead of during per-integration processing.

• Tests

  • Unit tests updated to expect the internal dataframe environment entry by default and adjusted counts/comments where applicable to reflect the new initialization behavior.

[coderabbitai]

Warning

Rate limit exceeded

@tkislan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 27 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between b10def7 and ca4a168.

📒 Files selected for processing (1)

• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (9 hunks)

📝 Walkthrough

Walkthrough

This PR moves creation of the internal DuckDB (dataframe) SQL integration environment variable out of the per-integration loop and into the start of getEnvironmentVariables. The provider now injects a single, fixed JSON-payload env var (name derived from SQL_DATAFRAME_SQL_INTEGRATION_ID) before processing other integrations; the per-integration loop skips the dataframe integration. Unit tests were updated to expect the internal dataframe env var even when no other SQL integrations exist, and counts in tests were adjusted accordingly.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller as getEnvironmentVariables
    participant Env as EnvVarsCollector
    participant Integrations as IntegrationsList

    Note over Caller,Env: New flow (after PR)
    Caller->>Env: create INTERNAL_DF env var (fixed JSON)
    Caller->>Integrations: fetch integrations
    loop for each integration
        Integrations->>Caller: integration
        alt integration is internal dataframe
            Caller->>Integrations: continue (skip)
        else other integration
            Caller->>Env: add per-integration env vars
        end
    end
    Caller->>Env: return collected env vars

Loading

sequenceDiagram
    autonumber
    participant Caller as getEnvironmentVariables
    participant Env as EnvVarsCollector
    participant Integrations as IntegrationsList

    Note over Caller,Env: Old flow (before PR)
    Caller->>Integrations: fetch integrations
    loop for each integration
        Integrations->>Caller: integration
        alt integration is internal dataframe
            Caller->>Env: add INTERNAL_DF env var (inside loop)
        else other integration
            Caller->>Env: add per-integration env vars
        end
    end
    Caller->>Env: return collected env vars

Loading

Possibly related PRs

• fix: Dataframe SQL block support #118: Modifies the same provider and tests to add an environment variable for the internal DuckDB (dataframe) SQL integration (related change but placed inside the loop).
• feat: SQL block credentials propagation to Jupyter kernel #78: Touches SqlIntegrationEnvironmentVariablesProvider and the internal DuckDB/dataframe env var handling that this PR refactors.

Suggested reviewers

• saltenasl
• jamesbhobbs
• Artmann

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 SQL dataframe integration" directly and specifically describes the main change in this pull request. The changeset refactors how the internal DuckDB (dataframe) SQL integration credentials are injected, moving them outside the conditional loop to ensure they're always set up regardless of other integrations being present. This aligns perfectly with the PR objective of fixing the duckdb integration setup when no other integrations are present, and the title is concise and clear without unnecessary verbosity or vagueness.

---

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

[codecov]

Codecov Report

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

Additional details and impacted files

@@          Coverage Diff           @@
##            main    #139    +/-   ##
======================================
  Coverage     72%     72%            
======================================
  Files        541     541            
  Lines      41481   41596   +115     
  Branches    5027    5031     +4     
======================================
+ Hits       30209   30323   +114     
  Misses      9593    9593            
- Partials    1679    1680     +1     

Files with missing lines	Coverage Δ
...note/sqlIntegrationEnvironmentVariablesProvider.ts	89% <100%> (ø)
...tegrationEnvironmentVariablesProvider.unit.test.ts	99% <100%> (+<1%)	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

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

52-63: Update test name - no longer returns empty object.

The test name claims "Returns empty object" but now expects the dataframe integration. Update to reflect the actual behavior.

Apply this diff:

-    test('Returns empty object when notebook has no SQL cells', async () => {
+    test('Returns dataframe integration when notebook has no SQL cells', async () => {

---

65-75: Update test name - no longer returns empty object.

Same issue: the test name claims "Returns empty object" but expects the dataframe integration.

Apply this diff:

-    test('Returns empty object when SQL cells have no integration ID', async () => {
+    test('Returns dataframe integration when SQL cells have no integration ID', async () => {

📜 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 065b4c0 and 0af50ab.

📒 Files selected for processing (2)

• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2 hunks)
• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (9 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

🧬 Code graph analysis (1)

src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2)

src/platform/notebooks/deepnote/integrationTypes.ts (1)

• DATAFRAME_SQL_INTEGRATION_ID (5-5)

src/platform/logging/index.ts (1)

• logger (35-48)

⏰ 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 (3)

src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2)

202-213: LGTM - Dataframe integration now always present.

The refactoring ensures the internal DuckDB integration is set up regardless of other integrations, resolving the stated issue.

---

234-237: Correct handling of internal integration.

Skipping the dataframe integration during the loop is appropriate since it's already been added.

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

17-19: Good use of constant for expected values.

The constant correctly matches the hardcoded dataframe integration from the provider.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

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

52-63: Update test name to reflect actual behavior.

The test name says "Returns empty object" but now returns the internal DuckDB integration. Consider renaming to reflect that the dataframe integration is always present.

-    test('Returns empty object when notebook has no SQL cells', async () => {
+    test('Returns only dataframe integration when notebook has no SQL cells', async () => {

---

65-75: Update test name to reflect actual behavior.

The test name says "Returns empty object" but now returns the internal DuckDB integration.

-    test('Returns empty object when SQL cells have no integration ID', async () => {
+    test('Returns only dataframe integration when SQL cells have no integration ID', async () => {

📜 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 0af50ab and ebfbc3f.

📒 Files selected for processing (1)

• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (9 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

**/*.{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

**/*.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

**/*.{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

🔇 Additional comments (6)

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

17-19: LGTM!

Constant clearly defines the baseline environment variables that always include the internal DuckDB integration.

---

193-195: LGTM!

Comment correctly clarifies that the count includes the dataframe integration plus the PostgreSQL integration. Past review concern has been addressed.

---

237-240: LGTM!

Comment correctly clarifies that the count includes the dataframe integration plus two other integrations. Past review concern has been addressed.

---

654-654: LGTM!

Assertion correctly updated to expect the dataframe integration for unsupported auth methods.

---

678-678: LGTM!

Assertion correctly updated to expect the dataframe integration for unsupported auth methods.

---

628-630: Update comment to reflect actual behavior.

The comment says "Should return empty object" but the assertion expects the dataframe integration to be present.

-            // Should return empty object when unsupported auth method is encountered
+            // Should return only dataframe integration when unsupported auth method is encountered
             const envVars = await provider.getEnvironmentVariables(uri);
             assert.deepStrictEqual(envVars, EXPECTED_EMPTY_ENV_VARS);

Likely an incorrect or invalid review comment.

[coderabbitai]

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 ebfbc3f and b10def7.

📒 Files selected for processing (1)

• src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (9 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

**/*.{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

**/*.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

**/*.{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

🔇 Additional comments (5)

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

193-195: Comment update looks good.

The updated comment correctly reflects that there are 2 total env vars (dataframe + postgres). Past review issue has been addressed.

---

237-240: Comment update looks good.

The updated comment correctly reflects that there are 3 total env vars (dataframe + postgres + bigquery). Past review issue has been addressed.

---

258-259: Comment update looks good.

The updated comment accurately describes the behavior. Past review issue has been addressed.

---

62-62: Assertions correctly updated.

All test assertions now properly expect the dataframe integration to be present in baseline scenarios. Changes are consistent with the PR objectives.

Also applies to: 74-74, 628-630, 654-654, 678-678

---

39-50: Early-exit tests are correct as written.

The production code confirms early exits occur before dataframe integration is added. All three early-exit scenarios (undefined resource, notebook not found, cancellation) return {} at lines 182-199, and dataframe integration is added only after at lines 202-212. Tests correctly expect {} for these conditions.