sample - #66
Conversation
chore: add SocialLink component
There was a problem hiding this comment.
Pull request overview
This pull request adds social account linking functionality to the user profile page, allowing users to link their Discord and Google accounts.
Changes:
- Added a new
SocialLinkcomponent that displays Discord and Google account linking options - Integrated the
SocialLinkcomponent into the Profile page - Implemented UI for showing linked/unlinked status with appropriate buttons
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| products/frontend/src/routes/Profile/index.tsx | Imports and renders the new SocialLink component within the profile page |
| products/frontend/src/routes/Profile/components/index.ts | Exports the SocialLink component for use in the Profile route |
| products/frontend/src/routes/Profile/components/SocialLink/index.tsx | Implements the social account linking component with state management and click handlers |
| products/frontend/src/routes/Profile/components/SocialLink/index.module.css | Provides styling for the social link component including responsive design |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <button className={styles.button} disabled={true}> | ||
| 連携済み | ||
| </button> | ||
| ) : ( | ||
| <button className={styles.button} onClick={handleDiscordLinkSocial} disabled={false}> | ||
| 連携する | ||
| </button> |
There was a problem hiding this comment.
The disabled prop is explicitly set to true and false, which is redundant. In React/JSX, you can simply use disabled without a value for true, and omit the prop entirely for false. This would make the code cleaner and more idiomatic.
| <button className={styles.button} disabled={true}> | ||
| 連携済み | ||
| </button> | ||
| ) : ( | ||
| <button className={styles.button} onClick={handleGoogleLinkSocial} disabled={false}> | ||
| 連携する | ||
| </button> |
There was a problem hiding this comment.
The disabled prop is explicitly set to true and false, which is redundant. In React/JSX, you can simply use disabled without a value for true, and omit the prop entirely for false. This would make the code cleaner and more idiomatic.
| @media screen and (min-width: 600px) { | ||
| .accountContainer { | ||
| display: flex; | ||
| gap: var(--space-2xl); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The media query for min-width: 600px repeats the same display: flex and gap properties that are already defined in the base .accountContainer rule (lines 8-9). This is redundant and can be removed. Only properties that differ from the base should be included in media queries.
| @media screen and (min-width: 600px) { | |
| .accountContainer { | |
| display: flex; | |
| gap: var(--space-2xl); | |
| } | |
| } |
| for (const account of session.data) { | ||
| setLinkedAccounts((prev) => [ | ||
| ...prev, | ||
| { provider: account.providerId as 'discord' | 'google', isLinked: true }, |
There was a problem hiding this comment.
The type assertion 'as "discord" | "google"' assumes that account.providerId will always be one of these two values. If the API returns other provider types or the providerId doesn't match exactly, this could lead to incorrect behavior. Consider validating the providerId before adding it to the state, or using a type guard to ensure type safety.
| } | ||
|
|
||
| .button { | ||
| font-size: var(--color-base); |
There was a problem hiding this comment.
The font-size property is incorrectly set to var(--color-base), which is a color value (#b09757). This should be a text size variable like var(--text-base) or var(--text-lg). This will cause the button text to render incorrectly.
| font-size: var(--color-base); | |
| font-size: var(--text-base); |
| for (const account of session.data) { | ||
| setLinkedAccounts((prev) => [ | ||
| ...prev, | ||
| { provider: account.providerId as 'discord' | 'google', isLinked: true }, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Calling setLinkedAccounts inside a loop will trigger multiple re-renders and may not capture all accounts correctly due to the asynchronous nature of state updates. The previous state may not include accounts added in earlier loop iterations. Consider building the complete array first and then setting state once, or use a functional update pattern that correctly accumulates all accounts.
| for (const account of session.data) { | |
| setLinkedAccounts((prev) => [ | |
| ...prev, | |
| { provider: account.providerId as 'discord' | 'google', isLinked: true }, | |
| ]); | |
| } | |
| const accounts: LinkedSocialAccount[] = session.data.map((account) => ({ | |
| provider: account.providerId as Provider, | |
| isLinked: true, | |
| })); | |
| setLinkedAccounts(accounts); |
| const handleDiscordLinkSocial = async () => { | ||
| await authClient.linkSocial({ | ||
| provider: 'discord', | ||
| callbackURL: `${import.meta.env.VITE_REDIRECT_URL satisfies string}profile`, | ||
| }); | ||
| }; | ||
| const handleGoogleLinkSocial = async () => { | ||
| await authClient.linkSocial({ | ||
| provider: 'google', | ||
| callbackURL: `${import.meta.env.VITE_REDIRECT_URL satisfies string}profile`, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The async functions handleDiscordLinkSocial and handleGoogleLinkSocial lack error handling. If authClient.linkSocial fails, the error will be unhandled. Consider adding try-catch blocks and providing user feedback via toast notifications, consistent with error handling patterns used elsewhere in the codebase (see products/frontend/src/routes/Profile/index.tsx lines 22, 33).
| useEffect(() => { | ||
| const fetchSession = async () => { | ||
| const session = await authClient.listAccounts(); | ||
| if (session.data) { | ||
| for (const account of session.data) { | ||
| setLinkedAccounts((prev) => [ | ||
| ...prev, | ||
| { provider: account.providerId as 'discord' | 'google', isLinked: true }, | ||
| ]); | ||
| } | ||
| } else { | ||
| console.log('No linked accounts found.'); | ||
| } | ||
| }; | ||
| fetchSession(); |
There was a problem hiding this comment.
The async function fetchSession inside useEffect lacks error handling. If authClient.listAccounts() fails, the error will be unhandled and could cause the component to fail silently. Consider adding a try-catch block and providing appropriate error feedback to the user, consistent with error handling patterns used elsewhere in the codebase.
| } else { | ||
| console.log('No linked accounts found.'); |
There was a problem hiding this comment.
Using console.log in production code is not recommended. The codebase doesn't use console.log elsewhere in the routes directory. If this logging is needed for debugging, it should be removed before merging. If it's intentional, consider using a proper logging library or removing it entirely since an empty data array is a valid state that doesn't require logging.
| } else { | |
| console.log('No linked accounts found.'); |
No description provided.