Add user profile image support across backend and frontend#553
Add user profile image support across backend and frontend#553nasagong wants to merge 5 commits intoyorkie-team:mainfrom
Conversation
|
|
📝 WalkthroughWalkthroughThis PR adds profile icon support throughout the application, capturing user profile images from social login providers (GitHub), persisting them in the database, and displaying them across various UI components and real-time collaborative features. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (4)
backend/src/users/types/user-domain.type.ts (1)
8-11: ExposeprofileIconin domain model looks good; consider fixing nearby typoThe new optional
profileIconwithApiPropertyaligns with the schema and API surface. While you’re in this block, you might also correct the description typo"Last worksace slug of user"→"Last workspace slug of user".frontend/src/hooks/useYorkieDocument.ts (1)
70-87: Profile icon is wired into Yorkie presence; verify behavior on mid‑session changesIncluding
profileIconininitialPresenceand keyingcreateYorkieDocumentoffuserStore.data?.profileIconmakes sense. Note, though, that the effect which callscreateYorkieDocumentbails out oncedocandclientare set, so subsequent profileIcon changes during a session won’t propagate into presence. If you expect avatars to update live when a user’s profile image changes, you may want to either (a) gate initialization onuserStore.databeing ready, or (b) explicitly update presence whenprofileIconchanges.frontend/src/pages/settings/profile/Index.tsx (1)
56-65: Preferundefinedover empty string for AvatarsrcfallbackTo avoid emitting an
<img src="">when there’s no profile image, consider:- src={userStore.data?.profileIcon || ""} + src={userStore.data?.profileIcon ?? undefined}MUI will still fall back to the initials, but without an unnecessary empty‑URL image load.
frontend/src/components/workspace/TableTab.tsx (1)
23-37: Consider consistent quote-stripping logic for all fields.The
profileIconfield uses conditional logic before stripping quotes (line 34), whilecolorandnamealways strip quotes (lines 31-32). This creates inconsistent handling: an empty stringprofileIconwould becomenull, but empty stringcolorornamewould remain as""after quote stripping.Apply this diff for consistency:
return { color: data.color.replace(/^"|"$/g, ""), name: data.name.replace(/^"|"$/g, ""), cursor: data.cursor, - profileIcon: data.profileIcon ? data.profileIcon.replace(/^"|"$/g, "") : null, + profileIcon: data.profileIcon?.replace(/^"|"$/g, "") ?? null, selection: data.selection, };This ensures that:
- Undefined/null → null
- Empty string → empty string (consistent with color/name)
- Quoted strings → unquoted strings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/prisma/schema.prisma(1 hunks)backend/src/auth/auth.service.ts(1 hunks)backend/src/auth/github.strategy.ts(1 hunks)backend/src/auth/types/login-request.type.ts(1 hunks)backend/src/users/types/user-domain.type.ts(1 hunks)backend/src/users/users.service.ts(3 hunks)backend/src/workspace-users/types/workspace-user.domain.ts(1 hunks)backend/src/workspace-users/workspace-users.service.ts(1 hunks)frontend/src/components/cards/DocumentCard.tsx(2 hunks)frontend/src/components/headers/SettingHeader.tsx(1 hunks)frontend/src/components/headers/UserPresenceList.tsx(2 hunks)frontend/src/components/headers/WorkspaceHeader.tsx(1 hunks)frontend/src/components/workspace/TableTab.tsx(2 hunks)frontend/src/hooks/api/types/document.d.ts(1 hunks)frontend/src/hooks/api/types/user.d.ts(1 hunks)frontend/src/hooks/useYorkieDocument.ts(1 hunks)frontend/src/pages/settings/profile/Index.tsx(1 hunks)frontend/src/store/userSlice.ts(2 hunks)frontend/src/utils/yorkie/yorkieSync.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/src/users/users.service.ts (2)
frontend/src/hooks/api/types/user.d.ts (1)
User(1-8)frontend/src/store/userSlice.ts (1)
User(5-12)
🔇 Additional comments (21)
backend/prisma/schema.prisma (1)
18-18: NewprofileIconfield is consistent and backward‑compatibleOptional
Stringmapped toprofile_iconfollows the existing schema conventions and cleanly supports nullable profile image URLs.backend/src/workspace-users/workspace-users.service.ts (1)
35-43: SelectingprofileIconin workspace user query is appropriateIncluding
profileIconin theuser.findManyselect cleanly surfaces profile images to callers and stays consistent with the new workspace‑user domain shape.backend/src/auth/auth.service.ts (1)
18-22: PassingprofileIconintofindOrCreatekeeps user data in syncForwarding
req.user.profileIcontousersService.findOrCreateis a straightforward way to persist social‑provider avatars without impacting the existing token structure.frontend/src/store/userSlice.ts (1)
5-12: ExtendingUserwithprofileIconis consistent with store designAdding
profileIcon: string | nulland documenting it in the slice comment matches how other optional user attributes are modeled and gives components a single source of truth for avatar URLs.Also applies to: 36-46
frontend/src/hooks/api/types/document.d.ts (1)
11-21: Presence type now correctly carries optionalprofileIconExtending
Document.presences[].datawithprofileIcon?: string | nullmatches the new presence shape and gives callers a typed way to render avatars with images when available.frontend/src/hooks/api/types/user.d.ts (1)
4-4: LGTM!The
profileIconfield is correctly typed as optional with a null union, consistent with the backend type definitions and other optional user fields.frontend/src/components/headers/SettingHeader.tsx (1)
36-38: LGTM!The Avatar correctly uses the profile icon when available and falls back to the user's initial. MUI's Avatar component handles image loading failures gracefully by falling back to the children content.
backend/src/auth/types/login-request.type.ts (1)
7-7: LGTM!The
profileIconfield is correctly added as a required field with null as a valid value, matching the return type from the GitHub strategy.frontend/src/components/headers/WorkspaceHeader.tsx (1)
74-76: LGTM!Consistent implementation with SettingHeader - the Avatar correctly displays the profile icon when available and falls back to initials.
frontend/src/utils/yorkie/yorkieSync.ts (1)
13-13: LGTM!The
profileIconfield correctly extends the Yorkie presence type to support profile images in collaborative editing sessions.frontend/src/components/headers/UserPresenceList.tsx (2)
55-55: LGTM!Profile icons are correctly displayed for users in the presence list, with appropriate fallback to initials.
94-94: LGTM!Consistent implementation - the popover list also displays profile icons when available.
frontend/src/components/workspace/TableTab.tsx (1)
91-91: LGTM!Avatar correctly uses the profile icon with appropriate fallback.
backend/src/auth/github.strategy.ts (1)
30-30: Verification confirmed: Implementation is correct.The
profile.photos?.[0]?.valuecorrectly accesses the avatar URL from passport-github's normalized Profile type. The photos property is indeed an array of objects with avalueproperty containing the URL string, and the optional chaining syntax appropriately handles cases where photos may be undefined.profileIcon: profile.photos?.[0]?.value || null,No changes needed.
frontend/src/components/cards/DocumentCard.tsx (2)
29-29: LGTM! Consistent type and parsing logic.The profileIcon field is properly typed as optional and the quote-stripping logic mirrors the pattern used for other presence fields.
Also applies to: 36-36
111-118: LGTM! Proper Avatar image handling with fallback.The Avatar correctly uses
profileIconas the image source with an appropriate empty string fallback. When the image is unavailable, MUI Avatar will display the initials as children, providing a good user experience.backend/src/workspace-users/types/workspace-user.domain.ts (1)
8-9: LGTM! Well-documented optional field.The profileIcon field is properly defined with clear API documentation and appropriate typing.
backend/src/users/users.service.ts (4)
36-36: LGTM! Proper field selection.The profileIcon field is correctly added to the select clause, ensuring it's included in the query results.
64-69: Verify the intended behavior when profileIcon is null.The current logic only updates the profile icon when a non-null value is provided that differs from the stored value. If
profileIconisnull(e.g., a user removes their GitHub profile picture), the existing stored icon will persist unchanged.Is this the intended behavior? Consider whether removing a profile picture from the OAuth provider should clear it in your application or preserve the last known value.
If clearing the icon is desired, consider this alternative:
if (foundUser) { - if (profileIcon && foundUser.profileIcon !== profileIcon) { + if (foundUser.profileIcon !== profileIcon) { return await this.prismaService.user.update({ where: { id: foundUser.id }, data: { profileIcon }, }); } return foundUser; }This would update the database whenever the profileIcon value differs, including when it becomes null.
77-77: LGTM! Proper handling of optional field during user creation.The profileIcon is correctly set to the provided value or null, which aligns with the nullable database schema.
51-55: Verify all callers have been updated for the newprofileIconparameter.The method signature now requires a third parameter (
profileIcon: string | null). Ensure all call sites in the codebase have been updated to pass this new parameter. A search of the codebase was unable to complete due to access constraints; manual verification is recommended to confirm all invocations offindOrCreateinclude the new parameter.
| if (profileIcon && foundUser.profileIcon !== profileIcon) { | ||
| return await this.prismaService.user.update({ | ||
| where: { id: foundUser.id }, | ||
| data: { profileIcon }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This part could potentially cause issues later when the user profile image change feature is added.
Let's assume the following scenario:
- User creates an account or login via social login.
- User changes profile image on CodePair.
- The user logs in again.
Even if a user wants to change to a different image, it will keep reverting to the image from social login info.
Maybe we can ignore this for now and just add a comment, or it would be good to discuss when the user image should be set.
There was a problem hiding this comment.
Sorry for the late response 😓. I added a TODO comment for now since there's no clear policy on profile image updates yet. If you'd prefer I can also make it so the profile icon is only set on the first login. Let me know!
- Add the profileIcon field to the User model in the Prisma schema - Prepare for storing and retrieving users' profile icons across the application
- Modified github strategy to request and retrieve the user's profile image - Updated the LoginUserInfo type to include the avatar URL field
- Added logic to save the user's profile image URL to the database during the updated login flow - Added profile icon support to workspace user information - Ensures user and workspace contexts have consistent access to profile images
- Added user profile icon to DocumentCard, headers, and TableTab - Updated type definitions for the profileIcon field
nasagong
force-pushed
the
feat/show-profile-image
branch
from
c846cd8 to
025ae2e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/frontend/src/features/editor/hooks/useYorkieDocument.ts (1)
79-86: Consider: Profile icon changes won't update presence on already-attached documents.The dependency on
userStore.data?.profileIconrecreates the callback, but theuseEffectat line 100 guards against re-initialization whendocorclientalready exist. If the user's profile icon changes mid-session, the presence won't update until they re-open the document.This is likely acceptable for the initial implementation since profile icons rarely change during a session. If real-time presence icon updates become a requirement, you'd need to call
doc.update()on presence changes separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/features/editor/hooks/useYorkieDocument.ts` around lines 79 - 86, The current callback recreation on userStore.data?.profileIcon doesn't update presence for already-attached documents because the useEffect that creates the doc/client guards against re-initialization; add a separate effect that watches userStore.data?.profileIcon and, when it changes and doc is attached, call the Yorkie document presence update (e.g., invoke doc.update(...) or the appropriate doc presence update API) to set presence.profileIcon so existing sessions reflect the new icon; reference the existing symbols useYorkieDocument state variables doc and client and the userStore.data?.profileIcon dependency to locate where to add this effect.packages/backend/src/users/users.service.ts (1)
32-36: Return type could bePromise<User>instead ofPromise<User | null>.The function always returns a
Userobject—either from finding an existing user, updating one, or creating a new one. The| nullin the return type appears unnecessary and could mislead callers into adding unnecessary null checks.♻️ Suggested fix
async findOrCreate( socialProvider: string, socialUid: string, profileIcon: string | null -): Promise<User | null> { +): Promise<User> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/users/users.service.ts` around lines 32 - 36, The findOrCreate function currently types its return as Promise<User | null> though all code paths return a User; update the signature of findOrCreate to return Promise<User> (replace Promise<User | null> with Promise<User>), ensure any internal returns still return a User instance (no nulls introduced), and update any callers or related type annotations that depended on the nullable return to expect a User instead of null.packages/frontend/src/components/workspace/TableTab.tsx (1)
23-37: Consider extractingparsePresenceDatato a shared utility.The
parsePresenceDatafunction is duplicated betweenTableTab.tsxandDocumentCard.tsxwith identical logic. Both handle the same presence data structure fromdocument.presences.♻️ Suggested approach
Create a shared utility in a common location (e.g.,
utils/presence.ts):export interface RawPresenceData { color: string; name: string; cursor: string | null; profileIcon?: string | null; selection: string | null; } export interface ParsedPresenceData { color: string; name: string; cursor: string | null; profileIcon: string | null; selection: string | null; } export const parsePresenceData = (data: RawPresenceData): ParsedPresenceData => ({ color: data.color.replace(/^"|"$/g, ""), name: data.name.replace(/^"|"$/g, ""), cursor: data.cursor, profileIcon: data.profileIcon ? data.profileIcon.replace(/^"|"$/g, "") : null, selection: data.selection, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/components/workspace/TableTab.tsx` around lines 23 - 37, Extract the duplicated parsePresenceData logic into a shared utility (e.g., utils/presence.ts) and replace the local implementations in TableTab.tsx and DocumentCard.tsx with a single exported parsePresenceData function; ensure the exported function accepts the RawPresenceData shape and returns the normalized ParsedPresenceData (stripping surrounding quotes from color, name, and profileIcon and preserving cursor/selection), then update both TableTab.tsx and DocumentCard.tsx to import parsePresenceData from the new module and remove the local copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/backend/src/users/users.service.ts`:
- Around line 32-36: The findOrCreate function currently types its return as
Promise<User | null> though all code paths return a User; update the signature
of findOrCreate to return Promise<User> (replace Promise<User | null> with
Promise<User>), ensure any internal returns still return a User instance (no
nulls introduced), and update any callers or related type annotations that
depended on the nullable return to expect a User instead of null.
In `@packages/frontend/src/components/workspace/TableTab.tsx`:
- Around line 23-37: Extract the duplicated parsePresenceData logic into a
shared utility (e.g., utils/presence.ts) and replace the local implementations
in TableTab.tsx and DocumentCard.tsx with a single exported parsePresenceData
function; ensure the exported function accepts the RawPresenceData shape and
returns the normalized ParsedPresenceData (stripping surrounding quotes from
color, name, and profileIcon and preserving cursor/selection), then update both
TableTab.tsx and DocumentCard.tsx to import parsePresenceData from the new
module and remove the local copies.
In `@packages/frontend/src/features/editor/hooks/useYorkieDocument.ts`:
- Around line 79-86: The current callback recreation on
userStore.data?.profileIcon doesn't update presence for already-attached
documents because the useEffect that creates the doc/client guards against
re-initialization; add a separate effect that watches
userStore.data?.profileIcon and, when it changes and doc is attached, call the
Yorkie document presence update (e.g., invoke doc.update(...) or the appropriate
doc presence update API) to set presence.profileIcon so existing sessions
reflect the new icon; reference the existing symbols useYorkieDocument state
variables doc and client and the userStore.data?.profileIcon dependency to
locate where to add this effect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c4e6044-5f61-4cc9-af71-51a1576a5769
📒 Files selected for processing (15)
packages/backend/prisma/schema.prismapackages/backend/src/auth/auth.service.tspackages/backend/src/auth/github.strategy.tspackages/backend/src/auth/types/login-request.type.tspackages/backend/src/users/types/user-domain.type.tspackages/backend/src/users/users.service.tspackages/backend/src/workspace-users/types/workspace-user.domain.tspackages/backend/src/workspace-users/workspace-users.service.tspackages/codemirror/src/plugins/yorkie/yorkieSync.tspackages/frontend/src/components/cards/DocumentCard.tsxpackages/frontend/src/components/headers/SettingHeader.tsxpackages/frontend/src/components/headers/UserPresenceList.tsxpackages/frontend/src/components/headers/WorkspaceHeader.tsxpackages/frontend/src/components/workspace/TableTab.tsxpackages/frontend/src/features/editor/hooks/useYorkieDocument.ts
✅ Files skipped from review due to trivial changes (4)
- packages/backend/src/workspace-users/types/workspace-user.domain.ts
- packages/backend/prisma/schema.prisma
- packages/codemirror/src/plugins/yorkie/yorkieSync.ts
- packages/backend/src/users/types/user-domain.type.ts
4 participants
What this PR does / why we need it:
Implemented user profile image support across both the backend and frontend.
Which issue(s) this PR fixes:
Fixes #348
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Release Notes