feat: migrate to the deepnote/database-integrations package #158
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs 📝 WalkthroughWalkthroughProject integrations were migrated to the centralized @deepnote/database-integrations model. Storage now persists versioned, encrypted DatabaseIntegrationConfig entries, upgrades legacy configs, validates metadata, and exposes new APIs (getAll, getIntegrationConfig, getProjectIntegrationConfig, clear). Environment-variable generation and kernel provisioning now gather all project integrations (including internal DuckDB) and call a single converter (getEnvironmentVariablesForIntegrations). UI, webviews, and forms were updated to DatabaseIntegrationConfig/DatabaseIntegrationType (added Snowflake support); cell-scanning helpers were removed and platform notebook/project abstractions were introduced. Sequence Diagram(s)sequenceDiagram
participant Kernel as Kernel Startup
participant EnvProv as SqlIntegrationEnvVarsProvider
participant Editor as IPlatformNotebookEditorProvider
participant Manager as IPlatformDeepnoteNotebookManager
participant Storage as IIntegrationStorage
participant Converter as getEnvironmentVariablesForIntegrations
rect rgb(220,240,255)
Note over EnvProv,Kernel: Kernel requests env vars for a resource
Kernel->>EnvProv: requestEnvVars(resourceUri)
EnvProv->>Editor: findAssociatedNotebookDocument(resourceUri)
Editor-->>EnvProv: NotebookDocument? / undefined
EnvProv->>Manager: getOriginalProject(projectId)
Manager-->>EnvProv: DeepnoteProject? / undefined
EnvProv->>Storage: getAll() / getProjectIntegrationConfig(...)
Storage-->>EnvProv: DatabaseIntegrationConfig[] (includes internal DuckDB)
EnvProv->>Converter: getEnvironmentVariablesForIntegrations(configs)
Converter-->>EnvProv: EnvironmentVariables map
EnvProv-->>Kernel: env vars
end
sequenceDiagram
participant Storage as IntegrationStorage
participant Encrypted as IEncryptedStorage
participant Upgrader as upgradeLegacyIntegrationConfig
participant Validator as databaseMetadataSchemasByType
rect rgb(240,230,230)
Note over Storage: Load/init path with upgrade, validate, persist
Storage->>Encrypted: read index + entries
Encrypted-->>Storage: raw JSON entries
loop per entry
alt legacy (no version)
Storage->>Upgrader: upgradeLegacyIntegrationConfig(raw)
Upgrader-->>Storage: DatabaseIntegrationConfig | null
else already versioned
Storage->>Validator: validate metadata
Validator-->>Storage: valid / invalid
end
alt valid
Storage->>Encrypted: storeEncryptedIntegrationConfig(versioned config)
Storage-->>Storage: update in-memory cache
else invalid
Storage->>Encrypted: delete entry & update index
end
end
end
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
jankuca
force-pushed
the
jk/integrations-package
branch
from
5e51842 to
2303399
Compare
jankuca
force-pushed
the
jk/integrations-package
branch
from
2303399 to
4a7c399
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
======================================
Coverage 73% 73%
======================================
Files 550 552 +2
Lines 44030 44264 +234
Branches 5314 5313 -1
======================================
+ Hits 32215 32498 +283
+ Misses 10048 10003 -45
+ Partials 1767 1763 -4
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
INTEGRATIONS_CREDENTIALS.md (1)
358-359: Docs contradict new credential scopeLines 219-224 now describe exposing environment variables for all configured integrations, yet here we still promise “Each notebook gets only the credentials it needs,” and the Execution Flow section still talks about scanning the current notebook. Please update the doc to reflect the new behavior (no per-notebook scan, project-wide exposure) so readers aren’t misled.
📜 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.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
INTEGRATIONS_CREDENTIALS.md(1 hunks)package.json(1 hunks)src/notebooks/deepnote/integrations/integrationDetector.ts(2 hunks)src/notebooks/deepnote/integrations/integrationManager.ts(2 hunks)src/notebooks/deepnote/integrations/integrationUtils.ts(0 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(4 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.ts(3 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(3 hunks)src/notebooks/serviceRegistry.node.ts(3 hunks)src/platform/notebooks/deepnote/integrationStorage.ts(4 hunks)src/platform/notebooks/deepnote/integrationTypes.ts(7 hunks)src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts(3 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts(0 hunks)src/platform/notebooks/deepnote/types.ts(4 hunks)src/webviews/webview-side/integrations/BigQueryForm.tsx(3 hunks)src/webviews/webview-side/integrations/ConfigurationForm.tsx(2 hunks)src/webviews/webview-side/integrations/IntegrationItem.tsx(1 hunks)src/webviews/webview-side/integrations/IntegrationPanel.tsx(5 hunks)src/webviews/webview-side/integrations/PostgresForm.tsx(8 hunks)src/webviews/webview-side/integrations/SnowflakeForm.tsx(5 hunks)src/webviews/webview-side/integrations/types.ts(2 hunks)
💤 Files with no reviewable changes (2)
- src/notebooks/deepnote/integrations/integrationUtils.ts
- src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.node.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.node.tsfor Desktop-specific implementations that require full file system access and Python environments
Files:
src/notebooks/serviceRegistry.node.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.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 withCancellationToken
Files:
src/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/platform/notebooks/deepnote/legacyIntegrationConfigUtils.tssrc/webviews/webview-side/integrations/IntegrationItem.tsxsrc/webviews/webview-side/integrations/BigQueryForm.tsxsrc/notebooks/deepnote/integrations/integrationDetector.tssrc/webviews/webview-side/integrations/PostgresForm.tsxsrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/webviews/webview-side/integrations/SnowflakeForm.tsxsrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/webviews/webview-side/integrations/types.tssrc/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/webviews/webview-side/integrations/ConfigurationForm.tsxsrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/platform/notebooks/deepnote/integrationStorage.ts
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/platform/notebooks/deepnote/legacyIntegrationConfigUtils.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/webviews/webview-side/integrations/types.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/platform/notebooks/deepnote/integrationStorage.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.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
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.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/legacyIntegrationConfigUtils.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/platform/notebooks/deepnote/types.tssrc/platform/notebooks/deepnote/integrationStorage.ts
🧬 Code graph analysis (14)
src/notebooks/serviceRegistry.node.ts (2)
src/notebooks/types.ts (4)
INotebookEditorProvider(14-14)INotebookEditorProvider(15-20)IDeepnoteNotebookManager(37-37)IDeepnoteNotebookManager(38-58)src/platform/notebooks/deepnote/types.ts (4)
IPlatformNotebookEditorProvider(86-86)IPlatformNotebookEditorProvider(87-89)IPlatformDeepnoteNotebookManager(96-96)IPlatformDeepnoteNotebookManager(97-99)
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.ts (1)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
LegacyIntegrationConfig(133-137)
src/webviews/webview-side/integrations/IntegrationItem.tsx (4)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
IntegrationWithStatus(151-163)src/webviews/webview-side/integrations/types.ts (1)
IntegrationWithStatus(5-11)src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
getIntegrationTypeLabel(433-444)src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
src/webviews/webview-side/react-common/locReactSide.ts (2)
getLocString(11-17)format(23-25)
src/notebooks/deepnote/integrations/integrationDetector.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/platform/notebooks/deepnote/integrationTypes.ts (1)
IntegrationWithStatus(151-163)
src/webviews/webview-side/integrations/PostgresForm.tsx (1)
src/webviews/webview-side/react-common/locReactSide.ts (2)
getLocString(11-17)format(23-25)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (5)
src/platform/notebooks/deepnote/types.ts (4)
IPlatformNotebookEditorProvider(86-86)IPlatformNotebookEditorProvider(87-89)IPlatformDeepnoteNotebookManager(96-96)IPlatformDeepnoteNotebookManager(97-99)src/platform/common/types.ts (1)
Resource(19-19)src/platform/common/variables/types.ts (1)
EnvironmentVariables(8-8)src/platform/logging/index.ts (1)
logger(35-48)src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)
src/webviews/webview-side/integrations/SnowflakeForm.tsx (2)
src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (3)
SnowflakeAuthMethod(13-13)SnowflakeAuthMethods(4-11)isSupportedSnowflakeAuthMethod(31-33)src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
src/webviews/webview-side/integrations/types.ts (1)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
IntegrationWithStatus(151-163)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
src/webviews/webview-side/integrations/ConfigurationForm.tsx (1)
ConfigurationForm(17-81)
src/webviews/webview-side/integrations/ConfigurationForm.tsx (3)
src/webviews/webview-side/integrations/PostgresForm.tsx (1)
PostgresForm(34-241)src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
BigQueryForm(29-167)src/webviews/webview-side/integrations/SnowflakeForm.tsx (1)
SnowflakeForm(38-440)
src/platform/notebooks/deepnote/types.ts (1)
src/platform/deepnote/deepnoteTypes.ts (1)
DeepnoteProject(12-12)
src/platform/notebooks/deepnote/integrationStorage.ts (3)
src/platform/notebooks/deepnote/types.ts (2)
IIntegrationStorage(27-27)IIntegrationStorage(28-66)src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.ts (1)
upgradeLegacyIntegrationConfig(10-89)
🪛 GitHub Check: Lint & Format
src/platform/notebooks/deepnote/integrationStorage.ts
[warning] 209-209:
'_version' is assigned a value but never used
⏰ 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). (2)
- GitHub Check: Build & Test
- GitHub Check: Build & Package Extension
🔇 Additional comments (1)
src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
36-51: State reset covers config churn.
structuredCloneplus the empty-config factory keeps the draft synced when the id or default name changes. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 12
📜 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.
📒 Files selected for processing (7)
src/messageTypes.ts(1 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(5 hunks)src/platform/common/utils/localize.ts(1 hunks)src/platform/notebooks/deepnote/integrationStorage.unit.test.ts(1 hunks)src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts(1 hunks)src/webviews/webview-side/integrations/ConfigurationForm.tsx(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
.tsfiles (not suffixed with.nodeor.web)
Files:
src/platform/notebooks/deepnote/integrationStorage.unit.test.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/platform/common/utils/localize.tssrc/messageTypes.tssrc/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.tssrc/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
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.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 withCancellationToken
Files:
src/platform/notebooks/deepnote/integrationStorage.unit.test.tssrc/webviews/webview-side/integrations/ConfigurationForm.tsxsrc/notebooks/deepnote/integrations/integrationWebview.tssrc/platform/common/utils/localize.tssrc/messageTypes.tssrc/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.tssrc/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
Files:
src/platform/notebooks/deepnote/integrationStorage.unit.test.tssrc/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.tssrc/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/integrationStorage.unit.test.tssrc/platform/common/utils/localize.tssrc/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.tssrc/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
thenproperty to avoid hanging tests
Files:
src/platform/notebooks/deepnote/integrationStorage.unit.test.tssrc/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
🧬 Code graph analysis (5)
src/platform/notebooks/deepnote/integrationStorage.unit.test.ts (1)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)
src/webviews/webview-side/integrations/ConfigurationForm.tsx (4)
src/webviews/webview-side/integrations/PostgresForm.tsx (1)
PostgresForm(34-241)src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
BigQueryForm(29-167)src/webviews/webview-side/integrations/SnowflakeForm.tsx (1)
SnowflakeForm(38-440)src/webviews/webview-side/react-common/locReactSide.ts (2)
getLocString(11-17)format(23-25)
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts (2)
src/platform/notebooks/deepnote/integrationTypes.ts (3)
LegacyPostgresIntegrationConfig(51-59)LegacyBigQueryIntegrationConfig(64-68)LegacySnowflakeIntegrationConfig(106-128)src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.ts (1)
upgradeLegacyIntegrationConfig(10-89)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (4)
src/platform/deepnote/deepnoteTypes.ts (1)
DeepnoteProject(12-12)src/platform/notebooks/deepnote/types.ts (6)
IIntegrationStorage(27-27)IIntegrationStorage(28-66)IPlatformNotebookEditorProvider(86-86)IPlatformNotebookEditorProvider(87-89)IPlatformDeepnoteNotebookManager(96-96)IPlatformDeepnoteNotebookManager(97-99)src/test/mocks/vsc/index.ts (2)
EventEmitter(65-89)CancellationTokenSource(114-125)src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)
🪛 GitHub Actions: CD
src/platform/notebooks/deepnote/integrationStorage.unit.test.ts
[error] 110-110: TS18048: 'result' is possibly 'undefined'.
🪛 GitHub Actions: CI
src/platform/notebooks/deepnote/integrationStorage.unit.test.ts
[error] 239-239: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises) (Command: 'npm run lint')
🪛 GitHub Check: Lint & Format
src/platform/notebooks/deepnote/integrationStorage.unit.test.ts
[failure] 360-360:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator
[failure] 290-290:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator
[failure] 239-239:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator
🪛 GitHub Check: TypeCheck
src/platform/notebooks/deepnote/integrationStorage.unit.test.ts
[failure] 680-680:
'bigquery' is possibly 'undefined'.
[failure] 677-677:
'postgres1' is possibly 'undefined'.
[failure] 390-390:
'result' is possibly 'undefined'.
[failure] 389-389:
'result' is possibly 'undefined'.
[failure] 189-189:
'result' is possibly 'undefined'.
[failure] 160-160:
'result' is possibly 'undefined'.
[failure] 136-136:
'result' is possibly 'undefined'.
[failure] 112-112:
'result' is possibly 'undefined'.
[failure] 111-111:
'result' is possibly 'undefined'.
[failure] 110-110:
'result' is possibly 'undefined'.
🪛 Gitleaks (8.28.0)
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts
[high] 100-221: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 236-249: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (9)
src/messageTypes.ts (1)
238-238: LGTM!Webview localization key properly added.
src/platform/common/utils/localize.ts (1)
834-834: LGTM!Extension-side localization helper correctly implemented with
l10n.t().src/webviews/webview-side/integrations/ConfigurationForm.tsx (3)
2-2: LGTM!Imports updated for database-integrations migration and localization needs.
Also applies to: 6-6
8-15: LGTM!Props updated for database-integrations types. Making
integrationTyperequired eliminates inference complexity.
41-81: LGTM!Rendering logic correctly handles all integration types with proper type narrowing. Localization of unsupported-type fallback (lines 74-78) addresses the past review comment.
src/notebooks/deepnote/integrations/integrationWebview.ts (4)
10-11: Import migration aligns with PR objectives.The updated imports correctly reference types from the new package structure.
180-181: LGTM: Localization strings for edge cases.The placeholder format is appropriate for webview string interpolation.
217-221: Type migration applied correctly.The DatabaseIntegrationConfig type aligns with the new package. Runtime validation is assumed to occur in integrationStorage.save() or webview-side.
262-262: LGTM: Consistent type migration.The parameter type change is consistent with the broader migration and properly propagates through the save flow.
src/platform/notebooks/deepnote/integrationStorage.unit.test.ts
Outdated
Show resolved
Hide resolved
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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 (1)
INTEGRATIONS_CREDENTIALS.md (1)
340-343: Exhaustively document environment variable naming rules.Line 340 states variables are "uppercased, special chars replaced with
_" but doesn't clarify:
- Exact character replacement rules (which chars? only non-alphanumeric?)
- Examples of IDs with special chars
- Edge cases (leading underscores, consecutive underscores, etc.)
Add concrete examples like
my-postgres-db→SQL_MY_POSTGRES_DBanddb.v2→SQL_DB_V2.
📜 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.
📒 Files selected for processing (1)
INTEGRATIONS_CREDENTIALS.md(12 hunks)
⏰ 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)
INTEGRATIONS_CREDENTIALS.md (3)
179-179: Clarify DuckDB integration handling in detector vs. provider.Line 179 states the detector excludes
deepnote-dataframe-sql, but line 332 states the environment variables provider always includes it. This creates confusion about the intended behavior. Explicitly explain:
- Why the detector excludes it (scope/filtering logic)
- Why the provider includes it (initialization needed in kernel)
- How this distinction benefits the overall architecture
Also applies to: 332-332
50-50: Clarify credential storage scope.Line 50 states storage is "scoped to the user's machine (shared across all Deepnote projects)", but line 336 states credentials are "project-scoped". Reconcile these statements:
- Are credentials stored user-wide and filtered per-project at retrieval?
- Or are they replicated per-project?
This affects user understanding of security isolation and credential visibility.
Also applies to: 336-336
18-40: Package exports verified—all documented items are real and in use.All six exports from
@deepnote/database-integrationsv1.1.0 are confirmed:
DatabaseIntegrationConfig,DatabaseIntegrationTypeimported across multiple integration filesgetEnvironmentVariablesForIntegrations()used in sqlIntegrationEnvironmentVariablesProviderdatabaseMetadataSchemasByTypeimported and actively used (pgsql, big-query, snowflake schemas) in legacyIntegrationConfigUtils and integrationStorageBigQueryAuthMethods,SnowflakeAuthMethodsimported in respective form componentsDocumentation is accurate.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts (1)
243-259: Verify exactauthMethodvalue.Line 258 only checks existence. The test at line 233 verifies the exact value—this test should too.
Apply this diff:
assert.ok(result); assert.strictEqual(result.type, 'snowflake'); - assert.ok(result.metadata.authMethod); + assert.strictEqual(result.metadata.authMethod, 'service-account-key-pair'); });src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (1)
1-1: Complete the assert import fix.The import should be
import { strict as assert } from 'assert'orimport assert from 'assert/strict'to accessdeepStrictEqualused throughout the tests.Apply this diff:
-import assert from 'assert'; +import { strict as assert } from 'assert';
📜 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.
📒 Files selected for processing (3)
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.ts(1 hunks)src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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
.tsfiles (not suffixed with.nodeor.web)
Files:
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.tssrc/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.tssrc/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
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.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 withCancellationToken
Files:
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.tssrc/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.tssrc/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/legacyIntegrationConfigUtils.tssrc/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.tssrc/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
Files:
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.tssrc/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
thenproperty to avoid hanging tests
Files:
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
🧬 Code graph analysis (3)
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.ts (1)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
LegacyIntegrationConfig(133-137)
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts (2)
src/platform/notebooks/deepnote/integrationTypes.ts (3)
LegacyPostgresIntegrationConfig(51-59)LegacyBigQueryIntegrationConfig(64-68)LegacySnowflakeIntegrationConfig(106-128)src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.ts (1)
upgradeLegacyIntegrationConfig(10-89)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (4)
src/platform/deepnote/deepnoteTypes.ts (1)
DeepnoteProject(12-12)src/platform/notebooks/deepnote/types.ts (6)
IIntegrationStorage(27-27)IIntegrationStorage(28-66)IPlatformNotebookEditorProvider(86-86)IPlatformNotebookEditorProvider(87-89)IPlatformDeepnoteNotebookManager(96-96)IPlatformDeepnoteNotebookManager(97-99)src/test/mocks/vsc/index.ts (2)
EventEmitter(65-89)CancellationTokenSource(114-125)src/platform/notebooks/deepnote/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)
🪛 Gitleaks (8.28.0)
src/platform/notebooks/deepnote/legacyIntegrationConfigUtils.unit.test.ts
[high] 101-223: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
[high] 238-251: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (4)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (4)
12-32: Clean helper for test data.Well-structured utility that creates minimal, properly-typed test fixtures.
42-62: Proper test lifecycle management.Setup wires mocks correctly, teardown disposes all resources.
252-361: Thorough validation of env var generation.These tests verify actual env var names, JSON structure, and connection details. Good coverage across PostgreSQL, BigQuery, and DuckDB.
365-384: Event test works, though setTimeout can be flaky.The test correctly verifies the event fires when integration storage changes. The 100ms timeout fallback is reasonable for a unit test.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Show resolved
Hide resolved
|
@coderabbitai resolve |
saltenasl
left a comment
saltenasl
left a comment
There was a problem hiding this comment.
dataframe SQL works, postgres works, snowflake now also works after @deepnote/database-integrations upgrade to 1.1.1. Thanks!
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
1 similar comment
✅ Actions performedComments resolved and changes approved. |
Summary by CodeRabbit
New Features
Chores
Tests
Docs