refactor(auth): Migrate auth to Tanstack Query#662
refactor(auth): Migrate auth to Tanstack Query#662CodeWithCJ merged 1 commit intoCodeWithCJ:mainfrom
Conversation
Summary of ChangesHello @Sim-sat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request continues the ongoing migration of the frontend application to use Tanstack Query, specifically focusing on the authentication pages. The primary goal is to modernize the data fetching and state management for various authentication flows, leading to a more robust and maintainable codebase. This refactoring centralizes authentication logic into reusable hooks and separates concerns, improving the overall architecture of the authentication module. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors authentication-related logic by introducing a new authKeys file for React Query keys and a useAuth.ts hook file that encapsulates various authentication API calls (login, register, magic link, OIDC, password reset, MFA factors) into custom React Query hooks. The Auth.tsx, ForgotPassword.tsx, and ResetPassword.tsx components were updated to utilize these new hooks, replacing direct authService calls and local state management for data fetching. This also involved removing explicit toast notifications and some error logging from these components, as success/error messages are now handled within the meta option of the React Query mutations. Additionally, the MagicLinkRequestDialog component was extracted into its own file. Review comments highlight the need to reintroduce error logging in ForgotPassword.tsx and ResetPassword.tsx for consistency and debugging, and to implement error handling for the useAuthSettings query in Auth.tsx to provide user feedback when login settings fail to load.
| useState(false); | ||
|
|
||
| const queryClient = useQueryClient(); | ||
| const { data: loginSettings } = useAuthSettings(); |
There was a problem hiding this comment.
The previous implementation handled errors when fetching login settings by displaying a warning to the user. This was removed in the refactoring. If the useAuthSettings query fails (e.g., due to a network error), the user currently receives no feedback. It would be beneficial to handle the error state from the query to improve the user experience, for instance by showing an error message in the UI.
There was a problem hiding this comment.
Not true is handled by the client directly.
| } catch (err: any) { | ||
| error( | ||
| loggingLevel, | ||
| 'ForgotPassword: Password reset request failed:', | ||
| err | ||
| ); | ||
| setMessage(err.message || 'An unexpected error occurred.'); | ||
| toast({ | ||
| title: 'Error', | ||
| description: err.message || 'Failed to send password reset email.', | ||
| variant: 'destructive', | ||
| }); | ||
| } finally { |
There was a problem hiding this comment.
The error logging was removed from this catch block. For consistency with other error handling in the application (like in Auth.tsx) and to aid in debugging, it's recommended to log the error here. You will also need to re-import error from @/utils/logging.
} catch (err: any) {
error(
loggingLevel,
'ForgotPassword: Password reset request failed:',
err
);
setMessage(err.message || 'An unexpected error occurred.');
}
There was a problem hiding this comment.
The hooks have logging defined in the query client
| } catch (err: any) { | ||
| error(loggingLevel, 'ResetPassword: Password reset failed:', err); | ||
| setMessage(err.message || 'An unexpected error occurred.'); | ||
| toast({ | ||
| title: 'Error', | ||
| description: err.message || 'Failed to reset password.', | ||
| variant: 'destructive', | ||
| }); | ||
| } finally { |
There was a problem hiding this comment.
Similar to ForgotPassword.tsx, the error logging has been removed from this catch block. It's best to log this error for debugging purposes and to maintain consistency across the codebase. Please also re-import error from @/utils/logging.
} catch (err: any) {
error(loggingLevel, 'ResetPassword: Password reset failed:', err);
setMessage(err.message || 'An unexpected error occurred.');
}
There was a problem hiding this comment.
The hooks have logging defined in the query client
Migration of the next part of the app to Tanstack query following the same procedure.
Sim-sat
force-pushed
the
migrate-auth
branch
from
708201f to
5688ce5
Compare
|
@Sim-sat there were some custom logic from global settings and user settings and it dynamically use to show OIDC, MFA or email one time code , backup code etc. Are they all tested? |
2 participants
Description
This commit continues the migration with the auth pages. I didn't touch the passkey and mfa functions since they already use the better auth client.
Part of #657