[Feat]: Implement memories features end to end backend and frontend both. #781
Conversation
github-actions
bot
added
backend
enhancement
WalkthroughAdds a new Memories feature: backend FastAPI router that groups images into time/location-based memories and exposes GET /memories/ and GET /memories/{memory_id}/images; frontend clients, types, pages, routes, components, and utilities to fetch and display memories; and OpenAPI updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Browser / UI
participant Backend as Backend API
participant DB as Database
Note over User,Browser: User opens Memories page
User->>Browser: Navigate to /memories
Browser->>Backend: GET /memories/
Backend->>DB: db_get_all_images()
DB-->>Backend: images list (id, date, lat, lon, metadata)
Backend->>Backend: group_images() — cluster by date & proximity
Backend->>Backend: decide_memory_type(), generate_title()
Backend-->>Browser: MemoriesApiResponse (memories list)
Browser->>Browser: Render memory cards
Note over User,Browser: User opens memory details
User->>Browser: Click memory card
Browser->>Backend: GET /memories/{memory_id}/images
Backend->>Backend: locate memory group, return images
Backend-->>Browser: MemoryImagesApiResponse (image list)
Browser->>Browser: Open Gallery with images
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Points to focus on:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
backend/app/routes/memories.py (1)
340-355: Performance: Re-computing all memories for single memory lookup.
get_memory_imagescallsdb_get_all_images()andgroup_images()to recompute all memories just to find one by ID. This is inefficient, especially as the image count grows.Consider caching the grouped memories or restructuring the endpoint to avoid redundant computation. For now, this works but may become a bottleneck.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/app/routes/memories.py(1 hunks)backend/main.py(2 hunks)frontend/src/api/api-functions/index.ts(1 hunks)frontend/src/api/api-functions/memories.ts(1 hunks)frontend/src/api/apiEndpoints.ts(1 hunks)frontend/src/components/Media/ChronologicalGallery.tsx(1 hunks)frontend/src/components/Media/MediaThumbnails.tsx(1 hunks)frontend/src/pages/Memories/Memories.tsx(1 hunks)frontend/src/pages/Memories/MemoryDetail.tsx(1 hunks)frontend/src/routes/AppRoutes.tsx(2 hunks)frontend/src/types/Media.ts(2 hunks)frontend/src/types/memories.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/api/api-functions/memories.ts (2)
frontend/src/types/memories.ts (2)
MemoriesApiResponse(3-6)MemoryImagesApiResponse(8-11)frontend/src/api/axiosConfig.ts (1)
apiClient(5-12)
frontend/src/types/memories.ts (1)
frontend/src/types/Media.ts (2)
Memory(58-67)Image(13-23)
frontend/src/routes/AppRoutes.tsx (1)
frontend/src/constants/routes.ts (1)
ROUTES(1-12)
backend/app/routes/memories.py (1)
backend/app/database/images.py (1)
db_get_all_images(123-214)
🔇 Additional comments (10)
frontend/src/types/Media.ts (1)
51-67: LGTM! Type definitions align well with backend response.The
MemoryImageandMemoryinterfaces correctly model the backend response structure. Therepresentative_imagebeing optional properly handles edge cases.backend/main.py (2)
29-29: LGTM!The import follows the established pattern used by other route modules.
136-136: LGTM!Router registration is consistent with other routers in the application.
frontend/src/api/apiEndpoints.ts (1)
34-37: LGTM!The endpoint definitions follow the established patterns and correctly match the backend route paths.
frontend/src/api/api-functions/index.ts (1)
7-7: LGTM!The export statement follows the established pattern and correctly exposes the new memories API module.
frontend/src/types/memories.ts (1)
1-11: LGTM!The API response types follow a consistent pattern with clear structure. The interfaces correctly wrap the domain types (Memory and Image) in success/data envelopes.
frontend/src/components/Media/ChronologicalGallery.tsx (1)
61-67: LGTM!The type change from
Map<string, number>toMap<number, number>correctly aligns with the updatedImage.idtype. The map key now properly reflects that image IDs are numeric.frontend/src/components/Media/MediaThumbnails.tsx (1)
4-14: LGTM!The
idtype change fromstringtonumbercorrectly aligns with the updatedImageinterface, maintaining type consistency across the codebase.frontend/src/api/api-functions/memories.ts (1)
7-17: LGTM!Both API functions follow the established pattern for API calls in this codebase. The functions are simple, well-typed, and correctly use the shared
apiClientinstance.frontend/src/pages/Memories/Memories.tsx (1)
15-167: LGTM with one refactoring note!The Memories page implementation is well-structured with proper loading states, error handling via
useMutationFeedback, and clean UI rendering. The helper functions (getMemoryIcon,getMemoryBadge) are well-organized and the empty state handling is appropriate.Note: The
formatDateRangefunction (lines 43-63) is duplicated inMemoryDetail.tsx. See the refactoring suggestion in the MemoryDetail.tsx review to extract this to a shared utility.
| const formatDateRange = (start: string, end: string) => { | ||
| const s = new Date(start); | ||
| const e = new Date(end); | ||
|
|
||
| if (s.toDateString() === e.toDateString()) { | ||
| return s.toLocaleDateString('en-US', { | ||
| month: 'long', | ||
| day: 'numeric', | ||
| year: 'numeric', | ||
| }); | ||
| } | ||
|
|
||
| return `${s.toLocaleDateString('en-US', { | ||
| month: 'short', | ||
| day: 'numeric', | ||
| })} – ${e.toLocaleDateString('en-US', { | ||
| month: 'short', | ||
| day: 'numeric', | ||
| year: 'numeric', | ||
| })}`; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor: Extract duplicated date formatting logic.
The formatDateRange function is duplicated between MemoryDetail.tsx and Memories.tsx (lines 43-63). Extract this to a shared utility module to follow DRY principles.
Create a new utility file (e.g., frontend/src/utils/memoryUtils.ts):
export const formatDateRange = (start: string, end: string): string => {
const s = new Date(start);
const e = new Date(end);
if (s.toDateString() === e.toDateString()) {
return s.toLocaleDateString('en-US', {
month: 'long',
day: 'numeric',
year: 'numeric',
});
}
return `${s.toLocaleDateString('en-US', {
month: 'short',
day: 'numeric',
})} – ${e.toLocaleDateString('en-US', {
month: 'short',
day: 'numeric',
year: 'numeric',
})}`;
};Then import and use it in both components:
+import { formatDateRange } from '@/utils/memoryUtils';
-
-const formatDateRange = (start: string, end: string) => {
- // ... function body
-};🤖 Prompt for AI Agents
In frontend/src/pages/Memories/MemoryDetail.tsx around lines 30 to 50, the
formatDateRange function is duplicated across MemoryDetail.tsx and Memories.tsx;
extract it into a shared utility (e.g., frontend/src/utils/memoryUtils.ts) that
exports formatDateRange(start: string, end: string): string, move the existing
logic there, then import and use that exported function in both MemoryDetail.tsx
and Memories.tsx to remove duplication and keep behavior identical.
| {memory.location_name && ( | ||
| <div className="flex items-center gap-1"> | ||
| <MapPin className="h-4 w-4" /> | ||
| {memory.location_name} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Critical: Accessing undefined Memory property.
The code references memory.location_name (line 77), but this field is not defined in the Memory interface. This will cause a runtime error.
Update the Memory interface to include the optional location_name field (see previous comment for the interface update), or remove this conditional rendering block if location names are not supported.
🤖 Prompt for AI Agents
In frontend/src/pages/Memories/MemoryDetail.tsx around lines 74–79, the
component reads memory.location_name which is not declared on the Memory
interface and can cause a runtime/type error; either add an optional
location_name?: string to the Memory interface where it is defined (e.g.,
frontend/src/types or the file noted in the prior comment) and run typecheck, or
if location names are not supported remove this conditional block from
MemoryDetail.tsx; if you add the field, ensure it is optional (string |
undefined) so the existing conditional rendering remains correct.
| <Route path={ROUTES.MEMORIES} element={<Memories />} /> | ||
| <Route path={`${ROUTES.MEMORIES}/:memoryId`} element={<MemoryDetail />} /> |
There was a problem hiding this comment.
Minor: Inconsistent indentation on Line 27.
Line 27 is missing leading spaces compared to sibling routes. This is a minor formatting issue.
- <Route path={ROUTES.MEMORIES} element={<Memories />} />
+ <Route path={ROUTES.MEMORIES} element={<Memories />} />📝 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.
| <Route path={ROUTES.MEMORIES} element={<Memories />} /> | |
| <Route path={`${ROUTES.MEMORIES}/:memoryId`} element={<MemoryDetail />} /> | |
| <Route path={ROUTES.MEMORIES} element={<Memories />} /> | |
| <Route path={`${ROUTES.MEMORIES}/:memoryId`} element={<MemoryDetail />} /> |
🤖 Prompt for AI Agents
In frontend/src/routes/AppRoutes.tsx around lines 27 to 28, the Route on line 27
is indented inconsistently compared to sibling Route entries; fix by aligning
leading spaces to match the surrounding JSX routes (apply the same indentation
level as the following Route line), then run the project's formatter or lint
autofix to ensure consistent styling across the file.
|
|
||
| export interface Image { | ||
| id: string; | ||
| id: number; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat frontend/src/types/Media.tsRepository: AOSSIE-Org/PictoPy
Length of output: 1602
🏁 Script executed:
cd frontend/src && git log -n 5 --oneline -- types/Media.tsRepository: AOSSIE-Org/PictoPy
Length of output: 106
🏁 Script executed:
rg -n '\.id\s*===' frontend/src/ --type=ts --type=tsx -C2Repository: AOSSIE-Org/PictoPy
Length of output: 89
🏁 Script executed:
rg -n 'Image\.id|image\.id' frontend/src/ --type=ts --type=tsx -C2Repository: AOSSIE-Org/PictoPy
Length of output: 89
🏁 Script executed:
git diff HEAD~1 frontend/src/types/Media.tsRepository: AOSSIE-Org/PictoPy
Length of output: 250
🏁 Script executed:
rg -n 'image\.id|Image\.id' frontend/src/ --type=ts -C2Repository: AOSSIE-Org/PictoPy
Length of output: 1672
🏁 Script executed:
rg -n "localStorage" frontend/src/ --type=ts -C2Repository: AOSSIE-Org/PictoPy
Length of output: 7020
🏁 Script executed:
rg -n "url.*id|params.*id" frontend/src/ --type=ts -C2Repository: AOSSIE-Org/PictoPy
Length of output: 458
🏁 Script executed:
git show 22e1b80:frontend/src/types/Media.ts 2>/dev/null | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 464
🏁 Script executed:
git log --all --oneline -- frontend/src/types/Media.ts | head -5Repository: AOSSIE-Org/PictoPy
Length of output: 227
🏁 Script executed:
rg -n 'toggleFavourite' frontend/src/ --type=ts -B2 -A5Repository: AOSSIE-Org/PictoPy
Length of output: 3689
🏁 Script executed:
rg -n 'const toggleFavourite|function toggleFavourite' frontend/src/ --type=ts -A10Repository: AOSSIE-Org/PictoPy
Length of output: 882
Breaking change: Image.id type changed to number, but the backend API expects a string.
The togglefav mutation function (line 7 in useToggleFav.ts) expects image_id: string, but Image.id is now typed as number. While the wrapper function uses id: any to mask the type error, this will cause a type mismatch at runtime when numeric IDs are sent to the backend endpoint. Update the type to match the backend contract or add explicit type conversion when calling the API.
🤖 Prompt for AI Agents
In frontend/src/types/Media.ts around line 14, Image.id is typed as number but
the backend expects a string; change the id type to string to match the API
contract (and update any code that constructs Image objects to produce string
IDs), OR if IDs can be numeric upstream, leave the type and instead convert to
string at the call site—specifically update useToggleFav.ts to pass image_id:
String(id) (or id.toString()) when invoking the mutation so the HTTP payload
always sends a string.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
backend/app/routes/memories.py (3)
15-24: Add type hints to function parameters.The
haversine_distancefunction lacks type annotations for its parameters, which reduces code clarity and type safety.-def haversine_distance(lat1, lon1, lat2, lon2) -> float: +def haversine_distance(lat1: float, lon1: float, lat2: float, lon2: float) -> float:
78-81: Date parsing may fail for non-ISO formats.The parsing handles ISO format with
Zsuffix but will fail for other common formats like"YYYY-MM-DD HH:MM:SS"without timezone. Consider adding more robust date parsing or documenting the expected format.try: - date = datetime.fromisoformat(date_str.replace("Z", "+00:00")) + # Handle ISO format with Z suffix and basic ISO format + date_str_normalized = date_str.replace("Z", "+00:00") + date = datetime.fromisoformat(date_str_normalized) except Exception: + # Skip images with unparseable dates continue
166-175: Consider adding response model and pagination for large galleries.The endpoint returns all memories without pagination. For galleries with thousands of images, this could result in large responses. Consider adding optional pagination or at minimum a limit parameter for future scalability.
frontend/src/types/Media.ts (2)
51-56: Consider makingmetadataoptional inMemoryImage.The
MemoryImageinterface hasmetadata: ImageMetadataas required, but theImageinterface (line 19) hasmetadata?: ImageMetadataas optional. The backend may return images without metadata, which could cause type errors when accessingrepresentative_image.metadata.export interface MemoryImage { id: number; path: string; thumbnailPath: string; - metadata: ImageMetadata; + metadata?: ImageMetadata; }
67-69: Unused fields:latitude,longitude,location_nameare not returned by the backend.The backend
group_imagesfunction (lines 139-154 in memories.py) does not includelatitude,longitude, orlocation_namein the memory response. These optional fields in the interface are never populated.Either remove these fields if they're not planned, or update the backend to include them:
export interface Memory { id: string; title: string; memory_type: 'on_this_day' | 'trip' | 'date_range'; date_range_start: string; date_range_end: string; image_count: number; representative_image?: MemoryImage; images: Image[]; - latitude?: number; - longitude?: number; - location_name?: string; }frontend/src/utils/memoryUtils.ts (1)
6-20: Consider extracting locale for future i18n support.The
'en-US'locale is hardcoded. If internationalization is planned, consider extracting this to a configuration or using the user's browser locale.+const DEFAULT_LOCALE = 'en-US'; + export const formatDateRange = (start: string, end: string): string => { // ... if (s.toDateString() === e.toDateString()) { - return s.toLocaleDateString('en-US', { + return s.toLocaleDateString(DEFAULT_LOCALE, {frontend/src/pages/Memories/Memories.tsx (2)
91-136: Consider adding keyboard accessibility to memory cards.The memory cards respond to mouse clicks but lack keyboard navigation support, which could limit accessibility for keyboard-only users.
Consider enhancing keyboard accessibility:
<Card key={memory.id} className="cursor-pointer transition hover:shadow-lg" + tabIndex={0} + role="button" onClick={() => handleMemoryClick(memory)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleMemoryClick(memory); + } + }} >
82-89: Optional: Add loading skeleton for better UX.Currently, the loading state is only communicated via a toast notification from
useMutationFeedback. Consider adding a loading skeleton to provide immediate visual feedback in the main content area while memories are being fetched.Example approach:
if (isLoading) { return ( <div className="flex h-full flex-col px-8 py-6"> <div className="mb-6"> <h1 className="text-2xl font-bold">Memories</h1> <p className="text-sm text-muted-foreground"> Relive moments from your past </p> </div> <div className="grid grid-cols-1 gap-4 md:grid-cols-2 lg:grid-cols-3"> {[1, 2, 3].map((i) => ( <Card key={i} className="animate-pulse"> <div className="aspect-video bg-muted" /> <CardContent className="p-4 space-y-2"> <div className="h-4 bg-muted rounded w-3/4" /> <div className="h-3 bg-muted rounded w-1/2" /> </CardContent> </Card> ))} </div> </div> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/routes/memories.py(1 hunks)frontend/src/api/api-functions/togglefav.ts(1 hunks)frontend/src/hooks/useToggleFav.ts(1 hunks)frontend/src/pages/Memories/Memories.tsx(1 hunks)frontend/src/types/Media.ts(2 hunks)frontend/src/utils/memoryUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/api/api-functions/togglefav.ts (3)
frontend/src/types/API.ts (1)
APIResponse(1-8)frontend/src/api/axiosConfig.ts (1)
apiClient(5-12)frontend/src/api/apiEndpoints.ts (1)
imagesEndpoints(1-4)
frontend/src/hooks/useToggleFav.ts (1)
frontend/src/api/api-functions/togglefav.ts (1)
togglefav(5-11)
frontend/src/pages/Memories/Memories.tsx (7)
frontend/src/types/Media.ts (2)
Image(13-23)Memory(58-70)frontend/src/features/imageSelectors.ts (1)
selectIsImageViewOpen(13-16)frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/memories.ts (1)
fetchAllMemories(7-10)frontend/src/features/imageSlice.ts (2)
setImages(18-20)setCurrentViewIndex(22-34)frontend/src/utils/memoryUtils.ts (1)
formatDateRange(1-21)frontend/src/components/Media/MediaView.tsx (1)
MediaView(25-224)
backend/app/routes/memories.py (1)
backend/app/database/images.py (1)
db_get_all_images(123-214)
🔇 Additional comments (6)
frontend/src/api/api-functions/togglefav.ts (1)
5-11: LGTM!The type change from
stringtonumberaligns with the updatedImage.idtype, and thetoString()conversion maintains backward compatibility with the backend API contract. This properly addresses the previous review comment about type consistency.frontend/src/hooks/useToggleFav.ts (1)
5-17: LGTM!The hook's type signatures are properly aligned with the updated
togglefavAPI function. Changing fromanytonumberimproves type safety and consistency across the codebase.frontend/src/pages/Memories/Memories.tsx (4)
1-19: LGTM! Clean imports and state setup.All necessary dependencies are imported, and the component state is initialized appropriately with local state for selected images and Redux integration for the image viewer.
21-36: LGTM! Data fetching properly integrated.The query setup with
usePictoQueryand feedback handling viauseMutationFeedbackfollows the established patterns in the codebase. Safe data extraction with fallback to empty array prevents runtime errors.
44-66: LGTM! Helper functions are well-structured.Both
getMemoryIconandgetMemoryBadgecorrectly handle all three memory types (on_this_day,trip,date_range) with appropriate visual distinctions. The implementation is clean and exhaustive.
68-80: LGTM! Clean empty state UI.The empty state provides clear user guidance with appropriate iconography and messaging, enhancing the user experience when no memories are available.
| def decide_memory_type(mem: Dict[str, Any]) -> str: | ||
| now = datetime.now() | ||
| date = mem["anchor_date"] | ||
| lat = mem["lat"] | ||
| lon = mem["lon"] | ||
|
|
||
| if date.month == now.month and date.day == now.day and date.year != now.year: | ||
| return "on_this_day" | ||
|
|
||
| if lat is not None and lon is not None: | ||
| return "trip" | ||
|
|
||
| return "date_range" |
There was a problem hiding this comment.
Potential timezone mismatch between datetime.now() and parsed dates.
The anchor_date is parsed with timezone info on line 79 (replace("Z", "+00:00")), but datetime.now() returns a naive datetime. Comparing timezone-aware and naive datetimes can raise TypeError or produce incorrect results.
Consider using timezone-aware now:
+from datetime import datetime, timezone
+
def decide_memory_type(mem: Dict[str, Any]) -> str:
- now = datetime.now()
+ now = datetime.now(timezone.utc)
date = mem["anchor_date"]Or alternatively, strip timezone info from parsed dates if local time comparison is intended.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/routes/memories.py around lines 27-39, the function uses
datetime.now() (naive) while mem["anchor_date"] is parsed with timezone info,
which can raise TypeError or give incorrect comparisons; update the code to use
a timezone-aware "now" (e.g., datetime.now(timezone.utc)) and compare against
anchor_date converted to the same timezone
(anchor_date.astimezone(timezone.utc)), or alternatively strip tzinfo from
anchor_date before comparing if local naive comparison is intended; ensure you
perform the month/day/year comparison using consistently timezone-aware or
consistently naive datetimes (or compare .date() after normalizing both to the
same tz).
| def generate_title(mem: Dict[str, Any]) -> str: | ||
| mtype = decide_memory_type(mem) | ||
| date = mem["anchor_date"] | ||
| now = datetime.now() | ||
|
|
||
| if mtype == "on_this_day": | ||
| years = now.year - date.year | ||
| return ( | ||
| "On this day last year" | ||
| if years == 1 | ||
| else f"On this day · {years} years ago" | ||
| ) | ||
|
|
||
| if mtype == "trip": | ||
| return f"Trip · {date.strftime('%B %Y')}" | ||
|
|
||
| return f"Memories from {date.strftime('%B %Y')}" |
There was a problem hiding this comment.
Same timezone issue applies here.
The datetime.now() call on line 45 has the same naive vs. aware datetime mismatch issue mentioned above. Apply the same fix to use datetime.now(timezone.utc).
🤖 Prompt for AI Agents
In backend/app/routes/memories.py around lines 42 to 58, the function uses
datetime.now() which produces a naive datetime and can mismatch against
timezone-aware anchor_date; change to use an aware UTC now (e.g.,
datetime.now(timezone.utc)) and ensure you import timezone from datetime; also
if anchor_date may be naive, normalize/convert it to UTC-aware before computing
year differences or formatting so all datetime comparisons are done with the
same tz-awareness.
| @router.get("/{memory_id}/images") | ||
| def get_memory_images(memory_id: str): | ||
| try: | ||
| images = db_get_all_images() | ||
| grouped_memories = group_images(images) | ||
|
|
||
| for mem in grouped_memories: | ||
| if mem["id"] == memory_id: | ||
| return { | ||
| "success": True, | ||
| "data": mem["images"], | ||
| } | ||
|
|
||
| raise HTTPException(status_code=404, detail="Memory not found") | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail=str(e)) |
There was a problem hiding this comment.
Critical: HTTPException is caught by the broad except block, converting 404s to 500s.
The HTTPException raised on line 191 is caught by the except Exception on line 192, which re-raises it as a 500 error with str(e) as the detail. This means clients will never receive the intended 404 status.
@router.get("/{memory_id}/images")
def get_memory_images(memory_id: str):
try:
images = db_get_all_images()
grouped_memories = group_images(images)
for mem in grouped_memories:
if mem["id"] == memory_id:
return {
"success": True,
"data": mem["images"],
}
raise HTTPException(status_code=404, detail="Memory not found")
+ except HTTPException:
+ raise
except Exception as e:
raise HTTPException(status_code=500, detail=str(e))🤖 Prompt for AI Agents
In backend/app/routes/memories.py around lines 178-193 the broad except
Exception catches the HTTPException(404) and converts it into a 500; update the
error handling so HTTPException passes through untouched (e.g., in the except
block check if isinstance(e, HTTPException) and re-raise it, otherwise raise a
new HTTPException(status_code=500, detail=str(e))) or replace the broad except
with more specific exception types to avoid swallowing intentional
HTTPExceptions.
| const handleMemoryClick = (memory: Memory) => { | ||
| setSelectedMemoryImages(memory.images); | ||
| dispatch(setImages(memory.images)); | ||
| dispatch(setCurrentViewIndex(0)); | ||
| }; |
There was a problem hiding this comment.
Clear stale state when MediaView closes.
The selectedMemoryImages local state is set when a memory card is clicked but is never cleared when the MediaView modal closes. While this doesn't cause a functional bug (the modal is guarded by isImageViewOpen), it leaves stale image references in memory.
Add an onClose handler to reset the local state:
+ const handleCloseMediaView = () => {
+ setSelectedMemoryImages([]);
+ };
+
return (
<div className="flex h-full flex-col px-8 py-6">
...
{isImageViewOpen && (
<MediaView
images={selectedMemoryImages}
type="image"
+ onClose={handleCloseMediaView}
/>
)}
</div>
);Also applies to: 138-143
🤖 Prompt for AI Agents
In frontend/src/pages/Memories/Memories.tsx around lines 38-42 (and similarly at
138-143), the local state selectedMemoryImages is set when a memory is clicked
but never cleared, leaving stale image references; add an onClose handler to the
MediaView modal that resets selectedMemoryImages to an empty array (and
optionally dispatches a store action to clear images/current index if needed),
and wire that handler into the MediaView component so selectedMemoryImages is
cleared whenever the modal closes.
| export const formatDateRange = (start: string, end: string): string => { | ||
| const s = new Date(start); | ||
| const e = new Date(end); | ||
|
|
||
| if (s.toDateString() === e.toDateString()) { | ||
| return s.toLocaleDateString('en-US', { | ||
| month: 'long', | ||
| day: 'numeric', | ||
| year: 'numeric', | ||
| }); | ||
| } | ||
|
|
||
| return `${s.toLocaleDateString('en-US', { | ||
| month: 'short', | ||
| day: 'numeric', | ||
| })} – ${e.toLocaleDateString('en-US', { | ||
| month: 'short', | ||
| day: 'numeric', | ||
| year: 'numeric', | ||
| })}`; | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Add validation for invalid date inputs.
If start or end are malformed strings, new Date() returns an Invalid Date object, and subsequent operations will produce "Invalid Date" strings in the UI.
Consider adding validation:
export const formatDateRange = (start: string, end: string): string => {
const s = new Date(start);
const e = new Date(end);
+
+ if (isNaN(s.getTime()) || isNaN(e.getTime())) {
+ return 'Unknown date';
+ }
if (s.toDateString() === e.toDateString()) {📝 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.
| export const formatDateRange = (start: string, end: string): string => { | |
| const s = new Date(start); | |
| const e = new Date(end); | |
| if (s.toDateString() === e.toDateString()) { | |
| return s.toLocaleDateString('en-US', { | |
| month: 'long', | |
| day: 'numeric', | |
| year: 'numeric', | |
| }); | |
| } | |
| return `${s.toLocaleDateString('en-US', { | |
| month: 'short', | |
| day: 'numeric', | |
| })} – ${e.toLocaleDateString('en-US', { | |
| month: 'short', | |
| day: 'numeric', | |
| year: 'numeric', | |
| })}`; | |
| }; | |
| export const formatDateRange = (start: string, end: string): string => { | |
| const s = new Date(start); | |
| const e = new Date(end); | |
| if (isNaN(s.getTime()) || isNaN(e.getTime())) { | |
| return 'Unknown date'; | |
| } | |
| if (s.toDateString() === e.toDateString()) { | |
| return s.toLocaleDateString('en-US', { | |
| month: 'long', | |
| day: 'numeric', | |
| year: 'numeric', | |
| }); | |
| } | |
| return `${s.toLocaleDateString('en-US', { | |
| month: 'short', | |
| day: 'numeric', | |
| })} – ${e.toLocaleDateString('en-US', { | |
| month: 'short', | |
| day: 'numeric', | |
| year: 'numeric', | |
| })}`; | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/utils/memoryUtils.ts around lines 1-21, validate the parsed Date
objects before using them: after creating s and e, check if either is invalid
(e.g. isNaN(s.getTime()) || isNaN(e.getTime())), and if so return a safe
fallback (empty string or a localized "Invalid date" placeholder) or throw a
clear error; additionally consider handling the case where start > end by
swapping or returning a single-date string. Ensure no toLocaleDateString calls
happen on invalid Date objects.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/backend/backend_python/openapi.json(2 hunks)
🔇 Additional comments (1)
docs/backend/backend_python/openapi.json (1)
1120-1127: Approve: input_type parameter schema wrapping looks correct.The
allOfwrapper with explicit title and default metadata improves schema clarity and aligns with OpenAPI best practices.
| "/memories/": { | ||
| "get": { | ||
| "tags": [ | ||
| "Memories" | ||
| ], | ||
| "summary": "Get Memories", | ||
| "operationId": "get_memories_memories__get", | ||
| "responses": { | ||
| "200": { | ||
| "description": "Successful Response", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": {} | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Incomplete API documentation: empty response schema for GET /memories/.
The 200 response schema is empty ("schema": {}), which fails to document the structure of the Memories response. This creates ambiguity for API consumers about what fields, types, and structure to expect.
Replace the empty schema with a proper response schema that documents the memory objects. For example:
"200": {
"description": "Successful Response",
"content": {
"application/json": {
- "schema": {}
+ "schema": {
+ "type": "array",
+ "items": {
+ "$ref": "#/components/schemas/MemoryResponse"
+ }
+ }
}
}
}Also, add error response documentation (e.g., 500 for server errors) consistent with other endpoints in the spec.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/backend/backend_python/openapi.json around lines 1308 to 1326 the 200
response schema for GET /memories/ is empty; replace the empty "schema": {} with
a concrete response definition that documents an array of memory objects (e.g.,
use an array of a Memory schema or inline object describing fields such as id
(string/integer), user_id (string/integer), content (string),
created_at/updated_at (string, date-time), optional tags (array of strings) and
any other fields your app returns), and add standard error responses (e.g., a
500 response with a reference to a common ErrorResponse schema consistent with
other endpoints or an inline object containing code/message) so clients can
understand both success and failure payloads.
| "/memories/{memory_id}/images": { | ||
| "get": { | ||
| "tags": [ | ||
| "Memories" | ||
| ], | ||
| "summary": "Get Memory Images", | ||
| "operationId": "get_memory_images_memories__memory_id__images_get", | ||
| "parameters": [ | ||
| { | ||
| "name": "memory_id", | ||
| "in": "path", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string", | ||
| "title": "Memory Id" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "Successful Response", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": {} | ||
| } | ||
| } | ||
| }, | ||
| "422": { | ||
| "description": "Validation Error", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/HTTPValidationError" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete API documentation: empty response schema for GET /memories/{memory_id}/images.
The 200 response schema is empty, leaving API consumers without documentation of the image structure. Additionally, error responses beyond 422 Validation Error are not documented; comparable endpoints document 500, 400, or 404 responses.
Replace the empty schema and add comprehensive error handling:
"200": {
"description": "Successful Response",
"content": {
"application/json": {
- "schema": {}
+ "schema": {
+ "type": "array",
+ "items": {
+ "$ref": "#/components/schemas/ImageData"
+ }
+ }
}
}
},
+ "404": {
+ "description": "Memory not found",
+ "content": {
+ "application/json": {
+ "schema": {
+ "$ref": "#/components/schemas/HTTPValidationError"
+ }
+ }
+ }
+ },
+ "500": {
+ "description": "Internal Server Error",
+ "content": {
+ "application/json": {
+ "schema": {
+ "$ref": "#/components/schemas/HTTPValidationError"
+ }
+ }
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/backend/backend_python/openapi.json around lines 1327 to 1366, the 200
response for GET /memories/{memory_id}/images uses an empty schema and lacks
other common error responses; update the 200 response to reference or inline the
correct image array schema (e.g., an array of Image objects with id, url,
caption, width, height, created_at) and add standard error responses mirrored
from comparable endpoints (400 Bad Request, 404 Not Found, 500 Internal Server
Error) with appropriate $ref schemas (e.g., HTTPError or ErrorModel) or inline
error object schemas so API consumers have a complete contract.
2 participants
Memories Feature – Automatic Time & Location Based Highlights
Fixes #723
Overview
This PR implements the Memories feature for PictoPy, inspired by Google Photos–style memory resurfacing.
Memories are generated automatically from existing gallery images using time and location metadata, without manual album creation and without introducing any new database tables.
What’s Implemented
Time-Based Memory Creation
Location-Aware (Optional, Graceful Fallback)
✅ Important:
Missing location metadata does not block memory creation.
Automatic Memory Types
Each memory is automatically classified as:
Dynamic titles are generated automatically, such as:
Highlight-Focused UI
Database Changes
This keeps the system lightweight, scalable, and backward-compatible.
🔄 Auto Refresh Behavior
🧪 Edge Cases Handled
date_created→ skipped safely📸 Screenshots
✅ Checklist
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.