feat: SQL block credentials propagation to Jupyter kernel #78
Conversation
📝 WalkthroughWalkthroughAdds platform-level Deepnote integrations support and SQL credential propagation: new INTEGRATIONS_CREDENTIALS.md; new ISqlIntegrationEnvVarsProvider and SqlIntegrationEnvironmentVariablesProvider that scan notebooks for SQL integration IDs, fetch integration configs from IIntegrationStorage, and expose per-notebook env vars; registers the provider as a singleton and wires it (optionally) into DeepnoteServerStarter and KernelEnvironmentVariablesService; adds SqlIntegrationStartupCodeProvider to inject Python startup code setting os.environ; consolidates integration types/storage to the platform layer; and adds unit tests for the provider and kernel helpers. Sequence Diagram(s)sequenceDiagram
participant VSCode as VSCode Notebook
participant DI as Service Registry
participant Storage as IIntegrationStorage
participant EnvProv as SqlIntegrationEnvironmentVariablesProvider
participant KernelEnv as KernelEnvironmentVariablesService
participant Server as DeepnoteServerStarter
participant Startup as SqlIntegrationStartupCodeProvider
participant Kernel as Kernel Startup
Note over DI: SqlIntegrationEnvironmentVariablesProvider registered as singleton
DI->>EnvProv: register provider
KernelEnv->>EnvProv: getEnvironmentVariables(resource)
EnvProv->>VSCode: scan notebook for sql_integration_id metadata
VSCode-->>EnvProv: integrationIds[]
loop per integrationId
EnvProv->>Storage: getIntegrationConfig(integrationId)
Storage-->>EnvProv: IntegrationConfig | undefined
EnvProv->>EnvProv: convertIntegrationConfigToJson(...)
end
EnvProv-->>KernelEnv: { SQL_<SANITIZED_ID>: JSON_CREDENTIALS, ... }
Server->>EnvProv: (optional, detached start) getEnvironmentVariables(resource)
EnvProv-->>Server: env vars map
Kernel->>Startup: getCode(kernel)
Startup->>EnvProv: getEnvironmentVariables(resource)
EnvProv-->>Startup: env vars map
Startup->>Kernel: emit Python startup code to set os.environ
Kernel->>Kernel: execute startup code (env injected)
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
======================================
Coverage 71% 71%
======================================
Files 523 527 +4
Lines 39001 39474 +473
Branches 4905 4933 +28
======================================
+ Hits 27834 28277 +443
- Misses 9542 9563 +21
- Partials 1625 1634 +9
🚀 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: 13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
INTEGRATIONS_CREDENTIALS.md(1 hunks)src/kernels/deepnote/deepnoteServerStarter.node.ts(4 hunks)src/kernels/kernel.ts(1 hunks)src/kernels/raw/launcher/kernelEnvVarsService.node.ts(8 hunks)src/kernels/serviceRegistry.node.ts(2 hunks)src/notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider.ts(1 hunks)src/notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts(1 hunks)src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts(1 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
**/!(*.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/kernels/kernel.tssrc/notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.tssrc/notebooks/deepnote/integrations/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/kernels/kernel.tssrc/notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.tssrc/kernels/raw/launcher/kernelEnvVarsService.node.tssrc/kernels/serviceRegistry.node.tssrc/notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider.tssrc/notebooks/serviceRegistry.node.ts
src/kernels/kernel.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Kernel must manage sessions via IKernelSession and implement lifecycle hooks (willRestart, willInterrupt, etc.) with startup code execution (e.g., matplotlib, IPyWidgets)
Files:
src/kernels/kernel.ts
src/kernels/{kernel.ts,kernelSessionFactory.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Prefer session reuse to avoid unnecessary kernel restarts
Files:
src/kernels/kernel.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/kernel.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.node.tssrc/kernels/serviceRegistry.node.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/notebooks/deepnote/integrations/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/notebooks/deepnote/integrations/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/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.tssrc/notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider.ts
**/*.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/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.node.tssrc/kernels/serviceRegistry.node.tssrc/notebooks/serviceRegistry.node.ts
src/kernels/raw/launcher/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Raw launcher must spawn kernel processes and manage ports/connection files
Files:
src/kernels/raw/launcher/kernelEnvVarsService.node.ts
src/kernels/{kernelFinder.ts,serviceRegistry.{node,web}.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Finders must be registered with the KernelFinder on startup
Files:
src/kernels/serviceRegistry.node.ts
src/kernels/serviceRegistry.node.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Node service registry must register raw kernel components and enable filesystem-based discovery
Files:
src/kernels/serviceRegistry.node.ts
src/kernels/serviceRegistry.{node,web}.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use dependency injection for services to enable testability and platform variance
Files:
src/kernels/serviceRegistry.node.ts
🧠 Learnings (3)
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/serviceRegistry.{node,web}.ts : Use dependency injection for services to enable testability and platform variance
Applied to files:
src/kernels/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/ipywidgets/serviceRegistry.{node,web}.ts : Register DI services in platform-specific registries (serviceRegistry.node.ts vs serviceRegistry.web.ts)
Applied to files:
src/kernels/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:56:39.535Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel-jupyter.instructions.md:0-0
Timestamp: 2025-09-03T12:56:39.535Z
Learning: Applies to src/kernels/jupyter/serviceRegistry.{node,web}.ts : Use dependency injection via the service registry for component creation and registration
Applied to files:
src/kernels/serviceRegistry.node.ts
🧬 Code graph analysis (3)
src/kernels/kernel.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/test/web/customReporter.ts (1)
output(153-155)
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts (6)
src/notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider.ts (1)
injectable(69-201)src/kernels/raw/launcher/kernelEnvVarsService.node.ts (1)
injectable(26-203)src/kernels/types.ts (1)
IStartupCodeProvider(917-920)src/platform/common/constants.ts (1)
JupyterNotebookView(13-13)src/kernels/helpers.ts (1)
isPythonKernelConnection(454-466)src/kernels/deepnote/types.ts (1)
DEEPNOTE_NOTEBOOK_TYPE(160-160)
src/notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider.ts (5)
src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)src/platform/common/types.ts (1)
Resource(19-19)src/platform/logging/index.ts (1)
logger(35-48)src/platform/common/variables/types.ts (1)
EnvironmentVariables(8-8)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)
🪛 GitHub Check: Lint & Format
src/kernels/deepnote/deepnoteServerStarter.node.ts
[failure] 20-20:
Unexpected path "../../notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider" imported in restricted zone. Only modules from ./src/platform and ./src/telemetry can be imported into ./src/kernels
src/kernels/raw/launcher/kernelEnvVarsService.node.ts
[failure] 21-21:
Unexpected path "../../../notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider" imported in restricted zone. Only modules from ./src/platform and ./src/telemetry can be imported into ./src/kernels
src/kernels/serviceRegistry.node.ts
[failure] 51-51:
Unexpected path "../notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider" imported in restricted zone. Only modules from ./src/platform and ./src/telemetry can be imported into ./src/kernels
🪛 markdownlint-cli2 (0.18.1)
INTEGRATIONS_CREDENTIALS.md
116-116: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
128-128: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
214-214: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
223-223: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
247-247: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
293-293: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
294-294: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
294-294: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
304-304: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
305-305: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
305-305: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
335-335: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
355-355: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (5)
src/kernels/serviceRegistry.node.ts (1)
51-52: No lint rule prevents this import; pattern is pre-existing and widespread.The
.eslintrc.jshas no layer or forbidden-import rules. The codebase already contains 120+../notebooksimports acrosssrc/kernels/, including similar patterns inkernelEnvVarsService.node.tsanddeepnoteServerStarter.node.ts. TheSqlIntegrationEnvironmentVariablesProviderimport follows the existing architectural pattern—it's not a lint blocker.If this is a desired refactoring to consolidate environment variable providers, that's a valid architectural suggestion, but it's not a blocking issue as stated.
src/notebooks/serviceRegistry.node.ts (1)
69-69: Startup provider wiring verified as correct.SqlIntegrationStartupCodeProvider properly implements IStartupCodeProvider with the correct getCode() signature, self-registers via activate() with KernelStartupCodeProviders, and is registered as IExtensionSyncActivationService in notebooks/serviceRegistry.node.ts. CancellationToken is not part of the IStartupCodeProvider interface contract, so the implementation is consistent with expectations.
src/notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (2)
1-2: Use Node’s assert for deepStrictEqual; keep Chai for property checks.
chai.assertdoesn’t expose deepStrictEqual. Switch to Node’sassert(strict) and alias Chai’s assert for.property. As per coding guidelines.Apply:
- import { assert } from 'chai'; + import { assert as chaiAssert } from 'chai'; + import { strict as assert } from 'assert';⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-09T11:21:57.494Z Learning: Applies to **/*.unit.test.ts : Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
31-31: Fix assertions to use the correct assert interfaces.Replace
chai.assert.deepStrictEqualcalls with Nodeassert.deepStrictEqual, and update Chai-only assertions tochaiAssert.Example edits:
- assert.deepStrictEqual(envVars, {}); + assert.deepStrictEqual(envVars, {}); - assert.property(envVars, 'SQL_MY_POSTGRES_DB'); + chaiAssert.property(envVars, 'SQL_MY_POSTGRES_DB'); - assert.deepStrictEqual(credentialsJson.params, { sslmode: 'require' }); + assert.deepStrictEqual(credentialsJson.params, { sslmode: 'require' });Perform the same replacements at the other highlighted lines.
Also applies to: 39-39, 52-52, 64-64, 78-78, 111-111, 142-145, 243-243
⛔ Skipped due to learnings
Learnt from: CR PR: deepnote/vscode-deepnote#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-09T11:21:57.494Z Learning: Applies to **/*.unit.test.ts : Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual propertiessrc/notebooks/deepnote/integrations/sqlIntegrationEnvironmentVariablesProvider.ts (1)
59-61: Based on my verification, the review comment is incorrect. The suggestedInvalidConfigurationErrorclass does not exist insrc/platform/errors/. The codebase search revealed 20+ error classes available, but none namedInvalidConfigurationError.While the intent aligns with coding guidelines (use typed error classes from
src/platform/errors/), the suggested implementation cannot work as written. Available error classes includeWrappedError,BaseError, and domain-specific errors likeDeepnoteServerNotFoundError, but no configuration-specific error exists.Appropriate next steps:
- Create a new
InvalidConfigurationErrorclass insrc/platform/errors/if configuration validation errors are common- Or use
WrappedErroras a generic fallback for nowThe review comment needs correction to either suggest an existing error class or acknowledge that a new one must be created.
Likely an incorrect or invalid review comment.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
Outdated
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
Outdated
Show resolved
Hide resolved
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts
Show resolved
Hide resolved
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 23
♻️ Duplicate comments (9)
src/kernels/raw/launcher/kernelEnvVarsService.node.ts (2)
21-21: Avoid concrete import; depend on a platform interface.Replace the concrete SqlIntegrationEnvironmentVariablesProvider import with a platform-defined ISqlIntegrationEnvVarsProvider.
-import { SqlIntegrationEnvironmentVariablesProvider } from '../../../platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider'; +import { ISqlIntegrationEnvVarsProvider } from '../../../platform/notebooks/deepnote/types';
35-38: Inject interface token, not class.- @inject(SqlIntegrationEnvironmentVariablesProvider) - @optional() - private readonly sqlIntegrationEnvVars?: SqlIntegrationEnvironmentVariablesProvider + @inject(ISqlIntegrationEnvVarsProvider) + @optional() + private readonly sqlIntegrationEnvVars?: ISqlIntegrationEnvVarsProvidersrc/kernels/deepnote/deepnoteServerStarter.node.ts (1)
172-177: Stop logging secret values.Printing substrings of env var values leaks credentials. Remove value logging; log keys/count only.
- Object.assign(env, sqlEnvVars); - // Log the first 100 chars of each env var value for debugging - for (const [key, value] of Object.entries(sqlEnvVars)) { - if (value) { - logger.info(`DeepnoteServerStarter: ${key} = ${value.substring(0, 100)}...`); - } - } + Object.assign(env, sqlEnvVars); + logger.info( + `DeepnoteServerStarter: Injected SQL env vars: ${Object.keys(sqlEnvVars).join(', ')}` + );src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts (2)
29-33: Register for Deepnote view to actually activate.You gate on DEEPNOTE_NOTEBOOK_TYPE later, but register for JupyterNotebookView, so this won’t run for Deepnote notebooks. Register for Deepnote (and optionally Jupyter if needed).
As per coding guidelines.
- logger.info('SqlIntegrationStartupCodeProvider: Activating and registering with JupyterNotebookView'); - this.registry.register(this, JupyterNotebookView); - logger.info('SqlIntegrationStartupCodeProvider: Successfully registered'); + logger.info('SqlIntegrationStartupCodeProvider: Activating and registering with Deepnote view'); + this.registry.register(this, DEEPNOTE_NOTEBOOK_TYPE); + // Optional hybrid support: + // this.registry.register(this, JupyterNotebookView); + logger.info('SqlIntegrationStartupCodeProvider: Successfully registered');
108-111: Do not log the startup code (contains secrets).Remove raw code logging; log only metadata (length).
As per coding guidelines.
- const fullCode = code.join('\n'); - logger.info(`SqlIntegrationStartupCodeProvider: Generated startup code (${fullCode.length} chars):`); - logger.info(fullCode); + const fullCode = code.join('\n'); + logger.info( + `SqlIntegrationStartupCodeProvider: Generated startup code (${fullCode.length} chars); content omitted for security.` + );src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (4)
150-152: Stop logging credential values.This prints secrets. Remove it.
As per coding guidelines.
- logger.info( - `SqlIntegrationEnvironmentVariablesProvider: Env var value: ${credentialsJson.substring(0, 100)}...` - ); + // Do not log credential contents.
36-41: URL‑encode the database name in the Postgres URL.Database names can contain special chars; encode the path segment.
- const url = `postgresql://${encodedUsername}:${encodedPassword}@${config.host}:${config.port}/${config.database}`; + const encodedDb = encodeURIComponent(config.database); + const url = `postgresql://${encodedUsername}:${encodedPassword}@${config.host}:${config.port}/${encodedDb}`;
75-87: Dispose the EventEmitter to avoid leaks.Push the emitter to the disposable registry.
As per coding guidelines.
constructor( @inject(IIntegrationStorage) private readonly integrationStorage: IIntegrationStorage, @inject(IDisposableRegistry) disposables: IDisposableRegistry ) { logger.info('SqlIntegrationEnvironmentVariablesProvider: Constructor called - provider is being instantiated'); + // Dispose emitter on shutdown + disposables.push(this._onDidChangeEnvironmentVariables); // Listen for changes to integration storage and fire change event disposables.push( this.integrationStorage.onDidChangeIntegrations(() => { // Fire change event for all notebooks this._onDidChangeEnvironmentVariables.fire(undefined); }) ); }
92-101: Respect CancellationToken to avoid unnecessary work.Early‑exit when cancelled; also check inside the loop.
As per coding guidelines.
- public async getEnvironmentVariables( - resource: Resource, - _token?: CancellationToken - ): Promise<EnvironmentVariables> { + public async getEnvironmentVariables( + resource: Resource, + token?: CancellationToken + ): Promise<EnvironmentVariables> { const envVars: EnvironmentVariables = {}; if (!resource) { return envVars; } + if (token?.isCancellationRequested) { + return envVars; + } ... - for (const integrationId of integrationIds) { + for (const integrationId of integrationIds) { + if (token?.isCancellationRequested) { + break; + } try { const config = await this.integrationStorage.get(integrationId);Also applies to: 131-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
src/kernels/deepnote/deepnoteServerStarter.node.ts(4 hunks)src/kernels/helpers.unit.test.ts(1 hunks)src/kernels/raw/launcher/kernelEnvVarsService.node.ts(8 hunks)src/kernels/raw/launcher/kernelEnvVarsService.unit.test.ts(14 hunks)src/kernels/serviceRegistry.node.ts(2 hunks)src/notebooks/deepnote/integrations/integrationDetector.ts(1 hunks)src/notebooks/deepnote/integrations/integrationManager.ts(1 hunks)src/notebooks/deepnote/integrations/integrationUtils.ts(1 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(1 hunks)src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts(1 hunks)src/notebooks/deepnote/integrations/types.ts(1 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.ts(1 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(1 hunks)src/notebooks/serviceRegistry.node.ts(3 hunks)src/notebooks/serviceRegistry.web.ts(1 hunks)src/platform/notebooks/deepnote/integrationStorage.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts(1 hunks)src/platform/notebooks/deepnote/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.web.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
*.web.tsfor Web-specific implementations using browser-compatible APIs
Files:
src/notebooks/serviceRegistry.web.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/serviceRegistry.web.tssrc/notebooks/serviceRegistry.node.tssrc/kernels/helpers.unit.test.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/platform/notebooks/deepnote/integrationStorage.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/notebooks/deepnote/integrations/integrationUtils.tssrc/kernels/serviceRegistry.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.node.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/types.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.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.tssrc/kernels/serviceRegistry.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.node.tssrc/kernels/deepnote/deepnoteServerStarter.node.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/kernels/helpers.unit.test.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/notebooks/deepnote/types.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/platform/notebooks/deepnote/integrationStorage.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/notebooks/deepnote/integrations/integrationUtils.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/kernels/helpers.unit.test.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/helpers.unit.test.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/kernels/serviceRegistry.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.node.tssrc/kernels/deepnote/deepnoteServerStarter.node.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/kernels/helpers.unit.test.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/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/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/platform/notebooks/deepnote/types.tssrc/platform/notebooks/deepnote/integrationStorage.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/notebooks/deepnote/integrations/integrationUtils.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
src/kernels/raw/launcher/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Raw launcher must spawn kernel processes and manage ports/connection files
Files:
src/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/kernels/raw/launcher/kernelEnvVarsService.node.ts
src/kernels/{kernelFinder.ts,serviceRegistry.{node,web}.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Finders must be registered with the KernelFinder on startup
Files:
src/kernels/serviceRegistry.node.ts
src/kernels/serviceRegistry.node.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Node service registry must register raw kernel components and enable filesystem-based discovery
Files:
src/kernels/serviceRegistry.node.ts
src/kernels/serviceRegistry.{node,web}.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use dependency injection for services to enable testability and platform variance
Files:
src/kernels/serviceRegistry.node.ts
🧠 Learnings (8)
📚 Learning: 2025-09-03T12:55:29.175Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/ipywidgets.instructions.md:0-0
Timestamp: 2025-09-03T12:55:29.175Z
Learning: Applies to src/notebooks/controllers/ipywidgets/serviceRegistry.{node,web}.ts : Register DI services in platform-specific registries (serviceRegistry.node.ts vs serviceRegistry.web.ts)
Applied to files:
src/notebooks/serviceRegistry.web.tssrc/notebooks/serviceRegistry.node.tssrc/kernels/serviceRegistry.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/notebooks/serviceRegistry.web.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles Deepnote notebook state management
Applied to files:
src/notebooks/serviceRegistry.web.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/integrations/integrationDetector.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/** : Deepnote integration code resides under src/notebooks/deepnote/
Applied to files:
src/notebooks/serviceRegistry.web.tssrc/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Use dependency injection with inversify
Applied to files:
src/platform/notebooks/deepnote/integrationStorage.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/serviceRegistry.{node,web}.ts : Use dependency injection for services to enable testability and platform variance
Applied to files:
src/kernels/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:56:39.535Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel-jupyter.instructions.md:0-0
Timestamp: 2025-09-03T12:56:39.535Z
Learning: Applies to src/kernels/jupyter/serviceRegistry.{node,web}.ts : Use dependency injection via the service registry for component creation and registration
Applied to files:
src/kernels/serviceRegistry.node.ts
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/**/*.ts : Use EventEmitter for event-driven updates and ensure proper disposal/cleanup of listeners to prevent leaks
Applied to files:
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
🧬 Code graph analysis (4)
src/kernels/helpers.unit.test.ts (3)
src/test/mocks/vsc/extHostedTypes.ts (1)
code(2303-2305)src/kernels/execution/cellExecution.ts (1)
result(69-71)src/kernels/kernel.ts (1)
executeSilently(1080-1093)
src/platform/notebooks/deepnote/types.ts (2)
src/platform/common/types.ts (1)
IDisposable(211-213)src/platform/notebooks/deepnote/integrationTypes.ts (1)
IntegrationConfig(49-49)
src/kernels/raw/launcher/kernelEnvVarsService.node.ts (2)
src/platform/common/types.ts (2)
IConfigurationService(130-130)IConfigurationService(131-141)src/platform/logging/index.ts (1)
logger(35-48)
src/kernels/deepnote/deepnoteServerStarter.node.ts (2)
src/platform/common/types.ts (2)
IAsyncDisposableRegistry(232-232)IAsyncDisposableRegistry(233-235)src/platform/logging/index.ts (1)
logger(35-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Check Licenses
- GitHub Check: Build & Test
🔇 Additional comments (14)
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
9-13: Import path refactoring looks correct.Types moved to platform layer as expected.
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (1)
16-16: Import path updated correctly.Test references now use platform-level types.
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
18-18: Platform import path correct.src/notebooks/serviceRegistry.web.ts (1)
41-41: IntegrationStorage import moved to platform layer.Registration binding unchanged at line 106.
src/notebooks/deepnote/integrations/integrationUtils.ts (1)
3-7: Platform types import correct.Utility function logic unchanged.
src/notebooks/deepnote/integrations/integrationManager.ts (1)
8-8: Platform import correct.src/platform/notebooks/deepnote/integrationStorage.ts (1)
4-6: Import paths adjusted for platform location.Relative paths shortened correctly after file relocation.
src/notebooks/deepnote/integrations/integrationDetector.ts (1)
5-5: Platform import refactoring verified across codebase.No old local integrationTypes or integrationStorage imports remain. All 9 files using integration types now correctly import from the platform path. The integrationDetector.ts import at line 5 is correct and consistent with the pattern throughout the codebase.
src/notebooks/serviceRegistry.node.ts (1)
151-154: Good DI wiring for SQL startup code provider.Singleton + activation binding look correct; no issues.
src/notebooks/deepnote/integrations/types.ts (1)
1-4: Consolidation to platform types looks right.Re-exporting IIntegrationStorage from platform removes duplication and avoids drift. LGTM.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (1)
115-147: BigQuery case looks good.Asserting parsed JSON is helpful; no issues here.
src/kernels/raw/launcher/kernelEnvVarsService.node.ts (2)
86-119: Good: token respected and provider guarded.Parallel fetch and guarded merge are sound; no issues here.
21-21: Verification passed: no forbidden cross-layer imports detected.The import at line 21 is from the platform layer (
../../../platform/notebooks/), which is architecturally allowed. The script's regex pattern produces false positives for platform imports; no actual violations exist.src/kernels/raw/launcher/kernelEnvVarsService.unit.test.ts (1)
280-417: Great coverage for SQL env var scenarios.Covers Python/non‑Python, empty/undefined/error provider, multi‑var, and optional dependency paths. LGTM.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
Outdated
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts (1)
78-82: Remove env var key names from logs.Logging key names exposes integration IDs. Log count only.
logger.info( `SqlIntegrationStartupCodeProvider: Injecting ${ Object.keys(envVars).length - } SQL integration env vars into kernel: ${Object.keys(envVars).join(', ')}` + } SQL integration env vars into kernel` );INTEGRATIONS_CREDENTIALS.md (1)
357-357: Fix contradictory logging policy.Line states credentials "are not logged" but allows "first 100 chars for debugging." Remove the parenthetical.
-5. **No Logging**: Credential values are not logged (only first 100 chars for debugging) +5. **No Logging**: Credential values are never logged; only non-sensitive metadata (key names, counts) is loggedsrc/kernels/deepnote/deepnoteServerStarter.node.ts (2)
52-54: Inject via interface, not concrete class.Violates DI pattern. Create
ISqlIntegrationEnvVarsProviderinterface insrc/platform/notebooks/deepnote/types.tswith Symbol token, bind in service registry, inject interface here.- @inject(SqlIntegrationEnvironmentVariablesProvider) + @inject(ISqlIntegrationEnvVarsProvider) @optional() - private readonly sqlIntegrationEnvVars?: SqlIntegrationEnvironmentVariablesProvider + private readonly sqlIntegrationEnvVars?: ISqlIntegrationEnvVarsProviderAs per coding guidelines (inject interfaces).
162-169: Lower log level to debug.Per-notebook injection logs create noise at info level.
- logger.info(`DeepnoteServerStarter: Injecting SQL integration env vars for ${deepnoteFileUri.toString()}`); + logger.debug(`DeepnoteServerStarter: Injecting SQL integration env vars for ${deepnoteFileUri.toString()}`); try { const sqlEnvVars = await this.sqlIntegrationEnvVars.getEnvironmentVariables(deepnoteFileUri, token); if (sqlEnvVars && Object.keys(sqlEnvVars).length > 0) { - logger.info( + logger.debug(src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (2)
60-61: Use typed error class.Generic
Errorshould be replaced with typed error fromsrc/platform/errors/.default: - throw new Error(`Unsupported integration type: ${(config as IntegrationConfig).type}`); + throw new UnsupportedIntegrationTypeError((config as IntegrationConfig).type);Define
UnsupportedIntegrationTypeErrorinsrc/platform/errors/if it doesn't exist.As per coding guidelines.
106-111: Lower log verbosity to trace level.Info-level logs expose URIs, integration IDs, and env var keys. Use trace, log counts only.
- logger.info(`SqlIntegrationEnvironmentVariablesProvider: Getting env vars for resource ${resource.toString()}`); - logger.info( - `SqlIntegrationEnvironmentVariablesProvider: Available notebooks: ${workspace.notebookDocuments - .map((nb) => nb.uri.toString()) - .join(', ')}` - ); + logger.trace(`SqlIntegrationEnvironmentVariablesProvider: Getting env vars for ${workspace.notebookDocuments.length} notebook(s)`);Apply same pattern to lines 129-132, 155-157, 166-169.
As per coding guidelines (scrub PII).
Also applies to: 129-132, 155-157, 166-169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
INTEGRATIONS_CREDENTIALS.md(1 hunks)src/kernels/deepnote/deepnoteServerStarter.node.ts(4 hunks)src/kernels/kernel.ts(1 hunks)src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/!(*.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/sqlIntegrationStartupCodeProvider.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/kernels/kernel.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/sqlIntegrationStartupCodeProvider.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/kernels/kernel.tssrc/kernels/deepnote/deepnoteServerStarter.node.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.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.ts
src/kernels/kernel.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Kernel must manage sessions via IKernelSession and implement lifecycle hooks (willRestart, willInterrupt, etc.) with startup code execution (e.g., matplotlib, IPyWidgets)
Files:
src/kernels/kernel.ts
src/kernels/{kernel.ts,kernelSessionFactory.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Prefer session reuse to avoid unnecessary kernel restarts
Files:
src/kernels/kernel.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/kernel.tssrc/kernels/deepnote/deepnoteServerStarter.node.ts
**/*.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/kernels/deepnote/deepnoteServerStarter.node.ts
🧠 Learnings (1)
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts
🧬 Code graph analysis (2)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (5)
src/platform/notebooks/deepnote/integrationTypes.ts (2)
IntegrationConfig(49-49)DATAFRAME_SQL_INTEGRATION_ID(5-5)src/platform/notebooks/deepnote/integrationStorage.ts (1)
injectable(17-189)src/platform/common/types.ts (1)
Resource(19-19)src/notebooks/deepnote/integrations/types.ts (1)
IIntegrationStorage(4-4)src/platform/common/variables/types.ts (1)
EnvironmentVariables(8-8)
src/kernels/kernel.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
🪛 markdownlint-cli2 (0.18.1)
INTEGRATIONS_CREDENTIALS.md
322-322: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
334-334: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
src/kernels/kernel.ts (1)
856-870: LGTM—proper observability without secrets leakage.Trace-level logging with output types only. No credential exposure.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/kernels/raw/launcher/kernelEnvVarsService.node.ts (1)
84-99: Avoid shadowing class fields.Local
customEnvVarsshadows the injected provider field. Rename the local to improve clarity.Apply:
- let [customEnvVars, interpreterEnv, sqlIntegrationEnvVars] = await Promise.all([ + let [customEnvFromProvider, interpreterEnv, sqlIntegrationEnvVars] = await Promise.all([ @@ - if (!interpreterEnv && Object.keys(customEnvVars || {}).length === 0) { + if (!interpreterEnv && Object.keys(customEnvFromProvider || {}).length === 0) { @@ - customEnvVars = customEnvVars || {}; + const customEnvVars = customEnvFromProvider || {}; @@ - this.envVarsService.mergeVariables(customEnvVars, mergedVars); // custom vars win over all. + this.envVarsService.mergeVariables(customEnvVars, mergedVars); // custom vars win over all.Also applies to: 125-141, 180-181
♻️ Duplicate comments (5)
src/platform/notebooks/deepnote/types.ts (1)
13-19: ClarifygetvsgetIntegrationConfig.Add brief JSDoc for
get(...)to distinguish it fromgetIntegrationConfig(...)(global canonical vs project-scoped effective config).Apply:
getAll(): Promise<IntegrationConfig[]>; - get(integrationId: string): Promise<IntegrationConfig | undefined>; + /** + * Retrieve the canonical integration by id (not project-scoped). + * Returns undefined if missing. + */ + get(integrationId: string): Promise<IntegrationConfig | undefined>;As per coding guidelines.
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts (3)
112-116: Scrub PII in logs (URIs, integration IDs).Avoid logging notebook URIs and integration IDs; log counts and non-sensitive summaries at debug/trace.
Apply:
- logger.warn(`SqlIntegrationEnvironmentVariablesProvider: No notebook found for ${resource.toString()}`); + logger.warn('SqlIntegrationEnvironmentVariablesProvider: No notebook found for resource'); @@ - logger.info( - `SqlIntegrationEnvironmentVariablesProvider: Added env var ${envVarName} for integration ${integrationId}` - ); + logger.debug(`SqlIntegrationEnvironmentVariablesProvider: Added env var ${envVarName}`); @@ - logger.trace( - `SqlIntegrationEnvironmentVariablesProvider: Found integration ${integrationId} in cell ${cell.index}` - ); + logger.trace( + `SqlIntegrationEnvironmentVariablesProvider: Found SQL cell at index ${cell.index}` + );As per coding guidelines.
Also applies to: 149-151, 187-189
60-62: Use a typed error, notError.Replace the generic
Errorwith a typed error fromsrc/platform/errors/(e.g., NotSupported/UnsupportedIntegrationType). This enables programmatic handling and consistent telemetry.As per coding guidelines.
129-158: Fetch credentials in parallel to reduce latency.Map IDs to promises and
Promise.allSettledto avoid N+1 round-trips serially.Apply:
- // Get credentials for each integration and add to environment variables - for (const integrationId of integrationIds) { - if (token?.isCancellationRequested) { - break; - } - - try { - const config = await this.integrationStorage.get(integrationId); - if (!config) { - logger.warn( - `SqlIntegrationEnvironmentVariablesProvider: No configuration found for integration ${integrationId}` - ); - continue; - } - - // Convert integration config to JSON and add as environment variable - const envVarName = convertToEnvironmentVariableName(getSqlEnvVarName(integrationId)); - const credentialsJson = convertIntegrationConfigToJson(config); - - envVars[envVarName] = credentialsJson; - logger.info( - `SqlIntegrationEnvironmentVariablesProvider: Added env var ${envVarName} for integration ${integrationId}` - ); - } catch (error) { - logger.error( - `SqlIntegrationEnvironmentVariablesProvider: Failed to get credentials for integration ${integrationId}`, - error - ); - } - } + // Get credentials for each integration and add to environment variables (in parallel) + const ids = Array.from(integrationIds); + const pending = ids.map(async (id) => { + const cfg = await this.integrationStorage.get(id); + return { id, cfg }; + }); + const results = await Promise.allSettled(pending); + if (token?.isCancellationRequested) { + return envVars; + } + for (const r of results) { + if (r.status !== 'fulfilled') { + logger.error('SqlIntegrationEnvironmentVariablesProvider: Failed to get credentials for an integration'); + continue; + } + const { id, cfg } = r.value; + if (!cfg) { + logger.warn('SqlIntegrationEnvironmentVariablesProvider: No configuration found for an integration'); + continue; + } + const envVarName = convertToEnvironmentVariableName(getSqlEnvVarName(id)); + envVars[envVarName] = convertIntegrationConfigToJson(cfg); + logger.debug(`SqlIntegrationEnvironmentVariablesProvider: Added env var ${envVarName}`); + }As per coding guidelines.
src/kernels/deepnote/deepnoteServerStarter.node.ts (1)
160-177: Drop duplicate log and avoid logging full error objects.Remove the second “Injected SQL env vars” log; keep one summary. Log sanitized error messages only.
Apply:
- Object.assign(env, sqlEnvVars); - logger.debug(`DeepnoteServerStarter: Injected SQL env vars: ${Object.keys(sqlEnvVars).join(', ')}`); + Object.assign(env, sqlEnvVars); @@ - } catch (error) { - logger.error('DeepnoteServerStarter: Failed to get SQL integration env vars', error); + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + logger.error(`DeepnoteServerStarter: Failed to get SQL integration env vars: ${msg}`); }As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
src/kernels/deepnote/deepnoteServerStarter.node.ts(4 hunks)src/kernels/helpers.unit.test.ts(1 hunks)src/kernels/raw/launcher/kernelEnvVarsService.node.ts(8 hunks)src/kernels/raw/launcher/kernelEnvVarsService.unit.test.ts(15 hunks)src/kernels/serviceRegistry.node.ts(2 hunks)src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts(1 hunks)src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts(1 hunks)src/platform/notebooks/deepnote/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.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/kernels/serviceRegistry.node.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.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
**/*.{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/kernels/serviceRegistry.node.tssrc/platform/notebooks/deepnote/types.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.node.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.tssrc/kernels/helpers.unit.test.ts
src/kernels/{kernelFinder.ts,serviceRegistry.{node,web}.ts}
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Finders must be registered with the KernelFinder on startup
Files:
src/kernels/serviceRegistry.node.ts
src/kernels/serviceRegistry.node.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Node service registry must register raw kernel components and enable filesystem-based discovery
Files:
src/kernels/serviceRegistry.node.ts
src/kernels/serviceRegistry.{node,web}.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Use dependency injection for services to enable testability and platform variance
Files:
src/kernels/serviceRegistry.node.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/serviceRegistry.node.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/kernels/helpers.unit.test.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/types.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.tssrc/kernels/helpers.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/types.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.tssrc/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
src/kernels/raw/launcher/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Raw launcher must spawn kernel processes and manage ports/connection files
Files:
src/kernels/raw/launcher/kernelEnvVarsService.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/kernels/helpers.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.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.tssrc/kernels/helpers.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts
🧠 Learnings (4)
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/serviceRegistry.{node,web}.ts : Use dependency injection for services to enable testability and platform variance
Applied to files:
src/kernels/serviceRegistry.node.tssrc/kernels/raw/launcher/kernelEnvVarsService.unit.test.ts
📚 Learning: 2025-10-07T15:31:30.150Z
Learnt from: hc2p
PR: deepnote/deepnote-internal#18615
File: apps/webapp-client/src/api/integration-api.ts:204-204
Timestamp: 2025-10-07T15:31:30.150Z
Learning: In apps/webapp-client/src/api/integration-api.ts, when reviewing bug fixes authored by the PR creator (e.g., changing integration.state to integration.type), use informational tone rather than marking as critical issues, since the fix is already implemented.
Applied to files:
src/platform/notebooks/deepnote/types.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/**/*.ts : Monitor and dispose pending promises to prevent leaks during teardown
Applied to files:
src/kernels/helpers.unit.test.ts
🧬 Code graph analysis (6)
src/kernels/serviceRegistry.node.ts (1)
src/platform/notebooks/deepnote/types.ts (2)
ISqlIntegrationEnvVarsProvider(27-27)ISqlIntegrationEnvVarsProvider(28-38)
src/platform/notebooks/deepnote/types.ts (4)
src/notebooks/deepnote/integrations/types.ts (1)
IIntegrationStorage(4-4)src/platform/common/types.ts (2)
IDisposable(211-213)Resource(19-19)src/platform/notebooks/deepnote/integrationTypes.ts (1)
IntegrationConfig(49-49)src/platform/common/variables/types.ts (1)
EnvironmentVariables(8-8)
src/kernels/raw/launcher/kernelEnvVarsService.node.ts (2)
src/platform/common/types.ts (2)
IConfigurationService(130-130)IConfigurationService(131-141)src/platform/logging/index.ts (1)
logger(35-48)
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts (2)
src/platform/notebooks/deepnote/integrationTypes.ts (2)
PostgresIntegrationConfig(27-35)BigQueryIntegrationConfig(40-44)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts (3)
src/platform/common/constants.ts (1)
JupyterNotebookView(13-13)src/kernels/helpers.ts (1)
isPythonKernelConnection(454-466)src/kernels/deepnote/types.ts (1)
DEEPNOTE_NOTEBOOK_TYPE(160-160)
src/kernels/helpers.unit.test.ts (2)
src/kernels/kernel.ts (1)
executeSilently(1080-1093)src/kernels/helpers.ts (1)
executeSilently(636-730)
🪛 GitHub Check: TypeCheck
src/kernels/deepnote/deepnoteServerStarter.node.ts
[failure] 20-20:
'ISqlIntegrationEnvVarsProvider' is declared but its value is never read.
[failure] 54-54:
Cannot find name 'SqlIntegrationEnvironmentVariablesProvider'. Did you mean 'ISqlIntegrationEnvVarsProvider'?
[failure] 52-52:
Cannot find name 'SqlIntegrationEnvironmentVariablesProvider'. Did you mean 'ISqlIntegrationEnvVarsProvider'?
⏰ 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: Lint & Format
🔇 Additional comments (5)
src/kernels/helpers.unit.test.ts (2)
650-700: Helper addresses past timing issues effectively.The
createMockKernelhelper consolidates the mock setup and fixes the race condition from the previous setTimeout approach by dispatching IOPub messages synchronously before resolvingdone.
753-810: Error and display_data tests look solid.Definitive assertions and proper use of
assert.deepStrictEqualfor object comparisons.src/kernels/serviceRegistry.node.ts (1)
67-70: Binding looks good.Registered the provider under the interface token; enables alternate impls and testing.
src/kernels/raw/launcher/kernelEnvVarsService.unit.test.ts (1)
76-97: Nice DRY helper.Factory simplifies setup and keeps tests readable. Good use of optional override.
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts (1)
67-76: Startup code generation looks correct and secret-safe.
- Correctly gated to Python/Deepnote.
- Only logs counts; no secret leakage.
- JSON.stringify ensures proper escaping for Python string literals.
Also applies to: 78-83, 87-110
src/notebooks/deepnote/integrations/sqlIntegrationStartupCodeProvider.ts
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.ts
Outdated
Show resolved
Hide resolved
src/platform/notebooks/deepnote/sqlIntegrationEnvironmentVariablesProvider.unit.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/kernels/raw/launcher/kernelEnvVarsService.node.ts (1)
104-109: Redact env-var keys in logs.Listing keys can leak sensitive metadata. Log count only.
Apply:- logger.debug( - `KernelEnvVarsService: Got ${Object.keys(vars).length} SQL integration env vars: ${Object.keys(vars).join(', ')}` - ); + logger.debug( + `KernelEnvVarsService: Got ${Object.keys(vars).length} SQL integration env vars` + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/kernels/raw/launcher/kernelEnvVarsService.node.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/kernels/raw/launcher/kernelEnvVarsService.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
**/*.{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/kernels/raw/launcher/kernelEnvVarsService.node.ts
src/kernels/raw/launcher/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
Raw launcher must spawn kernel processes and manage ports/connection files
Files:
src/kernels/raw/launcher/kernelEnvVarsService.node.ts
src/kernels/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/kernel.instructions.md)
src/kernels/**/*.ts: Use event-driven updates (EventEmitter) for state changes
Monitor and dispose pending promises to prevent leaks during teardown
Use WeakRef/weak references for notebook/object backreferences to avoid memory leaks
Respect CancellationToken in all async operations
Files:
src/kernels/raw/launcher/kernelEnvVarsService.node.ts
🧠 Learnings (2)
📚 Learning: 2025-09-03T12:58:18.091Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/kernel.instructions.md:0-0
Timestamp: 2025-09-03T12:58:18.091Z
Learning: Applies to src/kernels/serviceRegistry.{node,web}.ts : Use dependency injection for services to enable testability and platform variance
Applied to files:
src/kernels/raw/launcher/kernelEnvVarsService.node.ts
📚 Learning: 2025-09-03T13:00:18.307Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/platform.instructions.md:0-0
Timestamp: 2025-09-03T13:00:18.307Z
Learning: Applies to src/platform/**/*.ts : Use Inversify decorators for DI: annotate classes with injectable() and inject dependencies with inject(Token)
Applied to files:
src/kernels/raw/launcher/kernelEnvVarsService.node.ts
🧬 Code graph analysis (1)
src/kernels/raw/launcher/kernelEnvVarsService.node.ts (3)
src/platform/common/types.ts (2)
IConfigurationService(130-130)IConfigurationService(131-141)src/platform/notebooks/deepnote/types.ts (2)
ISqlIntegrationEnvVarsProvider(44-44)ISqlIntegrationEnvVarsProvider(45-55)src/platform/common/variables/types.ts (1)
EnvironmentVariables(8-8)
🔇 Additional comments (3)
src/kernels/raw/launcher/kernelEnvVarsService.node.ts (3)
21-21: Good: platform-level interface injection; layering fixed.Switched to ISqlIntegrationEnvVarsProvider with optional DI. This aligns with the platform boundary and DI guidance.
If not already, ensure the concrete provider is bound to ISqlIntegrationEnvVarsProvider in the node service registry. As per coding guidelines.Also applies to: 35-41
183-191: Non‑Python merge path LGTM.Uses mergeVariables and debug level; no secrets logged.
205-205: Typed substituteEnvVars input LGTM.EnvironmentVariables type is appropriate here.
jankuca
requested review from
Artmann,
andyjakubowski,
jamesbhobbs and
saltenasl
as code owners
saltenasl
left a comment
saltenasl
left a comment
There was a problem hiding this comment.
I am able to configure a postgresql integration and query a database set-up on local. Notres for future improvements:
- It says my integration is unnamed despite it having a name
- it says the dialect is MS SQL despite the type of integration being
pgsql - If an integration is in the file but not assigned in any block - it won't come up in the "manage integrations" view.
- Manage integrations view should use integration name instead of id
- When clicked on "Manage integrations" it only displays integrations from my current notebook as opposed to current project (when there's another notebook that has a different integration). This will probably get fixed together with 3rd
You can see successfully executed SQL block after configuring the integration:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores