fix(oauth2): keep the consent buttons disabled while the redirect is in flight - #3203
ChiragAgg5k wants to merge 1 commit into
Conversation
…in flight The approve and reject handlers reset their busy flag right after handing the browser a redirect, so the buttons re-enabled while the client was still loading. On a slow client (a Sites preview cold-starting behind edge) the user could press Authorize again and get a grant-not-found error for a flow that was succeeding.
Console (appwrite/console)Project ID: Tip Preview deployments create instant URLs for every branch and commit |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| {#if approving || redirecting} | ||
| <Spinner size="s" /> | ||
| {:else} | ||
| <Icon icon={IconCheck} slot="start" size="s" /> | ||
| {/if} | ||
| {approving ? 'Authorizing…' : 'Authorize'} | ||
| {redirecting ? 'Redirecting…' : approving ? 'Authorizing…' : 'Authorize'} | ||
| </Button> | ||
| <Button fullWidth secondary disabled={busy} on:click={reject}> | ||
| {rejecting ? 'Cancelling…' : 'Cancel'} |
There was a problem hiding this comment.
After Cancel starts a web redirect, finally clears rejecting while redirecting remains true. Because only the Authorize button renders the shared redirect state, Cancel returns to its idle label while Authorize misleadingly shows the spinner and “Redirecting…”. Track which action started the redirect so its progress appears on the correct button.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/(public)/oauth2/consent-card.svelte
Line: 851-859
Comment:
**Cancel progress appears elsewhere**
After Cancel starts a web redirect, `finally` clears `rejecting` while `redirecting` remains true. Because only the Authorize button renders the shared redirect state, Cancel returns to its idle label while Authorize misleadingly shows the spinner and “Redirecting…”. Track which action started the redirect so its progress appears on the correct button.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| onMount(() => { | ||
| // A back navigation restores this page from the bfcache with `redirecting` | ||
| // still set; the user must be able to act again. | ||
| const restore = (event: PageTransitionEvent) => { | ||
| if (event.persisted) redirecting = false; | ||
| }; | ||
| window.addEventListener('pageshow', restore); | ||
| return () => window.removeEventListener('pageshow', restore); | ||
| }); |
There was a problem hiding this comment.
The new redirect lifecycle has no automated observable-behavior coverage. Regressions in the persisted pageshow reset or either slow redirect path could leave the controls disabled or show progress on the wrong action without being caught. Add browser-level tests for slow approve and reject redirects plus back-navigation restoration, without asserting internal state flags.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/(public)/oauth2/consent-card.svelte
Line: 86-94
Comment:
**Redirect lifecycle lacks coverage**
The new redirect lifecycle has no automated observable-behavior coverage. Regressions in the persisted `pageshow` reset or either slow redirect path could leave the controls disabled or show progress on the wrong action without being caught. Add browser-level tests for slow approve and reject redirects plus back-navigation restoration, without asserting internal state flags.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Closing: the consent page users actually hit is served by the new console; the fix moves there. |
1 participant
What
The consent card re-enabled its Authorize and Cancel buttons right after assigning
window.location.href, because thefinallyblock reset the busy flag before the browser had actually left the page. The browser keeps the consent page on screen until the redirect target returns its first byte. For a Sites preview that is the whole edge callback chain plus the runtime cold start, which can take many seconds, so the user could keep pressing Authorize.A second press hits
POST /oauth2/console/grants/:id/approveon a grant whose code was already issued, which Cloud rejects with grant-not-found. The user then sees an error toast for a flow that is in fact succeeding.Fix
redirectingstate that is set right before the web redirect is issued and folded intobusy, so both buttons stay disabled for the whole redirect chain and cold start.Redirecting…on the Authorize button while in that state.redirectingonpageshowwhen the page is restored from the bfcache after a back navigation, so the card is not left stuck disabled.The already-consented fast path in
consent/+page.sveltealready stays in the loading phase during a web redirect, so it is unchanged.Test plan
Redirecting…and stays disabled until the site rendersVerification
Reproduced with Playwright against the dev server: the console API is mocked,
approvereturns a redirect to a route that never sends its first byte (a preview cold-starting behind edge), then the Authorize button is pressed once and three more times over the next ~2 seconds.Authorize, enabledRedirecting…, disabled