Skip to content

Connected indicator, Data Explorer-aware disconnect, and remove confirmation - #15241

Open
softwarenerd wants to merge 2 commits into
mainfrom
14623-data-connections-how-to-disconnect
Open

Connected indicator, Data Explorer-aware disconnect, and remove confirmation#15241
softwarenerd wants to merge 2 commits into
mainfrom
14623-data-connections-how-to-disconnect

Conversation

@softwarenerd

Copy link
Copy Markdown
Contributor

Fixes #14623

Summary

Removing a data connection deleted its saved profile but left the connection itself open. Because the profile's row was gone from the Data Connections panel, there was no longer any UI that could close it, so the underlying connection stayed open and unreachable for the rest of the session. Remove now closes the connection as part of removing the profile.

That turned out to need an answer to a broader question: when should a connection close? This PR settles the whole lifetime.

Collapsing a connection gives up the panel's use of it and closes it, as before -- unless Data Explorers previewed from that connection are still open, in which case the connection stays up and closes when the last of them is closed. Collapsing is how a user reclaims the panel's vertical space, so previews opened from a connection keep working across a collapse. Re-expanding cancels the pending close. Connection entries now show an indicator when they have a live connection, so a connection kept open behind a collapsed entry is visible rather than a surprise.

Remove now asks for confirmation first, naming the connection and how many open Data Explorers will close with it, since it deletes saved settings and stored secrets and cannot be undone. On confirmation it closes those Data Explorers, then the connection, then the profile. The Data Explorers are closed rather than left open because their backends die with the connection -- a tab left behind would be a grid that errors on the next scroll or filter.

For core to know which Data Explorers belong to which connection, DataConnectionNode.preview() now returns the dataset id it opened the explorer under (the same id passed to positron.dataExplorer.open). The return value is optional, so a driver that reports nothing still works; it just can't hold its connection open. All six in-repo drivers report it. Core records those ids per profile and asks the editor service which still have an editor open, so closing a Data Explorer tab is immediately reflected.

Supporting changes: a new DestructiveTwoButtonFooter for confirmation dialogs whose primary action can't be undone. It fills the primary button with the destructive red (positronModalDialog.buttonDestructiveBackground now resolves to the same red a destructive context menu item uses) and focuses Cancel rather than the destructive button, so Enter and Escape both back out. TwoButtonFooter is unchanged.

A dedicated Disconnect action is deliberately not added here (idea 2 in the issue). Collapse and Remove both close the connection now, so the gap the issue describes is closed without a third control; a separate Disconnect item can follow if it's still wanted.

Note for reviewers: this overlaps #15215 (Refresh / Refresh All) in four files, plus a hasLoadedChildren helper both branches add to PositronTreeInstance that git auto-merges into a duplicate. This branch will rebase onto main after #15215 lands.

Screenshots

Connected data connection indicated with a green dot on the right:

image

Remove connection confirmation when there are no Data Explorer instances open:

image

Data connection with a Data Explorer open:

image

Remove connection confirmation when there is one Data Explorer instance open:

image

Remove connection confirmation when there are more than one Data Explorer instances open:

image

Release Notes

New Features

Bug Fixes

Validation Steps

@:connections @:data-explorer

  1. Create a SQLite or DuckDB connection and expand it in the Data Connections panel. Confirm the entry shows the connected indicator.
  2. Double-click a table to open it in the Data Explorer, then collapse the connection. The Data Explorer must keep working (scroll, sort, filter) and the entry must still show the connected indicator.
  3. Re-expand the connection. It should expand immediately, without re-fetching, and stay connected.
  4. Collapse it again, then close the Data Explorer tab. The connected indicator should disappear, and re-expanding should reconnect and re-fetch the tree.
  5. Open two Data Explorers from one connection and collapse it. Closing the first must leave the connection open; closing the second must close it.
  6. Collapse a connection with no Data Explorers open. It should disconnect immediately.
  7. With a Data Explorer open, choose Remove from the connection's Actions menu. The dialog must name the connection and say one Data Explorer will close. Confirm that Cancel is focused, that Escape and Enter both dismiss it without removing, and that confirming closes the Data Explorer tab and removes the connection.
  8. Remove a connection with no Data Explorers open, and confirm the dialog omits the sentence about Data Explorers.

@github-actions

Copy link
Copy Markdown

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:connections @:data-explorer @:connect

Why these tags?
Tag Source
@:critical Always runs (required)
@:connections PR description
@:data-explorer PR description
@:connect Changed files

More on automatic tags from changed files.

readme  valid tags

@timtmok timtmok left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data Connections - Provide a way to disconnect from the database in addition to removing

2 participants