Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the SSO login form to improve the user experience with password managers by allowing form submission before the Turnstile captcha completes. The key change is to use the getResponsePromise() method to wait for captcha completion upon submission rather than blocking the submit button until the captcha is verified.
Changes:
- Modified captcha handling to fetch the token asynchronously during form submission instead of requiring completion before enabling the submit button
- Removed captchaToken from the form schema and state management, handling it separately at submission time
- Changed form fields visibility approach using the
invisibleclass instead of conditional rendering
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| apps/landing/components/auth/LoginOrSignUp.tsx | Updated form submission to call getResponsePromise() for captcha token; removed captchaToken from form schema; changed submit button to be enabled based on hasCheckedSso instead of finishedCaptcha; wrapped form fields in invisible span |
| apps/landing/components/auth/Captcha.tsx | Removed forwardRef wrapper, onChange, and onStateChange props; added isCaptchaRequired helper function; simplified component to just render Turnstile without state management |
Comments suppressed due to low confidence (2)
apps/landing/components/auth/LoginOrSignUp.tsx:481
- The span element is not a semantic HTML element for grouping form fields. Consider using a div instead of span, as span is typically used for inline content, not block-level form fields.
<span className={classNames('space-y-6', { invisible: !hasCheckedSso })}>
{action === 'register' && (
<Input
label="Full Name"
error={(errors as FieldErrors<RegisterForm>)?.name?.message}
inputProps={{
type: 'text',
required: true,
spellCheck: 'false',
autoComplete: 'name',
...register('name'),
}}
/>
)}
<Input
label="Password"
error={errors?.password?.message}
inputProps={{
type: showPasswordActive ? 'text' : 'password',
required: true,
autoComplete: action === 'login' ? 'current-password' : 'new-password',
spellCheck: 'false',
minLength: action === 'register' ? PASSWORD_MIN_LENGTH : 8,
maxLength: 255,
autoFocus: !ssoInfo?.available,
...register('password'),
}}
>
{action !== 'register' && (
<div className="flex items-center justify-between">
<Checkbox inputProps={{ ...register('rememberMe') }}>Remember Me</Checkbox>
<ShowPasswordButton isActive={showPasswordActive} onClick={() => setShowPasswordActive(!showPasswordActive)} />
</div>
)}
</Input>
{action === 'register' && (
<Input
label="Confirm Password"
error={(errors as FieldErrors<RegisterForm>)?.confirmPassword?.message}
inputProps={{
type: showPasswordActive ? 'text' : 'password',
required: true,
autoComplete: 'new-password',
minLength: PASSWORD_MIN_LENGTH,
maxLength: 255,
...register('confirmPassword'),
}}
>
<div className="flex items-center justify-between">
<Checkbox inputProps={{ ...register('rememberMe') }}>Remember Me</Checkbox>
<ShowPasswordButton isActive={showPasswordActive} onClick={() => setShowPasswordActive(!showPasswordActive)} />
</div>
</Input>
)}
{action === 'register' && watchPassword && (
<PasswordStrengthIndicator password={watchPassword} confirmPassword={watchConfirmPassword} email={watchEmail} />
)}
<div className="flex items-center justify-end">
<ForgotPasswordLink />
</div>
{hasCheckedSso && <Captcha ref={captchaRef} action={action} />}
<button
type="submit"
className="flex w-full justify-center rounded-md bg-blue-600 px-3 py-1.5 text-sm font-semibold leading-6 text-white shadow-xs hover:bg-blue-700 disabled:bg-gray-300 disabled:cursor-not-allowed focus-visible:outline-solid focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-blue-600"
disabled={!hasCheckedSso || isSubmitting}
>
{action === 'login' ? 'Sign in' : 'Sign up'}
</button>
</span>
apps/landing/components/auth/Captcha.tsx:23
- The isCaptchaRequired function checks both ENVIRONMENT.CAPTCHA_KEY and the playwright condition, but lines 22-23 also check ENVIRONMENT.CAPTCHA_KEY again using the same logic. This creates redundant checking since isCaptchaRequired() already handles both conditions. The condition should be simplified to just check isCaptchaRequired().
if (!ENVIRONMENT.CAPTCHA_KEY || !isCaptchaRequired()) {
return null;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface CaptchaProps { | ||
| formError?: string; | ||
| ref: React.Ref<TurnstileInstance>; | ||
| formError?: Maybe<string>; | ||
| action: string; | ||
| onLoad?: () => void; | ||
| /** | ||
| * Called once captcha has been successfully completed | ||
| */ | ||
| onChange: (token: string) => void; | ||
| /** | ||
| * Called once captcha has been successfully completed | ||
| * Called immediately if captcha is disabled | ||
| */ | ||
| onStateChange: (isFinished: boolean) => void; | ||
| } |
There was a problem hiding this comment.
The Captcha component API has been changed to remove onChange and onStateChange props, but PasswordResetInit.tsx still uses these props (line 150-151). This will cause a breaking change and PasswordResetInit.tsx will need to be updated to use the new pattern with getResponsePromise() like LoginOrSignUp.tsx does.
960acf4 to
f5fd6d8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (3)
apps/landing/components/auth/Captcha.tsx:46
- The
formErrorprop is still defined but is no longer being passed from the parent components (PasswordResetInit.tsx line 150 and LoginOrSignUp.tsx line 472). The error display logic on lines 42-46 will never be triggered. Consider either removing the unusedformErrorprop and its display logic, or passing error information from the parent components if captcha error feedback is still needed.
formError?: Maybe<string>;
action: string;
onLoad?: () => void;
}
export function isCaptchaRequired() {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return ENVIRONMENT.CAPTCHA_KEY && !(window as any)?.playwright;
}
export const Captcha = ({ action, formError, ref, onLoad }: CaptchaProps) => {
const id = useId();
// Skip rendering the captcha if we're running in Playwright or if the key is not set
// In real environments the server will still validate and prevent access if there isn't a valid token
if (!ENVIRONMENT.CAPTCHA_KEY || !isCaptchaRequired()) {
return null;
}
return (
<>
<Turnstile
id={id}
ref={ref}
siteKey={ENVIRONMENT.CAPTCHA_KEY}
options={{
action,
theme: 'light',
appearance: 'always',
size: 'flexible',
refreshExpired: 'auto',
feedbackEnabled: true,
}}
onWidgetLoad={onLoad}
/>
{formError && (
<p id={`${id}-error`} role="alert" className="mt-2 text-sm text-red-600">
{formError}
</p>
)}
apps/landing/components/auth/Captcha.tsx:22
- The condition
!ENVIRONMENT.CAPTCHA_KEY || !isCaptchaRequired()is redundant. TheisCaptchaRequired()function already checksENVIRONMENT.CAPTCHA_KEY, so the first part of this OR condition will always be caught by the second part. Either remove the first check or remove the redundant check insideisCaptchaRequired().
if (!ENVIRONMENT.CAPTCHA_KEY || !isCaptchaRequired()) {
apps/landing/components/auth/LoginOrSignUp.tsx:480
- The submit button wrapper
divwas removed in LoginOrSignUp.tsx but retained in PasswordResetInit.tsx (line 152). This creates an inconsistency in the styling and layout approach across similar authentication forms. Consider either wrapping the button in LoginOrSignUp.tsx or removing the wrapper in PasswordResetInit.tsx for consistency.
<button
type="submit"
className="flex w-full justify-center rounded-md bg-blue-600 px-3 py-1.5 text-sm font-semibold leading-6 text-white shadow-xs hover:bg-blue-700 disabled:bg-gray-300 disabled:cursor-not-allowed focus-visible:outline-solid focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-blue-600"
disabled={!hasCheckedSso || isSubmitting}
>
{action === 'login' ? 'Sign in' : 'Sign up'}
</button>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Allow submission prior to turnstile completion and wait for completion upon submission this provides a better password management experience
f5fd6d8 to
342a730
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
apps/landing/components/auth/Captcha.tsx:22
- The condition is redundant since isCaptchaRequired() already checks both ENVIRONMENT.CAPTCHA_KEY and the playwright window property. This check will always return false when isCaptchaRequired() returns false, making the second condition unnecessary.
if (!ENVIRONMENT.CAPTCHA_KEY || !isCaptchaRequired()) {
apps/landing/components/auth/PasswordResetInit.tsx:67
- The check for captchaToken after getting it from getResponsePromise() is redundant. If captchaToken is falsy, the error is thrown, so the subsequent line setting params with 'captchaToken || empty string' will never use the empty string fallback. Consider removing the '|| empty string' part on line 67.
params.set('captchaToken', captchaToken || '');
apps/landing/components/auth/LoginOrSignUp.tsx:155
- The check for captchaToken after getting it from getResponsePromise() is redundant. If captchaToken is falsy, the error is thrown, so the subsequent line setting params with 'captchaToken || empty string' will never use the empty string fallback. Consider removing the '|| empty string' part on line 155.
params.set('captchaToken', captchaToken || '');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Allow submission prior to turnstile completion
and wait for completion upon submission
this provides a better password management experience