feat: Add edit profile photo and name functionality#1895
feat: Add edit profile photo and name functionality#1895
Conversation
- Add avatarUrl column to users table with migration - Update API UserSchema and handlers to support avatarUrl - Add useUpdateProfile hook for saving profile changes - Fix name.tsx to use real user data and save via API - Make profile avatar tappable with image picker and upload - Add navigation from name row to name edit screen Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
@coderabbitai Please review this PR for code quality, best practices, and potential issues. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather the relevant code to perform a thorough review. 🏁 Script executed: Length of output: 418 🏁 Script executed: Length of output: 390 🏁 Script executed: Length of output: 157 🏁 Script executed: Length of output: 14882 🏁 Script executed: Length of output: 6695 🏁 Script executed: Length of output: 509 🏁 Script executed: Length of output: 594 🏁 Script executed: Length of output: 15013 🏁 Script executed: Length of output: 1481 🏁 Script executed: Length of output: 3367
Code Review: Edit Profile Photo & NameOverall the implementation is well-structured, but there are a few issues ranging from a functional bug to UX gaps. 🔴 High — Middle name is collected but silently discardedFile: The form has a // form.middle is never sent
async function handleSave() {
const success = await updateProfile({
firstName: form.first,
lastName: form.last, // ← form.middle dropped here
});Additionally, 🔴 High — Silent failure after successful R2 uploadFile: const remoteFileName = await uploadImage(image.fileName, image.uri);
if (remoteFileName) {
await updateProfile({ avatarUrl: remoteFileName }); // return value ignored
}
if (remoteFileName) {
const success = await updateProfile({ avatarUrl: remoteFileName });
if (!success) {
Alert.alert(t('errors.somethingWentWrong'), t('errors.tryAgain'));
}
}🟡 Medium — Upload spinner is invisible when user already has an avatarFile: <AvatarFallback>
{isUploading ? <ActivityIndicator /> : <Text>{initials}</Text>}
</AvatarFallback>
<TouchableOpacity onPress={handleAvatarPress} disabled={isUploading}>
<Avatar alt={...} className="h-24 w-24">
{avatarUri ? <AvatarImage source={{ uri: avatarUri }} /> : null}
<AvatarFallback><Text>{initials}</Text></AvatarFallback>
</Avatar>
{isUploading && (
<View className="absolute inset-0 items-center justify-center rounded-full bg-black/40">
<ActivityIndicator color="white" />
</View>
)}
</TouchableOpacity>🟡 Medium —
|
|
@copilot CodeRabbit has provided review feedback on this PR. Please review and address the comments. |
- Remove misleading middle name field (no DB column for it) - Fix silent failure: check updateProfile return value after R2 upload - Fix upload spinner: overlay on TouchableOpacity so it shows over avatar images - Fix canSave baseline: use useRef to capture initial name values at mount - Add 5 MB file size validation before avatar upload - Add profile.imageTooLarge translation key Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Addressed all CodeRabbit feedback in d5d8af0:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
⌛ 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. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request implements user profile customization functionality by enabling avatar uploads and name editing. Changes include a new profile update hook, database schema extension for avatar storage, API route updates to handle avatar persistence, and UI modifications to support image selection, validation, and editing with error handling. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ProfileUI as Profile Screen
participant ImagePicker as Image Picker
participant FileSystem as File System
participant APIClient as API Client
participant Server as Backend API
participant Database as Database
participant UserStore as User Store
User->>ProfileUI: Tap avatar
ProfileUI->>ImagePicker: Open image picker
ImagePicker-->>ProfileUI: Return selected image
ProfileUI->>FileSystem: Validate file size
alt File size > 5 MB
ProfileUI-->>User: Show "Image too large" error
else File size valid
ProfileUI->>APIClient: Upload image (PUT /api/user/profile)
APIClient->>Server: Send avatarUrl payload
Server->>Database: Update user avatarUrl
Database-->>Server: Confirm update
Server-->>APIClient: Return updated user profile
APIClient->>UserStore: Update user data
UserStore-->>ProfileUI: Refresh avatar display
ProfileUI-->>User: Show updated avatar
end
sequenceDiagram
actor User
participant NameScreen as Name Screen
participant UserStore as User Store
participant APIClient as API Client
participant Server as Backend API
participant Database as Database
NameScreen->>UserStore: Fetch user data (useUser)
UserStore-->>NameScreen: Return firstName, lastName
NameScreen->>NameScreen: Initialize form with current values
User->>NameScreen: Edit name fields
User->>NameScreen: Tap Save
NameScreen->>APIClient: Call updateProfile(firstName, lastName)
APIClient->>Server: PUT /api/user/profile
Server->>Database: Persist firstName, lastName
Database-->>Server: Confirm update
Server-->>APIClient: Return updated user profile
APIClient->>UserStore: Update user store
alt Update successful
NameScreen->>NameScreen: Navigate back
else Update failed
NameScreen-->>User: Show error alert
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/api/src/schemas/users.ts (1)
73-76: Consider adding URL format validation for avatarUrl.The schema accepts any string for
avatarUrl. If the avatar URLs are expected to be valid URLs (e.g., from R2 storage), consider adding.url()validation to catch malformed inputs early:- avatarUrl: z.string().nullable().optional().openapi({ + avatarUrl: z.string().url().nullable().optional().openapi({This would provide better input validation at the API boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/schemas/users.ts` around lines 73 - 76, The avatarUrl Zod schema currently allows any string; update the avatarUrl field in the users schema to validate that the value is a proper URL by chaining the Zod url() validator (e.g., avatarUrl: z.string().url().nullable().optional()) so malformed URLs are rejected at the API boundary while preserving nullable/optional behavior; ensure you apply the .url() call on the same schema symbol used in this file so OpenAPI example/description remain unchanged.apps/expo/features/profile/hooks/useUpdateProfile.ts (1)
12-35: Refactor to use React Query'suseMutationhook.As per coding guidelines, Expo feature hooks should use React Query hooks (
useQuery,useMutation,useInfiniteQuery) for data fetching. The current manual state management works but misses React Query benefits like retry logic, cache management, and consistent error handling patterns.♻️ Proposed refactor using useMutation
-import { useState } from 'react'; +import { useMutation } from '@tanstack/react-query'; export function useUpdateProfile() { - const [isLoading, setIsLoading] = useState(false); - const [error, setError] = useState<string | null>(null); - - const updateProfile = async (payload: UpdateProfilePayload): Promise<boolean> => { - setIsLoading(true); - setError(null); - try { - const response = await axiosInstance.put('/api/user/profile', payload); - if (response.data?.user) { - userStore.set(response.data.user); - } - return true; - } catch (err) { - const { message } = handleApiError(err); - setError(message); - return false; - } finally { - setIsLoading(false); - } - }; - - return { updateProfile, isLoading, error }; + const mutation = useMutation({ + mutationFn: async (payload: UpdateProfilePayload) => { + const response = await axiosInstance.put('/api/user/profile', payload); + return response.data; + }, + onSuccess: (data) => { + if (data?.user) { + userStore.set(data.user); + } + }, + }); + + const updateProfile = async (payload: UpdateProfilePayload): Promise<boolean> => { + try { + await mutation.mutateAsync(payload); + return true; + } catch (err) { + return false; + } + }; + + const error = mutation.error ? handleApiError(mutation.error).message : null; + + return { updateProfile, isLoading: mutation.isPending, error }; }As per coding guidelines: "Use React Query hooks (
useQuery,useMutation,useInfiniteQuery) for data fetching in Expo feature hooks".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/profile/hooks/useUpdateProfile.ts` around lines 12 - 35, Replace the manual stateful hook useUpdateProfile with a React Query mutation: convert updateProfile into a useMutation that calls axiosInstance.put('/api/user/profile', payload), uses onSuccess to update userStore.set(response.data.user) when present, and uses onError to extract the message via handleApiError(err) and set it into the mutation's error state (or into local state if you keep error/isLoading wrappers). Ensure you export the mutation's mutate/mutateAsync, isLoading, error (or map error.message) instead of the current updateProfile boolean pattern so callers use the React Query API and benefit from retries/cache.packages/api/drizzle/meta/_journal.json (1)
243-249: Migration timestamp is out of chronological order.The
whentimestamp for migration 0033 (1741516482000→ ~March 2025) is earlier than the preceding migration 0032 (1760175950793→ ~September 2025). While Drizzle typically orders migrations by theidxand filename rather than thewhenfield, this inconsistency could be confusing for auditing purposes. Consider regenerating the migration with a current timestamp if this causes any tooling issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/drizzle/meta/_journal.json` around lines 243 - 249, The migration entry with idx 33 (tag "0033_add_avatar_url_to_users") has a "when" timestamp (1741516482000) that is older than the previous migration; update the "when" value on that JSON object to a current/monotonic timestamp greater than the previous migration's timestamp (e.g., >1760175950793) or regenerate the migration so the "when" field reflects a chronological ordering to avoid confusion in auditing/tooling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/app/`(app)/(tabs)/profile/index.tsx:
- Around line 157-160: The catch block in
apps/expo/app/(app)/(tabs)/profile/index.tsx currently swallows the specific
permission-denied error thrown by useImagePicker.ts; update the catch so when
err instanceof Error && err.message === 'Permission to access media library was
denied' you show a permission-specific Alert (with text from i18n keys if
available) offering a button to open app settings (Linking.openSettings()) and a
cancel action, otherwise fall back to the existing generic Alert for other
errors; keep the existing error-check branch for non-permission errors intact.
In `@apps/expo/app/`(app)/(tabs)/profile/name.tsx:
- Around line 32-40: The save logic treats whitespace-only names as valid;
update canSave and handleSave to use trimmed values: compute trimmedFirst =
form.first.trim() and trimmedLast = form.last.trim(), then change the canSave
check to compare trimmedFirst/trimmedLast against
initialFirst.current/initialLast.current (also trim those or normalize when
stored) and require !!trimmedFirst && !!trimmedLast; when calling updateProfile
in handleSave, pass trimmedFirst and trimmedLast so the API receives trimmed
values. Ensure consistency between the comparison and submission (use the same
trimmed variables).
- Around line 37-47: Guard handleSave to prevent submitting when save is not
allowed or already in progress: at the start of the async function handleSave,
check the same conditions used by the button disabled state (e.g., if (!canSave
|| isLoading) return;), so it returns early when inputs are unchanged/invalid or
while a submission is underway; ensure you reference the existing symbols
handleSave, canSave, isLoading, updateProfile, form and preserve the existing
success/failure flow (router.back and Alert) only when the guard passes.
---
Nitpick comments:
In `@apps/expo/features/profile/hooks/useUpdateProfile.ts`:
- Around line 12-35: Replace the manual stateful hook useUpdateProfile with a
React Query mutation: convert updateProfile into a useMutation that calls
axiosInstance.put('/api/user/profile', payload), uses onSuccess to update
userStore.set(response.data.user) when present, and uses onError to extract the
message via handleApiError(err) and set it into the mutation's error state (or
into local state if you keep error/isLoading wrappers). Ensure you export the
mutation's mutate/mutateAsync, isLoading, error (or map error.message) instead
of the current updateProfile boolean pattern so callers use the React Query API
and benefit from retries/cache.
In `@packages/api/drizzle/meta/_journal.json`:
- Around line 243-249: The migration entry with idx 33 (tag
"0033_add_avatar_url_to_users") has a "when" timestamp (1741516482000) that is
older than the previous migration; update the "when" value on that JSON object
to a current/monotonic timestamp greater than the previous migration's timestamp
(e.g., >1760175950793) or regenerate the migration so the "when" field reflects
a chronological ordering to avoid confusion in auditing/tooling.
In `@packages/api/src/schemas/users.ts`:
- Around line 73-76: The avatarUrl Zod schema currently allows any string;
update the avatarUrl field in the users schema to validate that the value is a
proper URL by chaining the Zod url() validator (e.g., avatarUrl:
z.string().url().nullable().optional()) so malformed URLs are rejected at the
API boundary while preserving nullable/optional behavior; ensure you apply the
.url() call on the same schema symbol used in this file so OpenAPI
example/description remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96d558dc-7834-431e-b1e6-dd4a8c23295b
📒 Files selected for processing (10)
apps/expo/app/(app)/(tabs)/profile/index.tsxapps/expo/app/(app)/(tabs)/profile/name.tsxapps/expo/features/profile/hooks/useUpdateProfile.tsapps/expo/features/profile/types.tsapps/expo/lib/i18n/locales/en.jsonpackages/api/drizzle/0033_add_avatar_url_to_users.sqlpackages/api/drizzle/meta/_journal.jsonpackages/api/src/db/schema.tspackages/api/src/routes/user/index.tspackages/api/src/schemas/users.ts
|
@copilot Please address the CodeRabbit review feedback on this PR (3 inline comments). |
|
@copilot Reminder: please address the CodeRabbit review feedback on this PR. |
- Show permission-specific alert with Open Settings button when photo library access is denied (instead of silently swallowing the error) - Trim whitespace in canSave and handleSave so whitespace-only names are rejected; use useMemo to avoid duplication and redundant trims - Guard handleSave with !canSave || isLoading to prevent double-submit via keyboard Return key bypassing the disabled button state - Add permissions and common.cancel i18n keys Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
avatarUrlcolumn to DB schema + migrationUserSchemaand handlers to supportavatarUrlfeatures/profile/types.tswithavatarUrluseUpdateProfilehook for saving profile changesname.tsxto load real user data and save via APIprofile/index.tsx: make avatar tappable (image picker) and name row navigableupdateProfilereturn after R2 uploadcanSavebaseline withuseRefto avoid store sync driftprofile.imageTooLargetranslation keycanSave/handleSave; useuseMemoto avoid duplicationhandleSaveagainst double-submit via keyboard Return keypermissions.*andcommon.canceli18n keysOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by CodeRabbit