[Feature] OAuth implementation (Twitter & Github) - #682
Conversation
WalkthroughThis update primarily focuses on integrating OAuth functionalities for Twitter and GitHub, enhancing user profile management in the web application. Changes include the addition of OAuth fields in user profiles, new OAuth-related UI components, and updates to various backend services to support OAuth linking and unlinking. Changes
Poem
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}
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- packages/shared/package.json (1 hunks)
- packages/shared/src/types/profile.ts (1 hunks)
- packages/web/src/assets/icons/social/x.icon.js (1 hunks)
- packages/web/src/constants/constants.ts (1 hunks)
- packages/web/src/languages/en.json (1 hunks)
- packages/web/src/navigation/paths.ts (1 hunks)
- packages/web/src/navigation/routes.tsx (2 hunks)
- packages/web/src/pages/HackerProfile/HackerProfilePage/HackerProfilePage.tsx (7 hunks)
- packages/web/src/pages/HackerProfile/HackerProfilePage/styles.ts (2 hunks)
- packages/web/src/pages/HackerProfile/hooks.ts (2 hunks)
- packages/web/src/pages/HackerProfile/profilesService.ts (1 hunks)
- packages/web/src/pages/_oauth/GithubOauth.tsx (1 hunks)
- packages/web/src/pages/_oauth/TwitterOauth.tsx (1 hunks)
- packages/web/src/pages/_oauth/router.tsx (1 hunks)
Files skipped from review due to trivial changes (4)
- packages/shared/package.json
- packages/web/src/assets/icons/social/x.icon.js
- packages/web/src/constants/constants.ts
- packages/web/src/navigation/paths.ts
Additional comments not posted (20)
packages/shared/src/types/profile.ts (1)
12-15: The addition of theoauthfield to theIHackerProfileinterface is correctly implemented to support OAuth integration for Twitter and GitHub.packages/web/src/pages/_oauth/router.tsx (1)
1-22: TheoauthRouterfunction is correctly implemented with appropriate routes for OAuth operations, ensuring proper navigation and component rendering for GitHub and Twitter OAuth processes.packages/web/src/navigation/routes.tsx (1)
Line range hint
8-25: The integration ofoauthRouterinto the main routes configuration is correctly implemented, ensuring that OAuth-specific routes are part of the application's routing structure.packages/web/src/pages/_oauth/GithubOauth.tsx (1)
1-31: TheGithubOauthcomponent is correctly implemented to handle the OAuth process for GitHub, including code retrieval, local storage interaction, and navigation post-authentication.packages/web/src/pages/_oauth/TwitterOauth.tsx (1)
1-32: TheTwitterOauthcomponent is correctly implemented to handle the OAuth process for Twitter, including state and code verification, local storage interaction, and navigation post-authentication.packages/web/src/pages/HackerProfile/hooks.ts (1)
101-127: The new hooksuseLinkOAuthanduseUnlinkOAuthare correctly implemented to manage the linking and unlinking of OAuth accounts to user profiles, using theuseMutationhook for asynchronous operations.packages/web/src/pages/HackerProfile/profilesService.ts (1)
133-165: The functionslinkOAuthToProfileandunlinkOAuthFromProfileare correctly implemented to handle the linking and unlinking of OAuth accounts, with appropriate error handling and interaction with the backend API.packages/web/src/pages/HackerProfile/HackerProfilePage/styles.ts (1)
Line range hint
125-287: The new styles for.oauth-connectorsand.button-containerare correctly implemented, enhancing the UI for OAuth functionalities and providing a consistent user experience.packages/web/src/pages/HackerProfile/HackerProfilePage/HackerProfilePage.tsx (7)
7-7: Import ofXIconis added to replaceTwitterIcon.
10-10: Import ofLocalStoragefrom constants is crucial for managing OAuth data in local storage.
17-17: Import ofRoutePathsis used for navigation, which is essential for handling OAuth redirections.
28-34: Introduction of new hooks related to OAuth and address linking/unlinking.
50-50: StateoAuthLinkingis introduced to manage the current OAuth process. Ensure proper reset of this state to avoid stale state issues.Verification successful
The verification process has confirmed that the
setOAuthLinkingfunction is used appropriately within theHackerProfilePage.tsxfile to manage the state ofoAuthLinking. The state is reset toundefinedin multiple places, which aligns with the best practices for managing state in React to avoid stale state issues.Proper Reset of
oAuthLinkingState Verified:
- The state is initialized with
useState.- It is conditionally set based on the
oauthvariable.- It is explicitly reset to
undefinedin necessary scenarios to ensure no stale state.This confirms that the original review comment concern has been addressed correctly in the code.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper reset of `oAuthLinking` state across all components. # Test: Search for all occurrences of `setOAuthLinking`. Expect: Proper reset in all relevant places. rg --type tsx $'setOAuthLinking'Length of output: 59
Script:
#!/bin/bash # Description: Verify proper reset of `oAuthLinking` state across all components. # Test: Search for all occurrences of `setOAuthLinking` in TypeScript files. rg --type ts 'setOAuthLinking'Length of output: 531
65-66: HooksuseLinkOAuthanduseUnlinkOAuthare used for managing OAuth operations. Ensure these hooks handle all edge cases, such as network failures or unauthorized access.Verification successful
The hooks
useLinkOAuthanduseUnlinkOAuthare indeed implemented in thehooks.tsfile, and both utilize theuseMutationhook from@tanstack/react-query. This setup is generally used to handle asynchronous operations, such as network requests, which are common in OAuth operations.Analysis of
useLinkOAuthanduseUnlinkOAuth:
- Error Handling: Both hooks use the
useMutationhook, which inherently supports error handling by capturing errors during the mutation operation and providing them in the mutation result. This allows the consuming components to handle errors appropriately, such as displaying error messages or taking other corrective actions.- Unauthorized Access: While the hooks themselves do not explicitly handle unauthorized access, this is typically managed by the backend services (
linkOAuthToProfileandunlinkOAuthFromProfile). The frontend hook would receive an error if the backend service rejects the operation due to authorization issues.- Network Failures: The
useMutationhook also handles network failures by default. If the network request fails, the error is captured and can be handled by the components using these hooks.Given this information, it appears that the hooks are designed to handle edge cases like network failures and unauthorized access through the mechanisms provided by
useMutation. This aligns with best practices for handling such cases in React components using hooks.Conclusion:
The hooks
useLinkOAuthanduseUnlinkOAuthare implemented with appropriate error handling mechanisms for typical OAuth operation issues, including network failures and unauthorized access. This confirms that the original review comment concern has been addressed in the implementation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in `useLinkOAuth` and `useUnlinkOAuth` hooks. # Test: Retrieve implementations of these hooks. Expect: Comprehensive error handling. ast-grep --lang typescript --pattern $'function useLinkOAuth($_, $_) { $$$ }' ast-grep --lang typescript --pattern $'function useUnlinkOAuth($_, $_) { $$$ }'Length of output: 159
Script:
#!/bin/bash # Broaden the search to find any occurrences of `useLinkOAuth` and `useUnlinkOAuth` in the codebase. ast-grep --lang typescript --pattern $'$$$ useLinkOAuth($$$) $$$' ast-grep --lang typescript --pattern $'$$$ useUnlinkOAuth($$$) $$$'Length of output: 1033
Script:
#!/bin/bash # Search for TypeScript files that might contain the hooks `useLinkOAuth` and `useUnlinkOAuth`. rg --files-with-matches 'useLinkOAuth' --glob '*.ts' --glob '*.tsx' rg --files-with-matches 'useUnlinkOAuth' --glob '*.ts' --glob '*.tsx'Length of output: 383
Script:
#!/bin/bash # Retrieve the content of the file where the hooks are defined to inspect their implementation. cat packages/web/src/pages/HackerProfile/hooks.tsLength of output: 3241
390-413: Modal for linking OAuth accounts. Ensure that the modal is accessible and properly handles all user interactions, especially when OAuth linking is in progress.packages/web/src/languages/en.json (5)
769-773: The added localization strings for OAuth functionality are correctly implemented and formatted.
769-769: The localization string for 'streakExplanationDescription' is clear and well-formatted.
770-770: The localization string for 'connectOauthAccount' is correctly implemented and provides flexibility for different OAuth providers.
771-772: The localization strings for 'unlinkOAuth' and 'unlinkOAuthExplanation' are well-implemented, providing clear instructions for unlinking OAuth accounts.
773-773: The localization string for 'linkYourAccount' is correctly implemented and formatted, enhancing the user interface for linking OAuth accounts.
| useEffect(() => { | ||
| const oauth = searchParams.get("oauth") as typeof oAuthLinking; | ||
| if (!oauth || !createdProfile) return; | ||
| if (linkOAuth.isLoading || linkOAuth.isSuccess) return; | ||
| if (isSigningIn || oAuthLinking) return; | ||
|
|
||
| // If the user is not the owner of the profile, redirect to the home page | ||
| if (createdProfile.username.toLowerCase() !== username?.toLowerCase()) { | ||
| return navigate(`/`); | ||
| } | ||
|
|
||
| // Check if account has already a connected oauth account | ||
| if (createdProfile.oauth?.[oauth]) { | ||
| return navigate(`${RoutePaths.profile}/${createdProfile.username}`); | ||
| } | ||
|
|
||
| const oauthData = JSON.parse(localStorage.getItem(`${LocalStorage.oauthData}_${oauth}`) ?? "null"); | ||
| if (!oauthData) return; | ||
|
|
||
| setOAuthLinking(oauth ?? undefined); | ||
| }, [searchParams, createdProfile, navigate, username, linkOAuth, isSigningIn, oAuthLinking]); |
There was a problem hiding this comment.
The effect handles OAuth linking based on URL parameters. It should also handle cases where oauth parameter is manipulated or invalid.
- if (!oauth || !createdProfile) return;
+ if (!oauth || !createdProfile || !['twitter', 'github'].includes(oauth)) return;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.
| useEffect(() => { | |
| const oauth = searchParams.get("oauth") as typeof oAuthLinking; | |
| if (!oauth || !createdProfile) return; | |
| if (linkOAuth.isLoading || linkOAuth.isSuccess) return; | |
| if (isSigningIn || oAuthLinking) return; | |
| // If the user is not the owner of the profile, redirect to the home page | |
| if (createdProfile.username.toLowerCase() !== username?.toLowerCase()) { | |
| return navigate(`/`); | |
| } | |
| // Check if account has already a connected oauth account | |
| if (createdProfile.oauth?.[oauth]) { | |
| return navigate(`${RoutePaths.profile}/${createdProfile.username}`); | |
| } | |
| const oauthData = JSON.parse(localStorage.getItem(`${LocalStorage.oauthData}_${oauth}`) ?? "null"); | |
| if (!oauthData) return; | |
| setOAuthLinking(oauth ?? undefined); | |
| }, [searchParams, createdProfile, navigate, username, linkOAuth, isSigningIn, oAuthLinking]); | |
| useEffect(() => { | |
| const oauth = searchParams.get("oauth") as typeof oAuthLinking; | |
| if (!oauth || !createdProfile || !['twitter', 'github'].includes(oauth)) return; | |
| if (linkOAuth.isLoading || linkOAuth.isSuccess) return; | |
| if (isSigningIn || oAuthLinking) return; | |
| // If the user is not the owner of the profile, redirect to the home page | |
| if (createdProfile.username.toLowerCase() !== username?.toLowerCase()) { | |
| return navigate(`/`); | |
| } | |
| // Check if account has already a connected oauth account | |
| if (createdProfile.oauth?.[oauth]) { | |
| return navigate(`${RoutePaths.profile}/${createdProfile.username}`); | |
| } | |
| const oauthData = JSON.parse(localStorage.getItem(`${LocalStorage.oauthData}_${oauth}`) ?? "null"); | |
| if (!oauthData) return; | |
| setOAuthLinking(oauth ?? undefined); | |
| }, [searchParams, createdProfile, navigate, username, linkOAuth, isSigningIn, oAuthLinking]); |
| const handleLinkOAuth = async () => { | ||
| if (!oAuthLinking || !profileFound) return; | ||
|
|
||
| const auth = await tryAuthentication(); | ||
| if (!auth) return; | ||
|
|
||
| const oauthParams = JSON.parse(localStorage.getItem(`${LocalStorage.oauthData}_${oAuthLinking}`) ?? "null"); | ||
| if (!oauthParams) return; | ||
|
|
||
| try { | ||
| await linkOAuth.mutateAsync({ | ||
| username: profileFound.username, | ||
| oauth: oAuthLinking, | ||
| oauthParams, | ||
| }); | ||
|
|
||
| queryClient.invalidateQueries({ queryKey: ["hacker-profile-username", profileFound.username] }); | ||
| queryClient.invalidateQueries({ queryKey: ["all-profiles"] }); | ||
| setOAuthLinking(undefined); | ||
|
|
||
| return navigate(`${RoutePaths.profile}/${profileFound.username}`); | ||
| } catch (error) { | ||
| console.error("ERROR linking oauth", error); | ||
| return navigate(`${RoutePaths.profile}/${profileFound.username}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Function handleLinkOAuth manages the OAuth linking process. It lacks error handling for JSON parsing which could throw an exception if localStorage data is corrupted.
- const oauthParams = JSON.parse(localStorage.getItem(`${LocalStorage.oauthData}_${oAuthLinking}`) ?? "null");
+ let oauthParams;
+ try {
+ oauthParams = JSON.parse(localStorage.getItem(`${LocalStorage.oauthData}_${oAuthLinking}`) ?? "null");
+ } catch (error) {
+ console.error("Failed to parse OAuth parameters", error);
+ return;
+ }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 handleLinkOAuth = async () => { | |
| if (!oAuthLinking || !profileFound) return; | |
| const auth = await tryAuthentication(); | |
| if (!auth) return; | |
| const oauthParams = JSON.parse(localStorage.getItem(`${LocalStorage.oauthData}_${oAuthLinking}`) ?? "null"); | |
| if (!oauthParams) return; | |
| try { | |
| await linkOAuth.mutateAsync({ | |
| username: profileFound.username, | |
| oauth: oAuthLinking, | |
| oauthParams, | |
| }); | |
| queryClient.invalidateQueries({ queryKey: ["hacker-profile-username", profileFound.username] }); | |
| queryClient.invalidateQueries({ queryKey: ["all-profiles"] }); | |
| setOAuthLinking(undefined); | |
| return navigate(`${RoutePaths.profile}/${profileFound.username}`); | |
| } catch (error) { | |
| console.error("ERROR linking oauth", error); | |
| return navigate(`${RoutePaths.profile}/${profileFound.username}`); | |
| } | |
| }; | |
| const handleLinkOAuth = async () => { | |
| if (!oAuthLinking || !profileFound) return; | |
| const auth = await tryAuthentication(); | |
| if (!auth) return; | |
| let oauthParams; | |
| try { | |
| oauthParams = JSON.parse(localStorage.getItem(`${LocalStorage.oauthData}_${oAuthLinking}`) ?? "null"); | |
| } catch (error) { | |
| console.error("Failed to parse OAuth parameters", error); | |
| return; | |
| } | |
| if (!oauthParams) return; | |
| try { | |
| await linkOAuth.mutateAsync({ | |
| username: profileFound.username, | |
| oauth: oAuthLinking, | |
| oauthParams, | |
| }); | |
| queryClient.invalidateQueries({ queryKey: ["hacker-profile-username", profileFound.username] }); | |
| queryClient.invalidateQueries({ queryKey: ["all-profiles"] }); | |
| setOAuthLinking(undefined); | |
| return navigate(`${RoutePaths.profile}/${profileFound.username}`); | |
| } catch (error) { | |
| console.error("ERROR linking oauth", error); | |
| return navigate(`${RoutePaths.profile}/${profileFound.username}`); | |
| } | |
| }; |
| {profileFound.twitter_username && ( | ||
| <a href={`https://twitter.com/${profileFound.twitter_username}`} {...defaultAnchorProps}> | ||
| <TwitterIcon /> | ||
| <XIcon /> |
There was a problem hiding this comment.
Usage of XIcon for Twitter profile link seems incorrect as per the previous icon used (TwitterIcon). This might be a visual inconsistency.
- <XIcon />
+ <TwitterIcon />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.
| <XIcon /> | |
| <TwitterIcon /> |
1 participant
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation