Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 7 additions & 2 deletions apps/remix/app/routes/_unauthenticated+/signin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export default function SignIn({ loaderData }: Route.ComponentProps) {

const [searchParams] = useSearchParams();
const [isEmbeddedRedirect, setIsEmbeddedRedirect] = useState(false);
const [isRedirectFailed, setIsRedirectFailed] = useState(false);

const errorParam = searchParams.get('error');
const signupError = errorParam ? SIGNUP_ERROR_MESSAGES[errorParam] : undefined;
Expand Down Expand Up @@ -128,10 +129,14 @@ export default function SignIn({ loaderData }: Route.ComponentProps) {
return;
}

void authClient.oidc.signIn({ redirectPath: returnTo ?? '/' });
authClient.oidc.signIn({ redirectPath: returnTo ?? '/' }).catch(() => {
// Fall back to the manual form (retry button, break-glass door) instead
// of leaving the user on the spinner forever when the IdP is unreachable.
setIsRedirectFailed(true);
});
}, [shouldRedirectToOIDC, returnTo]);

if (shouldRedirectToOIDC) {
if (shouldRedirectToOIDC && !isRedirectFailed) {
return (
<div className="w-screen max-w-lg px-4">
<div className="flex flex-col items-center justify-center gap-y-4 py-12">
Expand Down
26 changes: 22 additions & 4 deletions apps/remix/app/routes/_unauthenticated+/signup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { isValidReturnTo, normalizeReturnTo } from '@documenso/lib/utils/is-vali
import { msg } from '@lingui/core/macro';
import { Trans } from '@lingui/react/macro';
import { Loader2Icon } from 'lucide-react';
import { useEffect } from 'react';
import { useEffect, useState } from 'react';
import { redirect, useSearchParams } from 'react-router';

import { SignUpForm } from '~/components/forms/signup';
Expand Down Expand Up @@ -78,24 +78,42 @@ export default function SignUp({ loaderData }: Route.ComponentProps) {
} = loaderData;

const [searchParams] = useSearchParams();
const [isRedirectFailed, setIsRedirectFailed] = useState(false);
Comment on lines 80 to +81

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

To prevent users in embedded contexts from getting stuck on the spinner forever, we need to track whether the signup is embedded. Let's add the isEmbeddedRedirect state and its corresponding useEffect to detect the #embedded=true hash on mount, matching the implementation in signin.tsx.

Suggested change
const [searchParams] = useSearchParams();
const [isRedirectFailed, setIsRedirectFailed] = useState(false);
const [searchParams] = useSearchParams();
const [isRedirectFailed, setIsRedirectFailed] = useState(false);
const [isEmbeddedRedirect, setIsEmbeddedRedirect] = useState(false);
useEffect(() => {
const hash = window.location.hash.slice(1);
const params = new URLSearchParams(hash);
setIsEmbeddedRedirect(params.get('embedded') === 'true');
}, []);

const [isEmbeddedRedirect, setIsEmbeddedRedirect] = useState(false);

// Suppress the automatic redirect when the user asked for the manual form
// via ?direct=1, or when a previous OIDC attempt bounced back with an error
// (avoids a redirect loop).
const isDirectEntry = searchParams.get('direct') === '1';
const hasIdpError = searchParams.get('error') !== null;

const shouldRedirectToOIDC = shouldAutoRedirectToOIDC && !isDirectEntry && !hasIdpError;
useEffect(() => {
const params = new URLSearchParams(window.location.hash.slice(1));

setIsEmbeddedRedirect(params.get('embedded') === 'true');
}, []);

const shouldRedirectToOIDC = shouldAutoRedirectToOIDC && !isDirectEntry && !hasIdpError && !isEmbeddedRedirect;

useEffect(() => {
if (!shouldRedirectToOIDC) {
return;
}

void authClient.oidc.signIn({ redirectPath: returnTo ?? '/' });
// Guard against the initial render racing the embedded detection above:
// read the hash synchronously so embedded contexts never bounce to the IdP.
if (new URLSearchParams(window.location.hash.slice(1)).get('embedded') === 'true') {
return;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

authClient.oidc.signIn({ redirectPath: returnTo ?? '/' }).catch(() => {
// Fall back to the manual form instead of leaving the user on the
// spinner forever when the IdP is unreachable.
setIsRedirectFailed(true);
});
}, [shouldRedirectToOIDC, returnTo]);

if (shouldRedirectToOIDC) {
if (shouldRedirectToOIDC && !isRedirectFailed) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Since shouldRedirectToOIDC is not updated to include !isEmbeddedRedirect (as that line is outside the diff hunks), we should guard the spinner rendering here by checking !isEmbeddedRedirect. This ensures that if the signup is embedded, we immediately render the manual form instead of showing the infinite spinner.

Suggested change
if (shouldRedirectToOIDC && !isRedirectFailed) {
if (shouldRedirectToOIDC && !isRedirectFailed && !isEmbeddedRedirect) {

return (
<div className="w-screen max-w-lg px-4">
<div className="flex flex-col items-center justify-center gap-y-4 py-12">
Expand Down
Loading