Feat/memories backend implementation#777
Conversation
✨ Features: - Display 'On this day last year' for memories from exactly 1 year ago - Format location-based memories as 'Trip to [Location], [Year]' (e.g., 'Trip to Jaipur, 2025') - Fix MediaView slideshow and info buttons not working in memory albums 🐛 Bug Fixes: - Fixed event bubbling issue where MediaView control clicks closed the entire viewer - Conditionally render MemoryViewer backdrop only when MediaView is closed - Prevent click handlers from interfering with MediaView controls 🎨 UI Improvements: - Enhanced FeaturedMemoryCard with contextual year display - Updated MemoryCard title formatting for better location context - Improved memory viewing experience with proper z-index layering 📦 Technical Changes: - Backend: Added reverse geocoding for location names in memory clustering - Backend: Fixed latitude/longitude handling for images without GPS data - Frontend: Refactored MemoryViewer JSX structure for proper conditional rendering - Frontend: Integrated MediaView component with full zoom/slideshow/info functionality This commit completes the Memories feature implementation with Google Photos-style presentation and fixes critical UX issues with the image viewer controls.
- Format Python files in backend/app/ with Black - Format TypeScript files in frontend/src/components/Memories/ with Prettier - Fix code style issues to meet project standards
✨ Added: - Updated docs/overview/features.md with Memories section - Created docs/frontend/memories.md with detailed documentation 📚 Documentation includes: - Feature overview (On This Day, Location/Date memories) - Memory types and sections (Recent, This Year, All) - Filtering options and implementation - Component architecture (MemoriesPage, MemoryCard, MemoryViewer) - State management with Redux Toolkit - API endpoints and parameters - Backend clustering algorithm (DBSCAN) - Reverse geocoding with city database - Bug fixes and improvements - Testing guidelines - Performance considerations - Future enhancement ideas The documentation provides both user-facing feature descriptions and technical implementation details for developers.
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request implements a comprehensive Memories feature enabling automatic grouping of photos into memories based on location via spatial clustering and temporal patterns. It includes backend database schema augmentation, metadata extraction, clustering algorithms, new API endpoints, and a complete React frontend with Redux state management for organizing and viewing memories. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Frontend as Frontend<br/>(MemoriesPage)
participant API as API<br/>(/api/memories)
participant DB as Database<br/>(Images Table)
participant Clustering as Memory<br/>Clustering
Client->>Frontend: View Memories
activate Frontend
Frontend->>API: generateMemories()
deactivate Frontend
activate API
API->>DB: db_get_all_images_for_memories()
activate DB
DB-->>API: Images with latitude, longitude,<br/>captured_at
deactivate DB
API->>Clustering: cluster_memories(images)
activate Clustering
Clustering->>Clustering: Filter by GPS + Date
Clustering->>Clustering: DBSCAN spatial<br/>clustering (5km radius)
Clustering->>Clustering: Reverse geocode<br/>locations
Clustering->>Clustering: Temporal grouping<br/>within clusters
Clustering->>Clustering: Generate memory IDs<br/>& titles
Clustering-->>API: List[Memory]
deactivate Clustering
API-->>Frontend: GenerateMemoriesResponse
deactivate API
activate Frontend
Frontend->>Frontend: Organize into sections:<br/>On This Day,<br/>Recent, Year, All
Frontend-->>Client: Render Memory Cards
deactivate Frontend
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
backend
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/utils/images.py (1)
155-207: Fix GPS “truthy” checks + normalizecaptured_attype consistently.
if latitude and longitude:should beis not Nonechecks (0.0 is valid and currently logs as “not extracted”).- Consider normalizing
captured_atto an ISO string (orNone) unconditionally to avoid mixed types reachingdb_bulk_insert_images.Suggested tweak:
- if latitude and longitude: + if latitude is not None and longitude is not None: logger.info(f"GPS extracted for {os.path.basename(image_path)}: ({latitude}, {longitude})") @@ - "captured_at": captured_at.isoformat() if isinstance(captured_at, datetime.datetime) and captured_at else captured_at, # Can be None + "captured_at": captured_at.isoformat() if captured_at else None,
🧹 Nitpick comments (21)
backend/app/database/images.py (4)
22-35: Consider usingtotal=Truewith explicitNotRequiredfor optional fields.Using
total=Falsemakes all fields optional, including core fields likeidandpaththat should always be present. Consider keepingtotal=True(or omitting it) and usingNotRequiredfromtypingfor truly optional fields:-class ImageRecord(TypedDict, total=False): +from typing import NotRequired + +class ImageRecord(TypedDict): """Represents the full images table structure""" id: ImageId path: ImagePath folder_id: FolderId thumbnailPath: str metadata: Union[Mapping[str, Any], str] isTagged: bool - isFavourite: bool + isFavourite: NotRequired[bool] # New fields for Memories feature - latitude: Optional[float] - longitude: Optional[float] - captured_at: Optional[datetime] + latitude: NotRequired[Optional[float]] + longitude: NotRequired[Optional[float]] + captured_at: NotRequired[Optional[datetime]]
81-93: Consider adding a composite index for spatial queries.The spatial queries in
db_get_images_near_locationfilter on bothlatitudeandlongitudesimultaneously. A composite index would be more efficient than separate single-column indexes for these range queries.cursor.execute( "CREATE INDEX IF NOT EXISTS ix_images_latitude ON images(latitude)" ) cursor.execute( "CREATE INDEX IF NOT EXISTS ix_images_longitude ON images(longitude)" ) + cursor.execute( + "CREATE INDEX IF NOT EXISTS ix_images_lat_lon ON images(latitude, longitude)" + ) cursor.execute( "CREATE INDEX IF NOT EXISTS ix_images_captured_at ON images(captured_at)" )
265-283: Move import outside the loop for better performance.The
image_util_parse_metadataimport is inside the loop, which adds overhead on each iteration. Move it to the top of the function or module level.+ from app.utils.images import image_util_parse_metadata + # Group results by image ID images_dict = {} for ( image_id, ... ) in results: if image_id not in images_dict: - # Safely parse metadata JSON -> dict - from app.utils.images import image_util_parse_metadata - metadata_dict = image_util_parse_metadata(metadata)
719-723: Consider using date range for index utilization.Using
strftime()prevents SQLite from using the index oncaptured_at. A date range query would be more efficient:- WHERE strftime('%Y', i.captured_at) = ? - AND strftime('%m', i.captured_at) = ? + WHERE i.captured_at >= ? AND i.captured_at < ?Then calculate the start and end of the month:
from calendar import monthrange start_date = f"{year:04d}-{month:02d}-01" last_day = monthrange(year, month)[1] end_date = f"{year:04d}-{month:02d}-{last_day:02d}T23:59:59"backend/app/utils/memory_clustering.py (1)
297-299: Fallback todatetime.now()may cause misleading memory IDs.When no dates are available, falling back to
datetime.now()fordate_objmeans the memory ID will be based on the generation time rather than the actual memory content. Consider using a content-based hash instead.else: date_start = date_end = None - date_obj = datetime.now() + # Use a hash of image IDs instead of current time + date_obj = None + + # Create memory ID + if date_obj: + memory_id = f"{memory_type}_{int(date_obj.timestamp())}_{len(images)}" + else: + # Content-based ID when no date available + content_hash = hash(tuple(img.get('id', '') for img in images[:5])) + memory_id = f"{memory_type}_nodate_{abs(content_hash)}_{len(images)}"backend/main.py (2)
15-15: Startup migration call is OK; consider avoiding redundant work on fresh DB.Calling
db_migrate_add_memories_columns()right afterdb_create_images_table()is safe, but it will always run PRAGMA + CREATE INDEX even on brand-new DBs. Consider guarding with “only if existing DB” (e.g., a lightweight check for missing columns/indexes) if startup time matters.Also applies to: 50-50
29-29: Router include relies on router-defined prefix—ensure it’s consistently set.
app.include_router(memories_router)assumes the router itself has the/api/memoriesprefix. If that prefix is missing or changes, routes/log line✅ Memories feature enabled at /api/memoriesbecomes misleading. Consider passingprefix="/api/memories"here for a single source of truth.Also applies to: 137-141
frontend/src/components/Memories/MemoriesPage.tsx (1)
262-320: Filter can yield a blank page (no per-filter empty state).When
hasAnyDatais true but the selected filter eliminates all items, none of the sections render and there’s no empty-state messaging. Consider showing an EmptyState like “No memories match this filter”.Also applies to: 343-425
frontend/src/store/hooks.ts (1)
8-14: Use.withTypes()for react-redux 9.x typed hooks.The project uses react-redux 9.2.0, which supports the modern
.withTypes()API (available since v9.1+). Update the typed hooks to:export const useAppDispatch = useDispatch.withTypes<AppDispatch>(); export const useAppSelector = useSelector.withTypes<RootState>();This is the recommended pattern in react-redux 9.x and provides cleaner syntax than the type annotation approach currently in use.
docs/backend/backend_python/openapi.json (1)
1460-1480: Consider adding error responses for the/api/memories/on-this-dayendpoint.This endpoint only defines a 200 response, but the description mentions it can raise
HTTPExceptionif the database query fails. Consider adding 500 error response documentation for consistency with other endpoints."responses": { "200": { "description": "Successful Response", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/OnThisDayResponse" } } } + }, + "500": { + "description": "Internal Server Error", + "content": { + "application/json": { + "schema": {} + } + } } }backend/test_memories_api.py (2)
35-56: Consider adding request timeouts for robustness.The API calls lack timeout parameters, which could cause the test script to hang indefinitely if the server becomes unresponsive. Consider adding timeouts consistent with
check_server().response = requests.post( f"{BASE_URL}/generate", params={ "location_radius_km": 5.0, "date_tolerance_days": 3, "min_images": 2 - } + }, + timeout=30 )
1-167: Consider converting to pytest for CI integration.This manual test script is useful for development but won't integrate with CI/CD pipelines. Consider either converting to pytest with proper assertions or clearly documenting this as a manual verification tool only.
backend/migrate_add_memories_columns.py (2)
21-47: Code duplication with verify_memories_setup.py.The
Colorsclass and print helper functions (print_header,print_success,print_error,print_info) are duplicated frombackend/app/utils/verify_memories_setup.py. Consider extracting these into a shared utility module to maintain DRY principle.# Create backend/app/utils/console_utils.py with shared Colors and print helpers # Then import in both files: from app.utils.console_utils import Colors, print_header, print_success, print_error, print_info
1-14: Consider adding a backup recommendation in the docstring.For a one-time migration script that modifies the database schema, it's good practice to recommend backing up the database before running. This protects users against unexpected issues.
""" One-time migration script to add Memories feature columns. Run this ONCE after pulling the new code. + +IMPORTANT: Back up your database before running this migration: + cp app/database/PictoPy.db app/database/PictoPy.db.backup This script adds:backend/app/utils/verify_memories_setup.py (2)
16-22: Consider extracting shared Colors class to avoid duplication.The
Colorsclass is duplicated inbackend/migrate_add_memories_columns.py. Consider extracting it to a shared utility module to maintain DRY principles.
252-268: Consider returning an exit code based on verification results.The script could return a non-zero exit code when checks fail, enabling use in CI/CD pipelines or automated setup scripts.
def main(): """Run all verification checks""" print(f"\n{Colors.BOLD}PictoPy Memories Feature Verification{Colors.RESET}") print(f"{Colors.BOLD}====================================={Colors.RESET}") results = { 'Dependencies': check_dependencies(), 'File Structure': check_file_structure(), 'Database Schema': check_database_schema(), 'Module Imports': check_imports(), 'API Routes': check_api_routes(), } print_summary(results) + + # Return non-zero exit code if any checks failed + all_passed = all(result is not False for result in results.values()) + return 0 if all_passed else 1 if __name__ == '__main__': - main() + exit(main())backend/app/utils/extract_location_metadata.py (2)
168-178: Fragile ISO datetime parsing logic.The timezone stripping logic using
split('-')is brittle and hard to follow. It works by coincidence but can fail for edge cases. Consider using a more robust approach.# Try ISO format first (handles timezone) if 'T' in date_str: try: - # Remove timezone suffix for simpler parsing - date_str_clean = date_str.replace('Z', '').split('+')[0].split('-') - # Rejoin only date-time parts (not timezone) - if len(date_str_clean) >= 3: - date_str_clean = '-'.join(date_str_clean[:3]) - captured_at = datetime.fromisoformat(date_str_clean) + # Remove timezone suffix for simpler parsing + clean = date_str.replace('Z', '+00:00') + # Remove timezone offset if present + if '+' in clean: + clean = clean.split('+')[0] + elif clean.count('-') > 2: # Has negative timezone offset + # Find the timezone separator (last '-' after 'T') + t_idx = clean.index('T') + last_dash = clean.rfind('-') + if last_dash > t_idx: + clean = clean[:last_dash] + captured_at = datetime.fromisoformat(clean) except Exception: pass
303-323: Consider using executemany for improved batch performance.The comment says "Batch update" but each row is updated individually. For large datasets,
executemany()with prepared statements or a single transaction with explicitBEGINcould improve performance.# Batch update database if updates: logger.info(f"Updating {len(updates)} images...") - for update_data in updates: - cursor.execute(""" - UPDATE images - SET latitude = ?, - longitude = ?, - captured_at = ? - WHERE id = ? - """, ( - update_data['latitude'], - update_data['longitude'], - update_data['captured_at'], - update_data['id'] - )) + cursor.executemany(""" + UPDATE images + SET latitude = ?, + longitude = ?, + captured_at = ? + WHERE id = ? + """, [ + (u['latitude'], u['longitude'], u['captured_at'], u['id']) + for u in updates + ]) conn.commit()backend/app/routes/memories.py (2)
129-131: Move import to module level for consistency.
db_get_all_images_for_memoriesis imported inside the function while similar functions are imported at module level. Unless avoiding a circular import, move it to the top.from app.database.images import ( db_get_images_with_location, db_get_images_by_date_range, - db_get_images_by_year_month + db_get_images_by_year_month, + db_get_all_images_for_memories )And remove the inline import at line 130.
362-365: Avoid accessing private methods ofMemoryClustering.Calling
_cluster_by_locationand_filter_valid_images(underscore-prefixed methods) creates tight coupling to implementation details. Consider adding a public method toMemoryClusteringfor location-only clustering.# In MemoryClustering class, add: def cluster_locations_only(self, images: List[dict]) -> List[List[dict]]: """Cluster images by location only (no date grouping).""" valid_images = self._filter_valid_images(images) return self._cluster_by_location(valid_images)Then update the endpoint:
- location_clusters = clustering._cluster_by_location( - clustering._filter_valid_images(images) - ) + location_clusters = clustering.cluster_locations_only(images)frontend/src/store/slices/memoriesSlice.ts (1)
177-196: Catch block is unreachable for thunk rejections.When
createAsyncThunkactions are dispatched, rejections don't throw—they return rejected action objects. The catch block will only catch non-thunk errors (which are unlikely here). Consider using.unwrap()if you need to catch rejections.export const fetchAllMemoriesData = createAsyncThunk< void, void, { rejectValue: string } >( 'memories/fetchAllData', async (_, { dispatch, rejectWithValue }) => { try { - await Promise.all([ - dispatch(fetchOnThisDay()), - dispatch(fetchRecentMemories(30)), - dispatch(fetchYearMemories(365)), - dispatch(fetchAllMemories()) - ]); + // Use unwrap() to convert rejected actions to thrown errors if needed + await Promise.all([ + dispatch(fetchOnThisDay()).unwrap(), + dispatch(fetchRecentMemories(30)).unwrap(), + dispatch(fetchYearMemories(365)).unwrap(), + dispatch(fetchAllMemories()).unwrap() + ]); } catch (error) { const apiError = error as ApiError; return rejectWithValue(apiError.message); } } );Alternatively, remove the try-catch since individual thunks already track their errors in state.
📜 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 (24)
backend/app/database/images.py(11 hunks)backend/app/routes/memories.py(1 hunks)backend/app/utils/extract_location_metadata.py(1 hunks)backend/app/utils/images.py(5 hunks)backend/app/utils/memory_clustering.py(1 hunks)backend/app/utils/verify_memories_setup.py(1 hunks)backend/extract_metadata_simple.py(1 hunks)backend/main.py(4 hunks)backend/migrate_add_memories_columns.py(1 hunks)backend/requirements.txt(2 hunks)backend/test_auto_gps_extraction.py(1 hunks)backend/test_memories_api.py(1 hunks)docs/backend/backend_python/openapi.json(6 hunks)frontend/src/app/store.ts(2 hunks)frontend/src/components/Memories/FeaturedMemoryCard.tsx(1 hunks)frontend/src/components/Memories/MemoriesPage.tsx(1 hunks)frontend/src/components/Memories/MemoryCard.tsx(1 hunks)frontend/src/components/Memories/MemoryViewer.tsx(1 hunks)frontend/src/components/Memories/index.ts(1 hunks)frontend/src/routes/AppRoutes.tsx(2 hunks)frontend/src/services/memoriesApi.ts(1 hunks)frontend/src/store/hooks.ts(1 hunks)frontend/src/store/slices/memoriesSlice.ts(1 hunks)frontend/tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
frontend/src/store/hooks.ts (1)
frontend/src/app/store.ts (2)
AppDispatch(26-26)RootState(24-24)
frontend/src/components/Memories/MemoryCard.tsx (2)
backend/app/routes/memories.py (1)
Memory(51-63)frontend/src/services/memoriesApi.ts (4)
Memory(32-44)getThumbnailUrl(462-470)formatPhotoCount(330-332)formatDateRangeRelative(341-389)
frontend/src/store/slices/memoriesSlice.ts (1)
frontend/src/services/memoriesApi.ts (6)
Memory(32-44)MemoryImage(20-27)generateMemories(120-139)ApiError(104-108)getTimeline(148-169)getOnThisDay(176-186)
backend/main.py (1)
backend/app/database/images.py (2)
db_create_images_table(58-109)db_migrate_add_memories_columns(112-165)
backend/test_auto_gps_extraction.py (1)
backend/app/utils/extract_location_metadata.py (2)
MetadataExtractor(27-356)extract_all(197-233)
backend/app/utils/verify_memories_setup.py (3)
backend/migrate_add_memories_columns.py (5)
Colors(21-27)print_header(31-35)print_success(37-39)print_error(41-43)print_info(45-47)backend/app/utils/extract_location_metadata.py (1)
main(359-386)backend/test_memories_api.py (1)
main(137-163)
backend/migrate_add_memories_columns.py (1)
backend/app/utils/verify_memories_setup.py (5)
Colors(16-22)print_header(24-28)print_success(30-32)print_error(34-36)print_info(42-44)
backend/app/database/images.py (3)
backend/app/models/FaceNet.py (1)
close(27-29)backend/app/models/FaceDetector.py (1)
close(69-79)backend/app/utils/images.py (1)
image_util_parse_metadata(524-541)
backend/app/utils/images.py (1)
backend/app/utils/extract_location_metadata.py (1)
extract_all(197-233)
frontend/src/services/memoriesApi.ts (1)
backend/app/routes/memories.py (7)
MemoryImage(41-48)Memory(51-63)GenerateMemoriesResponse(66-72)TimelineResponse(75-80)OnThisDayResponse(83-89)LocationCluster(92-98)LocationsResponse(101-105)
🔇 Additional comments (32)
backend/app/database/images.py (3)
112-166: Well-structured migration function with proper error handling.Good defensive coding: checks table existence before proceeding, uses PRAGMA to detect existing columns, and handles transactions properly with commit/rollback. The logging provides useful feedback on migration progress.
520-596: LGTM - datetime handling with SQLite.The date range query is well-structured. SQLite handles Python
datetimeobjects correctly when using the sqlite3 adapter. The optional favorites filter is cleanly integrated.Note: The same import-in-loop pattern exists here (line 574) - consider moving it outside.
177-194: The concern in this review is not applicable. Theimage_util_prepare_image_recordsfunction already ensures thatlatitude,longitude, andcaptured_atare always included in every record dictionary (even if set toNone), specifically to satisfy the SQL INSERT statement's named parameters. NoKeyErrorwill occur.backend/app/utils/memory_clustering.py (4)
70-103: LGTM - Simple reverse geocoding implementation.The Haversine distance calculation is correct, and the linear search through ~30 cities is acceptable. The 50km default radius is reasonable for city matching.
140-204: Well-structured main clustering method.The separation of images into GPS-based and date-only categories is clear, with appropriate fallback handling. Good use of logging to track processing counts.
591-635: LGTM - DBSCAN implementation with proper coordinate handling.The coordinates are correctly converted to radians for the haversine metric, and noise points are handled by creating individual clusters. Note: The
epsparameter issue was flagged in the earlier comment about initialization.
637-848: Helper methods are well-implemented.The temporal clustering, memory creation, and utility functions follow consistent patterns with proper datetime handling and error logging. The title and description generation provides good user-facing text.
frontend/src/routes/AppRoutes.tsx (1)
12-12: LGTM - Route correctly wired to MemoriesPage.The Memories route is properly updated to render the new
MemoriesPagecomponent instead of theComingSoonplaceholder. The barrel export inindex.tscorrectly exposes the component, and the import path resolves as expected.frontend/tsconfig.json (1)
25-25: Verify if deprecated TypeScript options are present and whether this suppression is necessary.The
ignoreDeprecations: "6.0"setting is the official TypeScript mechanism to suppress deprecation errors scheduled for TypeScript 6.0, but it should not be used as a permanent mask. Review which specific options in this configuration are deprecated and either migrate them or document why the suppression is needed. The current config uses modern settings (ES2020, ESNext, bundler moduleResolution), but the presence of this flag suggests at least one deprecated option requires attention.backend/requirements.txt (1)
34-34: Inline comments are helpful and versions are secure.The inline comments effectively document the dependency purposes for the Memories feature. Verification confirms that numpy 1.26.4 and scikit-learn 1.5.1 have no known CVEs. Both versions are from stable release series and are appropriately pinned for consistency.
frontend/src/components/Memories/index.ts (1)
1-14: Clean barrel exports.frontend/src/app/store.ts (1)
9-21: Store wiring formemorieslooks good.docs/backend/backend_python/openapi.json (2)
1308-1382: New Memories API endpoints are well-defined.The
/api/memories/generateendpoint has appropriate parameter constraints (location_radius_km: 0.1-100,date_tolerance_days: 1-30,min_images: 1-10) and proper response schemas. The OpenAPI spec correctly references the newGenerateMemoriesResponseandMemoryschemas.
2606-2686: Memory schema looks comprehensive.The
Memoryschema correctly defines all required fields includingmemory_id,title,location_name,date_start/date_end(nullable),images,thumbnail_image_id, and GPS coordinates. This aligns with the frontend'sMemoryinterface.frontend/src/components/Memories/FeaturedMemoryCard.tsx (1)
39-52: Good accessibility implementation.The component properly implements keyboard navigation with
role="button",tabIndex={0}, and Enter/Space key handling. Thearia-labelprovides meaningful context for screen readers.frontend/src/components/Memories/MemoryViewer.tsx (2)
211-248: Good image grid implementation with accessibility.The image grid properly handles click events, includes hover effects, and uses lazy loading. The structure is clean and performant.
281-291: The metadatawidthandheightfields are set to0to satisfy theImageMetadatatype interface requirements, but they are not used anywhere in the codebase. MediaView, ImageViewer, and MediaInfoPanel do not access or perform calculations with these values, so there is no risk of division-by-zero or aspect ratio issues. The fields can remain as-is since they're unused placeholders.frontend/src/components/Memories/MemoryCard.tsx (2)
26-34: Good defensive thumbnail resolution with fallback chain.The code properly handles missing thumbnails by checking for
thumbnail_image_id, falling back to the first image, and ultimately to a default placeholder. This prevents broken images in the UI.
55-68: Good accessibility implementation.Properly implements keyboard navigation with
role="button",tabIndex={0}, Enter/Space key handling, and a descriptivearia-label.backend/migrate_add_memories_columns.py (2)
113-126: Index creation is properly idempotent.Using
CREATE INDEX IF NOT EXISTSensures the migration can be safely re-run without errors. Good defensive approach.
199-214: Good error handling with rollback on failure.The migration properly rolls back changes on SQLite or unexpected errors, preventing partial migrations that could corrupt the database. The
finallyblock ensures the connection is always closed.backend/app/utils/verify_memories_setup.py (2)
46-73: LGTM!The dependency checking logic is sound with appropriate handling for version mismatches and missing packages.
194-226: LGTM!The API route verification correctly imports the FastAPI app and validates required endpoints with proper error handling.
backend/app/utils/extract_location_metadata.py (1)
197-233: LGTM!The
extract_allmethod properly handles edge cases (null, empty, bytes) and orchestrates the extraction process cleanly.backend/app/routes/memories.py (2)
41-106: LGTM!Response models are well-defined and align with the frontend TypeScript interfaces in
memoriesApi.ts.
246-313: LGTM!The "On This Day" endpoint correctly searches across previous years with proper error handling per year iteration.
frontend/src/store/slices/memoriesSlice.ts (2)
26-84: LGTM!The state interface is well-structured with granular loading and error states per section, enabling fine-grained UI feedback.
232-306: LGTM!The extra reducers correctly handle all async states (pending/fulfilled/rejected) for each thunk with appropriate error fallback messages.
frontend/src/services/memoriesApi.ts (4)
20-108: LGTM!TypeScript interfaces correctly mirror the backend Pydantic models from
memories.py.
341-389: LGTM!The relative date formatting logic is comprehensive and handles various time ranges appropriately.
398-434: LGTM!The memory title generation provides good UX by creating human-readable titles based on location and photo count with sensible fallbacks.
462-469: LGTM!Proper use of Tauri's
convertFileSrcfor desktop file path handling with a reasonable fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/utils/images.py (1)
24-30: Duplicate logger definitions - second overwrites the first.The logger is defined twice: line 24 uses
get_logger(__name__)(custom logger) and line 30 useslogging.getLogger(__name__)(standard logger). The second definition overwrites the first, which may cause inconsistent logging behavior.from app.logging.setup_logging import get_logger from app.utils.extract_location_metadata import MetadataExtractor logger = get_logger(__name__) -# GPS EXIF tag constant -GPS_INFO_TAG = 34853 - -logger = logging.getLogger(__name__) +# GPS EXIF tag constant +GPS_INFO_TAG = 34853
♻️ Duplicate comments (5)
backend/app/utils/verify_memories_setup.py (1)
120-123: Inconsistent database filename in warning message.The code checks for
PictoPy.db(line 118) but the warning message referencesgallery.db. This confuses users.if not db_path.exists(): - print_warning("Database file 'gallery.db' not found") + print_warning("Database file 'PictoPy.db' not found") print_info(" → Database will be created on first run") return None # Not an error, just not initialized yetfrontend/src/components/Memories/MemoriesPage.tsx (2)
154-171: Location/Date filtering viacenter_lat/center_lon === 0is incorrect.Using
0as "no GPS" breaks for valid locations on the equator/prime meridian (e.g., Gulf of Guinea, Null Island). Consider using an explicit discriminator from the backend (e.g.,location_name === 'Date-Based Memory') or null checks.// Calculate counts - const locationCount = allMemories.filter( - (m) => m.center_lat !== 0 || m.center_lon !== 0, - ).length; - const dateCount = allMemories.filter( - (m) => m.center_lat === 0 && m.center_lon === 0, - ).length; + const isDateBasedMemory = (m: Memory) => m.location_name === 'Date-Based Memory'; + const locationCount = allMemories.filter((m) => !isDateBasedMemory(m)).length; + const dateCount = allMemories.filter((m) => isDateBasedMemory(m)).length; // Simple filter function const applyFilter = (memories: Memory[]) => { if (filter === 'location') { - return memories.filter((m) => m.center_lat !== 0 || m.center_lon !== 0); + return memories.filter((m) => !isDateBasedMemory(m)); } if (filter === 'date') { - return memories.filter((m) => m.center_lat === 0 && m.center_lon === 0); + return memories.filter((m) => isDateBasedMemory(m)); } return memories; // 'all' };
188-207: On This Day temp memory assumes images are sorted.
date_start/date_enduse first/last array elements. IfonThisDayImagesisn't sorted bycaptured_at, the date range will be incorrect.Sort images or compute min/max dates explicitly:
const handleOnThisDayClick = () => { if (onThisDayImages.length > 0 && onThisDayMeta) { + // Sort images by captured_at to ensure correct date range + const sortedImages = [...onThisDayImages].sort((a, b) => { + const dateA = a.captured_at ? new Date(a.captured_at).getTime() : 0; + const dateB = b.captured_at ? new Date(b.captured_at).getTime() : 0; + return dateA - dateB; + }); const tempMemory: Memory = { memory_id: 'on-this-day', title: `On This Day - ${onThisDayMeta.today}`, description: `Photos from ${onThisDayMeta.years.join(', ')}`, location_name: 'Various locations', - date_start: onThisDayImages[0]?.captured_at || null, - date_end: - onThisDayImages[onThisDayImages.length - 1]?.captured_at || null, + date_start: sortedImages[0]?.captured_at || null, + date_end: sortedImages[sortedImages.length - 1]?.captured_at || null, image_count: onThisDayImages.length, images: onThisDayImages, - thumbnail_image_id: onThisDayImages[0]?.id || '', - center_lat: onThisDayImages[0]?.latitude || 0, - center_lon: onThisDayImages[0]?.longitude || 0, + thumbnail_image_id: sortedImages[0]?.id || '', + center_lat: sortedImages[0]?.latitude || 0, + center_lon: sortedImages[0]?.longitude || 0, };frontend/src/components/Memories/MemoryViewer.tsx (2)
57-74: ESC key still closes the entire viewer when MediaView is open.The past review comment on this code segment was not addressed. When
showMediaViewis true, pressing ESC will close the MemoryViewer entirely instead of just MediaView, bypassing MediaView's own keyboard handling.Apply this diff to respect MediaView's state:
useEffect(() => { const handleEsc = (e: KeyboardEvent) => { - if (e.key === 'Escape') { + if (e.key === 'Escape' && !showMediaView) { handleCloseViewer(); } }; if (memory) { document.addEventListener('keydown', handleEsc); // Prevent body scroll when modal is open document.body.style.overflow = 'hidden'; } return () => { document.removeEventListener('keydown', handleEsc); document.body.style.overflow = 'unset'; }; - }, [memory, handleCloseViewer]); + }, [memory, showMediaView, handleCloseViewer]);
70-73: Use empty string instead of 'unset' for body overflow reset.The past review comment on this code segment was not addressed. Setting
document.body.style.overflow = 'unset'may not properly restore the original value. Use an empty string to remove the inline style entirely, allowing CSS to take over.Apply this diff:
return () => { document.removeEventListener('keydown', handleEsc); - document.body.style.overflow = 'unset'; + document.body.style.overflow = ''; };
🧹 Nitpick comments (11)
backend/app/utils/verify_memories_setup.py (2)
17-51: Consider extracting shared CLI utilities to avoid code duplication.The
Colorsclass and print helper functions (print_header,print_success,print_error,print_warning,print_info) are duplicated frombackend/migrate_add_memories_columns.py. Extract these into a shared module (e.g.,app/utils/cli_helpers.py) to follow DRY principles.
67-80: Version comparison using string equality may produce false warnings.Comparing versions with
version != expected_versioncan flag warnings for compatible versions (e.g.,1.26.5vs1.26.4). Consider using semantic version comparison or removing strict version checks for informational purposes.- if expected_version and version != expected_version: - print_warning( - f"{package} installed (v{version}), expected v{expected_version}" - ) + if expected_version: + # Info only - exact version match not required + if version != expected_version: + print_info(f" Note: expected v{expected_version}") + print_success(f"{package} v{version}")frontend/src/services/memoriesApi.ts (1)
315-323:calculateYearsAgois imprecise for edge cases.Using
getFullYear()difference doesn't account for whether the anniversary date has passed this year. For example, on January 1, 2025, a photo from December 31, 2024 would show as "1 year ago" when it's actually 1 day ago.This is acceptable for a rough "years ago" display, but if precision matters, consider computing based on full date comparison.
backend/app/utils/images.py (1)
209-213: Minor: Redundant truthiness check on datetime object.The condition
isinstance(captured_at, datetime.datetime) and captured_atis redundant since datetime objects are always truthy. However, this doesn't affect correctness."captured_at": ( captured_at.isoformat() - if isinstance(captured_at, datetime.datetime) and captured_at + if isinstance(captured_at, datetime.datetime) else captured_at ), # Can be Nonedocs/backend/backend_python/openapi.json (1)
1460-1480: On This Day endpoint has no error responses defined.Unlike other memories endpoints,
/api/memories/on-this-dayonly defines a 200 response. Consider adding a 500 error response for consistency with other endpoints and to document potential database query failures mentioned in the description.frontend/src/components/Memories/MemoryCard.tsx (1)
26-36: Consider simplifying the thumbnail fallback chain.The nested ternary for thumbnail selection is complex. The logic tries
thumbnail_image_id, then first image, then first image path, then placeholder. Consider extracting to a helper function for clarity.Example refactor:
const getThumbnailUrl = (): string => { const thumbnailImage = memory.images.find((img) => img.id === memory.thumbnail_image_id) || memory.images[0]; if (thumbnailImage) { return getThumbnailUrl(thumbnailImage); } return '/photo.png'; // Default placeholder }; const thumbnailUrl = getThumbnailUrl();backend/app/database/images.py (1)
633-641: Consider additional safeguards for extreme latitude lon_offset values.While the clamping to 0.01 prevents division by zero, it can still produce very large
lon_offsetvalues (~180) near the poles. Consider adding an explicit upper bound check.Apply this diff:
# Calculate bounding box offsets lat_offset = radius_km / 111.0 cos_lat = abs(math.cos(math.radians(latitude))) # Clamp to avoid division by near-zero at poles - lon_offset = radius_km / (111.0 * max(cos_lat, 0.01)) + lon_offset = min(radius_km / (111.0 * max(cos_lat, 0.01)), 180.0)backend/app/routes/memories.py (2)
382-391: Avoid calling private methods of MemoryClustering.Directly accessing
_cluster_by_locationand_filter_valid_imagesbreaks encapsulation. Consider either:
- Making these methods public if they're part of the API surface
- Adding a dedicated public method to MemoryClustering for location-only clustering
Consider adding a public method to
MemoryClustering:def cluster_by_location_only(self, images: List[Dict[str, Any]]) -> List[List[Dict[str, Any]]]: """Public API for location-only clustering.""" valid_images = self._filter_valid_images(images) return self._cluster_by_location(valid_images)Then update the route:
- # Use internal method to get location clusters - location_clusters = clustering._cluster_by_location( - clustering._filter_valid_images(images) - ) + # Get location clusters + location_clusters = clustering.cluster_by_location_only(images)
382-386: Replace magic number with a constant or max value.Using
999999as a large date tolerance is unclear. Consider usingsys.maxsize,float('inf'), or a named constant likeNO_DATE_CLUSTERING.Example:
# At module level NO_DATE_CLUSTERING = sys.maxsize # Group all dates together # In the endpoint clustering = MemoryClustering( location_radius_km=location_radius_km, date_tolerance_days=NO_DATE_CLUSTERING, min_images_per_memory=1, )backend/app/utils/memory_clustering.py (2)
33-66: Consider externalizing city coordinates to a configuration file.The hardcoded
CITY_COORDINATESdictionary makes it difficult to add new cities or customize for different deployments. Consider moving to a JSON/YAML config file or database table.This would allow users to add their own cities without code changes:
# Load from config file with open('config/city_coordinates.json') as f: CITY_COORDINATES = json.load(f)Or make it extensible:
# Allow runtime additions def add_city_coordinate(city_name: str, latitude: float, longitude: float): CITY_COORDINATES[city_name] = (latitude, longitude)
326-359: Complex title generation logic could be extracted to a separate method.The title generation for location-based memories (lines 326-359) has nested conditionals for date formatting. Consider extracting the date range formatting logic to improve readability.
Example refactor:
def _format_location_title_with_dates(self, location_name: str, dates: List[datetime]) -> str: """Format location title with date range.""" if len(dates) <= 1: return location_name start_date = min(dates) end_date = max(dates) if start_date.strftime("%B %Y") == end_date.strftime("%B %Y"): # Same month return f"{location_name} in {start_date.strftime('%b %Y')}" elif start_date.year == end_date.year: # Same year return f"{location_name} - {start_date.strftime('%b')}-{end_date.strftime('%b %Y')}" else: # Different years return f"{location_name} - {start_date.strftime('%b %Y')} to {end_date.strftime('%b %Y')}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/app/database/images.py(11 hunks)backend/app/routes/memories.py(1 hunks)backend/app/utils/extract_location_metadata.py(1 hunks)backend/app/utils/images.py(5 hunks)backend/app/utils/memory_clustering.py(1 hunks)backend/app/utils/verify_memories_setup.py(1 hunks)docs/backend/backend_python/openapi.json(6 hunks)docs/frontend/memories.md(1 hunks)docs/overview/features.md(2 hunks)frontend/src/components/Memories/FeaturedMemoryCard.tsx(1 hunks)frontend/src/components/Memories/MemoriesPage.tsx(1 hunks)frontend/src/components/Memories/MemoryCard.tsx(1 hunks)frontend/src/components/Memories/MemoryViewer.tsx(1 hunks)frontend/src/components/Memories/index.ts(1 hunks)frontend/src/services/memoriesApi.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/overview/features.md
- docs/frontend/memories.md
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/Memories/FeaturedMemoryCard.tsx
- backend/app/utils/extract_location_metadata.py
🧰 Additional context used
🧬 Code graph analysis (5)
backend/app/utils/images.py (1)
backend/app/utils/extract_location_metadata.py (2)
MetadataExtractor(27-381)extract_all(209-247)
backend/app/utils/verify_memories_setup.py (1)
backend/migrate_add_memories_columns.py (3)
Colors(21-27)print_header(31-35)print_success(37-39)
backend/app/database/images.py (1)
backend/app/utils/images.py (1)
image_util_parse_metadata(534-551)
frontend/src/components/Memories/MemoryViewer.tsx (5)
frontend/src/store/hooks.ts (2)
useAppDispatch(13-13)useAppSelector(14-14)frontend/src/store/slices/memoriesSlice.ts (1)
selectSelectedMemory(329-329)frontend/src/features/imageSlice.ts (1)
setCurrentViewIndex(22-34)frontend/src/services/memoriesApi.ts (5)
generateMemoryTitle(399-435)formatLocationName(443-455)formatDateRangeRelative(342-390)formatPhotoCount(331-333)getThumbnailUrl(463-471)frontend/src/components/Media/MediaView.tsx (1)
MediaView(25-224)
frontend/src/services/memoriesApi.ts (3)
frontend/src/config/Backend.ts (1)
BACKEND_URL(1-1)backend/app/routes/memories.py (7)
MemoryImage(42-50)Memory(53-66)GenerateMemoriesResponse(69-76)TimelineResponse(79-85)OnThisDayResponse(88-95)LocationCluster(98-105)LocationsResponse(108-113)frontend/src/components/Memories/index.ts (2)
MemoryImage(14-14)Memory(14-14)
🔇 Additional comments (9)
backend/app/utils/verify_memories_setup.py (1)
211-243: Modifyingsys.pathcan cause import side effects.Inserting into
sys.pathat line 217 may cause unintended module resolution issues. This is acceptable for a standalone verification script, but ensure this script isn't imported elsewhere.frontend/src/services/memoriesApi.ts (2)
1-12: LGTM!Good use of centralized
BACKEND_URLfrom config, and the imports are appropriate for the service's responsibilities.
21-45: Interface definitions align well with backend schemas.The
MemoryImageandMemoryinterfaces correctly mirror the backend Pydantic models frombackend/app/routes/memories.py. The nullable fields (number | null,string | null) properly match the backend'sOptionaltypes.frontend/src/components/Memories/MemoriesPage.tsx (2)
37-58: LGTM!Clean skeleton components with appropriate Tailwind classes for dark mode support and consistent styling.
178-181: Data fetching on mount looks correct.The
useEffectproperly includesdispatchin the dependency array and triggersfetchAllMemoriesDataon mount.backend/app/utils/images.py (2)
155-156: LGTM - MetadataExtractor instantiation.Creating the extractor once outside the loop is efficient. Good practice.
173-216: GPS extraction integration looks good with proper error handling.The try/except ensures GPS extraction failures don't block image uploads. Logging provides visibility into extraction results. The record always includes latitude, longitude, and captured_at fields (even when None) to satisfy SQL INSERT parameters.
docs/backend/backend_python/openapi.json (2)
1308-1382: LGTM - Generate memories endpoint well-defined.The
/api/memories/generatePOST endpoint has proper parameter constraints (min/max values), sensible defaults, and appropriate response schemas.
1936-1972: LGTM - Memory schemas align with backend models.The
GenerateMemoriesResponse,Memory, andMemoryImageschemas correctly mirror the Pydantic models defined inbackend/app/routes/memories.py. Required fields and nullable types are properly specified.Also applies to: 2606-2746
|
here's the demo video : demo-video-comp.mov |
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
There is an error occurring. See the screenshot above. Please fix it.
Also, please pull the latest changes into this PR and resolve any merge conflicts. I have approved this PR for the Unstoppable Hackathon and your team will receive 100 points for this PR.
|
sure , i will fix it up shortly . also if you require any extra changes like the notification for the memories similar to what google photos does and any other further additions please feel free to reach out to me |
Sure. For now we will merge this and in future we can go for notifications. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/main.py`:
- Line 49: Guard the call to db_migrate_add_memories_columns() so only one
process runs migrations: wrap the call in a check that only runs when a
designated env flag is set (e.g., os.getenv("RUN_MIGRATIONS") == "true") or when
the process is the primary/master worker, and skip it otherwise; also add a safe
try/except around db_migrate_add_memories_columns() to log failures without
crashing other workers. Ensure you update the startup path that currently calls
db_migrate_add_memories_columns() so it only executes when the env flag or
master-process condition is satisfied.
In `@docs/backend/backend_python/openapi.json`:
- Around line 2661-2683: The Memory schema currently requires center_lat and
center_lon but date-based memories can lack GPS; make center_lat and center_lon
nullable and remove them from the required list in the Memory schema (same
approach used by MemoryImage.latitude and MemoryImage.longitude) so the fields
can be present with null values or omitted for non-GPS/date-based memories;
update the "center_lat" and "center_lon" definitions to allow null (e.g., type:
["number","null"]) and remove them from the required array for the Memory
schema.
🧹 Nitpick comments (3)
docs/backend/backend_python/openapi.json (3)
1308-1312: API path prefix inconsistency.The new memory endpoints use
/api/memories/prefix while all existing endpoints use bare paths like/folders/,/albums/,/images/. This inconsistency may cause confusion for API consumers.Consider aligning with the existing convention by using
/memories/instead of/api/memories/.
1360-1381: Missing 500 error response documentation.The memory endpoints only document 200 and 422 responses, but don't include 500 Internal Server Error responses. Other endpoints in this API (e.g.,
/folders/add-folder,/images/) consistently document 500 error responses with appropriate error schemas.For API consistency, consider adding 500 error responses to all memory endpoints, referencing a suitable error schema.
3071-3105: Consider defining explicit properties fordate_range.The
date_rangefield uses a genericobjecttype withadditionalProperties: string. For better API documentation and client code generation, consider defining explicitstartandendproperties.Suggested improvement
"date_range": { - "additionalProperties": { - "type": "string" + "type": "object", + "properties": { + "start": { + "type": "string", + "title": "Start Date" + }, + "end": { + "type": "string", + "title": "End Date" + } }, - "type": "object", + "required": ["start", "end"], "title": "Date Range" },
…nd-implementation
…tion' into feat/memories-backend-implementation
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@backend/app/routes/memories.py`:
- Around line 54-68: The Memory model currently declares center_lat and
center_lon as required floats which prevents nullable values; update the Memory
class to make these fields Optional[float] and assign a default of None (e.g.,
center_lat: Optional[float] = None, center_lon: Optional[float] = None) and
ensure Optional is imported from typing so the generated OpenAPI schema marks
them nullable and avoids validation errors for date-based memories without GPS.
In `@frontend/src/components/Memories/MemoryViewer.tsx`:
- Around line 41-53: handleToggleFavorite currently performs an optimistic UI
update by dispatching toggleImageFavorite after calling togglefav, but on API
failure it only logs to console; update handleToggleFavorite to either show a
user-facing toast (e.g., call showToast or enqueueSnackbar) with the error
message in the catch block and/or revert the optimistic change by dispatching
toggleImageFavorite(imageId) again to undo the state change; reference the
existing functions togglefav, toggleImageFavorite, and handleToggleFavorite and
ensure the catch block performs the toast and/or revert so users receive
immediate feedback when the API call fails.
In `@frontend/src/services/memoriesApi.ts`:
- Around line 34-46: The Memory interface defines center_lat and center_lon as
non-nullable numbers but the API can return null for memories without GPS;
update the Memory interface (symbols: Memory, center_lat, center_lon) to allow
null (e.g., number | null) and adjust any downstream consumers/type guards to
handle null values where Memory objects are used so code won’t assume numeric
coordinates exist.
🧹 Nitpick comments (7)
backend/app/routes/memories.py (2)
145-148: Move import to module level for clarity.The
db_get_all_images_for_memoriesimport is placed inside the function body. If this is to avoid circular imports, consider adding a comment explaining why. Otherwise, move it to the top-level imports for consistency with other database function imports.Proposed fix
from app.database.images import ( db_get_images_with_location, db_get_images_by_date_range, db_get_images_by_year_month, + db_get_all_images_for_memories, )Then remove the inline import at line 146.
389-422: Avoid accessing private methods ofMemoryClustering.The endpoint directly calls
_cluster_by_location,_filter_valid_images, and_reverse_geocodewhich are internal implementation details (prefixed with_). This creates tight coupling and fragile code that may break ifMemoryClusteringinternals change.Consider adding a public method to
MemoryClusteringspecifically for location-only clustering.Proposed approach
Add a public method to
MemoryClustering:# In memory_clustering.py def get_location_clusters(self, images: List[Dict]) -> List[Dict]: """Public API for location-only clustering.""" valid_images = self._filter_valid_images(images) clusters = self._cluster_by_location(valid_images) return [ { "images": cluster, "center_lat": sum(img["latitude"] for img in cluster) / len(cluster), "center_lon": sum(img["longitude"] for img in cluster) / len(cluster), "location_name": self._reverse_geocode(center_lat, center_lon), } for cluster in clusters if cluster ]Then use it in the endpoint:
location_clusters = clustering.get_location_clusters(images)backend/main.py (1)
49-60: Migration guard implemented, but consider fail-fast option for critical environments.The
RUN_MIGRATIONSenvironment variable guard addresses the multi-worker contention concern. However, the current implementation silently continues if migrations fail. For development/staging environments, consider an additional flag to fail-fast on migration errors to catch issues early.Proposed enhancement
if should_run_migrations: try: db_migrate_add_memories_columns() logger.info("Database migrations completed successfully") except Exception as e: logger.error(f"Failed to run database migrations: {e}", exc_info=True) + # Optionally fail-fast in non-production environments + if os.getenv("FAIL_ON_MIGRATION_ERROR", "false").lower() == "true": + raisefrontend/src/components/Memories/MemoryViewer.tsx (2)
61-80: Duplicate image transformation logic.The same image-to-Redux-format mapping appears twice: once in
handleImageClick(lines 61-80) and again inline in theMediaViewprops (lines 326-345). Extract this to a memoized helper to avoid duplication and ensure consistency.Proposed refactor
+ // Memoize transformed images for MediaView + const formattedImages = useMemo(() => { + if (!memory) return []; + return memory.images.map((img) => ({ + id: img.id, + path: img.path, + thumbnailPath: img.thumbnailPath, + folder_id: '', + isTagged: false, + isFavourite: img.isFavourite || false, + tags: [], + metadata: { + name: img.path.split('/').pop() || '', + date_created: img.captured_at, + width: 0, + height: 0, + file_location: img.path, + file_size: 0, + item_type: 'image' as const, + latitude: img.latitude || undefined, + longitude: img.longitude || undefined, + }, + })); + }, [memory]); // Handle image click - open MediaView const handleImageClick = useCallback( (index: number) => { if (!memory) return; - // Convert memory images to Image[] format for Redux state - const formattedImages = memory.images.map((img) => ({ - ...mapping logic... - })); - // Set images in Redux state first dispatch(setImages(formattedImages)); // Then set the current index dispatch(setCurrentViewIndex(index)); setShowMediaView(true); }, - [memory, dispatch], + [memory, dispatch, formattedImages], );Then use
formattedImagesdirectly in the MediaView:<MediaView onClose={handleMediaViewClose} type="image" onToggleFavorite={handleToggleFavorite} - images={memory.images.map((img) => ({ - ...mapping logic... - }))} + images={formattedImages} />Also applies to: 326-345
125-127: Consider memoizinghandleImageError.This handler is recreated on every render. While not critical for an
onErrorhandler, wrapping it inuseCallbackor moving it outside the component as a stable reference would be more consistent with the other handlers in this file.Proposed fix
+ // Handle image load error - show placeholder + const handleImageError = useCallback((e: React.SyntheticEvent<HTMLImageElement>) => { + e.currentTarget.src = '/photo.png'; + }, []); + // Don't render if no memory selected if (!memory) return null; // Generate better title and format location const displayTitle = generateMemoryTitle(memory); const displayLocation = formatLocationName(memory.location_name); - - // Handle image load error - const handleImageError = (e: React.SyntheticEvent<HTMLImageElement>) => { - e.currentTarget.src = '/photo.png'; - };frontend/src/store/slices/memoriesSlice.ts (2)
177-196: The try/catch infetchAllMemoriesDatawon't catch individual thunk rejections.When using
dispatch(thunk()), the returned promise resolves with the action (fulfilled or rejected), not the actual result. This meansPromise.allwill succeed even if individual thunks fail, and thecatchblock here will never execute from thunk failures.Since individual errors are already handled by each thunk's
extraReducers, this is functionally fine, but the try/catch is misleading. Consider either removing it or usingunwrapResultif you need to detect failures in the composite thunk.Option 1: Simplify by removing misleading try/catch
export const fetchAllMemoriesData = createAsyncThunk< void, void, - { rejectValue: string } + Record<string, never> >( 'memories/fetchAllData', - async (_, { dispatch, rejectWithValue }) => { - try { - await Promise.all([ - dispatch(fetchOnThisDay()), - dispatch(fetchRecentMemories(30)), - dispatch(fetchYearMemories(365)), - dispatch(fetchAllMemories()) - ]); - } catch (error) { - const apiError = error as ApiError; - return rejectWithValue(apiError.message); - } + async (_, { dispatch }) => { + await Promise.all([ + dispatch(fetchOnThisDay()), + dispatch(fetchRecentMemories(30)), + dispatch(fetchYearMemories(365)), + dispatch(fetchAllMemories()) + ]); } );Option 2: Properly handle rejections with unwrapResult
+import { unwrapResult } from '@reduxjs/toolkit'; + export const fetchAllMemoriesData = createAsyncThunk< void, void, { rejectValue: string } >( 'memories/fetchAllData', async (_, { dispatch, rejectWithValue }) => { try { - await Promise.all([ - dispatch(fetchOnThisDay()), - dispatch(fetchRecentMemories(30)), - dispatch(fetchYearMemories(365)), - dispatch(fetchAllMemories()) - ]); + const results = await Promise.allSettled([ + dispatch(fetchOnThisDay()).then(unwrapResult), + dispatch(fetchRecentMemories(30)).then(unwrapResult), + dispatch(fetchYearMemories(365)).then(unwrapResult), + dispatch(fetchAllMemories()).then(unwrapResult) + ]); + const rejected = results.filter(r => r.status === 'rejected'); + if (rejected.length > 0) { + return rejectWithValue('Some memories failed to load'); + } } catch (error) { const apiError = error as ApiError; return rejectWithValue(apiError.message); } } );
216-250: Consider normalizing state if performance becomes an issue.The
toggleImageFavoritereducer iterates through all memories and images to find and update matching IDs. For large photo libraries, this O(n×m) operation could cause UI lag.This is fine for now, but if performance issues arise, consider using RTK's
createEntityAdapterto normalize images by ID for O(1) lookups.
|
Also, the lint check is not passing again. To fix it use this command inside the backend folder: First do |
|
TLDR: Make the memories UI feel like a part of the app itself and not a one-off random feature. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/Media/MediaInfoPanel.tsx (1)
168-176:⚠️ Potential issue | 🟡 Minor"Open Original File" button is visually active but functionally dead.
The button retains hover styles and appears clickable, yet the handler is a no-op. This is confusing for users. Either disable it properly or remove it entirely.
Option A: Disable the button visually
<div className="mt-4 border-t border-white/10 pt-3"> <button - className="w-full rounded-lg bg-white/10 py-2 text-white hover:bg-white/20" - onClick={(e) => { - e.preventDefault(); - // Button disabled - does nothing - }} + className="w-full rounded-lg bg-white/10 py-2 text-white/40 cursor-not-allowed" + disabled > Open Original File </button> </div>Option B: Remove the button entirely
- <div className="mt-4 border-t border-white/10 pt-3"> - <button - className="w-full rounded-lg bg-white/10 py-2 text-white hover:bg-white/20" - onClick={(e) => { - e.preventDefault(); - // Button disabled - does nothing - }} - > - Open Original File - </button> - </div>
🤖 Fix all issues with AI agents
In `@backend/app/database/images.py`:
- Around line 892-893: The latitude/longitude assignment is treating 0.0 as
falsy and turning valid coordinates into None; update the two expressions to
check for None explicitly (e.g., use row[7] if row[7] is not None else None and
row[8] if row[8] is not None else None) so that legitimate 0.0 values for
"latitude" and "longitude" are preserved (refer to the "latitude" and
"longitude" dictionary entries that currently use row[7] and row[8]).
In `@frontend/src/components/Memories/MemoriesPage.tsx`:
- Around line 151-166: totalCount is computed from selectTotalMemoryCount (which
returns allMemories.length) but locationCount/dateCount only include
memoriesWithMultipleImages, causing mismatch; update totalCount to use the same
filter by calculating it from memoriesWithMultipleImages(allMemories).length (or
alternatively update selectTotalMemoryCount to apply the image_count >= 2
filter) so totalCount === locationCount + dateCount; reference the symbols
totalCount, selectTotalMemoryCount, memoriesWithMultipleImages, locationCount,
dateCount, and allMemories when making the change.
In `@frontend/src/components/Memories/MemoryDetail.tsx`:
- Around line 98-123: The loader is dispatched and hidden synchronously in
MemoryDetail.tsx so the UI never paints it; update the flow around
showLoader/hideLoader and setImages so the hide happens after a microtask/next
paint — e.g., dispatch(showLoader('Loading memory')) before you build
formattedImages, then defer dispatch(setImages(formattedImages)) and
dispatch(hideLoader()) to the next tick (use requestAnimationFrame or
Promise.resolve().then(...)) so the loader becomes visible; reference the
showLoader, hideLoader, setImages dispatches and the formattedImages
construction when applying the change.
- Around line 117-118: The code in MemoryDetail.tsx uses `img.latitude ||
undefined` and `img.longitude || undefined`, which treats 0.0 as falsy and turns
valid equatorial coordinates into undefined; replace these expressions so zero
is preserved (e.g., use the nullish coalescing operator `??` or an explicit
numeric check) so `img.latitude` and `img.longitude` remain 0 when appropriate.
- Around line 126-133: Remove the no-op useEffect in MemoryDetail.tsx: delete
the empty effect (the useEffect with empty body and cleanup) inside the
MemoryDetail component and either implement the intended refetch logic (e.g.,
dispatch a Redux action or subscribe to favorite-toggle events inside useEffect)
or simply remove the hook to avoid dead code; locate the useEffect by
referencing MemoryDetail and the empty useEffect block that contains only
comments and a return cleanup.
In `@frontend/src/store/slices/memoriesSlice.ts`:
- Around line 161-177: The catch/rejectWithValue in fetchAllMemoriesData is dead
because dispatching thunks returns fulfilled/rejected action objects (never
throws); remove the try/catch and rejectWithValue and simply run/await the group
of dispatched thunks (e.g., await Promise.all([dispatch(fetchOnThisDay()),
dispatch(fetchRecentMemories(30)), dispatch(fetchYearMemories(365)),
dispatch(fetchAllMemories())])) so the coordinator only groups the child thunks
(leave individual error handling to each thunk's extraReducers).
🧹 Nitpick comments (14)
backend/app/database/images.py (3)
532-905: Extract a shared row-to-dict helper to eliminate heavy duplication across all five memory query functions.The SELECT column list and the row-to-dict mapping are copy-pasted almost verbatim across
db_get_images_by_date_range,db_get_images_near_location,db_get_images_by_year_month,db_get_images_with_location, anddb_get_all_images_for_memories. A single_row_to_image_dict(row)helper would reduce ~150 lines of duplication and ensure consistency (e.g., the 0.0-latitude bug above would only need fixing in one place).♻️ Example helper
def _memory_row_to_dict(row) -> dict: from app.utils.images import image_util_parse_metadata return { "id": row[0], "path": row[1], "folder_id": str(row[2]) if row[2] else None, "thumbnailPath": row[3], "metadata": image_util_parse_metadata(row[4]), "isTagged": bool(row[5]), "isFavourite": bool(row[6]), "latitude": row[7], "longitude": row[8], "captured_at": row[9] if row[9] else None, "tags": row[10].split(",") if row[10] else None, }
81-91: Consider a composite index on(latitude, longitude)instead of two separate single-column indexes.The
db_get_images_near_locationquery filters on bothlatitude BETWEEN … AND longitude BETWEEN …. Two independent single-column indexes are less efficient for this range-range query than a single composite index. The individualix_images_latitudeandix_images_longitudeindexes add write overhead without meaningfully helping that query.♻️ Suggested change
- cursor.execute("CREATE INDEX IF NOT EXISTS ix_images_latitude ON images(latitude)") - cursor.execute( - "CREATE INDEX IF NOT EXISTS ix_images_longitude ON images(longitude)" - ) + cursor.execute( + "CREATE INDEX IF NOT EXISTS ix_images_lat_lon ON images(latitude, longitude)" + )Apply the same change in
db_migrate_add_memories_columns.
130-131: Movefrom app.utils.images import image_util_parse_metadatato the top of the file.This import is repeated inside
forloops in at least 7 different functions. While Python caches it after the first call, placing it at the module level is cleaner and avoids the visual noise. If there's a circular-import concern, a single lazy import at module scope or inside_memory_row_to_dict(if you extract the helper above) would suffice.Also applies to: 277-277, 348-348, 584-584, 678-678, 749-749, 816-816, 881-881
frontend/src/components/Memories/MemoryDetail.tsx (1)
77-89: Duplicate toggle-favorite logic — consider reusinguseToggleFavhook.
useToggleFav(infrontend/src/hooks/useToggleFav.ts) already wraps the sametogglefavAPI call +dispatch(updateImageFavoriteStatus(...))pattern, plus adds retry logic and cache invalidation viaautoInvalidateTags. Calling the API directly here loses those benefits and duplicates the concern.frontend/src/components/Memories/MemoriesPage.tsx (5)
61-77:SectionHeaderrendersdateovertitlewithout clear benefit.Line 69:
{date || title}means passing bothtitleanddate(e.g., lines 336-339 wheretitle="Past Year"anddate="Past Year") is redundant. Currentlydatealways duplicates or overridestitlewhen provided.
157-184:memoriesWithMultipleImagesis recomputed on every render for every section.Consider wrapping in
useMemoto avoid redundant filtering, especially as the memory list grows.
277-296: "On This Day" section conditionally renders based on data presence, but data + loading/error is checked in the wrong order.When
onThisDayImages.length > 0andonThisDayMetaare truthy (from a previous fetch), the section renders. But if a retry is in progress (loading.onThisDayis true), the skeleton shows over stale data — which is fine. However, on the very first load,onThisDayImagesis empty so this entire section is hidden, meaning the skeleton inside (line 281) is never reachable on initial load. TheFeaturedSkeletonat line 255 covers the global initial load, so this works but the per-section skeleton at line 281 only applies to retries.
301-329: Sections vanish entirely when the active filter produces zero results.When a user selects "Location" or "Date" filter and a section has no matching memories, the section disappears with no feedback. A per-section empty state (e.g., "No location-based memories in this period") would improve UX when filters are active.
Also applies to: 334-360, 365-391
161-166: Addtypefield to frontend Memory interface and use it for filtering instead of inferring from coordinates.The backend
Memorymodel (backend/app/routes/memories.py:65) includestype: Literal["location", "date"]and the API serializes this field. The frontendMemoryinterface (frontend/src/services/memoriesApi.ts:34-46) is missing this field, causing the code in MemoriesPage.tsx to duplicate the type-determination logic by checking coordinates instead. Addingtypeto the frontend interface and filtering on it directly will:
- Eliminate logic duplication
- Ensure frontend classification matches backend
- Reduce risk of divergence if coordinate-based filtering logic changes
frontend/src/components/Memories/MemoryCard.tsx (2)
26-66: HandlershandleImageErrorandhandleClickare recreated on every render insideReact.memo.Since
MemoryCardis wrapped inReact.memo, the outer component won't re-render unnecessarily. However, the inner handlers are still recreated each time the parent causes a render. WrappinghandleClickinuseCallbackwould be a small optimization.
92-135: Location pin SVG is duplicated — consider extracting a shared icon component.The same map-pin SVG paths appear in the type badge (lines 113-131) and the location row (lines 158-177). Extracting a small
<MapPinIcon />component would reduce duplication. Same applies to the calendar icon.Also applies to: 155-180
frontend/src/store/slices/memoriesSlice.ts (3)
89-101:generateMemoriesuses a POST request — calling it on every page load could be expensive.Per the API service (
memoriesApi.ts),generateMemoriesis a POST that presumably triggers server-side clustering. There's no caching or staleness check — every mount ofMemoriesPagedispatchesfetchAllMemoriesDatawhich re-generates all memories. Consider checkinglastFetchedbefore dispatching, or adding a cache TTL.
329-351: Selectors use inline{ memories: MemoriesState }instead of the store'sRootStatetype.This works but is fragile — if the slice key changes in the store configuration, these selectors silently break. Consider importing and using the centralized
RootStatetype.
106-135: Deduplication needed: Recent memories (30 days) are a subset of year memories (365 days), causing identical memory cards to appear in both sections.The backend's
get_timelineendpoint correctly filters by date range. When called withdays=30, it returns images from the past 30 days; when called withdays=365, it returns the past 365 days. Since the recent range is a subset of the year range, any memory from the past 30 days appears in bothrecentMemoriesandyearMemoriesstate arrays and gets rendered twice in the UI (lines 301–360 in MemoriesPage.tsx).Consider filtering year memories to exclude IDs already in recent memories, or implement deduplication in the selector layer.
|
@rahulharpal1603 i have added the ui changes requested :
3)the memories folder show only when the number of images are more than 2 4)the header of the memories was not like the home page corrected it 5)ran the pytest command for the backend 6)some smaller changes and code quality changes for the CodeRabbit review tell me any further improvements and changes. need to make |
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Auto-approval is disabled; enable |
|
@dehydrated-bear |
|
@rahulharpal1603 okay i stay on the system |
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
3 Tasks:
- Resolve all the comments.
- Remove any DB migration-related scripts. (Since the app is not yet available for users, we don't have to migrate)
- Use Tanstack Query to make API Calls. Refer one of the comments.
backend/extract_metadata_simple.py
Outdated
backend/main.py
Outdated
|
|
||
| # Only run migrations in the primary process or when explicitly enabled | ||
| should_run_migrations = os.getenv("RUN_MIGRATIONS", "true").lower() == "true" | ||
| if should_run_migrations: | ||
| try: | ||
| db_migrate_add_memories_columns() | ||
| logger.info("Database migrations completed successfully") | ||
| except Exception as e: | ||
| logger.error(f"Failed to run database migrations: {e}", exc_info=True) | ||
|
|
||
| else: | ||
| logger.info("Skipping migrations (RUN_MIGRATIONS not set or false)") |
There was a problem hiding this comment.
No need of migration logic because we have not rolled out the app to actual users yet.
backend/test_auto_gps_extraction.py
Outdated
backend/test_memories_api.py
Outdated
| // ============================================================================ | ||
| // Async Thunks | ||
| // ============================================================================ | ||
|
|
||
| /** | ||
| * Fetch all memories from photos with location data | ||
| */ | ||
| export const fetchAllMemories = createAsyncThunk< | ||
| Memory[], | ||
| void, | ||
| { rejectValue: string } | ||
| >('memories/fetchAll', async (_, { rejectWithValue }) => { | ||
| try { | ||
| const response = await generateMemories(); | ||
| return response.memories; | ||
| } catch (error) { | ||
| const apiError = error as ApiError; | ||
| return rejectWithValue(apiError.message); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * Fetch recent memories (last 30 days) | ||
| */ | ||
| export const fetchRecentMemories = createAsyncThunk< | ||
| Memory[], | ||
| number, | ||
| { rejectValue: string } | ||
| >('memories/fetchRecent', async (days = 30, { rejectWithValue }) => { | ||
| try { | ||
| const response = await getTimeline(days); | ||
| return response.memories; | ||
| } catch (error) { | ||
| const apiError = error as ApiError; | ||
| return rejectWithValue(apiError.message); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * Fetch memories from current year | ||
| */ | ||
| export const fetchYearMemories = createAsyncThunk< | ||
| Memory[], | ||
| number, | ||
| { rejectValue: string } | ||
| >('memories/fetchYear', async (days = 365, { rejectWithValue }) => { | ||
| try { | ||
| const response = await getTimeline(days); | ||
| return response.memories; | ||
| } catch (error) { | ||
| const apiError = error as ApiError; | ||
| return rejectWithValue(apiError.message); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * Fetch "On This Day" images | ||
| */ | ||
| export const fetchOnThisDay = createAsyncThunk< | ||
| { images: MemoryImage[]; today: string; years: number[] }, | ||
| void, | ||
| { rejectValue: string } | ||
| >('memories/fetchOnThisDay', async (_, { rejectWithValue }) => { | ||
| try { | ||
| const response = await getOnThisDay(); | ||
| return { | ||
| images: response.images, | ||
| today: response.today, | ||
| years: response.years, | ||
| }; | ||
| } catch (error) { | ||
| const apiError = error as ApiError; | ||
| return rejectWithValue(apiError.message); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * Fetch all memories data at once (parallel requests) | ||
| */ | ||
| export const fetchAllMemoriesData = createAsyncThunk< | ||
| void, | ||
| void, | ||
| { rejectValue: string } | ||
| >('memories/fetchAllData', async (_, { dispatch }) => { | ||
| await Promise.all([ | ||
| dispatch(fetchOnThisDay()), | ||
| dispatch(fetchRecentMemories(30)), | ||
| dispatch(fetchYearMemories(365)), | ||
| dispatch(fetchAllMemories()), | ||
| ]); | ||
| }); | ||
|
|
||
| // ============================================================================ | ||
| // Slice | ||
| // ============================================================================ | ||
|
|
||
| const memoriesSlice = createSlice({ | ||
| name: 'memories', | ||
| initialState, | ||
| reducers: { | ||
| /** | ||
| * Toggle favorite status of an image across all memories | ||
| */ | ||
| toggleImageFavorite: (state, action: PayloadAction<string>) => { | ||
| const imageId = action.payload; | ||
|
|
||
| // Helper function to update image in a memory array | ||
| const updateMemoriesArray = (memories: Memory[]) => { | ||
| memories.forEach((memory) => { | ||
| memory.images.forEach((image) => { | ||
| if (image.id === imageId) { | ||
| image.isFavourite = !image.isFavourite; | ||
| } | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| // Update across all memory collections | ||
| updateMemoriesArray(state.allMemories); | ||
| updateMemoriesArray(state.recentMemories); | ||
| updateMemoriesArray(state.yearMemories); | ||
|
|
||
| // Update onThisDay images | ||
| state.onThisDayImages.forEach((image) => { | ||
| if (image.id === imageId) { | ||
| image.isFavourite = !image.isFavourite; | ||
| } | ||
| }); | ||
| }, | ||
|
|
||
| /** | ||
| * Clear all errors | ||
| */ | ||
| clearErrors: (state) => { | ||
| state.error = { | ||
| all: null, | ||
| recent: null, | ||
| year: null, | ||
| onThisDay: null, | ||
| }; | ||
| }, | ||
|
|
||
| /** | ||
| * Reset memories state | ||
| */ | ||
| resetMemories: () => { | ||
| return initialState; | ||
| }, | ||
| }, | ||
| extraReducers: (builder) => { | ||
| // ======================================================================== | ||
| // Fetch All Memories | ||
| // ======================================================================== | ||
| builder | ||
| .addCase(fetchAllMemories.pending, (state) => { | ||
| state.loading.all = true; | ||
| state.error.all = null; | ||
| }) | ||
| .addCase(fetchAllMemories.fulfilled, (state, action) => { | ||
| state.loading.all = false; | ||
| state.allMemories = action.payload; | ||
| state.lastFetched = Date.now(); | ||
| }) | ||
| .addCase(fetchAllMemories.rejected, (state, action) => { | ||
| state.loading.all = false; | ||
| state.error.all = action.payload || 'Failed to fetch memories'; | ||
| }); | ||
|
|
||
| // ======================================================================== | ||
| // Fetch Recent Memories | ||
| // ======================================================================== | ||
| builder | ||
| .addCase(fetchRecentMemories.pending, (state) => { | ||
| state.loading.recent = true; | ||
| state.error.recent = null; | ||
| }) | ||
| .addCase(fetchRecentMemories.fulfilled, (state, action) => { | ||
| state.loading.recent = false; | ||
| state.recentMemories = action.payload; | ||
| }) | ||
| .addCase(fetchRecentMemories.rejected, (state, action) => { | ||
| state.loading.recent = false; | ||
| state.error.recent = | ||
| action.payload || 'Failed to fetch recent memories'; | ||
| }); | ||
|
|
||
| // ======================================================================== | ||
| // Fetch Year Memories | ||
| // ======================================================================== | ||
| builder | ||
| .addCase(fetchYearMemories.pending, (state) => { | ||
| state.loading.year = true; | ||
| state.error.year = null; | ||
| }) | ||
| .addCase(fetchYearMemories.fulfilled, (state, action) => { | ||
| state.loading.year = false; | ||
| state.yearMemories = action.payload; | ||
| }) | ||
| .addCase(fetchYearMemories.rejected, (state, action) => { | ||
| state.loading.year = false; | ||
| state.error.year = action.payload || 'Failed to fetch year memories'; | ||
| }); | ||
|
|
||
| // ======================================================================== | ||
| // Fetch On This Day | ||
| // ======================================================================== | ||
| builder | ||
| .addCase(fetchOnThisDay.pending, (state) => { | ||
| state.loading.onThisDay = true; | ||
| state.error.onThisDay = null; | ||
| }) | ||
| .addCase(fetchOnThisDay.fulfilled, (state, action) => { | ||
| state.loading.onThisDay = false; | ||
| state.onThisDayImages = action.payload.images; | ||
| state.onThisDayMeta = { | ||
| today: action.payload.today, | ||
| years: action.payload.years, | ||
| }; | ||
| }) | ||
| .addCase(fetchOnThisDay.rejected, (state, action) => { | ||
| state.loading.onThisDay = false; | ||
| state.error.onThisDay = action.payload || 'Failed to fetch On This Day'; | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| // ============================================================================ | ||
| // Exports | ||
| // ============================================================================ | ||
|
|
||
| export const { toggleImageFavorite, clearErrors, resetMemories } = | ||
| memoriesSlice.actions; | ||
|
|
||
| export default memoriesSlice.reducer; | ||
|
|
||
| // ============================================================================ | ||
| // Selectors | ||
| // ============================================================================ | ||
|
|
||
| export const selectAllMemories = (state: { memories: MemoriesState }) => | ||
| state.memories.allMemories; | ||
| export const selectRecentMemories = (state: { memories: MemoriesState }) => | ||
| state.memories.recentMemories; | ||
| export const selectYearMemories = (state: { memories: MemoriesState }) => | ||
| state.memories.yearMemories; | ||
| export const selectOnThisDayImages = (state: { memories: MemoriesState }) => | ||
| state.memories.onThisDayImages; | ||
| export const selectOnThisDayMeta = (state: { memories: MemoriesState }) => | ||
| state.memories.onThisDayMeta; | ||
| export const selectMemoriesLoading = (state: { memories: MemoriesState }) => | ||
| state.memories.loading; | ||
| export const selectMemoriesError = (state: { memories: MemoriesState }) => | ||
| state.memories.error; | ||
| export const selectLastFetched = (state: { memories: MemoriesState }) => | ||
| state.memories.lastFetched; | ||
|
|
||
| /** | ||
| * Select total memory count across all sections | ||
| */ | ||
| export const selectTotalMemoryCount = (state: { memories: MemoriesState }) => { | ||
| return state.memories.allMemories.length; | ||
| }; | ||
|
|
||
| /** | ||
| * Check if any section is loading | ||
| */ | ||
| export const selectIsAnyLoading = (state: { memories: MemoriesState }) => { | ||
| const { loading } = state.memories; | ||
| return loading.all || loading.recent || loading.year || loading.onThisDay; | ||
| }; | ||
|
|
||
| /** | ||
| * Check if there are any errors | ||
| */ | ||
| export const selectHasAnyError = (state: { memories: MemoriesState }) => { | ||
| const { error } = state.memories; | ||
| return !!(error.all || error.recent || error.year || error.onThisDay); | ||
| }; |
There was a problem hiding this comment.
We are not using async thunks to handle api calls. We are using tanstack query.
Refer to other files, on how to handle the api calls. Mainly, we use usePictoQuery and usePictoMutation which are based on tanstack.
You can refer this file too: frontend/src/hooks/useUserPreferences.tsx
Also to display error dialog, use this: frontend/src/components/Dialog/InfoDialog.tsx and nothing else.
|
@rahulharpal1603 addressed the review issues :
removed any migration code and corrected the main.py and replaced the thunk to tan query and refrenced the |
|
|
||
| return latitude, longitude, captured_at | ||
|
|
||
| def migrate_metadata(self) -> Dict[str, int]: |
There was a problem hiding this comment.
Remove this function
| extractor = MetadataExtractor() | ||
| stats = extractor.migrate_metadata() | ||
|
|
||
| # Exit with appropriate code | ||
| if stats["errors"] > 0: | ||
| logger.warning("Migration completed with errors") | ||
| exit(1) | ||
| else: | ||
| logger.info("✅ Migration completed successfully!") | ||
| exit(0) | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"❌ Migration failed: {e}") | ||
| exit(1) |
backend/requirements.txt
Outdated
| @@ -46,7 +46,7 @@ python-dotenv==1.0.1 | |||
| python-multipart==0.0.9 | |||
| PyYAML==6.0.1 | |||
| rich==13.7.1 | |||
| scikit-learn==1.5.1 | |||
| scikit-learn==1.5.1 # Required for Memories feature: DBSCAN spatial clustering algorithm | |||
There was a problem hiding this comment.
Remove comments from the requirements file.
frontend/src/services/memoriesApi.ts
Outdated
There was a problem hiding this comment.
API definitions are in the wrong place and do not follow the project conventions.
- API definitions and functions should be here:
frontend/src/api. So, remove memoriesApi.ts and write API calls. Refer to this file:/Users/rahul/Developer/PictoPy/frontend/src/api/api-functions/folders.ts.
There was a problem hiding this comment.
This file should be in this folder: frontend/src/features
|
Make sure that the response structure for the new API endpoints look like this: 
Refer the |
files changes : src/api/api-functions/memories.ts |
|
pretty sure i ran the linting code for frontend , maybe the 2 later files changes escaped the linting |
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
Thank you so much @dehydrated-bear and to your team!
Really good work with handling the reviews.
|
thank you @rahulharpal1603 for providing valuable reviews i really appreciate you taking the time , even after the pr was not particularly in best shape due to most of the initial code committed quickly due to the hackathon setting . would love to work on improving the memory tab as currently it has just some basic features , thinking of using the image tags to get data to provide more complex and better memories recommendation and work on solving bugs in it till its not battle tested Please let me know if this direction aligns with the project roadmap . I be happy to keep contributing and iterating on it 🙂 thank you for making the development and contribution experience smooth and welcoming |
Sure, I will thoroughly test the feature and suggest the improvements. You can work on those 😄 If you have any improvements in mind, open issues for this feature. I will review and assign |
2 participants
Feature: Complete Memories Implementation with Smart Clustering & UI Enhancements
solved the issuse #723
Overview
This PR implements a complete, production-ready Memories feature for PictoPy, similar to Google Photos' Memories. It automatically groups photos by location and date, creating meaningful albums that help users rediscover their photos.
Key Features
1. Smart Photo Clustering
2. "On This Day" Feature
3. Smart Titles & Formatting
4. Interactive UI
5. Full-Featured Image Viewer
Summary by CodeRabbit
Release Notes
New Features
Documentation