diff --git a/build/lib/stylelint/vscode-known-variables.json b/build/lib/stylelint/vscode-known-variables.json index b86fad6c43cf..5406845da14b 100644 --- a/build/lib/stylelint/vscode-known-variables.json +++ b/build/lib/stylelint/vscode-known-variables.json @@ -705,6 +705,7 @@ "--vscode-positronModalDialog-buttonBorder", "--vscode-positronModalDialog-buttonDestructiveBackground", "--vscode-positronModalDialog-buttonDestructiveForeground", + "--vscode-positronModalDialog-buttonDestructiveHoverBackground", "--vscode-positronModalDialog-buttonDisabledBackground", "--vscode-positronModalDialog-buttonDisabledBorder", "--vscode-positronModalDialog-buttonDisabledForeground", diff --git a/extensions/positron-data-driver-duckdb/src/duckdbConnection.ts b/extensions/positron-data-driver-duckdb/src/duckdbConnection.ts index 6d3cf3157ab1..a17a6defcdb0 100644 --- a/extensions/positron-data-driver-duckdb/src/duckdbConnection.ts +++ b/extensions/positron-data-driver-duckdb/src/duckdbConnection.ts @@ -95,9 +95,10 @@ export class DuckDBConnection implements positron.DataConnection, IDuckDBPreview /** * Opens the given table or view in the Data Explorer. Registers a table view with the RPC * handler under a stable per-connection dataset id, then asks Positron to open (or focus) the - * explorer backed by this extension's provider. + * explorer backed by this extension's provider. Returns the dataset id it was opened under, which + * Positron uses to tell that this connection has a Data Explorer open on it. */ - async previewObject(schemaName: string, tableName: string, kind: 'table' | 'view'): Promise { + async previewObject(schemaName: string, tableName: string, kind: 'table' | 'view'): Promise { this._ensureConnected(); const datasetId = `duckdbconn:${this._connectionId}:${kind}:${schemaName}.${tableName}`; await this._dataExplorerHandler.openTableView(datasetId, this._lease!.client, schemaName, tableName, kind); @@ -107,13 +108,15 @@ export class DuckDBConnection implements positron.DataConnection, IDuckDBPreview datasetId, displayName: tableName, }); + return datasetId; } /** * Opens a single column of the given table or view in the Data Explorer as a one-column grid. - * Uses a dataset id distinct from the table's so both can be open at once. + * Uses a dataset id distinct from the table's so both can be open at once. Returns the dataset id + * it was opened under. */ - async previewColumn(schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise { + async previewColumn(schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise { this._ensureConnected(); const datasetId = `duckdbconn:${this._connectionId}:column:${schemaName}.${tableName}.${columnName}`; await this._dataExplorerHandler.openColumnView(datasetId, this._lease!.client, schemaName, tableName, kind, columnName); @@ -123,6 +126,7 @@ export class DuckDBConnection implements positron.DataConnection, IDuckDBPreview datasetId, displayName: `${tableName}.${columnName}`, }); + return datasetId; } /** Returns whether this connection was opened in read-only mode. */ diff --git a/extensions/positron-data-driver-duckdb/src/duckdbNodes.ts b/extensions/positron-data-driver-duckdb/src/duckdbNodes.ts index e0baa54e110c..5f82f5452f85 100644 --- a/extensions/positron-data-driver-duckdb/src/duckdbNodes.ts +++ b/extensions/positron-data-driver-duckdb/src/duckdbNodes.ts @@ -11,10 +11,10 @@ import { IDuckDBQueryClient } from 'positron-data-explorer-duckdb'; * DuckDBConnection, which owns the worker client and the dataset registration. */ export interface IDuckDBPreviewHost { - /** Opens the given table or view in the Data Explorer. */ - previewObject(schemaName: string, tableName: string, kind: 'table' | 'view'): Promise; - /** Opens a single column of the given table or view in the Data Explorer. */ - previewColumn(schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise; + /** Opens the given table or view in the Data Explorer, returning its dataset id. */ + previewObject(schemaName: string, tableName: string, kind: 'table' | 'view'): Promise; + /** Opens a single column of the given table or view in the Data Explorer, returning its dataset id. */ + previewColumn(schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise; } /** diff --git a/extensions/positron-data-driver-pins/src/pinsConnection.ts b/extensions/positron-data-driver-pins/src/pinsConnection.ts index f59397b3d20f..7eef25fd1993 100644 --- a/extensions/positron-data-driver-pins/src/pinsConnection.ts +++ b/extensions/positron-data-driver-pins/src/pinsConnection.ts @@ -160,8 +160,12 @@ export class PinsConnection implements positron.DataConnection, IPinsBrowseHost * its data file, downloads that file (reusing the cached copy when present), loads it into the * DuckDB worker as a table, and opens the explorer over it. Convert-to-Code in the resulting * explorer emits `pin_read` code rather than SQL against the throwaway table. + * + * Returns the dataset id the explorer was opened under, which Positron uses to tell that this + * connection has a Data Explorer open on it, or undefined when a disconnect raced the preview and + * nothing was opened. */ - async previewPin(pin: PinInfo, bundleId: string, isActiveVersion: boolean): Promise { + async previewPin(pin: PinInfo, bundleId: string, isActiveVersion: boolean): Promise { this._ensureConnected(); // Resolve the version's data file and confirm it is a previewable, single-file tabular type. @@ -226,6 +230,7 @@ export class PinsConnection implements positron.DataConnection, IPinsBrowseHost this._logger.info(`Opening ${displayName} in the Data Explorer`); await positron.dataExplorer.open({ providerId: PINS_DATA_EXPLORER_PROVIDER_ID, datasetId, displayName }); + return datasetId; } /** Marks the connection disconnected, releases previewed views, and closes the DuckDB worker. */ diff --git a/extensions/positron-data-driver-pins/src/pinsNodes.ts b/extensions/positron-data-driver-pins/src/pinsNodes.ts index c795ea36d2c4..82e0e3b2b150 100644 --- a/extensions/positron-data-driver-pins/src/pinsNodes.ts +++ b/extensions/positron-data-driver-pins/src/pinsNodes.ts @@ -44,8 +44,10 @@ export interface IPinsBrowseHost { * @param bundleId The bundle (version) id whose data to preview. * @param isActiveVersion Whether `bundleId` is the pin's active version; the active version reads * as the latest, so its generated code omits the explicit `version` argument. + * @returns The dataset id the explorer was opened under, or undefined when a disconnect raced the + * preview and nothing was opened. */ - previewPin(pin: PinInfo, bundleId: string, isActiveVersion: boolean): Promise; + previewPin(pin: PinInfo, bundleId: string, isActiveVersion: boolean): Promise; } /** diff --git a/extensions/positron-data-driver-postgresql/src/postgresqlConnection.ts b/extensions/positron-data-driver-postgresql/src/postgresqlConnection.ts index fd7535a7259d..5bc7028d5c1e 100644 --- a/extensions/positron-data-driver-postgresql/src/postgresqlConnection.ts +++ b/extensions/positron-data-driver-postgresql/src/postgresqlConnection.ts @@ -311,9 +311,10 @@ export class PostgreSQLConnection implements positron.DataConnection, IPostgresC * handler under a stable per-connection dataset id, then asks Positron to open (or focus) the * explorer backed by this extension's provider. `client` is the client the object's node was built * against and `database` is the database it lives in (undefined in single-database mode), so the - * dataset id and query client match the right database. + * dataset id and query client match the right database. Returns the dataset id it was opened + * under, which Positron uses to tell that this connection has a Data Explorer open on it. */ - async previewObject(client: PostgreSQLClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise { + async previewObject(client: PostgreSQLClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise { this._ensureConnected(); const datasetId = `postgresql:${this._connectionId}:${database ?? ''}:${kind}:${schemaName}.${tableName}`; await this._dataExplorerHandler.openTableView(datasetId, this._queryClient(client), schemaName, tableName, kind); @@ -323,13 +324,15 @@ export class PostgreSQLConnection implements positron.DataConnection, IPostgresC datasetId, displayName: tableName, }); + return datasetId; } /** * Opens a single column of the given table or view in the Data Explorer as a one-column grid. - * Uses a dataset id distinct from the table's so both can be open at once. + * Uses a dataset id distinct from the table's so both can be open at once. Returns the dataset id + * it was opened under. */ - async previewColumn(client: PostgreSQLClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise { + async previewColumn(client: PostgreSQLClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise { this._ensureConnected(); const datasetId = `postgresql:${this._connectionId}:${database ?? ''}:column:${schemaName}.${tableName}.${columnName}`; await this._dataExplorerHandler.openColumnView(datasetId, this._queryClient(client), schemaName, tableName, kind, columnName); @@ -339,6 +342,7 @@ export class PostgreSQLConnection implements positron.DataConnection, IPostgresC datasetId, displayName: `${tableName}.${columnName}`, }); + return datasetId; } /** A query client over the given pg client, for the Data Explorer table views. */ diff --git a/extensions/positron-data-driver-postgresql/src/postgresqlNodes.ts b/extensions/positron-data-driver-postgresql/src/postgresqlNodes.ts index 308456463b38..899fdbd13508 100644 --- a/extensions/positron-data-driver-postgresql/src/postgresqlNodes.ts +++ b/extensions/positron-data-driver-postgresql/src/postgresqlNodes.ts @@ -14,10 +14,10 @@ import { PostgreSQLClient } from './postgresqlClient.js'; * so previewed datasets stay unique across databases. */ export interface IPostgresPreviewHost { - /** Opens the given table or view in the Data Explorer. */ - previewObject(client: PostgreSQLClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise; - /** Opens a single column of the given table or view in the Data Explorer. */ - previewColumn(client: PostgreSQLClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise; + /** Opens the given table or view in the Data Explorer, returning its dataset id. */ + previewObject(client: PostgreSQLClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise; + /** Opens a single column of the given table or view in the Data Explorer, returning its dataset id. */ + previewColumn(client: PostgreSQLClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise; } /** diff --git a/extensions/positron-data-driver-postgresql/src/test/postgresqlDriver.test.ts b/extensions/positron-data-driver-postgresql/src/test/postgresqlDriver.test.ts index c4a7f412bb81..7939c5c4641f 100644 --- a/extensions/positron-data-driver-postgresql/src/test/postgresqlDriver.test.ts +++ b/extensions/positron-data-driver-postgresql/src/test/postgresqlDriver.test.ts @@ -24,8 +24,8 @@ const TEST_CONFIG: PostgreSQLConnectionConfig = { // handler would register a vscode command that collides with the activated extension's. One object // satisfies both the connection's host interface and the node-builder's preview-host interface. const noopHost = { - previewObject: async () => { }, - previewColumn: async () => { }, + previewObject: async () => 'noop-dataset', + previewColumn: async () => 'noop-dataset', openTableView: async () => { }, openColumnView: async () => { }, closeTableView: () => { }, diff --git a/extensions/positron-data-driver-redshift/src/redshiftConnection.ts b/extensions/positron-data-driver-redshift/src/redshiftConnection.ts index 19581db56366..6059001dd097 100644 --- a/extensions/positron-data-driver-redshift/src/redshiftConnection.ts +++ b/extensions/positron-data-driver-redshift/src/redshiftConnection.ts @@ -109,9 +109,10 @@ export class RedshiftConnection implements positron.DataConnection, IRedshiftPre /** * Opens the given table or view in the Data Explorer. Registers a table view with the RPC handler * under a stable per-connection dataset id, then asks Positron to open (or focus) the explorer - * backed by this extension's provider. + * backed by this extension's provider. Returns the dataset id it was opened under, which Positron + * uses to tell that this connection has a Data Explorer open on it. */ - async previewObject(client: RedshiftClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise { + async previewObject(client: RedshiftClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise { this._ensureConnected(); const datasetId = `redshift:${this._connectionId}:${database ?? ''}:${kind}:${schemaName}.${tableName}`; await this._dataExplorerHandler.openTableView(datasetId, this._queryClient(client), database, schemaName, tableName, kind); @@ -121,13 +122,15 @@ export class RedshiftConnection implements positron.DataConnection, IRedshiftPre datasetId, displayName: tableName, }); + return datasetId; } /** * Opens a single column of the given table or view in the Data Explorer as a one-column grid. - * Uses a dataset id distinct from the table's so both can be open at once. + * Uses a dataset id distinct from the table's so both can be open at once. Returns the dataset id + * it was opened under. */ - async previewColumn(client: RedshiftClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise { + async previewColumn(client: RedshiftClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise { this._ensureConnected(); const datasetId = `redshift:${this._connectionId}:${database ?? ''}:column:${schemaName}.${tableName}.${columnName}`; await this._dataExplorerHandler.openColumnView(datasetId, this._queryClient(client), database, schemaName, tableName, kind, columnName); @@ -137,6 +140,7 @@ export class RedshiftConnection implements positron.DataConnection, IRedshiftPre datasetId, displayName: `${tableName}.${columnName}`, }); + return datasetId; } /** A query client over the given pg client, for the Data Explorer table views. */ diff --git a/extensions/positron-data-driver-redshift/src/redshiftNodes.ts b/extensions/positron-data-driver-redshift/src/redshiftNodes.ts index d7aa86944bab..f95e019e1c55 100644 --- a/extensions/positron-data-driver-redshift/src/redshiftNodes.ts +++ b/extensions/positron-data-driver-redshift/src/redshiftNodes.ts @@ -32,10 +32,10 @@ const SYSTEM_SCHEMAS_SQL = SYSTEM_SCHEMAS.map(s => `'${s}'`).join(', '); * database in single-database mode), so cross-database previews use a three-part reference. */ export interface IRedshiftPreviewHost { - /** Opens the given table or view in the Data Explorer. */ - previewObject(client: RedshiftClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise; - /** Opens a single column of the given table or view in the Data Explorer. */ - previewColumn(client: RedshiftClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise; + /** Opens the given table or view in the Data Explorer, returning its dataset id. */ + previewObject(client: RedshiftClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise; + /** Opens a single column of the given table or view in the Data Explorer, returning its dataset id. */ + previewColumn(client: RedshiftClient, database: string | undefined, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise; } // --- Single-database family (connected database, information_schema) --- diff --git a/extensions/positron-data-driver-redshift/src/test/redshiftDriver.test.ts b/extensions/positron-data-driver-redshift/src/test/redshiftDriver.test.ts index 01afe0afbe14..582f190af699 100644 --- a/extensions/positron-data-driver-redshift/src/test/redshiftDriver.test.ts +++ b/extensions/positron-data-driver-redshift/src/test/redshiftDriver.test.ts @@ -25,8 +25,8 @@ const TEST_CONFIG: RedshiftConnectionConfig = { // handler would register a vscode command that collides with the activated extension's. One object // satisfies both the connection's host interface and the node-builder's preview-host interface. const noopHost = { - previewObject: async () => { }, - previewColumn: async () => { }, + previewObject: async () => 'noop-dataset', + previewColumn: async () => 'noop-dataset', openTableView: async () => { }, openColumnView: async () => { }, closeTableView: () => { }, diff --git a/extensions/positron-data-driver-snowflake/src/snowflakeConnection.ts b/extensions/positron-data-driver-snowflake/src/snowflakeConnection.ts index b4ec25e476cc..25734ccbc225 100644 --- a/extensions/positron-data-driver-snowflake/src/snowflakeConnection.ts +++ b/extensions/positron-data-driver-snowflake/src/snowflakeConnection.ts @@ -85,9 +85,10 @@ export class SnowflakeConnection implements positron.DataConnection, ISnowflakeP /** * Opens the given table or view in the Data Explorer. Registers a table view with the RPC handler * under a stable per-connection dataset id, then asks Positron to open (or focus) the explorer - * backed by this extension's provider. + * backed by this extension's provider. Returns the dataset id it was opened under, which Positron + * uses to tell that this connection has a Data Explorer open on it. */ - async previewObject(client: SnowflakeClient, database: string, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise { + async previewObject(client: SnowflakeClient, database: string, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise { this._ensureConnected(); const datasetId = datasetKey(this._connectionId, kind, database, schemaName, tableName); await this._dataExplorerHandler.openTableView(datasetId, this._queryClient(client), database, schemaName, tableName, kind); @@ -97,13 +98,15 @@ export class SnowflakeConnection implements positron.DataConnection, ISnowflakeP datasetId, displayName: tableName, }); + return datasetId; } /** * Opens a single column of the given table or view in the Data Explorer as a one-column grid. - * Uses a dataset id distinct from the table's so both can be open at once. + * Uses a dataset id distinct from the table's so both can be open at once. Returns the dataset id + * it was opened under. */ - async previewColumn(client: SnowflakeClient, database: string, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise { + async previewColumn(client: SnowflakeClient, database: string, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise { this._ensureConnected(); const datasetId = datasetKey(this._connectionId, 'column', database, schemaName, tableName, columnName); await this._dataExplorerHandler.openColumnView(datasetId, this._queryClient(client), database, schemaName, tableName, kind, columnName); @@ -113,6 +116,7 @@ export class SnowflakeConnection implements positron.DataConnection, ISnowflakeP datasetId, displayName: `${tableName}.${columnName}`, }); + return datasetId; } /** A query client over the given sdk client, for the Data Explorer table views. */ diff --git a/extensions/positron-data-driver-snowflake/src/snowflakeNodes.ts b/extensions/positron-data-driver-snowflake/src/snowflakeNodes.ts index d4666d5a8a3a..9930a081212d 100644 --- a/extensions/positron-data-driver-snowflake/src/snowflakeNodes.ts +++ b/extensions/positron-data-driver-snowflake/src/snowflakeNodes.ts @@ -36,10 +36,10 @@ function schemaRef(database: string, schemaName: string): string { * against; `database` is the database the object lives in, so previews use a three-part reference. */ export interface ISnowflakePreviewHost { - /** Opens the given table or view in the Data Explorer. */ - previewObject(client: SnowflakeClient, database: string, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise; - /** Opens a single column of the given table or view in the Data Explorer. */ - previewColumn(client: SnowflakeClient, database: string, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise; + /** Opens the given table or view in the Data Explorer, returning its dataset id. */ + previewObject(client: SnowflakeClient, database: string, schemaName: string, tableName: string, kind: 'table' | 'view'): Promise; + /** Opens a single column of the given table or view in the Data Explorer, returning its dataset id. */ + previewColumn(client: SnowflakeClient, database: string, schemaName: string, tableName: string, kind: 'table' | 'view', columnName: string): Promise; } /** diff --git a/extensions/positron-data-driver-snowflake/src/test/snowflakeDriver.test.ts b/extensions/positron-data-driver-snowflake/src/test/snowflakeDriver.test.ts index 19cf8cfcdee7..86955376e22d 100644 --- a/extensions/positron-data-driver-snowflake/src/test/snowflakeDriver.test.ts +++ b/extensions/positron-data-driver-snowflake/src/test/snowflakeDriver.test.ts @@ -21,8 +21,8 @@ const TEST_CONFIG: SnowflakeConnectionConfig = { // handler would register a vscode command that collides with the activated extension's. One object // satisfies both the connection's host interface and the node-builder's preview-host interface. const noopHost = { - previewObject: async () => { }, - previewColumn: async () => { }, + previewObject: async () => 'noop-dataset', + previewColumn: async () => 'noop-dataset', openTableView: async () => { }, openColumnView: async () => { }, closeTableView: () => { }, @@ -353,7 +353,7 @@ suite('Snowflake Driver Tests', () => { // building the node through createSchemaNode must carry the database, schema, name, and kind // through to previewObject. const calls: unknown[][] = []; - const recordingHost = { ...noopHost, previewObject: async (...args: unknown[]) => { calls.push(args); } }; + const recordingHost = { ...noopHost, previewObject: async (...args: unknown[]) => { calls.push(args); return 'noop-dataset'; } }; const schemaNode = createSchemaNode(mock, recordingHost, 'ANALYTICS', 'PUBLIC'); const tables = await tablesOf(schemaNode); diff --git a/extensions/positron-data-driver-sqlite/src/sqliteConnection.ts b/extensions/positron-data-driver-sqlite/src/sqliteConnection.ts index 85542c2bcd89..263f0dbf9b1c 100644 --- a/extensions/positron-data-driver-sqlite/src/sqliteConnection.ts +++ b/extensions/positron-data-driver-sqlite/src/sqliteConnection.ts @@ -92,9 +92,10 @@ export class SQLiteConnection implements positron.DataConnection, ISqlitePreview /** * Opens the given table or view in the Data Explorer. Registers a table view with the RPC * handler under a stable per-connection dataset id, then asks Positron to open (or focus) the - * explorer backed by this extension's RPC command. + * explorer backed by this extension's RPC command. Returns the dataset id it was opened under, + * which Positron uses to tell that this connection has a Data Explorer open on it. */ - async previewObject(name: string, kind: 'table' | 'view'): Promise { + async previewObject(name: string, kind: 'table' | 'view'): Promise { this._ensureConnected(); const datasetId = `sqlite:${this._connectionId}:${kind}:${name}`; await this._dataExplorerHandler.openTableView(datasetId, this._client!, name, kind); @@ -104,13 +105,15 @@ export class SQLiteConnection implements positron.DataConnection, ISqlitePreview datasetId, displayName: name, }); + return datasetId; } /** * Opens a single column of the given table or view in the Data Explorer as a one-column grid. - * Uses a dataset id distinct from the table's so both can be open at once. + * Uses a dataset id distinct from the table's so both can be open at once. Returns the dataset id + * it was opened under. */ - async previewColumn(tableName: string, kind: 'table' | 'view', columnName: string): Promise { + async previewColumn(tableName: string, kind: 'table' | 'view', columnName: string): Promise { this._ensureConnected(); const datasetId = `sqlite:${this._connectionId}:column:${tableName}.${columnName}`; await this._dataExplorerHandler.openColumnView(datasetId, this._client!, tableName, kind, columnName); @@ -120,6 +123,7 @@ export class SQLiteConnection implements positron.DataConnection, ISqlitePreview datasetId, displayName: `${tableName}.${columnName}`, }); + return datasetId; } /** Returns whether this connection was opened in read-only mode. */ diff --git a/extensions/positron-data-driver-sqlite/src/sqliteNodes.ts b/extensions/positron-data-driver-sqlite/src/sqliteNodes.ts index bbaed454b9a4..9a9eca8ececc 100644 --- a/extensions/positron-data-driver-sqlite/src/sqliteNodes.ts +++ b/extensions/positron-data-driver-sqlite/src/sqliteNodes.ts @@ -11,10 +11,10 @@ import { ISqliteQueryClient } from './sqliteWorkerClient.js'; * SQLiteConnection, which owns the worker client and the dataset registration. */ export interface ISqlitePreviewHost { - /** Opens the given table or view in the Data Explorer. */ - previewObject(name: string, kind: 'table' | 'view'): Promise; - /** Opens a single column of the given table or view in the Data Explorer. */ - previewColumn(tableName: string, kind: 'table' | 'view', columnName: string): Promise; + /** Opens the given table or view in the Data Explorer, returning its dataset id. */ + previewObject(name: string, kind: 'table' | 'view'): Promise; + /** Opens a single column of the given table or view in the Data Explorer, returning its dataset id. */ + previewColumn(tableName: string, kind: 'table' | 'view', columnName: string): Promise; } /** diff --git a/src/positron-dts/positron.d.ts b/src/positron-dts/positron.d.ts index 4b552d1ddd12..28b8672ca4ed 100644 --- a/src/positron-dts/positron.d.ts +++ b/src/positron-dts/positron.d.ts @@ -2348,8 +2348,14 @@ declare module 'positron' { /** * Preview the data in this node (e.g., SELECT * FROM table LIMIT 100). + * + * Return the dataset id the preview was opened under -- the same `datasetId` passed to + * `positron.dataExplorer.open` -- so Positron can relate the open Data Explorer back to the + * connection it came from. Returning nothing is supported, but Positron then has no way to + * know the connection has an open Data Explorer, and may close the connection while it is + * still in use. */ - preview?(): Thenable; + preview?(): Thenable; } /** diff --git a/src/vs/workbench/api/browser/positron/mainThreadDataConnections.ts b/src/vs/workbench/api/browser/positron/mainThreadDataConnections.ts index fc5ef3a01e77..5f91ee4c929f 100644 --- a/src/vs/workbench/api/browser/positron/mainThreadDataConnections.ts +++ b/src/vs/workbench/api/browser/positron/mainThreadDataConnections.ts @@ -211,10 +211,12 @@ export class MainThreadDataConnections implements MainThreadDataConnectionsShape } /** - * Previews a node through the main thread handle. + * Previews a node through the main thread handle. Goes through the service rather than calling + * the handle directly so the resulting Data Explorer is recorded against the connection, the + * same as a preview started from the Data Connections panel. */ - async $nodePreviewViaService(connectionHandle: number, nodeHandle: number): Promise { - return this._getHandle(connectionHandle).nodePreview(nodeHandle); + async $nodePreviewViaService(connectionHandle: number, nodeHandle: number): Promise { + return this._dataConnectionsService.previewNode(this._getHandle(connectionHandle), nodeHandle); } /** @@ -348,9 +350,10 @@ class MainThreadDataConnectionHandleAdapter implements IDataConnectionHandle { } /** - * Triggers a data preview for the given node (e.g. table contents). + * Triggers a data preview for the given node (e.g. table contents), resolving to the dataset id + * the extension opened it under, if it reported one. */ - async nodePreview(nodeHandle: number): Promise { + async nodePreview(nodeHandle: number): Promise { return this._proxy.$nodePreview(this.handle, nodeHandle); } diff --git a/src/vs/workbench/api/common/positron/extHost.positron.protocol.ts b/src/vs/workbench/api/common/positron/extHost.positron.protocol.ts index b9732ec4b486..65894854a31e 100644 --- a/src/vs/workbench/api/common/positron/extHost.positron.protocol.ts +++ b/src/vs/workbench/api/common/positron/extHost.positron.protocol.ts @@ -280,9 +280,10 @@ export interface MainThreadDataConnectionsShape extends IDisposable { $nodeGetChildrenViaService(connectionHandle: number, nodeHandle: number): Promise; /** - * Previews a node via the main thread service. + * Previews a node via the main thread service. Resolves to the dataset id the preview was + * opened under, or undefined when the driver did not report one. */ - $nodePreviewViaService(connectionHandle: number, nodeHandle: number): Promise; + $nodePreviewViaService(connectionHandle: number, nodeHandle: number): Promise; /** * Releases a connection handle via the main thread service. @@ -304,7 +305,7 @@ export interface ExtHostDataConnectionsShape { $connectionDisconnect(connectionHandle: number): Promise; $connectionIsConnected(connectionHandle: number): Promise; $nodeGetChildren(connectionHandle: number, nodeHandle: number): Promise; - $nodePreview(connectionHandle: number, nodeHandle: number): Promise; + $nodePreview(connectionHandle: number, nodeHandle: number): Promise; $releaseConnection(connectionHandle: number): void; } diff --git a/src/vs/workbench/api/common/positron/extHostDataConnections.ts b/src/vs/workbench/api/common/positron/extHostDataConnections.ts index 8c9d02cb0a09..7e57ecb7a9cc 100644 --- a/src/vs/workbench/api/common/positron/extHostDataConnections.ts +++ b/src/vs/workbench/api/common/positron/extHostDataConnections.ts @@ -257,8 +257,12 @@ export class ExtHostDataConnections implements extHostProtocol.ExtHostDataConnec return this._serializeNodes(connectionHandle, children); } - /** Triggers a data preview for a node (e.g. SELECT * FROM table LIMIT 100). */ - async $nodePreview(connectionHandle: number, nodeHandle: number): Promise { + /** + * Triggers a data preview for a node (e.g. SELECT * FROM table LIMIT 100). Returns the dataset + * id the driver opened the preview under, so core can relate the resulting Data Explorer back to + * this connection. Drivers may return nothing, in which case there is nothing to relate. + */ + async $nodePreview(connectionHandle: number, nodeHandle: number): Promise { const nodeMap = this._nodes.get(connectionHandle); if (!nodeMap) { throw new Error(`Connection handle ${connectionHandle} not found`); @@ -267,7 +271,8 @@ export class ExtHostDataConnections implements extHostProtocol.ExtHostDataConnec if (!node || !node.preview) { throw new Error(`Node handle ${nodeHandle} does not support preview`); } - await node.preview(); + const datasetId = await node.preview(); + return typeof datasetId === 'string' ? datasetId : undefined; } /** Frees a connection handle and all its associated node handles. */ diff --git a/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/components/destructiveTwoButtonFooter.css b/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/components/destructiveTwoButtonFooter.css new file mode 100644 index 000000000000..772c112e198e --- /dev/null +++ b/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/components/destructiveTwoButtonFooter.css @@ -0,0 +1,26 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (C) 2026 Posit Software, PBC. All rights reserved. + * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. + *--------------------------------------------------------------------------------------------*/ + +/* + * Footer layout, matching the plain two button footer. The destructive treatment itself lives on + * `.positron-dynamic-modal-dialog-box .dialog-button.destructive` in positronDynamicModalDialog.css, + * alongside the default button's, so all of the dialog button variants stay in one place. + */ +.positron-dynamic-modal-dialog-box .destructive-two-button-footer { + gap: 10px; + height: 48px; + display: flex; + padding: 0 16px; + flex: 0 0 auto; + justify-content: end; + align-items: flex-start; +} + +.positron-dynamic-modal-dialog-box .destructive-two-button-footer.top-border { + height: auto; + padding-top: 16px; + padding-bottom: 16px; + border-top: solid 1px var(--vscode-positronModalDialog-border); +} diff --git a/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/components/destructiveTwoButtonFooter.tsx b/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/components/destructiveTwoButtonFooter.tsx new file mode 100644 index 000000000000..cdf8a4d03685 --- /dev/null +++ b/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/components/destructiveTwoButtonFooter.tsx @@ -0,0 +1,62 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (C) 2026 Posit Software, PBC. All rights reserved. + * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. + *--------------------------------------------------------------------------------------------*/ + +// CSS. +import './destructiveTwoButtonFooter.css'; + +// Other dependencies. +import { FooterButton } from './footerButton.js'; +import * as platform from '../../../../../base/common/platform.js'; +import { positronClassNames } from '../../../../../base/common/positronUtilities.js'; + +/** + * DestructiveTwoButtonFooterProps interface. + */ +interface DestructiveTwoButtonFooterProps { + primaryButtonTitle: string; + secondaryButtonTitle: string; + topBorder?: boolean; + onPrimaryButton: () => void; + onSecondaryButton: () => void; +} + +/** + * DestructiveTwoButtonFooter component. A two button footer whose primary button performs a + * destructive action -- removing a saved connection, discarding work -- and so is filled with the + * destructive red rather than the accent color a default button gets, and does not take the opening + * focus the way a plain two button footer's primary does. + * @param props A DestructiveTwoButtonFooterProps that contains the component properties. + * @returns The rendered component. + */ +export const DestructiveTwoButtonFooter = (props: DestructiveTwoButtonFooterProps) => { + // Primary button. Destructive rather than default: the two treatments are alternatives, and the + // red fill is what marks the action as one the user cannot take back. Deliberately not focused -- + // see the secondary button below. + const primaryButton = ( + + {props.primaryButtonTitle} + + ); + + // Secondary button. It takes the focus rather than the primary, unlike a plain two button footer: + // the primary action here cannot be taken back, so Enter and Escape should both back out of the + // dialog and taking the action should cost a deliberate click or Tab. + const secondaryButton = ( + + {props.secondaryButtonTitle} + + ); + + // Render. + return ( +
+ {/* On Windows, the primary button comes first; on macOS/Linux, the secondary button comes first. */} + {platform.isWindows + ? <>{primaryButton}{secondaryButton} + : <>{secondaryButton}{primaryButton} + } +
+ ); +}; diff --git a/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/components/footerButton.tsx b/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/components/footerButton.tsx index 1e0b15a7e04f..47c5e42b68c6 100644 --- a/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/components/footerButton.tsx +++ b/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/components/footerButton.tsx @@ -19,6 +19,12 @@ import { Button } from '../../../../../base/browser/ui/positronComponents/button interface FooterButtonProps { autoFocus?: boolean; default?: boolean; + /** + * Whether this button performs a destructive action, which fills it with the destructive red. + * An alternative to `default`, not a modifier on it: both set the button's fill, so a button is + * one or the other. + */ + destructive?: boolean; disabled?: boolean; type?: 'button' | 'submit'; onPressed: () => void; @@ -36,7 +42,8 @@ export const FooterButton = (props: PropsWithChildren) => { className={positronClassNames( 'dialog-button', 'footer-button', - { 'default': props.default } + { 'default': props.default }, + { 'destructive': props.destructive } )} disabled={props.disabled} type={props.type} diff --git a/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/positronDynamicModalDialog.css b/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/positronDynamicModalDialog.css index 4d71d81b18ab..2f31e70a8430 100644 --- a/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/positronDynamicModalDialog.css +++ b/src/vs/workbench/browser/positronComponents/positronDynamicModalDialog/positronDynamicModalDialog.css @@ -84,7 +84,7 @@ VS Code 1.109.0 changed secondary buttons to have transparent background and no border in dark mode. For modal dialog buttons, we need a visible border for secondary buttons (Cancel, Back, etc.) */ -.vs-dark .positron-dynamic-modal-dialog-box .dialog-button:not(.default) { +.vs-dark .positron-dynamic-modal-dialog-box .dialog-button:not(.default):not(.destructive) { border: 1px solid var(--vscode-button-border); } @@ -99,9 +99,20 @@ .positron-dynamic-modal-dialog-box .dialog-button.destructive { color: var(--vscode-positronModalDialog-buttonDestructiveForeground); + border: 1px solid var(--vscode-positronModalDialog-buttonDestructiveBackground); background-color: var(--vscode-positronModalDialog-buttonDestructiveBackground); } +/* + * Needed as well as the rule above, which sets a background at the same specificity as + * `.dialog-button:hover` but later in the file: without this, a destructive button would be the only + * one that doesn't respond to the pointer. + */ +.positron-dynamic-modal-dialog-box .dialog-button.destructive:hover { + border-color: var(--vscode-positronModalDialog-buttonDestructiveHoverBackground); + background: var(--vscode-positronModalDialog-buttonDestructiveHoverBackground); +} + .positron-dynamic-modal-dialog-box .dialog-button:focus { outline: none; } diff --git a/src/vs/workbench/browser/positronTree/classes/positronTreeInstance.tsx b/src/vs/workbench/browser/positronTree/classes/positronTreeInstance.tsx index 096c2d063816..2f6a3edfc20b 100644 --- a/src/vs/workbench/browser/positronTree/classes/positronTreeInstance.tsx +++ b/src/vs/workbench/browser/positronTree/classes/positronTreeInstance.tsx @@ -337,6 +337,15 @@ export class PositronTreeInstance extends DataGridInstance { return this._expanded.has(id); } + /** + * Gets whether the node's children are loaded. A node keeps its loaded children while collapsed, + * so consumers whose children carry per-fetch resources can use this to tell whether there is + * anything to drop (see {@link dropLoadedChildren}). + */ + hasLoadedChildren(id: string): boolean { + return this._children.has(id); + } + isLoading(id: string): boolean { return this._loading.has(id); } diff --git a/src/vs/workbench/common/theme.ts b/src/vs/workbench/common/theme.ts index 66ed8b285363..97c82318311e 100644 --- a/src/vs/workbench/common/theme.ts +++ b/src/vs/workbench/common/theme.ts @@ -1215,20 +1215,30 @@ export const POSITRON_MODAL_DIALOG_DEFAULT_BUTTON_FOREGROUND = registerColor('po hcLight: buttonForeground }, localize('positronModalDialog.defaultButtonForeground', "Positron modal dialog default button foreground color.")); -// Positron modal dialog button destructive background color. +// Positron modal dialog button destructive background color. Filled with the same red a destructive +// context menu item is labelled in, so an irreversible action reads the same wherever it is offered. export const POSITRON_MODAL_DIALOG_BUTTON_DESTRUCTIVE_BACKGROUND = registerColor('positronModalDialog.buttonDestructiveBackground', { - dark: buttonSecondaryBackground, - light: buttonSecondaryBackground, - hcDark: buttonSecondaryBackground, - hcLight: buttonSecondaryBackground + dark: errorForeground, + light: errorForeground, + hcDark: errorForeground, + hcLight: errorForeground }, localize('positronModalDialog.buttonDestructiveBackground', "Positron modal dialog button destructive background color.")); -// Positron modal dialog button destructive foreground color. +// Positron modal dialog button destructive hover background color. +export const POSITRON_MODAL_DIALOG_BUTTON_DESTRUCTIVE_HOVER_BACKGROUND = registerColor('positronModalDialog.buttonDestructiveHoverBackground', { + dark: lighten(POSITRON_MODAL_DIALOG_BUTTON_DESTRUCTIVE_BACKGROUND, 0.15), + light: darken(POSITRON_MODAL_DIALOG_BUTTON_DESTRUCTIVE_BACKGROUND, 0.15), + hcDark: lighten(POSITRON_MODAL_DIALOG_BUTTON_DESTRUCTIVE_BACKGROUND, 0.15), + hcLight: darken(POSITRON_MODAL_DIALOG_BUTTON_DESTRUCTIVE_BACKGROUND, 0.15) +}, localize('positronModalDialog.buttonDestructiveHoverBackground', "Positron modal dialog button destructive hover background color.")); + +// Positron modal dialog button destructive foreground color. White, for legibility on the red fill, +// following statusBarItem.errorForeground. export const POSITRON_MODAL_DIALOG_BUTTON_DESTRUCTIVE_FOREGROUND = registerColor('positronModalDialog.buttonDestructiveForeground', { - dark: buttonSecondaryForeground, - light: buttonSecondaryForeground, - hcDark: buttonSecondaryForeground, - hcLight: buttonSecondaryForeground + dark: Color.white, + light: Color.white, + hcDark: Color.white, + hcLight: Color.white }, localize('positronModalDialog.buttonDestructiveForeground', "Positron modal dialog button destructive foreground color.")); // Positron modal dialog button disabled foreground color. diff --git a/src/vs/workbench/contrib/positronDataConnections/browser/classes/dataConnectionsTreeInstance.tsx b/src/vs/workbench/contrib/positronDataConnections/browser/classes/dataConnectionsTreeInstance.tsx index 15200f4c3264..065fc41bc6b9 100644 --- a/src/vs/workbench/contrib/positronDataConnections/browser/classes/dataConnectionsTreeInstance.tsx +++ b/src/vs/workbench/contrib/positronDataConnections/browser/classes/dataConnectionsTreeInstance.tsx @@ -35,7 +35,7 @@ export interface DataConnectionEntry { /** * DataConnectionNode discriminated union. Each tree node wraps exactly one of: - * - an entry (root rows; expanding connects, collapsing disconnects), + * - an entry (root rows; expanding connects, collapsing may disconnect -- see collapse below), * - a server-side node DTO returned from a connection's getChildren / nodeGetChildren calls. * * DTO nodes carry the originating IDataConnectionHandle so deeper children can be fetched @@ -57,8 +57,8 @@ const dtoNodeId = (handle: IDataConnectionHandle, dto: IDataConnectionNodeDTO): const wrapEntry = (entry: DataConnectionEntry): TreeNode => ({ id: entryNodeId(entry.profile), data: { kind: 'entry', entry }, - // Entries always show a twistie -- clicking it connects (or disconnects). Whether children - // exist is only knowable after the connect succeeds. + // Entries always show a twistie -- clicking it connects (or collapses, which may disconnect). + // Whether children exist is only knowable after the connect succeeds. hasChildren: true, }); @@ -73,8 +73,9 @@ const wrapDto = (dto: IDataConnectionNodeDTO, handle: IDataConnectionHandle): Tr * * Roots are one entry per saved profile, joined with its live instance (if connected). Expanding * an entry opens the connection via the service and fetches the connection's top-level DTOs; - * collapsing an entry closes the connection and drops the loaded subtree so the next expand - * re-fetches against a fresh handle. + * collapsing an entry closes the connection -- immediately, or once the last Data Explorer previewed + * from it closes -- and drops the loaded subtree so the next expand re-fetches against a fresh + * handle. */ export class DataConnectionsTreeInstance extends PositronTreeInstance { constructor(private readonly _service: IPositronDataConnectionsService) { @@ -91,24 +92,53 @@ export class DataConnectionsTreeInstance extends PositronTreeInstance { - this.setRoots(buildEntries(this._service).map(wrapEntry)); + const entries = buildEntries(this._service); + this.setRoots(entries.map(wrapEntry)); + + // A loaded DTO subtree is only valid while its connection is open -- its node handles live + // in the ext host and die with the connection -- so drop the subtree of any entry that no + // longer has a live instance, and the next expand re-fetches against a fresh handle. Doing + // it here covers every route a connection can close by, not just the ones the tree starts: + // a collapse, a deferred close once the last Data Explorer closed, or the driver dropping + // the connection on its own. + for (const entry of entries) { + const id = entryNodeId(entry.profile); + if (entry.instance === undefined && this.hasLoadedChildren(id)) { + this.dropLoadedChildren(id); + } + } }; this._register(this._service.onDidChangeProfiles(refreshRoots)); this._register(this._service.onDidChangeInstances(refreshRoots)); } /** - * Tree-semantic collapse. For entry nodes, disconnects the underlying connection and drops - * any loaded DTO subtree so the next expand re-fetches against a fresh handle. Disconnect is - * fire-and-forget -- the UI shouldn't block on the network round trip to close the channel. + * Tree-semantic expand. Expanding a connected entry means the user wants the connection again, so + * it cancels any pending close a previous collapse left behind. + */ + override async expand(id: string): Promise { + const node = this._findEntryNode(id); + if (node !== undefined) { + this._service.cancelDisconnectWhenUnused(node.entry.profile.id); + } + await super.expand(id); + } + + /** + * Tree-semantic collapse. Collapsing an entry gives up the tree's use of its connection, which + * closes the connection unless Data Explorers previewed from it are still open. In that case the + * connection stays up and closes when the last of them does: collapsing an entry is how a user + * reclaims the panel's vertical space, and previews opened from it should keep working. The entry + * row's connected indicator shows the connection is still live in the meantime, and its loaded + * subtree is kept too, so re-expanding is immediate rather than a fresh round trip. + * + * The subtree is dropped when the connection actually closes, wherever that happens -- see the + * roots refresh in the constructor. */ override collapse(id: string): void { const node = this._findEntryNode(id); if (node !== undefined && node.entry.instance !== undefined) { - // Drop loaded children first so the projection updates before the service-driven - // rebuild (from onDidChangeInstances) lands. - this.dropLoadedChildren(id); - void this._service.disconnect(node.entry.profile.id); + this._service.disconnectWhenUnused(node.entry.profile.id); } super.collapse(id); } diff --git a/src/vs/workbench/contrib/positronDataConnections/browser/components/dataConnectionEntryRow.css b/src/vs/workbench/contrib/positronDataConnections/browser/components/dataConnectionEntryRow.css index 69d2d6a42196..ea1a871ffac2 100644 --- a/src/vs/workbench/contrib/positronDataConnections/browser/components/dataConnectionEntryRow.css +++ b/src/vs/workbench/contrib/positronDataConnections/browser/components/dataConnectionEntryRow.css @@ -28,6 +28,15 @@ transform: translateY(-0.5px); } +.data-connection-entry-connected { + flex: 0 0 auto; + width: 6px; + height: 6px; + margin: 0 5px; + border-radius: 50%; + background-color: var(--vscode-charts-green); +} + .data-connection-entry-actions { display: flex; width: 20px; diff --git a/src/vs/workbench/contrib/positronDataConnections/browser/components/dataConnectionEntryRow.tsx b/src/vs/workbench/contrib/positronDataConnections/browser/components/dataConnectionEntryRow.tsx index dfa9f148855f..82345d308a85 100644 --- a/src/vs/workbench/contrib/positronDataConnections/browser/components/dataConnectionEntryRow.tsx +++ b/src/vs/workbench/contrib/positronDataConnections/browser/components/dataConnectionEntryRow.tsx @@ -13,6 +13,7 @@ import { useRef } from 'react'; import { localize } from '../../../../../nls.js'; import { ConfigureDataConnection } from '../dialogs/configureDataConnection.js'; import { showConnectDataConnectionWith } from '../dialogs/connectDataConnectionWith.js'; +import { showRemoveDataConnectionConfirmation } from '../dialogs/removeDataConnectionConfirmation.js'; import { DataConnectionEntry } from '../classes/dataConnectionsTreeInstance.js'; import { usePositronReactServicesContext } from '../../../../../base/browser/positronReactRendererContext.js'; import { PositronModalDialogReactRenderer } from '../../../../../base/browser/positronModalDialogReactRenderer.js'; @@ -32,8 +33,9 @@ interface DataConnectionEntryRowProps { /** * DataConnectionEntryRow component. Renders a root-level entry: the saved profile plus, when - * connected, a live-status indicator. Twistie click (handled by PositronTree) opens or closes - * the connection; the actions menu exposes runtime-language connect options and edit/remove. + * connected, a live-status indicator. Twistie click (handled by PositronTree) opens the connection; + * collapsing closes it only when nothing else is using it (see DataConnectionsTreeInstance). The + * actions menu exposes runtime-language connect options and edit/remove. */ export const DataConnectionEntryRow = ({ entry }: DataConnectionEntryRowProps) => { // Services. @@ -99,6 +101,21 @@ export const DataConnectionEntryRow = ({ entry }: DataConnectionEntryRowProps) = ); }; + /** + * Confirms removing this connection profile, then removes it. Removal deletes the profile's saved + * settings and secrets, and closes its connection along with any Data Explorers previewed from it, + * so the count of those goes into the prompt. + */ + const confirmRemoveProfile = async () => { + const confirmed = await showRemoveDataConnectionConfirmation( + profile.connectionName, + positronDataConnectionsService.countOpenDataExplorers(profile.id) + ); + if (confirmed) { + positronDataConnectionsService.removeProfile(profile.id); + } + }; + /** * Shows the actions menu for this connection entry. */ @@ -208,7 +225,7 @@ export const DataConnectionEntryRow = ({ entry }: DataConnectionEntryRowProps) = destructive: true, icon: 'trash', label: localize('positron.dataConnections.remove', "Remove"), - onSelected: () => positronDataConnectionsService.removeProfile(profile.id), + onSelected: () => confirmRemoveProfile(), })); // Show the menu. @@ -221,6 +238,9 @@ export const DataConnectionEntryRow = ({ entry }: DataConnectionEntryRowProps) = }); }; + // The connected indicator's label, used as both its tooltip and its accessible name. + const connectedLabel = localize('positron.dataConnections.connected', "Connected"); + // Render. return (
@@ -230,6 +250,17 @@ export const DataConnectionEntryRow = ({ entry }: DataConnectionEntryRowProps) = {' ยท '} {profile.driverMetadata.name}
+ {entry.instance && ( + // Shown whenever the profile has a live connection, including while the entry is + // collapsed -- collapsing does not necessarily disconnect, so this is how the user + // tells a live connection from a saved-but-closed one. +
+ )}
+ } + footer={ + + } + renderer={props.renderer} + title={localize('positron.removeDataConnectionConfirmation.title', "Remove Connection?")} + titleSize='large' + width={REMOVE_CONNECTION_CONFIRMATION_WIDTH} + onCancel={props.onCancel} + /> + ); +}; diff --git a/src/vs/workbench/contrib/positronDataConnections/test/browser/dataConnectionEntryRow.vitest.tsx b/src/vs/workbench/contrib/positronDataConnections/test/browser/dataConnectionEntryRow.vitest.tsx new file mode 100644 index 000000000000..bc481ac1172e --- /dev/null +++ b/src/vs/workbench/contrib/positronDataConnections/test/browser/dataConnectionEntryRow.vitest.tsx @@ -0,0 +1,48 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (C) 2026 Posit Software, PBC. All rights reserved. + * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. + *--------------------------------------------------------------------------------------------*/ + +/// + +import { screen } from '@testing-library/react'; +import { stubInterface } from '../../../../../test/vitest/stubInterface.js'; +import { setupRTLRenderer } from '../../../../../test/vitest/reactTestingLibrary.js'; +import { createTestContainer } from '../../../../../test/vitest/positronTestContainer.js'; +import { IDataConnectionInstance } from '../../../../services/positronDataConnections/common/interfaces/dataConnectionInstance.js'; +import { IDataConnectionProfile } from '../../../../services/positronDataConnections/common/interfaces/dataConnectionDriver.js'; +import { DataConnectionEntryRow } from '../../browser/components/dataConnectionEntryRow.js'; + +const profile: IDataConnectionProfile = { + id: 'conn-1', + driverMetadata: { + id: 'test-driver', + name: 'Test Driver', + iconSvg: '', + supportedLanguageIds: [], + }, + connectionName: 'My Connection', + mechanismId: 'test-mechanism', + parameterValues: {}, +}; + +describe('DataConnectionEntryRow', () => { + const ctx = createTestContainer().withReactServices().build(); + const rtl = setupRTLRenderer(() => ctx.reactServices); + + it('shows the connected indicator for a profile with a live connection', () => { + const instance = stubInterface({ id: 'instance-1', profileId: profile.id }); + + rtl.render(); + + // The indicator is a bare dot, so its accessible name is the only thing to query it by. + expect(screen.getByRole('img', { name: 'Connected' })).toBeInTheDocument(); + }); + + it('shows no connected indicator for a saved profile that is not connected', () => { + rtl.render(); + + expect(screen.queryByRole('img', { name: 'Connected' })).not.toBeInTheDocument(); + expect(screen.getByText('My Connection', { exact: false })).toBeInTheDocument(); + }); +}); diff --git a/src/vs/workbench/contrib/positronDataConnections/test/browser/dataConnectionsTreeInstance.vitest.ts b/src/vs/workbench/contrib/positronDataConnections/test/browser/dataConnectionsTreeInstance.vitest.ts new file mode 100644 index 000000000000..20c3d1fab274 --- /dev/null +++ b/src/vs/workbench/contrib/positronDataConnections/test/browser/dataConnectionsTreeInstance.vitest.ts @@ -0,0 +1,133 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (C) 2026 Posit Software, PBC. All rights reserved. + * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. + *--------------------------------------------------------------------------------------------*/ + +/// + +import { Emitter, Event } from '../../../../../base/common/event.js'; +import { createTestContainer } from '../../../../../test/vitest/positronTestContainer.js'; +import { stubInterface } from '../../../../../test/vitest/stubInterface.js'; +import { DataConnectionsTreeInstance } from '../../browser/classes/dataConnectionsTreeInstance.js'; +import { IDataConnectionInstance } from '../../../../services/positronDataConnections/common/interfaces/dataConnectionInstance.js'; +import { IDataConnectionHandle, IDataConnectionProfile } from '../../../../services/positronDataConnections/common/interfaces/dataConnectionDriver.js'; +import { IPositronDataConnectionsService } from '../../../../services/positronDataConnections/common/interfaces/positronDataConnectionsService.js'; + +// The tree's id for the single profile these tests use. +const ENTRY_ID = 'entry:conn-1'; + +const profile: IDataConnectionProfile = { + id: 'conn-1', + driverMetadata: { + id: 'test-driver', + name: 'Test Driver', + iconSvg: '', + supportedLanguageIds: [], + }, + connectionName: 'Test Connection', + mechanismId: 'test-mechanism', + parameterValues: {}, +}; + +describe('DataConnectionsTreeInstance', () => { + const ctx = createTestContainer().build(); + + // Fires when the service's set of live connections changes, which is what drives the tree to + // rebuild its roots. + const onDidChangeInstances = new Emitter(); + + /** + * Builds a tree over one profile, connected unless `connected` says otherwise. `setConnected` + * flips the profile's live state and notifies the tree, standing in for the service connecting or + * disconnecting it. + */ + function createTree(connected = true) { + const getChildren = vi.fn(async () => []); + const instance = stubInterface({ + id: 'instance-1', + profileId: profile.id, + connectionHandle: stubInterface({ handle: 1, getChildren }), + }); + + let liveInstance = connected ? instance : undefined; + const service = stubInterface({ + onDidChangeProfiles: Event.None, + onDidChangeInstances: onDidChangeInstances.event, + getProfiles: () => [profile], + getInstanceForProfile: () => liveInstance, + connect: async () => instance, + disconnectWhenUnused: vi.fn(), + cancelDisconnectWhenUnused: vi.fn(), + }); + + const tree = new DataConnectionsTreeInstance(service); + ctx.disposables.add(tree); + + const setConnected = (nowConnected: boolean) => { + liveInstance = nowConnected ? instance : undefined; + onDidChangeInstances.fire(nowConnected ? [instance] : []); + }; + + return { tree, service, getChildren, setConnected }; + } + + it('gives up its use of the connection when a connected entry is collapsed', async () => { + const { tree, service } = createTree(); + await tree.refresh(); + await tree.expand(ENTRY_ID); + + tree.collapse(ENTRY_ID); + + // The service decides whether that closes the connection now or once the last Data Explorer + // previewed from it is closed. + expect(service.disconnectWhenUnused).toHaveBeenCalledWith(profile.id); + }); + + it('does not touch the connection when an entry that is not connected is collapsed', async () => { + const { tree, service } = createTree(false); + await tree.refresh(); + await tree.expand(ENTRY_ID); + + tree.collapse(ENTRY_ID); + + expect(service.disconnectWhenUnused).not.toHaveBeenCalled(); + }); + + it('cancels a pending close when the entry is expanded again', async () => { + const { tree, service } = createTree(); + await tree.refresh(); + await tree.expand(ENTRY_ID); + tree.collapse(ENTRY_ID); + + await tree.expand(ENTRY_ID); + + expect(service.cancelDisconnectWhenUnused).toHaveBeenCalledWith(profile.id); + }); + + it('keeps the loaded subtree across a collapse while the connection is still open', async () => { + const { tree, getChildren } = createTree(); + await tree.refresh(); + await tree.expand(ENTRY_ID); + expect(getChildren).toHaveBeenCalledTimes(1); + + tree.collapse(ENTRY_ID); + await tree.expand(ENTRY_ID); + + // The node handles in the loaded subtree are still valid, so re-expanding costs no round trip. + expect(getChildren).toHaveBeenCalledTimes(1); + }); + + it('drops the loaded subtree once the connection closes', async () => { + const { tree, getChildren, setConnected } = createTree(); + await tree.refresh(); + await tree.expand(ENTRY_ID); + tree.collapse(ENTRY_ID); + + // The connection closes -- here after its last Data Explorer did, which the service drives. + setConnected(false); + + // Its node handles died with it, so re-expanding has to fetch the subtree again. + await tree.expand(ENTRY_ID); + expect(getChildren).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/vs/workbench/contrib/positronDataConnections/test/browser/removeDataConnectionConfirmation.vitest.tsx b/src/vs/workbench/contrib/positronDataConnections/test/browser/removeDataConnectionConfirmation.vitest.tsx new file mode 100644 index 000000000000..e7ccef462540 --- /dev/null +++ b/src/vs/workbench/contrib/positronDataConnections/test/browser/removeDataConnectionConfirmation.vitest.tsx @@ -0,0 +1,88 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (C) 2026 Posit Software, PBC. All rights reserved. + * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. + *--------------------------------------------------------------------------------------------*/ + +/// + +import { screen } from '@testing-library/react'; +import { userEvent } from '@testing-library/user-event'; +import { PositronReactServices } from '../../../../../base/browser/positronReactServices.js'; +import { createTestContainer } from '../../../../../test/vitest/positronTestContainer.js'; +import { setupRTLRenderer } from '../../../../../test/vitest/reactTestingLibrary.js'; +import { showRemoveDataConnectionConfirmation } from '../../browser/dialogs/removeDataConnectionConfirmation.js'; + +describe('showRemoveDataConnectionConfirmation', () => { + const ctx = createTestContainer().withReactServices().build(); + + // The dialog renders itself through its own PositronModalDialogReactRenderer rather than being + // handed to rtl.render, so this only establishes the services context it renders into. + setupRTLRenderer(() => ctx.reactServices); + + beforeEach(() => { + // PositronModalDialogReactRenderer reads the services singleton in its constructor to find the + // container to render into, so the container's services have to be reachable from there. + PositronReactServices.services = ctx.reactServices; + }); + + it('names the connection and warns that the removal cannot be undone', async () => { + const confirmation = showRemoveDataConnectionConfirmation('My Connection', 0); + + expect(await screen.findByText(/'My Connection' will be deleted/)).toBeInTheDocument(); + expect(screen.getByText(/cannot be undone/)).toBeInTheDocument(); + + // No Data Explorers are open, so the dialog says nothing about closing any. + expect(screen.queryByText(/Data Explorer/)).not.toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: 'Cancel' })); + expect(await confirmation).toBe(false); + }); + + it('warns about a single open Data Explorer', async () => { + const confirmation = showRemoveDataConnectionConfirmation('My Connection', 1); + + expect(await screen.findByText('The Data Explorer open on this connection will close.')) + .toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: 'Remove' })); + expect(await confirmation).toBe(true); + }); + + it('opens with the focus on Cancel, not on the confirming button', async () => { + const confirmation = showRemoveDataConnectionConfirmation('My Connection', 0); + + // Removing a connection cannot be undone, so the keystrokes that dismiss a dialog must not + // carry it out: Enter on the focused Cancel button backs out. + const cancelButton = await screen.findByRole('button', { name: 'Cancel' }); + expect(cancelButton).toHaveFocus(); + expect(screen.getByRole('button', { name: 'Remove' })).not.toHaveFocus(); + + await userEvent.keyboard('{Enter}'); + expect(await confirmation).toBe(false); + }); + + it('styles the confirming button as destructive rather than as the default', async () => { + const confirmation = showRemoveDataConnectionConfirmation('My Connection', 0); + + // The class is what the destructive footer contributes, and carries the red; a default-styled + // primary would take the accent fill instead and leave that red unreadable. + const removeButton = await screen.findByRole('button', { name: 'Remove' }); + expect(removeButton).toHaveClass('destructive'); + expect(removeButton).not.toHaveClass('default'); + + // Settle the dialog: it renders into the layout container through its own renderer, which + // outlives RTL's cleanup, so a dialog left open would still be there for the next test. + await userEvent.click(screen.getByRole('button', { name: 'Cancel' })); + expect(await confirmation).toBe(false); + }); + + it('warns about several open Data Explorers', async () => { + const confirmation = showRemoveDataConnectionConfirmation('My Connection', 3); + + expect(await screen.findByText('The 3 Data Explorers open on this connection will close.')) + .toBeInTheDocument(); + + await userEvent.click(screen.getByRole('button', { name: 'Remove' })); + expect(await confirmation).toBe(true); + }); +}); diff --git a/src/vs/workbench/services/positronDataConnections/browser/positronDataConnectionsService.ts b/src/vs/workbench/services/positronDataConnections/browser/positronDataConnectionsService.ts index ee3f57bd8145..b49ce28a0469 100644 --- a/src/vs/workbench/services/positronDataConnections/browser/positronDataConnectionsService.ts +++ b/src/vs/workbench/services/positronDataConnections/browser/positronDataConnectionsService.ts @@ -9,12 +9,15 @@ import { Disposable } from '../../../../base/common/lifecycle.js'; import { ILogService } from '../../../../platform/log/common/log.js'; import { ISecretStorageService } from '../../../../platform/secrets/common/secrets.js'; import { DataConnectionsDriverManager } from './dataConnectionsDriverManager.js'; +import { IEditorIdentifier } from '../../../common/editor.js'; +import { IEditorService } from '../../editor/common/editorService.js'; import { IExtensionService } from '../../../services/extensions/common/extensions.js'; import { IDataConnectionInstance } from '../common/interfaces/dataConnectionInstance.js'; +import { PositronDataExplorerUri } from '../../positronDataExplorer/common/positronDataExplorerUri.js'; import { IPositronDataConnectionsService } from '../common/interfaces/positronDataConnectionsService.js'; import { IStorageService, StorageScope, StorageTarget } from '../../../../platform/storage/common/storage.js'; import { InstantiationType, registerSingleton } from '../../../../platform/instantiation/common/extensions.js'; -import { IDataConnectionProfile, resolveDataConnectionMechanism } from '../common/interfaces/dataConnectionDriver.js'; +import { IDataConnectionHandle, IDataConnectionProfile, resolveDataConnectionMechanism } from '../common/interfaces/dataConnectionDriver.js'; import { IDataConnectionsDriverManager } from '../common/interfaces/dataConnectionsDriverManager.js'; // Storage key prefix for persisted data connection profiles. Each data connection profile gets @@ -52,6 +55,18 @@ export class PositronDataConnectionsService extends Disposable implements IPosit // Data connection instances. private readonly _instances: IDataConnectionInstance[] = []; + // Dataset ids that previews opened in the Data Explorer, keyed by the profile whose connection + // they were previewed from. Recorded by previewNode and dropped when the profile disconnects. + // A recorded id outlives its editor -- the user can close the tab at any time -- so this is a + // record of what was opened, not of what is still open; countOpenDataExplorers resolves the + // difference against the editor service. + private readonly _previewedDatasetIds = new Map>(); + + // Profiles whose connection should close as soon as their last Data Explorer does. Populated by + // disconnectWhenUnused, drained when an editor closes and leaves the profile with none open, and + // cleared when the connection is wanted again or closes by another route. + private readonly _disconnectWhenUnused = new Set(); + // Fires when data connection profiles change. private readonly _onDidChangeProfilesEmitter = this._register(new Emitter()); @@ -65,12 +80,14 @@ export class PositronDataConnectionsService extends Disposable implements IPosit /** * Constructor. * @param extensionService The extension service. + * @param _editorService The editor service (used to see which previews are still open). * @param _logService The log service. * @param _secretStorageService The secret storage service (secret parameter values). * @param _storageService The storage service (profile metadata). */ constructor( @IExtensionService extensionService: IExtensionService, + @IEditorService private readonly _editorService: IEditorService, @ILogService private readonly _logService: ILogService, @ISecretStorageService private readonly _secretStorageService: ISecretStorageService, @IStorageService private readonly _storageService: IStorageService, @@ -84,6 +101,10 @@ export class PositronDataConnectionsService extends Disposable implements IPosit // Load data connection profiles from storage. Secret values stay in secret storage and are // fetched on demand by getProfileWithSecrets. this._loadProfiles(); + + // A closing editor may have been the last Data Explorer holding a connection open. The event + // fires after the editor leaves its group, so the count below already reflects the close. + this._register(this._editorService.onDidCloseEditor(() => this._disconnectUnusedProfiles())); } //#endregion Constructor & Dispose @@ -284,6 +305,12 @@ export class PositronDataConnectionsService extends Disposable implements IPosit return; } + // Close the profile's Data Explorers and its connection. Removing the profile takes its row + // out of the Data Connections panel, so a connection left open here would stay open and + // unreachable for the rest of the session. Fire-and-forget: the removal shouldn't wait on the + // round trips that close the editors and the channel. + void this._closeConnectionAndDataExplorers(id); + // Remove the data connection profile. this._profiles.splice(index, 1); @@ -366,6 +393,12 @@ export class PositronDataConnectionsService extends Disposable implements IPosit const instance = this._instances[index]; this._instances.splice(index, 1); + // The connection is going away, so its previewed datasets are no longer meaningful: the + // driver tears their backends down, and a later reconnect mints fresh dataset ids. Any + // pending close is moot for the same reason, whichever route brought us here. + this._previewedDatasetIds.delete(profileId); + this._disconnectWhenUnused.delete(profileId); + try { await instance.connectionHandle.disconnect(); } catch (err) { @@ -379,6 +412,64 @@ export class PositronDataConnectionsService extends Disposable implements IPosit this._onDidChangeInstancesEmitter.fire([...this._instances]); } + /** + * Previews a node in the Data Explorer, recording the dataset id the driver opened it under + * against the connection's profile. + */ + async previewNode(handle: IDataConnectionHandle, nodeHandle: number): Promise { + const datasetId = await handle.nodePreview(nodeHandle); + if (datasetId === undefined) { + return undefined; + } + + // Attribute the dataset to the profile this handle belongs to. A handle with no live + // instance (e.g. one already disconnected) has nothing to attribute it to; the preview + // still happened, it just isn't tracked. + const profileId = this._instances.find(i => i.connectionHandle === handle)?.profileId; + if (profileId !== undefined) { + const datasetIds = this._previewedDatasetIds.get(profileId); + if (datasetIds) { + datasetIds.add(datasetId); + } else { + this._previewedDatasetIds.set(profileId, new Set([datasetId])); + } + } + + return datasetId; + } + + /** + * Gets how many Data Explorers are open on data previewed from the given profile's connection. + */ + countOpenDataExplorers(profileId: string): number { + return this._openDataExplorers(profileId).length; + } + + /** + * Closes the profile's connection as soon as nothing is using it. + */ + disconnectWhenUnused(profileId: string): void { + if (this.getInstanceForProfile(profileId) === undefined) { + return; + } + + // Still in use: wait for the last Data Explorer to close. Otherwise close it now. Disconnect + // is fire-and-forget either way -- callers shouldn't block on the round trip that closes the + // channel. + if (this.countOpenDataExplorers(profileId) > 0) { + this._disconnectWhenUnused.add(profileId); + } else { + void this.disconnect(profileId); + } + } + + /** + * Cancels a pending disconnectWhenUnused for the profile. + */ + cancelDisconnectWhenUnused(profileId: string): void { + this._disconnectWhenUnused.delete(profileId); + } + /** * Gets all data connection instances. */ @@ -402,6 +493,51 @@ export class PositronDataConnectionsService extends Disposable implements IPosit //#endregion IPositronDataConnectionsService Implementation + //#region Private Methods + + /** + * Closes the connections of any profiles waiting on their last Data Explorer, now that one has + * closed. A profile whose other Data Explorers are still open keeps waiting. + */ + private _disconnectUnusedProfiles(): void { + // Iterate a copy: disconnect() mutates the pending set. + for (const profileId of [...this._disconnectWhenUnused]) { + if (this.countOpenDataExplorers(profileId) === 0) { + void this.disconnect(profileId); + } + } + } + + /** + * Gets the editors of the Data Explorers open on data previewed from the given profile's + * connection. The editor service is the authority on what is still open: a recorded dataset whose + * tab the user has since closed has no editors, so it doesn't appear here. + */ + private _openDataExplorers(profileId: string): readonly IEditorIdentifier[] { + const datasetIds = this._previewedDatasetIds.get(profileId); + if (datasetIds === undefined) { + return []; + } + return [...datasetIds].flatMap(datasetId => + this._editorService.findEditors(PositronDataExplorerUri.generate(datasetId)) + ); + } + + /** + * Closes the Data Explorers previewed from the given profile's connection, then the connection + * itself. The Data Explorers go first because their backends die with the connection: a tab left + * open would show a grid that errors on the next scroll or filter rather than any useful data. + */ + private async _closeConnectionAndDataExplorers(profileId: string): Promise { + const editors = this._openDataExplorers(profileId); + if (editors.length > 0) { + await this._editorService.closeEditors(editors); + } + await this.disconnect(profileId); + } + + //#endregion Private Methods + //#region Persistence /** diff --git a/src/vs/workbench/services/positronDataConnections/common/interfaces/dataConnectionDriver.ts b/src/vs/workbench/services/positronDataConnections/common/interfaces/dataConnectionDriver.ts index dd01939a2c7d..94209f484da3 100644 --- a/src/vs/workbench/services/positronDataConnections/common/interfaces/dataConnectionDriver.ts +++ b/src/vs/workbench/services/positronDataConnections/common/interfaces/dataConnectionDriver.ts @@ -188,6 +188,10 @@ export interface IDataConnectionHandle { disconnect(): Promise; isConnected(): Promise; nodeGetChildren(nodeHandle: number): Promise; - nodePreview(nodeHandle: number): Promise; + /** + * Previews a node's data in the Data Explorer. Resolves to the dataset id the preview was + * opened under, or undefined when the driver did not report one. + */ + nodePreview(nodeHandle: number): Promise; release(): void; } \ No newline at end of file diff --git a/src/vs/workbench/services/positronDataConnections/common/interfaces/positronDataConnectionsService.ts b/src/vs/workbench/services/positronDataConnections/common/interfaces/positronDataConnectionsService.ts index 655398d7f093..0bd81d2ab01e 100644 --- a/src/vs/workbench/services/positronDataConnections/common/interfaces/positronDataConnectionsService.ts +++ b/src/vs/workbench/services/positronDataConnections/common/interfaces/positronDataConnectionsService.ts @@ -7,7 +7,7 @@ import { Event } from '../../../../../base/common/event.js'; import { IDisposable } from '../../../../../base/common/lifecycle.js'; import { IDataConnectionInstance } from './dataConnectionInstance.js'; import { createDecorator } from '../../../../../platform/instantiation/common/instantiation.js'; -import { IDataConnectionProfile } from './dataConnectionDriver.js'; +import { IDataConnectionHandle, IDataConnectionProfile } from './dataConnectionDriver.js'; import { IDataConnectionsDriverManager } from './dataConnectionsDriverManager.js'; // DI token used to inject IPositronDataConnectionsService throughout the workbench. @@ -96,7 +96,10 @@ export interface IPositronDataConnectionsService extends IDisposable { setPreferredCodeVariant(profileId: string, languageId: string, variantId: string): void; /** - * Removes a data connection profile. + * Removes a data connection profile, deleting its persisted settings and stored secrets. Also + * closes anything still using it: the Data Explorers previewed from its connection, and then the + * connection itself, since a removed profile leaves no UI to manage a connection from. Callers + * should confirm with the user first -- none of this is recoverable. * @param id The data connection profile id to remove. */ removeProfile(id: string): void; @@ -119,6 +122,42 @@ export interface IPositronDataConnectionsService extends IDisposable { */ disconnect(profileId: string): Promise; + /** + * Previews a node in the Data Explorer, recording the dataset id the driver opened it under + * against the connection's profile so {@link countOpenDataExplorers} can report it later. Callers + * should preview through this method rather than calling handle.nodePreview() directly, so no + * Data Explorer goes unrecorded. + * @param handle The connection handle the node belongs to. + * @param nodeHandle The handle of the node to preview. + * @returns The dataset id the preview was opened under, or undefined if the driver reported none. + */ + previewNode(handle: IDataConnectionHandle, nodeHandle: number): Promise; + + /** + * Gets how many Data Explorers are open on data previewed from the given profile's connection. + * Counts only previews the driver reported a dataset id for, and only those whose editor is still + * open -- the user closing a Data Explorer tab brings the count back down. + * @param profileId The data connection profile id. + */ + countOpenDataExplorers(profileId: string): number; + + /** + * Closes the profile's connection as soon as nothing is using it: right away when it has no open + * Data Explorers, otherwise once the last one is closed. Lets a caller give up its own use of a + * connection without cutting off the Data Explorers still reading from it. No-op if the profile + * has no live connection. Cancel a pending close with {@link cancelDisconnectWhenUnused}. + * @param profileId The data connection profile id. + */ + disconnectWhenUnused(profileId: string): void; + + /** + * Cancels a pending {@link disconnectWhenUnused} for the profile, keeping its connection open. + * Call this when the connection is wanted again (e.g. the user re-expanded it). No-op if no close + * is pending. + * @param profileId The data connection profile id. + */ + cancelDisconnectWhenUnused(profileId: string): void; + /** * Gets all data connection instances. * @returns The data connection instances array. diff --git a/src/vs/workbench/services/positronDataConnections/test/browser/positronDataConnectionsService.vitest.ts b/src/vs/workbench/services/positronDataConnections/test/browser/positronDataConnectionsService.vitest.ts index 237a53ce4d53..1c6a2e53ba1e 100644 --- a/src/vs/workbench/services/positronDataConnections/test/browser/positronDataConnectionsService.vitest.ts +++ b/src/vs/workbench/services/positronDataConnections/test/browser/positronDataConnectionsService.vitest.ts @@ -5,15 +5,21 @@ /// +import { URI } from '../../../../../base/common/uri.js'; +import { Emitter } from '../../../../../base/common/event.js'; import { ILogService, NullLogService } from '../../../../../platform/log/common/log.js'; import { IStorageService } from '../../../../../platform/storage/common/storage.js'; import { ISecretStorageService } from '../../../../../platform/secrets/common/secrets.js'; import { TestSecretStorageService } from '../../../../../platform/secrets/test/common/testSecretStorageService.js'; import { TestStorageService } from '../../../../test/common/workbenchTestServices.js'; import { NullExtensionService, IExtensionService } from '../../../extensions/common/extensions.js'; +import { IEditorCloseEvent, IEditorIdentifier } from '../../../../common/editor.js'; +import { EditorInput } from '../../../../common/editor/editorInput.js'; +import { IEditorService } from '../../../editor/common/editorService.js'; +import { PositronDataExplorerUri } from '../../../positronDataExplorer/common/positronDataExplorerUri.js'; import { createTestContainer } from '../../../../../test/vitest/positronTestContainer.js'; import { stubInterface } from '../../../../../test/vitest/stubInterface.js'; -import { IDataConnectionDriver, IDataConnectionProfile } from '../../common/interfaces/dataConnectionDriver.js'; +import { IDataConnectionDriver, IDataConnectionDriverMetadata, IDataConnectionHandle, IDataConnectionProfile } from '../../common/interfaces/dataConnectionDriver.js'; import { IPositronDataConnectionsService } from '../../common/interfaces/positronDataConnectionsService.js'; import { PositronDataConnectionsService } from '../../browser/positronDataConnectionsService.js'; @@ -32,16 +38,63 @@ function createProfile(id: string): IDataConnectionProfile { }; } +function createDriverMetadata(): IDataConnectionDriverMetadata { + return { + id: 'test-driver', + name: 'Test Driver', + description: '', + iconSvg: '', + supportedLanguageIds: [], + mechanisms: [{ + id: 'test-mechanism', + label: 'Test Mechanism', + description: '', + parameters: [], + }], + }; +} + describe('PositronDataConnectionsService', () => { + // The dataset ids whose Data Explorer editor is currently open. The IEditorService stub below + // reports an editor for exactly these, so a test can open and close Data Explorer tabs by + // mutating the set. + const openDatasetIds = new Set(); + + // Fires when a Data Explorer editor closes. Tests close a tab by removing its dataset id from + // openDatasetIds and firing this, the way the editor part does. + const onDidCloseEditor = new Emitter(); + + // Maps a Data Explorer resource back to the open dataset id it belongs to, or undefined when no + // open dataset matches it. + const datasetIdForResource = (resource: URI) => [...openDatasetIds].find( + datasetId => PositronDataExplorerUri.generate(datasetId).toString() === resource.toString() + ); + const ctx = createTestContainer() .stub(IExtensionService, new NullExtensionService()) .stub(ILogService, new NullLogService()) + .stub(IEditorService, { + onDidCloseEditor: onDidCloseEditor.event, + findEditors: (resource: URI) => datasetIdForResource(resource) !== undefined + ? [stubInterface({ editor: stubInterface({ resource }) })] + : [], + closeEditors: async (editors: readonly IEditorIdentifier[]) => { + for (const { editor } of editors) { + const datasetId = editor.resource && datasetIdForResource(editor.resource); + if (datasetId !== undefined) { + openDatasetIds.delete(datasetId); + } + } + onDidCloseEditor.fire(stubInterface({})); + }, + }) .build(); let storageService: TestStorageService; let service: IPositronDataConnectionsService; beforeEach(() => { + openDatasetIds.clear(); storageService = new TestStorageService(); ctx.disposables.add(storageService); ctx.instantiationService.stub(IStorageService, storageService); @@ -122,4 +175,181 @@ describe('PositronDataConnectionsService', () => { // public profile returned by getProfile. expect(service.getProfile('conn-1')?.parameterValues).toEqual({}); }); + + describe('open Data Explorers', () => { + /** + * Connects 'conn-1' through a driver that opens a Data Explorer per preview, the way a real + * driver does, reporting `datasetIds` in order -- one per preview. An undefined entry stands + * for a driver that opens a preview without reporting the dataset id it used. + */ + async function connectProfile(datasetIds: readonly (string | undefined)[]) { + const remainingDatasetIds = [...datasetIds]; + const handle = stubInterface({ + handle: 1, + nodePreview: async () => { + const datasetId = remainingDatasetIds.shift(); + if (datasetId !== undefined) { + openDatasetIds.add(datasetId); + } + return datasetId; + }, + disconnect: async () => { }, + release: () => { }, + }); + service.driverManager.registerDriver(stubInterface({ + id: 'test-driver', + metadata: createDriverMetadata(), + connect: async () => handle, + })); + service.addUpdateProfile(createProfile('conn-1')); + return service.connect('conn-1'); + } + + it('reports a Data Explorer previewed from the connection', async () => { + const instance = await connectProfile(['sqlite:conn-1:table:flights']); + + expect(await service.previewNode(instance.connectionHandle, 7)) + .toBe('sqlite:conn-1:table:flights'); + expect(service.countOpenDataExplorers('conn-1')).toBe(1); + }); + + it('stops reporting one the user has closed', async () => { + const instance = await connectProfile(['sqlite:conn-1:table:flights']); + await service.previewNode(instance.connectionHandle, 7); + + // The user closes the Data Explorer tab. The dataset stays recorded against the profile, + // but it no longer has an editor, so it no longer counts. + openDatasetIds.clear(); + + expect(service.countOpenDataExplorers('conn-1')).toBe(0); + }); + + it('reports nothing for a driver that does not report its dataset ids', async () => { + const instance = await connectProfile([undefined]); + + expect(await service.previewNode(instance.connectionHandle, 7)).toBeUndefined(); + expect(service.countOpenDataExplorers('conn-1')).toBe(0); + }); + + it('forgets a profile\'s previews once it disconnects', async () => { + const instance = await connectProfile(['sqlite:conn-1:table:flights']); + await service.previewNode(instance.connectionHandle, 7); + + await service.disconnect('conn-1'); + + // The editor outlives the connection, but the connection's record of it does not: a + // reconnect mints fresh dataset ids, so the old ones must not carry over. + expect(openDatasetIds.size).toBe(1); + expect(service.countOpenDataExplorers('conn-1')).toBe(0); + }); + + it('reports nothing for a profile that has never been previewed', async () => { + await connectProfile(['sqlite:conn-1:table:flights']); + + expect(service.countOpenDataExplorers('conn-1')).toBe(0); + }); + + describe('disconnectWhenUnused', () => { + // Closes the Data Explorer for the given dataset id, as the editor part does: the editor + // leaves its group first, then the close event fires. + function closeDataExplorer(datasetId: string) { + openDatasetIds.delete(datasetId); + onDidCloseEditor.fire(stubInterface({})); + } + + it('closes the connection right away when nothing is using it', async () => { + await connectProfile([]); + + service.disconnectWhenUnused('conn-1'); + + expect(service.getInstanceForProfile('conn-1')).toBeUndefined(); + }); + + it('waits for the last Data Explorer to close, then closes the connection', async () => { + const instance = await connectProfile(['sqlite:conn-1:table:flights']); + await service.previewNode(instance.connectionHandle, 7); + + service.disconnectWhenUnused('conn-1'); + expect(service.getInstanceForProfile('conn-1')).toBeDefined(); + + closeDataExplorer('sqlite:conn-1:table:flights'); + + expect(service.getInstanceForProfile('conn-1')).toBeUndefined(); + }); + + it('keeps the connection while another Data Explorer is still open', async () => { + const instance = await connectProfile([ + 'sqlite:conn-1:table:flights', + 'sqlite:conn-1:table:airports', + ]); + await service.previewNode(instance.connectionHandle, 7); + await service.previewNode(instance.connectionHandle, 8); + + service.disconnectWhenUnused('conn-1'); + closeDataExplorer('sqlite:conn-1:table:flights'); + + expect(service.getInstanceForProfile('conn-1')).toBeDefined(); + + closeDataExplorer('sqlite:conn-1:table:airports'); + + expect(service.getInstanceForProfile('conn-1')).toBeUndefined(); + }); + + it('keeps the connection when the pending close is cancelled', async () => { + const instance = await connectProfile(['sqlite:conn-1:table:flights']); + await service.previewNode(instance.connectionHandle, 7); + service.disconnectWhenUnused('conn-1'); + + service.cancelDisconnectWhenUnused('conn-1'); + closeDataExplorer('sqlite:conn-1:table:flights'); + + expect(service.getInstanceForProfile('conn-1')).toBeDefined(); + }); + + it('is a no-op for a profile with no live connection', () => { + service.addUpdateProfile(createProfile('conn-1')); + + expect(() => service.disconnectWhenUnused('conn-1')).not.toThrow(); + }); + }); + + describe('removeProfile', () => { + it('closes the connection of a removed profile', async () => { + await connectProfile([]); + + service.removeProfile('conn-1'); + + // Removing the profile takes away the only UI for managing its connection, so leaving + // the connection open would leak it for the rest of the session. + expect(service.getProfile('conn-1')).toBeUndefined(); + await vi.waitFor(() => { + expect(service.getInstanceForProfile('conn-1')).toBeUndefined(); + }); + }); + + it('closes the Data Explorers previewed from a removed profile', async () => { + const instance = await connectProfile([ + 'sqlite:conn-1:table:flights', + 'sqlite:conn-1:table:airports', + ]); + await service.previewNode(instance.connectionHandle, 7); + await service.previewNode(instance.connectionHandle, 8); + + service.removeProfile('conn-1'); + + // Their backends die with the connection, so they would only survive as grids that + // error on the next interaction. + await vi.waitFor(() => { + expect(openDatasetIds.size).toBe(0); + }); + await vi.waitFor(() => { + expect(service.getInstanceForProfile('conn-1')).toBeUndefined(); + }); + }); + + it('is a no-op for a profile that does not exist', () => { + expect(() => service.removeProfile('missing')).not.toThrow(); + }); + }); + }); });