[frontend] Fix redirect issue after signin by using router.replace from client component #246
Conversation
…om client component
|
Warning Rate Limit Exceeded@usatie has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 11 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates involve modifying the authentication process in a web application. Instead of redirecting users from the server-side after successful authentication, the application now marks them as Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -7,6 +7,7 @@ import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card"; | |||
| import { Input } from "@/components/ui/input"; | |||
| import { Label } from "@/components/ui/label"; | |||
| import { useFormState, useFormStatus } from "react-dom"; | |||
| import { useRouter } from "next/navigation"; | |||
There was a problem hiding this comment.
Ensure that the import path for useRouter is correct. The correct import path from Next.js is "next/router", not "next/navigation".
- import { useRouter } from "next/navigation";
+ import { useRouter } from "next/router";Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| import { useRouter } from "next/navigation"; | |
| import { useRouter } from "next/router"; |
frontend/app/ui/auth/login-form.tsx
Outdated
| const router = useRouter(); | ||
| if (code === "Authenticated") { | ||
| router.replace("/"); | ||
| } |
There was a problem hiding this comment.
The redirection logic based on the code state value is correctly implemented. However, consider adding a dependency array to a useEffect hook to avoid potential issues with this logic executing more than once or at an unintended time.
+ import { useEffect } from 'react';
+
+ useEffect(() => {
+ if (code === "Authenticated") {
+ router.replace("/");
+ }
+ }, [code, router]);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const router = useRouter(); | |
| if (code === "Authenticated") { | |
| router.replace("/"); | |
| } | |
| import { useEffect } from 'react'; | |
| useEffect(() => { | |
| if (code === "Authenticated") { | |
| router.replace("/"); | |
| } | |
| }, [code, router]); |
There was a problem hiding this comment.
Since it's going to redirect, there is no way this code is going to be executed twice?
There was a problem hiding this comment.
@takumihara: You're correct that in this specific case, the redirection should ideally only occur once due to the nature of the navigation action. However, wrapping the redirection logic in a useEffect hook with [code, router] as dependencies is a best practice in React development. It ensures that the effect runs exactly once when the code state changes to "Authenticated", providing a safeguard against potential unexpected re-renders before the navigation completes. This approach enhances the predictability and maintainability of the code, especially in more complex applications where state changes and effects might interact in less straightforward ways. It's about adhering to best practices that help prevent subtle bugs in larger, more complex scenarios.
| // This is a temporary fix for the redirect issue. It seems that the redirect is not working as expected. | ||
| // Instead of redirecting from server action, router.refresh from client component is used. | ||
| // Maybe this is related?: https://github.com/vercel/next.js/issues/58263 | ||
| // redirect('/', 'layout'); | ||
| return "Authenticated"; |
There was a problem hiding this comment.
Remove commented-out code if it's no longer needed to keep the codebase clean and maintainable.
- // This is a temporary fix for the redirect issue. It seems that the redirect is not working as expected.
- // Instead of redirecting from server action, router.refresh from client component is used.
- // Maybe this is related?: https://github.com/vercel/next.js/issues/58263
- // redirect('/', 'layout');Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // This is a temporary fix for the redirect issue. It seems that the redirect is not working as expected. | |
| // Instead of redirecting from server action, router.refresh from client component is used. | |
| // Maybe this is related?: https://github.com/vercel/next.js/issues/58263 | |
| // redirect('/', 'layout'); | |
| return "Authenticated"; | |
| return "Authenticated"; |
#237
redirectするとNavBarが更新されないrouter.replaceすることで対応した