Improve user experience when no database is selected#3214
Conversation
norascheuch
left a comment
norascheuch
left a comment
There was a problem hiding this comment.
I think the flow looks amazing, just one minor question on the code. Thanks for making these changes! 💐
| ): Promise<DatabaseItem | undefined> { | ||
| if (this.databaseManager.currentDatabaseItem === undefined) { | ||
| await this.chooseAndSetDatabase(false, progress); | ||
| progress?.progress({ |
There was a problem hiding this comment.
Is it possible to remove one of the progress here? Similar to how it's done in https://github.com/github/vscode-codeql/blob/koesie10/code-server-docker-compose/extensions/ql-vscode/src/databases/database-fetcher.ts#L143
There was a problem hiding this comment.
It's because progress here is a ProgressContext instead of a regular ProgressCallback. I'm not totally sure why this code uses ProgressContext, but it doesn't seem to be something added by this PR.
You could do something like change the name of progress to progressContext (or whatever you want) and then do
const { progress, token } = progressContext;and then you can use progress and token as normal.
Or you could go further and basically remove the ProgressContext type. As far as I can tell, we only use it from this file, and we only ever use the progress field from it and never the token field. So arguably we could clean up this code and change all references to ProgressContext to just use ProgressCallback instead.
If that didn't make any sense I'm happy to explain more, or pair on it. We could also save it for a separate PR after this one.
There was a problem hiding this comment.
Thanks! For now, I've just renamed the first progress to progressContext to avoid some confusion. Agreed that the other changes would be nice, but slightly out of scope for this PR 😅
|
Drive by comment, shall we add some tests? Perhaps as a separate issue/PR. |
Yes, I'm keen to add tests as a follow-up PR! 🧪 If I don't get round to it in the next few days, I'll open a new issue too. |
4 participants
Previously, if you ran a query without having a DB selected, the extension would pop up a file explorer window prompting you to select a database archive from your file system. This isn't the most common way to select a database so it was a fairly confusing pop-up. See internal issue for more info 🔍
Changes
New flow (inspired by designs from the internal issue, though I updated the wording slightly):
In all cases, we select that DB and run the query against it 🎉 (Note: this also works in the same way for "CodeQL: Debug Query")
Notes
Checklist
ready-for-doc-reviewlabel there.