feat(#723): Implement Memories Page #826
feat(#723): Implement Memories Page #826Prashik-Sasane wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
feat(profile): add profile page, edit profile UI, and logout flow
|
|
WalkthroughIntroduces a new user profile feature including ProfilePage and EditProfilePage components for viewing and editing user profiles, a new LogoutDialog component, AlertDialog UI component library for consistent dialogs, and routes to access these pages. Enhances the onboarding state management with localStorage persistence and a reset function for logout workflow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
|
|
Prashik-Sasane
changed the title
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
frontend/src/components/Navigation/Navbar/Navbar.tsx (1)
89-89: Use React Router Link instead of anchor tag for client-side navigation.Using
<a href="/profile">causes a full page reload. In a React Router application, prefer<Link>oruseNavigate()for client-side navigation to preserve application state and improve performance.🔎 Proposed refactor
Import Link at the top:
+import { Link } from 'react-router';Then replace the anchor:
- <a href="/profile" className="p-2"> + <Link to="/profile" className="p-2"> <img src={userAvatar || '/photo1.png'} alt="User avatar" className="hover:ring-primary/50 h-8 w-8 cursor-pointer rounded-full transition hover:ring-2" /> - </a> + </Link>frontend/src/pages/ProfilePage/ProfilePage.tsx (2)
19-22: Use React Router navigation instead ofwindow.location.href.The logout flow uses
window.location.href = '/'which causes a full page reload. In a React Router application, preferuseNavigate()for client-side navigation to maintain application state and performance.🔎 Proposed refactor
Import useNavigate at the top:
+import { useNavigate } from 'react-router';Then update the component:
const ProfilePage = () => { const dispatch = useDispatch(); + const navigate = useNavigate(); const { theme } = useTheme(); // ... const handleLogout = () => { dispatch(resetOnboarding()); - window.location.href = '/'; + navigate('/'); };
82-88: Replace anchor tags with React Router Link components.The Edit Profile and Account Settings links use
<a href>which causes full page reloads. Use React Router's<Link>component for client-side navigation.🔎 Proposed refactor
Import Link:
+import { Link } from 'react-router';Then replace the anchors:
- <a - href="/profile/edit" + <Link + to="/profile/edit" className="border-border bg-muted hover:bg-accent flex items-center justify-between rounded-xl border px-4 py-3 text-sm font-medium" > <span>Edit Profile</span> <span className="text-muted-foreground text-xs">⌘E</span> - </a> + </Link> - <a - href="/settings" + <Link + to="/settings" className="border-border bg-muted hover:bg-accent flex items-center justify-between rounded-xl border px-4 py-3 text-sm font-medium" > <span>Account Settings</span> <span className="text-muted-foreground text-xs">⚙</span> - </a> + </Link>Also applies to: 90-96
frontend/src/features/onboardingSlice.ts (1)
28-28: Consider error handling for localStorage operations.The
setAvatarandsetNamereducers now write to localStorage without error handling. localStorage operations can throw exceptions (e.g., QuotaExceededError, SecurityError in private browsing).🔎 Proposed refactor
Wrap localStorage calls in try-catch:
setAvatar(state, action: PayloadAction<string>) { state.avatar = action.payload; - localStorage.setItem('avatar', action.payload); + try { + localStorage.setItem('avatar', action.payload); + } catch (e) { + console.warn('Failed to persist avatar to localStorage:', e); + } }, setName(state, action: PayloadAction<string>) { state.name = action.payload; - localStorage.setItem('name', action.payload); + try { + localStorage.setItem('name', action.payload); + } catch (e) { + console.warn('Failed to persist name to localStorage:', e); + } },Also applies to: 33-33
frontend/src/pages/ProfilePage/EditProfilePage.tsx (3)
16-20: Use React Router navigation instead ofwindow.location.href.Similar to ProfilePage, the save action uses
window.location.href = '/profile'which causes a full page reload. UseuseNavigate()from React Router for client-side navigation.🔎 Proposed refactor
+import { useNavigate } from 'react-router'; const EditProfilePage = () => { const dispatch = useDispatch(); + const navigate = useNavigate(); // ... const handleSave = () => { dispatch(setName(name)); dispatch(setAvatar(avatar)); - window.location.href = '/profile'; + navigate('/profile'); };
27-32: Replace anchor tags with React Router Link components.The Back and Cancel links use
<a href>which causes full page reloads. Use<Link>for client-side navigation.🔎 Proposed refactor
Import Link:
+import { Link } from 'react-router';Replace the anchors:
- <a - href="/profile" + <Link + to="/profile" className="text-muted-foreground hover:text-foreground text-sm" > ← Back - </a> + </Link> {/* ... */} - <a href="/profile"> - <button className="rounded-md border px-4 py-2">Cancel</button> - </a> + <Link to="/profile"> + <button className="rounded-md border px-4 py-2">Cancel</button> + </Link>Also applies to: 77-79
69-72: Add form validation for display name.The display name input has no validation. Consider adding checks for empty strings, length limits, or special characters to ensure data quality.
🔎 Suggested validation
const handleSave = () => { + if (!name.trim()) { + // Show error: Name cannot be empty + return; + } + if (name.length > 50) { + // Show error: Name too long + return; + } dispatch(setName(name)); dispatch(setAvatar(avatar)); navigate('/profile'); };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/components/Dialog/LogoutDialog.tsxfrontend/src/components/Navigation/Navbar/Navbar.tsxfrontend/src/components/ui/alert-dialog.tsxfrontend/src/constants/routes.tsfrontend/src/features/onboardingSlice.tsfrontend/src/pages/ProfilePage/EditProfilePage.tsxfrontend/src/pages/ProfilePage/ProfilePage.tsxfrontend/src/routes/AppRoutes.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/src/components/Dialog/LogoutDialog.tsx (1)
frontend/src/components/ui/alert-dialog.tsx (8)
AlertDialog(146-146)AlertDialogContent(148-148)AlertDialogHeader(149-149)AlertDialogTitle(150-150)AlertDialogDescription(151-151)AlertDialogFooter(152-152)AlertDialogCancel(154-154)AlertDialogAction(153-153)
frontend/src/pages/ProfilePage/ProfilePage.tsx (4)
frontend/src/contexts/ThemeContext.tsx (1)
useTheme(66-73)frontend/src/features/onboardingSelectors.ts (2)
selectName(15-15)selectAvatar(13-13)frontend/src/features/onboardingSlice.ts (1)
resetOnboarding(37-41)frontend/src/components/Dialog/LogoutDialog.tsx (1)
LogoutDialog(18-42)
frontend/src/pages/ProfilePage/EditProfilePage.tsx (2)
frontend/src/features/onboardingSelectors.ts (2)
selectName(15-15)selectAvatar(13-13)frontend/src/features/onboardingSlice.ts (2)
setName(31-34)setAvatar(26-29)
frontend/src/routes/AppRoutes.tsx (1)
frontend/src/constants/routes.ts (1)
ROUTES(1-14)
frontend/src/components/Navigation/Navbar/Navbar.tsx (2)
frontend/src/features/onboardingSelectors.ts (2)
selectName(15-15)selectAvatar(13-13)frontend/src/components/Dialog/FaceSearchDialog.tsx (1)
FaceSearchDialog(25-153)
🔇 Additional comments (4)
frontend/src/constants/routes.ts (1)
12-13: LGTM!The new route constants for profile and edit-profile are clearly defined and follow the existing pattern.
frontend/src/routes/AppRoutes.tsx (1)
12-13: LGTM!The new profile routes are properly registered and follow the existing routing structure. Integration with the Layout component ensures consistent navigation behavior.
Also applies to: 28-29
frontend/src/components/Dialog/LogoutDialog.tsx (1)
1-42: LGTM!The LogoutDialog component is well-structured with clear props, proper accessibility through AlertDialog primitives, and appropriate destructive styling for the logout action. The implementation follows best practices for confirmation dialogs.
frontend/src/components/ui/alert-dialog.tsx (1)
1-155: LGTM!This AlertDialog component library is well-implemented, following standard Radix UI wrapper patterns. The code includes:
- Proper ref forwarding with
React.forwardRef- Correct
displayNameassignments for debugging- Accessible markup (Portal, Overlay, proper ARIA semantics)
- Consistent styling with theme tokens
- Clean separation of concerns (Header, Footer, Actions)
The implementation follows React 19 best practices and integrates properly with the existing design system.
| <img | ||
| src={ | ||
| queryImage?.startsWith('data:') | ||
| queryImage.startsWith('data:') |
There was a problem hiding this comment.
Potential runtime error: removed optional chaining on non-string value.
The optional chaining was removed from queryImage.startsWith('data:'), but if queryImage is not a string (e.g., null, undefined, or another type), this will throw a runtime error. The parent conditional if (queryImage) only checks for truthiness, not type.
🔎 Proposed fix
- queryImage.startsWith('data:')
+ queryImage?.startsWith('data:')Or ensure type safety:
- {queryImage && (
+ {queryImage && typeof queryImage === 'string' && (📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| queryImage.startsWith('data:') | |
| queryImage?.startsWith('data:') |
🤖 Prompt for AI Agents
In frontend/src/components/Navigation/Navbar/Navbar.tsx around line 41, the code
calls queryImage.startsWith('data:') without ensuring queryImage is a string,
which can throw if queryImage is null/undefined or another type; update the
conditional to guard the type (e.g., check typeof queryImage === 'string' &&
queryImage.startsWith('data:') or restore optional chaining with typeof check)
so startsWith is only called on a string, or normalize/parse queryImage to a
string before calling startsWith.
| /** ✅ LOGOUT / RESET */ | ||
| resetOnboarding() { | ||
| localStorage.removeItem('avatar'); | ||
| localStorage.removeItem('name'); | ||
| return initialState; | ||
| }, |
There was a problem hiding this comment.
Critical bug: resetOnboarding returns stale initialState.
The initialState is computed once when the module loads (lines 18-19) by reading from localStorage. When resetOnboarding() removes items from localStorage and then returns initialState, it returns the stale values that were captured at module initialization, not fresh empty values.
This means logout won't properly clear the avatar/name from Redux state.
🔎 Proposed fix
Return a fresh state object instead of the captured initialState:
resetOnboarding() {
localStorage.removeItem('avatar');
localStorage.removeItem('name');
- return initialState;
+ return {
+ currentStepIndex: 0,
+ currentStepName: STEP_NAMES[0],
+ stepStatus: STEP_NAMES.map(() => false),
+ avatar: null,
+ name: '',
+ };
},📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** ✅ LOGOUT / RESET */ | |
| resetOnboarding() { | |
| localStorage.removeItem('avatar'); | |
| localStorage.removeItem('name'); | |
| return initialState; | |
| }, | |
| /** ✅ LOGOUT / RESET */ | |
| resetOnboarding() { | |
| localStorage.removeItem('avatar'); | |
| localStorage.removeItem('name'); | |
| return { | |
| currentStepIndex: 0, | |
| currentStepName: STEP_NAMES[0], | |
| stepStatus: STEP_NAMES.map(() => false), | |
| avatar: null, | |
| name: '', | |
| }; | |
| }, |
🤖 Prompt for AI Agents
In frontend/src/features/onboardingSlice.ts around lines 36–41,
resetOnboarding() currently returns the module-scoped initialState which was
computed once at module load (and thus may contain stale avatar/name values);
change it to return a fresh state object instead of the captured initialState —
either recompute the initial state after removing localStorage keys or
explicitly construct and return a new state object with avatar and name cleared
(and all other onboarding defaults preserved).
| const handleSave = () => { | ||
| dispatch(setName(name)); | ||
| dispatch(setAvatar(avatar)); | ||
| window.location.href = '/profile'; | ||
| }; |
There was a problem hiding this comment.
Avatar file is not uploaded to backend.
The avatar change only creates a local blob URL and saves it to Redux state. This blob URL won't persist across sessions or page reloads. The actual file should be uploaded to a backend service or converted to a data URL.
Would you like me to suggest an implementation that uploads the file to the backend or converts it to a base64 data URL for persistence?
🤖 Prompt for AI Agents
In frontend/src/pages/ProfilePage/EditProfilePage.tsx around lines 16-20, the
handler only stores a local blob URL in Redux which won't persist; change
handleSave to asynchronously upload the selected avatar file (or convert it to a
base64 data URL) to the backend, wait for the backend to return a persistent URL
or ID, then dispatch setAvatar with that returned persistent value, handle
upload errors and loading state, and only navigate to /profile after the upload
succeeds (or rollback on failure). Ensure the upload uses FormData or an API
endpoint that accepts base64, and update any reducers/state to store the
persistent avatar URL rather than a local blob.
| onChange={(e) => { | ||
| const file = e.target.files?.[0]; | ||
| if (!file) return; | ||
| setLocalAvatar(URL.createObjectURL(file)); | ||
| }} |
There was a problem hiding this comment.
Memory leak: URL.createObjectURL not revoked.
URL.createObjectURL creates a blob URL that persists in memory until explicitly revoked with URL.revokeObjectURL(). Without cleanup, repeated avatar changes will leak memory.
🔎 Proposed fix
Add cleanup in a useEffect:
+import { useEffect } from 'react';
const EditProfilePage = () => {
// ...
const [avatar, setLocalAvatar] = useState(currentAvatar || '/photo1.png');
+
+ // Cleanup blob URL when component unmounts or avatar changes
+ useEffect(() => {
+ return () => {
+ if (avatar && avatar.startsWith('blob:')) {
+ URL.revokeObjectURL(avatar);
+ }
+ };
+ }, [avatar]);
// ...
onChange={(e) => {
const file = e.target.files?.[0];
if (!file) return;
+ // Revoke previous blob URL if it exists
+ if (avatar && avatar.startsWith('blob:')) {
+ URL.revokeObjectURL(avatar);
+ }
setLocalAvatar(URL.createObjectURL(file));
}}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/pages/ProfilePage/EditProfilePage.tsx around lines 49–53, the
onChange handler uses URL.createObjectURL(file) but never revokes the blob URL,
causing a memory leak; modify the component to track the current object URL
(e.g., in a ref or state), revoke the previous URL with
URL.revokeObjectURL(prevUrl) before setting a new one, and add a cleanup
useEffect that revokes the current object URL on unmount so all created blob
URLs are released.
|
|
|
|
1 participant
Summary
This PR introduces a Google Photos–style Memories feature that enhances how users rediscover past moments.
It adds automatic memory generation on the backend and an Instagram-style story viewer on the frontend, allowing users to revisit photos based on time (today, past years, weekends, and yearly highlights) without manual album creation.
Changes Overview
backend/app/routes/memory.pybackend/main.pyMemories.tsxMediaView.tsxStoryProgressBar.tsxmemories.ts,apiEndpoints.tsAppRoutes.tsxMemory.ts,Media.tsDemo / Explanation Video
Video:
MemoriesPage.-.Made.with.Clipchamp.1.mp4
Demonstrates the complete Memories experience, including:
Notes
Future Improvements
Summary by CodeRabbit
New Features
Navigation & UI
✏️ Tip: You can customize this high-level summary in your review settings.