feat(amazonq): user cancellable lsp download #6573
Conversation
|
After some discussion, do we still want this? Cancelling the LSP download means the extension would become unusable. |
|
TBH I'm against this now after the meeting yesterday, I think we should still show progress but if the options are:
I'd much rather just have them always download the language server |
What happens if the download fails? We must handle that case anyway. So handling "Cancel" is trivial then. And if the network is very slow? The user has no way to cancel it? |
| @@ -82,7 +86,10 @@ export class HttpResourceFetcher implements ResourceFetcher<Response> { | |||
| `Error downloading ${this.logText()}: %s`, | |||
| error.message ?? error.code ?? error.response.statusText ?? error.response.status | |||
| ) | |||
| return undefined | |||
| if (this.params.swallowError) { | |||
There was a problem hiding this comment.
previously this is swallowing all errors from fetch. With this change we can detect and catch the user cancellation errors.
| retryOnFail: true, | ||
| retryOnFail: (error: Error) => { | ||
| // retry unless we got an user cancellation error | ||
| return !(error instanceof RequestCancelledError) |
There was a problem hiding this comment.
Purpose of this added waitUntil feature:
We need to stop download retries if its an user cancel error
But for all other error types from fetch, we continue retry.
packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts
Outdated
Show resolved
Hide resolved
| if (error === undefined) { | ||
| return typeof opt.retryOnFail === 'boolean' ? opt.retryOnFail : true | ||
| } | ||
| if (typeof opt.retryOnFail === 'function') { |
There was a problem hiding this comment.
Can we make it always be a function? That should simplify some logic, eliminate an overload, and eliminate some tests.
It's trivial for calls to pass () => true if they want a "constant".
packages/core/src/shared/resourcefetcher/httpResourceFetcher.ts
Outdated
Show resolved
Hide resolved
| throw new ToolkitError('Failed to download server from remote', { code: 'RemoteDownloadFailed' }) | ||
| } catch (err) { | ||
| if (err instanceof RequestCancelledError) { | ||
| throw new CancellationError('user') |
There was a problem hiding this comment.
Why not add err instanceof RequestCancelledError to isUserCancelledError ?
aws-toolkit-vscode/packages/core/src/shared/errors.ts
Lines 620 to 621 in 71650e5
Then you wouldn't need to catch-and-throw here. Instead the caller could just check isUserCancelledError , which is the expected pattern.
|
|
#6573 (comment) wasn't addressed |
Problem
the cancellation button on notification doesn't actually stop the LSP download
Solution
Route the notification timeout into
httpResourceFetchercall, so that the user cancellation event stops the download as well.Demo: non-cancelled and cancelled downloads:
Screen.Recording.2025-02-13.at.11.44.59.AM.mov
feature/xbranches will not be squash-merged at release time.