fix(): sleep before redirect #9079
Conversation
packages/twenty-front/src/modules/domain-manager/hooks/useRedirect.ts
Outdated
Show resolved
Hide resolved
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces a sleep delay before redirects to ensure proper cookie and state updates across the application's navigation flows.
- Added new
useRedirecthook in/packages/twenty-front/src/modules/domain-manager/hooks/useRedirect.tsthat enforces a sleep(0) delay before redirects - Modified
/packages/twenty-front/src/modules/auth/hooks/useAuth.tsto use the new redirect hook for SSO and authentication flows - Added sleep(0) in
/packages/twenty-front/src/modules/domain-manager/hooks/useGetPublicWorkspaceDataBySubdomain.tserror handler before default domain redirect - Replaced direct
window.location.hrefassignments withuseRedirecthook across multiple components for consistent redirect handling - Removed unused User repository dependency from
/packages/twenty-server/src/engine/core-modules/sso/services/sso.service.ts
11 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
| setLastAuthenticateWorkspaceDomain(null); | ||
| await sleep(0); | ||
| redirectToDefaultDomain(); |
There was a problem hiding this comment.
style: Consider adding error details to console.error and documenting why sleep(0) is necessary here
| import { sleep } from '~/utils/sleep'; | ||
|
|
||
| export const useRedirect = () => { | ||
| const redirect = (url: string) => { |
There was a problem hiding this comment.
logic: missing URL validation - could allow redirect to malicious domains
| // Don't use this hook directly! Prefer the high level hooks like: | ||
| // useRedirectToDefaultDomain and useRedirectToWorkspaceDomain |
There was a problem hiding this comment.
style: if this hook shouldn't be used directly, consider making it internal or private to prevent misuse
|
|
||
| export const useRedirectToDefaultDomain = () => { | ||
| const { defaultDomain } = useReadDefaultDomainFromConfiguration(); | ||
| const { redirect } = useRedirect(); | ||
| const redirectToDefaultDomain = () => { | ||
| const url = new URL(window.location.href); |
There was a problem hiding this comment.
logic: URL constructor may throw if window.location.href is malformed. Consider wrapping in try/catch.
| if (url.hostname !== defaultDomain) { | ||
| url.hostname = defaultDomain; | ||
| window.location.href = url.toString(); | ||
| redirect(url.toString()); | ||
| } |
There was a problem hiding this comment.
style: consider adding error handling in case redirect() fails or throws
|
|
||
| export const useRedirectToWorkspaceDomain = () => { | ||
| const isMultiWorkspaceEnabled = useRecoilValue(isMultiWorkspaceEnabledState); | ||
| const { buildWorkspaceUrl } = useBuildWorkspaceUrl(); | ||
| const { redirect } = useRedirect(); | ||
|
|
||
| const redirectToWorkspaceDomain = ( | ||
| subdomain: string, | ||
| pathname?: string, | ||
| searchParams?: Record<string, string>, | ||
| ) => { | ||
| if (!isMultiWorkspaceEnabled) return; |
There was a problem hiding this comment.
logic: No return value when isMultiWorkspaceEnabled is false could cause unexpected behavior in calling code
| onCompleted: (data) => { | ||
| window.location.href = data.authorizeApp.redirectUrl; | ||
| redirect(data.authorizeApp.redirectUrl); | ||
| }, |
There was a problem hiding this comment.
logic: no error handling for failed authorization - should catch and handle errors from authorizeApp mutation
| await clearSession(); | ||
| setCurrentUser(user); | ||
| setTokenPair(tokens); | ||
| await sleep(0); // This hacky workaround is necessary to ensure the tokens stored in the cookie are updated correctly. | ||
| window.location.href = AppPath.Index; | ||
| redirect(AppPath.Index); |
There was a problem hiding this comment.
logic: setCurrentUser and setTokenPair state updates may not be reflected before the redirect occurs. Consider awaiting a state update confirmation or using a useEffect.
| setError('Failed to impersonate user. Please try again.'); | ||
| setIsLoading(false); |
There was a problem hiding this comment.
style: setIsLoading(false) is not called if clearSession() fails. Consider using try/finally to ensure loading state is always reset.
|
Thanks @AMoreaux for your contribution! |
Summary
This Pull Request centralizes the redirection logic by introducing a reusable
useRedirecthook, which replaces direct usage ofwindow.location.hrefwith more standardized and testable functionality across multiple modules.useRedirecthook for handling redirection logic with optional controlled delays.useAuth, workspace, and settings-related hooks, etc.) to use the newly introduceduseRedirector related high-level hooks.