feat: Implement Memories feature (#723) #743
Conversation
- Add backend database models for memories with time and location grouping - Create API endpoints for generating and retrieving memories - Implement memory generation algorithm that groups photos by date and location - Add frontend Memories page with responsive card grid layout - Create interactive memory cards showing representative images - Update routing to include Memories page - Add unit tests for memories functionality Resolves AOSSIE-Org#723
github-actions
bot
added
backend
enhancement
WalkthroughAdds an end-to-end Memories feature: new SQLite-backed memories cache and generator, FastAPI routes and Pydantic schemas, startup integration and background prepopulation, tests and fixtures, frontend endpoints and API functions, a React Memories page, and routing updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Browser (Memories Page)
participant API as Backend API (/memories)
participant DB as SQLite DB
User->>Frontend: Open Memories page
Frontend->>Frontend: decide fetchAll or fetchToday
Frontend->>API: GET /memories or /memories/today
API->>DB: call db_generate_memories() or db_get_memories_for_current_date()
DB->>DB: read images, parse metadata, group by date/location, filter, persist/read memories
DB-->>API: return memory objects + representative image IDs
API->>API: map to Pydantic schemas
API-->>Frontend: return GetMemoriesResponse
Frontend->>Frontend: render MemoryCards (thumbnails + metadata)
Frontend-->>User: display memories UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
backend/app/database/memories.py (4)
21-59: Missing error handling for table creation.The
db_create_memories_tablefunction lacks a try/finally block for connection management, which is inconsistent with other functions in this file and could leave connections open on failure.def db_create_memories_table() -> None: """Create the memories table to cache generated memories.""" conn = _connect() cursor = conn.cursor() - - cursor.execute( - """ - CREATE TABLE IF NOT EXISTS memories ( - ... - ) - """ - ) - ... - conn.commit() - conn.close() + try: + cursor.execute( + """ + CREATE TABLE IF NOT EXISTS memories ( + ... + ) + """ + ) + ... + conn.commit() + finally: + conn.close()
84-86: Move import to top of file.The
from app.utils.images import image_util_parse_metadataimport is placed inside the function. While this may avoid circular imports, it's duplicated in multiple functions (lines 86 and 217). Consider restructuring to import at module level or use a lazy import pattern once.
98-106: Date parsing may fail silently for edge cases.The date parsing handles ISO format with "T" and space-separated format, but:
replace("Z", "+00:00")only handles UTC timezone indicator- Dates with other timezone offsets (e.g.,
+05:30) after "Z" replacement could fail- The broad
except Exceptionmasks specific parsing issuesConsider using a more robust date parsing approach.
- try: - if "T" in date_created: - date_obj = datetime.fromisoformat(date_created.replace("Z", "+00:00")) - else: - date_obj = datetime.strptime(date_created.split()[0], "%Y-%m-%d") - except Exception as e: - logger.warning(f"Could not parse date {date_created}: {e}") - continue + try: + date_str = date_created.replace("Z", "+00:00") + if "T" in date_str: + date_obj = datetime.fromisoformat(date_str) + else: + date_obj = datetime.strptime(date_str.split()[0], "%Y-%m-%d") + except (ValueError, TypeError) as e: + logger.warning(f"Could not parse date {date_created}: {e}") + continue
255-263: Title overwriting may cause inconsistent display.When a memory matches the "On This Day" criteria, the original title (which may include location information) is completely overwritten with just the time-based title. This loses the location context that was appended in
db_generate_memories.Consider preserving location info:
if years_ago > 0: - memory["title"] = f"On This Day {years_ago} Year{'s' if years_ago > 1 else ''} Ago" + location_suffix = f" • {memory['location']}" if memory.get('location') and memory['location'] != "Unknown Location" else "" + memory["title"] = f"On This Day {years_ago} Year{'s' if years_ago > 1 else ''} Ago{location_suffix}" relevant_memories.append(memory)backend/tests/test_memories.py (2)
1-6: Missing import fordb_get_memories_for_current_date.The function
db_get_memories_for_current_dateis defined inmemories.pybut not imported or tested here. Consider adding test coverage for this function.from app.database.memories import ( db_create_memories_table, db_generate_memories, db_get_memory_images, + db_get_memories_for_current_date, )
91-96: Fragile assertion for title content.The assertion checks for hardcoded years "2023", "2024", "2025" in titles. This will break in 2026 or fail for memories from other years.
# Title should contain year or time reference - assert any( - indicator in memory["title"].lower() - for indicator in ["year", "ago", "2023", "2024", "2025"] - ) + # Title should contain year indicator or a 4-digit year + import re + title_lower = memory["title"].lower() + has_time_ref = "year" in title_lower or "ago" in title_lower + has_year = bool(re.search(r'\b20\d{2}\b', memory["title"])) + assert has_time_ref or has_year, f"Title missing time reference: {memory['title']}"frontend/src/pages/Memories/Memories.tsx (3)
2-2: Remove unused dispatch.The
useDispatchimport anddispatchvariable are declared but never used in the component.Apply this diff to remove the unused code:
-import { useDispatch } from 'react-redux';const Memories = () => { - const dispatch = useDispatch(); const [memories, setMemories] = useState<Memory[]>([]);Also applies to: 218-218
45-45: Consider adding click handler for memory cards.The card has
cursor-pointerstyling but no click handler. Consider adding navigation to a memory detail view or expanding the memory inline for better user interaction.
49-143: Consider extracting image rendering logic.The image grid layouts have repeated patterns for error handling and path conversion. Consider extracting a reusable
MemoryImageThumbnailcomponent to reduce duplication and improve maintainability.Example helper component:
const MemoryImageThumbnail = ({ image, index, alt, imageLoadError, onError, className = "" }: { image: MemoryImage; index: number; alt: string; imageLoadError: Set<number>; onError: (idx: number) => void; className?: string; }) => ( <img src={ imageLoadError.has(index) ? '/placeholder.svg' : convertFileSrc(image.thumbnail || image.path) } alt={alt} className={cn("h-full w-full object-cover transition-transform duration-300 group-hover:scale-105", className)} onError={() => onError(index)} /> );backend/app/routes/memories.py (2)
38-63: Extract duplicate memory transformation logic.The code for converting database results to
MemoryandMemoryImagePydantic models is duplicated betweenget_all_memoriesandget_memories_for_today. This duplication makes the code harder to maintain and increases the risk of inconsistencies.Consider extracting a helper function:
def _transform_memories_to_response(memories_data: List[dict]) -> List[Memory]: """Transform database memory records to Memory response models.""" memories = [] for memory in memories_data: memory_images = [ MemoryImage( id=img["id"], path=img["path"], thumbnail=img["thumbnail"], date=img["date"].isoformat(), location=img.get("location") ) for img in memory.get("images", []) ] memories.append( Memory( id=memory["id"], title=memory["title"], description=memory["description"], start_date=memory["start_date"], end_date=memory["end_date"], location=memory.get("location"), latitude=memory.get("latitude"), longitude=memory.get("longitude"), image_count=memory["image_count"], images=memory_images ) ) return memoriesThen use it in both endpoints:
memories = _transform_memories_to_response(memories_data)Also applies to: 98-124
71-72: Consider using logger.exception for better debugging.Using
logger.exception()instead oflogger.error()automatically includes the full stack trace, making it easier to diagnose issues in production.Apply this change:
except Exception as e: - logger.error(f"Error in get_all_memories: {e}") + logger.exception("Error in get_all_memories") raise HTTPException(Similar changes for the other two endpoints.
Also applies to: 132-133, 180-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/app/database/memories.py(1 hunks)backend/app/routes/memories.py(1 hunks)backend/app/schemas/memories.py(1 hunks)backend/main.py(3 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/pages/Memories/Memories.tsx(1 hunks)frontend/src/routes/AppRoutes.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
backend/tests/test_memories.py (2)
backend/app/database/memories.py (3)
db_create_memories_table(21-59)db_generate_memories(62-194)db_get_memory_images(197-234)backend/tests/test_folders.py (1)
test_db(20-33)
frontend/src/routes/AppRoutes.tsx (1)
frontend/src/constants/routes.ts (1)
ROUTES(1-12)
backend/main.py (2)
backend/app/database/memories.py (1)
db_create_memories_table(21-59)backend/app/utils/microservice.py (1)
microservice_util_start_sync_service(41-71)
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(30-34)
backend/app/database/memories.py (1)
backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)
backend/app/routes/memories.py (2)
backend/app/database/memories.py (3)
db_generate_memories(62-194)db_get_memory_images(197-234)db_get_memories_for_current_date(237-271)backend/app/schemas/memories.py (5)
Memory(13-23)MemoryImage(5-10)GetMemoriesResponse(26-29)GetMemoryImagesResponse(32-35)ErrorResponse(38-41)
frontend/src/pages/Memories/Memories.tsx (6)
backend/app/schemas/memories.py (2)
MemoryImage(5-10)Memory(13-23)frontend/src/components/ui/badge.tsx (1)
Badge(46-46)frontend/src/components/ui/skeleton.tsx (1)
Skeleton(13-13)frontend/src/hooks/useQueryExtension.ts (1)
usePictoQuery(80-108)frontend/src/api/api-functions/memories.ts (1)
fetchAllMemories(5-10)frontend/src/hooks/useMutationFeedback.tsx (1)
useMutationFeedback(60-135)
🔇 Additional comments (6)
frontend/src/api/api-functions/index.ts (1)
7-7: LGTM!The new memories module export follows the established pattern for API function re-exports.
frontend/src/routes/AppRoutes.tsx (1)
12-12: LGTM!The Memories route is correctly integrated, replacing the placeholder with the actual component. The import style matches the default export from the Memories module.
Also applies to: 25-25
frontend/src/api/apiEndpoints.ts (1)
30-34: LGTM!The memories endpoints follow the established patterns in the codebase. The function-based
getMemoryImagesendpoint correctly mirrors the pattern used byfaceClustersEndpoints.getClusterImages. Backend routes confirm all three endpoints are properly implemented:
GET /memories/→get_all_memories()GET /memories/today→get_memories_for_today()GET /memories/{memory_id}/images→get_memory_detail()backend/main.py (1)
22-22: LGTM! Clean integration of memories feature.The memories module is properly integrated with correct import placement, table creation ordering (after images table for FK constraints), and router registration. The implementation follows existing patterns in the codebase.
Also applies to: 30-30, 57-57, 138-138
frontend/src/api/api-functions/memories.ts (1)
5-26: LGTM! Clean API function implementations.All three functions follow consistent patterns and properly leverage the existing
apiClientand endpoint definitions. The implementations are straightforward and correctly typed withAPIResponse.Note:
fetchTodayMemories(lines 12-17) doesn't appear to be used in the provided files. If it's intended for future use, consider adding a comment or tracking it as a planned feature.backend/app/schemas/memories.py (1)
5-41: LGTM! Clean schema definitions.The Pydantic models are well-structured and provide clear API contracts for the memories feature. The use of
Optionalfields is appropriate for the data model.Optional enhancement: Consider adding field validators to ensure data integrity (e.g.,
image_count >= 0, date format validation). However, since the data is sourced from a controlled database layer, this is not critical.
- Add database persistence for memories and memory_images tables - Fix memory-image association storage in db_generate_memories - Remove unused database connection in db_get_memories_for_current_date - Add defensive metadata handling in routes to prevent AttributeError - Add sample_images fixtures to conftest.py for shared test data - Remove unsafe type casting in Memories component with runtime validation - Add proper error handling and data validation throughout
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/conftest.py (1)
17-49: DB isolation is missing: tests currently use the production databaseapp/database/PictoPy.db.Setting
TEST_MODE=truedoes not redirectDATABASE_PATH. Thesetup_before_all_testsfixture creates tables directly in your development database. Seetest_folders.pyfor the correct pattern: overrideapp.config.settings.DATABASE_PATHto a temporary file at session setup.Additionally, the
sample_imagesandsample_images_with_locationfixtures have a schema mismatch: they insert into columnpath, but the actualfolderstable schema definesfolder_path(line 25 offolders.py). These fixtures will fail immediately.
🧹 Nitpick comments (6)
frontend/src/pages/Memories/Memories.tsx (3)
13-32: Consider extracting interfaces to a shared types file.The
MemoryImageandMemoryinterfaces are defined locally in this component. For better maintainability and type consistency across the application, consider moving these to a shared types file (e.g.,@/types/memories.tsor@/api/types.ts). This ensures consistency with backend schemas and allows reuse across other components.
49-143: Consider refactoring duplicated image rendering logic.The four conditional grid layouts (1, 2, 3, and 4+ images) contain similar image rendering code with only layout differences. Consider extracting a reusable
MemoryImagesub-component or a render helper function to reduce duplication and improve maintainability.Example refactor:
const MemoryImage = ({ image, index, alt, imageLoadError, onError }: { image: MemoryImage; index: number; alt: string; imageLoadError: Set<number>; onError: (index: number) => void; }) => ( <img src={ imageLoadError.has(index) ? '/placeholder.svg' : convertFileSrc(image.thumbnail || image.path) } alt={alt} className="h-full w-full object-cover transition-transform duration-300 group-hover:scale-105" onError={() => onError(index)} /> );
236-256: Improve type safety with a type guard function.While the runtime validation is a good improvement over the previous unsafe type assertion, the code still uses
anytype (line 243) and a type assertion (line 251). Consider defining a proper TypeScript type guard for cleaner, type-safe validation.Apply this refactor:
+// Define type guard functions +const isMemoryImage = (obj: unknown): obj is MemoryImage => { + return ( + typeof obj === 'object' && + obj !== null && + 'id' in obj && + typeof (obj as any).id === 'string' && + 'path' in obj && + typeof (obj as any).path === 'string' + ); +}; + +const isMemory = (obj: unknown): obj is Memory => { + return ( + typeof obj === 'object' && + obj !== null && + 'id' in obj && + typeof (obj as any).id === 'string' && + 'title' in obj && + typeof (obj as any).title === 'string' && + 'images' in obj && + Array.isArray((obj as any).images) && + (obj as any).images.every(isMemoryImage) + ); +}; + const Memories = () => { - const [memories, setMemories] = useState<Memory[]>([]); - const { data, isLoading, isSuccess, isError, error } = usePictoQuery({ queryKey: ['memories'], queryFn: () => fetchAllMemories(), }); - useEffect(() => { - if (isSuccess && data?.data) { - // Validate data structure before setting state - const memoriesData = data.data; - - if (Array.isArray(memoriesData)) { - // Filter and validate each memory object - const validMemories = memoriesData.filter((item: any) => { - return ( - item && - typeof item === 'object' && - typeof item.id === 'string' && - typeof item.title === 'string' && - Array.isArray(item.images) - ); - }) as Memory[]; - - setMemories(validMemories); - } - } - }, [data, isSuccess]); + // Use query data directly with validation + const memories = useMemo(() => { + if (!data?.data || !Array.isArray(data.data)) return []; + return data.data.filter(isMemory); + }, [data]);This approach:
- Eliminates
anytypes and unsafe assertions- Removes unnecessary local state
- Uses
useMemofor derived data (more idiomatic React)- TypeScript can properly infer types through the type guard
backend/app/database/memories.py (1)
15-18: Add a busy timeout (and consider WAL) to reduce “database is locked” failures under concurrent requests.SQLite writes here (DELETE+INSERT) can collide with other readers/writers. At minimum, set a timeout; optionally enable WAL.
def _connect() -> sqlite3.Connection: - conn = sqlite3.connect(DATABASE_PATH) + conn = sqlite3.connect(DATABASE_PATH, timeout=5) conn.execute("PRAGMA foreign_keys = ON") + # Optional: better concurrency for read-heavy workloads + # conn.execute("PRAGMA journal_mode = WAL") return connbackend/tests/conftest.py (2)
61-108: Avoid redundant sqlite connections/commits arounddb_bulk_insert_images()(it uses its own transaction).Both fixtures open
sqlite3.connect(DATABASE_PATH), then calldb_bulk_insert_images(sample_data)(which opens/commits separately), thenconn.commit()—which doesn’t affect the bulk insert. Suggest either:
- do everything via one connection (requires adding a “conn” parameter to
db_bulk_insert_images), or- remove the extra
conn/commitand only keep a connection for the folder insert.Also applies to: 123-179
109-118: Consider cleaning upmemories/memory_imagestoo (cross-test interference).These fixtures delete only
imagesandfolders. If any test generates memories, cache tables can persist across tests and affect expectations. Add cleanup formemoriesandmemory_images(or make a per-test DB).Also applies to: 180-188
📜 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 (4)
backend/app/database/memories.py(1 hunks)backend/app/routes/memories.py(1 hunks)backend/tests/conftest.py(3 hunks)frontend/src/pages/Memories/Memories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/routes/memories.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/tests/conftest.py (7)
backend/app/database/faces.py (1)
db_create_faces_table(29-52)backend/app/database/images.py (2)
db_create_images_table(52-86)db_bulk_insert_images(89-120)backend/app/database/yolo_mapping.py (1)
db_create_YOLO_classes_table(6-35)backend/app/database/albums.py (2)
db_create_albums_table(7-26)db_create_album_images_table(29-48)backend/app/database/folders.py (1)
db_create_folders_table(15-36)backend/app/database/metadata.py (1)
db_create_metadata_table(8-30)backend/app/database/memories.py (1)
db_create_memories_table(21-59)
backend/app/database/memories.py (1)
backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)
🔇 Additional comments (2)
frontend/src/pages/Memories/Memories.tsx (2)
188-215: LGTM!The skeleton and empty state components are well-implemented with appropriate styling and messaging.
273-287: LGTM!The conditional rendering logic with loading skeletons, populated grid, and empty state is well-structured and provides good UX.
- Add json_valid() guard to ORDER BY queries to prevent malformed JSON errors - Convert datetime objects to ISO strings for JSON serialization - Make cache refresh atomic with BEGIN IMMEDIATE transaction - Optimize db_get_memories_for_current_date to query persisted table - Fix column name from 'path' to 'folder_path' in test fixtures - Remove unused Redux dispatch and cn imports from Memories.tsx - Remove cursor-pointer class from Card without onClick handler - Add placeholder.svg for image fallback handling Addresses feedback from PR AOSSIE-Org#743
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
frontend/src/pages/Memories/Memories.tsx (2)
233-253: Removeany+as Memory[]by turning the filter into a type guard (and clear state on invalid data).
You’re already validating; you can let TS infer safely withoutas Memory[](Line 248) and avoid leaving stalememorieswhen the payload isn’t an array.useEffect(() => { if (isSuccess && data?.data) { // Validate data structure before setting state const memoriesData = data.data; - - if (Array.isArray(memoriesData)) { - // Filter and validate each memory object - const validMemories = memoriesData.filter((item: any) => { - return ( - item && - typeof item === 'object' && - typeof item.id === 'string' && - typeof item.title === 'string' && - Array.isArray(item.images) - ); - }) as Memory[]; - - setMemories(validMemories); - } + + if (!Array.isArray(memoriesData)) { + setMemories([]); + return; + } + + const validMemories = memoriesData.filter((item: unknown): item is Memory => { + if (!item || typeof item !== 'object') return false; + const m = item as Partial<Memory>; + return typeof m.id === 'string' && typeof m.title === 'string' && Array.isArray(m.images); + }); + + setMemories(validMemories); } }, [data, isSuccess]);
47-141:/placeholder.svgfallback likely 404s; use a guaranteed asset or inline fallback.
This file still hardcodes'/placeholder.svg'in several places (e.g., Lines 52, 69, 87, 100, 118, 131). If the asset isn’t present in the app’s public bundle, failed image loads will cascade into more failures.
🧹 Nitpick comments (3)
frontend/src/pages/Memories/Memories.tsx (1)
166-172: Date label ignoresend_date; multi-day memories will look wrong.
If memories can span a range (you havestart_date+end_date), consider rendering a range when they differ.backend/app/database/memories.py (2)
21-56: Consider indexes / schema tweaks fordb_get_memories_for_current_datequery.
strftime(...)+ABS(...)will force scans as the table grows; also the “±7 day” logic won’t include cross-month edges (e.g., Jan 2 won’t match Dec 28).Options:
- Store
start_month/start_dayas INTEGER columns (or generated columns) and index them.- Or query by a date range using SQLite date functions (and handle month boundaries correctly).
- Add indexes like
CREATE INDEX IF NOT EXISTS idx_memories_start_date ON memories(start_date);andCREATE INDEX IF NOT EXISTS idx_memory_images_memory_rep ON memory_images(memory_id, is_representative);.Also applies to: 309-321
390-402: Title may be misleading for a ±7-day window.
You always label items “On This Day …” even when they’re “near this day”. Consider keeping the persisted title, or changing the label when not an exact month/day match.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/public/placeholder.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
backend/app/database/memories.py(1 hunks)backend/tests/conftest.py(3 hunks)frontend/src/pages/Memories/Memories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/database/memories.py (1)
backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)
- Add deterministic tie-breaker (id) to all ORDER BY queries for stable sorting - Normalize datetime handling to date-only to avoid timezone-aware/naive TypeError - Change exception handling to best-effort approach (log but don't fail on persistence errors) - Make thumbnail optional in MemoryImage interface to match actual usage These changes ensure: - Stable, deterministic query results even with NULL metadata - No timezone-related errors when comparing dates - Graceful degradation if persistence fails (still returns in-memory data) - TypeScript type safety matches runtime behavior
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/pages/Memories/Memories.tsx (1)
51-52: Missing/placeholder.svgasset causes 404 errors on image load failure.The fallback path
/placeholder.svgis used throughout the image error handling but doesn't exist in the public directory. This will result in broken images when the primary image fails to load.Consider using a data URL or inline SVG fallback, or add the missing asset to
frontend/public/:+ // At the top of the file, define a fallback + const PLACEHOLDER_IMAGE = 'data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100"><rect fill="%23374151" width="100" height="100"/><text x="50" y="55" font-size="14" fill="%239CA3AF" text-anchor="middle">No Image</text></svg>'; // Then use it in place of '/placeholder.svg' - ? '/placeholder.svg' + ? PLACEHOLDER_IMAGEAlso applies to: 68-70, 86-88, 99-101, 117-119, 130-132
🧹 Nitpick comments (5)
frontend/src/pages/Memories/Memories.tsx (2)
112-141: Grid layout for 4+ images may not display correctly.When
displayImages.length >= 4, the grid usesgrid-cols-3with the first image spanningcol-span-2 row-span-2. However, the container for the first image lacks explicit height, which may cause layout issues depending on the aspect ratios of subsequent images.Consider adding explicit aspect ratio to ensure consistent layout:
{displayImages.length >= 4 && ( - <div className="grid grid-cols-3 gap-1"> + <div className="grid grid-cols-3 grid-rows-2 gap-1"> <div className="col-span-2 row-span-2 overflow-hidden"> + <div className="aspect-square"> <img ... /> + </div> </div>
218-221: Remove redundant arrow function wrapper in queryFn.The arrow function wrapper is unnecessary since
fetchAllMemoriesalready returns a promise.const { data, isLoading, isSuccess, isError, error } = usePictoQuery({ queryKey: ['memories'], - queryFn: () => fetchAllMemories(), + queryFn: fetchAllMemories, });backend/app/database/memories.py (3)
88-89: Import inside function adds overhead on each call.The
image_util_parse_metadataimport is placed inside the function body. While this avoids circular imports, it adds import overhead on every invocation ofdb_generate_memories().If there's no circular dependency concern, move to top-level imports:
# App-specific imports from app.config.settings import DATABASE_PATH from app.logging.setup_logging import get_logger +from app.utils.images import image_util_parse_metadataThen remove the local imports at lines 89 and 277.
372-372: Repeated import inside loop iteration.The
from app.utils.images import image_util_parse_metadataappears insidedb_get_memories_for_current_dateat line 372, which is inside the row iteration loop's scope. While Python caches imports, this is still suboptimal placement.Move to top-level or at least before the loop:
+ from app.utils.images import image_util_parse_metadata + for row in rows: ... - from app.utils.images import image_util_parse_metadata
141-143: Minimum 3 images threshold is reasonable but not configurable.The hardcoded threshold of 3 images may need adjustment based on user feedback. Consider extracting to a constant.
+MIN_IMAGES_FOR_MEMORY = 3 + def db_generate_memories() -> List[Dict[str, Any]]: ... - if len(images_in_group) < 3: # Skip groups with too few images + if len(images_in_group) < MIN_IMAGES_FOR_MEMORY: continue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/database/memories.py(1 hunks)frontend/src/pages/Memories/Memories.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/database/memories.py (1)
backend/app/utils/images.py (1)
image_util_parse_metadata(496-513)
🔇 Additional comments (8)
frontend/src/pages/Memories/Memories.tsx (3)
32-37: Consider using aMapinstead ofSetfor better type safety with index tracking.The
imageLoadErrorstate uses aSet<number>which works, but creates a new Set on each error. This is fine for small image counts (max 5), so no functional issue.
186-200: Skeleton component looks good.Clean implementation of the loading skeleton that matches the card layout structure.
233-253: Data validation logic is well-implemented.The validation now properly checks the array structure and validates each memory object before setting state, addressing the previous unsafe type assertion concern.
backend/app/database/memories.py (5)
15-18: Connection helper is clean and enables foreign keys correctly.
21-59: Table creation logic is correct.The schema properly defines the memories table with appropriate columns and a junction table for many-to-many relationships with ON DELETE CASCADE.
254-294:db_get_memory_imagesimplementation is correct and clean.Good use of parameterized queries, proper error handling, and the ORDER BY includes tie-breaker as previously suggested.
240-243: Best-effort persistence approach is reasonable.The decision to log errors but not re-raise during persistence failure allows the in-memory data to still be returned, which is a sensible tradeoff. The comment clearly documents this choice.
199-204: No issue with transaction handling. TheBEGIN IMMEDIATEat line 199 is correctly placed at the start of a fresh transaction. The connection is created at line 67 withconn = _connect(), and only read-only SELECT operations occur before the explicit transaction start. The prior call todb_create_memories_table()uses its own separate connection, so there is no coupling or conflict with auto-commit behavior.
- Add module-level threading lock to prevent concurrent cache regenerations - Implement background thread for async memory cache regeneration - Add db_prepopulate_memories_cache() for startup cache initialization - Fix month-boundary edge case using Julian day comparison (e.g., Dec 30 -> Jan 5) - Return immediately on cache miss instead of blocking request handlers - Add db_check_memories_cache_exists() helper function Benefits: - Non-blocking request handlers (no synchronous regeneration on cache miss) - Single-regeneration guard prevents multiple concurrent regenerations - Startup pre-population option prevents initial cache misses - Cross-month matching works correctly at month boundaries - Better performance and user experience during cache operations
- Move test_db fixture from test_folders.py to conftest.py (proper implementation) - Remove duplicate test_db fixture from test_folders.py - Fix test_get_memory_images to insert real test data before assertions - Add proper cleanup in test_get_memory_images - Replace conditional assertions with actual assertions that always run - Test now properly verifies image retrieval with required fields Benefits: - Shared test_db fixture across all test modules (pytest auto-discovery) - No conditional test logic that might not execute - Proper test isolation with cleanup - Tests actually verify behavior instead of skipping assertions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/tests/test_memories.py (1)
150-165: Consider strengthening location grouping validation.The test verifies that location fields exist but doesn't validate that images are actually grouped by location. Since
sample_images_with_locationcreates 4 images at each of 3 locations, you could assert that memories are created per location and that images within each memory share coordinates.Example strengthening:
def test_memory_grouping_by_location(test_db, sample_images_with_location): """Test that memories are grouped by location.""" db_create_memories_table() memories = db_generate_memories() # With 3 distinct locations × 4 images each, expect memories grouped by location assert len(memories) >= 3, "Should create memories for distinct locations" for memory in memories: if memory.get("location") and memory.get("latitude"): # Verify all images in this memory share the same location lat = memory["latitude"] lon = memory["longitude"] images = db_get_memory_images(memory["id"]) for img in images: img_lat = img["metadata"].get("latitude") img_lon = img["metadata"].get("longitude") if img_lat and img_lon: assert abs(img_lat - lat) < 0.01, "Images should share location" assert abs(img_lon - lon) < 0.01, "Images should share location"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/tests/conftest.py(3 hunks)backend/tests/test_folders.py(0 hunks)backend/tests/test_memories.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/tests/test_folders.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/test_memories.py (2)
backend/app/database/memories.py (3)
db_create_memories_table(26-64)db_generate_memories(67-256)db_get_memory_images(259-299)backend/tests/conftest.py (3)
test_db(53-66)sample_images(70-128)sample_images_with_location(132-199)
🔇 Additional comments (5)
backend/tests/test_memories.py (5)
1-6: LGTM!Import statements are clean and appropriate for the test module.
9-30: LGTM!The test correctly verifies that both
memoriesandmemory_imagestables are created by queryingsqlite_master.
33-38: LGTM!The test correctly validates that
db_generate_memories()returns an empty list when no images are present.
60-129: LGTM!The test now properly validates
db_get_memory_images()by directly inserting test data and asserting on concrete results. The fix addresses the previous review concern about conditional assertions. Good cleanup logic ensures test isolation.
168-177: LGTM!The test correctly validates that
image_countrepresents the total images in a memory group while theimagesarray contains at most 5 representatives.
- Remove 'if len(memories) > 0' conditional logic from test_generate_memories_with_data - Remove 'if len(memories) > 0' conditional logic from test_memory_title_generation - Add explicit assertions that memories were generated before field checks - Add clear assertion messages for better debugging - Tests now fail properly when no memories are generated instead of passing silently Changes: - test_generate_memories_with_data: Assert len(memories) > 0 before accessing fields - test_memory_title_generation: Assert len(memories) > 0 before iterating - Both tests now have descriptive error messages when assertions fail Benefits: - Tests actually verify behavior instead of conditionally skipping - Failures are more informative with clear messages - No silent passes when functionality is broken - Better test reliability and coverage
…ick-to-view functionality, fetch all images, uniform card sizes
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/tests/test_memories.py (1)
156-171: Strengthen location grouping validation.The test collects unique locations but doesn't verify that images are actually grouped by location. It only checks that
locationis not None when present. Consider asserting that:
- Multiple distinct locations result in separate memories
- Images within each memory share the same/nearby location coordinates
- The grouping algorithm correctly separates images by location thresholds
Example enhancement:
def test_memory_grouping_by_location(test_db, sample_images_with_location): """Test that memories are grouped by location.""" db_create_memories_table() memories = db_generate_memories() - # If we have multiple locations, they should be in separate memories - locations = set() - for memory in memories: - if memory.get("location"): - locations.add(memory["location"]) - - # Each memory should have a consistent location - for memory in memories: - if memory.get("location") and len(memory["images"]) > 0: - # All images in this memory should have similar locations - assert memory["location"] is not None + if len(memories) > 1: + # Verify that memories with different locations are separate + locations = [m.get("location") for m in memories if m.get("location")] + assert len(locations) == len(set(locations)), "Each memory should have unique location" + + # Verify images within each memory share similar coordinates + for memory in memories: + if memory.get("latitude") and len(memory["images"]) > 1: + # Check coordinate consistency within memory + lat, lon = memory["latitude"], memory["longitude"] + assert lat is not None and lon is not Nonefrontend/src/pages/Memories/Memories.tsx (2)
342-366: Add keyboard navigation for modal accessibility.The fullscreen image modal can only be closed by clicking the X button or backdrop. Add ESC key handling and consider focus management for better accessibility.
Apply this enhancement:
+ // Add keyboard handler for ESC key + useEffect(() => { + if (selectedImageIndex === null) return; + + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === 'Escape') { + setSelectedImageIndex(null); + } + }; + + window.addEventListener('keydown', handleKeyDown); + return () => window.removeEventListener('keydown', handleKeyDown); + }, [selectedImageIndex]); + return ( // ... component JSXAlso consider adding
role="dialog"andaria-modal="true"to the modal container for screen reader support.
230-256: Consider validating date and improving type safety in image mapping.The mapping at line 241 allows empty strings for dates (
img.metadata?.date_created || ''), which may cause issues in date formatting or sorting. Also, theanytype at line 237 bypasses type checking.Consider adding runtime validation:
const loadAllImages = async () => { setIsLoadingImages(true); try { const response = await fetchMemoryImages(memory.id); if (response.success && response.data && Array.isArray(response.data)) { - // Map the response to MemoryImage format - const images: MemoryImage[] = response.data.map((img: any) => ({ + const images: MemoryImage[] = response.data + .filter((img: any) => img.id && img.path) // Validate required fields + .map((img: any) => ({ id: img.id, path: img.path, thumbnail: img.thumbnail, - date: img.metadata?.date_created || '', + date: img.metadata?.date_created || new Date().toISOString(), location: img.metadata?.location, })); setAllImages(images); }Alternatively, define a proper type for the API response instead of using
any.docs/backend/backend_python/openapi.json (1)
1120-1128: Consider simplifyingallOfwrapper for single reference.The
InputTypeparameter usesallOfwith a single$refto add a title. While valid in OpenAPI 3.1, this can be simplified since OpenAPI 3.1 allows$refsiblings.If desired, simplify to:
"schema": { - "allOf": [ - { - "$ref": "#/components/schemas/InputType" - } - ], + "$ref": "#/components/schemas/InputType", "description": "Choose input type: 'path' or 'base64'", "default": "path", "title": "Input Type" }However, the current structure is not incorrect—just more verbose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/routes/memories.py(1 hunks)backend/tests/test_memories.py(1 hunks)docs/backend/backend_python/openapi.json(5 hunks)frontend/src/pages/Memories/Memories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/routes/memories.py
| "MemoryImage": { | ||
| "properties": { | ||
| "id": { | ||
| "type": "string", | ||
| "title": "Id" | ||
| }, | ||
| "path": { | ||
| "type": "string", | ||
| "title": "Path" | ||
| }, | ||
| "thumbnail": { | ||
| "type": "string", | ||
| "title": "Thumbnail" | ||
| }, | ||
| "date": { | ||
| "type": "string", | ||
| "title": "Date" | ||
| }, | ||
| "location": { | ||
| "anyOf": [ | ||
| { | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ], | ||
| "title": "Location" | ||
| } | ||
| }, | ||
| "type": "object", | ||
| "required": [ | ||
| "id", | ||
| "path", | ||
| "thumbnail", | ||
| "date" | ||
| ], | ||
| "title": "MemoryImage" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the openapi.json file at the specified lines
wc -l docs/backend/backend_python/openapi.jsonRepository: AOSSIE-Org/PictoPy
Length of output: 107
🏁 Script executed:
# Read the MemoryImage schema section
sed -n '2517,2555p' docs/backend/backend_python/openapi.jsonRepository: AOSSIE-Org/PictoPy
Length of output: 897
🏁 Script executed:
# Find the frontend Memories.tsx file
fd -i memories.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 102
🏁 Script executed:
# Search for backend MemoryImage schema definition
fd -E node_modules -type f -name "*.py" | xargs rg -l "MemoryImage" | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 470
🏁 Script executed:
# Read frontend Memories.tsx to find the TypeScript interface (around line 15)
head -50 frontend/src/pages/Memories/Memories.tsx | cat -nRepository: AOSSIE-Org/PictoPy
Length of output: 2049
🏁 Script executed:
# Read backend schema definition for MemoryImage
cat backend/app/schemas/memories.pyRepository: AOSSIE-Org/PictoPy
Length of output: 815
🏁 Script executed:
# Check how MemoryImage is instantiated in backend
cat backend/app/routes/memories.pyRepository: AOSSIE-Org/PictoPy
Length of output: 6267
🏁 Script executed:
# Check if frontend has defensive fallback for thumbnail as claimed
rg -n "thumbnail.*\|\|.*path|path.*fallback" frontend/src/pages/Memories/Memories.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 628
🏁 Script executed:
# Also check the image rendering parts more broadly
sed -n '50,150p' frontend/src/pages/Memories/Memories.tsx | cat -nRepository: AOSSIE-Org/PictoPy
Length of output: 5122
Schema mismatch: MemoryImage.thumbnail required vs optional.
The OpenAPI schema declares thumbnail as required (lines 2527-2530, 2548-2553), and the backend Pydantic schema also requires it (thumbnail: str). However, the frontend TypeScript interface declares it as optional (thumbnail?: string in Memories.tsx line 15). Throughout the frontend, the code defensively handles missing thumbnails with img.thumbnail || img.path fallbacks at 7 locations (lines 56, 72, 90, 103, 121, 134, 330 in Memories.tsx), and the backend uses .get("thumbnail", "") which provides an empty string fallback.
This reveals a contract violation:
- OpenAPI and Pydantic schemas promise
thumbnailis required - Frontend and backend defensive code suggest it can be missing or falsy
- If thumbnail is always present, the defensive coding is unnecessary
- If thumbnail can be absent, the schemas should mark it as optional
Align the OpenAPI schema, Pydantic model, and actual backend implementation: either ensure thumbnail is always returned as a non-empty value, or make it optional in all three places.
🤖 Prompt for AI Agents
In docs/backend/backend_python/openapi.json around lines 2517 to 2555, the
MemoryImage schema wrongly marks "thumbnail" as required; remove "thumbnail"
from the "required" array and change its property to allow null (e.g., make it
anyOf [{"type":"string"},{"type":"null"}]) so the OpenAPI matches runtime. Also
update the backend Pydantic model to declare thumbnail as Optional[str] (with a
default, e.g., "" or None) and ensure the serialization consistently returns
.get("thumbnail", "") (or an explicit fallback) so frontend consumers keep their
defensive img.thumbnail || img.path handling.
|
I am sorry, but this is not the outcome we were expecting. You have just implemented month-wise grouping, from what it looks like on the video. |
3 participants
Resolves #723
Screen.Recording.2025-12-14.at.1.01.50.AM_compressed.mov
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.