feat: Add Snowflake SQL integration support #121
Conversation
- Add Snowflake integration type with username+password and key-pair auth methods - Create SnowflakeForm UI component with auth method switcher - Add connection string generation for both auth methods - Add all localization strings for Snowflake form fields - Update integration type mappings and labels
📝 WalkthroughWalkthroughAdds Snowflake integration end-to-end: new localization keys; Snowflake auth constants and supported-method guard; IntegrationType and IntegrationConfig extended for Snowflake; SnowflakeForm React component with validation and submission; webview loc payload and label mapping updated; sqlIntegrationEnvironmentVariablesProvider extended to build Snowflake connection URLs/params (with unit tests); IntegrationManager now resolves integration name/type via IDeepnoteNotebookManager; new UnsupportedIntegrationError and error-category addition. Sequence Diagram(s)sequenceDiagram
participant User
participant Webview
participant Loc as Localization
participant Types as IntegrationTypes
participant Form as SnowflakeForm
participant Auth as SnowflakeAuthConstants
participant EnvProv as EnvVarsProvider
participant Manager as IntegrationManager
participant NotebookMgr as IDeepnoteNotebookManager
rect rgba(150,200,255,0.12)
Webview->>Loc: request locStrings (includes Snowflake keys)
Loc-->>Webview: localized labels/placeholders
Webview->>Types: resolve integration type (infer from id/config)
Types-->>Webview: 'snowflake' (when applicable)
end
rect rgba(200,255,200,0.12)
Webview->>Form: render SnowflakeForm (existingConfig?)
Form->>Auth: isSupportedSnowflakeAuthMethod?
Auth-->>Form: supported / unsupported
User->>Form: fill fields & submit
Form-->>Webview: SnowflakeIntegrationConfig
end
rect rgba(255,230,150,0.12)
Webview->>EnvProv: convert config -> env JSON
EnvProv->>Auth: validate auth method
alt supported
EnvProv-->>Webview: { url, params, param_style }
else unsupported
EnvProv-->>Webview: throws UnsupportedIntegrationError
end
Webview->>Manager: register integration (id, config)
Manager->>NotebookMgr: getOriginalProject(projectId)
NotebookMgr-->>Manager: project metadata (integration name/type)
Manager->>Manager: attach resolved name/type to integration entry
end
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
src/messageTypes.ts(2 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(2 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.ts(1 hunks)src/platform/common/utils/localize.ts(2 hunks)src/platform/notebooks/deepnote/integrationTypes.ts(3 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts(2 hunks)src/webviews/webview-side/integrations/ConfigurationForm.tsx(4 hunks)src/webviews/webview-side/integrations/IntegrationItem.tsx(1 hunks)src/webviews/webview-side/integrations/SnowflakeForm.tsx(1 hunks)src/webviews/webview-side/integrations/types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.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/webviews/webview-side/integrations/types.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/messageTypes.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/platform/common/utils/localize.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
**/*.{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/webviews/webview-side/integrations/types.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/webviews/webview-side/integrations/IntegrationItem.tsxsrc/webviews/webview-side/integrations/SnowflakeForm.tsxsrc/messageTypes.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/integrations/ConfigurationForm.tsxsrc/platform/common/utils/localize.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.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/platform/common/utils/localize.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/integrations/integrationWebview.ts
🧬 Code graph analysis (6)
src/webviews/webview-side/integrations/types.ts (1)
src/platform/notebooks/deepnote/integrationTypes.ts (5)
SnowflakeIntegrationConfig(78-92)BaseIntegrationConfig(39-43)IntegrationConfig(97-97)PostgresIntegrationConfig(48-56)BigQueryIntegrationConfig(61-65)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (1)
src/webviews/webview-side/integrations/types.ts (2)
IntegrationType(1-1)SnowflakeAuthMethod(5-5)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
src/webviews/webview-side/integrations/types.ts (6)
SnowflakeAuthMethod(5-5)SnowflakeIntegrationConfig(29-43)BaseIntegrationConfig(7-11)IntegrationConfig(45-45)PostgresIntegrationConfig(13-21)BigQueryIntegrationConfig(23-27)
src/webviews/webview-side/integrations/IntegrationItem.tsx (1)
src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
src/webviews/webview-side/integrations/SnowflakeForm.tsx (2)
src/webviews/webview-side/integrations/types.ts (2)
SnowflakeIntegrationConfig(29-43)SnowflakeAuthMethod(5-5)src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
🪛 Biome (2.1.2)
src/webviews/webview-side/integrations/SnowflakeForm.tsx
[error] 118-118: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
🔇 Additional comments (16)
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
134-189: LGTM!Snowflake localization keys are comprehensive and follow the established pattern.
src/webviews/webview-side/integrations/IntegrationItem.tsx (1)
17-18: LGTM!Snowflake label case follows the established pattern.
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
440-441: LGTM!Snowflake label case matches the pattern.
src/webviews/webview-side/integrations/ConfigurationForm.tsx (1)
5-5: LGTM!SnowflakeForm integration follows the established pattern for Postgres and BigQuery forms.
Also applies to: 26-26, 41-43, 74-90
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (3)
92-109: Verify empty password handling.Line 94 allows empty password with
config.password || ''. Confirm this is intentional, as empty passwords may fail authentication.
110-134: Verify empty private key handling.Line 130 allows empty private key with
config.privateKey || ''. Confirm this is intentional, as empty keys will fail authentication.
81-141: URL encoding and authentication logic looks correct.The Snowflake connection URL construction properly encodes credentials and handles both authentication methods. Query parameters and the params object are correctly structured.
src/webviews/webview-side/integrations/types.ts (1)
1-1: Type definitions are well-structured.IntegrationType extension, SnowflakeAuthMethod, and IntegrationConfig union are correctly defined.
Also applies to: 5-5, 45-45
src/platform/notebooks/deepnote/integrationTypes.ts (1)
12-13: Type mappings and enums are correct.IntegrationType extension, bidirectional mappings, SnowflakeAuthMethod enum, and IntegrationConfig union are properly defined.
Also applies to: 21-22, 32-33, 70-73, 97-97
src/webviews/webview-side/integrations/SnowflakeForm.tsx (2)
134-249: Conditional rendering and form validation are well-implemented.The form correctly uses
requiredattributes based on auth method, and conditional rendering ensures only relevant fields are shown.
1-31: Component structure and state management are solid.Props interface, state initialization, effect synchronization, and non-auth form fields are well-implemented.
Also applies to: 34-58, 83-112, 251-300
src/messageTypes.ts (2)
185-185: LGTM!The Snowflake type label follows the established pattern for integration type labels.
208-235: LGTM!The Snowflake form string properties are comprehensive and follow the established pattern. Both authentication methods are well-covered with appropriate labels, placeholders, and help text.
src/platform/common/utils/localize.ts (3)
837-837: LGTM!Snowflake type label properly localized and positioned.
867-883: LGTM!Snowflake form strings properly localized with clear labels and helpful placeholders.
884-893: LGTM!Private key configuration strings include helpful guidance. The reminder about BEGIN/END markers is particularly useful.
- Change auth method from enum to const object with PASSWORD, OKTA, NATIVE_SNOWFLAKE, AZURE_AD, KEY_PAIR, SERVICE_ACCOUNT_KEY_PAIR - Implement discriminated union for SnowflakeIntegrationConfig based on authMethod - Support PASSWORD and SERVICE_ACCOUNT_KEY_PAIR auth methods (non-user-specific) - Mark OKTA, NATIVE_SNOWFLAKE, AZURE_AD, KEY_PAIR as unsupported - Add type-safe connection string generation with proper type narrowing - Show error message and disable form for unsupported auth methods - Add localization for unsupported auth method warning - Update form submission to create correct discriminated union types
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
======================================
Coverage 72% 72%
======================================
Files 539 541 +2
Lines 41267 41443 +176
Branches 5012 5023 +11
======================================
+ Hits 30001 30174 +173
+ Misses 9592 9591 -1
- Partials 1674 1678 +4
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (1)
25-29: MoveUnsupportedIntegrationErrortosrc/platform/errors/.The error is currently defined locally but the coding guidelines require typed error classes from
src/platform/errors/. Create a new file (e.g.,unsupportedIntegrationError.ts) or add to an existing file, then import it intosqlIntegrationEnvironmentVariablesProvider.ts. The error is used at lines 138 and 151.
♻️ Duplicate comments (1)
src/webviews/webview-side/integrations/types.ts (1)
5-26: Single source of truth for Snowflake auth constants.These constants duplicate platform definitions. Extract to a shared module used by both sides.
As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/messageTypes.ts(2 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(2 hunks)src/platform/common/utils/localize.ts(2 hunks)src/platform/notebooks/deepnote/integrationTypes.ts(3 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts(2 hunks)src/webviews/webview-side/integrations/SnowflakeForm.tsx(1 hunks)src/webviews/webview-side/integrations/integrations.css(2 hunks)src/webviews/webview-side/integrations/types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/!(*.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/webviews/webview-side/integrations/types.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/common/utils/localize.tssrc/messageTypes.tssrc/platform/notebooks/deepnote/integrationTypes.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
**/*.{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/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/integrations/types.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/webviews/webview-side/integrations/SnowflakeForm.tsxsrc/platform/common/utils/localize.tssrc/messageTypes.tssrc/platform/notebooks/deepnote/integrationTypes.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationWebview.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.tssrc/platform/common/utils/localize.tssrc/platform/notebooks/deepnote/integrationTypes.ts
🧬 Code graph analysis (4)
src/webviews/webview-side/integrations/types.ts (1)
src/platform/notebooks/deepnote/integrationTypes.ts (8)
SnowflakeAuthMethods(70-77)SnowflakeAuthMethod(79-79)SUPPORTED_SNOWFLAKE_AUTH_METHODS(84-88)BaseIntegrationConfig(39-43)SnowflakeIntegrationConfig(104-131)IntegrationConfig(136-136)PostgresIntegrationConfig(48-56)BigQueryIntegrationConfig(61-65)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2)
src/webviews/webview-side/integrations/types.ts (2)
IntegrationType(1-1)SnowflakeAuthMethods(8-15)src/platform/notebooks/deepnote/integrationTypes.ts (1)
SnowflakeAuthMethods(70-77)
src/webviews/webview-side/integrations/SnowflakeForm.tsx (2)
src/webviews/webview-side/integrations/types.ts (3)
SnowflakeIntegrationConfig(64-91)SnowflakeAuthMethod(17-17)SnowflakeAuthMethods(8-15)src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
src/webviews/webview-side/integrations/types.ts (9)
IntegrationType(1-1)SnowflakeAuthMethods(8-15)SnowflakeAuthMethod(17-17)SUPPORTED_SNOWFLAKE_AUTH_METHODS(22-26)BaseIntegrationConfig(28-32)SnowflakeIntegrationConfig(64-91)IntegrationConfig(93-93)PostgresIntegrationConfig(34-42)BigQueryIntegrationConfig(44-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). (4)
- GitHub Check: Audit - Production
- GitHub Check: Lint & Format
- GitHub Check: Build & Test
- GitHub Check: Build & Package Extension
🔇 Additional comments (4)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
67-131: Good discriminated union for Snowflake.Branches enforce required fields per authMethod, including legacy null. Looks solid.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (1)
239-247: Avoid logging secrets inadvertently.Confirm logger.error here never logs serialized config/credentials. Current code logs only the error; keep it that way. If BaseError carries PII, scrub before logging.
src/webviews/webview-side/integrations/types.ts (1)
64-91: Snowflake union looks correct; matches platform.Auth branches and legacy null are enforced; unsupported methods are captured.
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
134-189: All localization keys properly defined. EveryintegrationsSnowflake*key referenced in the code has a corresponding definition inmessageTypes.ts. No undefined UI strings risk.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (1)
241-243: Lower log level for per-integration success.This is chatty on large notebooks; use trace/debug instead of info.
- logger.info( + logger.trace( `SqlIntegrationEnvironmentVariablesProvider: Added env var ${envVarName} for integration ${integrationId}` );
♻️ Duplicate comments (2)
src/webviews/webview-side/integrations/SnowflakeForm.tsx (1)
1-3: Group imports: third‑party vs local.-import * as React from 'react'; -import { format, getLocString } from '../react-common/locReactSide'; +import * as React from 'react'; + +import { format, getLocString } from '../react-common/locReactSide'; import { SnowflakeIntegrationConfig, SnowflakeAuthMethod, SnowflakeAuthMethods } from './types';src/platform/common/utils/localize.ts (1)
897-903: Empty placeholders acknowledged.Leaving database/role/warehouse placeholders empty was previously discussed and accepted for cloud parity. No change requested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/messageTypes.ts(2 hunks)src/platform/common/utils/localize.ts(2 hunks)src/platform/notebooks/deepnote/integrationTypes.ts(3 hunks)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts(2 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts(2 hunks)src/webviews/webview-side/integrations/SnowflakeForm.tsx(1 hunks)src/webviews/webview-side/integrations/types.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
.tsfiles (not suffixed with.nodeor.web)
Files:
src/messageTypes.tssrc/platform/notebooks/deepnote/snowflakeAuthConstants.tssrc/webviews/webview-side/integrations/types.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/common/utils/localize.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
**/*.{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/messageTypes.tssrc/platform/notebooks/deepnote/snowflakeAuthConstants.tssrc/webviews/webview-side/integrations/types.tssrc/webviews/webview-side/integrations/SnowflakeForm.tsxsrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/common/utils/localize.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/snowflakeAuthConstants.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/common/utils/localize.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
**/*.{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/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
🧬 Code graph analysis (1)
src/webviews/webview-side/integrations/types.ts (2)
src/platform/notebooks/deepnote/integrationTypes.ts (6)
BaseIntegrationConfig(39-43)SnowflakeIntegrationConfig(85-112)SnowflakeAuthMethods(69-69)IntegrationConfig(117-117)PostgresIntegrationConfig(48-56)BigQueryIntegrationConfig(61-65)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (1)
SnowflakeAuthMethods(4-11)
⏰ 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 (6)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (2)
529-563: Encoding assertions are solid.Good coverage of special chars in userinfo, host, db, and query. LGTM.
565-593: Minimal config case covered.Ensures application=Deepnote with no optional params. LGTM.
src/webviews/webview-side/integrations/SnowflakeForm.tsx (1)
222-344: Form logic and guards look good.Required+pattern checks and privateKey guard prevent empty secrets. A11y associations via htmlFor/aria-describedby are in place. LGTM.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (1)
25-29: No rewrite needed; original review comment is unfounded.The only available error matching the review's suggestion is
NotSupportedInWebErrorfromsrc/platform/errors/notSupportedInWebError.ts. However, it's environment-specific (web vs. Node runtime), with'nodeonly'category and web-localized messaging. Reusing it for "unsupported integration type" would conflate different error concerns and break error classification. The customUnsupportedIntegrationErroris the correct approach.src/messageTypes.ts (1)
185-185: All Snowflake localization keys are properly referenced.Verification confirms all 29 keys defined in messageTypes.ts (lines 209-237) are used. integrationsSnowflakeUnnamedIntegration appears only in SnowflakeForm.tsx, which is expected for a form-specific fallback. No unused or missing keys detected.
src/webviews/webview-side/integrations/types.ts (1)
1-5: SnowflakeAuthMethod export verified and correct.The constants module properly exports
SnowflakeAuthMethodas a type on line 13 ofsnowflakeAuthConstants.ts. The import statement is valid.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Outdated
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (1)
26-30: Local error class is fine; consider centralizing.You extend BaseError correctly. If other components may reuse UnsupportedIntegrationError, move it to src/platform/errors/ to keep typed errors centralized.
♻️ Duplicate comments (2)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (1)
106-119: Deduplicate Snowflake query param assembly.Two near-identical URLSearchParams blocks. Extract a tiny helper to reduce drift.
+function mkSnowflakeQS(opts: { warehouse?: string; role?: string; extra?: Record<string, string> }) { + const qp = new URLSearchParams(); + if (opts.warehouse) qp.set('warehouse', opts.warehouse); + if (opts.role) qp.set('role', opts.role); + qp.set('application', 'Deepnote'); + for (const [k, v] of Object.entries(opts.extra ?? {})) qp.set(k, v); + return qp.toString(); +} @@ - const queryParams = new URLSearchParams(); - if (config.warehouse) { queryParams.set('warehouse', config.warehouse); } - if (config.role) { queryParams.set('role', config.role); } - queryParams.set('application', 'Deepnote'); - const queryString = queryParams.toString(); + const queryString = mkSnowflakeQS({ warehouse: config.warehouse, role: config.role }); if (queryString) { url += `?${queryString}`; } @@ - const queryParams = new URLSearchParams(); - if (config.warehouse) { queryParams.set('warehouse', config.warehouse); } - if (config.role) { queryParams.set('role', config.role); } - queryParams.set('authenticator', 'snowflake_jwt'); - queryParams.set('application', 'Deepnote'); - const queryString = queryParams.toString(); + const queryString = mkSnowflakeQS({ + warehouse: config.warehouse, + role: config.role, + extra: { authenticator: 'snowflake_jwt' }, + }); if (queryString) { url += `?${queryString}`; }Also applies to: 133-147
src/platform/notebooks/deepnote/integrationTypes.ts (1)
67-75: Optional: re‑export SupportedSnowflakeAuthMethod.UI and provider sometimes need the narrowed supported type. Re‑export it for convenience.
import { SnowflakeAuthMethod, SnowflakeAuthMethods, SUPPORTED_SNOWFLAKE_AUTH_METHODS, - isSupportedSnowflakeAuthMethod + isSupportedSnowflakeAuthMethod, + SupportedSnowflakeAuthMethod } from './snowflakeAuthConstants'; -export { SnowflakeAuthMethod, SnowflakeAuthMethods, SUPPORTED_SNOWFLAKE_AUTH_METHODS, isSupportedSnowflakeAuthMethod }; +export { + SnowflakeAuthMethod, + SnowflakeAuthMethods, + SUPPORTED_SNOWFLAKE_AUTH_METHODS, + isSupportedSnowflakeAuthMethod, + SupportedSnowflakeAuthMethod +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/platform/common/utils/localize.ts(2 hunks)src/platform/notebooks/deepnote/integrationTypes.ts(3 hunks)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts(2 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts(2 hunks)src/webviews/webview-side/integrations/SnowflakeForm.tsx(1 hunks)src/webviews/webview-side/integrations/types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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
**/*.{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/webviews/webview-side/integrations/SnowflakeForm.tsxsrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/snowflakeAuthConstants.tssrc/webviews/webview-side/integrations/types.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/common/utils/localize.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/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/snowflakeAuthConstants.tssrc/webviews/webview-side/integrations/types.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/common/utils/localize.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.tssrc/platform/notebooks/deepnote/snowflakeAuthConstants.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/common/utils/localize.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
**/*.{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/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
🧬 Code graph analysis (5)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (3)
src/webviews/webview-side/integrations/types.ts (3)
IntegrationType(8-8)isSupportedSnowflakeAuthMethod(13-13)SnowflakeAuthMethods(13-13)src/platform/notebooks/deepnote/integrationTypes.ts (2)
isSupportedSnowflakeAuthMethod(74-74)SnowflakeAuthMethods(74-74)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (2)
isSupportedSnowflakeAuthMethod(31-33)SnowflakeAuthMethods(4-11)
src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (2)
src/webviews/webview-side/integrations/types.ts (4)
SnowflakeAuthMethods(13-13)SnowflakeAuthMethod(13-13)SUPPORTED_SNOWFLAKE_AUTH_METHODS(13-13)isSupportedSnowflakeAuthMethod(13-13)src/platform/notebooks/deepnote/integrationTypes.ts (4)
SnowflakeAuthMethods(74-74)SnowflakeAuthMethod(74-74)SUPPORTED_SNOWFLAKE_AUTH_METHODS(74-74)isSupportedSnowflakeAuthMethod(74-74)
src/webviews/webview-side/integrations/types.ts (2)
src/platform/notebooks/deepnote/integrationTypes.ts (6)
BaseIntegrationConfig(39-43)SnowflakeIntegrationConfig(90-117)SnowflakeAuthMethods(74-74)IntegrationConfig(122-122)PostgresIntegrationConfig(48-56)BigQueryIntegrationConfig(61-65)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (1)
SnowflakeAuthMethods(4-11)
src/platform/notebooks/deepnote/integrationTypes.ts (2)
src/webviews/webview-side/integrations/types.ts (6)
IntegrationType(8-8)BaseIntegrationConfig(15-19)SnowflakeAuthMethods(13-13)IntegrationConfig(75-75)PostgresIntegrationConfig(21-29)BigQueryIntegrationConfig(31-35)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (1)
SnowflakeAuthMethods(4-11)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (3)
src/platform/notebooks/deepnote/integrationTypes.ts (2)
SnowflakeIntegrationConfig(90-117)SnowflakeAuthMethods(74-74)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(8-8)src/test/vscode-mock.ts (1)
mockedVSCodeNamespaces(17-17)
🪛 Gitleaks (8.28.0)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
[high] 452-494: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ 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 (9)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (1)
376-667: Snowflake test coverage is strong and representative.Covers PASSWORD, legacy null, key‑pair (with/without passphrase), encoding, minimal optionals, and unsupported auth paths. Assertions match provider behavior.
src/platform/notebooks/deepnote/integrationTypes.ts (2)
20-35: Type mappings look correct.pgsql/big‑query/snowflake conversions are consistent.
90-117: Snowflake discriminated union reads well.Auth‑specific fields are enforced; unsupported branch is permissive. Good balance.
src/webviews/webview-side/integrations/SnowflakeForm.tsx (1)
181-197: Form validation and a11y look solid.Required + pattern guards, passphrase optionality, help text associations, and submit guard for empty private key are correct.
Also applies to: 218-341
src/webviews/webview-side/integrations/types.ts (1)
51-75: Snowflake union is concise and correct.Legacy null merged with PASSWORD; key‑pair variant enforces privateKey presence. Good.
src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (3)
1-33: LGTM! Type guard implementation is solid.The constants, types, and type guard are well-structured. The type guard correctly handles the
nullcase for legacy support, and the readonly cast ensures type safety.
837-837: LGTM!Type label follows the established pattern.
866-903: LGTM! Comprehensive localization coverage.All Snowflake form strings are properly wrapped with
l10n.t()and follow the pattern established by PostgreSQL and BigQuery integrations. Empty placeholders are intentional per product requirements.src/platform/common/utils/localize.ts (1)
837-837: LGTM!Follows the established pattern for integration type labels.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/platform/notebooks/deepnote/integrationTypes.ts (1)
67-80: Re‑export SupportedSnowflakeAuthMethod to avoid duplicate imports.Consumers (UI/provider) often need the narrowed type. Re‑export it here with the rest to keep a single import surface.
Apply this diff:
import { - type SnowflakeAuthMethod, + type SnowflakeAuthMethod, + type SupportedSnowflakeAuthMethod, SnowflakeAuthMethods, SUPPORTED_SNOWFLAKE_AUTH_METHODS, isSupportedSnowflakeAuthMethod } from './snowflakeAuthConstants'; export { - type SnowflakeAuthMethod, + type SnowflakeAuthMethod, + type SupportedSnowflakeAuthMethod, SnowflakeAuthMethods, SUPPORTED_SNOWFLAKE_AUTH_METHODS, isSupportedSnowflakeAuthMethod };Run to check usages expecting this re-export:
#!/bin/bash rg -nP --type=ts -C2 '\bSupportedSnowflakeAuthMethod\b'src/webviews/webview-side/integrations/SnowflakeForm.tsx (1)
1-8: Type‑only import and import grouping.Avoid runtime import for the type and add a blank line between third‑party and local imports.
Apply this diff:
-import * as React from 'react'; -import { format, getLocString } from '../react-common/locReactSide'; -import { - SnowflakeIntegrationConfig, - SnowflakeAuthMethod, - SnowflakeAuthMethods, - isSupportedSnowflakeAuthMethod -} from './types'; +import * as React from 'react'; + +import { format, getLocString } from '../react-common/locReactSide'; +import { SnowflakeIntegrationConfig, SnowflakeAuthMethods, isSupportedSnowflakeAuthMethod } from './types'; +import type { SnowflakeAuthMethod } from './types';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/messageTypes.ts(2 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(2 hunks)src/platform/common/utils/localize.ts(2 hunks)src/platform/notebooks/deepnote/integrationTypes.ts(3 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts(2 hunks)src/webviews/webview-side/integrations/SnowflakeForm.tsx(1 hunks)src/webviews/webview-side/integrations/types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.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/webviews/webview-side/integrations/types.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/messageTypes.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/platform/common/utils/localize.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
**/*.{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/webviews/webview-side/integrations/types.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/messageTypes.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/webviews/webview-side/integrations/SnowflakeForm.tsxsrc/notebooks/deepnote/integrations/integrationWebview.tssrc/platform/common/utils/localize.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/integrationTypes.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/common/utils/localize.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
**/*.{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/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationWebview.ts
🧬 Code graph analysis (2)
src/platform/notebooks/deepnote/integrationTypes.ts (2)
src/webviews/webview-side/integrations/types.ts (6)
IntegrationType(8-8)BaseIntegrationConfig(20-24)SnowflakeAuthMethods(15-15)IntegrationConfig(80-80)PostgresIntegrationConfig(26-34)BigQueryIntegrationConfig(36-40)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (1)
SnowflakeAuthMethods(4-11)
src/webviews/webview-side/integrations/SnowflakeForm.tsx (4)
src/platform/notebooks/deepnote/integrationTypes.ts (4)
SnowflakeIntegrationConfig(95-122)SnowflakeAuthMethods(76-76)isSupportedSnowflakeAuthMethod(78-78)SnowflakeAuthMethod(75-75)src/webviews/webview-side/integrations/types.ts (4)
SnowflakeIntegrationConfig(56-78)SnowflakeAuthMethods(15-15)isSupportedSnowflakeAuthMethod(17-17)SnowflakeAuthMethod(14-14)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (3)
SnowflakeAuthMethods(4-11)isSupportedSnowflakeAuthMethod(31-33)SnowflakeAuthMethod(13-13)src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
⏰ 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 & Test
- GitHub Check: Lint & Format
- GitHub Check: Build & Package Extension
🔇 Additional comments (6)
src/platform/common/utils/localize.ts (1)
866-903: Snowflake localization block looks good.Strings use l10n.t consistently; passphrase help punctuation fixed; placeholders align with prior decisions.
src/webviews/webview-side/integrations/types.ts (1)
1-6: Solid Snowflake typing and import hygiene.Type-only import and the collapsed null/PASSWORD branch improve correctness and DX. Looks good.
Also applies to: 12-18, 56-79
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (1)
449-466: Verification confirms good hygiene; no changes needed.Split PEM markers avoid scanner false positives. No secrets leaked in test files.
src/platform/notebooks/deepnote/integrationTypes.ts (1)
12-13: LGTM: Snowflake type and mappings.Enum additions, Deepnote ↔︎ internal mappings, and IntegrationConfig inclusion look correct.
Also applies to: 21-23, 32-33, 127-127
src/webviews/webview-side/integrations/SnowflakeForm.tsx (2)
107-155: Form logic, validation, and localization look solid.Submit handler builds correct config for both auth methods; required + pattern guards prevent whitespace-only inputs; l10n keys used consistently; private key guard present.
Also applies to: 218-334, 379-386
63-71: Review comment is incorrect—null auth is already supported.The
SUPPORTED_SNOWFLAKE_AUTH_METHODSarray explicitly includesnullfor legacy configs. WhenauthMethodisnull,isSupportedSnowflakeAuthMethod(null)returnstrue, soisUnsupportedbecomesfalseand the form enables normally. The proposed diff is unnecessary and would change intended behavior.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
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/webviews/webview-side/integrations/BigQueryForm.tsx (1)
51-53: Localize the “Invalid JSON” error message.Use the provided key instead of a hard-coded string.
- const errorMessage = error instanceof Error ? error.message : 'Invalid JSON format'; - const invalidJsonMsg = format('Invalid JSON: {0}', errorMessage); + const errorMessage = error instanceof Error ? error.message : 'Invalid JSON format'; + const invalidJsonMsg = format( + getLocString('integrationsBigQueryInvalidJson', 'Invalid JSON: {0}'), + errorMessage + );src/notebooks/deepnote/integrations/integrationWebview.ts (1)
67-77: Localize the webview panel title.Use the i18n title instead of a literal.
- this.currentPanel = window.createWebviewPanel( - 'deepnoteIntegrations', - 'Deepnote Integrations', + this.currentPanel = window.createWebviewPanel( + 'deepnoteIntegrations', + localize.Integrations.title, column || ViewColumn.One,
♻️ Duplicate comments (2)
src/webviews/webview-side/integrations/SnowflakeForm.tsx (2)
1-9: Use a type‑only import for SnowflakeAuthMethod.As per coding guidelines
-import { - SnowflakeIntegrationConfig, - SnowflakeAuthMethod, - SnowflakeAuthMethods, - isSupportedSnowflakeAuthMethod -} from './types'; +import { + SnowflakeIntegrationConfig, + SnowflakeAuthMethods, + isSupportedSnowflakeAuthMethod +} from './types'; +import type { SnowflakeAuthMethod } from './types';
1-3: Group imports: add a blank line after React.As per coding guidelines
-import * as React from 'react'; -import { format, getLocString } from '../react-common/locReactSide'; +import * as React from 'react'; + +import { format, getLocString } from '../react-common/locReactSide';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/messageTypes.ts(2 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(2 hunks)src/platform/common/utils/localize.ts(2 hunks)src/platform/notebooks/deepnote/integrationTypes.ts(3 hunks)src/webviews/webview-side/integrations/BigQueryForm.tsx(1 hunks)src/webviews/webview-side/integrations/PostgresForm.tsx(1 hunks)src/webviews/webview-side/integrations/SnowflakeForm.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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
**/*.{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/webviews/webview-side/integrations/BigQueryForm.tsxsrc/webviews/webview-side/integrations/PostgresForm.tsxsrc/notebooks/deepnote/integrations/integrationWebview.tssrc/platform/notebooks/deepnote/integrationTypes.tssrc/messageTypes.tssrc/webviews/webview-side/integrations/SnowflakeForm.tsxsrc/platform/common/utils/localize.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/platform/notebooks/deepnote/integrationTypes.tssrc/messageTypes.tssrc/platform/common/utils/localize.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationWebview.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/integrationTypes.tssrc/platform/common/utils/localize.ts
🧬 Code graph analysis (5)
src/webviews/webview-side/integrations/BigQueryForm.tsx (4)
src/platform/common/utils/localize.ts (1)
unnamedIntegration(833-833)src/webviews/webview-side/react-common/locReactSide.ts (2)
getLocString(11-17)format(23-25)src/platform/notebooks/deepnote/integrationTypes.ts (1)
BigQueryIntegrationConfig(61-65)src/webviews/webview-side/integrations/types.ts (1)
BigQueryIntegrationConfig(36-40)
src/webviews/webview-side/integrations/PostgresForm.tsx (3)
src/platform/common/utils/localize.ts (1)
unnamedIntegration(833-833)src/webviews/webview-side/react-common/locReactSide.ts (2)
getLocString(11-17)format(23-25)src/webviews/webview-side/integrations/types.ts (1)
PostgresIntegrationConfig(26-34)
src/platform/notebooks/deepnote/integrationTypes.ts (2)
src/webviews/webview-side/integrations/types.ts (6)
IntegrationType(8-8)BaseIntegrationConfig(20-24)SnowflakeAuthMethods(15-15)IntegrationConfig(80-80)PostgresIntegrationConfig(26-34)BigQueryIntegrationConfig(36-40)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (1)
SnowflakeAuthMethods(4-11)
src/webviews/webview-side/integrations/SnowflakeForm.tsx (4)
src/webviews/webview-side/integrations/types.ts (4)
SnowflakeIntegrationConfig(56-78)SnowflakeAuthMethods(15-15)isSupportedSnowflakeAuthMethod(17-17)SnowflakeAuthMethod(14-14)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (3)
SnowflakeAuthMethods(4-11)isSupportedSnowflakeAuthMethod(31-33)SnowflakeAuthMethod(13-13)src/platform/common/utils/localize.ts (1)
unnamedIntegration(833-833)src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
src/platform/common/utils/localize.ts (2)
src/test/mocks/vsc/extHostedTypes.ts (1)
id(1619-1621)src/notebooks/controllers/vscodeNotebookController.ts (1)
id(109-111)
🔇 Additional comments (6)
src/platform/common/utils/localize.ts (1)
865-901: Snowflake i18n block looks consistent.Labels, helps, and placeholders align with project style; empty placeholders for optional fields match prior decisions.
src/webviews/webview-side/integrations/PostgresForm.tsx (1)
52-57: LGTM on localized unnamed fallback.Correct use of integrationsUnnamedIntegration and format for name default.
src/platform/notebooks/deepnote/integrationTypes.ts (1)
10-34: Snowflake typing and mappings look good.Enum, maps, and discriminated union are coherent; re-exports are useful.
Also applies to: 67-117, 122-122
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
120-186: Verification passed: all integration keys are declared.All 64 keys sent in
integrationWebview.ts(lines 120-186) have matching declarations in theLocalizedMessagesinterface inmessageTypes.ts. No missing or orphaned keys detected.src/messageTypes.ts (2)
185-185: Comprehensive Snowflake localization keys.The keys cover both auth methods (username+password and key-pair) and all connection parameters. Structure and naming follow the established pattern.
Also applies to: 208-233
237-237: No issues—the key is properly integrated.The
integrationsUnnamedIntegrationkey is sent to the webview and actively used by all three forms (SnowflakeForm, PostgresForm, BigQueryForm) viagetLocString()at their submit handlers. No hardcoding found.
saltenasl
left a comment
saltenasl
left a comment
There was a problem hiding this comment.
the config doesn't look like in the screenshot in description and i don't know what to fill in there to test
Add validation in IntegrationManager to check that projectIntegration.type exists as a key in DEEPNOTE_TO_INTEGRATION_TYPE before performing the lookup. If the type is unknown, log a warning with context (integration ID, project ID, and the unexpected type value) and leave integrationType undefined so callers can handle missing mappings explicitly.
saltenasl
left a comment
saltenasl
left a comment
There was a problem hiding this comment.
working well after running npm run compile
Create a new shared UnsupportedIntegrationError in src/platform/errors to conform to the project error taxonomy. Replace the locally defined error in sqlIntegrationEnvironmentVariablesProvider.ts with the shared version. The shared error maintains the same behavior (BaseError with 'unknown' category) and accepts a message parameter to support both integration type and auth method error cases. All three throw sites (lines 76-78, 109-111, and 148-150) now use the shared error with appropriate messages for consistent telemetry and error handling across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2)
91-104: DRY the Snowflake query string build.Tiny helper avoids duplication across auth branches.
+function mkSnowflakeQS(opts: { warehouse?: string; role?: string; extra?: Record<string, string> }) { + const qp = new URLSearchParams(); + if (opts.warehouse) qp.set('warehouse', opts.warehouse); + if (opts.role) qp.set('role', opts.role); + qp.set('application', 'Deepnote'); + for (const [k, v] of Object.entries(opts.extra ?? {})) qp.set(k, v); + return qp.toString(); +} @@ - const queryParams = new URLSearchParams(); - if (config.warehouse) { - queryParams.set('warehouse', config.warehouse); - } - if (config.role) { - queryParams.set('role', config.role); - } - queryParams.set('application', 'Deepnote'); - const queryString = queryParams.toString(); + const queryString = mkSnowflakeQS({ warehouse: config.warehouse, role: config.role }); if (queryString) { url += `?${queryString}`; } @@ - const queryParams = new URLSearchParams(); - if (config.warehouse) { - queryParams.set('warehouse', config.warehouse); - } - if (config.role) { - queryParams.set('role', config.role); - } - queryParams.set('authenticator', 'snowflake_jwt'); - queryParams.set('application', 'Deepnote'); - const queryString = queryParams.toString(); + const queryString = mkSnowflakeQS({ + warehouse: config.warehouse, + role: config.role, + extra: { authenticator: 'snowflake_jwt' }, + }); if (queryString) { url += `?${queryString}`; }Also applies to: 118-131
133-137: Fix Node/web base64 handling; btoa is undefined in Node.Provider runs in Node (tests too). Use Buffer with browser fallback.
Apply:
+// Cross‑platform base64 helper: Node Buffer first, browser btoa fallback. +const toBase64 = (s: string) => + typeof Buffer !== 'undefined' + ? Buffer.from(s, 'utf8').toString('base64') + : btoa(unescape(encodeURIComponent(s))); @@ - params.snowflake_private_key = btoa(config.privateKey); + params.snowflake_private_key = toBase64(config.privateKey);Optionally import Buffer explicitly for bundlers:
+import { Buffer } from 'buffer';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/notebooks/deepnote/integrations/integrationManager.ts(3 hunks)src/platform/errors/unsupportedIntegrationError.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts(2 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.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/errors/unsupportedIntegrationError.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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
**/*.{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/errors/unsupportedIntegrationError.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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/errors/unsupportedIntegrationError.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationManager.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
**/*.{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/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/integrations/integrationManager.ts (3)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationWebviewProvider(19-19)IIntegrationWebviewProvider(20-32)src/notebooks/types.ts (2)
IDeepnoteNotebookManager(37-37)IDeepnoteNotebookManager(38-58)src/platform/notebooks/deepnote/integrationTypes.ts (2)
DEEPNOTE_TO_INTEGRATION_TYPE(30-34)RawIntegrationType(25-25)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (4)
src/webviews/webview-side/integrations/types.ts (4)
IntegrationType(8-8)isSupportedSnowflakeAuthMethod(17-17)SnowflakeAuthMethods(15-15)IntegrationConfig(80-80)src/platform/notebooks/deepnote/integrationTypes.ts (3)
isSupportedSnowflakeAuthMethod(78-78)SnowflakeAuthMethods(76-76)IntegrationConfig(122-122)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (2)
isSupportedSnowflakeAuthMethod(31-33)SnowflakeAuthMethods(4-11)src/platform/errors/unsupportedIntegrationError.ts (1)
UnsupportedIntegrationError(17-21)
⏰ 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 & Test
- GitHub Check: Lint & Format
- GitHub Check: Build & Package Extension
🔇 Additional comments (4)
src/notebooks/deepnote/integrations/integrationManager.ts (3)
8-14: Imports look good.Centralizing types/mapping from platform is correct and consistent.
16-16: DI binding confirmed forIDeepnoteNotebookManager.Both
serviceRegistry.web.ts(line 111) andserviceRegistry.node.ts(line 149) register the binding:serviceManager.addSingleton<IDeepnoteNotebookManager>(IDeepnoteNotebookManager, DeepnoteNotebookManager). The concrete implementation is properly decorated with@injectable(), and constructor injection via@inject()is correctly applied across all consumers.
183-188: Original code is correct; no refactor needed.The interface
IntegrationWithStatusalready declaresintegrationNameandintegrationTypeas optional fields (with JSDoc explaining they're for prefilling when config is null). Assigningundefinedto optional fields is valid—the conditional spreads in the suggested diff add unnecessary complexity.Likely an incorrect or invalid review comment.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (1)
382-676: Snowflake tests are thorough. LGTM.Covers both auth paths, encoding, minimal fields, and unsupported methods; PEM stub avoids scanners. No changes requested.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2)
125-129: Fix: btoa is undefined in Node; use a cross‑platform base64 helper.This will throw in the extension host. Use Buffer (Node) with a safe browser fallback.
Apply within this block:
- params.snowflake_private_key = btoa(config.privateKey); + params.snowflake_private_key = toBase64(config.privateKey);Add outside this block (module‑level):
// Add import near the top import { Buffer } from 'buffer'; // Add helper (e.g., after getSqlEnvVarName) const toBase64 = (s: string) => typeof Buffer !== 'undefined' ? Buffer.from(s, 'utf8').toString('base64') : btoa(unescape(encodeURIComponent(s)));Alternatively, suffix this file with .node.ts if it’s strictly Node-only, but the helper keeps it portable. As per coding guidelines.
83-96: DRY the Snowflake query‑string assembly.Both branches build identical QS with minor differences; extract a tiny helper.
Example:
function mkSnowflakeQS(opts: { warehouse?: string; role?: string; extra?: Record<string, string> }) { const qp = new URLSearchParams(); if (opts.warehouse) qp.set('warehouse', opts.warehouse); if (opts.role) qp.set('role', opts.role); qp.set('application', 'Deepnote'); for (const [k, v] of Object.entries(opts.extra ?? {})) qp.set(k, v); return qp.toString(); }Then replace:
- const queryParams = new URLSearchParams(); - if (config.warehouse) { queryParams.set('warehouse', config.warehouse); } - if (config.role) { queryParams.set('role', config.role); } - queryParams.set('application', 'Deepnote'); - const queryString = queryParams.toString(); + const queryString = mkSnowflakeQS({ warehouse: config.warehouse, role: config.role });and
- const queryParams = new URLSearchParams(); - if (config.warehouse) { queryParams.set('warehouse', config.warehouse); } - if (config.role) { queryParams.set('role', config.role); } - queryParams.set('authenticator', 'snowflake_jwt'); - queryParams.set('application', 'Deepnote'); - const queryString = queryParams.toString(); + const queryString = mkSnowflakeQS({ + warehouse: config.warehouse, + role: config.role, + extra: { authenticator: 'snowflake_jwt' }, + });Also applies to: 110-123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/platform/errors/types.ts(1 hunks)src/platform/errors/unsupportedIntegrationError.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/!(*.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/errors/types.tssrc/platform/errors/unsupportedIntegrationError.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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
**/*.{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/errors/types.tssrc/platform/errors/unsupportedIntegrationError.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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/errors/types.tssrc/platform/errors/unsupportedIntegrationError.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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). (8)
- GitHub Check: Audit - Production
- GitHub Check: Lint & Format
- GitHub Check: Build & Test
- GitHub Check: Audit - All
- GitHub Check: Check Licenses
- GitHub Check: Spell Check
- GitHub Check: TypeCheck
- GitHub Check: Build & Package Extension
🔇 Additional comments (2)
src/platform/errors/types.ts (1)
108-110: Add unsupported_integration — LGTM.Consistent with taxonomy; no further changes needed here.
src/platform/errors/unsupportedIntegrationError.ts (1)
14-18: Typed error aligns with taxonomy — LGTM.Constructor uses the specific category; docs are clear.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2)
83-96: Optional: dedupe Snowflake query param assembly.Tiny helper reduces duplication between auth branches.
// helper (place near convertToEnvironmentVariableName) function mkSnowflakeQS(opts: { warehouse?: string; role?: string; extra?: Record<string,string> }) { const qp = new URLSearchParams(); if (opts.warehouse) qp.set('warehouse', opts.warehouse); if (opts.role) qp.set('role', opts.role); qp.set('application', 'Deepnote'); for (const [k, v] of Object.entries(opts.extra ?? {})) qp.set(k, v); return qp.toString(); }- const queryParams = new URLSearchParams(); - if (config.warehouse) { - queryParams.set('warehouse', config.warehouse); - } - if (config.role) { - queryParams.set('role', config.role); - } - queryParams.set('application', 'Deepnote'); - const queryString = queryParams.toString(); + const queryString = mkSnowflakeQS({ warehouse: config.warehouse, role: config.role }); if (queryString) { url += `?${queryString}`; } @@ - const queryParams = new URLSearchParams(); - if (config.warehouse) { - queryParams.set('warehouse', config.warehouse); - } - if (config.role) { - queryParams.set('role', config.role); - } - queryParams.set('authenticator', 'snowflake_jwt'); - queryParams.set('application', 'Deepnote'); - const queryString = queryParams.toString(); + const queryString = mkSnowflakeQS({ + warehouse: config.warehouse, + role: config.role, + extra: { authenticator: 'snowflake_jwt' }, + }); if (queryString) { url += `?${queryString}`; }Also applies to: 113-127
128-133: btoa is undefined in Node; fix the cross-platform encoding but remove the unnecessary Buffer import.The issue is real—btoa isn't available in Node.js environments. However, the proposed import of
Bufferfrom 'buffer' breaks browser compatibility. Buffer is global in Node and should be checked at runtime without importing, matching the existing pattern insrc/platform/common/utils/string.ts.- // For key-pair auth, pass the private key and passphrase as params - params.snowflake_private_key = btoa(config.privateKey); + // For key-pair auth, pass the private key and passphrase as params + const toBase64 = (s: string) => + (typeof Buffer !== 'undefined' + ? Buffer.from(s, 'utf8').toString('base64') + : btoa(unescape(encodeURIComponent(s)))); + params.snowflake_private_key = toBase64(config.privateKey); if (config.privateKeyPassphrase) { params.snowflake_private_key_passphrase = config.privateKeyPassphrase; }Do not add the Buffer import—it's a global in Node and doesn't need importing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 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
.tsfiles (not suffixed with.nodeor.web)
Files:
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.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
**/*.{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.tssrc/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.tssrc/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
thenproperty to avoid hanging tests
Files:
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
🧬 Code graph analysis (2)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (3)
src/platform/notebooks/deepnote/integrationTypes.ts (2)
SnowflakeIntegrationConfig(95-117)SnowflakeAuthMethods(76-76)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(8-8)src/test/vscode-mock.ts (1)
mockedVSCodeNamespaces(17-17)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (4)
src/webviews/webview-side/integrations/types.ts (3)
IntegrationType(8-8)SnowflakeAuthMethods(15-15)IntegrationConfig(80-80)src/platform/notebooks/deepnote/integrationTypes.ts (2)
SnowflakeAuthMethods(76-76)IntegrationConfig(122-122)src/platform/notebooks/deepnote/snowflakeAuthConstants.ts (1)
SnowflakeAuthMethods(4-11)src/platform/errors/unsupportedIntegrationError.ts (1)
UnsupportedIntegrationError(14-18)
⏰ 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). (8)
- GitHub Check: Audit - All
- GitHub Check: Check Licenses
- GitHub Check: Audit - Production
- GitHub Check: Build & Package Extension
- GitHub Check: Spell Check
- GitHub Check: Build & Test
- GitHub Check: TypeCheck
- GitHub Check: Lint & Format
🔇 Additional comments (3)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (1)
458-465: Good: test fixture avoids PEM leaks.Breaking PEM markers prevents false positives in secret scanners. LGTM.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2)
135-139: Correct param_style for Snowflake.Using 'pyformat' matches Snowflake Python connector. Good.
101-106: Localized errors: good.l10n.t used for user‑facing messages. Consistent with guidelines.
Also applies to: 144-145
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Localization
Errors
Style
Tests