feat: SQL integration management #24
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds Deepnote integrations support: user-scoped encrypted storage for integration configs (Postgres, BigQuery), types and utils to detect SQL integrations in projects and notebook cells, a webview-based UI to list/configure/delete integrations, and an IntegrationManager that updates VS Code context and registers the Sequence Diagram(s)sequenceDiagram
participant User
participant VSCode
participant Activation as DeepnoteActivationService
participant Manager as IntegrationManager
participant Detector as IntegrationDetector
participant Storage as IntegrationStorage
participant Webview
note right of Activation #E8F0FF: extension activation
VSCode->>Activation: activate()
Activation->>Manager: integrationManager.activate()
note right of User #FFF4E0: user triggers UI
User->>VSCode: invoke deepnote.manageIntegrations (toolbar/command)
VSCode->>Manager: deepnote.manageIntegrations
Manager->>Detector: detectIntegrations(projectId)
Detector->>Storage: get(integrationId)
Storage-->>Detector: IntegrationConfig | undefined
Detector-->>Manager: Map<id, IntegrationWithStatus>
Manager->>Webview: show(integrations)
Webview-->>User: render UI
User->>Webview: configure / save / delete
Webview->>Storage: save(config) / delete(id)
Storage-->>Webview: success / error
Webview->>Manager: notify changes
Manager->>VSCode: updateContext() (set hasIntegrations / hasUnconfiguredIntegrations)
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (27)
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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
package.json(2 hunks)src/notebooks/deepnote/deepnoteActivationService.ts(3 hunks)src/notebooks/deepnote/deepnoteActivationService.unit.test.ts(3 hunks)src/notebooks/deepnote/deepnoteSerializer.ts(6 hunks)src/notebooks/deepnote/deepnoteTypes.ts(2 hunks)src/notebooks/deepnote/integrations/integrationDetector.ts(1 hunks)src/notebooks/deepnote/integrations/integrationManager.ts(1 hunks)src/notebooks/deepnote/integrations/integrationStorage.ts(1 hunks)src/notebooks/deepnote/integrations/integrationTypes.ts(1 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(1 hunks)src/notebooks/deepnote/integrations/types.ts(1 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/notebooks/serviceRegistry.web.ts(2 hunks)src/notebooks/types.ts(2 hunks)src/platform/common/constants.ts(1 hunks)test-integration.deepnote(1 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/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationStorage.tssrc/platform/common/constants.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/integrations/integrationTypes.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/types.tssrc/notebooks/deepnote/integrations/integrationManager.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, then alphabetically
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationStorage.tssrc/platform/common/constants.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/integrations/integrationTypes.tssrc/notebooks/serviceRegistry.web.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/types.tssrc/notebooks/deepnote/integrations/integrationManager.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationStorage.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/integrations/integrationTypes.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/integrations/integrationManager.ts
src/notebooks/deepnote/deepnoteTypes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteTypes.ts should contain only type definitions
Files:
src/notebooks/deepnote/deepnoteTypes.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Add a blank line after const groups and before return statements (whitespace for readability)
Files:
src/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationStorage.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/integrations/integrationTypes.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/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: Name unit test files with the .unit.test.ts extension and use Mocha/Chai
Place unit test files alongside the source files they test
Files:
src/notebooks/deepnote/deepnoteActivationService.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/deepnoteActivationService.unit.test.ts
{**/*.unit.test.ts,out/**/*.unit.test.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use assert.deepStrictEqual() for object comparisons in tests
Files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
src/notebooks/deepnote/deepnoteSerializer.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteSerializer.ts acts as the main serializer/orchestrator
Files:
src/notebooks/deepnote/deepnoteSerializer.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/common/constants.ts
**/*.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
src/notebooks/deepnote/deepnoteActivationService.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteActivationService.ts wires VSCode activation
Files:
src/notebooks/deepnote/deepnoteActivationService.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.ts
🧠 Learnings (10)
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts should contain only type definitions
Applied to files:
src/notebooks/deepnote/deepnoteTypes.tssrc/notebooks/deepnote/integrations/integrationTypes.tssrc/notebooks/types.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous with signature `public activate()` and returns void. It should be called without await in tests.
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/deepnoteActivationService.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous and returns void, not a Promise. It should be called without await in tests.
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts wires VSCode activation
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/serviceRegistry.web.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteSerializer.ts : deepnoteSerializer.ts acts as the main serializer/orchestrator
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles state management
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/types.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/notebooks/serviceRegistry.web.tssrc/notebooks/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/notebooks/serviceRegistry.web.ts
📚 Learning: 2025-09-03T13:01:10.017Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/standalone.instructions.md:0-0
Timestamp: 2025-09-03T13:01:10.017Z
Learning: Applies to src/standalone/serviceRegistry.{node,web}.ts : Register new services for both platforms in serviceRegistry.node.ts and serviceRegistry.web.ts as appropriate
Applied to files:
src/notebooks/serviceRegistry.web.tssrc/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/** : Deepnote integration code resides under src/notebooks/deepnote/
Applied to files:
src/notebooks/serviceRegistry.node.ts
🧬 Code graph analysis (6)
src/notebooks/deepnote/integrations/integrationDetector.ts (3)
src/notebooks/deepnote/integrations/integrationManager.ts (1)
injectable(14-159)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationWithStatus(57-61)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/integrations/integrationStorage.ts (1)
src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationConfig(43-43)
src/notebooks/deepnote/integrations/types.ts (1)
src/notebooks/deepnote/integrations/integrationTypes.ts (2)
IntegrationConfig(43-43)IntegrationWithStatus(57-61)
src/notebooks/deepnote/deepnoteActivationService.ts (1)
src/notebooks/types.ts (2)
IDeepnoteNotebookManager(28-28)IDeepnoteNotebookManager(29-36)
src/notebooks/types.ts (1)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteProject(10-10)
🔇 Additional comments (13)
src/notebooks/deepnote/deepnoteSerializer.ts (2)
5-5: LGTM: Correct logger import.Follows guideline to use
ILoggerservice. Import organization (third-party, blank line, local) is correct.
39-39: LGTM: Logger usage is correct.Replaces
consolewithloggerper guidelines. Debug logs provide useful tracing; error logs preserve stack traces. No logic changes.Also applies to: 56-56, 68-68, 71-71, 86-86, 153-153
src/notebooks/deepnote/deepnoteTypes.ts (2)
32-37: LGTM!Clean interface definition for SQL integration metadata.
48-48: LGTM!Intersection type correctly extends metadata while preserving backward compatibility.
src/platform/common/constants.ts (1)
225-225: LGTM!Command constant correctly defined and matches package.json registration.
src/notebooks/serviceRegistry.web.ts (1)
100-103: LGTM!Integration services registered correctly following established DI patterns.
package.json (2)
88-93: LGTM!Command contribution properly defined with appropriate icon.
716-720: IntegrationManager registersdeepnote.manageIntegrationshandler
Confirmedcommands.registerCommand(Commands.ManageIntegrations, …)in src/notebooks/deepnote/integrations/integrationManager.ts:33.test-integration.deepnote (1)
1-31: LGTM!Test data properly structured to exercise integration detection and management workflows.
src/notebooks/serviceRegistry.node.ts (1)
134-137: LGTM!Integration services registered consistently with web registry, following platform-specific DI guidelines.
src/notebooks/deepnote/deepnoteActivationService.ts (2)
22-28: LGTM!Constructor properly injects
IntegrationManagerfollowing DI patterns. The constructor signature change will require test updates.
41-41: LGTM!Integration manager activation correctly added to the activation sequence. The synchronous
activate()call aligns with the existing pattern.src/notebooks/types.ts (1)
31-34: Compatibility verified
DeepnoteNotebookManager is the sole implementer of IDeepnoteNotebookManager and consistently uses DeepnoteProject in its method signatures and storage maps.
GRN-4764 Manage integrations
We need to implement a way for users to define integration credentials when using SQL blocks. The Deepnote file will contain the name and ID of the integrations connected to the project. We'll need an interface to display and edit them.
Create a task list from this and execute on the items. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/notebooks/deepnote/integrations/integrationManager.ts (2)
88-88: Localize user-facing strings.Hard-coded error/info messages must use
l10n.t()for localization.Apply this diff:
-import { commands, NotebookDocument, window, workspace } from 'vscode'; +import { commands, l10n, NotebookDocument, window, workspace } from 'vscode'; ... - void window.showErrorMessage('No active Deepnote notebook'); + void window.showErrorMessage(l10n.t('No active Deepnote notebook')); ... - void window.showErrorMessage('Cannot determine project ID'); + void window.showErrorMessage(l10n.t('Cannot determine project ID')); ... - void window.showInformationMessage(`No integrations found in this project.`); + void window.showInformationMessage(l10n.t('No integrations found in this project.'));As per coding guidelines
Also applies to: 94-94, 114-114
88-88: Localize user-facing strings.Hard-coded error messages should use
l10n.t()for internationalization. Also applies to lines 94 and 114.As per coding guidelines
Importl10nfromvscodeand wrap each message string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/notebooks/deepnote/integrations/integrationManager.ts(1 hunks)src/notebooks/deepnote/integrations/integrationStorage.ts(1 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(1 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/integrationManager.tssrc/notebooks/deepnote/integrations/integrationStorage.tssrc/notebooks/deepnote/integrations/integrationWebview.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, then alphabetically
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationStorage.tssrc/notebooks/deepnote/integrations/integrationWebview.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/integrationStorage.tssrc/notebooks/deepnote/integrations/integrationWebview.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Add a blank line after const groups and before return statements (whitespace for readability)
Files:
src/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationStorage.tssrc/notebooks/deepnote/integrations/integrationWebview.ts
🧠 Learnings (3)
📚 Learning: 2025-10-07T15:24:08.015Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationManager.ts:1-3
Timestamp: 2025-10-07T15:24:08.015Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required and should not be suggested for any files.
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.ts
📚 Learning: 2025-10-07T15:24:14.028Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationTypes.ts:1-7
Timestamp: 2025-10-07T15:24:14.028Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required for new files and should not be suggested.
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/integrations/integrationManager.ts (4)
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
injectable(12-738)src/notebooks/deepnote/integrations/integrationDetector.ts (1)
injectable(11-101)src/platform/logging/index.ts (1)
logger(35-48)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationWithStatus(57-61)
src/notebooks/deepnote/integrations/integrationStorage.ts (2)
src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationConfig(43-43)src/platform/logging/index.ts (1)
logger(35-48)
🪛 GitHub Actions: CI
src/notebooks/deepnote/integrations/integrationManager.ts
[error] 38-38: ESLint: no-void - Expected 'undefined' and instead saw 'void'.
🪛 GitHub Check: Lint & Format
src/notebooks/deepnote/integrations/integrationManager.ts
[failure] 44-44:
Expected 'undefined' and instead saw 'void'
[failure] 42-42:
Expected 'undefined' and instead saw 'void'
[failure] 38-38:
Expected 'undefined' and instead saw 'void'
🔇 Additional comments (7)
src/notebooks/deepnote/integrations/integrationManager.ts (3)
1-3: Past copyright header suggestion is obsolete.The prior review requested a Microsoft copyright header, but this repository does not require them.
Based on learnings
38-38: Void usage is allowed by ESLint
Your ESLint config setsno-voidwithallowAsStatement: true, sovoid this.updateContext()is permitted—no change needed.
126-160: LGTM!The fallback detection logic correctly scans cells when the project isn't stored, properly excludes internal integrations, and aligns with
IntegrationDetector's approach.src/notebooks/deepnote/integrations/integrationWebview.ts (2)
369-436: LGTM—XSS issue resolved.
renderIntegrationsnow correctly uses DOM APIs (createElement,textContent) instead ofinnerHTML, eliminating the XSS vector flagged in prior review.
369-436: XSS fix correctly applied.The previous innerHTML vulnerability has been eliminated. All integration data now renders via DOM APIs (
createElement,textContent,dataset), preventing script injection.src/notebooks/deepnote/integrations/integrationStorage.ts (2)
1-151: LGTM—logging issue resolved.The prior review requested replacing
console.errorwith the shared logger. This has been correctly implemented at lines 132 and 137, with appropriate context prefixes.
132-132: Logging fix correctly applied.The previous
console.errorcalls have been replaced withlogger.error, ensuring errors follow platform telemetry and scrubbing conventions. Also applies to line 137.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/notebooks/deepnote/integrations/integrationManager.ts (6)
104-104: Localize user-facing strings.Hard-coded error message should use
l10n.t()for localization.- void window.showErrorMessage('No active Deepnote notebook'); + void window.showErrorMessage(l10n.t('deepnote.noActiveNotebook', 'No active Deepnote notebook'));As per coding guidelines
110-110: Localize user-facing strings.Hard-coded error message should use
l10n.t()for localization.- void window.showErrorMessage('Cannot determine project ID'); + void window.showErrorMessage(l10n.t('deepnote.cannotDetermineProjectId', 'Cannot determine project ID'));As per coding guidelines
130-130: Localize user-facing strings.Hard-coded info message should use
l10n.t()for localization.- void window.showInformationMessage(`No integrations found in this project.`); + void window.showInformationMessage(l10n.t('deepnote.noIntegrationsFound', 'No integrations found in this project.'));Also add
l10nto the VS Code imports at the top:-import { commands, NotebookDocument, window, workspace } from 'vscode'; +import { commands, l10n, NotebookDocument, window, workspace } from 'vscode';As per coding guidelines
142-160: Consider extracting cell scanning logic.This method duplicates the cell-to-block transformation logic found in
IntegrationDetector. While the core scanning is now shared viascanBlocksForIntegrations, the cell metadata extraction could also be centralized.
104-130: Localize user-facing strings.Lines 104, 110, and 130 display hard-coded messages. Use
l10n.t()for localization.As per coding guidelines
142-160: Eliminate duplication with IntegrationDetector.This method mirrors
IntegrationDetector.detectIntegrations. Both scan forsql_integration_id, build block lists, and callscanBlocksForIntegrations. Extract a method onIntegrationDetectorthat acceptsNotebookDocumentdirectly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/notebooks/deepnote/integrations/integrationDetector.ts(1 hunks)src/notebooks/deepnote/integrations/integrationManager.ts(1 hunks)src/notebooks/deepnote/integrations/integrationTypes.ts(1 hunks)src/notebooks/deepnote/integrations/integrationUtils.ts(1 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/integrationUtils.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/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, then alphabetically
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/integrations/integrationUtils.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationTypes.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationUtils.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationTypes.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Add a blank line after const groups and before return statements (whitespace for readability)
Files:
src/notebooks/deepnote/integrations/integrationUtils.tssrc/notebooks/deepnote/integrations/integrationDetector.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationTypes.ts
🧠 Learnings (5)
📚 Learning: 2025-10-07T15:24:08.015Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationManager.ts:1-3
Timestamp: 2025-10-07T15:24:08.015Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required and should not be suggested for any files.
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationTypes.ts
📚 Learning: 2025-10-07T15:24:14.028Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationTypes.ts:1-7
Timestamp: 2025-10-07T15:24:14.028Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required for new files and should not be suggested.
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationTypes.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.ts
📚 Learning: 2025-09-09T11:31:30.479Z
Learnt from: Artmann
PR: deepnote/vscode-extension#11
File: src/notebooks/deepnote/deepnoteActivationService.ts:1-3
Timestamp: 2025-09-09T11:31:30.479Z
Learning: New files in the Deepnote VS Code extension project should not include copyright headers.
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationTypes.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts should contain only type definitions
Applied to files:
src/notebooks/deepnote/integrations/integrationTypes.ts
🧬 Code graph analysis (3)
src/notebooks/deepnote/integrations/integrationUtils.ts (2)
src/notebooks/deepnote/integrations/integrationTypes.ts (2)
IntegrationWithStatus(63-67)DATAFRAME_SQL_INTEGRATION_ID(5-5)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/integrations/integrationDetector.ts (3)
src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationWithStatus(63-67)src/platform/logging/index.ts (1)
logger(35-48)src/notebooks/deepnote/integrations/integrationUtils.ts (2)
BlockWithIntegration(8-11)scanBlocksForIntegrations(22-58)
src/notebooks/deepnote/integrations/integrationManager.ts (4)
src/notebooks/deepnote/integrations/integrationDetector.ts (1)
injectable(12-75)src/platform/logging/index.ts (1)
logger(35-48)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationWithStatus(63-67)src/notebooks/deepnote/integrations/integrationUtils.ts (2)
BlockWithIntegration(8-11)scanBlocksForIntegrations(22-58)
⏰ 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: Lint & Format
- GitHub Check: Build & Test
🔇 Additional comments (9)
src/notebooks/deepnote/integrations/integrationManager.ts (3)
1-10: LGTM: Imports properly organized.Third-party imports correctly separated from local imports by blank line.
21-26: LGTM: Proper dependency injection.All dependencies are injected as interfaces following the coding guidelines.
28-64: LGTM: Proper async error handling.Event handlers correctly use
.catch()to prevent unhandled promise rejections. Addressed previous review concern.src/notebooks/deepnote/integrations/integrationDetector.ts (3)
1-7: LGTM: Clean imports and structure.Third-party imports properly separated from local imports. All dependencies are interfaces.
22-59: LGTM: Robust detection implementation.Properly handles missing projects, logs scan progress, and leverages the shared
scanBlocksForIntegrationsutility. Clean separation of concerns.
64-74: LGTM: Simple and correct.Clean implementation that reuses
detectIntegrationsto check for disconnected status.src/notebooks/deepnote/integrations/integrationUtils.ts (2)
1-11: LGTM: Clean utility structure.Well-defined interface and proper imports.
22-58: LGTM: Solid shared utility.Properly deduplicates integrations, excludes internal integrations, queries storage once per integration, and builds correct status map. Good logging at appropriate levels.
src/notebooks/deepnote/integrations/integrationTypes.ts (1)
1-67: LGTM: Clean type definitions.Well-documented constant, enums, and interfaces. Proper use of discriminated union for
IntegrationConfig. Types-only file as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
162-205: Localize the success/error payloads.These literals should flow through
l10n.t()before we send them to the webview. Keeps us aligned with the localization guideline. Please wrap them accordingly.-import { Disposable, ViewColumn, WebviewPanel, window } from 'vscode'; +import { Disposable, ViewColumn, WebviewPanel, window, l10n } from 'vscode'; @@ - await this.currentPanel?.webview.postMessage({ - message: 'Configuration saved successfully', - type: 'success' - }); + await this.currentPanel?.webview.postMessage({ + message: l10n.t('Configuration saved successfully'), + type: 'success' + }); @@ - await this.currentPanel?.webview.postMessage({ - message: `Failed to save configuration: ${error instanceof Error ? error.message : 'Unknown error'}`, - type: 'error' - }); + const reason = error instanceof Error ? error.message : 'Unknown error'; + await this.currentPanel?.webview.postMessage({ + message: l10n.t('Failed to save configuration: {0}', reason), + type: 'error' + }); @@ - await this.currentPanel?.webview.postMessage({ - message: 'Configuration deleted successfully', - type: 'success' - }); + await this.currentPanel?.webview.postMessage({ + message: l10n.t('Configuration deleted successfully'), + type: 'success' + }); @@ - await this.currentPanel?.webview.postMessage({ - message: `Failed to delete configuration: ${error instanceof Error ? error.message : 'Unknown error'}`, - type: 'error' - }); + const reason = error instanceof Error ? error.message : 'Unknown error'; + await this.currentPanel?.webview.postMessage({ + message: l10n.t('Failed to delete configuration: {0}', reason), + type: 'error' + });src/notebooks/deepnote/integrations/integrationManager.ts (1)
100-130: Localize the command feedback strings.
window.showErrorMessage/InformationMessageshould not ship raw English. Please run them throughl10n.t()and importl10nfromvscode.-import { commands, NotebookDocument, window, workspace } from 'vscode'; +import { commands, NotebookDocument, window, workspace, l10n } from 'vscode'; @@ - if (!activeNotebook || activeNotebook.notebookType !== 'deepnote') { - void window.showErrorMessage('No active Deepnote notebook'); + if (!activeNotebook || activeNotebook.notebookType !== 'deepnote') { + void window.showErrorMessage(l10n.t('No active Deepnote notebook')); return; } @@ - if (!projectId) { - void window.showErrorMessage('Cannot determine project ID'); + if (!projectId) { + void window.showErrorMessage(l10n.t('Cannot determine project ID')); return; } @@ - if (integrations.size === 0) { - void window.showInformationMessage(`No integrations found in this project.`); + if (integrations.size === 0) { + void window.showInformationMessage(l10n.t('No integrations found in this project.')); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/notebooks/deepnote/integrations/integrationManager.ts(1 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(1 hunks)src/notebooks/deepnote/integrations/types.ts(1 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/notebooks/serviceRegistry.web.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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, then alphabetically
Separate third-party imports from local file imports
Files:
src/notebooks/serviceRegistry.web.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationManager.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.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/types.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationManager.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationManager.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Add a blank line after const groups and before return statements (whitespace for readability)
Files:
src/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationManager.ts
🧠 Learnings (13)
📚 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.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/notebooks/serviceRegistry.web.tssrc/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts wires VSCode activation
Applied to files:
src/notebooks/serviceRegistry.web.tssrc/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-03T13:01:10.017Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/standalone.instructions.md:0-0
Timestamp: 2025-09-03T13:01:10.017Z
Learning: Applies to src/standalone/serviceRegistry.{node,web}.ts : Register new services for both platforms in serviceRegistry.node.ts and serviceRegistry.web.ts as appropriate
Applied to files:
src/notebooks/serviceRegistry.node.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/notebooks/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/serviceRegistry.{node,web}.ts : Register dependencies via DI with platform-appropriate services for Node and Web builds
Applied to files:
src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/** : Deepnote integration code resides under src/notebooks/deepnote/
Applied to files:
src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-12T12:52:09.726Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T12:52:09.726Z
Learning: Applies to src/notebooks/deepnote/deepnoteNotebookManager.ts : deepnoteNotebookManager.ts handles state management
Applied to files:
src/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-10-07T15:24:08.015Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationManager.ts:1-3
Timestamp: 2025-10-07T15:24:08.015Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required and should not be suggested for any files.
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.ts
📚 Learning: 2025-10-07T15:24:14.028Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationTypes.ts:1-7
Timestamp: 2025-10-07T15:24:14.028Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required for new files and should not be suggested.
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.ts
📚 Learning: 2025-09-09T11:31:30.479Z
Learnt from: Artmann
PR: deepnote/vscode-extension#11
File: src/notebooks/deepnote/deepnoteActivationService.ts:1-3
Timestamp: 2025-09-09T11:31:30.479Z
Learning: New files in the Deepnote VS Code extension project should not include copyright headers.
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Inject interfaces, not concrete classes
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.ts
🧬 Code graph analysis (4)
src/notebooks/serviceRegistry.web.ts (1)
src/notebooks/deepnote/integrations/types.ts (6)
IIntegrationStorage(3-3)IIntegrationStorage(4-11)IIntegrationDetector(13-13)IIntegrationDetector(14-24)IIntegrationWebviewProvider(26-26)IIntegrationWebviewProvider(27-32)
src/notebooks/serviceRegistry.node.ts (1)
src/notebooks/deepnote/integrations/types.ts (6)
IIntegrationStorage(3-3)IIntegrationStorage(4-11)IIntegrationDetector(13-13)IIntegrationDetector(14-24)IIntegrationWebviewProvider(26-26)IIntegrationWebviewProvider(27-32)
src/notebooks/deepnote/integrations/types.ts (1)
src/notebooks/deepnote/integrations/integrationTypes.ts (2)
IntegrationConfig(49-49)IntegrationWithStatus(63-67)
src/notebooks/deepnote/integrations/integrationWebview.ts (3)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationWebviewProvider(26-26)IIntegrationWebviewProvider(27-32)src/notebooks/deepnote/integrations/integrationTypes.ts (2)
IntegrationWithStatus(63-67)IntegrationConfig(49-49)src/platform/logging/index.ts (1)
logger(35-48)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/notebooks/deepnote/integrations/integrationUtils.ts(1 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/integrationUtils.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, then alphabetically
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/integrations/integrationUtils.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationUtils.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Add a blank line after const groups and before return statements (whitespace for readability)
Files:
src/notebooks/deepnote/integrations/integrationUtils.ts
🔇 Additional comments (2)
src/notebooks/deepnote/integrations/integrationUtils.ts (2)
1-3: Imports organized correctly.Import structure follows guidelines—all local imports properly separated by concern.
22-26: Function signature and documentation look good.Clear parameter types and comprehensive JSDoc.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
163-204: Localize success/error messages.The strings sent to the webview (
Configuration saved successfully,Failed to save…, etc.) must go throughl10n.t(...)before posting.src/notebooks/deepnote/integrations/integrationUtils.ts (2)
50-51: Missing error handling for storage call.
integrationStorage.get()can throw; uncaught errors will halt the scan.Wrap in try/catch:
- // Check if the integration is configured - const config = await integrationStorage.get(integrationId); + // Check if the integration is configured + let config; + try { + config = await integrationStorage.get(integrationId); + } catch (error) { + logger.error(`${logContext}: Failed to retrieve config for integration ${integrationId}`, error); + config = null; + }
29-59: Consider parallel storage fetches.Sequential
awaitin loop degrades performance with multiple integrations.Collect IDs first, then fetch in parallel:
+const integrationIds = new Set<string>(); + for (const block of blocks) { const integrationId = block.sql_integration_id; // Skip blocks without integration IDs if (!integrationId) { continue; } // Skip excluded integrations (e.g., internal DuckDB integration) if (integrationId === DATAFRAME_SQL_INTEGRATION_ID) { logger.trace(`${logContext}: Skipping excluded integration: ${integrationId} in block ${block.id}`); continue; } - // Skip if we've already detected this integration - if (integrations.has(integrationId)) { + // Collect unique integration IDs + if (integrationIds.has(integrationId)) { continue; } + integrationIds.add(integrationId); logger.debug(`${logContext}: Found integration: ${integrationId} in block ${block.id}`); +} + +// Fetch all configs in parallel +await Promise.all( + Array.from(integrationIds).map(async (integrationId) => { + let config; + try { + config = await integrationStorage.get(integrationId); + } catch (error) { + logger.error(`${logContext}: Failed to retrieve config for integration ${integrationId}`, error); + config = null; + } - // Check if the integration is configured - const config = await integrationStorage.get(integrationId); - - const status: IntegrationWithStatus = { - config: config || null, - status: config ? IntegrationStatus.Connected : IntegrationStatus.Disconnected - }; + const status: IntegrationWithStatus = { + config: config || null, + status: config ? IntegrationStatus.Connected : IntegrationStatus.Disconnected + }; - integrations.set(integrationId, status); -} + integrations.set(integrationId, status); + }) +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/notebooks/deepnote/integrations/integrationUtils.ts(1 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(1 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/notebooks/deepnote/integrations/integrationUtils.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, then alphabetically
Separate third-party imports from local file imports
Files:
src/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationUtils.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationUtils.ts
src/notebooks/deepnote/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Add a blank line after const groups and before return statements (whitespace for readability)
Files:
src/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationUtils.ts
🧬 Code graph analysis (1)
src/notebooks/deepnote/integrations/integrationUtils.ts (2)
src/notebooks/deepnote/integrations/integrationTypes.ts (2)
IntegrationWithStatus(63-67)DATAFRAME_SQL_INTEGRATION_ID(5-5)src/platform/logging/index.ts (1)
logger(35-48)
🔇 Additional comments (5)
src/notebooks/deepnote/integrations/integrationUtils.ts (5)
1-3: Imports properly organized.All local imports grouped together as expected.
8-11: Interface definition is clear.Type declarations are appropriate.
13-26: Function signature and documentation look good.Iterable param provides needed flexibility.
32-35: Validation check properly added.Addresses previous feedback by skipping blocks without integration IDs.
61-63: Blank line before return properly added.Coding guideline now satisfied.
|
@Artmann This is fully vibe coded. I did address many useful pieces of feedback from coderabbit and it should be fine now. I tested that it works as intended.
|
|
The interface sucks since there are only UUIDs shown. It will make more sense when we allow opening this from the individual blocks later. We should be able to determine the integration type from the deepnote project file as well - also a follow up. I'm creating issues for this. Let's get the base in first. |
|
I see that we use the |
jankuca
requested review from
andyjakubowski,
jamesbhobbs and
saltenasl
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
=====================================
Coverage 70% 70%
=====================================
Files 515 516 +1
Lines 38159 38178 +19
Branches 4854 4854
=====================================
+ Hits 27053 27068 +15
- Misses 9501 9505 +4
Partials 1605 1605
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (3)
src/notebooks/deepnote/integrations/integrationWebview.ts (3)
196-196: Localize error message.Hard-coded string should use
l10n.t()for internationalization.As per coding guidelines
Apply this diff:
- await this.currentPanel?.webview.postMessage({ - message: `Failed to delete configuration: ${error instanceof Error ? error.message : 'Unknown error'}`, - type: 'error' - }); + await this.currentPanel?.webview.postMessage({ + message: l10n.t('Failed to delete configuration: {0}', error instanceof Error ? error.message : l10n.t('Unknown error')), + type: 'error' + });
167-167: Localize error message.Hard-coded string should use
l10n.t()for internationalization.As per coding guidelines
Apply this diff:
- await this.currentPanel?.webview.postMessage({ - message: `Failed to save configuration: ${error instanceof Error ? error.message : 'Unknown error'}`, - type: 'error' - }); + await this.currentPanel?.webview.postMessage({ + message: l10n.t('Failed to save configuration: {0}', error instanceof Error ? error.message : l10n.t('Unknown error')), + type: 'error' + });
161-161: Localize success message.Hard-coded string should use
l10n.t()for internationalization.As per coding guidelines
Apply this diff:
- await this.currentPanel?.webview.postMessage({ - message: 'Configuration saved successfully', - type: 'success' - }); + await this.currentPanel?.webview.postMessage({ + message: l10n.t('Configuration saved successfully'), + type: 'success' + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
build/esbuild/build.ts(1 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(1 hunks)src/webviews/webview-side/integrations/BigQueryForm.tsx(1 hunks)src/webviews/webview-side/integrations/ConfigurationForm.tsx(1 hunks)src/webviews/webview-side/integrations/IntegrationItem.tsx(1 hunks)src/webviews/webview-side/integrations/IntegrationList.tsx(1 hunks)src/webviews/webview-side/integrations/IntegrationPanel.tsx(1 hunks)src/webviews/webview-side/integrations/PostgresForm.tsx(1 hunks)src/webviews/webview-side/integrations/index.html(1 hunks)src/webviews/webview-side/integrations/index.tsx(1 hunks)src/webviews/webview-side/integrations/integrations.css(1 hunks)src/webviews/webview-side/integrations/types.ts(1 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:
build/esbuild/build.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/integrations/types.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:
build/esbuild/build.tssrc/webviews/webview-side/integrations/PostgresForm.tsxsrc/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/webviews/webview-side/integrations/ConfigurationForm.tsxsrc/webviews/webview-side/integrations/IntegrationList.tsxsrc/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/integrations/BigQueryForm.tsxsrc/webviews/webview-side/integrations/types.tssrc/webviews/webview-side/integrations/index.tsxsrc/webviews/webview-side/integrations/IntegrationItem.tsx
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationWebview.ts
🧠 Learnings (3)
📚 Learning: 2025-10-07T16:30:58.450Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationWebview.ts:1-1
Timestamp: 2025-10-07T16:30:58.450Z
Learning: In the deepnote/vscode-deepnote repository, do not suggest adding Microsoft copyright headers or any copyright headers to files. The maintainers have explicitly declined this practice.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
📚 Learning: 2025-10-07T15:24:14.050Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationTypes.ts:1-7
Timestamp: 2025-10-07T15:24:14.050Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required for new files and should not be suggested.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
📚 Learning: 2025-10-07T15:24:08.041Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationManager.ts:1-3
Timestamp: 2025-10-07T15:24:08.041Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required and should not be suggested for any files.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
🧬 Code graph analysis (8)
src/webviews/webview-side/integrations/PostgresForm.tsx (1)
src/webviews/webview-side/integrations/types.ts (1)
PostgresIntegrationConfig(14-22)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (5)
src/webviews/webview-side/react-common/postOffice.ts (1)
IVsCodeApi(9-16)src/webviews/webview-side/integrations/types.ts (3)
IntegrationWithStatus(32-36)IntegrationConfig(30-30)WebviewMessage(60-60)src/notebooks/deepnote/integrations/integrationWebview.ts (1)
handleMessage(104-126)src/webviews/webview-side/integrations/IntegrationList.tsx (1)
IntegrationList(14-31)src/webviews/webview-side/integrations/ConfigurationForm.tsx (1)
ConfigurationForm(16-74)
src/webviews/webview-side/integrations/ConfigurationForm.tsx (3)
src/webviews/webview-side/integrations/types.ts (1)
IntegrationConfig(30-30)src/webviews/webview-side/integrations/PostgresForm.tsx (1)
PostgresForm(14-156)src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
BigQueryForm(14-93)
src/webviews/webview-side/integrations/IntegrationList.tsx (2)
src/webviews/webview-side/integrations/types.ts (1)
IntegrationWithStatus(32-36)src/webviews/webview-side/integrations/IntegrationItem.tsx (1)
IntegrationItem(13-35)
src/notebooks/deepnote/integrations/integrationWebview.ts (4)
src/notebooks/deepnote/integrations/integrationManager.ts (1)
injectable(14-160)src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationWebviewProvider(26-26)IIntegrationWebviewProvider(27-32)src/webviews/webview-side/integrations/types.ts (3)
IntegrationWithStatus(32-36)IntegrationConfig(30-30)IntegrationStatus(6-6)src/platform/logging/index.ts (1)
logger(35-48)
src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
src/webviews/webview-side/integrations/types.ts (1)
BigQueryIntegrationConfig(24-28)
src/webviews/webview-side/integrations/index.tsx (2)
src/webviews/webview-side/react-common/postOffice.ts (1)
IVsCodeApi(9-16)src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
IntegrationPanel(15-140)
src/webviews/webview-side/integrations/IntegrationItem.tsx (1)
src/webviews/webview-side/integrations/types.ts (1)
IntegrationWithStatus(32-36)
🪛 Biome (2.1.2)
src/webviews/webview-side/integrations/ConfigurationForm.tsx
[error] 45-45: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
src/webviews/webview-side/integrations/IntegrationItem.tsx
[error] 26-26: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
[error] 28-28: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🔇 Additional comments (1)
build/esbuild/build.ts (1)
304-309: ESM bundle requirestype="module"(or change build format).Web target defaults to
format: 'esm'for this entry. Ensure the HTML script tag usestype="module", or set this build toformat: 'iife'/'cjs'for compatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
build/esbuild/build.ts(1 hunks)package.json(2 hunks)src/notebooks/deepnote/deepnoteSerializer.ts(6 hunks)src/notebooks/deepnote/deepnoteTypes.ts(0 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/notebooks/types.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/notebooks/deepnote/deepnoteTypes.ts
🧰 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:
build/esbuild/build.tssrc/notebooks/types.tssrc/notebooks/deepnote/deepnoteSerializer.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:
build/esbuild/build.tssrc/notebooks/types.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteSerializer.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.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
src/notebooks/deepnote/deepnoteSerializer.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteSerializer.ts is the main serializer orchestrating Deepnote integration
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
🧠 Learnings (10)
📓 Common learnings
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/
📚 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/types.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteSerializer.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/deepnoteTypes.ts : deepnoteTypes.ts contains Deepnote-related type definitions
Applied to files:
src/notebooks/types.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/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-03T13:01:10.017Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/standalone.instructions.md:0-0
Timestamp: 2025-09-03T13:01:10.017Z
Learning: Applies to src/standalone/serviceRegistry.{node,web}.ts : Register new services for both platforms in serviceRegistry.node.ts and serviceRegistry.web.ts as appropriate
Applied to files:
src/notebooks/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/notebooks/serviceRegistry.node.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/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: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/deepnoteSerializer.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.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/deepnoteSerializer.ts : deepnoteSerializer.ts is the main serializer orchestrating Deepnote integration
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.ts
🧬 Code graph analysis (3)
src/notebooks/types.ts (1)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
DeepnoteProject(10-10)
src/notebooks/serviceRegistry.node.ts (1)
src/notebooks/deepnote/integrations/types.ts (6)
IIntegrationStorage(3-3)IIntegrationStorage(4-11)IIntegrationDetector(13-13)IIntegrationDetector(14-24)IIntegrationWebviewProvider(26-26)IIntegrationWebviewProvider(27-32)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
🔇 Additional comments (7)
build/esbuild/build.ts (1)
346-351: LGTM!The new build step follows the established pattern for webview compilation and integrates correctly into the build sequence.
package.json (1)
88-93: LGTM!Command registration and toolbar integration are correctly structured. The when clause appropriately scopes the toolbar item to Deepnote notebooks only.
Also applies to: 716-720
src/notebooks/serviceRegistry.node.ts (1)
46-50: LGTM!Integration services properly wired into DI container. Registrations follow existing patterns and placement groups related Deepnote services logically.
Also applies to: 136-139
src/notebooks/deepnote/deepnoteSerializer.ts (2)
5-5: LGTM: Logger migration complete.Console calls correctly replaced with platform logger. Error details preserved.
As per coding guidelines.
Also applies to: 39-39, 68-68, 86-86, 153-153
56-56: Verify ID logging aligns with telemetry policy.Project and notebook IDs are logged at debug level. Confirm these identifiers aren't considered sensitive if debug logs are collected as telemetry.
As per coding guidelines on PII scrubbing.
Also applies to: 71-71
src/notebooks/types.ts (2)
7-7: Import path is correct.DeepnoteProjectis defined and exported indeepnoteTypes.ts.
31-31: Approve type strengtheningImplementations of IDeepnoteNotebookManager and all callsites of getOriginalProject/storeOriginalProject now align with DeepnoteProject; no further changes needed.
jankuca
force-pushed
the
jk/feat/integration-management
branch
from
eedac9c to
fcd612c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/notebooks/serviceRegistry.node.ts (1)
139-139: IntegrationManager should be registered via an interface.This registration violates the "inject interfaces, not concrete classes" guideline. Create an
IIntegrationManagerinterface and update the registration to use it, aligning with the pattern used for the other integration services.As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
build/esbuild/build.ts(1 hunks)package.json(2 hunks)src/notebooks/deepnote/deepnoteSerializer.ts(6 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/notebooks/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/notebooks/deepnote/deepnoteSerializer.tsbuild/esbuild/build.tssrc/notebooks/types.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/deepnoteSerializer.tsbuild/esbuild/build.tssrc/notebooks/types.tssrc/notebooks/serviceRegistry.node.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteSerializer.ts
src/notebooks/deepnote/deepnoteSerializer.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteSerializer.ts is the main serializer orchestrating Deepnote integration
Files:
src/notebooks/deepnote/deepnoteSerializer.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.ts
🧠 Learnings (8)
📚 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/deepnoteSerializer.ts : deepnoteSerializer.ts is the main serializer orchestrating Deepnote integration
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.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/deepnote/deepnoteSerializer.tssrc/notebooks/types.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: Applies to src/notebooks/deepnote/deepnoteActivationService.ts : deepnoteActivationService.ts manages VS Code activation for Deepnote
Applied to files:
src/notebooks/deepnote/deepnoteSerializer.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: Applies to src/notebooks/deepnote/deepnoteTypes.ts : deepnoteTypes.ts contains Deepnote-related type definitions
Applied to files:
src/notebooks/types.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/notebooks/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/notebooks/serviceRegistry.node.ts
📚 Learning: 2025-09-03T13:01:10.017Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/standalone.instructions.md:0-0
Timestamp: 2025-09-03T13:01:10.017Z
Learning: Applies to src/standalone/serviceRegistry.{node,web}.ts : Register new services for both platforms in serviceRegistry.node.ts and serviceRegistry.web.ts as appropriate
Applied to files:
src/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: Applies to src/notebooks/deepnote/** : Deepnote integration code resides under src/notebooks/deepnote/
Applied to files:
src/notebooks/serviceRegistry.node.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/deepnoteSerializer.ts (1)
src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/serviceRegistry.node.ts (1)
src/notebooks/deepnote/integrations/types.ts (6)
IIntegrationStorage(3-3)IIntegrationStorage(4-11)IIntegrationDetector(13-13)IIntegrationDetector(14-24)IIntegrationWebviewProvider(26-26)IIntegrationWebviewProvider(27-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: copilot-setup-steps
🔇 Additional comments (10)
build/esbuild/build.ts (1)
347-351: Approve integration webview build configuration
Entry point verified; config mirrors existing webview patterns.src/notebooks/types.ts (2)
31-31: LGTM! Type tightening improves type safety.Replacing
unknownwithDeepnoteProjectprovides proper type checking and aligns with the broader integration management work.Also applies to: 34-34
4-7: Reorder imports and use a type‐only import for DeepnoteProject. Move
import type { DeepnoteProject } from './deepnote/deepnoteTypes';
below the local imports block. EnsuredeepnoteTypes.tsdoesn’t import from this file to avoid a circular dependency.package.json (1)
716-720: LGTM!The toolbar item is correctly scoped to Deepnote notebooks and properly positioned.
src/notebooks/serviceRegistry.node.ts (2)
46-50: LGTM!Imports are properly organized and grouped with other local file imports.
136-138: LGTM!These registrations correctly follow the interface injection pattern.
src/notebooks/deepnote/deepnoteSerializer.ts (4)
5-5: LGTM on logger import.Correctly imports from the platform logging service as per coding guidelines.
39-39: LGTM on log level choices.Debug level appropriate for trace messages, error level for exceptions. Consistent throughout.
Also applies to: 68-68
56-56: No redaction needed for project/notebook IDs. These UUIDs are public in URLs and don’t grant access; retaining them in debug logs is acceptable.Likely an incorrect or invalid review comment.
86-86: Confirm PII scrubbing for logged errors. Raw error objects at lines 86 and 153 may contain PII (file paths, usernames). Verify ILogger.error sanitizes sensitive data before logging.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/webviews/webview-side/integrations/BigQueryForm.tsx (2)
1-2: Remove copyright header.Per repository practice, do not include copyright headers.
Based on learnings
29-41: Consider validating credentials JSON.Parsing credentials before save would provide immediate feedback on malformed JSON.
const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); + + // Validate credentials JSON + try { + JSON.parse(credentials); + } catch { + // Could set error state and show inline message + return; + } const config: BigQueryIntegrationConfig = {src/notebooks/deepnote/integrations/integrationManager.ts (4)
129-129: Localize user-facing info message.Hard-coded string should use
l10n.t()per guidelines.As per coding guidelines
Apply this diff:
- void window.showInformationMessage(`No integrations found in this project.`); + void window.showInformationMessage(l10n.t('No integrations found in this project.'));
103-103: Localize user-facing error message.Hard-coded string should use
l10n.t()per guidelines.As per coding guidelines
Apply this diff:
- void window.showErrorMessage('No active Deepnote notebook'); + void window.showErrorMessage(l10n.t('No active Deepnote notebook'));
109-109: Localize user-facing error message.Hard-coded string should use
l10n.t()per guidelines.As per coding guidelines
Apply this diff:
- void window.showErrorMessage('Cannot determine project ID'); + void window.showErrorMessage(l10n.t('Cannot determine project ID'));
16-25: Reorder class members per guidelines.Private fields appear before the constructor. Per coding guidelines, order should be: constructor, public methods, then private members alphabetically.
As per coding guidelines
Apply this diff:
@injectable() export class IntegrationManager implements IIntegrationManager { - private hasIntegrationsContext = 'deepnote.hasIntegrations'; - - private hasUnconfiguredIntegrationsContext = 'deepnote.hasUnconfiguredIntegrations'; - constructor( @inject(IExtensionContext) private readonly extensionContext: IExtensionContext, @inject(IIntegrationDetector) private readonly integrationDetector: IIntegrationDetector, @inject(IIntegrationStorage) private readonly integrationStorage: IIntegrationStorage, @inject(IIntegrationWebviewProvider) private readonly webviewProvider: IIntegrationWebviewProvider ) {} public activate(): void {Then after the
activate()method and before the first private method, add:} + + private hasIntegrationsContext = 'deepnote.hasIntegrations'; + + private hasUnconfiguredIntegrationsContext = 'deepnote.hasUnconfiguredIntegrations'; /** * Update the context keys based on the active notebook
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
package.json(2 hunks)package.nls.json(1 hunks)src/notebooks/deepnote/deepnoteActivationService.ts(3 hunks)src/notebooks/deepnote/deepnoteActivationService.unit.test.ts(3 hunks)src/notebooks/deepnote/integrations/integrationManager.ts(1 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(1 hunks)src/notebooks/deepnote/integrations/types.ts(1 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/notebooks/serviceRegistry.web.ts(2 hunks)src/webviews/webview-side/integrations/BigQueryForm.tsx(1 hunks)src/webviews/webview-side/integrations/ConfigurationForm.tsx(1 hunks)src/webviews/webview-side/integrations/IntegrationItem.tsx(1 hunks)src/webviews/webview-side/integrations/IntegrationList.tsx(1 hunks)src/webviews/webview-side/integrations/IntegrationPanel.tsx(1 hunks)src/webviews/webview-side/integrations/PostgresForm.tsx(1 hunks)src/webviews/webview-side/integrations/index.html(1 hunks)src/webviews/webview-side/integrations/index.tsx(1 hunks)src/webviews/webview-side/integrations/integrations.css(1 hunks)src/webviews/webview-side/integrations/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.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/webviews/webview-side/integrations/IntegrationItem.tsxsrc/notebooks/deepnote/integrations/types.tssrc/notebooks/serviceRegistry.node.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/webviews/webview-side/integrations/PostgresForm.tsxsrc/webviews/webview-side/integrations/BigQueryForm.tsxsrc/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/webviews/webview-side/integrations/types.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/webviews/webview-side/integrations/IntegrationList.tsxsrc/webviews/webview-side/integrations/index.tsxsrc/notebooks/deepnote/integrations/integrationManager.tssrc/webviews/webview-side/integrations/ConfigurationForm.tsx
**/!(*.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/types.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/webviews/webview-side/integrations/types.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/integrations/integrationManager.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/integrations/integrationManager.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.ts
src/notebooks/deepnote/deepnoteActivationService.ts
📄 CodeRabbit inference engine (CLAUDE.md)
deepnoteActivationService.ts manages VS Code activation for Deepnote
Files:
src/notebooks/deepnote/deepnoteActivationService.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/deepnoteActivationService.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/deepnoteActivationService.unit.test.ts
🧠 Learnings (18)
📚 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.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/notebooks/serviceRegistry.web.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/notebooks/serviceRegistry.web.tssrc/notebooks/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/serviceRegistry.{node,web}.ts : Register dependencies via DI with platform-appropriate services for Node and Web builds
Applied to files:
src/notebooks/serviceRegistry.web.ts
📚 Learning: 2025-09-03T13:01:10.017Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/standalone.instructions.md:0-0
Timestamp: 2025-09-03T13:01:10.017Z
Learning: Applies to src/standalone/serviceRegistry.{node,web}.ts : Register new services for both platforms in serviceRegistry.node.ts and serviceRegistry.web.ts as appropriate
Applied to files:
src/notebooks/serviceRegistry.web.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/deepnoteActivationService.tssrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/integrations/integrationManager.ts
📚 Learning: 2025-09-09T11:31:30.479Z
Learnt from: Artmann
PR: deepnote/vscode-extension#11
File: src/notebooks/deepnote/deepnoteActivationService.ts:1-3
Timestamp: 2025-09-09T11:31:30.479Z
Learning: New files in the Deepnote VS Code extension project should not include copyright headers.
Applied to files:
src/webviews/webview-side/integrations/integrations.csssrc/webviews/webview-side/integrations/types.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/webviews/webview-side/integrations/ConfigurationForm.tsx
📚 Learning: 2025-10-07T15:24:14.050Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationTypes.ts:1-7
Timestamp: 2025-10-07T15:24:14.050Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required for new files and should not be suggested.
Applied to files:
src/webviews/webview-side/integrations/integrations.csssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/integrations/types.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/webviews/webview-side/integrations/ConfigurationForm.tsx
📚 Learning: 2025-10-07T15:24:08.041Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationManager.ts:1-3
Timestamp: 2025-10-07T15:24:08.041Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required and should not be suggested for any files.
Applied to files:
src/webviews/webview-side/integrations/integrations.csssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/integrations/types.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/webviews/webview-side/integrations/ConfigurationForm.tsx
📚 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.node.tssrc/notebooks/deepnote/deepnoteActivationService.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.node.ts
📚 Learning: 2025-10-07T16:30:58.450Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationWebview.ts:1-1
Timestamp: 2025-10-07T16:30:58.450Z
Learning: In the deepnote/vscode-deepnote repository, do not suggest adding Microsoft copyright headers or any copyright headers to files. The maintainers have explicitly declined this practice.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/integrations/types.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/webviews/webview-side/integrations/ConfigurationForm.tsx
📚 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/deepnoteSerializer.ts : deepnoteSerializer.ts is the main serializer orchestrating Deepnote integration
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/webviews/webview-side/integrations/IntegrationList.tsxsrc/notebooks/deepnote/integrations/integrationManager.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 **/*.{ts,tsx} : Do not add the Microsoft copyright header to new files
Applied to files:
src/webviews/webview-side/integrations/types.tssrc/webviews/webview-side/integrations/ConfigurationForm.tsx
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous with signature `public activate()` and returns void. It should be called without await in tests.
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous and returns void, not a Promise. It should be called without await in tests.
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Inject interfaces, not concrete classes
Applied to files:
src/notebooks/deepnote/integrations/integrationManager.ts
🧬 Code graph analysis (11)
src/notebooks/serviceRegistry.web.ts (1)
src/notebooks/deepnote/integrations/types.ts (8)
IIntegrationStorage(3-3)IIntegrationStorage(4-11)IIntegrationDetector(13-13)IIntegrationDetector(14-24)IIntegrationWebviewProvider(26-26)IIntegrationWebviewProvider(27-32)IIntegrationManager(34-34)IIntegrationManager(35-40)
src/webviews/webview-side/integrations/IntegrationItem.tsx (1)
src/webviews/webview-side/integrations/types.ts (1)
IntegrationWithStatus(29-33)
src/notebooks/serviceRegistry.node.ts (1)
src/notebooks/deepnote/integrations/types.ts (8)
IIntegrationStorage(3-3)IIntegrationStorage(4-11)IIntegrationDetector(13-13)IIntegrationDetector(14-24)IIntegrationWebviewProvider(26-26)IIntegrationWebviewProvider(27-32)IIntegrationManager(34-34)IIntegrationManager(35-40)
src/notebooks/deepnote/deepnoteActivationService.ts (2)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationManager(34-34)IIntegrationManager(35-40)src/notebooks/types.ts (2)
IDeepnoteNotebookManager(28-28)IDeepnoteNotebookManager(29-38)
src/webviews/webview-side/integrations/PostgresForm.tsx (1)
src/webviews/webview-side/integrations/types.ts (1)
PostgresIntegrationConfig(11-19)
src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
src/webviews/webview-side/integrations/types.ts (1)
BigQueryIntegrationConfig(21-25)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (5)
src/webviews/webview-side/react-common/postOffice.ts (1)
IVsCodeApi(9-16)src/webviews/webview-side/integrations/types.ts (3)
IntegrationWithStatus(29-33)IntegrationConfig(27-27)WebviewMessage(57-57)src/notebooks/deepnote/integrations/integrationWebview.ts (1)
handleMessage(104-126)src/webviews/webview-side/integrations/IntegrationList.tsx (1)
IntegrationList(12-29)src/webviews/webview-side/integrations/ConfigurationForm.tsx (1)
ConfigurationForm(13-67)
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts (1)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationManager(34-34)IIntegrationManager(35-40)
src/webviews/webview-side/integrations/IntegrationList.tsx (2)
src/webviews/webview-side/integrations/types.ts (1)
IntegrationWithStatus(29-33)src/webviews/webview-side/integrations/IntegrationItem.tsx (1)
IntegrationItem(11-35)
src/webviews/webview-side/integrations/index.tsx (2)
src/webviews/webview-side/react-common/postOffice.ts (1)
IVsCodeApi(9-16)src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
IntegrationPanel(13-173)
src/webviews/webview-side/integrations/ConfigurationForm.tsx (3)
src/webviews/webview-side/integrations/types.ts (1)
IntegrationConfig(27-27)src/webviews/webview-side/integrations/PostgresForm.tsx (1)
PostgresForm(12-161)src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
BigQueryForm(15-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (13)
package.json (2)
88-93: LGTM! Command registration follows conventions.Past review concern about localization has been addressed.
716-720: Toolbar item properly scoped to Deepnote notebooks.Condition and group placement are appropriate.
package.nls.json (1)
252-252: LGTM!Localization entry matches command registration.
src/webviews/webview-side/integrations/index.tsx (1)
1-19: LGTM! Clean webview bootstrap.Past review suggestions already applied (import order, no eslint disables, proper
declare function).src/webviews/webview-side/integrations/index.html (1)
1-23: LGTM! Secure webview scaffold.Past review suggestions addressed (CSP placeholders, module type).
src/notebooks/deepnote/deepnoteActivationService.ts (2)
8-8: LGTM! Proper DI injection.Integration manager correctly injected and stored.
Also applies to: 18-18, 25-28
41-41: LGTM! Activation sequencing is appropriate.Integration manager activated after explorer view setup.
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts (1)
83-86: Constructor calls correctly updated.New integration manager parameter properly passed in all test instances.
Also applies to: 111-114
src/notebooks/serviceRegistry.web.ts (2)
41-50: LGTM! Clean imports.Integration service imports properly organized.
105-108: LGTM! DI registrations follow existing patterns.All four integration services registered as singletons.
Based on learnings
src/notebooks/deepnote/integrations/types.ts (1)
1-40: LGTM! Clear interface definitions.All integration service interfaces properly defined with DI symbols.
src/notebooks/serviceRegistry.node.ts (1)
141-144: LGTM!Integration services properly registered with interface injection per guidelines.
src/webviews/webview-side/integrations/types.ts (1)
1-57: LGTM!Type definitions are clear, well-structured, and properly discriminated.
as per in person dicussion on the offsite, will figure this out in the follow up PRs |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts (2)
83-86: Fix ESLint failures blocking CI.Same issue as lines 20-23: empty mock functions violate the linting rule.
Apply the same fix pattern:
- const mockIntegrationManager1: IIntegrationManager = { activate: () => {} }; - const mockIntegrationManager2: IIntegrationManager = { activate: () => {} }; + const mockIntegrationManager1: IIntegrationManager = { activate: () => { /* no-op mock */ } }; + const mockIntegrationManager2: IIntegrationManager = { activate: () => { /* no-op mock */ } };
111-114: Fix ESLint failures blocking CI.Same empty function issue in this test block.
Apply the same fix:
- const mockIntegrationManager1: IIntegrationManager = { activate: () => {} }; - const mockIntegrationManager2: IIntegrationManager = { activate: () => {} }; + const mockIntegrationManager1: IIntegrationManager = { activate: () => { /* no-op mock */ } }; + const mockIntegrationManager2: IIntegrationManager = { activate: () => { /* no-op mock */ } };src/notebooks/deepnote/integrations/integrationWebview.ts (1)
28-30: Copy the integrations Map to avoid shared mutable state.Line 30 stores a direct reference to the caller's Map. If the caller mutates that Map elsewhere, this provider sees those mutations, creating subtle state consistency bugs. Copy it instead.
Apply this diff:
public async show(integrations: Map<string, IntegrationWithStatus>): Promise<void> { // Update the stored integrations with the latest data - this.integrations = integrations; + this.integrations = new Map(integrations);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts(3 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(1 hunks)src/webviews/webview-side/integrations/BigQueryForm.tsx(1 hunks)src/webviews/webview-side/integrations/integrations.css(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/deepnoteActivationService.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/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/integrations/BigQueryForm.tsxsrc/notebooks/deepnote/deepnoteActivationService.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.tssrc/notebooks/deepnote/deepnoteActivationService.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/notebooks/deepnote/deepnoteActivationService.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/deepnoteActivationService.unit.test.ts
🧠 Learnings (7)
📚 Learning: 2025-10-07T16:30:58.450Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationWebview.ts:1-1
Timestamp: 2025-10-07T16:30:58.450Z
Learning: In the deepnote/vscode-deepnote repository, do not suggest adding Microsoft copyright headers or any copyright headers to files. The maintainers have explicitly declined this practice.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
📚 Learning: 2025-10-07T15:24:14.050Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationTypes.ts:1-7
Timestamp: 2025-10-07T15:24:14.050Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required for new files and should not be suggested.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
📚 Learning: 2025-10-07T15:24:08.041Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationManager.ts:1-3
Timestamp: 2025-10-07T15:24:08.041Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required and should not be suggested for any files.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.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/deepnoteActivationService.unit.test.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous with signature `public activate()` and returns void. It should be called without await in tests.
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous and returns void, not a Promise. It should be called without await in tests.
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
📚 Learning: 2025-09-12T07:57:49.414Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:48-66
Timestamp: 2025-09-12T07:57:49.414Z
Learning: The activate() method in DeepnoteActivationService returns void (synchronous), not Promise<void>, so tests should not await it or be marked as async.
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
🧬 Code graph analysis (3)
src/notebooks/deepnote/integrations/integrationWebview.ts (3)
src/notebooks/deepnote/integrations/types.ts (4)
IIntegrationWebviewProvider(26-26)IIntegrationWebviewProvider(27-32)IIntegrationStorage(3-3)IIntegrationStorage(4-11)src/webviews/webview-side/integrations/types.ts (3)
IntegrationWithStatus(29-33)IntegrationConfig(27-27)IntegrationStatus(3-3)src/platform/logging/index.ts (1)
logger(35-48)
src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
src/webviews/webview-side/integrations/types.ts (1)
BigQueryIntegrationConfig(21-25)
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts (1)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationManager(34-34)IIntegrationManager(35-40)
🪛 GitHub Actions: CI
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
[error] 21-21: eslint --ext .ts,.js src: ESLint error: Unexpected empty function 'activate'. (@typescript-eslint/no-empty-function)
🪛 GitHub Check: Lint & Format
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
[failure] 21-21:
Unexpected empty method 'activate'
[failure] 84-84:
Unexpected empty method 'activate'
[failure] 83-83:
Unexpected empty method 'activate'
[failure] 112-112:
Unexpected empty method 'activate'
[failure] 111-111:
Unexpected empty method 'activate'
🔇 Additional comments (6)
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts (3)
6-6: LGTM!Import correctly added for the new dependency.
12-12: LGTM!Variable declaration follows test conventions.
23-23: Test structure correctly updated.Constructor calls properly pass the new third parameter, and tests maintain independence with separate mock instances.
Also applies to: 85-86, 113-114
src/webviews/webview-side/integrations/BigQueryForm.tsx (2)
73-136: Form implementation looks solid.Good use of:
l10n.t()for all user-facing strings- Security attributes on credentials textarea (autoComplete, spellCheck, etc.)
- Accessibility attributes (aria-invalid, aria-describedby, role="alert")
- Error feedback with inline validation
1-2: Remove copyright header.Per repository guidelines, new files should not include the Microsoft copyright header.
Based on coding guidelines
-// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - import * as React from 'react';Likely an incorrect or invalid review comment.
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
155-155: IntegrationStatus enum usage is correct. IntegrationStatus is defined as an enum mapping to string literals—usingIntegrationStatus.Connectedis appropriate.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts (1)
20-24: ESLint error blocks CI.The
activatemock bodies trigger@typescript-eslint/no-empty-function. Add inline comments to indicate intentional no-ops.Apply this diff to all three mock definitions:
mockIntegrationManager = { - activate: () => { - return; - } + activate: () => { /* no-op mock */ } };Or:
mockIntegrationManager = { - activate: () => { - return; - } + activate: () => { return; /* intentional no-op */ } };Also applies to: 85-94, 121-130
src/notebooks/deepnote/integrations/integrationWebview.ts (1)
28-31: Create a defensive copy of the integrations Map.Direct assignment at line 30 means the provider shares a reference with the caller. If the caller mutates the Map after calling
show(), the provider's state changes unexpectedly, breaking encapsulation.Apply this diff:
- // Update the stored integrations with the latest data - this.integrations = integrations; + // Store a defensive copy to prevent external mutations + this.integrations = new Map(integrations);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts(3 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(1 hunks)src/webviews/webview-side/integrations/BigQueryForm.tsx(1 hunks)src/webviews/webview-side/integrations/PostgresForm.tsx(1 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/BigQueryForm.tsxsrc/webviews/webview-side/integrations/PostgresForm.tsxsrc/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/integrations/integrationWebview.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/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/integrations/integrationWebview.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/deepnoteActivationService.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/deepnoteActivationService.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.tssrc/notebooks/deepnote/integrations/integrationWebview.ts
🧠 Learnings (8)
📚 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/deepnoteActivationService.unit.test.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous with signature `public activate()` and returns void. It should be called without await in tests.
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
📚 Learning: 2025-09-12T07:58:04.493Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:31-45
Timestamp: 2025-09-12T07:58:04.493Z
Learning: In src/notebooks/deepnote/deepnoteActivationService.ts, the activate() method is synchronous and returns void, not a Promise. It should be called without await in tests.
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
📚 Learning: 2025-09-12T07:57:49.414Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#12
File: src/notebooks/deepnote/deepnoteActivationService.unit.test.ts:48-66
Timestamp: 2025-09-12T07:57:49.414Z
Learning: The activate() method in DeepnoteActivationService returns void (synchronous), not Promise<void>, so tests should not await it or be marked as async.
Applied to files:
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts
📚 Learning: 2025-10-07T16:30:58.450Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationWebview.ts:1-1
Timestamp: 2025-10-07T16:30:58.450Z
Learning: In the deepnote/vscode-deepnote repository, do not suggest adding Microsoft copyright headers or any copyright headers to files. The maintainers have explicitly declined this practice.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
📚 Learning: 2025-10-07T15:24:14.050Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationTypes.ts:1-7
Timestamp: 2025-10-07T15:24:14.050Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required for new files and should not be suggested.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
📚 Learning: 2025-10-07T15:24:08.041Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationManager.ts:1-3
Timestamp: 2025-10-07T15:24:08.041Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required and should not be suggested for any files.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Applies to **/*.{ts,tsx} : Use `l10n.t()` for user-facing strings
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
🧬 Code graph analysis (4)
src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
src/webviews/webview-side/integrations/types.ts (1)
BigQueryIntegrationConfig(21-25)
src/webviews/webview-side/integrations/PostgresForm.tsx (1)
src/webviews/webview-side/integrations/types.ts (1)
PostgresIntegrationConfig(11-19)
src/notebooks/deepnote/deepnoteActivationService.unit.test.ts (1)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationManager(34-34)IIntegrationManager(35-40)
src/notebooks/deepnote/integrations/integrationWebview.ts (4)
src/notebooks/deepnote/integrations/integrationManager.ts (1)
injectable(14-160)src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationWebviewProvider(26-26)IIntegrationWebviewProvider(27-32)src/webviews/webview-side/integrations/types.ts (3)
IntegrationWithStatus(29-33)IntegrationConfig(27-27)IntegrationStatus(3-3)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: Build & Test
- GitHub Check: Lint & Format
🔇 Additional comments (11)
src/webviews/webview-side/integrations/PostgresForm.tsx (1)
1-161: Previous concerns addressed; implementation solid.The form correctly:
- Uses HTML5 validation to prevent invalid port submission before
parseInt- Applies appropriate autocomplete attributes per field type
- Wraps checkbox in label for accessibility
- Localizes all user-facing strings
- Maintains type safety with the
PostgresIntegrationConfiginterfacesrc/webviews/webview-side/integrations/BigQueryForm.tsx (1)
8-136: LGTM – past feedback addressed.The implementation is clean:
- Validation now properly handles empty credentials with error messages
- Textarea hardened with autoComplete/spellCheck/autoCorrect/autoCapitalize disabled
- Accessibility attributes (aria-invalid, aria-describedby, role="alert") properly implemented
- Name fallback uses translated string instead of raw UUID
- All user-facing strings localized via l10n.t()
src/notebooks/deepnote/integrations/integrationWebview.ts (9)
1-23: Imports and setup look good.DI wiring, logging, and import organization follow guidelines. No copyright header per project preference.
32-79: Panel creation and lifecycle management are correct.Message handling and disposal are properly wired. State cleanup on dispose is appropriate.
84-99: LGTM.Webview update logic is straightforward and correct.
104-126: Message handling is functional.The loosely typed message parameter was previously noted as a minor improvement opportunity (typed discriminated union would provide better type safety), but the current implementation with runtime checks works correctly.
131-142: LGTM.Configuration form display logic is correct.
147-174: Save logic is correct.Error handling, logging, and localized messages with placeholders follow guidelines and address previous feedback.
179-206: Delete logic is correct.Mirrors save logic with proper error handling and localization.
211-267: Webview HTML generation is secure.CSP is properly configured (no unsafe-inline, nonce-based scripts). React bundle loading and Uri.joinPath() usage follow best practices.
269-276: LGTM.Nonce generation is standard and secure.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/webviews/webview-side/integrations/BigQueryForm.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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.tsx
🧠 Learnings (5)
📓 Common learnings
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/
📚 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 **/*.{ts,tsx} : Do not add the Microsoft copyright header to new files
Applied to files:
src/webviews/webview-side/integrations/BigQueryForm.tsx
📚 Learning: 2025-10-07T15:24:14.050Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationTypes.ts:1-7
Timestamp: 2025-10-07T15:24:14.050Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required for new files and should not be suggested.
Applied to files:
src/webviews/webview-side/integrations/BigQueryForm.tsx
📚 Learning: 2025-10-07T15:24:08.041Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#24
File: src/notebooks/deepnote/integrations/integrationManager.ts:1-3
Timestamp: 2025-10-07T15:24:08.041Z
Learning: In the deepnote/vscode-deepnote repository, Microsoft copyright headers are not required and should not be suggested for any files.
Applied to files:
src/webviews/webview-side/integrations/BigQueryForm.tsx
📚 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 **/*.{ts,tsx} : Separate third-party imports from local file imports
Applied to files:
src/webviews/webview-side/integrations/BigQueryForm.tsx
🧬 Code graph analysis (1)
src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
src/webviews/webview-side/integrations/types.ts (1)
BigQueryIntegrationConfig(21-25)
⏰ 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
jankuca
force-pushed
the
jk/feat/integration-management
branch
from
0f71538 to
9b5b7aa
Compare
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Resolves OSS-55
Summary by CodeRabbit
New Features
Tests