refactor: rework frontend use effect calls#630
Conversation
📝 WalkthroughWalkthroughAdds a new frontend hook to parse and validate redirect URIs (domain trust, protocol, HTTPS downgrade), removes a small URL utility, and updates auth pages to use the hook, unify redirect URL construction, introduce one-time redirect guards, and move timer cleanup into effect-scoped handlers. (50 words) Changes
Sequence Diagram(s)(Skipped — changes are a frontend refactor and per-page control-flow adjustments that don't introduce a new multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #630 +/- ##
=======================================
Coverage 16.16% 16.16%
=======================================
Files 45 45
Lines 3427 3427
=======================================
Hits 554 554
Misses 2816 2816
Partials 57 57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@frontend/src/pages/continue-page.tsx`:
- Around line 83-90: The redirect URL construction uses the raw search param
variable redirectUri (from searchParams.get) which can be null and yields
"/loginnull"; update the conditional to explicitly check redirectUri for
null/empty before appending the query string (e.g. use a ternary like
redirectUri ? `?redirect_uri=${encodeURIComponent(redirectUri)}` : "") so the
Navigate target is `/login` when redirectUri is absent; change the code around
the Navigate JSX in continue-page.tsx that references redirectUri to use that
explicit check.
In `@frontend/src/pages/login-page.tsx`:
- Around line 159-167: The effect watching redirectTimer and redirectButtonTimer
never returns a cleanup so timers set later can leak; update the useEffect in
login-page.tsx (the one referencing redirectTimer and redirectButtonTimer) to
use an empty dependency array and return a cleanup function that checks
redirectTimer.current and redirectButtonTimer.current and calls clearTimeout on
each (and optionally sets .current = null) so both timeouts are reliably cleared
on unmount.
In `@frontend/src/pages/logout-page.tsx`:
- Around line 42-44: The useEffect that currently checks redirectTimer.current
but doesn't return a cleanup should be changed to return a cleanup function that
clears the pending timeout on unmount (i.e., call
clearTimeout(redirectTimer.current) if set); update the effect for the stable
ref by using an empty dependency array and ensure the cleanup clears
redirectTimer.current after clearing so window.location.replace won't fire after
unmount — locate the effect using useEffect and the redirectTimer ref to
implement this.
In `@frontend/src/pages/totp-page.tsx`:
- Around line 62-64: The current useEffect in totp-page.tsx that tries to clear
redirectTimer is a no-op because redirectTimer ref identity is stable; change
the effect to run on totpPending (not the ref) and return a cleanup that clears
the timeout held in redirectTimer.current so the pending redirect is cancelled
if totpPending flips to false (reference redirectTimer and totpPending in your
effect, and ensure you call clearTimeout(redirectTimer.current) in the returned
cleanup). This ensures the timeout set elsewhere (where you call setTimeout for
Navigate / window.location.replace) is properly cleared when totpPending
changes.
🧹 Nitpick comments (3)
frontend/src/lib/hooks/redirect-uri.ts (1)
42-55: Prefer strict equality (===) over loose equality (==).Lines 43, 49, and 53 use
==. While both operands are strings so the result is identical,===is the idiomatic TypeScript convention and avoids any ambiguity.Proposed diff
if ( - url.hostname == cookieDomain || + url.hostname === cookieDomain || url.hostname.endsWith(`.${cookieDomain}`) ) { isTrusted = true; } - if (url.protocol == "http:" || url.protocol == "https:") { + if (url.protocol === "http:" || url.protocol === "https:") { isAllowedProto = true; } - if (window.location.protocol == "https:" && url.protocol == "http:") { + if (window.location.protocol === "https:" && url.protocol === "http:") { isHttpsDowngrade = true; }frontend/src/pages/login-page.tsx (1)
58-61: State is inferred asstring | falseinstead ofboolean.
... !== undefined && props.redirect_urievaluates to the redirect URI string (truthy) orfalse. This works at runtime in boolean contexts, but the inferred type isstring | false, which muddies the intent. A!!coercion keeps it cleanly boolean.Proposed fix
const [isOauthAutoRedirect, setIsOauthAutoRedirect] = useState( providers.find((provider) => provider.id === oauthAutoRedirect) !== - undefined && props.redirect_uri, + undefined && !!props.redirect_uri, );frontend/src/pages/continue-page.tsx (1)
35-39: Non-null assertion onurlis safe today but fragile.
url!relies on rendering guards (!valid || !allowedProto→ Navigate to/logout) ensuringurlis defined beforehandleRedirectcan be called. If the rendering order changes, this would throw. A guard clause would be more defensive:Optional defensive check
const handleRedirect = useCallback(() => { + if (!url) return; hasRedirected.current = true; setIsLoading(true); - window.location.assign(url!); + window.location.assign(url); }, [url]);
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/login-page.tsx (1)
199-207:⚠️ Potential issue | 🟡 Minor
oauthData?.data.urlcan beundefined, causing navigation to literal"undefined".
window.location.replace(undefined)coerces towindow.location.replace("undefined"). WhileshowRedirectButtonis only set insideonSuccess, the optional chaining onoauthData?.data.urlsignals the value isn't guaranteed. If for any reasonoauthDatais cleared (e.g. a re-render resets query state), the user would navigate to"undefined".Proposed fix — guard the click handler
<Button onClick={() => { - window.location.replace(oauthData?.data.url); + if (oauthData?.data.url) { + window.location.replace(oauthData.data.url); + } }} >
🧹 Nitpick comments (3)
frontend/src/pages/continue-page.tsx (2)
35-39:url!non-null assertion is fragile — guard againstundefinedinstead.
urlisundefinedwhenvalidisfalse(the hook only sets it on successful parse). Although the current effect guards and early-return JSX paths should preventhandleRedirectfrom being called with an undefinedurl, the non-null assertion silently masks the invariant. A small future refactor (e.g., changing effect conditions or adding a new call site) could trigger a runtime crash.Proposed fix — add a runtime guard
const handleRedirect = useCallback(() => { + if (!url) return; hasRedirected.current = true; setIsLoading(true); - window.location.assign(url!); + window.location.assign(url); }, [url]);
41-81: Effect dependency array includes stable refs and state setters — harmless but noisy.
hasRedirected(auseRef),setIsLoading, andsetShowRedirectButton(state setters) are stable across renders and never trigger re-runs. Including them in the dep array is unnecessary noise. Moderneslint-plugin-react-hookswon't require them either. Consider trimming to the values that actually change.frontend/src/pages/login-page.tsx (1)
58-61:isOauthAutoRedirectis typedstring | falserather thanboolean.The
&&expression yields the right-handstringoperand when the left is truthy. This works at runtime due to JS truthiness, but astring-typed state can be confusing (e.g.,setIsOauthAutoRedirect(false)on Line 98 mixes types). Wrapping in!!keeps it a clean boolean.- const [isOauthAutoRedirect, setIsOauthAutoRedirect] = useState( - providers.find((provider) => provider.id === oauthAutoRedirect) !== - undefined && props.redirect_uri, - ); + const [isOauthAutoRedirect, setIsOauthAutoRedirect] = useState( + providers.find((provider) => provider.id === oauthAutoRedirect) !== + undefined && !!props.redirect_uri, + );
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores