feat: Buttons to open SQL block integration config + fix localization #73
Conversation
📝 WalkthroughWalkthroughCommand ManageIntegrations now accepts variadic args and extracts an optional integrationId passed to showIntegrationsUI(selectedIntegrationId?). showIntegrationsUI and IntegrationWebview.show accept an optional selectedIntegrationId and will fetch and insert a fallback integration entry from storage if the selected ID is not found among detected integrations. IntegrationStorage implements IIntegrationStorage/IDisposable, exposes onDidChangeIntegrations, adds getIntegrationConfig and clear(), fires change events after save/delete/clear, and supports disposal. Webview now sends loc_init localization payloads; many webview React components use the new locReactSide. A new SqlCellStatusBarProvider displays per-cell integration status and registers as an activation service. Sequence Diagram(s)sequenceDiagram
participant User
participant Cell as NotebookCell
participant StatusBar as SqlCellStatusBarProvider
participant Mgr as IntegrationManager
participant Storage as IntegrationStorage
participant Webview as IntegrationWebview
participant UI as Webview UI
User->>Cell: focus/click SQL cell
Cell->>StatusBar: read cell.metadata.sql_integration_id
alt valid integrationId & not internal
StatusBar->>Storage: getIntegrationConfig(projectId, integrationId)
Storage-->>StatusBar: IntegrationConfig?
StatusBar->>User: render status item (ManageIntegrations, args:[integrationId])
else no projectId or invalid
StatusBar-->>User: no status item
end
User->>StatusBar: invoke ManageIntegrations (integrationId)
StatusBar->>Mgr: ManageIntegrations handler (args...)
Mgr->>Storage: detect/list integrations
Storage-->>Mgr: integrations map
alt selectedIntegrationId not in map
Mgr->>Storage: getIntegrationConfig(projectId, selectedIntegrationId)
Storage-->>Mgr: config or undefined
Mgr->>Mgr: insert fallback entry (Connected/Disconnected)
end
Mgr->>Webview: show(integrationsMap, selectedIntegrationId)
Webview->>UI: post loc_init (if new) and update (includes selectedIntegrationId)
UI->>Storage: save/delete integration actions
Storage->>Storage: persist, fire onDidChangeIntegrations
Storage-->>StatusBar: onDidChangeIntegrations event
StatusBar->>StatusBar: refresh items
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (5 passed)
Comment |
jankuca
force-pushed
the
jk/feat/sql-block-open-credentials
branch
from
cb75865 to
a179762
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
======================================
Coverage 71% 71%
======================================
Files 520 523 +3
Lines 38865 39001 +136
Branches 4895 4905 +10
======================================
+ Hits 27705 27834 +129
- Misses 9537 9542 +5
- Partials 1623 1625 +2
🚀 New features to boost your workflow:
|
jankuca
force-pushed
the
jk/feat/sql-block-open-credentials
branch
from
a179762 to
6bf4689
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/notebooks/deepnote/integrations/integrationStorage.ts (1)
110-124: Fire change event after clear().clear() updates storage/cache but doesn’t emit onDidChangeIntegrations. UIs depending on the event won’t refresh.
Apply this diff:
async clear(): Promise<void> { await this.ensureCacheLoaded(); // Delete all integrations from encrypted storage const integrationIds = Array.from(this.cache.keys()); for (const id of integrationIds) { await this.encryptedStorage.store(INTEGRATION_SERVICE_NAME, id, undefined); } // Clear the index await this.encryptedStorage.store(INTEGRATION_SERVICE_NAME, 'index', undefined); // Clear cache this.cache.clear(); + + // Notify listeners + this._onDidChangeIntegrations.fire(); }src/notebooks/deepnote/integrations/integrationWebview.ts (1)
49-59: Invalid Webview option and missing localization.
- enableForms is not a valid option; this likely fails type-check or is ignored.
- Localize the panel title.
Apply this diff:
- this.currentPanel = window.createWebviewPanel( + this.currentPanel = window.createWebviewPanel( 'deepnoteIntegrations', - 'Deepnote Integrations', + l10n.t('Deepnote Integrations'), column || ViewColumn.One, { enableScripts: true, retainContextWhenHidden: true, localResourceRoots: [this.extensionContext.extensionUri], - enableForms: true } );As per coding guidelines.
src/notebooks/deepnote/integrations/integrationManager.ts (1)
123-131: Localize user messages.Use l10n.t for user-facing strings.
Apply this diff:
- void window.showErrorMessage('No active Deepnote notebook'); + void window.showErrorMessage(l10n.t('No active Deepnote notebook')); return;- void window.showErrorMessage('Cannot determine project ID'); + void window.showErrorMessage(l10n.t('Cannot determine project ID')); return;Add import if missing:
import { l10n } from 'vscode';As per coding guidelines.
♻️ Duplicate comments (4)
src/webviews/webview-side/integrations/PostgresForm.tsx (1)
38-38: Same localization issue as IntegrationItem.tsx.Hard-coded English strings violate the coding guideline requiring
l10n.t()for user-facing text. See comment on IntegrationItem.tsx for suggested alternatives.Based on coding guidelines.
Also applies to: 54-155
src/webviews/webview-side/integrations/IntegrationList.tsx (1)
13-13: Same localization issue.Hard-coded English string. See IntegrationItem.tsx comment for suggested alternatives.
Based on coding guidelines.
src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
30-30: Same localization issue.Hard-coded English strings. See IntegrationItem.tsx comment for suggested alternatives.
Based on coding guidelines.
Also applies to: 40-40, 61-61, 73-129
src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
135-135: Same localization issue.Hard-coded English strings. See IntegrationItem.tsx comment for suggested alternatives.
Based on coding guidelines.
Also applies to: 154-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/notebooks/deepnote/integrations/integrationManager.ts(4 hunks)src/notebooks/deepnote/integrations/integrationStorage.ts(5 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(4 hunks)src/notebooks/deepnote/integrations/types.ts(2 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.ts(1 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(1 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/notebooks/serviceRegistry.web.ts(2 hunks)src/webviews/webview-side/integrations/BigQueryForm.tsx(7 hunks)src/webviews/webview-side/integrations/IntegrationItem.tsx(2 hunks)src/webviews/webview-side/integrations/IntegrationList.tsx(1 hunks)src/webviews/webview-side/integrations/IntegrationPanel.tsx(4 hunks)src/webviews/webview-side/integrations/PostgresForm.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/PostgresForm.tsxsrc/webviews/webview-side/integrations/IntegrationList.tsxsrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/integrations/BigQueryForm.tsxsrc/notebooks/deepnote/integrations/types.tssrc/notebooks/serviceRegistry.node.tssrc/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/webviews/webview-side/integrations/IntegrationItem.tsxsrc/notebooks/serviceRegistry.web.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationStorage.ts
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationStorage.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/sqlCellStatusBarProvider.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/integrations/integrationStorage.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
**/*.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
🧬 Code graph analysis (6)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (5)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationStorage(4-4)IIntegrationStorage(5-23)src/test/mocks/vsc/index.ts (2)
CancellationToken(100-112)CancellationTokenSource(114-125)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)src/test/datascience/mockDocument.ts (1)
languageId(102-104)src/test/datascience/mockTextEditor.ts (1)
document(67-69)
src/notebooks/deepnote/integrations/integrationWebview.ts (3)
src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationWithStatus(63-67)src/webviews/webview-side/integrations/types.ts (1)
IntegrationWithStatus(29-33)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/integrations/types.ts (2)
src/notebooks/deepnote/integrations/integrationTypes.ts (2)
IntegrationConfig(49-49)IntegrationWithStatus(63-67)src/webviews/webview-side/integrations/types.ts (2)
IntegrationConfig(27-27)IntegrationWithStatus(29-33)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (2)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationStorage(4-4)IIntegrationStorage(5-23)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)
src/notebooks/deepnote/integrations/integrationManager.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/webviews/webview-side/integrations/types.ts (1)
IntegrationStatus(3-3)
src/notebooks/deepnote/integrations/integrationStorage.ts (3)
src/test/mocks/vsc/index.ts (1)
EventEmitter(65-89)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationConfig(49-49)src/webviews/webview-side/integrations/types.ts (1)
IntegrationConfig(27-27)
⏰ 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: Audit - Production
🔇 Additional comments (5)
src/notebooks/deepnote/integrations/types.ts (1)
1-46: LGTM! Clean API additions.The new
onDidChangeIntegrationsevent,getIntegrationConfigmethod, and optionalselectedIntegrationIdparameter follow VS Code API conventions. Documentation is clear and the changes are backwards compatible.src/notebooks/serviceRegistry.web.ts (1)
51-51: LGTM! Service registration is correct.SqlCellStatusBarProvider properly registered as an activation service in the web registry.
Also applies to: 110-113
src/notebooks/serviceRegistry.node.ts (1)
56-56: LGTM! Service registration is correct.SqlCellStatusBarProvider properly registered as an activation service in the node registry, consistent with the web registry.
Also applies to: 146-149
src/webviews/webview-side/integrations/IntegrationItem.tsx (1)
12-13: Verify webview localization approach.
Webview React code can’t importl10n.t()directly. Please choose and implement one of:
- Pass localized strings from the extension host via postMessage
- Integrate a browser-friendly i18n library in the webview
- Document this limitation and track it with a TODO/issue
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (1)
71-80: IntegrationType enum usage is correct. IntegrationType.Postgres is a string enum value ('postgres') and compiles as expected.
jankuca
force-pushed
the
jk/feat/sql-block-open-credentials
branch
2 times, most recently
from
77a90f1 to
0819fb0
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (2)
19-20: Use cross‑env timer type.NodeJS.Timeout is wrong in the browser/webview. Use ReturnType.
- const messageTimerRef = React.useRef<NodeJS.Timeout | null>(null); - const confirmDeleteTimerRef = React.useRef<NodeJS.Timeout | null>(null); + const messageTimerRef = React.useRef<ReturnType<typeof setTimeout> | null>(null); + const confirmDeleteTimerRef = React.useRef<ReturnType<typeof setTimeout> | null>(null);
80-87: Dead timer ref; either remove or implement auto‑dismiss.You clear confirmDeleteTimerRef but never set it. Implement timeout or drop the ref.
Example auto‑dismiss:
const handleDelete = (integrationId: string) => { // Clear any existing confirmDelete timer before creating a new one if (confirmDeleteTimerRef.current) { clearTimeout(confirmDeleteTimerRef.current); } setConfirmDelete(integrationId); + confirmDeleteTimerRef.current = setTimeout(() => { + setConfirmDelete(null); + confirmDeleteTimerRef.current = null; + }, 10000); };src/notebooks/deepnote/integrations/integrationStorage.ts (1)
110-124: Fire change event on clear().Listeners won’t refresh after a bulk clear.
async clear(): Promise<void> { await this.ensureCacheLoaded(); @@ // Clear cache this.cache.clear(); + // Notify listeners + this._onDidChangeIntegrations.fire(); }src/notebooks/deepnote/integrations/integrationWebview.ts (1)
53-58: Invalid webview option.enableForms isn’t a valid WebviewOptions flag; remove it.
- { - enableScripts: true, - retainContextWhenHidden: true, - localResourceRoots: [this.extensionContext.extensionUri], - enableForms: true - } + { + enableScripts: true, + retainContextWhenHidden: true, + localResourceRoots: [this.extensionContext.extensionUri] + }src/webviews/webview-side/integrations/PostgresForm.tsx (1)
54-156: Re-add l10n for all user-facing strings.Hard-coded English strings violate the coding guidelines.
As per coding guidelines.
Apply this diff to restore localization:
+import { l10n } from 'vscode'; + export const PostgresForm: React.FC<IPostgresFormProps> = ({ integrationId, existingConfig, onSave, onCancel }) => { // ... state declarations ... const config: PostgresIntegrationConfig = { id: integrationId, - name: name || `Unnamed PostgreSQL Integration (${integrationId})`, + name: name || l10n.t('Unnamed PostgreSQL Integration ({0})', integrationId), type: 'postgres', // ... }; return ( <form onSubmit={handleSubmit}> <div className="form-group"> - <label htmlFor="name">Name (optional)</label> + <label htmlFor="name">{l10n.t('Name (optional)')}</label> <input type="text" id="name" value={name} onChange={(e) => setName(e.target.value)} - placeholder="My PostgreSQL Database" + placeholder={l10n.t('My PostgreSQL Database')} autoComplete="off" /> </div> <div className="form-group"> <label htmlFor="host"> - Host <span className="required">*</span> + {l10n.t('Host')} <span className="required">*</span> </label> {/* ... */} </div> {/* Apply l10n.t() to all remaining labels, placeholders, and button text */}
♻️ Duplicate comments (2)
src/webviews/webview-side/integrations/IntegrationItem.tsx (1)
28-28: Same localization issue.Reset button uses hard-coded English (see comment on lines 12-13).
src/notebooks/deepnote/integrations/integrationManager.ts (1)
148-157: Deferred: Use getIntegrationConfig for project-aware lookup.Already noted in past review and deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
src/notebooks/deepnote/integrations/integrationManager.ts(5 hunks)src/notebooks/deepnote/integrations/integrationStorage.ts(5 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(4 hunks)src/notebooks/deepnote/integrations/types.ts(2 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.ts(1 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(1 hunks)src/notebooks/serviceRegistry.node.ts(2 hunks)src/notebooks/serviceRegistry.web.ts(2 hunks)src/webviews/webview-side/integrations/BigQueryForm.tsx(7 hunks)src/webviews/webview-side/integrations/IntegrationItem.tsx(2 hunks)src/webviews/webview-side/integrations/IntegrationList.tsx(1 hunks)src/webviews/webview-side/integrations/IntegrationPanel.tsx(2 hunks)src/webviews/webview-side/integrations/PostgresForm.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/BigQueryForm.tsxsrc/notebooks/serviceRegistry.node.tssrc/webviews/webview-side/integrations/IntegrationList.tsxsrc/webviews/webview-side/integrations/PostgresForm.tsxsrc/notebooks/deepnote/integrations/integrationStorage.tssrc/webviews/webview-side/integrations/IntegrationItem.tsxsrc/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.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/integrationStorage.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationStorage.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/notebooks/deepnote/integrations/integrationManager.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.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/sqlCellStatusBarProvider.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
🧬 Code graph analysis (4)
src/notebooks/deepnote/integrations/integrationWebview.ts (3)
src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationWithStatus(63-67)src/webviews/webview-side/integrations/types.ts (1)
IntegrationWithStatus(29-33)src/platform/logging/index.ts (1)
logger(35-48)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (4)
src/test/mocks/vsc/index.ts (2)
CancellationToken(100-112)CancellationTokenSource(114-125)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)src/test/datascience/mockDocument.ts (1)
languageId(102-104)
src/notebooks/deepnote/integrations/types.ts (3)
types/slickgrid/index.d.ts (1)
Event(86-126)src/notebooks/deepnote/integrations/integrationTypes.ts (2)
IntegrationConfig(49-49)IntegrationWithStatus(63-67)src/webviews/webview-side/integrations/types.ts (2)
IntegrationConfig(27-27)IntegrationWithStatus(29-33)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
src/notebooks/deepnote/integrations/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)
🔇 Additional comments (7)
src/notebooks/serviceRegistry.web.ts (1)
110-113: LGTM: provider registration looks correct.SqlCellStatusBarProvider is bound as an activation service alongside others.
Please confirm the provider registers a NotebookCellStatusBarItemProvider with VS Code within its activate() and is disposed via the injected disposables.
src/notebooks/serviceRegistry.node.ts (1)
146-149: LGTM: native registry wiring is consistent with web.Registration mirrors web path; no issues spotted.
Confirm the provider self‑registers with vscode.notebooks.registerNotebookCellStatusBarItemProvider and cleans up on dispose.
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
101-108: Past review comment addressed correctly.The implementation now uses
NotebookCellStatusBarItemwith proper alignment and l10n.src/notebooks/deepnote/integrations/types.ts (2)
6-22: LGTM.Interface extensions support the new integration pre-selection flow.
42-45: LGTM.Optional parameter maintains backward compatibility.
src/notebooks/deepnote/integrations/integrationManager.ts (2)
33-49: LGTM.Robust handling of VS Code's context injection with variadic args.
177-190: LGTM.Type-safe metadata extraction with appropriate logging.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (3)
74-74: Use string literal for IntegrationType.
IntegrationTypeis a string union type, not an enum. Use'postgres'directly.Apply this diff:
- type: IntegrationType.Postgres, + type: 'postgres',
86-86: Replace magic number with enum.Use
NotebookCellStatusBarAlignment.Leftinstead of1.Apply this diff:
- assert.strictEqual((result as any).alignment, 1); // NotebookCellStatusBarAlignment.Left + assert.strictEqual((result as any).alignment, NotebookCellStatusBarAlignment.Left);Add import:
import { CancellationToken, CancellationTokenSource, NotebookCell, + NotebookCellStatusBarAlignment, NotebookCellKind,
89-89: Chai assert lacks deepStrictEqual.Use
assert.deepEqualor switch to Node's strict assert.Apply this diff:
- assert.deepStrictEqual((result as any).command.arguments, [integrationId]); + assert.deepEqual((result as any).command.arguments, [integrationId]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts(1 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.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/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.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/sqlCellStatusBarProvider.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.ts
🧠 Learnings (3)
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/**/*.ts : Add unit and integration tests for new features and modifications; include performance and cross-platform test coverage
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-10-07T15:31:30.150Z
Learnt from: hc2p
PR: deepnote/deepnote-internal#18615
File: apps/webapp-client/src/api/integration-api.ts:204-204
Timestamp: 2025-10-07T15:31:30.150Z
Learning: In apps/webapp-client/src/api/integration-api.ts, when reviewing bug fixes authored by the PR creator (e.g., changing integration.state to integration.type), use informational tone rather than marking as critical issues, since the fix is already implemented.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.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 **/*.unit.test.ts : Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
🧬 Code graph analysis (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (3)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationStorage(4-4)IIntegrationStorage(5-23)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
src/notebooks/deepnote/integrations/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)
🪛 GitHub Actions: CI
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
[error] 1739-1739: SqlCellStatusBarProvider: NotebookCellStatusBarItem is not a constructor. TypeError occurred at createStatusBarItem.
jankuca
force-pushed
the
jk/feat/sql-block-open-credentials
branch
from
0819fb0 to
626028f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/webviews/webview-side/integrations/BigQueryForm.tsx (2)
18-27: Reset fields when existingConfig becomes null.Effect only updates when existingConfig is truthy; switching from a populated config to null leaves stale values. Add an else branch to clear fields.
React.useEffect(() => { - if (existingConfig) { + if (existingConfig) { setName(existingConfig.name || ''); setProjectId(existingConfig.projectId || ''); setCredentials(existingConfig.credentials || ''); setCredentialsError(null); - } + } else { + setName(''); + setProjectId(''); + setCredentials(''); + setCredentialsError(null); + } }, [existingConfig]);Based on learnings.
55-76: Trim inputs before save.Prevent accidental whitespace in ids; no behavior change.
- const config: BigQueryIntegrationConfig = { + const config: BigQueryIntegrationConfig = { id: integrationId, - name: name || unnamedIntegration, + name: (name || unnamedIntegration).trim(), type: 'bigquery', - projectId, - credentials + projectId: projectId.trim(), + credentials };Based on learnings.
src/webviews/webview-side/integrations/PostgresForm.tsx (2)
34-55: Validate and clamp port; trim string fields.parseInt can yield NaN despite input attributes. Add runtime validation and normalize fields before constructing config.
const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); - const unnamedIntegration = getLocString( + const unnamedIntegration = getLocString( 'integrationsPostgresUnnamedIntegration', 'Unnamed PostgreSQL Integration ({0})' ).replace('{0}', integrationId); - const config: PostgresIntegrationConfig = { - id: integrationId, - name: name || unnamedIntegration, - type: 'postgres', - host, - port: parseInt(port, 10), - database, - username, - password, - ssl - }; + const portNum = parseInt(port, 10); + if (!Number.isInteger(portNum) || portNum < 1 || portNum > 65535) { + // Rely on simple alert UX; form can later show inline error if desired. + return; + } + const config: PostgresIntegrationConfig = { + id: integrationId, + name: (name || unnamedIntegration).trim(), + type: 'postgres', + host: host.trim(), + port: portNum, + database: database.trim(), + username: username.trim(), + password, + ssl + }; onSave(config); };
21-33: Also clear fields when existingConfig becomes null.Mirror the BigQuery form to avoid stale state.
React.useEffect(() => { - if (existingConfig) { + if (existingConfig) { setName(existingConfig.name || ''); setHost(existingConfig.host || ''); setPort(existingConfig.port?.toString() || '5432'); setDatabase(existingConfig.database || ''); setUsername(existingConfig.username || ''); setPassword(existingConfig.password || ''); setSsl(existingConfig.ssl || false); - } + } else { + setName(''); + setHost(''); + setPort('5432'); + setDatabase(''); + setUsername(''); + setPassword(''); + setSsl(false); + } }, [existingConfig]);src/notebooks/deepnote/integrations/integrationStorage.ts (2)
117-131: Fire change event on clear().Consumers rely on onDidChangeIntegrations; clearing should notify too.
async clear(): Promise<void> { await this.ensureCacheLoaded(); @@ // Clear cache this.cache.clear(); + // Notify listeners + this._onDidChangeIntegrations.fire(); }
16-18: Declare interface implementation for compile-time safety.Have the class implement IIntegrationStorage (it already satisfies it).
-@injectable() -export class IntegrationStorage { +@injectable() +export class IntegrationStorage implements IIntegrationStorage {Add import if missing:
+import { IIntegrationStorage } from './types';
♻️ Duplicate comments (2)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (1)
89-90: Use a supported deep equality assert.Chai’s assert doesn’t expose deepStrictEqual. Either switch to Node’s strict assert or use Chai’s deepEqual.
Option A (Node assert strict):
-import { assert } from 'chai'; +import { strict as assert } from 'assert'; ... - assert.deepStrictEqual((result as any).command.arguments, [integrationId]); + assert.deepStrictEqual((result as any).command.arguments, [integrationId]);Option B (stay on Chai):
- assert.deepStrictEqual((result as any).command.arguments, [integrationId]); + assert.deepEqual((result as any).command.arguments, [integrationId]);src/notebooks/deepnote/sqlCellStatusBarProvider.ts (1)
100-109: Use the enum instead of magic number for alignment.Prefer NotebookCellStatusBarAlignment.Left for clarity/typing.
Apply:
@@ - return { - text: `$(database) ${displayName}`, - alignment: 1, // NotebookCellStatusBarAlignment.Left + return { + text: `$(database) ${displayName}`, + alignment: NotebookCellStatusBarAlignment.Left, tooltip: l10n.t('SQL Integration: {0}\nClick to configure', displayName), command: { title: l10n.t('Configure Integration'), command: Commands.ManageIntegrations, arguments: [integrationId] } };Add import:
import { CancellationToken, EventEmitter, NotebookCell, + NotebookCellStatusBarAlignment, NotebookCellStatusBarItem,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
src/messageTypes.ts(1 hunks)src/notebooks/deepnote/integrations/integrationManager.ts(5 hunks)src/notebooks/deepnote/integrations/integrationStorage.ts(6 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(5 hunks)src/notebooks/deepnote/integrations/types.ts(2 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.ts(1 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts(1 hunks)src/platform/common/utils/localize.ts(1 hunks)src/platform/webviews/webviewHost.ts(1 hunks)src/webviews/webview-side/integrations/BigQueryForm.tsx(6 hunks)src/webviews/webview-side/integrations/ConfigurationForm.tsx(2 hunks)src/webviews/webview-side/integrations/IntegrationItem.tsx(3 hunks)src/webviews/webview-side/integrations/IntegrationList.tsx(2 hunks)src/webviews/webview-side/integrations/IntegrationPanel.tsx(4 hunks)src/webviews/webview-side/integrations/PostgresForm.tsx(5 hunks)src/webviews/webview-side/integrations/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/!(*.node|*.web).ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place shared cross-platform logic in common
.tsfiles (not suffixed with.nodeor.web)
Files:
src/notebooks/deepnote/integrations/integrationManager.tssrc/platform/webviews/webviewHost.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/integrations/types.tssrc/messageTypes.tssrc/notebooks/deepnote/integrations/integrationStorage.tssrc/webviews/webview-side/integrations/types.tssrc/platform/common/utils/localize.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.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 (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/integrationManager.tssrc/platform/webviews/webviewHost.tssrc/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/webviews/webview-side/integrations/PostgresForm.tsxsrc/webviews/webview-side/integrations/BigQueryForm.tsxsrc/notebooks/deepnote/integrations/types.tssrc/webviews/webview-side/integrations/IntegrationItem.tsxsrc/messageTypes.tssrc/notebooks/deepnote/integrations/integrationStorage.tssrc/webviews/webview-side/integrations/types.tssrc/platform/common/utils/localize.tssrc/webviews/webview-side/integrations/IntegrationList.tsxsrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/integrations/ConfigurationForm.tsx
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/sqlCellStatusBarProvider.unit.test.tssrc/notebooks/deepnote/integrations/types.tssrc/notebooks/deepnote/integrations/integrationStorage.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/notebooks/deepnote/integrations/integrationWebview.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging
Files:
src/platform/webviews/webviewHost.tssrc/platform/common/utils/localize.ts
**/*.unit.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place unit tests in files matching
*.unit.test.ts
**/*.unit.test.ts: Unit tests must use Mocha/Chai and have the .unit.test.ts extension
Place unit test files alongside the source files they test
Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
**/*.{test,spec}.ts
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
In unit tests, when a mock is returned from a promise, ensure the mocked instance has an undefined
thenproperty to avoid hanging tests
Files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
🧠 Learnings (8)
📚 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/BigQueryForm.tsxsrc/webviews/webview-side/integrations/IntegrationList.tsx
📚 Learning: 2025-10-17T11:36:40.042Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#73
File: src/webviews/webview-side/integrations/BigQueryForm.tsx:0-0
Timestamp: 2025-10-17T11:36:40.042Z
Learning: The vscode API (including l10n) is not available in webview React components located in src/webviews/webview-side/. User-facing strings in these components cannot use l10n.t() for localization.
Applied to files:
src/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/webviews/webview-side/integrations/BigQueryForm.tsxsrc/webviews/webview-side/integrations/IntegrationItem.tsxsrc/webviews/webview-side/integrations/IntegrationList.tsx
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/**/*.ts : Add unit and integration tests for new features and modifications; include performance and cross-platform test coverage
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-10-07T15:31:30.150Z
Learnt from: hc2p
PR: deepnote/deepnote-internal#18615
File: apps/webapp-client/src/api/integration-api.ts:204-204
Timestamp: 2025-10-07T15:31:30.150Z
Learning: In apps/webapp-client/src/api/integration-api.ts, when reviewing bug fixes authored by the PR creator (e.g., changing integration.state to integration.type), use informational tone rather than marking as critical issues, since the fix is already implemented.
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.tssrc/webviews/webview-side/integrations/PostgresForm.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 **/*.unit.test.ts : Use assert.deepStrictEqual() for object comparisons in tests instead of checking individual properties
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/**/*.ts : Use EventEmitter for event-driven updates and ensure proper disposal/cleanup of listeners to prevent leaks
Applied to files:
src/notebooks/deepnote/integrations/integrationStorage.ts
📚 Learning: 2025-10-09T11:21:57.494Z
Learnt from: CR
PR: deepnote/vscode-deepnote#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-09T11:21:57.494Z
Learning: Use dependency injection with inversify
Applied to files:
src/notebooks/deepnote/integrations/integrationStorage.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/chat/extension.node.ts : Register chat tools in chat/extension.node.ts via vscode.lm.registerTool(...) and add the disposables to context.subscriptions
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
🧬 Code graph analysis (12)
src/notebooks/deepnote/integrations/integrationManager.ts (2)
src/platform/logging/index.ts (1)
logger(35-48)src/webviews/webview-side/integrations/types.ts (1)
IntegrationStatus(3-3)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
src/webviews/webview-side/react-common/locReactSide.ts (2)
storeLocStrings(19-21)getLocString(11-17)
src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts (4)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationStorage(5-5)IIntegrationStorage(6-24)src/test/mocks/vsc/index.ts (2)
CancellationToken(100-112)CancellationTokenSource(114-125)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)src/webviews/webview-side/integrations/types.ts (1)
IntegrationType(1-1)
src/webviews/webview-side/integrations/PostgresForm.tsx (2)
src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)src/webviews/webview-side/integrations/types.ts (1)
PostgresIntegrationConfig(11-19)
src/webviews/webview-side/integrations/BigQueryForm.tsx (3)
src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)src/webviews/webview-side/integrations/types.ts (1)
BigQueryIntegrationConfig(21-25)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
BigQueryIntegrationConfig(40-44)
src/notebooks/deepnote/integrations/types.ts (4)
src/platform/common/types.ts (1)
IDisposable(211-213)types/slickgrid/index.d.ts (1)
Event(86-126)src/webviews/webview-side/integrations/types.ts (2)
IntegrationConfig(27-27)IntegrationWithStatus(29-33)src/notebooks/deepnote/integrations/integrationTypes.ts (2)
IntegrationConfig(49-49)IntegrationWithStatus(63-67)
src/webviews/webview-side/integrations/IntegrationItem.tsx (1)
src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
src/notebooks/deepnote/integrations/integrationStorage.ts (3)
src/test/mocks/vsc/index.ts (1)
EventEmitter(65-89)src/webviews/webview-side/integrations/types.ts (1)
IntegrationConfig(27-27)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationConfig(49-49)
src/webviews/webview-side/integrations/IntegrationList.tsx (1)
src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (3)
src/notebooks/deepnote/integrations/integrationStorage.ts (1)
injectable(16-185)src/notebooks/deepnote/integrations/integrationManager.ts (1)
injectable(14-198)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)
src/notebooks/deepnote/integrations/integrationWebview.ts (3)
src/webviews/webview-side/integrations/types.ts (1)
IntegrationWithStatus(29-33)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationWithStatus(63-67)src/messageTypes.ts (1)
LocalizedMessages(123-210)
src/webviews/webview-side/integrations/ConfigurationForm.tsx (1)
src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (13)
src/platform/webviews/webviewHost.ts (1)
268-308: LGTM—integration keys properly scaffolded.Empty strings are appropriate placeholders since this webview host doesn't render integration UI. The integration webview uses its own localization flow via
LocInitmessages.src/webviews/webview-side/integrations/IntegrationItem.tsx (1)
13-19: Localization properly implemented.Correctly uses
getLocString()with appropriate fallback strings. Past review concerns resolved.src/platform/common/utils/localize.ts (1)
817-860: LGTM—comprehensive integration localization.Namespace properly structured with general UI strings and form-specific keys. Functional keys correctly use
l10n.t()with parameters.src/notebooks/deepnote/integrations/integrationManager.ts (2)
33-49: Command arg extraction reasonable.Variadic pattern handles VS Code's context object injection. Loop to find first string argument is straightforward.
177-197: Cell scanning logic is sound.Safe metadata extraction with proper type checks. Correctly builds integration list for utility function.
src/webviews/webview-side/integrations/IntegrationList.tsx (1)
14-18: Localization correctly applied.src/webviews/webview-side/integrations/ConfigurationForm.tsx (1)
38-41: Title localization correct.Placeholder replacement properly implemented with
replace('{0}', integrationId).src/webviews/webview-side/integrations/types.ts (1)
57-62: Message type properly extended.
LocInitMessagecorrectly added to union for localization initialization flow.src/webviews/webview-side/integrations/IntegrationPanel.tsx (2)
43-48: loc_init handler functional.Type guard on line 44 is redundant (switch already matched), but harmless.
140-180: All UI text properly localized.Comprehensive migration to
getLocString()throughout the panel. Past review concerns addressed.src/notebooks/deepnote/integrations/integrationWebview.ts (1)
165-171: Good: sanitized logging.Only logging counts now, avoiding secret leakage.
src/notebooks/deepnote/integrations/types.ts (1)
6-24: Interface extensions look good.Events and clear()/getIntegrationConfig() additions align with usage.
src/messageTypes.ts (1)
168-210: All integration keys properly used and populated.*Verification confirms:
- All 38 keys defined in messageTypes.ts are used in webviews (caught earlier misses:
integrationsConfirmResetMessage,integrationsConfirmResetDetails,integrationsOptionalFieldused via multilinegetLocStringcalls)- All keys initialized in webviewHost.ts (lines 269–307) as placeholder empty strings
- All keys populated in integrationWebview.ts (lines 112–119+) from localize.Integrations namespace
- LocInit mechanism properly wired: messageTypes.ts → handleLocInit → redux → webviews
No orphan or missing keys. No host/webview mismatch.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/platform/webviews/webviewHost.ts (2)
232-238: Wrong localization keys mapped.
- filterRowsTooltip points to sliceMismatchedAxesError.
- clearFilters points to refreshDataViewer.
Fix to the correct keys.
- filterRowsTooltip: localize.WebViews.sliceMismatchedAxesError, + filterRowsTooltip: localize.WebViews.filterRowsTooltip, @@ - clearFilters: localize.WebViews.refreshDataViewer, + clearFilters: localize.WebViews.clearFilters,
186-188: LocInit sent twice.sendLocStrings runs in loadWebview and again on Started. Consider sending once or de-duping to reduce traffic.
Also applies to: 328-331
src/webviews/webview-side/integrations/BigQueryForm.tsx (1)
18-27: Reset fields when existingConfig becomes null.Current effect ignores null and leaves stale values.
React.useEffect(() => { - if (existingConfig) { + if (existingConfig) { setName(existingConfig.name || ''); setProjectId(existingConfig.projectId || ''); setCredentials(existingConfig.credentials || ''); setCredentialsError(null); - } + } else { + setName(''); + setProjectId(''); + setCredentials(''); + setCredentialsError(null); + } }, [existingConfig]);src/webviews/webview-side/integrations/PostgresForm.tsx (1)
21-33: Reset fields when existingConfig becomes null.Avoid stale form state.
React.useEffect(() => { - if (existingConfig) { + if (existingConfig) { setName(existingConfig.name || ''); setHost(existingConfig.host || ''); setPort(existingConfig.port?.toString() || '5432'); setDatabase(existingConfig.database || ''); setUsername(existingConfig.username || ''); setPassword(existingConfig.password || ''); setSsl(existingConfig.ssl || false); - } + } else { + setName(''); + setHost(''); + setPort('5432'); + setDatabase(''); + setUsername(''); + setPassword(''); + setSsl(false); + } }, [existingConfig]);src/notebooks/deepnote/integrations/integrationWebview.ts (2)
55-61: Invalid Webview options: remove enableForms.WebviewOptions/WebviewPanelOptions don’t include enableForms.
{ enableScripts: true, retainContextWhenHidden: true, localResourceRoots: [this.extensionContext.extensionUri], - enableForms: true }
51-54: Localize the panel/window title.Use l10n.t for user-facing strings.
- this.currentPanel = window.createWebviewPanel( + this.currentPanel = window.createWebviewPanel( 'deepnoteIntegrations', - 'Deepnote Integrations', + l10n.t('Deepnote Integrations'), column || ViewColumn.One, {- <title>Deepnote Integrations</title> + <title>${l10n.t('Deepnote Integrations')}</title>As per coding guidelines.
Also applies to: 321-330
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/messageTypes.ts(1 hunks)src/notebooks/deepnote/integrations/integrationWebview.ts(5 hunks)src/notebooks/deepnote/sqlCellStatusBarProvider.ts(1 hunks)src/platform/webviews/webviewHost.ts(1 hunks)src/webviews/webview-side/integrations/BigQueryForm.tsx(6 hunks)src/webviews/webview-side/integrations/PostgresForm.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Inject interfaces, not concrete classes
Avoid circular dependencies
Usel10n.t()for user-facing strings
Use typed error classes fromsrc/platform/errors/when throwing or handling errors
Use theILoggerservice instead ofconsole.log
Preserve error details while scrubbing PII in messages and telemetry
Include the Microsoft copyright header in source files
Prefer async/await and handle cancellation withCancellationToken
**/*.{ts,tsx}: Order class members (methods, fields, properties) first by accessibility (public/protected/private) and then alphabetically
Do not add the Microsoft copyright header to new files
Use Uri.joinPath() to construct file paths instead of manual string concatenation
Add a blank line after groups of const declarations and before return statements for readability
Separate third-party imports from local file imports
Files:
src/webviews/webview-side/integrations/BigQueryForm.tsxsrc/platform/webviews/webviewHost.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/messageTypes.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.tssrc/webviews/webview-side/integrations/PostgresForm.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/platform/webviews/webviewHost.tssrc/notebooks/deepnote/integrations/integrationWebview.tssrc/messageTypes.tssrc/notebooks/deepnote/sqlCellStatusBarProvider.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/webviews/webviewHost.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/sqlCellStatusBarProvider.ts
🧠 Learnings (5)
📚 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/BigQueryForm.tsx
📚 Learning: 2025-10-17T11:36:40.042Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#73
File: src/webviews/webview-side/integrations/BigQueryForm.tsx:0-0
Timestamp: 2025-10-17T11:36:40.042Z
Learning: The vscode API (including l10n) is not available in webview React components located in src/webviews/webview-side/. User-facing strings in these components cannot use l10n.t() for localization.
Applied to files:
src/webviews/webview-side/integrations/BigQueryForm.tsx
📚 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/chat/extension.node.ts : Register chat tools in chat/extension.node.ts via vscode.lm.registerTool(...) and add the disposables to context.subscriptions
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.ts
📚 Learning: 2025-09-03T12:54:35.368Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/interactiveWindow.instructions.md:0-0
Timestamp: 2025-09-03T12:54:35.368Z
Learning: Applies to src/interactive-window/**/*.ts : Use EventEmitter for event-driven updates and ensure proper disposal/cleanup of listeners to prevent leaks
Applied to files:
src/notebooks/deepnote/sqlCellStatusBarProvider.ts
📚 Learning: 2025-10-07T15:31:30.150Z
Learnt from: hc2p
PR: deepnote/deepnote-internal#18615
File: apps/webapp-client/src/api/integration-api.ts:204-204
Timestamp: 2025-10-07T15:31:30.150Z
Learning: In apps/webapp-client/src/api/integration-api.ts, when reviewing bug fixes authored by the PR creator (e.g., changing integration.state to integration.type), use informational tone rather than marking as critical issues, since the fix is already implemented.
Applied to files:
src/webviews/webview-side/integrations/PostgresForm.tsx
🧬 Code graph analysis (3)
src/webviews/webview-side/integrations/BigQueryForm.tsx (3)
src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)src/webviews/webview-side/integrations/types.ts (1)
BigQueryIntegrationConfig(21-25)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
BigQueryIntegrationConfig(40-44)
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (2)
src/notebooks/deepnote/integrations/types.ts (2)
IIntegrationStorage(5-5)IIntegrationStorage(6-24)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
DATAFRAME_SQL_INTEGRATION_ID(5-5)
src/webviews/webview-side/integrations/PostgresForm.tsx (3)
src/webviews/webview-side/integrations/types.ts (1)
PostgresIntegrationConfig(11-19)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
PostgresIntegrationConfig(27-35)src/webviews/webview-side/react-common/locReactSide.ts (1)
getLocString(11-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (7)
src/notebooks/deepnote/integrations/integrationWebview.ts (2)
142-146: No issues found. Payload shape matches webview listener.Verification confirms the integration webview sends
{ type: 'loc_init', locStrings: string }which matches theLocInitMessageinterface and the listener's expectations inIntegrationPanel.tsx.
63-74: Proposed solution is incomplete and would hang indefinitely.The race condition is real—IntegrationPanel.tsx attaches its message listener in useEffect (line 77), which runs asynchronously after React renders. Meanwhile, integrationWebview.ts sends messages immediately (lines 87–93), before the webview's listener is attached.
However, the diff is broken: it expects the webview to emit a
SharedMessages.Startedor'ready'message, but IntegrationPanel.tsx sends only'configure','delete', and'save'message types. Waiting forwebviewReady.promisewill hang indefinitely because that signal is never sent.To fix this properly, either:
- Modify IntegrationPanel.tsx to post a
SharedMessages.Startedmessage when the listener attaches, or- Use a different synchronization mechanism (e.g., listen for the first valid message type instead of waiting for a nonexistent signal).
src/notebooks/deepnote/sqlCellStatusBarProvider.ts (5)
1-18: Imports look good.All necessary dependencies imported and properly ordered per guidelines.
34-47: LGTM! EventEmitter disposal properly handled.Past review concern about EventEmitter leaks is now addressed (line 46).
49-74: LGTM! Proper filtering and cancellation handling.Logic correctly filters SQL cells, excludes internal integrations, and checks cancellation.
76-87: LGTM! Safe metadata access with proper type guards.Implementation matches the pattern used elsewhere in the codebase.
89-113: LGTM! Past review issues resolved.Returns plain object (not constructor), uses l10n.t() for localization, and includes appropriate fallback logic. Alignment value documented inline.
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 (5)
src/notebooks/deepnote/integrations/integrationWebview.ts(5 hunks)src/platform/webviews/webviewHost.ts(1 hunks)src/webviews/webview-side/integrations/IntegrationPanel.tsx(4 hunks)src/webviews/webview-side/integrations/types.ts(1 hunks)src/webviews/webview-side/react-common/locReactSide.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/webviews/webview-side/react-common/locReactSide.tssrc/platform/webviews/webviewHost.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:
src/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/react-common/locReactSide.tssrc/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/platform/webviews/webviewHost.tssrc/webviews/webview-side/integrations/types.ts
src/notebooks/deepnote/**
📄 CodeRabbit inference engine (CLAUDE.md)
Deepnote integration code resides under src/notebooks/deepnote/
Files:
src/notebooks/deepnote/integrations/integrationWebview.ts
src/platform/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/platform.instructions.md)
src/platform/**/*.ts: Use Inversify decorators for DI: annotate classes with @Injectable() and inject dependencies with @Inject(Token)
Use the centralized logger (import { logger } from '../platform/logging') instead of console.log for application logging
Files:
src/platform/webviews/webviewHost.ts
🧠 Learnings (4)
📚 Learning: 2025-10-17T12:29:11.814Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#73
File: src/webviews/webview-side/integrations/PostgresForm.tsx:45-51
Timestamp: 2025-10-17T12:29:11.814Z
Learning: In webview-side React components (src/webviews/webview-side/), use the format() function with hardcoded template strings for dynamic localization with placeholders, not getLocString(). The LocalizedMessages type only supports string properties, not functions, so templates like format('Unnamed PostgreSQL Integration ({0})', id) are the correct pattern.
Applied to files:
src/notebooks/deepnote/integrations/integrationWebview.tssrc/webviews/webview-side/react-common/locReactSide.tssrc/webviews/webview-side/integrations/IntegrationPanel.tsxsrc/platform/webviews/webviewHost.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/chat/extension.node.ts : Register chat tools in chat/extension.node.ts via vscode.lm.registerTool(...) and add the disposables to context.subscriptions
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/webviews/webview-side/integrations/IntegrationPanel.tsx
📚 Learning: 2025-10-17T11:36:40.042Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#73
File: src/webviews/webview-side/integrations/BigQueryForm.tsx:0-0
Timestamp: 2025-10-17T11:36:40.042Z
Learning: The vscode API (including l10n) is not available in webview React components located in src/webviews/webview-side/. User-facing strings in these components cannot use l10n.t() for localization.
Applied to files:
src/webviews/webview-side/integrations/IntegrationPanel.tsx
🧬 Code graph analysis (4)
src/notebooks/deepnote/integrations/integrationWebview.ts (4)
src/webviews/webview-side/integrations/types.ts (1)
IntegrationWithStatus(29-33)src/notebooks/deepnote/integrations/integrationTypes.ts (1)
IntegrationWithStatus(63-67)src/messageTypes.ts (1)
LocalizedMessages(123-207)src/platform/logging/index.ts (1)
logger(35-48)
src/webviews/webview-side/react-common/locReactSide.ts (1)
src/messageTypes.ts (1)
LocalizedMessages(123-207)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
src/webviews/webview-side/react-common/locReactSide.ts (2)
storeLocStrings(19-21)getLocString(11-17)
src/platform/webviews/webviewHost.ts (1)
src/messageTypes.ts (1)
LocalizedMessages(123-207)
⏰ 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 (7)
src/platform/webviews/webviewHost.ts (1)
224-269: LGTM — Partial localization pattern applied correctly.The type relaxation to
Partial<LocalizedMessages>allows sending only the keys this webview actually uses, preventing empty-string overrides in the webview-side fallback logic.src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
140-180: LGTM — getLocString usage is correct.All user-facing strings properly use
getLocStringwith fallback defaults, following the webview-side localization pattern.Based on learnings
src/webviews/webview-side/react-common/locReactSide.ts (1)
9-21: LGTM — Partial localization support implemented correctly.The type relaxation to
Partial<LocalizedMessages>with?? defValuefallback (line 13) properly handles optional keys, ensuring getLocString returns the default when a key is omitted from the payload.src/webviews/webview-side/integrations/types.ts (1)
57-62: LGTM — LocInitMessage type definitions correct.The interface properly types the loc_init payload with
Partial<LocalizedMessages>, coordinating with the webview host and receiver implementations.src/notebooks/deepnote/integrations/integrationWebview.ts (3)
32-94: LGTM — selectedIntegrationId parameter integrated correctly.The optional parameter enables pre-selection of an integration for immediate configuration. Both code paths (panel reuse at lines 43-46 and new panel creation at lines 90-93) consistently handle the conditional form display.
96-146: LGTM — sendLocStrings implementation is correct.Localization strings are sent as an object (line 144), not stringified, which aligns with the
Partial<LocalizedMessages>type inLocInitMessageand avoids unnecessary serialization round-trips. The method pulls from thelocalize.Integrationsnamespace and correctly usesPartial<LocalizedMessages>to send only integration-related keys.
151-162: LGTM — Debug logging is safe.Logs only count metadata (line 162), avoiding exposure of credentials or config details from integration objects.
jankuca
changed the title
jankuca
changed the title
jankuca
requested review from
Artmann,
andyjakubowski,
jamesbhobbs and
saltenasl
as code owners
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 (1)
src/webviews/webview-side/integrations/IntegrationPanel.tsx(4 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/IntegrationPanel.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-17T12:29:11.814Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#73
File: src/webviews/webview-side/integrations/PostgresForm.tsx:45-51
Timestamp: 2025-10-17T12:29:11.814Z
Learning: In webview-side React components (src/webviews/webview-side/), use the format() function with hardcoded template strings for dynamic localization with placeholders, not getLocString(). The LocalizedMessages type only supports string properties, not functions, so templates like format('Unnamed PostgreSQL Integration ({0})', id) are the correct pattern.
Applied to files:
src/webviews/webview-side/integrations/IntegrationPanel.tsx
📚 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.tsx
📚 Learning: 2025-10-17T11:36:40.042Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#73
File: src/webviews/webview-side/integrations/BigQueryForm.tsx:0-0
Timestamp: 2025-10-17T11:36:40.042Z
Learning: The vscode API (including l10n) is not available in webview React components located in src/webviews/webview-side/. User-facing strings in these components cannot use l10n.t() for localization.
Applied to files:
src/webviews/webview-side/integrations/IntegrationPanel.tsx
🧬 Code graph analysis (1)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
src/webviews/webview-side/react-common/locReactSide.ts (2)
storeLocStrings(19-21)getLocString(11-17)
🪛 Biome (2.1.2)
src/webviews/webview-side/integrations/IntegrationPanel.tsx
[error] 44-44: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (2)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (2)
3-3: LGTM: Correct localization imports.Properly imports webview-side localization utilities.
138-178: LGTM: Localization correctly implemented.All user-facing strings use
getLocString()with appropriate keys and fallback values per webview-side requirements.Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
43-45: Eliminate intermediate variable or wrap in braces.The
const locStringsdeclaration can leak to other cases. Either passmsg.locStringsdirectly tostoreLocStrings()or wrap the case body in braces.Apply this diff:
case 'loc_init': - const locStrings = msg.locStrings; - storeLocStrings(locStrings); + storeLocStrings(msg.locStrings); break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/webviews/webview-side/integrations/IntegrationPanel.tsx(4 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/IntegrationPanel.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-17T12:29:11.814Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#73
File: src/webviews/webview-side/integrations/PostgresForm.tsx:45-51
Timestamp: 2025-10-17T12:29:11.814Z
Learning: In webview-side React components (src/webviews/webview-side/), use the format() function with hardcoded template strings for dynamic localization with placeholders, not getLocString(). The LocalizedMessages type only supports string properties, not functions, so templates like format('Unnamed PostgreSQL Integration ({0})', id) are the correct pattern.
Applied to files:
src/webviews/webview-side/integrations/IntegrationPanel.tsx
📚 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.tsx
📚 Learning: 2025-10-17T11:36:40.042Z
Learnt from: jankuca
PR: deepnote/vscode-deepnote#73
File: src/webviews/webview-side/integrations/BigQueryForm.tsx:0-0
Timestamp: 2025-10-17T11:36:40.042Z
Learning: The vscode API (including l10n) is not available in webview React components located in src/webviews/webview-side/. User-facing strings in these components cannot use l10n.t() for localization.
Applied to files:
src/webviews/webview-side/integrations/IntegrationPanel.tsx
🧬 Code graph analysis (1)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
src/webviews/webview-side/react-common/locReactSide.ts (2)
storeLocStrings(19-21)getLocString(11-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & Test
🔇 Additional comments (1)
src/webviews/webview-side/integrations/IntegrationPanel.tsx (1)
137-137: LGTM: Localization implementation.All user-facing strings correctly use
getLocStringwith appropriate fallbacks. Pattern aligns with webview-side localization approach.Also applies to: 156-177
Summary by CodeRabbit
New Features
Improvements
Tests