Remember remote queries across restarts #1162
Conversation
aeisenberg
force-pushed
the
aeisenberg/remote-query-restart
branch
from
579d210 to
cf0728a
Compare
Remote query items will be stored in query history and will remain available across restarts. When the extension is restarted, any `InProgress` remote queries will be monitored until they complete. When clicked on, a remote query is opened and its results can be downloaded. The query text and the query file can be opened from the history menu. A remote query can be deleted as well, which will purge all results from global storage. Limitations: 1. Labels are not editable 2. Running multiple queries that each run on the same repository will have conflicting results and there will be errors when trying to view the results of the second query. This limitation is not new, but it is easier to hit now. See #1089. Both of these limitations will be addressed in future PRs.
aeisenberg
force-pushed
the
aeisenberg/remote-query-restart
branch
from
cf0728a to
5e06a61
Compare
| private async handleOpenQueryItem(queryItem: QueryHistoryInfo) { | ||
| if (queryItem?.t === 'remote') { | ||
| try { | ||
| const remoteQueryResult = await this.retrieveJsonFile(queryItem, 'query-result.json') as RemoteQueryResult; |
There was a problem hiding this comment.
Did you mean to have this instead?
| const remoteQueryResult = await this.retrieveJsonFile(queryItem, 'query-result.json') as RemoteQueryResult; | |
| const remoteQueryResult = await this.retrieveJsonFile<RemoteQueryResult>(queryItem, 'query-result.json'); |
| this.push(this.qhm.onWillOpenQueryItem(this.handleOpenQueryItem.bind(this))); | ||
| } | ||
|
|
||
| private async handleAddQueryItem(queryItem: QueryHistoryInfo) { |
There was a problem hiding this comment.
A convention I've been following that I'd love to see (and we can discuss about whether we agree/want to enforce it with a linter) is to have public functions before private functions. I find that easier when I'm reading through code because I can easily see the public interface of a module.
I'd be great if we could do the same with these functions, but ignore me if you disagree/would cause too much merge pain.
| }; | ||
| } | ||
|
|
||
| public async openResults(query: RemoteQuery, queryResult: RemoteQueryResult) { |
There was a problem hiding this comment.
The public functions of the RemoteQueriesManager are runRemoteQuery, monitorRemoteQuery, etc. so perhaps a better name for this function would be openRemoteQuery or showRemoteQuery.
| await fs.writeFile(filePath, JSON.stringify(obj, null, 2), 'utf8'); | ||
| } | ||
|
|
||
| private async retrieveJsonFile<T>(queryHistoryItem: RemoteQueryHistoryItem, fileName: string): Promise<T> { |
There was a problem hiding this comment.
Is retrieveQueryJson a better name for this?
| export class RemoteQueryHistoryItem { | ||
| readonly t = 'remote'; | ||
| failureReason: string | undefined; | ||
| export interface RemoteQueryHistoryItem { |
There was a problem hiding this comment.
Yay! Very happy to see this being turned back into a plain interface!
| this.remoteQueriesMonitor = new RemoteQueriesMonitor(ctx, logger); | ||
|
|
||
| // Handle events from the query history manager | ||
| this.push(this.qhm.onDidAddQueryItem(this.handleAddQueryItem.bind(this))); |
There was a problem hiding this comment.
🤔 There is a level of indirection that I feel we should be able to simplify: we have a logical circular dependency between the two managers (RemoteQueryManager and QueryHistoryManager) because the two need to work together - the responsibilities get a bit blurred. For example, handleAddQueryItem will be called for non remote queries so we have to check that. It'd be great if there was better isolation there (i.e. never call that for non remote queries).
What you have here works 🎉 so in the sake of speed, I think we should go ahead with what we've got, but I have some ideas about how we could simplify this and make it a bit more readable/maintainable
Remote query items will be stored in query history and will remain
available across restarts.
When the extension is restarted, any
InProgressremote queries willbe monitored until they complete.
When clicked on, a remote query is opened and its results can be
downloaded. The query text and the query file can be opened from the
history menu. A remote query can be deleted as well, which will purge
all results from global storage.
Limitations:
will have conflicting results and there will be errors when trying
to view the results of the second query. This limitation is not new,
but it is easier to hit now. See Download and process analyses results #1089.
Both of these limitations will be addressed in future PRs.
Checklist
ready-for-doc-reviewlabel there.