plg: display errors when autocomplete rate limits trigger #2135
Conversation
kalanchan
requested review from
toolmantim,
philipp-spiess,
DanTup,
abeatrix and
a team
| @@ -575,7 +575,16 @@ export class InlineCompletionItemProvider implements vscode.InlineCompletionItem | |||
| */ | |||
| private onError(error: Error): void { | |||
| if (error instanceof RateLimitError) { | |||
| if (this.resetRateLimitErrorsAfter && this.resetRateLimitErrorsAfter > Date.now()) { | |||
| let hasRateLimitError | |||
| const allStatusBarErrors = this.config.statusBar.checkErrors() | |||
There was a problem hiding this comment.
I think this could be simplified to something like:
const hasRateLimitError = this.config.statusBar.checkErrors().find(statusError => statusError.error.errorType === error.name)
(although maybe it's worth wrapping up as adding this.config.statusBar.hasError(error.name)?)
There was a problem hiding this comment.
thats why you're my TL! thanks danny I made your suggestion
vscode/src/services/StatusBar.ts
Outdated
| checkErrors(): { | ||
| error: StatusBarError | ||
| createdAt: number | ||
| }[] |
There was a problem hiding this comment.
Any reason to use a method checkErrors() rather than just using errors (exposing the underlying field)? I ask because the name feels a little odd to me - it's getting the errors, not really checking anything.
There was a problem hiding this comment.
good call! changed this to hasError in the latest commit
There was a problem hiding this comment.
by design the headers retry-after return back the same time as when the request was made.
Wait this doesn't really make sense, shouldn't it be a time in the future at which we can retry the requests again? If we set it to the same time, what value does the header bring?
I’m pretty sure it worked like this before 😅
vscode/src/services/StatusBar.ts
Outdated
| checkErrors(): { | ||
| error: StatusBarError | ||
| createdAt: number | ||
| }[] |
There was a problem hiding this comment.
Can this be done by setting a bit in the inline-completion-item-provider and then using the callbacks to reset it and avoid exposing this interface?
|
I think something with the rate limited test account might be incorrect as it says "limit will reset in less than a minute". This isn't how our rate limiter works AFAIK? |
There is clearly something I misunderstand about this header 😆 Should we remove the text that depends on the |
|
Getting some clarity here: https://sourcegraph.slack.com/archives/C05AGQYD528/p1701859689434749 |
vscode/src/services/StatusBar.ts
Outdated
| @@ -9,6 +10,7 @@ import { FeedbackOptionItems } from './FeedbackOptions' | |||
| interface StatusBarError { | |||
| title: string | |||
| description: string | |||
| errorType: string | RateLimitError | |||
There was a problem hiding this comment.
| errorType: string | RateLimitError | |
| errorType: string |
That should not be set to a class I assume? 🤔
There was a problem hiding this comment.
I changed this to type StatusBarErrorName = 'auth' | 'RateLimitError' for now (these are the only two we use), since it felt a little less error-prone than just string.
Also pushed some fixes to the tests because the mocks didn't have the hasError impl.
|
@kalanchan pushed some tweaks (363a716), think this is good to merge? |
|
@philipp-spiess @DanTup THANKS ALL for reviewing and helping, feels good to be contributing back to the team 💪 |
Closes #2134
This bug has 2 parts to it:
retry-afterreturn back the same time as when the request was made. This causes the error messages to constantly be displayed in the quick pick viewThis PR adds a check to see whether or not the errors have been added in the
statusBarand to display the rate limited error if no otherRateLimitErroris present. This also makes sure the error is only displayed once, instead of spamming the user if autocomplete keeps getting triggered.Before (bug 1):
CleanShot.2023-12-05.at.17.41.29.mp4
Before (bug2, after hardcoding a later
retry-afterdate):CleanShot.2023-12-05.at.17.35.22.mp4
After:
CleanShot.2023-12-05.at.17.38.25.mp4
Test plan
https://sourcegraph.comusing the free demo account (thread)