Implement Memories feature end-to-end (time & location based highlights) #773
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughA new Memories feature is introduced across frontend and backend. The backend provides API endpoints to retrieve auto-generated photo memories clustered by date and location. The frontend implements components for viewing memories as cards and detailed views, routes to the feature, and TypeScript types. Integration includes FastAPI router registration, memory generation utilities, and API client functions. Changes
Sequence DiagramsequenceDiagram
participant User
participant FrontendApp as Frontend App
participant API as Backend API
participant MemoryUtil as Memory Utils
participant ImageDB as Image Database
User->>FrontendApp: Navigate to Memories page
FrontendApp->>API: GET /memories?limit=X
API->>MemoryUtil: generate_memories()
MemoryUtil->>ImageDB: Retrieve all images with metadata
ImageDB-->>MemoryUtil: Image list
MemoryUtil->>MemoryUtil: Cluster by date (same day)
MemoryUtil->>MemoryUtil: Cluster by location (haversine distance)
MemoryUtil->>MemoryUtil: Generate "On this day" memories
MemoryUtil->>MemoryUtil: Deduplicate & sort by end date
MemoryUtil-->>API: List[Memory]
API->>API: Map to GetMemoriesResponse schema
API-->>FrontendApp: GetMemoriesResponse
FrontendApp->>FrontendApp: Render memory cards grid
FrontendApp-->>User: Display memories
User->>FrontendApp: Click on memory card
FrontendApp->>API: GET /memories/{memory_id}
API-->>FrontendApp: Memory object
FrontendApp->>ImageDB: Fetch all images (via fetchAllImages)
ImageDB-->>FrontendApp: Image list
FrontendApp->>FrontendApp: Filter by memory.media_ids & sort
FrontendApp-->>User: Display memory detail + image gallery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (10)
backend/app/utils/memories.py (4)
26-51: Clamp Haversine intermediate to avoid rare NaNs.
On Line 48,acan slightly exceed 1.0 due to float error; clamp beforeasin.- c = 2 * math.asin(math.sqrt(a)) + a = min(1.0, max(0.0, a)) + c = 2 * math.asin(math.sqrt(a))
53-68: _parse_date is fragile for ISO/timezones; prefer a real ISO parser.
The manual splitting on Lines 59-61 will mis-handle timezone offsets (Z,+05:30) and other valid ISO forms.-from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone ... def _parse_date(date_str: Optional[str]) -> Optional[datetime]: """Parse ISO date string to datetime object.""" if not date_str: return None try: - # Handle ISO format with or without microseconds - if "T" in date_str: - date_str = date_str.split("T")[0] + " " + date_str.split("T")[1].split(".")[0] - return datetime.strptime(date_str, "%Y-%m-%d %H:%M:%S") + # Python 3.11+: fromisoformat handles many ISO variants (but not all 'Z' forms) + s = date_str.replace("Z", "+00:00") + return datetime.fromisoformat(s) except (ValueError, AttributeError): try: # Try just date return datetime.strptime(date_str.split(" ")[0], "%Y-%m-%d") except (ValueError, AttributeError): return None
70-103: “Today/Yesterday” titles depend on wall-clock time (tests/UX).
If you’re aiming for deterministic behavior, consider passingnowinto_format_memory_title(or generating titles in the route layer where time can be controlled).
304-366: Sorting by ISO strings is brittle; sort by datetimes instead.
Line 356 sorts bym["date_range"]["end"](a string). This is usually OK for ISO, but safer to sort by parsed datetimes (and you already have datetimes in cluster creation). Consider storingend_dtinternally or parsing before sort.frontend/src/pages/Memories/Memories.tsx (1)
15-31: Avoidas Memory[]by typing the query result.
Line 30 forces a cast; it’ll hide backend/frontend contract drift. Prefer typingfetchAllMemories()(orusePictoQuery) sodatais strongly typed.backend/tests/test_memories.py (1)
13-57: Unused fixture:mock_images_with_metadatais never referenced in any test.This fixture is defined but none of the tests use it. Either remove it or add tests that exercise memory generation with this sample data.
-@pytest.fixture -def mock_images_with_metadata(): - """Sample images with metadata for testing.""" - return [ - { - "id": "img1", - "path": "/test/img1.jpg", - "folder_id": "1", - "thumbnailPath": "/thumb/img1.jpg", - "metadata": { - "date_created": "2023-06-15T10:00:00", - "latitude": 26.9124, - "longitude": 75.7873, - }, - "isTagged": True, - "isFavourite": False, - "tags": None, - }, - { - "id": "img2", - "path": "/test/img2.jpg", - "folder_id": "1", - "thumbnailPath": "/thumb/img2.jpg", - "metadata": { - "date_created": "2023-06-15T11:00:00", - "latitude": 26.9125, - "longitude": 75.7874, - }, - "isTagged": True, - "isFavourite": False, - "tags": None, - }, - { - "id": "img3", - "path": "/test/img3.jpg", - "folder_id": "1", - "thumbnailPath": "/thumb/img3.jpg", - "metadata": { - "date_created": "2022-06-15T10:00:00", - }, - "isTagged": True, - "isFavourite": False, - "tags": None, - }, - ]frontend/src/components/Memories/MemoryDetailView.tsx (2)
34-40: Fetching all images then filtering client-side may be inefficient for large libraries.Consider adding a backend endpoint that accepts memory ID or media IDs to return only the relevant images, avoiding the need to fetch and filter the entire image collection.
43-47: String comparison for date sorting is fragile.
localeCompareon ISO date strings works but is semantically unclear. Prefer parsing to timestamps for explicit chronological ordering.memoryImages.sort((a, b) => { - const dateA = a.metadata?.date_created || ''; - const dateB = b.metadata?.date_created || ''; - return dateA.localeCompare(dateB); + const dateA = new Date(a.metadata?.date_created || 0).getTime(); + const dateB = new Date(b.metadata?.date_created || 0).getTime(); + return dateA - dateB; });frontend/src/types/Memory.ts (1)
11-20: Consider using a union type for thetypefield.A union type would provide better type safety and IDE autocomplete for known memory types.
+export type MemoryType = 'on_this_day' | 'trip' | 'date_cluster'; + export interface Memory { id: string; title: string; - type: string; // "on_this_day", "trip", "date_cluster", etc. + type: MemoryType; date_range: DateRange; location: string | null; media_count: number; representative_media: RepresentativeMedia[]; media_ids: string[]; }backend/app/routes/memories.py (1)
65-100: De-duplicate dict→model mapping via Pydantic validation (and centralize KeyError/ValidationError handling).
You can replace the manual field-by-field construction withMemory.model_validate(...)(works with nesteddate_range/representative_mediadicts too), and reuse it in both endpoints.+from pydantic import BaseModel, ValidationError @@ +def _to_memory_model(mem: dict) -> Memory: + return Memory.model_validate(mem) @@ - memory_models = [ - Memory( - id=mem["id"], - title=mem["title"], - type=mem["type"], - date_range=DateRange( - start=mem["date_range"]["start"], - end=mem["date_range"]["end"], - ), - location=mem.get("location"), - media_count=mem["media_count"], - representative_media=[ - RepresentativeMedia( - id=media["id"], - thumbnailPath=media["thumbnailPath"], - ) - for media in mem["representative_media"] - ], - media_ids=mem["media_ids"], - ) - for mem in memories - ] + memory_models = [_to_memory_model(mem) for mem in memories] @@ - return Memory( - id=memory["id"], - title=memory["title"], - type=memory["type"], - date_range=DateRange( - start=memory["date_range"]["start"], - end=memory["date_range"]["end"], - ), - location=memory.get("location"), - media_count=memory["media_count"], - representative_media=[ - RepresentativeMedia( - id=media["id"], - thumbnailPath=media["thumbnailPath"], - ) - for media in memory["representative_media"] - ], - media_ids=memory["media_ids"], - ) + return _to_memory_model(memory)Also applies to: 123-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
README.md(1 hunks)backend/app/routes/memories.py(1 hunks)backend/app/utils/memories.py(1 hunks)backend/main.py(2 hunks)backend/tests/test_memories.py(1 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/EmptyStates/EmptyMemoriesState.tsx(1 hunks)frontend/src/components/Memories/MemoryCard.tsx(1 hunks)frontend/src/components/Memories/MemoryDetailView.tsx(1 hunks)frontend/src/pages/Memories/Memories.tsx(1 hunks)frontend/src/routes/AppRoutes.tsx(2 hunks)frontend/src/types/Memory.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
frontend/src/types/Memory.ts (1)
backend/app/routes/memories.py (3)
RepresentativeMedia(17-20)DateRange(23-26)Memory(29-38)
backend/tests/test_memories.py (1)
backend/tests/test_folders.py (1)
client(76-78)
frontend/src/components/Memories/MemoryDetailView.tsx (6)
frontend/src/types/Memory.ts (1)
Memory(11-20)frontend/src/types/Media.ts (1)
Image(13-23)frontend/src/features/imageSelectors.ts (2)
selectCurrentViewIndex(9-10)selectImages(5-7)frontend/src/api/api-functions/images.ts (1)
fetchAllImages(5-14)frontend/src/features/imageSlice.ts (2)
setImages(18-20)setCurrentViewIndex(22-34)frontend/src/components/Media/MediaView.tsx (1)
MediaView(25-224)
frontend/src/api/api-functions/memories.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)
memoriesEndpoints(34-37)
backend/app/utils/memories.py (2)
backend/app/database/images.py (1)
db_get_all_images(123-214)backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)
backend/app/routes/memories.py (2)
backend/app/utils/memories.py (1)
generate_memories(304-365)frontend/src/types/Memory.ts (3)
RepresentativeMedia(1-4)DateRange(6-9)Memory(11-20)
frontend/src/routes/AppRoutes.tsx (1)
frontend/src/constants/routes.ts (1)
ROUTES(1-12)
frontend/src/components/Memories/MemoryCard.tsx (3)
backend/app/routes/memories.py (1)
Memory(29-38)frontend/src/types/Memory.ts (1)
Memory(11-20)frontend/src/components/ui/card.tsx (1)
CardDescription(90-90)
🔇 Additional comments (17)
backend/main.py (1)
29-29: Router wiring looks correct (import + include_router).
Just verify the router inapp.routes.memoriesdefines paths compatible with the frontend (/memories/and/memories/{id}).Also applies to: 136-136
frontend/src/routes/AppRoutes.tsx (1)
12-26: Memories route correctly points to the real page now.
Line 25 switches ROUTES.MEMORIES to<Memories />, matching the PR objective.frontend/src/api/api-functions/index.ts (1)
1-7: Good: memories API functions are exported via the barrel.
Keeps imports consistent with the rest of the API functions.README.md (1)
33-43: Docs update is clear and appropriately scoped.
The Memories feature is described succinctly in the features list (Line 37).frontend/src/api/apiEndpoints.ts (1)
34-37: Endpoints look consistent with the backend router include prefix.
Just verify backend paths tolerate the trailing slash ongetAllMemories(Line 35).frontend/src/components/EmptyStates/EmptyMemoriesState.tsx (1)
1-32: Clean, self-contained empty state component.
Nice lightweight UX when there are no memories.backend/tests/test_memories.py (4)
60-88: Good test coverage for successful memory retrieval.The test properly mocks
generate_memories, verifies the HTTP status code, success flag, and validates key response fields.
91-106: LGTM: Limit parameter test.Correctly validates that the
limitquery parameter restricts the number of returned memories.
109-118: LGTM: Empty memories test.Validates graceful handling when no memories exist.
121-159: LGTM: Get by ID and 404 tests.Both success and not-found scenarios are properly covered for the single-memory endpoint.
frontend/src/api/api-functions/memories.ts (2)
5-14: LGTM:fetchAllMemoriesimplementation.Clean implementation with optional limit parameter handling. Follows the existing API function patterns in the codebase.
16-23: LGTM:fetchMemoryByIdimplementation.Straightforward GET request using the endpoint builder function.
frontend/src/components/Memories/MemoryCard.tsx (2)
23-41: LGTM: Date formatting logic.Handles
on_this_daytype, same-day ranges, and multi-day ranges correctly with appropriate formatting options.
46-117: Overall component structure looks good.The card properly handles empty states, uses appropriate Tailwind utilities for hover effects, and displays memory metadata clearly.
frontend/src/components/Memories/MemoryDetailView.tsx (2)
81-91: LGTM: MediaView integration.Properly conditionally renders the image viewer when an image is selected and handles close by resetting the view index.
93-158: LGTM: Detail view layout and rendering.Header with metadata, loading state, empty state, and responsive image grid are well-implemented.
frontend/src/types/Memory.ts (1)
1-9: LGTM:RepresentativeMediaandDateRangeinterfaces.Clean type definitions that align with backend Pydantic models.
Comment on lines
+48
to
+112
| @router.get( | ||
| "/", | ||
| response_model=GetMemoriesResponse, | ||
| responses={500: {"model": ErrorResponse}}, | ||
| ) | ||
| def get_memories( | ||
| limit: Optional[int] = Query(None, description="Maximum number of memories to return", ge=1, le=100) | ||
| ): | ||
| """ | ||
| Get all auto-generated memories. | ||
|
|
||
| Memories are automatically generated by clustering photos based on: | ||
| - Date similarity (same day, month, year, or "on this day" from past years) | ||
| - Geographic proximity (nearby locations) | ||
|
|
||
| Returns memories sorted by date (most recent first). | ||
| """ | ||
| try: | ||
| memories = generate_memories() | ||
|
|
||
| # Apply limit if specified | ||
| if limit is not None: | ||
| memories = memories[:limit] | ||
|
|
||
| # Convert to response models | ||
| memory_models = [ | ||
| Memory( | ||
| id=mem["id"], | ||
| title=mem["title"], | ||
| type=mem["type"], | ||
| date_range=DateRange( | ||
| start=mem["date_range"]["start"], | ||
| end=mem["date_range"]["end"], | ||
| ), | ||
| location=mem.get("location"), | ||
| media_count=mem["media_count"], | ||
| representative_media=[ | ||
| RepresentativeMedia( | ||
| id=media["id"], | ||
| thumbnailPath=media["thumbnailPath"], | ||
| ) | ||
| for media in mem["representative_media"] | ||
| ], | ||
| media_ids=mem["media_ids"], | ||
| ) | ||
| for mem in memories | ||
| ] | ||
|
|
||
| return GetMemoriesResponse( | ||
| success=True, | ||
| message=f"Successfully retrieved {len(memory_models)} memories", | ||
| data=memory_models, | ||
| ) | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Error retrieving memories: {e}", exc_info=True) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=ErrorResponse( | ||
| success=False, | ||
| error="Internal server error", | ||
| message=f"Unable to retrieve memories: {str(e)}", | ||
| ).model_dump(), | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
Error responses won’t match ErrorResponse schema (wrapped under detail) + leak internals.
HTTPException(detail=ErrorResponse(...).model_dump()) yields {"detail": {...}}, not an ErrorResponse body, and message echoes str(e) to clients.
-from fastapi import APIRouter, HTTPException, status, Query
+from fastapi import APIRouter, HTTPException, status, Query
+from fastapi.responses import JSONResponse
@@
- except Exception as e:
+ except Exception as e:
logger.error(f"Error retrieving memories: {e}", exc_info=True)
- raise HTTPException(
- status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
- detail=ErrorResponse(
- success=False,
- error="Internal server error",
- message=f"Unable to retrieve memories: {str(e)}",
- ).model_dump(),
- )
+ err = ErrorResponse(
+ success=False,
+ error="Internal server error",
+ message="Unable to retrieve memories",
+ )
+ return JSONResponse(
+ status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+ content=err.model_dump(),
+ )🤖 Prompt for AI Agents
In backend/app/routes/memories.py around lines 48 to 112, the exception handler
currently wraps an ErrorResponse into HTTPException(detail=...) which results in
a {"detail": {...}} envelope and also exposes internal error text to clients;
instead, log the full exception (keep logger.error with exc_info=True) but
return the ErrorResponse as the actual response body (not nested under "detail")
and avoid leaking internals by using a generic client-facing message; implement
this by returning a JSONResponse (status_code=500,
content=ErrorResponse(success=False, error="Internal server error",
message="Unable to retrieve memories").model_dump()) or equivalent so the
response matches ErrorResponse exactly and do not include str(e) in the response
content.
Comment on lines
+114
to
+171
| @router.get( | ||
| "/{memory_id}", | ||
| response_model=Memory, | ||
| responses={404: {"model": ErrorResponse}, 500: {"model": ErrorResponse}}, | ||
| ) | ||
| def get_memory_by_id(memory_id: str): | ||
| """ | ||
| Get a specific memory by ID. | ||
| """ | ||
| try: | ||
| memories = generate_memories() | ||
|
|
||
| # Find memory by ID | ||
| memory = next((m for m in memories if m["id"] == memory_id), None) | ||
|
|
||
| if not memory: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail=ErrorResponse( | ||
| success=False, | ||
| error="Not Found", | ||
| message=f"Memory with ID '{memory_id}' not found", | ||
| ).model_dump(), | ||
| ) | ||
|
|
||
| return Memory( | ||
| id=memory["id"], | ||
| title=memory["title"], | ||
| type=memory["type"], | ||
| date_range=DateRange( | ||
| start=memory["date_range"]["start"], | ||
| end=memory["date_range"]["end"], | ||
| ), | ||
| location=memory.get("location"), | ||
| media_count=memory["media_count"], | ||
| representative_media=[ | ||
| RepresentativeMedia( | ||
| id=media["id"], | ||
| thumbnailPath=media["thumbnailPath"], | ||
| ) | ||
| for media in memory["representative_media"] | ||
| ], | ||
| media_ids=memory["media_ids"], | ||
| ) | ||
|
|
||
| except HTTPException: | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Error retrieving memory {memory_id}: {e}", exc_info=True) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=ErrorResponse( | ||
| success=False, | ||
| error="Internal server error", | ||
| message=f"Unable to retrieve memory: {str(e)}", | ||
| ).model_dump(), | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
Same error-shape + info-leak problem on 404/500 for GET /{memory_id}.
The 404 path also returns {"detail": ErrorResponse} rather than an ErrorResponse body, and the 500 path echoes str(e).
@@
- if not memory:
- raise HTTPException(
- status_code=status.HTTP_404_NOT_FOUND,
- detail=ErrorResponse(
- success=False,
- error="Not Found",
- message=f"Memory with ID '{memory_id}' not found",
- ).model_dump(),
- )
+ if not memory:
+ err = ErrorResponse(
+ success=False,
+ error="Not Found",
+ message=f"Memory with ID '{memory_id}' not found",
+ )
+ return JSONResponse(
+ status_code=status.HTTP_404_NOT_FOUND,
+ content=err.model_dump(),
+ )
@@
- except HTTPException:
- raise
except Exception as e:
logger.error(f"Error retrieving memory {memory_id}: {e}", exc_info=True)
- raise HTTPException(
- status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
- detail=ErrorResponse(
- success=False,
- error="Internal server error",
- message=f"Unable to retrieve memory: {str(e)}",
- ).model_dump(),
- )
+ err = ErrorResponse(
+ success=False,
+ error="Internal server error",
+ message="Unable to retrieve memory",
+ )
+ return JSONResponse(
+ status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+ content=err.model_dump(),
+ )🤖 Prompt for AI Agents
In backend/app/routes/memories.py around lines 114-171, the 404 and 500 branches
currently return an HTTPException with its ErrorResponse put into the exception
"detail" (causing the API response to be {"detail": {...}}) and the 500 path
echoes str(e) (information leak); change both to return proper JSON body
ErrorResponse objects and avoid exposing internal errors: import and use
fastapi.responses.JSONResponse (or return Response with JSON) to return
JSONResponse(status_code=404, content=ErrorResponse(...).model_dump()) for the
not-found case, and for unexpected errors log the exception (keep
logger.error(..., exc_info=True)) but return JSONResponse(status_code=500,
content=ErrorResponse(success=False, error="Internal server error",
message="Unable to retrieve memory").model_dump()) so the client gets a clean
ErrorResponse body and no internal exception text is leaked.
Comment on lines
+20
to
+24
| # Constants for clustering | ||
| DATE_CLUSTER_THRESHOLD_DAYS = 7 # Photos within 7 days are considered same event | ||
| LOCATION_CLUSTER_THRESHOLD_KM = 10 # Photos within 10km are considered same location | ||
| REPRESENTATIVE_MEDIA_COUNT = 6 # Number of thumbnails to show per memory | ||
|
|
Contributor
There was a problem hiding this comment.
DATE_CLUSTER_THRESHOLD_DAYS is unused (behavior mismatch with docs).
Line 21 says “within 7 days”, but no code uses it. Either remove the constant or implement threshold-based clustering.
Comment on lines
+105
to
+113
| def _get_location_name(lat: float, lon: float) -> str: | ||
| """ | ||
| Get a human-readable location name from coordinates. | ||
| For now, returns a simple format. Can be enhanced with reverse geocoding. | ||
| """ | ||
| # Simple format: "Location (lat, lon)" | ||
| # In production, this could use a reverse geocoding service | ||
| return f"Location ({lat:.4f}, {lon:.4f})" | ||
|
|
Contributor
There was a problem hiding this comment.
Location string exposes raw coordinates (privacy + UX).
Line 112 will surface precise lat/lon to users; consider rounding more aggressively, hiding coordinates by default, or using a reverse-geocode name behind a user setting.
Comment on lines
+115
to
+137
| def _cluster_by_date(images: List[Dict[str, Any]]) -> List[List[Dict[str, Any]]]: | ||
| """ | ||
| Cluster images by date similarity. | ||
| Returns list of clusters, each containing images from similar dates. | ||
| """ | ||
| # Group by date (same day) | ||
| date_groups = defaultdict(list) | ||
| for img in images: | ||
| metadata = img.get("metadata", {}) | ||
| date_str = metadata.get("date_created") | ||
| date = _parse_date(date_str) | ||
| if date: | ||
| date_key = date.date() # Group by date only | ||
| date_groups[date_key].append(img) | ||
|
|
||
| # Convert to list of clusters | ||
| clusters = [] | ||
| for date_key, imgs in date_groups.items(): | ||
| if len(imgs) >= 2: # Only create clusters with 2+ images | ||
| clusters.append(imgs) | ||
|
|
||
| return clusters | ||
|
|
Contributor
There was a problem hiding this comment.
_cluster_by_date ignores DATE_CLUSTER_THRESHOLD_DAYS; also cluster order is unstable.
Lines 120-128 only group “same day”, not “within N days”. Also, iterating date_groups.items() means output cluster ordering depends on insertion order (which may not be “most recent first”).
def _cluster_by_date(images: List[Dict[str, Any]]) -> List[List[Dict[str, Any]]]:
@@
- # Group by date (same day)
- date_groups = defaultdict(list)
- for img in images:
+ # Collect dated images
+ dated: List[Tuple[datetime, Dict[str, Any]]] = []
+ for img in images:
metadata = img.get("metadata", {})
date_str = metadata.get("date_created")
date = _parse_date(date_str)
if date:
- date_key = date.date() # Group by date only
- date_groups[date_key].append(img)
+ dated.append((date, img))
+
+ # Deterministic order
+ dated.sort(key=lambda x: x[0])
+
+ # Cluster by threshold (simple 1D time window)
+ clusters: List[List[Dict[str, Any]]] = []
+ current: List[Dict[str, Any]] = []
+ start: Optional[datetime] = None
+ for dt, img in dated:
+ if not current:
+ current = [img]
+ start = dt
+ continue
+ assert start is not None
+ if (dt - start).days <= DATE_CLUSTER_THRESHOLD_DAYS:
+ current.append(img)
+ else:
+ if len(current) >= 2:
+ clusters.append(current)
+ current = [img]
+ start = dt
+ if len(current) >= 2:
+ clusters.append(current)
- # Convert to list of clusters
- clusters = []
- for date_key, imgs in date_groups.items():
- if len(imgs) >= 2: # Only create clusters with 2+ images
- clusters.append(imgs)
-
- return clusters
+ # Prefer most-recent first
+ return list(reversed(clusters))🤖 Prompt for AI Agents
In backend/app/utils/memories.py around lines 115 to 137, the current
implementation only groups images by exact same-day and returns clusters in
insertion order; change it to cluster images whose dates are within
DATE_CLUSTER_THRESHOLD_DAYS of each other and return clusters sorted
most-recent-first. To fix: extract and parse all image dates, drop images with
no date, sort images descending by date, then iterate the sorted list building
clusters by adding an image to the current cluster if its date is within
DATE_CLUSTER_THRESHOLD_DAYS of the cluster's latest date (otherwise start a new
cluster); only keep clusters with length >= 2 and finally return the clusters in
the already-sorted (most recent first) order. Ensure you reference the module
constant DATE_CLUSTER_THRESHOLD_DAYS and handle timezone-aware vs naive
datetimes consistently when comparing days.
Comment on lines
+182
to
+238
| def _get_on_this_day_memories(images: List[Dict[str, Any]]) -> List[Dict[str, Any]]: | ||
| """ | ||
| Generate "On this day" memories - photos from the same date in past years. | ||
| """ | ||
| today = datetime.now() | ||
| memories = [] | ||
|
|
||
| # Group images by month-day (ignoring year) | ||
| month_day_groups = defaultdict(list) | ||
| for img in images: | ||
| metadata = img.get("metadata", {}) | ||
| date_str = metadata.get("date_created") | ||
| date = _parse_date(date_str) | ||
| if date and date.year < today.year: # Only past years | ||
| month_day_key = (date.month, date.day) | ||
| month_day_groups[month_day_key].append((img, date)) | ||
|
|
||
| # Check if today's month-day matches any past year | ||
| today_key = (today.month, today.day) | ||
| if today_key in month_day_groups: | ||
| past_images = month_day_groups[today_key] | ||
| if len(past_images) >= 2: | ||
| # Get the most recent year's images | ||
| past_images.sort(key=lambda x: x[1], reverse=True) | ||
| images_for_memory = [img for img, _ in past_images[:20]] # Limit to 20 | ||
|
|
||
| # Get representative subset | ||
| representative = images_for_memory[:REPRESENTATIVE_MEDIA_COUNT] | ||
|
|
||
| # Get date range | ||
| dates = [date for _, date in past_images] | ||
| min_date = min(dates) | ||
| max_date = max(dates) | ||
|
|
||
| memory = { | ||
| "id": f"on_this_day_{min_date.year}", | ||
| "title": _format_memory_title("on_this_day", min_date, None), | ||
| "type": "on_this_day", | ||
| "date_range": { | ||
| "start": min_date.isoformat(), | ||
| "end": max_date.isoformat(), | ||
| }, | ||
| "location": None, | ||
| "media_count": len(images_for_memory), | ||
| "representative_media": [ | ||
| { | ||
| "id": img["id"], | ||
| "thumbnailPath": img["thumbnailPath"], | ||
| } | ||
| for img in representative | ||
| ], | ||
| "media_ids": [img["id"] for img in images_for_memory], | ||
| } | ||
| memories.append(memory) | ||
|
|
||
| return memories | ||
|
|
Contributor
There was a problem hiding this comment.
“On this day” memory mixes years in date_range/id/title but selects only some images.
- Line 205 sorts by date (good), but Line 206 takes up to 20 images across all years for that month/day.
- Lines 212-215 compute min/max from all past years, so
date_rangespans multiple years (andidon Line 217 usesmin_date.year, likely the oldest year).
This makes the memory metadata internally inconsistent. Prefer either:
- create one memory per year, or
- pick a single target year (e.g., most recent year present) and compute title/id/date_range from that year’s images only.
🤖 Prompt for AI Agents
In backend/app/utils/memories.py around lines 182 to 238, the "on this day"
memory mixes images from multiple years but builds id/title/date_range from
min/max across all years, creating inconsistent metadata; instead pick a single
target year (the most recent year present) and build the memory only from images
in that year: after sorting past_images by date descending, determine
target_year = past_images[0][1].year, filter past_images to only those with that
year, then set images_for_memory to the first 20 of that filtered list, compute
min_date/max_date from those filtered dates, set id/title to include
target_year, compute media_count and media_ids from the filtered list, and build
representative_media from that filtered subset.
Comment on lines
+240
to
+302
| def _create_memory_from_cluster( | ||
| cluster: List[Dict[str, Any]], | ||
| memory_type: str, | ||
| cluster_id: str, | ||
| ) -> Dict[str, Any]: | ||
| """ | ||
| Create a memory object from a cluster of images. | ||
| """ | ||
| if not cluster: | ||
| return None | ||
|
|
||
| # Get dates from cluster | ||
| dates = [] | ||
| locations = [] | ||
| for img in cluster: | ||
| metadata = img.get("metadata", {}) | ||
| date_str = metadata.get("date_created") | ||
| date = _parse_date(date_str) | ||
| if date: | ||
| dates.append(date) | ||
|
|
||
| lat = metadata.get("latitude") | ||
| lon = metadata.get("longitude") | ||
| if lat is not None and lon is not None: | ||
| locations.append((lat, lon)) | ||
|
|
||
| # Determine date range | ||
| if dates: | ||
| min_date = min(dates) | ||
| max_date = max(dates) | ||
| else: | ||
| min_date = max_date = datetime.now() | ||
|
|
||
| # Determine location (average if multiple) | ||
| location = None | ||
| if locations: | ||
| avg_lat = sum(lat for lat, _ in locations) / len(locations) | ||
| avg_lon = sum(lon for _, lon in locations) / len(locations) | ||
| location = _get_location_name(avg_lat, avg_lon) | ||
|
|
||
| # Get representative subset | ||
| representative = cluster[:REPRESENTATIVE_MEDIA_COUNT] | ||
|
|
||
| return { | ||
| "id": cluster_id, | ||
| "title": _format_memory_title(memory_type, min_date, location), | ||
| "type": memory_type, | ||
| "date_range": { | ||
| "start": min_date.isoformat(), | ||
| "end": max_date.isoformat(), | ||
| }, | ||
| "location": location, | ||
| "media_count": len(cluster), | ||
| "representative_media": [ | ||
| { | ||
| "id": img["id"], | ||
| "thumbnailPath": img["thumbnailPath"], | ||
| } | ||
| for img in representative | ||
| ], | ||
| "media_ids": [img["id"] for img in cluster], | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Type contract bug: function claims Dict but returns None; also nondeterministic fallback date.
Line 249 returns None, but the signature returns Dict[str, Any]. Also Lines 270-272 use datetime.now() when dates are missing, which makes output time-dependent.
-def _create_memory_from_cluster(
+def _create_memory_from_cluster(
cluster: List[Dict[str, Any]],
memory_type: str,
cluster_id: str,
-) -> Dict[str, Any]:
+) -> Optional[Dict[str, Any]]:
@@
- if not cluster:
- return None
+ if not cluster:
+ return None
@@
- else:
- min_date = max_date = datetime.now()
+ else:
+ # If we can't date the cluster, don't create a memory (keeps output deterministic)
+ return None🤖 Prompt for AI Agents
In backend/app/utils/memories.py around lines 240 to 302, the function currently
declares a return type of Dict[str, Any] but returns None when cluster is empty
and uses datetime.now() as a nondeterministic fallback; change the signature to
return Optional[Dict[str, Any]] so returning None is type-correct, and replace
the datetime.now() fallback with a deterministic sentinel (e.g., epoch:
datetime(1970,1,1,tzinfo=timezone.utc)) so date_range is stable across runs;
ensure the module imports timezone if needed and that downstream callers handle
an Optional return.
Comment on lines
+73
to
+83
| {memory.representative_media.length < 6 && ( | ||
| <div className="flex items-center justify-center bg-black/20 text-white"> | ||
| <span className="text-xs font-medium"> | ||
| + | ||
| {Math.max( | ||
| 0, | ||
| memory.media_count - memory.representative_media.length, | ||
| )} | ||
| </span> | ||
| </div> | ||
| )} |
Contributor
There was a problem hiding this comment.
The "+N" indicator condition may be incorrect.
The indicator displays when representative_media.length < 6, but should likely show when there are more media items than displayed thumbnails. Current logic will show "+0" when there are exactly as many items in representative_media as in the memory.
- {memory.representative_media.length < 6 && (
+ {memory.media_count > memory.representative_media.slice(0, 6).length && (
<div className="flex items-center justify-center bg-black/20 text-white">
<span className="text-xs font-medium">
+
{Math.max(
0,
memory.media_count - memory.representative_media.length,
)}
</span>
</div>
)}🤖 Prompt for AI Agents
In frontend/src/components/Memories/MemoryCard.tsx around lines 73 to 83, the
"+N" indicator currently renders whenever representative_media.length < 6 which
can produce a "+0"; change the condition to check whether there are more total
media items than are being displayed (e.g., memory.media_count >
memory.representative_media.length) and compute remaining = memory.media_count -
memory.representative_media.length, then only render the "+{remaining}" badge
when remaining > 0 so "+0" is never shown.
Comment on lines
+30
to
+60
| useEffect(() => { | ||
| const loadImages = async () => { | ||
| try { | ||
| setLoading(true); | ||
| const response = await fetchAllImages(); | ||
| const allImages = (response.data as Image[]) || []; | ||
|
|
||
| // Filter to only images in this memory | ||
| const memoryImages = allImages.filter((img) => | ||
| memory.media_ids.includes(img.id), | ||
| ); | ||
|
|
||
| // Sort by date to maintain chronological order | ||
| memoryImages.sort((a, b) => { | ||
| const dateA = a.metadata?.date_created || ''; | ||
| const dateB = b.metadata?.date_created || ''; | ||
| return dateA.localeCompare(dateB); | ||
| }); | ||
|
|
||
| setLocalImages(memoryImages); | ||
| // Set images in Redux for MediaView | ||
| dispatch(setImages(memoryImages)); | ||
| } catch (error) { | ||
| console.error('Error loading memory images:', error); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| loadImages(); | ||
| }, [memory.media_ids]); |
Contributor
There was a problem hiding this comment.
Missing dispatch in useEffect dependency array.
The dispatch function is used inside the effect but not listed in the dependency array. While dispatch is stable in Redux, ESLint's exhaustive-deps rule will flag this.
loadImages();
- }, [memory.media_ids]);
+ }, [memory.media_ids, dispatch]);📝 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.
Suggested change
| useEffect(() => { | |
| const loadImages = async () => { | |
| try { | |
| setLoading(true); | |
| const response = await fetchAllImages(); | |
| const allImages = (response.data as Image[]) || []; | |
| // Filter to only images in this memory | |
| const memoryImages = allImages.filter((img) => | |
| memory.media_ids.includes(img.id), | |
| ); | |
| // Sort by date to maintain chronological order | |
| memoryImages.sort((a, b) => { | |
| const dateA = a.metadata?.date_created || ''; | |
| const dateB = b.metadata?.date_created || ''; | |
| return dateA.localeCompare(dateB); | |
| }); | |
| setLocalImages(memoryImages); | |
| // Set images in Redux for MediaView | |
| dispatch(setImages(memoryImages)); | |
| } catch (error) { | |
| console.error('Error loading memory images:', error); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| loadImages(); | |
| }, [memory.media_ids]); | |
| useEffect(() => { | |
| const loadImages = async () => { | |
| try { | |
| setLoading(true); | |
| const response = await fetchAllImages(); | |
| const allImages = (response.data as Image[]) || []; | |
| // Filter to only images in this memory | |
| const memoryImages = allImages.filter((img) => | |
| memory.media_ids.includes(img.id), | |
| ); | |
| // Sort by date to maintain chronological order | |
| memoryImages.sort((a, b) => { | |
| const dateA = a.metadata?.date_created || ''; | |
| const dateB = b.metadata?.date_created || ''; | |
| return dateA.localeCompare(dateB); | |
| }); | |
| setLocalImages(memoryImages); | |
| // Set images in Redux for MediaView | |
| dispatch(setImages(memoryImages)); | |
| } catch (error) { | |
| console.error('Error loading memory images:', error); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| loadImages(); | |
| }, [memory.media_ids, dispatch]); |
🤖 Prompt for AI Agents
In frontend/src/components/Memories/MemoryDetailView.tsx around lines 30 to 60,
the useEffect uses dispatch but does not include it in the dependency array
which will trigger ESLint exhaustive-deps; update the effect to include dispatch
in the dependency array (e.g., [..., dispatch]) so ESLint is satisfied and the
effect's dependencies are explicit; if you prefer to avoid changing deps
semantics, memoize any callbacks or wrap dispatch usage in a stable ref, but the
simplest fix is to add dispatch to the dependency array.
Comment on lines
+15
to
+67
| const { data, isLoading, isSuccess, isError, error } = usePictoQuery({ | ||
| queryKey: ['memories'], | ||
| queryFn: () => fetchAllMemories(), | ||
| }); | ||
|
|
||
| useMutationFeedback( | ||
| { isPending: isLoading, isSuccess, isError, error }, | ||
| { | ||
| loadingMessage: 'Loading memories', | ||
| showSuccess: false, | ||
| errorTitle: 'Error', | ||
| errorMessage: 'Failed to load memories. Please try again later.', | ||
| }, | ||
| ); | ||
|
|
||
| const memories = (data?.data as Memory[]) || []; | ||
|
|
||
| // If memory detail view is open, show that | ||
| if (selectedMemory) { | ||
| return ( | ||
| <MemoryDetailView | ||
| memory={selectedMemory} | ||
| onClose={() => setSelectedMemory(null)} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| // Loading state | ||
| if (isLoading) { | ||
| return ( | ||
| <div className="flex h-full flex-col overflow-y-auto p-6"> | ||
| <h1 className="mb-6 text-2xl font-bold">Memories</h1> | ||
| <div className="grid grid-cols-1 gap-6 md:grid-cols-2 lg:grid-cols-3"> | ||
| {[1, 2, 3, 4, 5, 6].map((i) => ( | ||
| <Card key={i} className="overflow-hidden"> | ||
| <CardContent className="p-0"> | ||
| <Skeleton className="h-48 w-full" /> | ||
| <div className="p-4"> | ||
| <Skeleton className="mb-2 h-6 w-3/4" /> | ||
| <Skeleton className="h-4 w-1/2" /> | ||
| </div> | ||
| </CardContent> | ||
| </Card> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| // Empty state | ||
| if (memories.length === 0) { | ||
| return <EmptyMemoriesState />; | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t show “empty” UI on network/server error.
Right now, when isError is true, memories becomes [] (Line 30) and the component returns <EmptyMemoriesState /> (Lines 65-67), which reads like “no memories exist” rather than “failed to load”. Handle isError before the empty-state branch.
- // Empty state
- if (memories.length === 0) {
- return <EmptyMemoriesState />;
- }
+ // Error state (avoid mislabeling errors as "empty")
+ if (isError) {
+ return (
+ <div className="flex h-full flex-col overflow-y-auto p-6">
+ <h1 className="mb-2 text-2xl font-bold">Memories</h1>
+ <p className="text-sm text-muted-foreground">
+ Failed to load memories. Please try again later.
+ </p>
+ </div>
+ );
+ }
+
+ // Empty state
+ if (memories.length === 0) return <EmptyMemoriesState />;📝 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.
Suggested change
| const { data, isLoading, isSuccess, isError, error } = usePictoQuery({ | |
| queryKey: ['memories'], | |
| queryFn: () => fetchAllMemories(), | |
| }); | |
| useMutationFeedback( | |
| { isPending: isLoading, isSuccess, isError, error }, | |
| { | |
| loadingMessage: 'Loading memories', | |
| showSuccess: false, | |
| errorTitle: 'Error', | |
| errorMessage: 'Failed to load memories. Please try again later.', | |
| }, | |
| ); | |
| const memories = (data?.data as Memory[]) || []; | |
| // If memory detail view is open, show that | |
| if (selectedMemory) { | |
| return ( | |
| <MemoryDetailView | |
| memory={selectedMemory} | |
| onClose={() => setSelectedMemory(null)} | |
| /> | |
| ); | |
| } | |
| // Loading state | |
| if (isLoading) { | |
| return ( | |
| <div className="flex h-full flex-col overflow-y-auto p-6"> | |
| <h1 className="mb-6 text-2xl font-bold">Memories</h1> | |
| <div className="grid grid-cols-1 gap-6 md:grid-cols-2 lg:grid-cols-3"> | |
| {[1, 2, 3, 4, 5, 6].map((i) => ( | |
| <Card key={i} className="overflow-hidden"> | |
| <CardContent className="p-0"> | |
| <Skeleton className="h-48 w-full" /> | |
| <div className="p-4"> | |
| <Skeleton className="mb-2 h-6 w-3/4" /> | |
| <Skeleton className="h-4 w-1/2" /> | |
| </div> | |
| </CardContent> | |
| </Card> | |
| ))} | |
| </div> | |
| </div> | |
| ); | |
| } | |
| // Empty state | |
| if (memories.length === 0) { | |
| return <EmptyMemoriesState />; | |
| } | |
| const { data, isLoading, isSuccess, isError, error } = usePictoQuery({ | |
| queryKey: ['memories'], | |
| queryFn: () => fetchAllMemories(), | |
| }); | |
| useMutationFeedback( | |
| { isPending: isLoading, isSuccess, isError, error }, | |
| { | |
| loadingMessage: 'Loading memories', | |
| showSuccess: false, | |
| errorTitle: 'Error', | |
| errorMessage: 'Failed to load memories. Please try again later.', | |
| }, | |
| ); | |
| const memories = (data?.data as Memory[]) || []; | |
| // If memory detail view is open, show that | |
| if (selectedMemory) { | |
| return ( | |
| <MemoryDetailView | |
| memory={selectedMemory} | |
| onClose={() => setSelectedMemory(null)} | |
| /> | |
| ); | |
| } | |
| // Loading state | |
| if (isLoading) { | |
| return ( | |
| <div className="flex h-full flex-col overflow-y-auto p-6"> | |
| <h1 className="mb-6 text-2xl font-bold">Memories</h1> | |
| <div className="grid grid-cols-1 gap-6 md:grid-cols-2 lg:grid-cols-3"> | |
| {[1, 2, 3, 4, 5, 6].map((i) => ( | |
| <Card key={i} className="overflow-hidden"> | |
| <CardContent className="p-0"> | |
| <Skeleton className="h-48 w-full" /> | |
| <div className="p-4"> | |
| <Skeleton className="mb-2 h-6 w-3/4" /> | |
| <Skeleton className="h-4 w-1/2" /> | |
| </div> | |
| </CardContent> | |
| </Card> | |
| ))} | |
| </div> | |
| </div> | |
| ); | |
| } | |
| // Error state (avoid mislabeling errors as "empty") | |
| if (isError) { | |
| return ( | |
| <div className="flex h-full flex-col overflow-y-auto p-6"> | |
| <h1 className="mb-2 text-2xl font-bold">Memories</h1> | |
| <p className="text-sm text-muted-foreground"> | |
| Failed to load memories. Please try again later. | |
| </p> | |
| </div> | |
| ); | |
| } | |
| // Empty state | |
| if (memories.length === 0) return <EmptyMemoriesState />; |
Contributor
|
Great effort, guys. Your PR was the 2nd-best for this issue. I will be merging #777, but your team will receive 25 points for the effort put into this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🚀 Overview (Issue #602 )
This PR completes the Memories feature end-to-end, transitioning it from a “Coming Soon” placeholder to a fully functional, production-ready experience.
The implementation automatically generates meaningful memory highlights by clustering media using time and location, enabling users to effortlessly revisit important moments from their photo library.
This contribution was developed as part of an open-source + hackathon contribution, focusing on feature completeness, usability, and clean architecture.
THE UNSTOPPABLE HACKATHON (https://docs.stability.nexus/about-us/unstoppable-hackathon)
TEAM - NoLOGIC
Contributors -
🧩 Problem Statement
Problem / Issue:
Implements the missing Memories feature described in the project roadmap
Reference:
Closes #<ISSUE_NUMBER>(replace with the actual issue number if available)The application UI already exposed a Memories section, but:
✨ What’s Included
🧠 Backend (Python)
🎨 Frontend (React / TypeScript)
🧪 Testing
📚 Documentation
🧠 Why This Matters
This feature significantly improves user experience by enabling:
It transforms PictoPy from a gallery tool into a memory-driven experience, aligning with the project’s long-term vision.
🏗️ Implementation Notes
pre-commitCLI, but CI should validate formatting and tests🤝 Contribution Context
✅ Checklist
Looking forward to feedback and happy to iterate if needed 🚀
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.