refactor(continue-page): simplify useEffect to avoid unnecessary dependencies#641
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces guarded, derived redirect flags and a controlled auto-redirect flow in the continue page: adds redirect guard state, a redirectToTarget callback with repeated-redirect protection, delays auto-redirect and reveal of manual option, and updates conditional rendering and early logout when redirect is invalid. Changes
Sequence DiagramsequenceDiagram
actor User
participant ContinuePage
participant Browser as window.location
User->>ContinuePage: Load page / continue
ContinuePage->>ContinuePage: Compute flags (hasValidRedirect, trusted, httpsDowngrade, shouldAutoRedirect)
alt !hasValidRedirect
ContinuePage->>Browser: Navigate to logout
else hasValidRedirect
alt shouldAutoRedirect (logged in, no warnings)
ContinuePage->>ContinuePage: start short delay (100ms)
ContinuePage->>ContinuePage: set timer to reveal manual option (5s)
ContinuePage->>ContinuePage: check hasRedirected guard
ContinuePage->>Browser: window.location.assign(url)
else show warning or require manual action
ContinuePage->>User: show untrusted/insecure warning + manual button
User->>ContinuePage: clicks manual redirect
ContinuePage->>ContinuePage: check hasRedirected guard
ContinuePage->>Browser: window.location.assign(url)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 #641 +/- ##
=======================================
Coverage 16.35% 16.35%
=======================================
Files 45 45
Lines 3431 3431
=======================================
Hits 561 561
Misses 2813 2813
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/pages/continue-page.tsx`:
- Around line 46-53: The redirect callback is being recreated each render
because useRedirectUri returns a new URL object each time, which causes the
effect that manages the reveal timeout to reset; fix this by stabilizing the url
dependency: either (A) change redirectToTarget and any deps to depend on the
primitive url?.href (use url?.href in the useCallback dependency array and in
the useEffect that sets timeouts) so the ref is stable across renders, or (B)
memoize the URL inside useRedirectUri (wrap the returned URL in useMemo keyed on
redirect_uri and cookieDomain) so consumers like redirectToTarget receive a
stable URL object; update references to hasRedirected, redirectToTarget, the
useEffect that controls showRedirectButton, and any timeout-clearing logic
accordingly.
steveiliop56
requested changes
Feb 15, 2026
dep in the callback
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/pages/continue-page.tsx`:
- Around line 57-60: handleRedirect sets isLoading true unconditionally but
never resets it if redirectToTarget is a no-op; change the logic so we only set
loading when a redirect will occur or we always clear loading when
redirectToTarget bails. Specifically, either (a) make redirectToTarget return a
boolean/promise indicating whether a redirect happened and in handleRedirect
call setIsLoading(true) only if it will redirect (or setIsLoading(false) when it
returns false), or (b) check hasRedirected.current before setting loading in
handleRedirect, or wrap the call in try/finally and call setIsLoading(false)
when no navigation occurred; reference handleRedirect, redirectToTarget,
hasRedirected.current, and setIsLoading when making the change.
Comment on lines
+57
to
+60
| const handleRedirect = useCallback(() => { | ||
| setIsLoading(true); | ||
| redirectToTarget(); | ||
| }, [redirectToTarget]); |
Contributor
There was a problem hiding this comment.
isLoading is never reset if redirectToTarget is a no-op.
If handleRedirect is called but redirectToTarget bails out (e.g., hasRedirected.current is already true), isLoading stays true and the button remains in its loading state permanently. Given the rendering guards this is very unlikely to surface, but worth noting.
🤖 Prompt for AI Agents
In `@frontend/src/pages/continue-page.tsx` around lines 57 - 60, handleRedirect
sets isLoading true unconditionally but never resets it if redirectToTarget is a
no-op; change the logic so we only set loading when a redirect will occur or we
always clear loading when redirectToTarget bails. Specifically, either (a) make
redirectToTarget return a boolean/promise indicating whether a redirect happened
and in handleRedirect call setIsLoading(true) only if it will redirect (or
setIsLoading(false) when it returns false), or (b) check hasRedirected.current
before setting loading in handleRedirect, or wrap the call in try/finally and
call setIsLoading(false) when no navigation occurred; reference handleRedirect,
redirectToTarget, hasRedirected.current, and setIsLoading when making the
change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors the redirect logic in the
ContinuePagecomponent to improve clarity, maintainability, and user experience. The main changes include consolidating redirect condition checks, improving naming for readability, and streamlining the auto-redirect and warning logic.Summary by CodeRabbit
Bug Fixes
Refactor
Chores