From 3906a850e0035deef3eff0fc6faa028a8ab12419 Mon Sep 17 00:00:00 2001 From: James Hobbs Date: Thu, 10 Sep 2026 14:03:36 +0100 Subject: [PATCH 1/4] feat(integrations): add an "Add Existing Integration" picker to reuse integrations across projects Integrations created through the extension UI are stored in SecretStorage keyed by integration id, but each project's `.deepnote` roster decides which of them apply to it, so the same database had to be configured again for every project in a workspace. Add `deepnote.addExistingIntegration` (command palette, and an "Add Existing Integration" button in the Manage Integrations panel). It scans the workspace's `.deepnote` files for integrations other projects declare that have a stored config, offers them in a QuickPick (name, type, which projects use them), and links the chosen one into the active project's roster through `persistProjectIntegrations`. Credentials are not copied: the roster entry is the only per-project scoping, so the linked project resolves the same config (and federated refresh token). Running kernels of the project get an env refresh and the panel is re-shown, since no storage change event fires for a roster-only edit. Integrations already on the roster, file-only (`.deepnote.env.yaml`) ones and ids whose roster type disagrees with the stored config are not offered; the last case is reported with a warning. Co-Authored-By: Claude Fable 5.1 --- package.json | 6 + package.nls.json | 1 + specs/INTEGRATIONS_CREDENTIALS.md | 14 + src/messageTypes.ts | 1 + .../integrations/existingIntegrationPicker.ts | 204 +++++++++++ .../existingIntegrationPicker.unit.test.ts | 340 ++++++++++++++++++ .../integrations/integrationManager.ts | 188 +++++++++- .../integrationManager.unit.test.ts | 239 ++++++++++++ .../integrations/integrationWebview.ts | 11 + .../integrationWebview.unit.test.ts | 19 + src/platform/analytics/types.ts | 3 + src/platform/common/constants.ts | 1 + src/platform/common/utils/localize.ts | 18 + .../integrations/IntegrationPanel.tsx | 6 +- .../integrations/IntegrationTypeSelector.tsx | 11 +- .../integrations/integrations.css | 11 +- .../webview-side/integrations/types.ts | 1 + .../suite/workspace/integrations.e2e.test.ts | 4 + 18 files changed, 1068 insertions(+), 10 deletions(-) create mode 100644 src/notebooks/deepnote/integrations/existingIntegrationPicker.ts create mode 100644 src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts create mode 100644 src/notebooks/deepnote/integrations/integrationManager.unit.test.ts diff --git a/package.json b/package.json index 38a7179ac9..bf7a055e96 100644 --- a/package.json +++ b/package.json @@ -144,6 +144,12 @@ "category": "Deepnote", "icon": "$(plug)" }, + { + "command": "deepnote.addExistingIntegration", + "title": "%deepnote.commands.addExistingIntegration.title%", + "category": "Deepnote", + "icon": "$(link)" + }, { "command": "deepnote.authenticateIntegration", "title": "%deepnote.commands.authenticateIntegration.title%", diff --git a/package.nls.json b/package.nls.json index 2cf9d50a05..aeb287ddcf 100644 --- a/package.nls.json +++ b/package.nls.json @@ -256,6 +256,7 @@ "deepnote.commands.enableSnapshots.title": "Enable Snapshots", "deepnote.commands.disableSnapshots.title": "Disable Snapshots", "deepnote.commands.manageIntegrations.title": "Manage Integrations", + "deepnote.commands.addExistingIntegration.title": "Add Existing Integration", "deepnote.commands.authenticateIntegration.title": "Authenticate Integration", "deepnote.commands.newProject.title": "New Project", "deepnote.commands.importNotebook.title": "Import Notebook", diff --git a/specs/INTEGRATIONS_CREDENTIALS.md b/specs/INTEGRATIONS_CREDENTIALS.md index 8edc5e2233..8b382cd553 100644 --- a/specs/INTEGRATIONS_CREDENTIALS.md +++ b/specs/INTEGRATIONS_CREDENTIALS.md @@ -433,6 +433,20 @@ Orchestrates the integration management UI and commands. 3. Manager opens webview with integration list 4. Optionally pre-selects a specific integration for configuration +**Add Existing Integration** (`deepnote.addExistingIntegration`, also the "Add Existing Integration" button in the +webview, implemented in `existingIntegrationPicker.ts`): + +1. Scans every `.deepnote` file in the workspace folders (snapshots excluded) and collects the roster entries of + _other_ projects whose id has a config in SecretStorage; ids already on the active project's roster are skipped, + as are file-only (`.deepnote.env.yaml`) integrations, which already apply workspace-wide. +2. Shows a QuickPick (name, type, "Used in: "). An id whose roster type disagrees with the stored config's + type is dropped with a warning rather than offered. +3. On selection, appends `{ id, name, type }` to the active project's roster via `persistProjectIntegrations`. + Nothing is copied in SecretStorage: configs (and federated refresh tokens) are keyed by integration id alone, and + the roster entry is what scopes an integration to a project, so the linked project resolves the same credentials. +4. Re-runs the integration env refresh in the project's running kernels and re-shows the panel, since no storage + change event fires for a roster-only edit. + #### 4. **Integration Webview** (`integrationWebview.ts`) Provides the webview-based UI for managing integration credentials. diff --git a/src/messageTypes.ts b/src/messageTypes.ts index 76d893cd53..17974216e9 100644 --- a/src/messageTypes.ts +++ b/src/messageTypes.ts @@ -186,6 +186,7 @@ export type LocalizedMessages = { integrationsCancel: string; integrationsSave: string; integrationsAddNewIntegration: string; + integrationsAddExistingIntegration: string; integrationsDatabase: string; integrationsDataWarehousesLakes: string; integrationsDatabases: string; diff --git a/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts b/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts new file mode 100644 index 0000000000..215ca7372a --- /dev/null +++ b/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts @@ -0,0 +1,204 @@ +import { RelativePattern, Uri, workspace } from 'vscode'; + +import * as localize from '../../../platform/common/utils/localize'; +import { readDeepnoteProjectFile } from '../../../platform/deepnote/deepnoteProjectFileReader'; +import { logger } from '../../../platform/logging'; +import { + ConfigurableDatabaseIntegrationType, + isConfigurableDatabaseIntegrationType +} from '../../../platform/notebooks/deepnote/integrationTypes'; +import { IDeepnoteNotebookManager, ProjectIntegration } from '../../types'; +import { isSnapshotFile } from '../snapshots/snapshotFiles'; +import { PersistIntegrationsResult, persistProjectIntegrations } from './projectIntegrationsWriter'; +import { IIntegrationStorage } from './types'; + +/** Human-readable type labels for the picker; mirrors `integrationTypeLabels` in the webview bundle. */ +const INTEGRATION_TYPE_LABELS: Record = { + alloydb: localize.Integrations.alloyDBTypeLabel, + athena: localize.Integrations.athenaTypeLabel, + 'big-query': localize.Integrations.bigQueryTypeLabel, + clickhouse: localize.Integrations.clickHouseTypeLabel, + 'cloud-sql': localize.Integrations.cloudSqlTypeLabel, + databricks: localize.Integrations.databricksTypeLabel, + dremio: localize.Integrations.dremioTypeLabel, + mariadb: localize.Integrations.mariaDBTypeLabel, + materialize: localize.Integrations.materializeTypeLabel, + mindsdb: localize.Integrations.mindsDBTypeLabel, + mongodb: localize.Integrations.mongoDBTypeLabel, + mysql: localize.Integrations.mySQLTypeLabel, + pgsql: localize.Integrations.postgresTypeLabel, + redshift: localize.Integrations.redshiftTypeLabel, + snowflake: localize.Integrations.snowflakeTypeLabel, + spanner: localize.Integrations.spannerTypeLabel, + 'sql-server': localize.Integrations.sqlServerTypeLabel, + trino: localize.Integrations.trinoTypeLabel +}; + +export function integrationTypeLabel(type: ConfigurableDatabaseIntegrationType): string { + return INTEGRATION_TYPE_LABELS[type] ?? type; +} + +/** + * A SecretStorage integration declared by at least one *other* project in the workspace, so it can be linked into + * the current project without re-entering credentials. + */ +export interface ReusableIntegration { + id: string; + /** Name from the stored config — the same source the panel writes to the roster on save. */ + name: string; + /** Display names of the other projects whose roster declares this integration; deduped and sorted. */ + projectNames: string[]; + type: ConfigurableDatabaseIntegrationType; +} + +export interface CollectReusableIntegrationsParams { + /** Integration ids already on the current project's roster; never offered again. */ + excludeIntegrationIds: ReadonlySet; + integrationStorage: IIntegrationStorage; + /** The project being extended; its own `.deepnote` files are skipped. */ + projectId: string; +} + +export interface CollectReusableIntegrationsResult { + /** + * Ids skipped because a project's roster declares the integration with a type that differs from the stored + * configuration. Linking such an entry would put a roster type on this project that the credentials cannot + * back, so the caller warns instead. + */ + conflictingIds: string[]; + integrations: ReusableIntegration[]; +} + +export interface AttachExistingIntegrationParams { + activeFileUri: Uri; + /** The current project's roster as cached by the notebook manager; the new entry is appended to it. */ + currentIntegrations: readonly ProjectIntegration[]; + integration: ReusableIntegration; + notebookManager: IDeepnoteNotebookManager; + projectId: string; +} + +/** + * Scans every `.deepnote` file in the open workspace folders and collects the SecretStorage integrations other + * projects declare. + * + * Storage design: `IntegrationStorage` keys configs by integration id alone (there is no per-project namespace), + * and both the env-var provider and the detector resolve credentials from the project roster + * (`project.integrations[].id`). The roster entry is therefore the only thing that "attaches" an integration to a + * project, and reusing one is a pure link: no config is copied. Federated (`google-oauth`) integrations are + * included for the same reason — `FederatedAuthTokenStorage` is also keyed by integration id, and the per-cell + * code generator resolves the config through the roster of the notebook being run. + * + * Integrations configured only in `.deepnote.env.yaml` (no stored config) are not offered: that file already + * applies to every project under it, and the panel cannot write that layer. + */ +export async function collectReusableIntegrations( + params: CollectReusableIntegrationsParams +): Promise { + const { excludeIntegrationIds, integrationStorage, projectId } = params; + + const candidates = new Map }>(); + const conflictingIds = new Set(); + const visited = new Set(); + + for (const workspaceFolder of workspace.workspaceFolders || []) { + let files: Uri[]; + + try { + files = await workspace.findFiles(new RelativePattern(workspaceFolder, '**/*.deepnote')); + } catch (error) { + logger.error('collectReusableIntegrations: failed to enumerate .deepnote files', error); + + continue; + } + + for (const fileUri of files) { + const key = fileUri.toString(); + + if (visited.has(key) || isSnapshotFile(fileUri)) { + continue; + } + + visited.add(key); + + // Per-file try/catch: one unreadable file must not hide every other project's integrations. + try { + const projectData = await readDeepnoteProjectFile(fileUri); + + if (!projectData?.project || projectData.project.id === projectId) { + continue; + } + + const projectName = projectData.project.name || projectData.project.id; + + for (const entry of projectData.project.integrations ?? []) { + if (excludeIntegrationIds.has(entry.id) || !isConfigurableDatabaseIntegrationType(entry.type)) { + continue; + } + + const storedConfig = await integrationStorage.getIntegrationConfig(entry.id); + + if (!storedConfig) { + // File-only or never-configured: there are no credentials in SecretStorage to reuse. + continue; + } + + if (storedConfig.type !== entry.type) { + logger.warn( + `collectReusableIntegrations: ${entry.id} is declared as ${entry.type} in ${fileUri.path} but stored as ${storedConfig.type}; skipping` + ); + conflictingIds.add(entry.id); + + continue; + } + + const existing = candidates.get(entry.id); + + if (existing) { + existing.projectNameSet.add(projectName); + } else { + candidates.set(entry.id, { + id: entry.id, + name: storedConfig.name || entry.name || entry.id, + projectNameSet: new Set([projectName]), + projectNames: [], + type: storedConfig.type + }); + } + } + } catch (error) { + logger.error(`collectReusableIntegrations: failed to read ${fileUri.path}`, error); + } + } + } + + // A conflict in any project disqualifies the id everywhere: the stored config is the single shared truth. + for (const id of conflictingIds) { + candidates.delete(id); + } + + const integrations = Array.from(candidates.values()) + .map(({ projectNameSet, ...candidate }) => ({ + ...candidate, + projectNames: Array.from(projectNameSet).sort((a, b) => a.localeCompare(b)) + })) + .sort((a, b) => a.name.localeCompare(b.name) || a.id.localeCompare(b.id)); + + return { conflictingIds: Array.from(conflictingIds).sort(), integrations }; +} + +/** + * Links `integration` into the project's roster and persists it through the same writer the panel uses, so the + * cache, the active file and every sibling `.deepnote` file of the project are updated together. Idempotent for an + * id already on the roster (the entry is replaced, not duplicated). + */ +export function attachExistingIntegration(params: AttachExistingIntegrationParams): Promise { + const { activeFileUri, currentIntegrations, integration, notebookManager, projectId } = params; + + const integrations: ProjectIntegration[] = [ + ...currentIntegrations.filter((entry) => entry.id !== integration.id), + { id: integration.id, name: integration.name, type: integration.type } + ]; + + return persistProjectIntegrations({ activeFileUri, integrations, notebookManager, projectId }); +} diff --git a/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts b/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts new file mode 100644 index 0000000000..283c64641a --- /dev/null +++ b/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts @@ -0,0 +1,340 @@ +import { deserializeDeepnoteFile, serializeDeepnoteFile, type DeepnoteFile } from '@deepnote/blocks'; +import { assert } from 'chai'; +import { anything, instance, mock, when } from 'ts-mockito'; +import { Uri, workspace } from 'vscode'; + +import { ConfigurableDatabaseIntegrationConfig } from '../../../platform/notebooks/deepnote/integrationTypes'; +import { mockedVSCodeNamespaces, resetVSCodeMocks } from '../../../test/vscode-mock'; +import { IDeepnoteNotebookManager, ProjectIntegration } from '../../types'; +import { createDeepnoteFile, createDeepnoteProject, createWorkspaceFolder } from '../deepnoteTestHelpers'; +import { + attachExistingIntegration, + collectReusableIntegrations, + integrationTypeLabel, + ReusableIntegration +} from './existingIntegrationPicker'; +import { buildGoogleOauthIntegration, buildPostgresIntegration } from './federatedAuth/federatedAuthTestHelpers'; +import { IIntegrationStorage } from './types'; + +const CURRENT_PROJECT_ID = 'project-current'; + +interface OnDiskProject { + uri: Uri; + projectId: string; + projectName?: string; + integrations?: Array<{ id: string; name: string; type: string }>; +} + +function projectFile(project: OnDiskProject): DeepnoteFile { + return createDeepnoteFile({ + project: createDeepnoteProject({ + id: project.projectId, + name: project.projectName ?? project.projectId, + // The roster type is a plain string on disk; the cast keeps the fixture free to declare unknown types. + integrations: project.integrations as ProjectIntegration[] | undefined + }) + }); +} + +/** Stubs `workspace.findFiles` + `workspace.fs` over the given files; `unreadable` URIs reject on read. */ +function stubWorkspace(opts: { projects: OnDiskProject[]; unreadable?: Uri[]; hasWorkspaceFolder?: boolean }): { + writes: Map; +} { + when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn( + opts.hasWorkspaceFolder === false ? undefined : [createWorkspaceFolder(Uri.file('/ws'))] + ); + + const discovered = [...opts.projects.map((project) => project.uri), ...(opts.unreadable ?? [])]; + when(mockedVSCodeNamespaces.workspace.findFiles(anything())).thenReturn(Promise.resolve(discovered)); + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([]); + + const byPath = new Map(opts.projects.map((project) => [project.uri.fsPath, projectFile(project)] as const)); + const writes = new Map(); + const mockFs = mock(); + + when(mockFs.readFile(anything())).thenCall((uri: Uri) => { + const file = byPath.get(uri.fsPath); + + return file + ? Promise.resolve(new TextEncoder().encode(serializeDeepnoteFile(file))) + : Promise.reject(new Error(`no readFile stub for ${uri.fsPath}`)); + }); + when(mockFs.writeFile(anything(), anything())).thenCall((uri: Uri, bytes: Uint8Array) => { + writes.set(uri.fsPath, deserializeDeepnoteFile(new TextDecoder().decode(bytes))); + + return Promise.resolve(); + }); + when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); + + return { writes }; +} + +function stubStorage(configs: ConfigurableDatabaseIntegrationConfig[]): IIntegrationStorage { + const byId = new Map(configs.map((config) => [config.id, config] as const)); + const storage = mock(); + + when(storage.getIntegrationConfig(anything())).thenCall((id: string) => Promise.resolve(byId.get(id))); + + return instance(storage); +} + +suite('existingIntegrationPicker', () => { + setup(() => { + resetVSCodeMocks(); + }); + + suite('collectReusableIntegrations', () => { + const pgConfig = buildPostgresIntegration({ id: 'pg-shared', name: 'Shared Postgres' }); + const bqConfig = buildGoogleOauthIntegration({ id: 'bq-oauth', name: 'Team BigQuery' }); + + async function collect( + excludeIntegrationIds: string[] = [], + configs: ConfigurableDatabaseIntegrationConfig[] = [pgConfig, bqConfig] + ) { + return collectReusableIntegrations({ + excludeIntegrationIds: new Set(excludeIntegrationIds), + integrationStorage: stubStorage(configs), + projectId: CURRENT_PROJECT_ID + }); + } + + test('lists integrations other projects declare, deduped by id with every using project named', async () => { + stubWorkspace({ + projects: [ + { + uri: Uri.file('/ws/a.deepnote'), + projectId: 'project-a', + projectName: 'Alpha', + integrations: [{ id: 'pg-shared', name: 'Alpha Postgres', type: 'pgsql' }] + }, + { + uri: Uri.file('/ws/b.deepnote'), + projectId: 'project-b', + projectName: 'Beta', + integrations: [ + { id: 'pg-shared', name: 'Beta Postgres', type: 'pgsql' }, + { id: 'bq-oauth', name: 'Team BigQuery', type: 'big-query' } + ] + }, + // A second notebook file of Beta must not list Beta twice. + { + uri: Uri.file('/ws/b-2.deepnote'), + projectId: 'project-b', + projectName: 'Beta', + integrations: [{ id: 'pg-shared', name: 'Beta Postgres', type: 'pgsql' }] + } + ] + }); + + const result = await collect(); + + const expected: ReusableIntegration[] = [ + { id: 'pg-shared', name: 'Shared Postgres', projectNames: ['Alpha', 'Beta'], type: 'pgsql' }, + { id: 'bq-oauth', name: 'Team BigQuery', projectNames: ['Beta'], type: 'big-query' } + ]; + assert.deepStrictEqual(result, { conflictingIds: [], integrations: expected }); + }); + + test('takes the name from the stored config, not from whichever roster was read first', async () => { + stubWorkspace({ + projects: [ + { + uri: Uri.file('/ws/a.deepnote'), + projectId: 'project-a', + integrations: [{ id: 'pg-shared', name: 'Stale roster name', type: 'pgsql' }] + } + ] + }); + + const { integrations } = await collect(); + + assert.strictEqual(integrations[0].name, 'Shared Postgres'); + }); + + test('excludes ids already on the current project roster and the current project files themselves', async () => { + stubWorkspace({ + projects: [ + { + uri: Uri.file('/ws/current.deepnote'), + projectId: CURRENT_PROJECT_ID, + integrations: [{ id: 'bq-oauth', name: 'Team BigQuery', type: 'big-query' }] + }, + { + uri: Uri.file('/ws/a.deepnote'), + projectId: 'project-a', + integrations: [ + { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' }, + { id: 'bq-oauth', name: 'Team BigQuery', type: 'big-query' } + ] + } + ] + }); + + const { integrations } = await collect(['pg-shared']); + + assert.deepStrictEqual( + integrations.map((integration) => integration.id), + ['bq-oauth'], + 'pg-shared is already attached; bq-oauth is offered because project-a (not the current project) declares it' + ); + }); + + test('skips roster entries with no stored config (file-only or never configured) and unsupported types', async () => { + stubWorkspace({ + projects: [ + { + uri: Uri.file('/ws/a.deepnote'), + projectId: 'project-a', + integrations: [ + { id: 'file-only', name: 'From env yaml', type: 'pgsql' }, + { id: 'duckdb', name: 'DuckDB', type: 'pandas-dataframe' }, + { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' } + ] + } + ] + }); + + const { integrations } = await collect(); + + assert.deepStrictEqual( + integrations.map((integration) => integration.id), + ['pg-shared'] + ); + }); + + test('reports an id whose roster type disagrees with the stored config as conflicting and drops it everywhere', async () => { + stubWorkspace({ + projects: [ + { + uri: Uri.file('/ws/a.deepnote'), + projectId: 'project-a', + integrations: [{ id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' }] + }, + { + uri: Uri.file('/ws/b.deepnote'), + projectId: 'project-b', + // Same id, but declared as a different database than the credentials are for. + integrations: [{ id: 'pg-shared', name: 'Not really Postgres', type: 'mysql' }] + } + ] + }); + + const result = await collect(); + + assert.deepStrictEqual(result, { conflictingIds: ['pg-shared'], integrations: [] }); + }); + + test('ignores snapshot files and keeps going past an unreadable file', async () => { + stubWorkspace({ + projects: [ + { + uri: Uri.file('/ws/snapshots/a_project-a_2024.snapshot.deepnote'), + projectId: 'project-snapshot', + integrations: [{ id: 'bq-oauth', name: 'Team BigQuery', type: 'big-query' }] + }, + { + uri: Uri.file('/ws/a.deepnote'), + projectId: 'project-a', + integrations: [{ id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' }] + } + ], + unreadable: [Uri.file('/ws/broken.deepnote')] + }); + + const { integrations } = await collect(); + + assert.deepStrictEqual( + integrations.map((integration) => integration.id), + ['pg-shared'] + ); + }); + + test('returns nothing without an open workspace folder', async () => { + stubWorkspace({ projects: [], hasWorkspaceFolder: false }); + + const result = await collect(); + + assert.deepStrictEqual(result, { conflictingIds: [], integrations: [] }); + }); + }); + + suite('attachExistingIntegration', () => { + const activeUri = Uri.file('/ws/current.deepnote'); + const shared: ReusableIntegration = { + id: 'pg-shared', + name: 'Shared Postgres', + projectNames: ['Alpha'], + type: 'pgsql' + }; + + let notebookManager: IDeepnoteNotebookManager; + let cacheUpdates: Array<{ projectId: string; integrations: ProjectIntegration[] }>; + + setup(() => { + cacheUpdates = []; + const mockManager = mock(); + when(mockManager.updateProjectIntegrations(anything(), anything())).thenCall( + (projectId: string, integrations: ProjectIntegration[]) => { + cacheUpdates.push({ projectId, integrations }); + + return true; + } + ); + notebookManager = instance(mockManager); + }); + + test('appends the linked entry to the roster in the cache and on disk, keeping existing entries', async () => { + const { writes } = stubWorkspace({ + projects: [ + { + uri: activeUri, + projectId: CURRENT_PROJECT_ID, + integrations: [{ id: 'bq-own', name: 'Own BigQuery', type: 'big-query' }] + } + ] + }); + const currentIntegrations: ProjectIntegration[] = [ + { id: 'bq-own', name: 'Own BigQuery', type: 'big-query' } + ]; + + const result = await attachExistingIntegration({ + activeFileUri: activeUri, + currentIntegrations, + integration: shared, + notebookManager, + projectId: CURRENT_PROJECT_ID + }); + + const expectedRoster: ProjectIntegration[] = [ + { id: 'bq-own', name: 'Own BigQuery', type: 'big-query' }, + { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' } + ]; + assert.deepStrictEqual(result, { activePersisted: true, siblingsFailed: 0 }); + assert.deepStrictEqual(cacheUpdates, [{ projectId: CURRENT_PROJECT_ID, integrations: expectedRoster }]); + assert.deepStrictEqual(writes.get(activeUri.fsPath)?.project.integrations, expectedRoster); + }); + + test('replaces rather than duplicates an entry whose id is already on the roster', async () => { + const { writes } = stubWorkspace({ + projects: [{ uri: activeUri, projectId: CURRENT_PROJECT_ID }] + }); + + await attachExistingIntegration({ + activeFileUri: activeUri, + currentIntegrations: [{ id: 'pg-shared', name: 'Old name', type: 'pgsql' }], + integration: shared, + notebookManager, + projectId: CURRENT_PROJECT_ID + }); + + assert.deepStrictEqual(writes.get(activeUri.fsPath)?.project.integrations, [ + { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' } + ]); + }); + }); + + test('integrationTypeLabel maps every configurable type to a display label', () => { + assert.strictEqual(integrationTypeLabel('pgsql'), 'PostgreSQL'); + assert.strictEqual(integrationTypeLabel('big-query'), 'Google BigQuery'); + }); +}); diff --git a/src/notebooks/deepnote/integrations/integrationManager.ts b/src/notebooks/deepnote/integrations/integrationManager.ts index 72b58dfada..fd1f56b379 100644 --- a/src/notebooks/deepnote/integrations/integrationManager.ts +++ b/src/notebooks/deepnote/integrations/integrationManager.ts @@ -1,12 +1,31 @@ -import { inject, injectable } from 'inversify'; -import { commands, l10n, NotebookDocument, window, workspace } from 'vscode'; +import { inject, injectable, optional } from 'inversify'; +import { commands, l10n, NotebookDocument, QuickPickItem, window, workspace } from 'vscode'; +import { CommandOutcome, ITelemetryService } from '../../../platform/analytics/types'; import { IExtensionContext } from '../../../platform/common/types'; import { Commands } from '../../../platform/common/constants'; +import * as localize from '../../../platform/common/utils/localize'; import { logger } from '../../../platform/logging'; -import { IIntegrationDetector, IIntegrationManager, IIntegrationStorage, IIntegrationWebviewProvider } from './types'; -import { IDeepnoteNotebookManager } from '../../types'; +import { isConfigurableDatabaseIntegrationType } from '../../../platform/notebooks/deepnote/integrationTypes'; +import { + IIntegrationDetector, + IIntegrationEnvLiveRefresher, + IIntegrationManager, + IIntegrationStorage, + IIntegrationWebviewProvider +} from './types'; +import { IDeepnoteNotebookManager, ProjectIntegration } from '../../types'; import { DatabaseIntegrationType, databaseIntegrationTypes } from '@deepnote/database-integrations'; +import { + attachExistingIntegration, + collectReusableIntegrations, + integrationTypeLabel, + ReusableIntegration +} from './existingIntegrationPicker'; + +interface ReusableIntegrationQuickPickItem extends QuickPickItem { + integration: ReusableIntegration; +} /** * Manages integration UI and commands for Deepnote notebooks @@ -18,7 +37,12 @@ export class IntegrationManager implements IIntegrationManager { @inject(IIntegrationDetector) private readonly integrationDetector: IIntegrationDetector, @inject(IIntegrationStorage) private readonly integrationStorage: IIntegrationStorage, @inject(IIntegrationWebviewProvider) private readonly webviewProvider: IIntegrationWebviewProvider, - @inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager + @inject(IDeepnoteNotebookManager) private readonly notebookManager: IDeepnoteNotebookManager, + @inject(ITelemetryService) private readonly analytics: ITelemetryService, + // Node-only service: the web extension has no kernels to refresh. + @inject(IIntegrationEnvLiveRefresher) + @optional() + private readonly liveRefresher?: IIntegrationEnvLiveRefresher ) {} public activate(): void { @@ -48,6 +72,127 @@ export class IntegrationManager implements IIntegrationManager { return this.showIntegrationsUI(integrationId, notebookUri); }) ); + + // Links an integration another project in the workspace already configured into the active project. + // Takes the same argument shapes as ManageIntegrations so the panel and menus can pass a notebook URI. + this.extensionContext.subscriptions.push( + commands.registerCommand(Commands.AddExistingIntegration, (...args: unknown[]) => { + let notebookUri: string | undefined; + + for (const arg of args) { + notebookUri ??= this.extractNotebookUri(arg); + } + + return this.addExistingIntegration(notebookUri); + }) + ); + } + + /** + * Offers the SecretStorage integrations other projects in the workspace declare, and links the chosen one into + * the active project's roster. Public so tests can drive it without `commands.executeCommand`. + * + * Credentials are not copied: `IntegrationStorage` is keyed by integration id alone, so the roster entry is all + * that scopes an integration to a project (see `collectReusableIntegrations`). + */ + public async addExistingIntegration(notebookUri?: string): Promise { + const activeNotebook = this.resolveDeepnoteNotebook(notebookUri); + + if (!activeNotebook) { + void window.showErrorMessage(l10n.t('No active Deepnote notebook')); + + return 'failed'; + } + + const projectId = activeNotebook.metadata?.deepnoteProjectId; + const notebookId = activeNotebook.metadata?.deepnoteNotebookId; + + if (!projectId || !notebookId) { + void window.showErrorMessage(l10n.t('Cannot determine project or notebook ID')); + + return 'failed'; + } + + const currentIntegrations = this.getCachedRoster(projectId, notebookId); + const { conflictingIds, integrations } = await collectReusableIntegrations({ + excludeIntegrationIds: new Set(currentIntegrations.map((entry) => entry.id)), + integrationStorage: this.integrationStorage, + projectId + }); + + if (conflictingIds.length > 0) { + void window.showWarningMessage( + localize.Integrations.addExistingIntegrationConflictsSkipped(conflictingIds.length) + ); + } + + if (integrations.length === 0) { + void window.showInformationMessage(localize.Integrations.addExistingIntegrationNoneAvailable); + + return 'completed'; + } + + const items: ReusableIntegrationQuickPickItem[] = integrations.map((integration) => ({ + description: integrationTypeLabel(integration.type), + detail: localize.Integrations.addExistingIntegrationUsedIn(integration.projectNames.join(', ')), + integration, + label: integration.name + })); + + const picked = await window.showQuickPick(items, { + matchOnDescription: true, + matchOnDetail: true, + placeHolder: localize.Integrations.addExistingIntegrationPlaceholder + }); + + if (!picked) { + return 'cancelled'; + } + + const { integration } = picked; + let outcome: CommandOutcome = 'failed'; + + try { + const { activePersisted, siblingsFailed } = await attachExistingIntegration({ + activeFileUri: activeNotebook.uri, + currentIntegrations, + integration, + notebookManager: this.notebookManager, + projectId + }); + + if (activePersisted) { + outcome = 'completed'; + void window.showInformationMessage( + localize.Integrations.addExistingIntegrationSucceeded(integration.name) + ); + + if (siblingsFailed > 0) { + void window.showWarningMessage( + l10n.t( + 'Integrations saved, but {0} related notebook file(s) could not be updated.', + siblingsFailed + ) + ); + } + + // Storage did not change, so the storage-change listeners that normally refresh kernels and the + // panel after a save stay silent; do both explicitly for this project. + await this.refreshAfterRosterChange(projectId, activeNotebook); + } else { + void window.showErrorMessage(localize.Integrations.addExistingIntegrationFailed); + } + } catch (error) { + logger.error(`IntegrationManager: failed to add existing integration ${integration.id}`, error); + void window.showErrorMessage(localize.Integrations.addExistingIntegrationFailed); + } + + this.analytics.trackEvent({ + eventName: 'add_existing_integration', + properties: { integrationType: integration.type, outcome } + }); + + return outcome; } /** The notebook URI a menu contribution passed; `notebook/toolbar` sends `{ notebookEditor: { notebookUri } }`. */ @@ -62,6 +207,39 @@ export class IntegrationManager implements IIntegrationManager { return uri ? String(uri) : undefined; } + /** The project's roster as the notebook manager caches it, narrowed to the types the panel can manage. */ + private getCachedRoster(projectId: string, notebookId: string): ProjectIntegration[] { + const project = this.notebookManager.getProjectForNotebook(projectId, notebookId); + const roster: ProjectIntegration[] = []; + + for (const entry of project?.project.integrations ?? []) { + if (isConfigurableDatabaseIntegrationType(entry.type)) { + roster.push({ id: entry.id, name: entry.name, type: entry.type }); + } + } + + return roster; + } + + /** Re-applies integration env in the project's running kernels and re-renders the panel with the new roster. */ + private async refreshAfterRosterChange(projectId: string, activeNotebook: NotebookDocument): Promise { + const projectNotebooks = workspace.notebookDocuments.filter( + (notebook) => notebook.notebookType === 'deepnote' && notebook.metadata?.deepnoteProjectId === projectId + ); + + try { + await this.liveRefresher?.refresh(projectNotebooks, 'integration_config'); + } catch (error) { + logger.error('IntegrationManager: failed to refresh integration env after adding an integration', error); + } + + try { + await this.showIntegrationsUI(undefined, activeNotebook.uri.toString()); + } catch (error) { + logger.error('IntegrationManager: failed to refresh the integrations panel', error); + } + } + /** * The Deepnote notebook to act on: `window.activeNotebookEditor` is unset until an editor is focused, so a * restored but not yet focused notebook resolves via the menu's URI or the one visible editor instead. diff --git a/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts b/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts new file mode 100644 index 0000000000..9b15dcf3a4 --- /dev/null +++ b/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts @@ -0,0 +1,239 @@ +import { deserializeDeepnoteFile, serializeDeepnoteFile, type DeepnoteFile } from '@deepnote/blocks'; +import { assert } from 'chai'; +import sinon from 'sinon'; +import { anything, deepEqual, instance, mock, verify, when } from 'ts-mockito'; +import { NotebookDocument, QuickPickItem, Uri, workspace } from 'vscode'; + +import { ITelemetryService } from '../../../platform/analytics/types'; +import { IExtensionContext } from '../../../platform/common/types'; +import { ConfigurableDatabaseIntegrationConfig } from '../../../platform/notebooks/deepnote/integrationTypes'; +import { mockedVSCodeNamespaces, resetVSCodeMocks } from '../../../test/vscode-mock'; +import { IDeepnoteNotebookManager, ProjectIntegration } from '../../types'; +import { + createDeepnoteFile, + createDeepnoteProject, + createMockNotebook, + createWorkspaceFolder +} from '../deepnoteTestHelpers'; +import { buildPostgresIntegration } from './federatedAuth/federatedAuthTestHelpers'; +import { IntegrationManager } from './integrationManager'; +import { + IIntegrationDetector, + IIntegrationEnvLiveRefresher, + IIntegrationStorage, + IIntegrationWebviewProvider +} from './types'; + +const CURRENT_PROJECT_ID = 'project-current'; +const CURRENT_NOTEBOOK_ID = 'notebook-current'; +const OTHER_PROJECT_ID = 'project-other'; +const CURRENT_URI = Uri.file('/ws/current.deepnote'); +const OTHER_URI = Uri.file('/ws/other.deepnote'); + +const SHARED_CONFIG = buildPostgresIntegration({ id: 'pg-shared', name: 'Shared Postgres' }); + +function projectFile(projectId: string, notebookId: string, integrations: ProjectIntegration[]): DeepnoteFile { + return createDeepnoteFile({ + project: createDeepnoteProject({ + id: projectId, + name: projectId, + notebooks: [{ id: notebookId, name: 'Notebook', blocks: [] }], + integrations + }) + }); +} + +suite('IntegrationManager.addExistingIntegration', () => { + let currentNotebook: NotebookDocument; + let otherNotebook: NotebookDocument; + let currentProject: DeepnoteFile; + let writes: Map; + let cacheUpdates: ProjectIntegration[][]; + let refreshSpy: sinon.SinonSpy; + let quickPickItems: QuickPickItem[] | undefined; + + let detector: IIntegrationDetector; + let webviewProvider: IIntegrationWebviewProvider; + let notebookManager: IDeepnoteNotebookManager; + let telemetry: ITelemetryService; + let storedConfigs: ConfigurableDatabaseIntegrationConfig[]; + let onDiskOther: DeepnoteFile | undefined; + + setup(() => { + resetVSCodeMocks(); + + currentNotebook = createMockNotebook({ + uri: CURRENT_URI, + metadata: { deepnoteProjectId: CURRENT_PROJECT_ID, deepnoteNotebookId: CURRENT_NOTEBOOK_ID } + }); + otherNotebook = createMockNotebook({ + uri: OTHER_URI, + metadata: { deepnoteProjectId: OTHER_PROJECT_ID, deepnoteNotebookId: 'notebook-other' } + }); + currentProject = projectFile(CURRENT_PROJECT_ID, CURRENT_NOTEBOOK_ID, []); + onDiskOther = projectFile(OTHER_PROJECT_ID, 'notebook-other', [ + { id: SHARED_CONFIG.id, name: SHARED_CONFIG.name, type: SHARED_CONFIG.type } + ]); + storedConfigs = [SHARED_CONFIG]; + writes = new Map(); + cacheUpdates = []; + quickPickItems = undefined; + + when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn([createWorkspaceFolder(Uri.file('/ws'))]); + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([currentNotebook, otherNotebook]); + when(mockedVSCodeNamespaces.workspace.findFiles(anything())).thenCall(() => + Promise.resolve(onDiskOther ? [CURRENT_URI, OTHER_URI] : [CURRENT_URI]) + ); + + const mockFs = mock(); + when(mockFs.readFile(anything())).thenCall((uri: Uri) => { + const file = uri.fsPath === CURRENT_URI.fsPath ? currentProject : onDiskOther; + + return file + ? Promise.resolve(new TextEncoder().encode(serializeDeepnoteFile(file))) + : Promise.reject(new Error(`no readFile stub for ${uri.fsPath}`)); + }); + when(mockFs.writeFile(anything(), anything())).thenCall((uri: Uri, bytes: Uint8Array) => { + writes.set(uri.fsPath, deserializeDeepnoteFile(new TextDecoder().decode(bytes))); + + return Promise.resolve(); + }); + when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); + + // Picks the first offered item; tests that want a cancel override this. + when(mockedVSCodeNamespaces.window.showQuickPick(anything(), anything())).thenCall((items: QuickPickItem[]) => { + quickPickItems = items; + + return Promise.resolve(items[0]); + }); + + const mockDetector = mock(); + when(mockDetector.detectIntegrations(anything())).thenResolve(new Map()); + detector = mockDetector; + + webviewProvider = mock(); + when(webviewProvider.show(anything(), anything(), anything(), anything(), anything())).thenResolve(); + + const mockManager = mock(); + when(mockManager.getProjectForNotebook(CURRENT_PROJECT_ID, CURRENT_NOTEBOOK_ID)).thenCall(() => currentProject); + when(mockManager.updateProjectIntegrations(anything(), anything())).thenCall( + (_projectId: string, integrations: ProjectIntegration[]) => { + cacheUpdates.push(integrations); + + return true; + } + ); + notebookManager = mockManager; + + telemetry = mock(); + refreshSpy = sinon.spy(async () => undefined); + }); + + function buildManager(): IntegrationManager { + const extensionContext = mock(); + when(extensionContext.subscriptions).thenReturn([]); + + const storage = mock(); + when(storage.getIntegrationConfig(anything())).thenCall((id: string) => + Promise.resolve(storedConfigs.find((config) => config.id === id)) + ); + + const liveRefresher: IIntegrationEnvLiveRefresher = { refresh: refreshSpy }; + + return new IntegrationManager( + instance(extensionContext), + instance(detector), + instance(storage), + instance(webviewProvider), + instance(notebookManager), + instance(telemetry), + liveRefresher + ); + } + + test("links the picked integration into the roster, refreshes only this project's kernels and re-shows the panel", async () => { + const outcome = await buildManager().addExistingIntegration(CURRENT_URI.toString()); + + assert.strictEqual(outcome, 'completed'); + + const expectedRoster: ProjectIntegration[] = [{ id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' }]; + assert.deepStrictEqual(writes.get(CURRENT_URI.fsPath)?.project.integrations, expectedRoster); + assert.deepStrictEqual(cacheUpdates, [expectedRoster]); + assert.isUndefined(writes.get(OTHER_URI.fsPath), 'the other project must not be rewritten'); + + assert.isTrue(refreshSpy.calledOnce); + assert.deepStrictEqual(refreshSpy.firstCall.args, [[currentNotebook], 'integration_config']); + + verify(webviewProvider.show(CURRENT_PROJECT_ID, anything(), anything(), anything(), anything())).once(); + + verify( + telemetry.trackEvent( + deepEqual({ + eventName: 'add_existing_integration', + properties: { integrationType: 'pgsql', outcome: 'completed' } + }) + ) + ).once(); + + assert.strictEqual(quickPickItems?.length, 1); + assert.strictEqual(quickPickItems?.[0].label, 'Shared Postgres'); + assert.strictEqual(quickPickItems?.[0].description, 'PostgreSQL'); + assert.strictEqual(quickPickItems?.[0].detail, `Used in: ${OTHER_PROJECT_ID}`); + }); + + test('shows an information message and writes nothing when no other project has a reusable integration', async () => { + onDiskOther = undefined; + + const outcome = await buildManager().addExistingIntegration(CURRENT_URI.toString()); + + assert.strictEqual(outcome, 'completed'); + assert.strictEqual(writes.size, 0); + assert.deepStrictEqual(cacheUpdates, []); + assert.isTrue(refreshSpy.notCalled); + verify(mockedVSCodeNamespaces.window.showQuickPick(anything(), anything())).never(); + verify(mockedVSCodeNamespaces.window.showInformationMessage(anything())).once(); + }); + + test('does not offer an integration the current project already has', async () => { + currentProject = projectFile(CURRENT_PROJECT_ID, CURRENT_NOTEBOOK_ID, [ + { id: SHARED_CONFIG.id, name: SHARED_CONFIG.name, type: SHARED_CONFIG.type } + ]); + + await buildManager().addExistingIntegration(CURRENT_URI.toString()); + + assert.strictEqual(writes.size, 0); + verify(mockedVSCodeNamespaces.window.showQuickPick(anything(), anything())).never(); + }); + + test('warns and offers nothing when the only candidate is declared with a type the stored config does not match', async () => { + onDiskOther = projectFile(OTHER_PROJECT_ID, 'notebook-other', [ + { id: SHARED_CONFIG.id, name: SHARED_CONFIG.name, type: 'mysql' } + ]); + + await buildManager().addExistingIntegration(CURRENT_URI.toString()); + + assert.strictEqual(writes.size, 0); + verify(mockedVSCodeNamespaces.window.showWarningMessage(anything())).once(); + verify(mockedVSCodeNamespaces.window.showQuickPick(anything(), anything())).never(); + }); + + test('returns cancelled and writes nothing when the picker is dismissed', async () => { + when(mockedVSCodeNamespaces.window.showQuickPick(anything(), anything())).thenResolve(undefined); + + const outcome = await buildManager().addExistingIntegration(CURRENT_URI.toString()); + + assert.strictEqual(outcome, 'cancelled'); + assert.strictEqual(writes.size, 0); + assert.isTrue(refreshSpy.notCalled); + verify(telemetry.trackEvent(anything())).never(); + }); + + test('fails without a Deepnote notebook to act on', async () => { + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([]); + + const outcome = await buildManager().addExistingIntegration(Uri.file('/ws/missing.deepnote').toString()); + + assert.strictEqual(outcome, 'failed'); + verify(mockedVSCodeNamespaces.window.showErrorMessage(anything())).once(); + }); +}); diff --git a/src/notebooks/deepnote/integrations/integrationWebview.ts b/src/notebooks/deepnote/integrations/integrationWebview.ts index e47249afc4..0798a363ee 100644 --- a/src/notebooks/deepnote/integrations/integrationWebview.ts +++ b/src/notebooks/deepnote/integrations/integrationWebview.ts @@ -171,6 +171,7 @@ export class IntegrationWebviewProvider implements IIntegrationWebviewProvider { integrationsConfirmDeleteDetails: localize.Integrations.confirmDeleteDetails, integrationsConfigureTitle: localize.Integrations.configureTitle, integrationsAddNewIntegration: localize.Integrations.addNewIntegration, + integrationsAddExistingIntegration: localize.Integrations.addExistingIntegration, integrationsDatabase: localize.Integrations.database, integrationsDataWarehousesLakes: localize.Integrations.dataWarehousesLakes, integrationsDatabases: localize.Integrations.databases, @@ -744,6 +745,16 @@ export class IntegrationWebviewProvider implements IIntegrationWebviewProvider { await this.signOutIntegration(message.integrationId); } break; + case 'addExisting': + // The command owns the QuickPick and the roster write, and re-shows this panel with the new roster. + try { + await commands.executeCommand(Commands.AddExistingIntegration, { + notebookUri: this.activeFileUri?.toString() + }); + } catch (error) { + logger.error('IntegrationWebviewProvider: AddExistingIntegration command failed', error); + } + break; case 'authenticate': if (message.integrationId) { const integrationType = this.integrations.get(message.integrationId)?.integrationType; diff --git a/src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts b/src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts index 903759ea99..149f744670 100644 --- a/src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts +++ b/src/notebooks/deepnote/integrations/integrationWebview.unit.test.ts @@ -517,6 +517,25 @@ suite('IntegrationWebviewProvider', () => { ); }); + test('handleMessage: "addExisting" → executeCommand(AddExistingIntegration, { notebookUri }) for the active file', async () => { + const executeCommandStub = sinon.stub().resolves(undefined); + when(mockedVSCodeNamespaces.commands.executeCommand(anyString(), anything())).thenCall((command, arg) => + executeCommandStub(command, arg) + ); + + const provider = buildProvider(); + await show(provider, new Map()); + + await fakePanel.onDidReceiveMessage({ type: 'addExisting' }); + + assert.isTrue( + executeCommandStub.calledOnceWithExactly(Commands.AddExistingIntegration, { + notebookUri: ACTIVE_FILE_URI.toString() + }), + "expected the command to receive the panel's active notebook so the picker targets the same project" + ); + }); + suite('handleMessage: "authenticate" telemetry outcome', () => { async function authenticate(commandResult: Promise): Promise { when(mockedVSCodeNamespaces.commands.executeCommand(anyString(), anything(), anything())).thenReturn( diff --git a/src/platform/analytics/types.ts b/src/platform/analytics/types.ts index e97288dc2e..99c92d10f8 100644 --- a/src/platform/analytics/types.ts +++ b/src/platform/analytics/types.ts @@ -12,6 +12,7 @@ export type TelemetryEventName = | 'delete_integration' | 'delete_notebook' | 'duplicate_notebook' + | 'add_existing_integration' | 'execute_cell' | 'execute_notebook' | 'export_notebook' @@ -57,6 +58,8 @@ export interface TelemetryEventProperties { * No `outcome`: nothing here is a command the user waits on — there is no progress UI and no cancel, so * `'cancelled'` is unreachable and a partial pass has no defensible single value. The counts carry it. */ + /** A SecretStorage integration from another project in the workspace was linked into this project's roster. */ + add_existing_integration: { integrationType: string; outcome: CommandOutcome }; refresh_integration_env: { /** Notebooks the refresh ran over, including ones skipped for having no started kernel. */ attemptedCount: number; diff --git a/src/platform/common/constants.ts b/src/platform/common/constants.ts index f840ff1aa5..2d5273ebb3 100644 --- a/src/platform/common/constants.ts +++ b/src/platform/common/constants.ts @@ -225,6 +225,7 @@ export namespace Commands { export const CopyNotebookDetails = 'deepnote.copyNotebookDetails'; export const EnableSnapshots = 'deepnote.enableSnapshots'; export const DisableSnapshots = 'deepnote.disableSnapshots'; + export const AddExistingIntegration = 'deepnote.addExistingIntegration'; export const AuthenticateIntegration = 'deepnote.authenticateIntegration'; export const ManageIntegrations = 'deepnote.manageIntegrations'; export const AddAgentBlock = 'deepnote.addAgentBlock'; diff --git a/src/platform/common/utils/localize.ts b/src/platform/common/utils/localize.ts index 30d282fda7..68d2637486 100644 --- a/src/platform/common/utils/localize.ts +++ b/src/platform/common/utils/localize.ts @@ -837,6 +837,24 @@ export namespace Integrations { export const cancel = l10n.t('Cancel'); export const save = l10n.t('Save'); export const addNewIntegration = l10n.t('Add New Integration'); + export const addExistingIntegration = l10n.t('Add Existing Integration'); + export const addExistingIntegrationPlaceholder = l10n.t( + 'Select an integration configured in another project of this workspace' + ); + export const addExistingIntegrationNoneAvailable = l10n.t( + 'No integrations from other projects in this workspace are available to add. Integrations configured only in .deepnote.env.yaml already apply to every project under it.' + ); + export const addExistingIntegrationConflictsSkipped = (count: number) => + l10n.t( + '{0} integration(s) were skipped because another project declares them with a different type than the stored configuration.', + count + ); + export const addExistingIntegrationSucceeded = (integrationName: string) => + l10n.t('Added integration "{0}" to this project.', integrationName); + export const addExistingIntegrationFailed = l10n.t( + 'Failed to add the integration to the notebook file. Please try again.' + ); + export const addExistingIntegrationUsedIn = (projectNames: string) => l10n.t('Used in: {0}', projectNames); export const database = l10n.t('Database'); export const dataWarehousesLakes = l10n.t('Data Warehouses & Lakes'); export const databases = l10n.t('Databases'); diff --git a/src/webviews/webview-side/integrations/IntegrationPanel.tsx b/src/webviews/webview-side/integrations/IntegrationPanel.tsx index 9c92ec19a6..d17bf5826c 100644 --- a/src/webviews/webview-side/integrations/IntegrationPanel.tsx +++ b/src/webviews/webview-side/integrations/IntegrationPanel.tsx @@ -212,6 +212,10 @@ export const IntegrationPanel: React.FC = ({ baseTheme, setSelectedIntegrationType(undefined); }; + const handleAddExisting = () => { + postOutbound({ type: 'addExisting' }); + }; + const handleSelectIntegrationType = (type: ConfigurableDatabaseIntegrationType) => { // Generate a new UUID for the integration const newId = generateUuid(); @@ -239,7 +243,7 @@ export const IntegrationPanel: React.FC = ({ baseTheme, onSignOut={handleSignOut} /> - + {selectedIntegrationId && selectedIntegrationType && ( void; onSelectType: (type: ConfigurableDatabaseIntegrationType) => void; } @@ -111,10 +113,15 @@ const DATABASE_INTEGRATION_TYPES: IntegrationTypeInfo[] = [ } ]; -export const IntegrationTypeSelector: React.FC = ({ onSelectType }) => { +export const IntegrationTypeSelector: React.FC = ({ onAddExisting, onSelectType }) => { return (
-

{getLocString('integrationsAddNewIntegration', 'Add New Integration')}

+
+

{getLocString('integrationsAddNewIntegration', 'Add New Integration')}

+ +

diff --git a/src/webviews/webview-side/integrations/integrations.css b/src/webviews/webview-side/integrations/integrations.css index 2ade190f93..02e691ead3 100644 --- a/src/webviews/webview-side/integrations/integrations.css +++ b/src/webviews/webview-side/integrations/integrations.css @@ -329,9 +329,16 @@ form { border-top: 1px solid var(--vscode-panel-border); } -.integration-type-selector h2 { - margin-top: 0; +.integration-type-selector-header { + display: flex; + align-items: center; + justify-content: space-between; + gap: 16px; margin-bottom: 24px; +} + +.integration-type-selector h2 { + margin: 0; font-size: 1.2em; font-weight: 600; } diff --git a/src/webviews/webview-side/integrations/types.ts b/src/webviews/webview-side/integrations/types.ts index 3809658ed8..31978089f6 100644 --- a/src/webviews/webview-side/integrations/types.ts +++ b/src/webviews/webview-side/integrations/types.ts @@ -62,4 +62,5 @@ export type WebviewOutboundMessage = | { type: 'reset'; integrationId: string } | { type: 'delete'; integrationId: string } | { type: 'signOut'; integrationId: string } + | { type: 'addExisting' } | AuthenticateMessage; diff --git a/test/e2e/suite/workspace/integrations.e2e.test.ts b/test/e2e/suite/workspace/integrations.e2e.test.ts index 22f903aae4..733fe1c5fb 100644 --- a/test/e2e/suite/workspace/integrations.e2e.test.ts +++ b/test/e2e/suite/workspace/integrations.e2e.test.ts @@ -24,6 +24,9 @@ const WEBVIEW_READ_TIMEOUT = 15_000; // Empty-state text asserted to prove the panel actually opened (else the negative `not.contain` // below passes trivially against a blank/failed `''` read). const NO_INTEGRATIONS_TEXT = 'No integrations found in this project.'; +// The "Add Existing Integration" entry point (reuse an integration another project configured) must render in +// the panel regardless of whether this project has integrations of its own. +const ADD_EXISTING_INTEGRATION_TEXT = 'Add Existing Integration'; // Prior editors finish closing before reopening the target notebook. const EDITORS_CLOSE_DELAY = 500; // Freshly opened notebook paints before we refocus it. @@ -126,5 +129,6 @@ describe('Deepnote — the integrations UI', function () { // Positive signal that the panel rendered (a blank read would make `not.contain` pass trivially). expect(text, 'integrations webview text').to.contain(NO_INTEGRATIONS_TEXT); expect(text, 'integrations webview text').to.not.contain(INTEGRATION_NAME); + expect(text, 'integrations webview text').to.contain(ADD_EXISTING_INTEGRATION_TEXT); }); }); From 2197dbe7dbc92e8f3b54cb68ee4e1c1eccdf0350 Mon Sep 17 00:00:00 2001 From: James Hobbs Date: Thu, 10 Sep 2026 14:56:22 +0100 Subject: [PATCH 2/4] fix(integrations): keep the full project roster when attaching an existing integration `getCachedRoster` narrowed the cached roster to the types the panel can manage and `attachExistingIntegration` persisted that narrowed array, so linking an integration silently dropped `pandas-dataframe` (and any type this build does not know) from the project's integrations. The roster now passes through verbatim, following the same cast-not-narrow pattern `SqlCellStatusBarProvider.addToProjectIntegrations` already uses, and only the picker's exclusion set is derived from it. Adds a picker test and a manager test asserting a `pandas-dataframe` entry survives the attach in both the cache update and the written file. Co-Authored-By: Claude Fable 5.1 --- .../integrations/existingIntegrationPicker.ts | 20 ++++++++++---- .../existingIntegrationPicker.unit.test.ts | 26 +++++++++++++++++++ .../integrations/integrationManager.ts | 21 +++++++-------- .../integrationManager.unit.test.ts | 21 ++++++++++++++- 4 files changed, 70 insertions(+), 18 deletions(-) diff --git a/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts b/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts index 215ca7372a..478ec936a7 100644 --- a/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts +++ b/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts @@ -1,3 +1,4 @@ +import type { DeepnoteFile } from '@deepnote/blocks'; import { RelativePattern, Uri, workspace } from 'vscode'; import * as localize from '../../../platform/common/utils/localize'; @@ -38,6 +39,9 @@ export function integrationTypeLabel(type: ConfigurableDatabaseIntegrationType): return INTEGRATION_TYPE_LABELS[type] ?? type; } +/** A roster entry exactly as the `.deepnote` file records it; `type` is not narrowed to the types this build knows. */ +export type RawProjectIntegration = NonNullable[number]; + /** * A SecretStorage integration declared by at least one *other* project in the workspace, so it can be linked into * the current project without re-entering credentials. @@ -71,8 +75,11 @@ export interface CollectReusableIntegrationsResult { export interface AttachExistingIntegrationParams { activeFileUri: Uri; - /** The current project's roster as cached by the notebook manager; the new entry is appended to it. */ - currentIntegrations: readonly ProjectIntegration[]; + /** + * The current project's roster exactly as cached by the notebook manager. Every entry passes through to the + * write verbatim (including `pandas-dataframe` and any type this build does not know), so nothing is pruned. + */ + currentIntegrations: readonly RawProjectIntegration[]; integration: ReusableIntegration; notebookManager: IDeepnoteNotebookManager; projectId: string; @@ -195,10 +202,13 @@ export async function collectReusableIntegrations( export function attachExistingIntegration(params: AttachExistingIntegrationParams): Promise { const { activeFileUri, currentIntegrations, integration, notebookManager, projectId } = params; - const integrations: ProjectIntegration[] = [ + const linked: ProjectIntegration = { id: integration.id, name: integration.name, type: integration.type }; + // Cast rather than narrow: validating the existing entries would silently drop any type this build does not + // know about (`pandas-dataframe` included), which is pruning by another name. + const integrations = [ ...currentIntegrations.filter((entry) => entry.id !== integration.id), - { id: integration.id, name: integration.name, type: integration.type } - ]; + linked + ] as ProjectIntegration[]; return persistProjectIntegrations({ activeFileUri, integrations, notebookManager, projectId }); } diff --git a/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts b/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts index 283c64641a..212a117029 100644 --- a/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts +++ b/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts @@ -11,6 +11,7 @@ import { attachExistingIntegration, collectReusableIntegrations, integrationTypeLabel, + RawProjectIntegration, ReusableIntegration } from './existingIntegrationPicker'; import { buildGoogleOauthIntegration, buildPostgresIntegration } from './federatedAuth/federatedAuthTestHelpers'; @@ -314,6 +315,31 @@ suite('existingIntegrationPicker', () => { assert.deepStrictEqual(writes.get(activeUri.fsPath)?.project.integrations, expectedRoster); }); + test('passes roster entries of types it cannot manage (e.g. pandas-dataframe) through verbatim', async () => { + const { writes } = stubWorkspace({ + projects: [{ uri: activeUri, projectId: CURRENT_PROJECT_ID }] + }); + const currentIntegrations: RawProjectIntegration[] = [ + { id: 'duckdb', name: 'DuckDB', type: 'pandas-dataframe' }, + { id: 'future', name: 'Unknown to this build', type: 'some-future-type' } + ]; + + await attachExistingIntegration({ + activeFileUri: activeUri, + currentIntegrations, + integration: shared, + notebookManager, + projectId: CURRENT_PROJECT_ID + }); + + const expectedRoster = [ + ...currentIntegrations, + { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' } + ]; + assert.deepStrictEqual(cacheUpdates, [{ projectId: CURRENT_PROJECT_ID, integrations: expectedRoster }]); + assert.deepStrictEqual(writes.get(activeUri.fsPath)?.project.integrations, expectedRoster); + }); + test('replaces rather than duplicates an entry whose id is already on the roster', async () => { const { writes } = stubWorkspace({ projects: [{ uri: activeUri, projectId: CURRENT_PROJECT_ID }] diff --git a/src/notebooks/deepnote/integrations/integrationManager.ts b/src/notebooks/deepnote/integrations/integrationManager.ts index fd1f56b379..44b1dd8c62 100644 --- a/src/notebooks/deepnote/integrations/integrationManager.ts +++ b/src/notebooks/deepnote/integrations/integrationManager.ts @@ -6,7 +6,6 @@ import { IExtensionContext } from '../../../platform/common/types'; import { Commands } from '../../../platform/common/constants'; import * as localize from '../../../platform/common/utils/localize'; import { logger } from '../../../platform/logging'; -import { isConfigurableDatabaseIntegrationType } from '../../../platform/notebooks/deepnote/integrationTypes'; import { IIntegrationDetector, IIntegrationEnvLiveRefresher, @@ -14,12 +13,13 @@ import { IIntegrationStorage, IIntegrationWebviewProvider } from './types'; -import { IDeepnoteNotebookManager, ProjectIntegration } from '../../types'; +import { IDeepnoteNotebookManager } from '../../types'; import { DatabaseIntegrationType, databaseIntegrationTypes } from '@deepnote/database-integrations'; import { attachExistingIntegration, collectReusableIntegrations, integrationTypeLabel, + RawProjectIntegration, ReusableIntegration } from './existingIntegrationPicker'; @@ -207,18 +207,15 @@ export class IntegrationManager implements IIntegrationManager { return uri ? String(uri) : undefined; } - /** The project's roster as the notebook manager caches it, narrowed to the types the panel can manage. */ - private getCachedRoster(projectId: string, notebookId: string): ProjectIntegration[] { + /** + * The project's roster exactly as the notebook manager caches it. Entries are never filtered here: this array + * is what `attachExistingIntegration` persists, so narrowing it (e.g. dropping `pandas-dataframe`) would + * rewrite the project's integrations rather than add to them. Callers derive their own exclusion set from it. + */ + private getCachedRoster(projectId: string, notebookId: string): RawProjectIntegration[] { const project = this.notebookManager.getProjectForNotebook(projectId, notebookId); - const roster: ProjectIntegration[] = []; - - for (const entry of project?.project.integrations ?? []) { - if (isConfigurableDatabaseIntegrationType(entry.type)) { - roster.push({ id: entry.id, name: entry.name, type: entry.type }); - } - } - return roster; + return [...(project?.project.integrations ?? [])]; } /** Re-applies integration env in the project's running kernels and re-renders the panel with the new roster. */ diff --git a/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts b/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts index 9b15dcf3a4..48f56ab156 100644 --- a/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts +++ b/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts @@ -16,6 +16,7 @@ import { createWorkspaceFolder } from '../deepnoteTestHelpers'; import { buildPostgresIntegration } from './federatedAuth/federatedAuthTestHelpers'; +import { RawProjectIntegration } from './existingIntegrationPicker'; import { IntegrationManager } from './integrationManager'; import { IIntegrationDetector, @@ -32,7 +33,7 @@ const OTHER_URI = Uri.file('/ws/other.deepnote'); const SHARED_CONFIG = buildPostgresIntegration({ id: 'pg-shared', name: 'Shared Postgres' }); -function projectFile(projectId: string, notebookId: string, integrations: ProjectIntegration[]): DeepnoteFile { +function projectFile(projectId: string, notebookId: string, integrations: RawProjectIntegration[]): DeepnoteFile { return createDeepnoteFile({ project: createDeepnoteProject({ id: projectId, @@ -194,6 +195,24 @@ suite('IntegrationManager.addExistingIntegration', () => { verify(mockedVSCodeNamespaces.window.showInformationMessage(anything())).once(); }); + test('keeps roster entries the panel cannot manage (pandas-dataframe) when attaching', async () => { + currentProject = projectFile(CURRENT_PROJECT_ID, CURRENT_NOTEBOOK_ID, [ + { id: 'duckdb', name: 'DuckDB', type: 'pandas-dataframe' } + ]); + + const outcome = await buildManager().addExistingIntegration(CURRENT_URI.toString()); + + assert.strictEqual(outcome, 'completed'); + + const expectedRoster = [ + { id: 'duckdb', name: 'DuckDB', type: 'pandas-dataframe' }, + { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' } + ]; + assert.deepStrictEqual(writes.get(CURRENT_URI.fsPath)?.project.integrations, expectedRoster); + assert.deepStrictEqual(cacheUpdates, [expectedRoster]); + assert.strictEqual(quickPickItems?.length, 1, 'the DuckDB entry is neither offered nor a candidate'); + }); + test('does not offer an integration the current project already has', async () => { currentProject = projectFile(CURRENT_PROJECT_ID, CURRENT_NOTEBOOK_ID, [ { id: SHARED_CONFIG.id, name: SHARED_CONFIG.name, type: SHARED_CONFIG.type } From 14299d52e7ceb063432a6d53097189038c61c942 Mon Sep 17 00:00:00 2001 From: tomas Date: Fri, 18 Sep 2026 11:53:18 +0000 Subject: [PATCH 3/4] chore(integrations): trim comments to carry why, not what Drops comments that restated the code they sat on, condenses the docstrings that explained storage internals at tutorial length, and states the "reuse is a link, not a copy" rationale once in collectReusableIntegrations instead of in three places. Also moves add_existing_integration to its alphabetical slot in TelemetryEventProperties. It had been inserted between the "No `outcome`: ..." docblock and refresh_integration_env, so that block read as documenting an event that does carry an outcome. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NjAiQLqjuPiZmN9GiTXCfd --- .../integrations/existingIntegrationPicker.ts | 51 +++++++------------ .../integrations/integrationManager.ts | 18 ++----- .../integrations/integrationWebview.ts | 2 +- src/platform/analytics/types.ts | 4 +- .../integrations/IntegrationTypeSelector.tsx | 2 +- .../suite/workspace/integrations.e2e.test.ts | 3 +- 6 files changed, 29 insertions(+), 51 deletions(-) diff --git a/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts b/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts index 478ec936a7..c87f2d559c 100644 --- a/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts +++ b/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts @@ -13,7 +13,7 @@ import { isSnapshotFile } from '../snapshots/snapshotFiles'; import { PersistIntegrationsResult, persistProjectIntegrations } from './projectIntegrationsWriter'; import { IIntegrationStorage } from './types'; -/** Human-readable type labels for the picker; mirrors `integrationTypeLabels` in the webview bundle. */ +/** Mirrors `integrationTypeLabels` in the webview bundle. */ const INTEGRATION_TYPE_LABELS: Record = { alloydb: localize.Integrations.alloyDBTypeLabel, athena: localize.Integrations.athenaTypeLabel, @@ -39,24 +39,21 @@ export function integrationTypeLabel(type: ConfigurableDatabaseIntegrationType): return INTEGRATION_TYPE_LABELS[type] ?? type; } -/** A roster entry exactly as the `.deepnote` file records it; `type` is not narrowed to the types this build knows. */ +/** A roster entry as recorded on disk: unlike `ProjectIntegration`, `type` is not narrowed to the known types. */ export type RawProjectIntegration = NonNullable[number]; -/** - * A SecretStorage integration declared by at least one *other* project in the workspace, so it can be linked into - * the current project without re-entering credentials. - */ +/** An integration another project in the workspace has credentials stored for, so linking it needs no re-entry. */ export interface ReusableIntegration { id: string; /** Name from the stored config — the same source the panel writes to the roster on save. */ name: string; - /** Display names of the other projects whose roster declares this integration; deduped and sorted. */ + /** The other projects declaring this integration; deduped and sorted. */ projectNames: string[]; type: ConfigurableDatabaseIntegrationType; } export interface CollectReusableIntegrationsParams { - /** Integration ids already on the current project's roster; never offered again. */ + /** Ids already on the current project's roster. */ excludeIntegrationIds: ReadonlySet; integrationStorage: IIntegrationStorage; /** The project being extended; its own `.deepnote` files are skipped. */ @@ -65,9 +62,8 @@ export interface CollectReusableIntegrationsParams { export interface CollectReusableIntegrationsResult { /** - * Ids skipped because a project's roster declares the integration with a type that differs from the stored - * configuration. Linking such an entry would put a roster type on this project that the credentials cannot - * back, so the caller warns instead. + * Ids skipped because some project's roster type disagrees with the stored config: linking one would put a type + * on this project that the credentials cannot back. */ conflictingIds: string[]; integrations: ReusableIntegration[]; @@ -75,10 +71,7 @@ export interface CollectReusableIntegrationsResult { export interface AttachExistingIntegrationParams { activeFileUri: Uri; - /** - * The current project's roster exactly as cached by the notebook manager. Every entry passes through to the - * write verbatim (including `pandas-dataframe` and any type this build does not know), so nothing is pruned. - */ + /** The project's full roster: every entry is written back verbatim, so a filtered array drops entries. */ currentIntegrations: readonly RawProjectIntegration[]; integration: ReusableIntegration; notebookManager: IDeepnoteNotebookManager; @@ -86,18 +79,13 @@ export interface AttachExistingIntegrationParams { } /** - * Scans every `.deepnote` file in the open workspace folders and collects the SecretStorage integrations other - * projects declare. + * Scans every `.deepnote` file in the open workspace folders for integrations other projects declare. * - * Storage design: `IntegrationStorage` keys configs by integration id alone (there is no per-project namespace), - * and both the env-var provider and the detector resolve credentials from the project roster - * (`project.integrations[].id`). The roster entry is therefore the only thing that "attaches" an integration to a - * project, and reusing one is a pure link: no config is copied. Federated (`google-oauth`) integrations are - * included for the same reason — `FederatedAuthTokenStorage` is also keyed by integration id, and the per-cell - * code generator resolves the config through the roster of the notebook being run. + * Reuse is a link, not a copy: `IntegrationStorage` and `FederatedAuthTokenStorage` both key configs by integration + * id alone, so the roster entry is the only thing that scopes an integration to a project. * - * Integrations configured only in `.deepnote.env.yaml` (no stored config) are not offered: that file already - * applies to every project under it, and the panel cannot write that layer. + * Integrations configured only in `.deepnote.env.yaml` are not offered: that file already applies to every project + * under it, and the panel cannot write that layer. */ export async function collectReusableIntegrations( params: CollectReusableIntegrationsParams @@ -128,7 +116,7 @@ export async function collectReusableIntegrations( visited.add(key); - // Per-file try/catch: one unreadable file must not hide every other project's integrations. + // One unreadable file must not hide every other project's integrations. try { const projectData = await readDeepnoteProjectFile(fileUri); @@ -146,7 +134,7 @@ export async function collectReusableIntegrations( const storedConfig = await integrationStorage.getIntegrationConfig(entry.id); if (!storedConfig) { - // File-only or never-configured: there are no credentials in SecretStorage to reuse. + // File-only or never configured — no stored credentials to reuse. continue; } @@ -195,16 +183,15 @@ export async function collectReusableIntegrations( } /** - * Links `integration` into the project's roster and persists it through the same writer the panel uses, so the - * cache, the active file and every sibling `.deepnote` file of the project are updated together. Idempotent for an - * id already on the roster (the entry is replaced, not duplicated). + * Links `integration` into the project's roster through the same writer the panel uses, so the cache, the active + * file and every sibling `.deepnote` file are updated together. Re-linking an id already there replaces its entry. */ export function attachExistingIntegration(params: AttachExistingIntegrationParams): Promise { const { activeFileUri, currentIntegrations, integration, notebookManager, projectId } = params; const linked: ProjectIntegration = { id: integration.id, name: integration.name, type: integration.type }; - // Cast rather than narrow: validating the existing entries would silently drop any type this build does not - // know about (`pandas-dataframe` included), which is pruning by another name. + // Cast rather than validate: filtering out types this build does not know (`pandas-dataframe`) would delete + // them from the file. const integrations = [ ...currentIntegrations.filter((entry) => entry.id !== integration.id), linked diff --git a/src/notebooks/deepnote/integrations/integrationManager.ts b/src/notebooks/deepnote/integrations/integrationManager.ts index 44b1dd8c62..004d41db22 100644 --- a/src/notebooks/deepnote/integrations/integrationManager.ts +++ b/src/notebooks/deepnote/integrations/integrationManager.ts @@ -73,8 +73,7 @@ export class IntegrationManager implements IIntegrationManager { }) ); - // Links an integration another project in the workspace already configured into the active project. - // Takes the same argument shapes as ManageIntegrations so the panel and menus can pass a notebook URI. + // Accepts the same argument shapes as ManageIntegrations so the panel and menus can pass a notebook URI. this.extensionContext.subscriptions.push( commands.registerCommand(Commands.AddExistingIntegration, (...args: unknown[]) => { let notebookUri: string | undefined; @@ -89,11 +88,9 @@ export class IntegrationManager implements IIntegrationManager { } /** - * Offers the SecretStorage integrations other projects in the workspace declare, and links the chosen one into - * the active project's roster. Public so tests can drive it without `commands.executeCommand`. - * - * Credentials are not copied: `IntegrationStorage` is keyed by integration id alone, so the roster entry is all - * that scopes an integration to a project (see `collectReusableIntegrations`). + * Offers the integrations other projects in the workspace declare and links the chosen one into this project's + * roster; no credentials are copied (see `collectReusableIntegrations`). Public so tests can drive it without + * `commands.executeCommand`. */ public async addExistingIntegration(notebookUri?: string): Promise { const activeNotebook = this.resolveDeepnoteNotebook(notebookUri); @@ -207,18 +204,13 @@ export class IntegrationManager implements IIntegrationManager { return uri ? String(uri) : undefined; } - /** - * The project's roster exactly as the notebook manager caches it. Entries are never filtered here: this array - * is what `attachExistingIntegration` persists, so narrowing it (e.g. dropping `pandas-dataframe`) would - * rewrite the project's integrations rather than add to them. Callers derive their own exclusion set from it. - */ + /** Unfiltered on purpose: `attachExistingIntegration` writes this array back, so anything dropped here is lost. */ private getCachedRoster(projectId: string, notebookId: string): RawProjectIntegration[] { const project = this.notebookManager.getProjectForNotebook(projectId, notebookId); return [...(project?.project.integrations ?? [])]; } - /** Re-applies integration env in the project's running kernels and re-renders the panel with the new roster. */ private async refreshAfterRosterChange(projectId: string, activeNotebook: NotebookDocument): Promise { const projectNotebooks = workspace.notebookDocuments.filter( (notebook) => notebook.notebookType === 'deepnote' && notebook.metadata?.deepnoteProjectId === projectId diff --git a/src/notebooks/deepnote/integrations/integrationWebview.ts b/src/notebooks/deepnote/integrations/integrationWebview.ts index 0798a363ee..5a68229e88 100644 --- a/src/notebooks/deepnote/integrations/integrationWebview.ts +++ b/src/notebooks/deepnote/integrations/integrationWebview.ts @@ -746,7 +746,7 @@ export class IntegrationWebviewProvider implements IIntegrationWebviewProvider { } break; case 'addExisting': - // The command owns the QuickPick and the roster write, and re-shows this panel with the new roster. + // The command owns the picker and the roster write, and re-shows this panel when it is done. try { await commands.executeCommand(Commands.AddExistingIntegration, { notebookUri: this.activeFileUri?.toString() diff --git a/src/platform/analytics/types.ts b/src/platform/analytics/types.ts index 99c92d10f8..3aeb0c4c96 100644 --- a/src/platform/analytics/types.ts +++ b/src/platform/analytics/types.ts @@ -37,6 +37,8 @@ export type CommandOutcome = 'completed' | 'cancelled' | 'failed'; /** Caller-supplied properties per event; `undefined` means none beyond the common properties the service attaches. */ export interface TelemetryEventProperties { add_block: { blockType: string; isEphemeral: boolean }; + /** "Existing" means configured in another project of this workspace. */ + add_existing_integration: { integrationType: string; outcome: CommandOutcome }; authenticate_integration: { integrationType: string; outcome: CommandOutcome }; configure_integration: { integrationType: string }; copy_notebook_details: undefined; @@ -58,8 +60,6 @@ export interface TelemetryEventProperties { * No `outcome`: nothing here is a command the user waits on — there is no progress UI and no cancel, so * `'cancelled'` is unreachable and a partial pass has no defensible single value. The counts carry it. */ - /** A SecretStorage integration from another project in the workspace was linked into this project's roster. */ - add_existing_integration: { integrationType: string; outcome: CommandOutcome }; refresh_integration_env: { /** Notebooks the refresh ran over, including ones skipped for having no started kernel. */ attemptedCount: number; diff --git a/src/webviews/webview-side/integrations/IntegrationTypeSelector.tsx b/src/webviews/webview-side/integrations/IntegrationTypeSelector.tsx index 078777fcae..f62d5b409d 100644 --- a/src/webviews/webview-side/integrations/IntegrationTypeSelector.tsx +++ b/src/webviews/webview-side/integrations/IntegrationTypeSelector.tsx @@ -4,7 +4,7 @@ import { ConfigurableDatabaseIntegrationType } from './types'; import { integrationTypeLabels, integrationTypeIcons } from './integrationUtils'; export interface IIntegrationTypeSelectorProps { - /** Opens the extension-side picker of integrations other projects in the workspace already configured. */ + /** Opens the extension-host picker of integrations other projects already configured. */ onAddExisting: () => void; onSelectType: (type: ConfigurableDatabaseIntegrationType) => void; } diff --git a/test/e2e/suite/workspace/integrations.e2e.test.ts b/test/e2e/suite/workspace/integrations.e2e.test.ts index 733fe1c5fb..0256ae637e 100644 --- a/test/e2e/suite/workspace/integrations.e2e.test.ts +++ b/test/e2e/suite/workspace/integrations.e2e.test.ts @@ -24,8 +24,7 @@ const WEBVIEW_READ_TIMEOUT = 15_000; // Empty-state text asserted to prove the panel actually opened (else the negative `not.contain` // below passes trivially against a blank/failed `''` read). const NO_INTEGRATIONS_TEXT = 'No integrations found in this project.'; -// The "Add Existing Integration" entry point (reuse an integration another project configured) must render in -// the panel regardless of whether this project has integrations of its own. +// The reuse entry point must render even when this project has no integrations of its own. const ADD_EXISTING_INTEGRATION_TEXT = 'Add Existing Integration'; // Prior editors finish closing before reopening the target notebook. const EDITORS_CLOSE_DELAY = 500; From a0f9dcae61de69ea021fbec549c6badd1ea6ef3c Mon Sep 17 00:00:00 2001 From: tomas Date: Mon, 21 Sep 2026 15:07:52 +0000 Subject: [PATCH 4/4] fix(integrations): close data-loss windows in add existing integration Addresses the review of this branch. Correctness: - Re-read the project's integrations after the QuickPick instead of writing back a snapshot taken before it. The file watcher replaces the cached project on an integrations-only external write without any UI event, so the pre-pick array could be stale, and the writer stamps whatever it is given onto every .deepnote file of the project. - Refuse the command on a *.snapshot.deepnote file. Snapshots match the notebook selector and the writer skips them, so the command reported a failure only after it had already updated the cache. - getCachedProjectIntegrations returns undefined on a cache miss rather than an empty array, so "not cached" can no longer be written back as "no integrations at all". - Make the workspace scan cancellable: window.withProgress plumbs a token into collectReusableIntegrations, findFiles and both scan loops. Types: - Widen the write path to RawProjectIntegration, now defined once in notebooks/types.ts. This removes both `as ProjectIntegration[]` assertions and two duplicate local declarations of the type. Narrowing inside the writer is now a compile error rather than a silently exhaustive switch. Naming: - "roster" becomes "project integrations" throughout. In the SQL status bar the narrower and wider lists are now projectIntegrations and selectableIntegrations, matching getSelectableIntegrations. Localization: - Integration type labels were written out in seven places, one of which (the webview map) was never localized at all. They now live only in localize.Integrations.typeLabels, keyed by a bundle key derived from the integration type so there is no second list to keep in sync. Drops the dead integrationsDuckDBTypeLabel. Tests: - Cover every failure branch of addExistingIntegration, plus the stale re-read, the snapshot guard, the cache miss and both cancellation paths. Each new test was verified to fail against the unfixed code. - Type refreshSpy off IIntegrationEnvLiveRefresher so a signature change fails the compile, not just the runtime assertion. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Ns2d146xs51a6E8LeDgJQS --- src/messageTypes.ts | 23 +- .../deepnote/deepnoteNotebookManager.ts | 4 +- .../integrations/existingIntegrationPicker.ts | 79 +++---- .../existingIntegrationPicker.unit.test.ts | 106 +++++++--- .../integrations/integrationDetector.ts | 15 +- .../integrations/integrationManager.ts | 82 ++++++-- .../integrationManager.unit.test.ts | 198 ++++++++++++++++-- .../integrations/integrationWebview.ts | 22 +- .../integrations/projectIntegrationsWriter.ts | 6 +- .../deepnote/sqlCellStatusBarProvider.ts | 75 +++---- .../sqlCellStatusBarProvider.unit.test.ts | 4 +- src/notebooks/types.ts | 8 +- src/platform/analytics/types.ts | 3 +- src/platform/common/utils/localize.ts | 52 +++-- .../deepnote/integrationTypeLabels.ts | 13 ++ .../integrationTypeLabels.unit.test.ts | 25 +++ .../integrations/ConfigurationForm.tsx | 4 +- .../integrations/IntegrationItem.tsx | 49 +---- .../integrations/IntegrationTypeSelector.tsx | 183 +++++----------- .../integrations/integrationUtils.ts | 31 +-- 20 files changed, 542 insertions(+), 440 deletions(-) create mode 100644 src/platform/notebooks/deepnote/integrationTypeLabels.ts create mode 100644 src/platform/notebooks/deepnote/integrationTypeLabels.unit.test.ts diff --git a/src/messageTypes.ts b/src/messageTypes.ts index 17974216e9..01fea32ffe 100644 --- a/src/messageTypes.ts +++ b/src/messageTypes.ts @@ -10,6 +10,7 @@ import { import { KernelSocketOptions } from './kernels/types'; import { IJupyterVariable, IJupyterVariablesRequest, IJupyterVariablesResponse } from './kernels/variables/types'; import { WidgetScriptSource } from './notebooks/controllers/ipywidgets/types'; +import type { IntegrationTypeLabelKey } from './platform/notebooks/deepnote/integrationTypeLabels'; export type NotifyIPyWidgetWidgetVersionNotSupportedAction = { moduleName: 'qgrid'; @@ -120,7 +121,7 @@ export enum SharedMessages { LocInit = 'loc_init' } -export type LocalizedMessages = { +export type LocalizedMessages = { [K in IntegrationTypeLabelKey]: string } & { collapseSingle: string; expandSingle: string; openExportFileYes: string; @@ -190,26 +191,6 @@ export type LocalizedMessages = { integrationsDatabase: string; integrationsDataWarehousesLakes: string; integrationsDatabases: string; - // Integration type labels - integrationsPostgresTypeLabel: string; - integrationsBigQueryTypeLabel: string; - integrationsSnowflakeTypeLabel: string; - integrationsAlloyDBTypeLabel: string; - integrationsAthenaTypeLabel: string; - integrationsClickHouseTypeLabel: string; - integrationsCloudSqlTypeLabel: string; - integrationsDatabricksTypeLabel: string; - integrationsDremioTypeLabel: string; - integrationsMariaDBTypeLabel: string; - integrationsMaterializeTypeLabel: string; - integrationsMindsDBTypeLabel: string; - integrationsMongoDBTypeLabel: string; - integrationsMySQLTypeLabel: string; - integrationsDuckDBTypeLabel: string; - integrationsRedshiftTypeLabel: string; - integrationsSpannerTypeLabel: string; - integrationsSQLServerTypeLabel: string; - integrationsTrinoTypeLabel: string; // PostgreSQL form strings integrationsPostgresNameLabel: string; integrationsPostgresNamePlaceholder: string; diff --git a/src/notebooks/deepnote/deepnoteNotebookManager.ts b/src/notebooks/deepnote/deepnoteNotebookManager.ts index 3772f20098..1c43874f7c 100644 --- a/src/notebooks/deepnote/deepnoteNotebookManager.ts +++ b/src/notebooks/deepnote/deepnoteNotebookManager.ts @@ -1,7 +1,7 @@ import { injectable } from 'inversify'; import type { DeepnoteFile } from '@deepnote/blocks'; -import { IDeepnoteNotebookManager, ProjectIntegration } from '../types'; +import { IDeepnoteNotebookManager, RawProjectIntegration } from '../types'; /** * Centralized manager for tracking Deepnote notebook selections and project state. @@ -40,7 +40,7 @@ export class DeepnoteNotebookManager implements IDeepnoteNotebookManager { * Updates the integrations list across every cached notebook entry under the project (cache-only). * @returns `true` if at least one cached entry was updated, `false` otherwise. */ - updateProjectIntegrations(projectId: string, integrations: ProjectIntegration[]): boolean { + updateProjectIntegrations(projectId: string, integrations: RawProjectIntegration[]): boolean { const notebookEntries = this.originalProjects.get(projectId); if (!notebookEntries || notebookEntries.size === 0) { diff --git a/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts b/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts index c87f2d559c..ec207d9a8e 100644 --- a/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts +++ b/src/notebooks/deepnote/integrations/existingIntegrationPicker.ts @@ -1,51 +1,20 @@ -import type { DeepnoteFile } from '@deepnote/blocks'; -import { RelativePattern, Uri, workspace } from 'vscode'; +import { CancellationToken, RelativePattern, Uri, workspace } from 'vscode'; -import * as localize from '../../../platform/common/utils/localize'; import { readDeepnoteProjectFile } from '../../../platform/deepnote/deepnoteProjectFileReader'; import { logger } from '../../../platform/logging'; import { ConfigurableDatabaseIntegrationType, isConfigurableDatabaseIntegrationType } from '../../../platform/notebooks/deepnote/integrationTypes'; -import { IDeepnoteNotebookManager, ProjectIntegration } from '../../types'; +import { IDeepnoteNotebookManager, ProjectIntegration, RawProjectIntegration } from '../../types'; import { isSnapshotFile } from '../snapshots/snapshotFiles'; import { PersistIntegrationsResult, persistProjectIntegrations } from './projectIntegrationsWriter'; import { IIntegrationStorage } from './types'; -/** Mirrors `integrationTypeLabels` in the webview bundle. */ -const INTEGRATION_TYPE_LABELS: Record = { - alloydb: localize.Integrations.alloyDBTypeLabel, - athena: localize.Integrations.athenaTypeLabel, - 'big-query': localize.Integrations.bigQueryTypeLabel, - clickhouse: localize.Integrations.clickHouseTypeLabel, - 'cloud-sql': localize.Integrations.cloudSqlTypeLabel, - databricks: localize.Integrations.databricksTypeLabel, - dremio: localize.Integrations.dremioTypeLabel, - mariadb: localize.Integrations.mariaDBTypeLabel, - materialize: localize.Integrations.materializeTypeLabel, - mindsdb: localize.Integrations.mindsDBTypeLabel, - mongodb: localize.Integrations.mongoDBTypeLabel, - mysql: localize.Integrations.mySQLTypeLabel, - pgsql: localize.Integrations.postgresTypeLabel, - redshift: localize.Integrations.redshiftTypeLabel, - snowflake: localize.Integrations.snowflakeTypeLabel, - spanner: localize.Integrations.spannerTypeLabel, - 'sql-server': localize.Integrations.sqlServerTypeLabel, - trino: localize.Integrations.trinoTypeLabel -}; - -export function integrationTypeLabel(type: ConfigurableDatabaseIntegrationType): string { - return INTEGRATION_TYPE_LABELS[type] ?? type; -} - -/** A roster entry as recorded on disk: unlike `ProjectIntegration`, `type` is not narrowed to the known types. */ -export type RawProjectIntegration = NonNullable[number]; - /** An integration another project in the workspace has credentials stored for, so linking it needs no re-entry. */ export interface ReusableIntegration { id: string; - /** Name from the stored config — the same source the panel writes to the roster on save. */ + /** Name from the stored config — the same source the panel writes to the project integrations on save. */ name: string; /** The other projects declaring this integration; deduped and sorted. */ projectNames: string[]; @@ -53,17 +22,21 @@ export interface ReusableIntegration { } export interface CollectReusableIntegrationsParams { - /** Ids already on the current project's roster. */ + /** Ids the current project already declares. */ excludeIntegrationIds: ReadonlySet; integrationStorage: IIntegrationStorage; /** The project being extended; its own `.deepnote` files are skipped. */ projectId: string; + /** Stops the scan; the partial result is only fit to be discarded. */ + token?: CancellationToken; } export interface CollectReusableIntegrationsResult { + /** The scan stopped early, so the other two fields are partial and must not be written anywhere. */ + cancelled: boolean; /** - * Ids skipped because some project's roster type disagrees with the stored config: linking one would put a type - * on this project that the credentials cannot back. + * Ids skipped because some project declares a type the stored config disagrees with: linking one would put a + * type on this project that the credentials cannot back. */ conflictingIds: string[]; integrations: ReusableIntegration[]; @@ -71,7 +44,7 @@ export interface CollectReusableIntegrationsResult { export interface AttachExistingIntegrationParams { activeFileUri: Uri; - /** The project's full roster: every entry is written back verbatim, so a filtered array drops entries. */ + /** The project's full integration list: every entry is written back verbatim, so a filtered array drops entries. */ currentIntegrations: readonly RawProjectIntegration[]; integration: ReusableIntegration; notebookManager: IDeepnoteNotebookManager; @@ -82,7 +55,7 @@ export interface AttachExistingIntegrationParams { * Scans every `.deepnote` file in the open workspace folders for integrations other projects declare. * * Reuse is a link, not a copy: `IntegrationStorage` and `FederatedAuthTokenStorage` both key configs by integration - * id alone, so the roster entry is the only thing that scopes an integration to a project. + * id alone, so the project's own entry is the only thing that scopes an integration to a project. * * Integrations configured only in `.deepnote.env.yaml` are not offered: that file already applies to every project * under it, and the panel cannot write that layer. @@ -90,17 +63,26 @@ export interface AttachExistingIntegrationParams { export async function collectReusableIntegrations( params: CollectReusableIntegrationsParams ): Promise { - const { excludeIntegrationIds, integrationStorage, projectId } = params; + const { excludeIntegrationIds, integrationStorage, projectId, token } = params; const candidates = new Map }>(); const conflictingIds = new Set(); const visited = new Set(); for (const workspaceFolder of workspace.workspaceFolders || []) { + if (token?.isCancellationRequested) { + return { cancelled: true, conflictingIds: [], integrations: [] }; + } + let files: Uri[]; try { - files = await workspace.findFiles(new RelativePattern(workspaceFolder, '**/*.deepnote')); + files = await workspace.findFiles( + new RelativePattern(workspaceFolder, '**/*.deepnote'), + undefined, + undefined, + token + ); } catch (error) { logger.error('collectReusableIntegrations: failed to enumerate .deepnote files', error); @@ -108,6 +90,10 @@ export async function collectReusableIntegrations( } for (const fileUri of files) { + if (token?.isCancellationRequested) { + return { cancelled: true, conflictingIds: [], integrations: [] }; + } + const key = fileUri.toString(); if (visited.has(key) || isSnapshotFile(fileUri)) { @@ -179,23 +165,18 @@ export async function collectReusableIntegrations( })) .sort((a, b) => a.name.localeCompare(b.name) || a.id.localeCompare(b.id)); - return { conflictingIds: Array.from(conflictingIds).sort(), integrations }; + return { cancelled: false, conflictingIds: Array.from(conflictingIds).sort(), integrations }; } /** - * Links `integration` into the project's roster through the same writer the panel uses, so the cache, the active + * Links `integration` into the project's integrations through the same writer the panel uses, so the cache, the active * file and every sibling `.deepnote` file are updated together. Re-linking an id already there replaces its entry. */ export function attachExistingIntegration(params: AttachExistingIntegrationParams): Promise { const { activeFileUri, currentIntegrations, integration, notebookManager, projectId } = params; const linked: ProjectIntegration = { id: integration.id, name: integration.name, type: integration.type }; - // Cast rather than validate: filtering out types this build does not know (`pandas-dataframe`) would delete - // them from the file. - const integrations = [ - ...currentIntegrations.filter((entry) => entry.id !== integration.id), - linked - ] as ProjectIntegration[]; + const integrations = [...currentIntegrations.filter((entry) => entry.id !== integration.id), linked]; return persistProjectIntegrations({ activeFileUri, integrations, notebookManager, projectId }); } diff --git a/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts b/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts index 212a117029..80c06ca0bf 100644 --- a/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts +++ b/src/notebooks/deepnote/integrations/existingIntegrationPicker.unit.test.ts @@ -1,17 +1,15 @@ import { deserializeDeepnoteFile, serializeDeepnoteFile, type DeepnoteFile } from '@deepnote/blocks'; import { assert } from 'chai'; import { anything, instance, mock, when } from 'ts-mockito'; -import { Uri, workspace } from 'vscode'; +import { CancellationToken, CancellationTokenSource, Uri, workspace } from 'vscode'; import { ConfigurableDatabaseIntegrationConfig } from '../../../platform/notebooks/deepnote/integrationTypes'; import { mockedVSCodeNamespaces, resetVSCodeMocks } from '../../../test/vscode-mock'; -import { IDeepnoteNotebookManager, ProjectIntegration } from '../../types'; +import { IDeepnoteNotebookManager, ProjectIntegration, RawProjectIntegration } from '../../types'; import { createDeepnoteFile, createDeepnoteProject, createWorkspaceFolder } from '../deepnoteTestHelpers'; import { attachExistingIntegration, collectReusableIntegrations, - integrationTypeLabel, - RawProjectIntegration, ReusableIntegration } from './existingIntegrationPicker'; import { buildGoogleOauthIntegration, buildPostgresIntegration } from './federatedAuth/federatedAuthTestHelpers'; @@ -31,14 +29,19 @@ function projectFile(project: OnDiskProject): DeepnoteFile { project: createDeepnoteProject({ id: project.projectId, name: project.projectName ?? project.projectId, - // The roster type is a plain string on disk; the cast keeps the fixture free to declare unknown types. - integrations: project.integrations as ProjectIntegration[] | undefined + integrations: project.integrations }) }); } /** Stubs `workspace.findFiles` + `workspace.fs` over the given files; `unreadable` URIs reject on read. */ -function stubWorkspace(opts: { projects: OnDiskProject[]; unreadable?: Uri[]; hasWorkspaceFolder?: boolean }): { +function stubWorkspace(opts: { + projects: OnDiskProject[]; + unreadable?: Uri[]; + hasWorkspaceFolder?: boolean; + onRead?: (uri: Uri) => void; +}): { + reads: string[]; writes: Map; } { when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn( @@ -46,16 +49,25 @@ function stubWorkspace(opts: { projects: OnDiskProject[]; unreadable?: Uri[]; ha ); const discovered = [...opts.projects.map((project) => project.uri), ...(opts.unreadable ?? [])]; + when(mockedVSCodeNamespaces.workspace.findFiles(anything(), anything(), anything(), anything())).thenReturn( + Promise.resolve(discovered) + ); + // `persistProjectIntegrations` enumerates without a token; the scan passes one. when(mockedVSCodeNamespaces.workspace.findFiles(anything())).thenReturn(Promise.resolve(discovered)); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([]); const byPath = new Map(opts.projects.map((project) => [project.uri.fsPath, projectFile(project)] as const)); + const reads: string[] = []; const writes = new Map(); const mockFs = mock(); when(mockFs.readFile(anything())).thenCall((uri: Uri) => { + reads.push(uri.fsPath); + const file = byPath.get(uri.fsPath); + opts.onRead?.(uri); + return file ? Promise.resolve(new TextEncoder().encode(serializeDeepnoteFile(file))) : Promise.reject(new Error(`no readFile stub for ${uri.fsPath}`)); @@ -67,7 +79,7 @@ function stubWorkspace(opts: { projects: OnDiskProject[]; unreadable?: Uri[]; ha }); when(mockedVSCodeNamespaces.workspace.fs).thenReturn(instance(mockFs)); - return { writes }; + return { reads, writes }; } function stubStorage(configs: ConfigurableDatabaseIntegrationConfig[]): IIntegrationStorage { @@ -90,12 +102,14 @@ suite('existingIntegrationPicker', () => { async function collect( excludeIntegrationIds: string[] = [], - configs: ConfigurableDatabaseIntegrationConfig[] = [pgConfig, bqConfig] + configs: ConfigurableDatabaseIntegrationConfig[] = [pgConfig, bqConfig], + token?: CancellationToken ) { return collectReusableIntegrations({ excludeIntegrationIds: new Set(excludeIntegrationIds), integrationStorage: stubStorage(configs), - projectId: CURRENT_PROJECT_ID + projectId: CURRENT_PROJECT_ID, + token }); } @@ -133,16 +147,16 @@ suite('existingIntegrationPicker', () => { { id: 'pg-shared', name: 'Shared Postgres', projectNames: ['Alpha', 'Beta'], type: 'pgsql' }, { id: 'bq-oauth', name: 'Team BigQuery', projectNames: ['Beta'], type: 'big-query' } ]; - assert.deepStrictEqual(result, { conflictingIds: [], integrations: expected }); + assert.deepStrictEqual(result, { cancelled: false, conflictingIds: [], integrations: expected }); }); - test('takes the name from the stored config, not from whichever roster was read first', async () => { + test('takes the name from the stored config, not from whichever project was read first', async () => { stubWorkspace({ projects: [ { uri: Uri.file('/ws/a.deepnote'), projectId: 'project-a', - integrations: [{ id: 'pg-shared', name: 'Stale roster name', type: 'pgsql' }] + integrations: [{ id: 'pg-shared', name: 'Stale project name', type: 'pgsql' }] } ] }); @@ -152,7 +166,7 @@ suite('existingIntegrationPicker', () => { assert.strictEqual(integrations[0].name, 'Shared Postgres'); }); - test('excludes ids already on the current project roster and the current project files themselves', async () => { + test('excludes ids the current project already declares and the current project files themselves', async () => { stubWorkspace({ projects: [ { @@ -180,7 +194,7 @@ suite('existingIntegrationPicker', () => { ); }); - test('skips roster entries with no stored config (file-only or never configured) and unsupported types', async () => { + test('skips entries with no stored config (file-only or never configured) and unsupported types', async () => { stubWorkspace({ projects: [ { @@ -203,7 +217,7 @@ suite('existingIntegrationPicker', () => { ); }); - test('reports an id whose roster type disagrees with the stored config as conflicting and drops it everywhere', async () => { + test('reports an id whose declared type disagrees with the stored config as conflicting and drops it everywhere', async () => { stubWorkspace({ projects: [ { @@ -222,7 +236,7 @@ suite('existingIntegrationPicker', () => { const result = await collect(); - assert.deepStrictEqual(result, { conflictingIds: ['pg-shared'], integrations: [] }); + assert.deepStrictEqual(result, { cancelled: false, conflictingIds: ['pg-shared'], integrations: [] }); }); test('ignores snapshot files and keeps going past an unreadable file', async () => { @@ -250,12 +264,41 @@ suite('existingIntegrationPicker', () => { ); }); + test('stops at the next file and reports cancellation when the token trips mid-scan', async () => { + const cts = new CancellationTokenSource(); + + try { + const { reads } = stubWorkspace({ + projects: [ + { + uri: Uri.file('/ws/a.deepnote'), + projectId: 'project-a', + integrations: [{ id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' }] + }, + { + uri: Uri.file('/ws/b.deepnote'), + projectId: 'project-b', + integrations: [{ id: 'bq-oauth', name: 'Team BigQuery', type: 'big-query' }] + } + ], + onRead: () => cts.cancel() + }); + + const result = await collect([], [pgConfig, bqConfig], cts.token); + + assert.deepStrictEqual(result, { cancelled: true, conflictingIds: [], integrations: [] }); + assert.deepStrictEqual(reads, [Uri.file('/ws/a.deepnote').fsPath], 'the scan must not read on'); + } finally { + cts.dispose(); + } + }); + test('returns nothing without an open workspace folder', async () => { stubWorkspace({ projects: [], hasWorkspaceFolder: false }); const result = await collect(); - assert.deepStrictEqual(result, { conflictingIds: [], integrations: [] }); + assert.deepStrictEqual(result, { cancelled: false, conflictingIds: [], integrations: [] }); }); }); @@ -284,7 +327,7 @@ suite('existingIntegrationPicker', () => { notebookManager = instance(mockManager); }); - test('appends the linked entry to the roster in the cache and on disk, keeping existing entries', async () => { + test('appends the linked entry to the project integrations in the cache and on disk, keeping existing entries', async () => { const { writes } = stubWorkspace({ projects: [ { @@ -306,16 +349,18 @@ suite('existingIntegrationPicker', () => { projectId: CURRENT_PROJECT_ID }); - const expectedRoster: ProjectIntegration[] = [ + const expectedIntegrations: ProjectIntegration[] = [ { id: 'bq-own', name: 'Own BigQuery', type: 'big-query' }, { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' } ]; assert.deepStrictEqual(result, { activePersisted: true, siblingsFailed: 0 }); - assert.deepStrictEqual(cacheUpdates, [{ projectId: CURRENT_PROJECT_ID, integrations: expectedRoster }]); - assert.deepStrictEqual(writes.get(activeUri.fsPath)?.project.integrations, expectedRoster); + assert.deepStrictEqual(cacheUpdates, [ + { projectId: CURRENT_PROJECT_ID, integrations: expectedIntegrations } + ]); + assert.deepStrictEqual(writes.get(activeUri.fsPath)?.project.integrations, expectedIntegrations); }); - test('passes roster entries of types it cannot manage (e.g. pandas-dataframe) through verbatim', async () => { + test('passes entries of types it cannot manage (e.g. pandas-dataframe) through verbatim', async () => { const { writes } = stubWorkspace({ projects: [{ uri: activeUri, projectId: CURRENT_PROJECT_ID }] }); @@ -332,15 +377,17 @@ suite('existingIntegrationPicker', () => { projectId: CURRENT_PROJECT_ID }); - const expectedRoster = [ + const expectedIntegrations = [ ...currentIntegrations, { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' } ]; - assert.deepStrictEqual(cacheUpdates, [{ projectId: CURRENT_PROJECT_ID, integrations: expectedRoster }]); - assert.deepStrictEqual(writes.get(activeUri.fsPath)?.project.integrations, expectedRoster); + assert.deepStrictEqual(cacheUpdates, [ + { projectId: CURRENT_PROJECT_ID, integrations: expectedIntegrations } + ]); + assert.deepStrictEqual(writes.get(activeUri.fsPath)?.project.integrations, expectedIntegrations); }); - test('replaces rather than duplicates an entry whose id is already on the roster', async () => { + test('replaces rather than duplicates an entry whose id the project already declares', async () => { const { writes } = stubWorkspace({ projects: [{ uri: activeUri, projectId: CURRENT_PROJECT_ID }] }); @@ -358,9 +405,4 @@ suite('existingIntegrationPicker', () => { ]); }); }); - - test('integrationTypeLabel maps every configurable type to a display label', () => { - assert.strictEqual(integrationTypeLabel('pgsql'), 'PostgreSQL'); - assert.strictEqual(integrationTypeLabel('big-query'), 'Google BigQuery'); - }); }); diff --git a/src/notebooks/deepnote/integrations/integrationDetector.ts b/src/notebooks/deepnote/integrations/integrationDetector.ts index d860175e96..e35044fd84 100644 --- a/src/notebooks/deepnote/integrations/integrationDetector.ts +++ b/src/notebooks/deepnote/integrations/integrationDetector.ts @@ -26,11 +26,11 @@ export class IntegrationDetector implements IIntegrationDetector { /** * Detect all integrations for the notebook's project. Three inputs, three roles: - * - `project.integrations` is the roster (ids, names and types only — never credentials), so it decides + * - `project.integrations` holds ids, names and types only — never credentials — so it decides * the order and the names the panel shows. * - SecretStorage supplies the editable config for each one; integrations configured only in * `.deepnote.env.yaml` stay `null` here, since those configs are never persisted through it. - * - `.deepnote.env.yaml` entries missing from the roster are appended, matching what actually applies at + * - `.deepnote.env.yaml` entries the project omits are appended, matching what actually applies at * execution time. Without this a file-only integration works but is invisible, and a federated one is * unusable outright — its Authenticate action exists only as a row in this panel. */ @@ -76,9 +76,9 @@ export class IntegrationDetector implements IIntegrationDetector { } /** - * Adds `.deepnote.env.yaml` integrations the roster omits. `config` stays `null` because the panel edits + * Adds `.deepnote.env.yaml` integrations the project omits. `config` stays `null` because the panel edits * SecretStorage only and the file layer cannot be written back; the name and type are carried so the row - * renders. A failed lookup leaves the roster-only result rather than blocking the panel. + * renders. A failed lookup leaves the project's own integrations rather than blocking the panel. */ private async appendFileOnlyIntegrations( notebookUri: Uri, @@ -89,13 +89,16 @@ export class IntegrationDetector implements IIntegrationDetector { try { mergedIntegrationConfigs = await this.sqlIntegrationEnvVars.getMergedIntegrationConfigs(notebookUri); } catch (error) { - logger.error('IntegrationDetector: failed to read file integrations; listing the roster only', error); + logger.error( + "IntegrationDetector: failed to read file integrations; listing the project's own only", + error + ); return; } for (const config of mergedIntegrationConfigs) { - // Anything merged but absent here came from the file alone — the merge resolves roster ids first. + // Anything merged but absent here came from the file alone — the merge resolves the project's ids first. if (integrations.has(config.id) || !isConfigurableDatabaseIntegrationType(config.type)) { continue; } diff --git a/src/notebooks/deepnote/integrations/integrationManager.ts b/src/notebooks/deepnote/integrations/integrationManager.ts index 004d41db22..f7739c79e4 100644 --- a/src/notebooks/deepnote/integrations/integrationManager.ts +++ b/src/notebooks/deepnote/integrations/integrationManager.ts @@ -1,5 +1,5 @@ import { inject, injectable, optional } from 'inversify'; -import { commands, l10n, NotebookDocument, QuickPickItem, window, workspace } from 'vscode'; +import { commands, l10n, NotebookDocument, ProgressLocation, QuickPickItem, window, workspace } from 'vscode'; import { CommandOutcome, ITelemetryService } from '../../../platform/analytics/types'; import { IExtensionContext } from '../../../platform/common/types'; @@ -13,15 +13,14 @@ import { IIntegrationStorage, IIntegrationWebviewProvider } from './types'; -import { IDeepnoteNotebookManager } from '../../types'; +import { IDeepnoteNotebookManager, RawProjectIntegration } from '../../types'; import { DatabaseIntegrationType, databaseIntegrationTypes } from '@deepnote/database-integrations'; import { attachExistingIntegration, collectReusableIntegrations, - integrationTypeLabel, - RawProjectIntegration, ReusableIntegration } from './existingIntegrationPicker'; +import { isSnapshotFile } from '../snapshots/snapshotFiles'; interface ReusableIntegrationQuickPickItem extends QuickPickItem { integration: ReusableIntegration; @@ -88,8 +87,8 @@ export class IntegrationManager implements IIntegrationManager { } /** - * Offers the integrations other projects in the workspace declare and links the chosen one into this project's - * roster; no credentials are copied (see `collectReusableIntegrations`). Public so tests can drive it without + * Offers the integrations other projects in the workspace declare and links the chosen one into this project; + * no credentials are copied (see `collectReusableIntegrations`). Public so tests can drive it without * `commands.executeCommand`. */ public async addExistingIntegration(notebookUri?: string): Promise { @@ -101,6 +100,14 @@ export class IntegrationManager implements IIntegrationManager { return 'failed'; } + // `*.snapshot.deepnote` matches the notebook selector, so this command can run against a focused snapshot. + // The writer skips snapshots, which would report a failure only after the cache had already been updated. + if (isSnapshotFile(activeNotebook.uri)) { + void window.showErrorMessage(localize.Integrations.addExistingIntegrationSnapshotUnsupported); + + return 'failed'; + } + const projectId = activeNotebook.metadata?.deepnoteProjectId; const notebookId = activeNotebook.metadata?.deepnoteNotebookId; @@ -110,12 +117,32 @@ export class IntegrationManager implements IIntegrationManager { return 'failed'; } - const currentIntegrations = this.getCachedRoster(projectId, notebookId); - const { conflictingIds, integrations } = await collectReusableIntegrations({ - excludeIntegrationIds: new Set(currentIntegrations.map((entry) => entry.id)), - integrationStorage: this.integrationStorage, - projectId - }); + const currentIntegrations = this.getCachedProjectIntegrations(projectId, notebookId); + + if (!currentIntegrations) { + void window.showErrorMessage(localize.Integrations.addExistingIntegrationFailed); + + return 'failed'; + } + + const { cancelled, conflictingIds, integrations } = await window.withProgress( + { + cancellable: true, + location: ProgressLocation.Notification, + title: localize.Integrations.addExistingIntegrationScanning + }, + (_progress, token) => + collectReusableIntegrations({ + excludeIntegrationIds: new Set(currentIntegrations.map((entry) => entry.id)), + integrationStorage: this.integrationStorage, + projectId, + token + }) + ); + + if (cancelled) { + return 'cancelled'; + } if (conflictingIds.length > 0) { void window.showWarningMessage( @@ -130,7 +157,7 @@ export class IntegrationManager implements IIntegrationManager { } const items: ReusableIntegrationQuickPickItem[] = integrations.map((integration) => ({ - description: integrationTypeLabel(integration.type), + description: localize.Integrations.typeLabel(integration.type), detail: localize.Integrations.addExistingIntegrationUsedIn(integration.projectNames.join(', ')), integration, label: integration.name @@ -147,12 +174,23 @@ export class IntegrationManager implements IIntegrationManager { } const { integration } = picked; + // The file watcher replaces the cached project on any external write — including one that changes only + // the integrations, which it reloads past without a UI event — so the pre-pick array can be stale, and + // the writer stamps whatever it is given onto every `.deepnote` file of the project. + const integrationsAtWrite = this.getCachedProjectIntegrations(projectId, notebookId); + + if (!integrationsAtWrite) { + void window.showErrorMessage(localize.Integrations.addExistingIntegrationFailed); + + return 'failed'; + } + let outcome: CommandOutcome = 'failed'; try { const { activePersisted, siblingsFailed } = await attachExistingIntegration({ activeFileUri: activeNotebook.uri, - currentIntegrations, + currentIntegrations: integrationsAtWrite, integration, notebookManager: this.notebookManager, projectId @@ -175,7 +213,7 @@ export class IntegrationManager implements IIntegrationManager { // Storage did not change, so the storage-change listeners that normally refresh kernels and the // panel after a save stay silent; do both explicitly for this project. - await this.refreshAfterRosterChange(projectId, activeNotebook); + await this.refreshAfterProjectIntegrationsChange(projectId, activeNotebook); } else { void window.showErrorMessage(localize.Integrations.addExistingIntegrationFailed); } @@ -204,14 +242,20 @@ export class IntegrationManager implements IIntegrationManager { return uri ? String(uri) : undefined; } - /** Unfiltered on purpose: `attachExistingIntegration` writes this array back, so anything dropped here is lost. */ - private getCachedRoster(projectId: string, notebookId: string): RawProjectIntegration[] { + /** + * Unfiltered on purpose: `attachExistingIntegration` writes this array back, so anything dropped here is lost. + * `undefined` means the project is not cached, which must not be written back as an empty list. + */ + private getCachedProjectIntegrations(projectId: string, notebookId: string): RawProjectIntegration[] | undefined { const project = this.notebookManager.getProjectForNotebook(projectId, notebookId); - return [...(project?.project.integrations ?? [])]; + return project ? [...(project.project.integrations ?? [])] : undefined; } - private async refreshAfterRosterChange(projectId: string, activeNotebook: NotebookDocument): Promise { + private async refreshAfterProjectIntegrationsChange( + projectId: string, + activeNotebook: NotebookDocument + ): Promise { const projectNotebooks = workspace.notebookDocuments.filter( (notebook) => notebook.notebookType === 'deepnote' && notebook.metadata?.deepnoteProjectId === projectId ); diff --git a/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts b/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts index 48f56ab156..993746b728 100644 --- a/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts +++ b/src/notebooks/deepnote/integrations/integrationManager.unit.test.ts @@ -2,13 +2,13 @@ import { deserializeDeepnoteFile, serializeDeepnoteFile, type DeepnoteFile } fro import { assert } from 'chai'; import sinon from 'sinon'; import { anything, deepEqual, instance, mock, verify, when } from 'ts-mockito'; -import { NotebookDocument, QuickPickItem, Uri, workspace } from 'vscode'; +import { CancellationToken, CancellationTokenSource, NotebookDocument, QuickPickItem, Uri, workspace } from 'vscode'; import { ITelemetryService } from '../../../platform/analytics/types'; import { IExtensionContext } from '../../../platform/common/types'; import { ConfigurableDatabaseIntegrationConfig } from '../../../platform/notebooks/deepnote/integrationTypes'; import { mockedVSCodeNamespaces, resetVSCodeMocks } from '../../../test/vscode-mock'; -import { IDeepnoteNotebookManager, ProjectIntegration } from '../../types'; +import { IDeepnoteNotebookManager, ProjectIntegration, RawProjectIntegration } from '../../types'; import { createDeepnoteFile, createDeepnoteProject, @@ -16,7 +16,6 @@ import { createWorkspaceFolder } from '../deepnoteTestHelpers'; import { buildPostgresIntegration } from './federatedAuth/federatedAuthTestHelpers'; -import { RawProjectIntegration } from './existingIntegrationPicker'; import { IntegrationManager } from './integrationManager'; import { IIntegrationDetector, @@ -30,9 +29,14 @@ const CURRENT_NOTEBOOK_ID = 'notebook-current'; const OTHER_PROJECT_ID = 'project-other'; const CURRENT_URI = Uri.file('/ws/current.deepnote'); const OTHER_URI = Uri.file('/ws/other.deepnote'); +/** A second file of the CURRENT project, so the sibling-write branch has something to fail on. */ +const SIBLING_URI = Uri.file('/ws/sibling.deepnote'); +const SNAPSHOT_URI = Uri.file('/ws/snapshots/current_project-current_latest.snapshot.deepnote'); const SHARED_CONFIG = buildPostgresIntegration({ id: 'pg-shared', name: 'Shared Postgres' }); +type RefreshFn = IIntegrationEnvLiveRefresher['refresh']; + function projectFile(projectId: string, notebookId: string, integrations: RawProjectIntegration[]): DeepnoteFile { return createDeepnoteFile({ project: createDeepnoteProject({ @@ -47,18 +51,23 @@ function projectFile(projectId: string, notebookId: string, integrations: RawPro suite('IntegrationManager.addExistingIntegration', () => { let currentNotebook: NotebookDocument; let otherNotebook: NotebookDocument; - let currentProject: DeepnoteFile; + let currentProject: DeepnoteFile | undefined; let writes: Map; let cacheUpdates: ProjectIntegration[][]; - let refreshSpy: sinon.SinonSpy; + // Typed off the interface so a signature change fails the compile, not just the `firstCall.args` assertion. + let refreshSpy: sinon.SinonSpy, ReturnType>; let quickPickItems: QuickPickItem[] | undefined; + let scanProgress: CancellationTokenSource; + let writeFailures: Set; let detector: IIntegrationDetector; let webviewProvider: IIntegrationWebviewProvider; let notebookManager: IDeepnoteNotebookManager; let telemetry: ITelemetryService; + let cacheUpdateError: Error | undefined; let storedConfigs: ConfigurableDatabaseIntegrationConfig[]; let onDiskOther: DeepnoteFile | undefined; + let onDiskSibling: DeepnoteFile | undefined; setup(() => { resetVSCodeMocks(); @@ -75,26 +84,52 @@ suite('IntegrationManager.addExistingIntegration', () => { onDiskOther = projectFile(OTHER_PROJECT_ID, 'notebook-other', [ { id: SHARED_CONFIG.id, name: SHARED_CONFIG.name, type: SHARED_CONFIG.type } ]); + onDiskSibling = undefined; + cacheUpdateError = undefined; storedConfigs = [SHARED_CONFIG]; writes = new Map(); cacheUpdates = []; quickPickItems = undefined; + scanProgress = new CancellationTokenSource(); + writeFailures = new Set(); when(mockedVSCodeNamespaces.workspace.workspaceFolders).thenReturn([createWorkspaceFolder(Uri.file('/ws'))]); when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([currentNotebook, otherNotebook]); - when(mockedVSCodeNamespaces.workspace.findFiles(anything())).thenCall(() => - Promise.resolve(onDiskOther ? [CURRENT_URI, OTHER_URI] : [CURRENT_URI]) + const discovered = () => + Promise.resolve([ + CURRENT_URI, + ...(onDiskOther ? [OTHER_URI] : []), + ...(onDiskSibling ? [SIBLING_URI] : []) + ]); + + // The writer enumerates without a token; the scan passes one. + when(mockedVSCodeNamespaces.workspace.findFiles(anything())).thenCall(discovered); + when(mockedVSCodeNamespaces.workspace.findFiles(anything(), anything(), anything(), anything())).thenCall( + discovered + ); + + when(mockedVSCodeNamespaces.window.withProgress(anything(), anything())).thenCall( + (_options: unknown, task: (progress: unknown, token: CancellationToken) => unknown) => + task({ report: () => undefined }, scanProgress.token) ); const mockFs = mock(); when(mockFs.readFile(anything())).thenCall((uri: Uri) => { - const file = uri.fsPath === CURRENT_URI.fsPath ? currentProject : onDiskOther; + const file = new Map([ + [CURRENT_URI.fsPath, currentProject], + [OTHER_URI.fsPath, onDiskOther], + [SIBLING_URI.fsPath, onDiskSibling] + ]).get(uri.fsPath); return file ? Promise.resolve(new TextEncoder().encode(serializeDeepnoteFile(file))) : Promise.reject(new Error(`no readFile stub for ${uri.fsPath}`)); }); when(mockFs.writeFile(anything(), anything())).thenCall((uri: Uri, bytes: Uint8Array) => { + if (writeFailures.has(uri.fsPath)) { + return Promise.reject(new Error(`write blocked for ${uri.fsPath}`)); + } + writes.set(uri.fsPath, deserializeDeepnoteFile(new TextDecoder().decode(bytes))); return Promise.resolve(); @@ -119,6 +154,10 @@ suite('IntegrationManager.addExistingIntegration', () => { when(mockManager.getProjectForNotebook(CURRENT_PROJECT_ID, CURRENT_NOTEBOOK_ID)).thenCall(() => currentProject); when(mockManager.updateProjectIntegrations(anything(), anything())).thenCall( (_projectId: string, integrations: ProjectIntegration[]) => { + if (cacheUpdateError) { + throw cacheUpdateError; + } + cacheUpdates.push(integrations); return true; @@ -127,7 +166,11 @@ suite('IntegrationManager.addExistingIntegration', () => { notebookManager = mockManager; telemetry = mock(); - refreshSpy = sinon.spy(async () => undefined); + refreshSpy = sinon.spy(async () => undefined); + }); + + teardown(() => { + scanProgress.dispose(); }); function buildManager(): IntegrationManager { @@ -152,14 +195,16 @@ suite('IntegrationManager.addExistingIntegration', () => { ); } - test("links the picked integration into the roster, refreshes only this project's kernels and re-shows the panel", async () => { + test("links the picked integration, refreshes only this project's kernels and re-shows the panel", async () => { const outcome = await buildManager().addExistingIntegration(CURRENT_URI.toString()); assert.strictEqual(outcome, 'completed'); - const expectedRoster: ProjectIntegration[] = [{ id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' }]; - assert.deepStrictEqual(writes.get(CURRENT_URI.fsPath)?.project.integrations, expectedRoster); - assert.deepStrictEqual(cacheUpdates, [expectedRoster]); + const expectedIntegrations: ProjectIntegration[] = [ + { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' } + ]; + assert.deepStrictEqual(writes.get(CURRENT_URI.fsPath)?.project.integrations, expectedIntegrations); + assert.deepStrictEqual(cacheUpdates, [expectedIntegrations]); assert.isUndefined(writes.get(OTHER_URI.fsPath), 'the other project must not be rewritten'); assert.isTrue(refreshSpy.calledOnce); @@ -195,7 +240,7 @@ suite('IntegrationManager.addExistingIntegration', () => { verify(mockedVSCodeNamespaces.window.showInformationMessage(anything())).once(); }); - test('keeps roster entries the panel cannot manage (pandas-dataframe) when attaching', async () => { + test('keeps entries the panel cannot manage (pandas-dataframe) when attaching', async () => { currentProject = projectFile(CURRENT_PROJECT_ID, CURRENT_NOTEBOOK_ID, [ { id: 'duckdb', name: 'DuckDB', type: 'pandas-dataframe' } ]); @@ -204,12 +249,12 @@ suite('IntegrationManager.addExistingIntegration', () => { assert.strictEqual(outcome, 'completed'); - const expectedRoster = [ + const expectedIntegrations = [ { id: 'duckdb', name: 'DuckDB', type: 'pandas-dataframe' }, { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' } ]; - assert.deepStrictEqual(writes.get(CURRENT_URI.fsPath)?.project.integrations, expectedRoster); - assert.deepStrictEqual(cacheUpdates, [expectedRoster]); + assert.deepStrictEqual(writes.get(CURRENT_URI.fsPath)?.project.integrations, expectedIntegrations); + assert.deepStrictEqual(cacheUpdates, [expectedIntegrations]); assert.strictEqual(quickPickItems?.length, 1, 'the DuckDB entry is neither offered nor a candidate'); }); @@ -255,4 +300,123 @@ suite('IntegrationManager.addExistingIntegration', () => { assert.strictEqual(outcome, 'failed'); verify(mockedVSCodeNamespaces.window.showErrorMessage(anything())).once(); }); + // Every branch below reports trouble to the user; without cover they can each regress into silent success. + const earlyFailures: { arrange: () => string; name: string }[] = [ + { + arrange: () => { + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([ + createMockNotebook({ uri: CURRENT_URI, metadata: {} }) + ]); + + return CURRENT_URI.toString(); + }, + name: 'the notebook declares no project or notebook id' + }, + { + arrange: () => { + when(mockedVSCodeNamespaces.workspace.notebookDocuments).thenReturn([ + createMockNotebook({ + uri: SNAPSHOT_URI, + metadata: { + deepnoteProjectId: CURRENT_PROJECT_ID, + deepnoteNotebookId: CURRENT_NOTEBOOK_ID + } + }) + ]); + + return SNAPSHOT_URI.toString(); + }, + name: 'the active file is a snapshot' + }, + { + arrange: () => { + currentProject = undefined; + + return CURRENT_URI.toString(); + }, + name: 'the project is not in the cache' + } + ]; + + for (const { arrange, name } of earlyFailures) { + test(`fails without reading or writing any file when ${name}`, async () => { + const uri = arrange(); + + const outcome = await buildManager().addExistingIntegration(uri); + + assert.strictEqual(outcome, 'failed'); + assert.strictEqual(writes.size, 0); + assert.deepStrictEqual(cacheUpdates, [], 'the cache must not move before the file does'); + verify(mockedVSCodeNamespaces.window.showErrorMessage(anything())).once(); + verify(mockedVSCodeNamespaces.window.showQuickPick(anything(), anything())).never(); + verify(telemetry.trackEvent(anything())).never(); + }); + } + + const lateFailures: { arrange: () => void; name: string }[] = [ + { arrange: () => writeFailures.add(CURRENT_URI.fsPath), name: 'the active file cannot be written' }, + { arrange: () => (cacheUpdateError = new Error('cache rejected the update')), name: 'the writer throws' } + ]; + + for (const { arrange, name } of lateFailures) { + test(`reports failure and records the outcome in telemetry when ${name}`, async () => { + arrange(); + + const outcome = await buildManager().addExistingIntegration(CURRENT_URI.toString()); + + assert.strictEqual(outcome, 'failed'); + assert.isTrue(refreshSpy.notCalled, 'nothing reached disk for the kernels to pick up'); + verify(mockedVSCodeNamespaces.window.showErrorMessage(anything())).once(); + verify( + telemetry.trackEvent( + deepEqual({ + eventName: 'add_existing_integration', + properties: { integrationType: 'pgsql', outcome: 'failed' } + }) + ) + ).once(); + }); + } + + test('completes with a warning when a sibling file of the same project cannot be updated', async () => { + onDiskSibling = projectFile(CURRENT_PROJECT_ID, 'notebook-sibling', []); + writeFailures.add(SIBLING_URI.fsPath); + + const outcome = await buildManager().addExistingIntegration(CURRENT_URI.toString()); + + assert.strictEqual(outcome, 'completed'); + assert.isDefined(writes.get(CURRENT_URI.fsPath), 'the active file is still persisted'); + assert.isUndefined(writes.get(SIBLING_URI.fsPath)); + verify(mockedVSCodeNamespaces.window.showWarningMessage(anything())).once(); + }); + + test('writes the integrations as they stand after the pick, not the snapshot taken before it', async () => { + when(mockedVSCodeNamespaces.window.showQuickPick(anything(), anything())).thenCall((items: QuickPickItem[]) => { + // What the file watcher does to the cache when another writer touches the file mid-pick. + currentProject = projectFile(CURRENT_PROJECT_ID, CURRENT_NOTEBOOK_ID, [ + { id: 'added-meanwhile', name: 'Added meanwhile', type: 'mysql' } + ]); + + return Promise.resolve(items[0]); + }); + + const outcome = await buildManager().addExistingIntegration(CURRENT_URI.toString()); + + assert.strictEqual(outcome, 'completed'); + assert.deepStrictEqual(writes.get(CURRENT_URI.fsPath)?.project.integrations, [ + { id: 'added-meanwhile', name: 'Added meanwhile', type: 'mysql' }, + { id: 'pg-shared', name: 'Shared Postgres', type: 'pgsql' } + ]); + }); + + test('returns cancelled and writes nothing when the scan is cancelled', async () => { + scanProgress.cancel(); + + const outcome = await buildManager().addExistingIntegration(CURRENT_URI.toString()); + + assert.strictEqual(outcome, 'cancelled'); + assert.strictEqual(writes.size, 0); + verify(mockedVSCodeNamespaces.window.showQuickPick(anything(), anything())).never(); + verify(telemetry.trackEvent(anything())).never(); + }); }); diff --git a/src/notebooks/deepnote/integrations/integrationWebview.ts b/src/notebooks/deepnote/integrations/integrationWebview.ts index 5a68229e88..052a7421cf 100644 --- a/src/notebooks/deepnote/integrations/integrationWebview.ts +++ b/src/notebooks/deepnote/integrations/integrationWebview.ts @@ -175,25 +175,7 @@ export class IntegrationWebviewProvider implements IIntegrationWebviewProvider { integrationsDatabase: localize.Integrations.database, integrationsDataWarehousesLakes: localize.Integrations.dataWarehousesLakes, integrationsDatabases: localize.Integrations.databases, - integrationsPostgresTypeLabel: localize.Integrations.postgresTypeLabel, - integrationsBigQueryTypeLabel: localize.Integrations.bigQueryTypeLabel, - integrationsSnowflakeTypeLabel: localize.Integrations.snowflakeTypeLabel, - integrationsAlloyDBTypeLabel: localize.Integrations.alloyDBTypeLabel, - integrationsAthenaTypeLabel: localize.Integrations.athenaTypeLabel, - integrationsClickHouseTypeLabel: localize.Integrations.clickHouseTypeLabel, - integrationsCloudSqlTypeLabel: localize.Integrations.cloudSqlTypeLabel, - integrationsDatabricksTypeLabel: localize.Integrations.databricksTypeLabel, - integrationsDremioTypeLabel: localize.Integrations.dremioTypeLabel, - integrationsMariaDBTypeLabel: localize.Integrations.mariaDBTypeLabel, - integrationsMaterializeTypeLabel: localize.Integrations.materializeTypeLabel, - integrationsMindsDBTypeLabel: localize.Integrations.mindsDBTypeLabel, - integrationsMongoDBTypeLabel: localize.Integrations.mongoDBTypeLabel, - integrationsMySQLTypeLabel: localize.Integrations.mySQLTypeLabel, - integrationsDuckDBTypeLabel: localize.Integrations.duckDBTypeLabel, - integrationsRedshiftTypeLabel: localize.Integrations.redshiftTypeLabel, - integrationsSpannerTypeLabel: localize.Integrations.spannerTypeLabel, - integrationsSQLServerTypeLabel: localize.Integrations.sqlServerTypeLabel, - integrationsTrinoTypeLabel: localize.Integrations.trinoTypeLabel, + ...localize.Integrations.typeLabels, integrationsCancel: localize.Integrations.cancel, integrationsSave: localize.Integrations.save, integrationsRequiredField: localize.Integrations.requiredField, @@ -746,7 +728,7 @@ export class IntegrationWebviewProvider implements IIntegrationWebviewProvider { } break; case 'addExisting': - // The command owns the picker and the roster write, and re-shows this panel when it is done. + // The command owns the picker and the integrations write, and re-shows this panel when it is done. try { await commands.executeCommand(Commands.AddExistingIntegration, { notebookUri: this.activeFileUri?.toString() diff --git a/src/notebooks/deepnote/integrations/projectIntegrationsWriter.ts b/src/notebooks/deepnote/integrations/projectIntegrationsWriter.ts index 6822a22fa8..e9f56ba8a7 100644 --- a/src/notebooks/deepnote/integrations/projectIntegrationsWriter.ts +++ b/src/notebooks/deepnote/integrations/projectIntegrationsWriter.ts @@ -4,7 +4,7 @@ import { RelativePattern, Uri, workspace } from 'vscode'; import { flushNotebookDocumentIfDirty } from '../../../platform/deepnote/deepnoteDocumentFlush'; import { readDeepnoteProjectFile } from '../../../platform/deepnote/deepnoteProjectFileReader'; import { logger } from '../../../platform/logging'; -import { IDeepnoteNotebookManager, ProjectIntegration } from '../../types'; +import { IDeepnoteNotebookManager, RawProjectIntegration } from '../../types'; import { isSnapshotFile } from '../snapshots/snapshotFiles'; export interface PersistIntegrationsResult { @@ -15,14 +15,14 @@ export interface PersistIntegrationsResult { export interface PersistProjectIntegrationsParams { notebookManager: IDeepnoteNotebookManager; projectId: string; - integrations: ProjectIntegration[]; + integrations: RawProjectIntegration[]; activeFileUri: Uri; } interface WriteIntegrationsToFileParams { fileUri: Uri; projectId: string; - integrations: ProjectIntegration[]; + integrations: RawProjectIntegration[]; } type IntegrationWriteOutcome = 'failed' | 'skipped' | 'written'; diff --git a/src/notebooks/deepnote/sqlCellStatusBarProvider.ts b/src/notebooks/deepnote/sqlCellStatusBarProvider.ts index d5f7d2694e..3e83519b80 100644 --- a/src/notebooks/deepnote/sqlCellStatusBarProvider.ts +++ b/src/notebooks/deepnote/sqlCellStatusBarProvider.ts @@ -21,19 +21,18 @@ import { inject, injectable } from 'inversify'; import { IExtensionSyncActivationService } from '../../platform/activation/types'; import { ITelemetryService } from '../../platform/analytics/types'; import { IDisposableRegistry } from '../../platform/common/types'; +import * as localize from '../../platform/common/utils/localize'; import { IIntegrationStorage } from './integrations/types'; import { Commands } from '../../platform/common/constants'; import { - ConfigurableDatabaseIntegrationType, DATAFRAME_SQL_INTEGRATION_ID, isConfigurableDatabaseIntegrationType, toTelemetryIntegrationType } from '../../platform/notebooks/deepnote/integrationTypes'; import { persistProjectIntegrations } from './integrations/projectIntegrationsWriter'; -import { IDeepnoteNotebookManager, ProjectIntegration } from '../types'; +import { IDeepnoteNotebookManager, RawProjectIntegration } from '../types'; import { logger } from '../../platform/logging'; import { ISqlIntegrationEnvVarsProvider } from '../../platform/notebooks/deepnote/types'; -import type { DeepnoteFile } from '@deepnote/blocks'; import { DatabaseIntegrationConfig, DatabaseIntegrationType, @@ -41,7 +40,6 @@ import { } from '@deepnote/database-integrations'; /** One entry of a project's `integrations` list as it appears in the file, where `type` is not yet validated. */ -type RawProjectIntegration = NonNullable[number]; /** * QuickPick item with an integration ID @@ -50,27 +48,6 @@ interface LocalQuickPickItem extends QuickPickItem { id: string; } -const integrationTypeLabels: Record = { - alloydb: l10n.t('Google AlloyDB'), - athena: l10n.t('Amazon Athena'), - 'big-query': l10n.t('Google BigQuery'), - clickhouse: l10n.t('ClickHouse'), - 'cloud-sql': l10n.t('Google Cloud SQL'), - databricks: l10n.t('Databricks'), - dremio: l10n.t('Dremio'), - mariadb: l10n.t('MariaDB'), - materialize: l10n.t('Materialize'), - mindsdb: l10n.t('MindsDB'), - mongodb: l10n.t('MongoDB'), - mysql: l10n.t('MySQL'), - pgsql: l10n.t('PostgreSQL'), - redshift: l10n.t('Amazon Redshift'), - snowflake: l10n.t('Snowflake'), - spanner: l10n.t('Google Spanner'), - 'sql-server': l10n.t('Microsoft SQL Server'), - trino: l10n.t('Trino') -}; - /** * Provides status bar items for SQL cells showing the integration name and variable name */ @@ -240,7 +217,7 @@ export class SqlCellStatusBarProvider implements NotebookCellStatusBarItemProvid // Merged first: at execution time a `.deepnote.env.yaml` config wins on id conflict, so resolving // SecretStorage first would label the cell with a database it does not connect to. const mergedConfig = (await this.getMergedIntegrationConfigs(cell)).find((c) => c.id === integrationId); - // The merge only resolves ids the project roster or the file declares; SecretStorage still answers for the rest. + // The merge only resolves ids the project or the file declares; SecretStorage still answers for the rest. const config = mergedConfig ?? (await this.integrationStorage.getProjectIntegrationConfig(projectId, integrationId)); @@ -344,17 +321,17 @@ export class SqlCellStatusBarProvider implements NotebookCellStatusBarItemProvid } /** - * Appends a picked integration to the project roster so the `.deepnote` file records what it uses. + * Appends a picked integration to the project's integrations so the `.deepnote` file records what it uses. * Additive only: existing entries pass through verbatim, never filtered, because a project's integrations * are shared with sibling notebooks whose blocks this cannot see — dropping one would be data loss. */ private async addToProjectIntegrations( cell: NotebookCell, projectId: string, - roster: RawProjectIntegration[], + projectIntegrations: RawProjectIntegration[], selected: RawProjectIntegration ): Promise { - // No usable `type` means no valid roster entry; leave it out rather than guessing one. + // No usable `type` means no valid entry; leave it out rather than guessing one. if (!isConfigurableDatabaseIntegrationType(selected.type)) { return; } @@ -363,9 +340,7 @@ export class SqlCellStatusBarProvider implements NotebookCellStatusBarItemProvid await persistProjectIntegrations({ notebookManager: this.notebookManager, projectId, - // Cast rather than narrow: validating the existing entries would silently drop any type this - // build does not know about, which is pruning by another name. - integrations: [...roster, selected] as ProjectIntegration[], + integrations: [...projectIntegrations, selected], activeFileUri: cell.notebook.uri }); } catch (error) { @@ -389,19 +364,20 @@ export class SqlCellStatusBarProvider implements NotebookCellStatusBarItemProvid } /** - * The roster plus any `.deepnote.env.yaml` integrations it omits, so a file-only one can be picked here - * instead of only by hand-editing `sql_integration_id`. A failed lookup falls back to the roster alone. + * The project's integrations plus any `.deepnote.env.yaml` ones they omit, so a file-only integration can be + * picked here instead of only by hand-editing `sql_integration_id`. A failed lookup falls back to the + * project's integrations alone. */ private async getSelectableIntegrations( cell: NotebookCell, projectIntegrations: RawProjectIntegration[] ): Promise { const mergedIntegrationConfigs = await this.getMergedIntegrationConfigs(cell); - const rosterIds = new Set(projectIntegrations.map((integration) => integration.id)); + const projectIntegrationIds = new Set(projectIntegrations.map((integration) => integration.id)); - // Anything merged but absent from the roster came from the file alone — the merge resolves roster ids first. + // Anything merged but absent here came from the file alone — the merge resolves the project's ids first. const fileOnly = mergedIntegrationConfigs - .filter((config) => !rosterIds.has(config.id)) + .filter((config) => !projectIntegrationIds.has(config.id)) .map((config) => ({ id: config.id, name: config.name, type: config.type })); return fileOnly.length > 0 ? [...projectIntegrations, ...fileOnly] : projectIntegrations; @@ -428,14 +404,14 @@ export class SqlCellStatusBarProvider implements NotebookCellStatusBarItemProvid // Build quick pick items from project integrations const items: (QuickPickItem | LocalQuickPickItem)[] = []; - const roster = project.project.integrations || []; - const projectIntegrations = await this.getSelectableIntegrations(cell, roster); + const projectIntegrations = project.project.integrations || []; + const selectableIntegrations = await this.getSelectableIntegrations(cell, projectIntegrations); // Check if current integration is unknown (not in the project's list) const isCurrentIntegrationUnknown = currentIntegrationId && currentIntegrationId !== DATAFRAME_SQL_INTEGRATION_ID && - !projectIntegrations.some((i) => i.id === currentIntegrationId); + !selectableIntegrations.some((i) => i.id === currentIntegrationId); // Add current unknown integration first if it exists if (isCurrentIntegrationUnknown && currentIntegrationId) { @@ -449,7 +425,7 @@ export class SqlCellStatusBarProvider implements NotebookCellStatusBarItemProvid } // Add all project integrations - for (const projectIntegration of projectIntegrations) { + for (const projectIntegration of selectableIntegrations) { const integrationType = projectIntegration.type && (databaseIntegrationTypes as readonly string[]).includes(projectIntegration.type) @@ -461,10 +437,9 @@ export class SqlCellStatusBarProvider implements NotebookCellStatusBarItemProvid continue; } - const typeLabel = - integrationType && (databaseIntegrationTypes as readonly string[]).includes(integrationType) - ? integrationTypeLabels[integrationType] ?? integrationType - : projectIntegration.type; + const typeLabel = isConfigurableDatabaseIntegrationType(projectIntegration.type) + ? localize.Integrations.typeLabel(projectIntegration.type) + : projectIntegration.type; const item: LocalQuickPickItem = { label: projectIntegration.name || projectIntegration.id, @@ -543,13 +518,15 @@ export class SqlCellStatusBarProvider implements NotebookCellStatusBarItemProvid return; } - // Picking a file-declared integration is the one place the roster drifts, so reconcile it here. - const selectedIntegration = projectIntegrations.find((integration) => integration.id === selectedId); + // Picking a file-declared integration is the one place the project's integrations drift, so reconcile + // it here. + const selectedIntegration = selectableIntegrations.find((integration) => integration.id === selectedId); // Shared with the telemetry below so the reconciliation and what it reports cannot drift apart. const fromEnvFile = - selectedIntegration !== undefined && !roster.some((integration) => integration.id === selectedId); + selectedIntegration !== undefined && + !projectIntegrations.some((integration) => integration.id === selectedId); if (selectedIntegration && fromEnvFile) { - await this.addToProjectIntegrations(cell, projectId, roster, selectedIntegration); + await this.addToProjectIntegrations(cell, projectId, projectIntegrations, selectedIntegration); } // Trigger status bar update diff --git a/src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts b/src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts index 5a51ac8194..8e6d11f358 100644 --- a/src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts +++ b/src/notebooks/deepnote/sqlCellStatusBarProvider.unit.test.ts @@ -246,7 +246,7 @@ suite('SqlCellStatusBarProvider', () => { notebookMetadata: { deepnoteProjectId: 'project-1' } }); - // The merge only resolves roster and file ids; a bare merged-first rewrite would regress this to (configure). + // The merge only resolves project and file ids; a bare merged-first rewrite would regress this to (configure). when(integrationStorage.getProjectIntegrationConfig(anything(), anything())).thenResolve({ id: integrationId, name: 'Stored Only', @@ -549,7 +549,7 @@ suite('SqlCellStatusBarProvider', () => { verify(mockedVSCodeNamespaces.window.showQuickPick(anything(), anything())).once(); }); - test('switchSqlIntegration offers `.deepnote.env.yaml` integrations the project roster omits', async () => { + test('switchSqlIntegration offers `.deepnote.env.yaml` integrations the project omits', async () => { let commandHandler: ((cell?: NotebookCell) => Promise) | undefined; when(mockedVSCodeNamespaces.commands.registerCommand('deepnote.switchSqlIntegration', anything())).thenCall( (_name, handler) => { diff --git a/src/notebooks/types.ts b/src/notebooks/types.ts index b543673a46..e24e8c8be9 100644 --- a/src/notebooks/types.ts +++ b/src/notebooks/types.ts @@ -35,6 +35,12 @@ export interface ProjectIntegration { type: ConfigurableDatabaseIntegrationType; } +/** + * An entry as recorded on disk: unlike `ProjectIntegration`, `type` is not narrowed to the known types, so it also + * covers the internal `pandas-dataframe` integration and anything a newer Deepnote release writes. + */ +export type RawProjectIntegration = NonNullable[number]; + export const IDeepnoteNotebookManager = Symbol('IDeepnoteNotebookManager'); export interface IDeepnoteNotebookManager { /** @@ -52,5 +58,5 @@ export interface IDeepnoteNotebookManager { * @param integrations - Array of integration metadata to store in the project * @returns `true` if at least one cached entry was found and updated, `false` otherwise */ - updateProjectIntegrations(projectId: string, integrations: ProjectIntegration[]): boolean; + updateProjectIntegrations(projectId: string, integrations: RawProjectIntegration[]): boolean; } diff --git a/src/platform/analytics/types.ts b/src/platform/analytics/types.ts index 3aeb0c4c96..bba50dd4bd 100644 --- a/src/platform/analytics/types.ts +++ b/src/platform/analytics/types.ts @@ -37,7 +37,6 @@ export type CommandOutcome = 'completed' | 'cancelled' | 'failed'; /** Caller-supplied properties per event; `undefined` means none beyond the common properties the service attaches. */ export interface TelemetryEventProperties { add_block: { blockType: string; isEphemeral: boolean }; - /** "Existing" means configured in another project of this workspace. */ add_existing_integration: { integrationType: string; outcome: CommandOutcome }; authenticate_integration: { integrationType: string; outcome: CommandOutcome }; configure_integration: { integrationType: string }; @@ -75,7 +74,7 @@ export interface TelemetryEventProperties { save_integration: { integrationType: string; authMethod?: 'service-account' | 'google-oauth' }; select_environment: undefined; split_notebook: { notebookCount: number; outcome: CommandOutcome }; - /** `fromEnvFile` is file-ONLY, not file-configured: false when the id is also in the project roster. */ + /** `fromEnvFile` is file-ONLY, not file-configured: false when the id is also in the project's integrations. */ switch_sql_integration: { fromEnvFile: boolean; integrationType: string }; toggle_snapshots: { enabled: boolean }; update_environment: { field: 'name' | 'packages'; packageCount?: number }; diff --git a/src/platform/common/utils/localize.ts b/src/platform/common/utils/localize.ts index 68d2637486..3a12ddb924 100644 --- a/src/platform/common/utils/localize.ts +++ b/src/platform/common/utils/localize.ts @@ -5,6 +5,8 @@ import { l10n } from 'vscode'; import { PythonEnvironment } from '../../pythonEnvironments/info'; import { fromNow } from './date'; import { getPythonEnvDisplayName } from '../../interpreter/helpers'; +import { integrationTypeLabelKey, type IntegrationTypeLabelKey } from '../../notebooks/deepnote/integrationTypeLabels'; +import type { ConfigurableDatabaseIntegrationType } from '../../notebooks/deepnote/integrationTypes'; function getInterpreterDisplayName(interpreter: PythonEnvironment) { return getPythonEnvDisplayName(interpreter); @@ -838,6 +840,10 @@ export namespace Integrations { export const save = l10n.t('Save'); export const addNewIntegration = l10n.t('Add New Integration'); export const addExistingIntegration = l10n.t('Add Existing Integration'); + export const addExistingIntegrationScanning = l10n.t('Looking for integrations in other projects...'); + export const addExistingIntegrationSnapshotUnsupported = l10n.t( + 'Integrations cannot be added to a snapshot file. Open the project notebook and try again.' + ); export const addExistingIntegrationPlaceholder = l10n.t( 'Select an integration configured in another project of this workspace' ); @@ -864,26 +870,32 @@ export namespace Integrations { export const defaultName = (type: string) => l10n.t('My {0} integration', type); export const unsupportedIntegrationType = (type: string) => l10n.t('Unsupported integration type: {0}', type); - // Integration type labels - export const postgresTypeLabel = l10n.t('PostgreSQL'); - export const bigQueryTypeLabel = l10n.t('Google BigQuery'); - export const snowflakeTypeLabel = l10n.t('Snowflake'); - export const alloyDBTypeLabel = l10n.t('Google AlloyDB'); - export const athenaTypeLabel = l10n.t('Amazon Athena'); - export const clickHouseTypeLabel = l10n.t('ClickHouse'); - export const cloudSqlTypeLabel = l10n.t('Google Cloud SQL'); - export const databricksTypeLabel = l10n.t('Databricks'); - export const dremioTypeLabel = l10n.t('Dremio'); - export const mariaDBTypeLabel = l10n.t('MariaDB'); - export const materializeTypeLabel = l10n.t('Materialize'); - export const mindsDBTypeLabel = l10n.t('MindsDB'); - export const mongoDBTypeLabel = l10n.t('MongoDB'); - export const mySQLTypeLabel = l10n.t('MySQL'); - export const duckDBTypeLabel = l10n.t('DuckDB'); - export const redshiftTypeLabel = l10n.t('Amazon Redshift'); - export const spannerTypeLabel = l10n.t('Google Spanner'); - export const sqlServerTypeLabel = l10n.t('Microsoft SQL Server'); - export const trinoTypeLabel = l10n.t('Trino'); + /** + * The one place integration type labels are written down. Keyed by bundle key so `integrationWebview` can + * spread it into the bundle whole; `l10n.t` extracts literal arguments only, so these stay literal. + */ + export const typeLabels = { + 'integrationType.alloydb': l10n.t('Google AlloyDB'), + 'integrationType.athena': l10n.t('Amazon Athena'), + 'integrationType.big-query': l10n.t('Google BigQuery'), + 'integrationType.clickhouse': l10n.t('ClickHouse'), + 'integrationType.cloud-sql': l10n.t('Google Cloud SQL'), + 'integrationType.databricks': l10n.t('Databricks'), + 'integrationType.dremio': l10n.t('Dremio'), + 'integrationType.mariadb': l10n.t('MariaDB'), + 'integrationType.materialize': l10n.t('Materialize'), + 'integrationType.mindsdb': l10n.t('MindsDB'), + 'integrationType.mongodb': l10n.t('MongoDB'), + 'integrationType.mysql': l10n.t('MySQL'), + 'integrationType.pgsql': l10n.t('PostgreSQL'), + 'integrationType.redshift': l10n.t('Amazon Redshift'), + 'integrationType.snowflake': l10n.t('Snowflake'), + 'integrationType.spanner': l10n.t('Google Spanner'), + 'integrationType.sql-server': l10n.t('Microsoft SQL Server'), + 'integrationType.trino': l10n.t('Trino') + } satisfies Record; + + export const typeLabel = (type: ConfigurableDatabaseIntegrationType) => typeLabels[integrationTypeLabelKey(type)]; // PostgreSQL form strings export const postgresNameLabel = l10n.t('Name (optional)'); diff --git a/src/platform/notebooks/deepnote/integrationTypeLabels.ts b/src/platform/notebooks/deepnote/integrationTypeLabels.ts new file mode 100644 index 0000000000..edc1625fa3 --- /dev/null +++ b/src/platform/notebooks/deepnote/integrationTypeLabels.ts @@ -0,0 +1,13 @@ +import type { ConfigurableDatabaseIntegrationType } from './integrationTypes'; + +/** + * Where an integration type's display label sits in the string bundle the extension host sends the webview. + * + * Derived from the type rather than listed, so `localize.Integrations.typeLabels` is the only place the labels + * themselves are written down — the webview is bundled separately and cannot read `localize.ts` directly. + */ +export type IntegrationTypeLabelKey = `integrationType.${ConfigurableDatabaseIntegrationType}`; + +export function integrationTypeLabelKey(type: ConfigurableDatabaseIntegrationType): IntegrationTypeLabelKey { + return `integrationType.${type}`; +} diff --git a/src/platform/notebooks/deepnote/integrationTypeLabels.unit.test.ts b/src/platform/notebooks/deepnote/integrationTypeLabels.unit.test.ts new file mode 100644 index 0000000000..f2c72238ae --- /dev/null +++ b/src/platform/notebooks/deepnote/integrationTypeLabels.unit.test.ts @@ -0,0 +1,25 @@ +import { databaseIntegrationTypes } from '@deepnote/database-integrations'; +import { assert } from 'chai'; + +import * as localize from '../../common/utils/localize'; +import { integrationTypeLabelKey } from './integrationTypeLabels'; +import { isConfigurableDatabaseIntegrationType } from './integrationTypes'; + +suite('integrationTypeLabels', () => { + // The webview asks the string bundle for exactly this key, so its shape is a cross-process contract. + test('derives the bundle key from the integration type', () => { + assert.strictEqual(integrationTypeLabelKey('pgsql'), 'integrationType.pgsql'); + assert.strictEqual(integrationTypeLabelKey('sql-server'), 'integrationType.sql-server'); + }); + + test('resolves a label for every configurable type', () => { + assert.strictEqual(localize.Integrations.typeLabel('pgsql'), 'PostgreSQL'); + assert.strictEqual(localize.Integrations.typeLabel('big-query'), 'Google BigQuery'); + + const configurable = databaseIntegrationTypes.filter(isConfigurableDatabaseIntegrationType); + const unresolved = configurable.filter((type) => !localize.Integrations.typeLabel(type)); + + assert.deepStrictEqual(unresolved, [], 'every configurable type needs a label the panel can show'); + assert.strictEqual(Object.keys(localize.Integrations.typeLabels).length, configurable.length); + }); +}); diff --git a/src/webviews/webview-side/integrations/ConfigurationForm.tsx b/src/webviews/webview-side/integrations/ConfigurationForm.tsx index 1799d71eb7..893451c18e 100644 --- a/src/webviews/webview-side/integrations/ConfigurationForm.tsx +++ b/src/webviews/webview-side/integrations/ConfigurationForm.tsx @@ -19,7 +19,7 @@ import { SpannerForm } from './SpannerForm'; import { SQLServerForm } from './SQLServerForm'; import { isTrinoPasswordConfig, TrinoForm } from './TrinoForm'; import { ConfigurableDatabaseIntegrationConfig, ConfigurableDatabaseIntegrationType } from './types'; -import { integrationTypeLabels } from './integrationUtils'; +import { integrationTypeLabel } from './integrationUtils'; export interface IConfigurationFormProps { integrationId: string; @@ -38,7 +38,7 @@ export const ConfigurationForm: React.FC = ({ onSave, onCancel }) => { - const typeLabel = integrationTypeLabels[integrationType] || integrationType; + const typeLabel = integrationTypeLabel(integrationType); const title = getLocString('integrationsConfigureTitle', '{0} integration').replace('{0}', typeLabel); return ( diff --git a/src/webviews/webview-side/integrations/IntegrationItem.tsx b/src/webviews/webview-side/integrations/IntegrationItem.tsx index 02607c1156..02d9a31d34 100644 --- a/src/webviews/webview-side/integrations/IntegrationItem.tsx +++ b/src/webviews/webview-side/integrations/IntegrationItem.tsx @@ -1,8 +1,8 @@ import * as React from 'react'; import { getLocString } from '../react-common/locReactSide'; -import { ConfigurableDatabaseIntegrationType, DetectedIntegration } from './types'; -import { integrationTypeIcons } from './integrationUtils'; +import { DetectedIntegration } from './types'; +import { integrationTypeIcons, integrationTypeLabel } from './integrationUtils'; export interface IIntegrationItemProps { integration: DetectedIntegration; @@ -13,49 +13,6 @@ export interface IIntegrationItemProps { onSignOut: (integrationId: string) => void; } -const getIntegrationTypeLabel = (type: ConfigurableDatabaseIntegrationType): string => { - switch (type) { - case 'alloydb': - return getLocString('integrationsAlloyDBTypeLabel', 'Google AlloyDB'); - case 'athena': - return getLocString('integrationsAthenaTypeLabel', 'Amazon Athena'); - case 'big-query': - return getLocString('integrationsBigQueryTypeLabel', 'Google BigQuery'); - case 'clickhouse': - return getLocString('integrationsClickHouseTypeLabel', 'ClickHouse'); - case 'cloud-sql': - return getLocString('integrationsCloudSqlTypeLabel', 'Google Cloud SQL'); - case 'databricks': - return getLocString('integrationsDatabricksTypeLabel', 'Databricks'); - case 'dremio': - return getLocString('integrationsDremioTypeLabel', 'Dremio'); - case 'mariadb': - return getLocString('integrationsMariaDBTypeLabel', 'MariaDB'); - case 'materialize': - return getLocString('integrationsMaterializeTypeLabel', 'Materialize'); - case 'mindsdb': - return getLocString('integrationsMindsDBTypeLabel', 'MindsDB'); - case 'mongodb': - return getLocString('integrationsMongoDBTypeLabel', 'MongoDB'); - case 'mysql': - return getLocString('integrationsMySQLTypeLabel', 'MySQL'); - case 'pgsql': - return getLocString('integrationsPostgresTypeLabel', 'PostgreSQL'); - case 'redshift': - return getLocString('integrationsRedshiftTypeLabel', 'Amazon Redshift'); - case 'snowflake': - return getLocString('integrationsSnowflakeTypeLabel', 'Snowflake'); - case 'spanner': - return getLocString('integrationsSpannerTypeLabel', 'Google Spanner'); - case 'sql-server': - return getLocString('integrationsSQLServerTypeLabel', 'Microsoft SQL Server'); - case 'trino': - return getLocString('integrationsTrinoTypeLabel', 'Trino'); - default: - return type; - } -}; - export const IntegrationItem: React.FC = ({ integration, onConfigure, @@ -86,7 +43,7 @@ export const IntegrationItem: React.FC = ({ const type = integration.config?.type || integration.integrationType; // Get the type label and icon - const typeLabel = type ? getIntegrationTypeLabel(type) : undefined; + const typeLabel = type ? integrationTypeLabel(type) : undefined; const typeIcon = type ? integrationTypeIcons[type] : undefined; // Federated-auth UI: `tokenStatus` alone decides. The extension gates on its candidate set, which also diff --git a/src/webviews/webview-side/integrations/IntegrationTypeSelector.tsx b/src/webviews/webview-side/integrations/IntegrationTypeSelector.tsx index f62d5b409d..d85a5377ae 100644 --- a/src/webviews/webview-side/integrations/IntegrationTypeSelector.tsx +++ b/src/webviews/webview-side/integrations/IntegrationTypeSelector.tsx @@ -1,7 +1,7 @@ import * as React from 'react'; import { getLocString } from '../react-common/locReactSide'; import { ConfigurableDatabaseIntegrationType } from './types'; -import { integrationTypeLabels, integrationTypeIcons } from './integrationUtils'; +import { integrationTypeIcons, integrationTypeLabel } from './integrationUtils'; export interface IIntegrationTypeSelectorProps { /** Opens the extension-host picker of integrations other projects already configured. */ @@ -9,108 +9,29 @@ export interface IIntegrationTypeSelectorProps { onSelectType: (type: ConfigurableDatabaseIntegrationType) => void; } -interface IntegrationTypeInfo { - type: ConfigurableDatabaseIntegrationType; - label: string; - icon: string; -} - -// Data Warehouses & Lakes -const WAREHOUSE_INTEGRATION_TYPES: IntegrationTypeInfo[] = [ - { - type: 'clickhouse', - label: integrationTypeLabels['clickhouse'], - icon: integrationTypeIcons['clickhouse'] - }, - { - type: 'redshift', - label: integrationTypeLabels['redshift'], - icon: integrationTypeIcons['redshift'] - }, - { - type: 'athena', - label: integrationTypeLabels['athena'], - icon: integrationTypeIcons['athena'] - }, - { - type: 'big-query', - label: integrationTypeLabels['big-query'], - icon: integrationTypeIcons['big-query'] - }, - { - type: 'snowflake', - label: integrationTypeLabels['snowflake'], - icon: integrationTypeIcons['snowflake'] - }, - { - type: 'databricks', - label: integrationTypeLabels['databricks'], - icon: integrationTypeIcons['databricks'] - }, - { - type: 'dremio', - label: integrationTypeLabels['dremio'], - icon: integrationTypeIcons['dremio'] - }, - { - type: 'trino', - label: integrationTypeLabels['trino'], - icon: integrationTypeIcons['trino'] - } +// Display order, not the map's alphabetical order. +const WAREHOUSE_INTEGRATION_TYPES: ConfigurableDatabaseIntegrationType[] = [ + 'clickhouse', + 'redshift', + 'athena', + 'big-query', + 'snowflake', + 'databricks', + 'dremio', + 'trino' ]; -// Databases -const DATABASE_INTEGRATION_TYPES: IntegrationTypeInfo[] = [ - { - type: 'mongodb', - label: integrationTypeLabels['mongodb'], - icon: integrationTypeIcons['mongodb'] - }, - { - type: 'pgsql', - label: integrationTypeLabels['pgsql'], - icon: integrationTypeIcons['pgsql'] - }, - { - type: 'mysql', - label: integrationTypeLabels['mysql'], - icon: integrationTypeIcons['mysql'] - }, - { - type: 'mariadb', - label: integrationTypeLabels['mariadb'], - icon: integrationTypeIcons['mariadb'] - }, - { - type: 'sql-server', - label: integrationTypeLabels['sql-server'], - icon: integrationTypeIcons['sql-server'] - }, - { - type: 'alloydb', - label: integrationTypeLabels['alloydb'], - icon: integrationTypeIcons['alloydb'] - }, - { - type: 'spanner', - label: integrationTypeLabels['spanner'], - icon: integrationTypeIcons['spanner'] - }, - { - type: 'cloud-sql', - label: integrationTypeLabels['cloud-sql'], - icon: integrationTypeIcons['cloud-sql'] - }, - { - type: 'materialize', - label: integrationTypeLabels['materialize'], - icon: integrationTypeIcons['materialize'] - }, - { - type: 'mindsdb', - label: integrationTypeLabels['mindsdb'], - icon: integrationTypeIcons['mindsdb'] - } +const DATABASE_INTEGRATION_TYPES: ConfigurableDatabaseIntegrationType[] = [ + 'mongodb', + 'pgsql', + 'mysql', + 'mariadb', + 'sql-server', + 'alloydb', + 'spanner', + 'cloud-sql', + 'materialize', + 'mindsdb' ]; export const IntegrationTypeSelector: React.FC = ({ onAddExisting, onSelectType }) => { @@ -128,38 +49,46 @@ export const IntegrationTypeSelector: React.FC = {getLocString('integrationsDataWarehousesLakes', 'Data Warehouses & Lakes')}

- {WAREHOUSE_INTEGRATION_TYPES.map((integrationInfo) => ( - - ))} + {WAREHOUSE_INTEGRATION_TYPES.map((type) => { + const label = integrationTypeLabel(type); + + return ( + + ); + })}

{getLocString('integrationsDatabases', 'Databases')}

- {DATABASE_INTEGRATION_TYPES.map((integrationInfo) => ( - - ))} + {DATABASE_INTEGRATION_TYPES.map((type) => { + const label = integrationTypeLabel(type); + + return ( + + ); + })}
diff --git a/src/webviews/webview-side/integrations/integrationUtils.ts b/src/webviews/webview-side/integrations/integrationUtils.ts index cd329c8490..4538e1cf72 100644 --- a/src/webviews/webview-side/integrations/integrationUtils.ts +++ b/src/webviews/webview-side/integrations/integrationUtils.ts @@ -1,3 +1,4 @@ +import { integrationTypeLabelKey } from '../../../platform/notebooks/deepnote/integrationTypeLabels'; import { getLocString } from '../react-common/locReactSide'; import { ConfigurableDatabaseIntegrationType } from './types'; @@ -23,27 +24,13 @@ const mindsdbLogo: string = require('./icons/mindsdb.svg'); const trinoLogo: string = require('./icons/trino.svg'); /* eslint-enable @typescript-eslint/no-require-imports */ -// Localized labels for integration types (duplicated from sqlCellStatusBarProvider.ts due to import restrictions) -export const integrationTypeLabels: Record = { - alloydb: 'Google AlloyDB', - athena: 'Amazon Athena', - 'big-query': 'Google BigQuery', - clickhouse: 'ClickHouse', - 'cloud-sql': 'Google Cloud SQL', - databricks: 'Databricks', - dremio: 'Dremio', - mariadb: 'MariaDB', - materialize: 'Materialize', - mindsdb: 'MindsDB', - mongodb: 'MongoDB', - mysql: 'MySQL', - pgsql: 'PostgreSQL', - redshift: 'Amazon Redshift', - snowflake: 'Snowflake', - spanner: 'Google Spanner', - 'sql-server': 'Microsoft SQL Server', - trino: 'Trino' -}; +/** + * The panel is bundled separately and cannot reach `localize.ts`, so labels resolve against the string bundle. + * The host sends that bundle before the first `update`, so the raw type is a fallback that should never render. + */ +export function integrationTypeLabel(type: ConfigurableDatabaseIntegrationType): string { + return getLocString(integrationTypeLabelKey(type), type); +} // Icon mapping for integration types export const integrationTypeIcons: Record = { @@ -73,6 +60,6 @@ export const integrationTypeIcons: Record