Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding connection state to database side panel #226

Merged
merged 62 commits into from
Jul 4, 2024

Conversation

daveajrussell
Copy link
Collaborator

@daveajrussell daveajrussell commented Jun 18, 2024

This PR introduces connection state to the database side panel.

At the moment, the side panel is fairly basic and will simply show a state for connected, even if under the hood the connection has failed.

In this change, the side panel becomes much smarter and hooks into newly emitting events from the underlying driver/database connection. The side panel will update itself when a database connection drops, when it reconnects and when it gives up trying to connect to the database.

Also introduced is instant connection status feedback when adding/editing/connecting to/toggling a Connection.. whether this is bad auth, a non existent database or an unreachable server.

In the side panel, when you connect to a connection you are first presented with a "Connecting" state.. the label next to the connection will briefly read connecting... until the connection promise resolves, at which point we switch the label to connected and show a success info message.

For connections that error (and are retriable), we will show a reconnecting... label and an accompanying warning message.

For connections that fail, we will remove the connecting state entirely and display an error message.

Copy link

changeset-bot bot commented Jun 18, 2024

⚠️ No Changeset found

Latest commit: 9d65833

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@daveajrussell
Copy link
Collaborator Author

@daveajrussell daveajrussell changed the title Prototyping connection state - simper approach Prototyping connection state v2 Jun 18, 2024
@daveajrussell daveajrussell force-pushed the dajr/prototype-connection-state-simple branch from 550b84c to 4a1fa54 Compare June 18, 2024 14:40
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changes in here support;

  • prefilled inputs when adding a new connection
  • refilling all possible inputs when editing a connection
  • highlighting server invalidated fields on save

@daveajrussell daveajrussell changed the title Prototyping connection state v2 Adding connection state to database side panel Jun 24, 2024
@daveajrussell daveajrussell marked this pull request as ready for review June 25, 2024 08:23
Copy link
Collaborator

@OskarDamkjaer OskarDamkjaer left a comment

Choose a reason for hiding this comment

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

Nice work! I think the approach makes sense and have gone through the code, but not yet tested the vsix yet

packages/schema-poller/src/connectionErrorHandler.ts Outdated Show resolved Hide resolved
packages/schema-poller/src/connectionErrorHandler.ts Outdated Show resolved Hide resolved
packages/schema-poller/src/connectionErrorHandler.ts Outdated Show resolved Hide resolved
packages/vscode-extension/README.md Show resolved Hide resolved
packages/schema-poller/src/schemaPoller.ts Outdated Show resolved Hide resolved
packages/vscode-extension/src/commandHandlers.ts Outdated Show resolved Hide resolved
packages/vscode-extension/src/extension.ts Show resolved Hide resolved
) {
_context = context;
_languageClient = languageClient;
_databaseConnectionManager = new SchemaPollerConnectionManager();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ncordon the vscode extension already has it's own driver instance here, you can create another one if you prefer, but I think it'd be nice to reuse this one as it's already got all the lifecycle (create, update, verify connectivity) set up

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well this wasn't my point. This in fact means we already have two drivers at the moment: one on the VSCode side and one embedded in the language server

Copy link
Collaborator

@ncordon ncordon left a comment

Choose a reason for hiding this comment

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

LGTM for the moment, but I need to better understand the flow of the application

packages/schema-poller/src/schemaPoller.ts Outdated Show resolved Hide resolved
packages/schema-poller/src/schemaPoller.ts Show resolved Hide resolved
packages/vscode-extension/CHANGELOG.md Show resolved Hide resolved
packages/vscode-extension/README.md Show resolved Hide resolved
packages/vscode-extension/src/webviews/connectionPanel.ts Outdated Show resolved Hide resolved
) {
_context = context;
_languageClient = languageClient;
_databaseConnectionManager = new SchemaPollerConnectionManager();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well this wasn't my point. This in fact means we already have two drivers at the moment: one on the VSCode side and one embedded in the language server

Copy link
Collaborator

@ncordon ncordon left a comment

Choose a reason for hiding this comment

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

It looks good to me.

Only one last thing: the package.json seems to have a lot of weird changes.

I'd recommend doing this to regenerate it again:

nvm use
npm i

Copy link
Collaborator

@ncordon ncordon left a comment

Choose a reason for hiding this comment

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

I've been trying out some interactions again and it all seems to be working correctly

@daveajrussell daveajrussell merged commit 690f150 into main Jul 4, 2024
4 checks passed
@daveajrussell daveajrussell deleted the dajr/prototype-connection-state-simple branch July 4, 2024 08:50
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.

None yet

3 participants