harden auth account and locale mutation UX#62
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
Pull request overview
This PR hardens authentication/account and locale-mutation UX by extracting Turnstile orchestration into a dedicated hook, making locale mutations return typed results with clearer pending/error behavior, tightening profile editor feedback and destructive-dialog focus behavior, and expanding production e2e coverage for these flows.
Changes:
- Extracted SignUpForm Turnstile token orchestration into
useTurnstileTokenand aligned busy/error feedback + test ids. - Made display-locale mutations return typed
InlineActionResultdata and updatedLocalePickerto use explicit pending/error state. - Improved profile account editor pending/inline feedback, destructive dialog initial focus behavior, and added/updated Playwright e2e tests.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/SignUpForm/useTurnstileToken.ts | New hook encapsulating Turnstile token acquisition, timeouts, and error state. |
| src/components/SignUpForm/SignUpForm.tsx | Integrates the new hook; standardizes busy/disabled state and adds test ids for validation/errors. |
| src/components/ProfileAccountSettings/ProfileAccountSettings.tsx | Adds explicit aria-busy, disables inputs while pending, and adds test ids for inline messages. |
| src/components/LocalePicker/LocalePicker.tsx | Switches to explicit pending state + optimistic selection with typed action result handling. |
| src/components/InputHint/InputHint.tsx | Extends props so callers can pass attributes like data-testid. |
| src/components/Input/Input.tsx | Tightens typing by introducing InputProps with an error prop and omitting invalid. |
| src/components/FormMessage/FormMessage.tsx | Adds role and aria-live for improved announcement semantics. |
| src/components/ButtonToDialog/ButtonToDialog.tsx | Ensures danger dialogs focus the cancel action first; adds dialog button test ids. |
| src/app/actions.ts | Updates setDisplayLocaleAction to return typed InlineActionResult data with localized errors. |
| e2e/profile.spec.ts | Adds coverage for email edit inline errors and danger-dialog focus behavior. |
| e2e/i18n.spec.ts | Improves locale-switch coverage and reduces reliance on arbitrary timeouts. |
| e2e/auth.spec.ts | Adds sign-up client validation + pending/server-error preservation coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setIsPending(true); | ||
|
|
||
| try { | ||
| const formData = new FormData(); | ||
| formData.set("locale", nextLocale); | ||
| const result = await setDisplayLocaleAction(formData); | ||
|
|
||
| if (!result.success || result.error) { | ||
| setSelectedLocale(locale); | ||
| setError(result.error || t("Errors.genericLater")); | ||
| setIsPending(false); | ||
| return; | ||
| } | ||
|
|
||
| window.location.reload(); | ||
| } catch (error) { | ||
| console.error("Error updating display locale:", error); | ||
| setError(t("Errors.genericLater")); | ||
| } | ||
| })(); | ||
| }); | ||
| window.location.reload(); | ||
| } catch (error) { |
There was a problem hiding this comment.
isPending is set to true when a locale change starts, but it is never reset on the success path (the code calls window.location.reload() and returns). If reload() is blocked or fails to navigate (e.g. in embedded contexts/tests), the select stays disabled and aria-busy remains true indefinitely. Consider resetting isPending in a finally block, or explicitly calling setIsPending(false) after the reload call (or before it, if you prefer not to re-enable the UI).
| setError(null); | ||
| }, []); | ||
|
|
||
| useEffect(() => clearTokenPromise, [clearTokenPromise]); |
There was a problem hiding this comment.
On unmount, the effect cleanup calls clearTokenPromise(), which nulls the stored resolvers/rejecters and clears the timeout, but it does not reject any in-flight waitForToken() promise. If a caller is awaiting requestToken() and the component unmounts (e.g. user navigates away mid-verification), that await can remain pending indefinitely. Consider rejecting any outstanding token promise during cleanup (and optionally clearing isWaitingForToken) so callers always settle.
| useEffect(() => clearTokenPromise, [clearTokenPromise]); | |
| useEffect( | |
| () => () => { | |
| rejectTokenPromise( | |
| new Error("Turnstile token request was cancelled because the component unmounted.") | |
| ); | |
| }, | |
| [rejectTokenPromise] | |
| ); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setIsWaitingForToken(false); | ||
| setError(errorMessage); |
There was a problem hiding this comment.
The timeout callback in scheduleTokenTimeout calls setIsWaitingForToken/setError unconditionally. If the component unmounts right as the timer fires, this can still attempt a state update after unmount (cleanup clears the timer, but it can’t stop a callback already executing). Guard the timer callback with isMountedRef.current (or move these state updates behind the existing mount check pattern used in requestToken).
| setIsWaitingForToken(false); | |
| setError(errorMessage); | |
| if (isMountedRef.current) { | |
| setIsWaitingForToken(false); | |
| setError(errorMessage); | |
| } |
| const waitForToken = useCallback( | ||
| (timeout = BACKGROUND_TOKEN_TIMEOUT) => | ||
| new Promise<string>((resolve, reject) => { | ||
| tokenResolverRef.current = resolve; | ||
| tokenRejecterRef.current = reject; | ||
| scheduleTokenTimeout(timeout); | ||
| }), |
There was a problem hiding this comment.
waitForToken overwrites tokenResolverRef/tokenRejecterRef without cancelling any in-flight token request. If requestToken is triggered twice before the first resolves (e.g. double-submit before the disabled state re-renders), the first promise can be left pending forever. Consider rejecting/clearing any existing promise before starting a new waitForToken (or internally track a request id and ignore stale callbacks).
| setIsWaitingForToken(true); | ||
| turnstileRef.current?.reset(); | ||
|
|
||
| try { | ||
| const tokenPromise = waitForToken(BACKGROUND_TOKEN_TIMEOUT); | ||
| turnstileRef.current?.execute(); |
There was a problem hiding this comment.
When enabled is true but turnstileRef.current is still null (e.g. user submits before the Turnstile component mounts), requestToken will wait for a token that can never arrive and eventually show a timeout. Consider failing fast if the instance isn’t available (set an error + return/throw) instead of executing a timeout-based wait.
| setIsWaitingForToken(true); | |
| turnstileRef.current?.reset(); | |
| try { | |
| const tokenPromise = waitForToken(BACKGROUND_TOKEN_TIMEOUT); | |
| turnstileRef.current?.execute(); | |
| const turnstile = turnstileRef.current; | |
| if (!turnstile) { | |
| const errorMessage = "Verification is not ready yet. Please try again."; | |
| if (isMountedRef.current) { | |
| setError(errorMessage); | |
| setIsWaitingForToken(false); | |
| setIsInteractive(false); | |
| } | |
| throw new Error(errorMessage); | |
| } | |
| setIsWaitingForToken(true); | |
| turnstile.reset(); | |
| try { | |
| const tokenPromise = waitForToken(BACKGROUND_TOKEN_TIMEOUT); | |
| turnstile.execute(); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const languageLabels: Record<string, string> = { | ||
| de: "Deutsch", | ||
| en: "English", | ||
| es: "Español", | ||
| fr: "Français", | ||
| "pt-BR": "Português (Brasil)", | ||
| }; |
There was a problem hiding this comment.
languageLabels duplicates the app’s localeLabels mapping (src/i18n/config.ts) and is typed as Record<string, string>, so it’s easy for test expectations to drift if labels/locales change. Consider importing and reusing localeLabels (and/or the Locale type) so the test stays in sync with the source of truth and gets compile-time checking for locale keys.
Summary
Validation
Notes