Skip to content
This repository was archived by the owner on Sep 21, 2026. It is now read-only.

fix(oauth2): keep the consent buttons disabled while the redirect is in flight #3203

Closed
ChiragAgg5k wants to merge 1 commit into from
Closed

fix(oauth2): keep the consent buttons disabled while the redirect is in flight #3203

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions src/routes/(public)/oauth2/consent-card.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<script lang="ts">
import type { Models } from '@appwrite.io/console';
import { onMount } from 'svelte';
import { Card, Layout, Typography, Icon, Spinner } from '@appwrite.io/pink-svelte';
import {
IconCheck,
Expand Down Expand Up @@ -76,7 +77,21 @@
let error = $state<string | null>(null);
let approving = $state(false);
let rejecting = $state(false);
let busy = $derived(approving || rejecting);
// Set once the browser has been handed a web redirect. The consent page stays
// on screen until the client's first byte arrives, which on a cold start can
// take a while: the buttons must stay disabled for that whole stretch.
let redirecting = $state(false);
let busy = $derived(approving || rejecting || redirecting);

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);
});
Comment on lines +86 to +94

@greptile-apps greptile-apps Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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.

Fix in Claude Code Fix in Codex


// The `scope` param carries every requested privilege. Scopes are shown
// read-only — the client decides what it asks for. authorization_details
Expand Down Expand Up @@ -349,8 +364,9 @@
onDone?.('approved', result.redirectUrl);
return;
}
redirecting = isWebRedirect(result.redirectUrl);
window.location.href = result.redirectUrl;
if (!isWebRedirect(result.redirectUrl)) {
if (!redirecting) {
onDone?.('approved', result.redirectUrl);
}
} catch (e: unknown) {
Expand All @@ -377,8 +393,9 @@
onDone?.('denied', result.redirectUrl);
return;
}
redirecting = isWebRedirect(result.redirectUrl);
window.location.href = result.redirectUrl;
if (!isWebRedirect(result.redirectUrl)) {
if (!redirecting) {
onDone?.('denied', result.redirectUrl);
}
} catch (e: unknown) {
Expand Down Expand Up @@ -831,12 +848,12 @@
<Form onSubmit={approve}>
<Layout.Stack gap="s">
<Button fullWidth submit disabled={busy || blocked}>
{#if approving}
{#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'}
Comment on lines +851 to 859

@greptile-apps greptile-apps Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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!

Fix in Claude Code Fix in Codex

Expand Down
Loading