Fix redirect logic in analytics platform #2531
Conversation
📝 WalkthroughWalkthroughThe changes refactor the redirection mechanism in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (12)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/platform/src/core/utils/protectedRoute.js (1)
102-107: Good separation of concerns with dedicated useEffect.Creating a separate useEffect for handling redirects follows the principle of separation of concerns and keeps the code more maintainable.
However, consider resetting the redirectToLogin state after navigation occurs to prevent potential issues if the component doesn't unmount:
useEffect(() => { if (redirectToLogin) { router.push(LOGIN_ROUTE); + setRedirectToLogin(false); } }, [redirectToLogin, router]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/platform/src/core/utils/protectedRoute.js(5 hunks)
🔇 Additional comments (6)
src/platform/src/core/utils/protectedRoute.js (6)
17-17: Good practice: Using a constant for the login route path.Using a constant instead of hardcoded strings improves maintainability. If the login route changes in the future, you'll only need to update it in one place rather than hunting down multiple occurrences.
27-27: Clean approach: Using state to manage redirect logic.Using a dedicated state variable to control redirects separates the decision to redirect from the actual redirection action. This is a cleaner architectural approach than directly calling navigation methods throughout the code.
79-87: Improved error handling with centralized redirect logic.The changes properly centralize the redirect logic by setting a state flag instead of directly calling router.push. This is more consistent and follows React's declarative approach to UI management.
82-83: Consider the timing of this redirect check.The check
if (!userCredentials.success)in the finally block might cause unexpected behavior. This will run regardless of whether the preceding code succeeded or failed, potentially triggering a redirect even if the error was unrelated to authentication.Consider moving this check outside the finally block or being more specific about when to redirect:
.catch((error) => { console.error('Google auth error:', error); setIsRedirecting(false); setRedirectToLogin(true); }) .finally(() => { - if (!userCredentials.success) setRedirectToLogin(true); });
93-94: Consistent redirect approach.Using
setRedirectToLogin(true)here maintains consistency with the new pattern established elsewhere in the file. Good consistency!
63-63: Ensure this reset occurs at the right time.This line resets
isRedirectingto false once the user session is successfully set up, which is appropriate. Make sure this matches your intended flow of showing/hiding the spinner during authentication.
|
New next-platform changes available for preview here |
|
New next-platform changes available for preview here |
This pull request includes changes to the
withAuthfunction insrc/platform/src/core/utils/protectedRoute.jsto improve the handling of user redirection during authentication. The most important changes include defining a constant for the login route, adding a state to manage redirection to the login page, and ensuring the redirection logic is handled in a separateuseEffecthook.Improvements to user redirection:
LOGIN_ROUTEfor the login path to avoid hardcoding the route multiple times.redirectToLoginto manage the redirection to the login page.redirectToLogininstead of directly pushing to the login route.useEffecthook to handle the deferred redirect to the login page based on theredirectToLoginstate.isRedirectingstate is reset tofalseupon successful authentication.Summary by CodeRabbit
New Features
Refactor