Feat: Scrollbar Timeline with {Month-Year} Markers for Home page and AI Tagging page.#504
Feat: Scrollbar Timeline with {Month-Year} Markers for Home page and AI Tagging page.#504rohan-pandeyy wants to merge 11 commits intoAOSSIE-Org:mainfrom
Conversation
Refactored image metadata to use dictionaries instead of strings, added automatic extraction of image metadata using Pillow, and ensured metadata is stored as JSON in the database.
WalkthroughAdds backend image metadata extraction and normalization (store JSON, expose objects) and frontend chronological gallery with a timeline scrollbar, related utilities/types, layout/CSS tweaks, and a Jest ResizeObserver polyfill. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Routes as Backend Routes
participant DB as DB Layer
participant FS as Filesystem
participant PIL as PIL (Pillow)
Client->>Routes: db_bulk_insert_images(records)
Routes->>DB: open connection via _connect()
loop per record
DB->>FS: stat file (size/mtime)
DB->>PIL: open image for width/height/EXIF
DB-->>Routes: metadata dict (filled/fallbacks)
end
Routes->>DB: INSERT ... ON CONFLICT(path) DO UPDATE (metadata JSON)
DB-->>Client: insert result
Client->>Routes: db_get_all_images()
Routes->>DB: query rows via _connect()
DB-->>Routes: rows with metadata JSON string
Routes->>Routes: parse metadata JSON -> dict, dedupe tags, sort
Routes-->>Client: images with metadata object
sequenceDiagram
autonumber
actor User
participant Page as Home/AITagging
participant Gallery as ChronologicalGallery
participant TL as TimelineScrollbar
participant Utils as dateUtils/timelineUtils
Page->>Gallery: render(images, scrollContainerRef)
Gallery->>Utils: groupImagesByYearMonthFromMetadata(images)
Gallery-->>Page: onMonthOffsetsChange(markers)
Page->>TL: render(scrollableRef, monthMarkers)
User->>TL: hover / drag / click
TL->>Utils: useScroll/useWheel/getMarkerForScrollPosition()
TL->>Page: set scrollTop on scrollableRef (smooth on marker click)
Page->>Gallery: scroll position updates (visual sync)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Introduces ChronologicalGallery for displaying images grouped by year and month using metadata. Refactors Home and AI Tagging pages to use the new gallery component. Updates Image type to include structured metadata and adds dateUtils helpers for grouping and indexing images.
Simplifies Gallery by removing two scroll areas - inner and outter. Updates usage in Home and AITagging pages to match new props and layout, including gallery section styling and title changes.
No more double scroll !Improved Gallery layout by removing inner/outer scroll area conflict. |
Introduces TimelineScrollbar which marks and shows where a new month begins, on the scrollbar itself. Adds a new timelineUtils with the states, hooks and helpers for scroll behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/backend/backend_python/openapi.json (1)
1933-1942: Inconsistent metadata type across endpoints (object vs string).
ImageInCluster.metadataremainsstring|null, whileImageData.metadatais nowobject. This inconsistency will force conditional handling in clients and can cause subtle bugs.Apply this schema adjustment for consistency:
- "metadata": { - "anyOf": [ - { "type": "string" }, - { "type": "null" } - ], - "title": "Metadata" - }, + "metadata": { + "anyOf": [ + { "type": "object" }, + { "type": "null" } + ], + "title": "Metadata" + },Also bump docs/changelog to call out this breaking change.
frontend/src/components/Media/MediaInfoPanel.tsx (1)
27-33: Date should come from image metadata, not “now”.Using the current date is misleading in an info panel. Prefer
currentImage?.metadata?.date_createdwith a safe fallback.- const getFormattedDate = () => { - return new Date().toLocaleDateString('en-US', { + const getFormattedDate = () => { + const iso = currentImage?.metadata && (currentImage.metadata as any).date_created; + const d = iso ? new Date(iso) : null; + return (d ?? new Date(0)).toLocaleDateString('en-US', { year: 'numeric', month: 'long', day: 'numeric', }); };backend/app/database/images.py (1)
259-296: Return parsed metadata here too for consistency with the API.
db_get_untagged_imagescurrently returns rawmetadata(likely a JSON string), diverging fromdb_get_all_imagesand the OpenAPI. Parse todictbefore returning.- for image_id, path, folder_id, thumbnail_path, metadata in results: + for image_id, path, folder_id, thumbnail_path, metadata in results: + md: dict = {} + if metadata: + try: + md = json.loads(metadata) if isinstance(metadata, str) else (metadata or {}) + except Exception: + md = {} untagged_images.append( { "id": image_id, "path": path, "folder_id": folder_id, "thumbnailPath": thumbnail_path, - "metadata": metadata, + "metadata": md, } )
🧹 Nitpick comments (20)
frontend/src/App.css (2)
199-207: Scrollbar hiding: OK, but watch a11y discoverability.The utility is fine. Ensure the timeline remains keyboard-accessible (arrow/PageUp/PageDown/Home/End) wherever
.hide-scrollbaris applied, since hidden scrollbars can impair discoverability of scrollable regions.
209-214: Consider adding touch-action for smoother drag.For drag handles/tracks that use
.no-select, consider also applyingtouch-action: noneon the draggable element (not globally) to prevent browser scroll interference during drag.+.no-select.touch-none { + touch-action: none; +}frontend/jest.setup.ts (1)
11-18: Strengthen ResizeObserver mock to accept a callback (constructor).Some components instantiate
new ResizeObserver(cb)and expect a callback signature. Make the mock accept and optionally invoke the callback to avoid brittle tests. Also guard assignment to avoid overriding an existing polyfill.-class ResizeObserver { - observe() {} - unobserve() {} - disconnect() {} -} - -(global as any).ResizeObserver = ResizeObserver; +if (typeof (global as any).ResizeObserver === 'undefined') { + class ResizeObserver { + private _cb: any; + constructor(cb?: any) { + this._cb = cb; + } + observe(target?: Element) { + // Optionally fire once for tests that rely on an initial measurement + if (typeof this._cb === 'function') { + this._cb([], this); + } + } + unobserve() {} + disconnect() {} + } + (global as any).ResizeObserver = ResizeObserver; +}frontend/src/components/Media/MediaInfoPanel.tsx (1)
83-87: “Location” renders the entire metadata JSON. Show a specific field.Display a meaningful location/path (e.g.,
metadata.file_location) and keep raw JSON out of the primary UI.- <p className="font-medium text-white"> - {typeof currentImage?.metadata === 'string' - ? currentImage.metadata - : currentImage?.metadata - ? JSON.stringify(currentImage.metadata) - : 'No location data'} - </p> + <p className="font-medium break-all text-white"> + {(() => { + const md: any = currentImage?.metadata; + if (!md) return 'No location data'; + if (typeof md === 'string') return md; + return md.file_location ?? 'No location data'; + })()} + </p>Optional: add a collapsible “Raw metadata” section if you still want to expose the full JSON.
frontend/src/types/Media.ts (2)
1-9: Type aligns with backend; consider making fields optional for resilience.
extract_image_metadatacan return{}; makingImageMetadatafields optional avoids unsafe access in the UI code.-export interface ImageMetadata { - name: string; - date_created: string; - width: number; - height: number; - file_location: string; - file_size: number; - item_type: string; -} +export interface ImageMetadata { + name?: string; + date_created?: string; // ISO 8601 + width?: number; + height?: number; + file_location?: string; + file_size?: number; + item_type?: string; +}
17-17: metadata optionality vs API contract.OpenAPI requires
metadata, but backend may fill{}. Keepingmetadata?is okay for UI ergonomics, but consider documenting that API always returns an object (possibly empty).backend/app/database/images.py (1)
84-118: Consider indexes to speed joins/filters.Add indexes on
images(folder_id),image_classes(image_id), andimage_classes(class_id)to improve query performance in larger libraries.backend/app/routes/images.py (1)
52-56: Message uses count of images: good; consider pagination later.For large libraries this endpoint may become heavy; consider adding pagination and date-based filters in a follow-up.
frontend/src/layout/layout.tsx (1)
12-12: Spacing change: verify it doesn’t reintroduce double-scroll on small viewports.Replacing
p-4with outerm-4changes available height; confirm Home and AI Tagging pages with the timeline don’t overflow/clip atheight: calc(100vh - 56px).frontend/src/pages/Home/Home.tsx (2)
58-64: Render TimelineScrollbar only when markers exist and match its width to the track.Prevents a floating empty control and avoids width mismatch (wrapper w-4 vs internal track w-6).
Apply this diff:
- {/* Timeline Scrollbar */} - <TimelineScrollbar - scrollableRef={scrollableRef} - monthMarkers={monthMarkers} - className="absolute top-0 right-0 h-full w-4" - /> + {/* Timeline Scrollbar */} + {monthMarkers.length > 0 && ( + <TimelineScrollbar + scrollableRef={scrollableRef} + monthMarkers={monthMarkers} + className="absolute top-0 right-0 h-full w-6" + /> + )}
66-66: Drop no‑op onClose prop to MediaView (or wire it properly).The handler is empty and comment says Redux handles closing. Remove for clarity.
Apply this diff:
- {isImageViewOpen && <MediaView onClose={handleCloseMediaView} />} + {isImageViewOpen && <MediaView />}frontend/src/utils/dateUtils.ts (1)
31-43: Be defensive when parsing metadata.date_created.Date strings can be invalid/locale‑dependent. Normalize parsing and bail early on invalid/empty.
Apply this diff:
-export const groupImagesByYearMonthFromMetadata = (images: Image[]) => { +export const groupImagesByYearMonthFromMetadata = (images: Image[]) => { const grouped: Record<string, Record<string, Image[]>> = {}; images.forEach((image) => { - const dateStr = image.metadata?.date_created; // extract date from metadata.date_created - if (!dateStr) return; - - const date = new Date(dateStr); + const dateStr = image.metadata?.date_created; + if (!dateStr) return; + // Accept ISO strings or epoch millis + const epoch = /^\d+$/.test(String(dateStr)) ? Number(dateStr) : dateStr; + const date = new Date(epoch); if (isNaN(date.getTime())) return;frontend/src/pages/AITagging/AITagging.tsx (2)
75-79: Guard TimelineScrollbar render and align width.Avoids drawing a timeline with no data and fixes width mismatch with track.
Apply this diff:
- <TimelineScrollbar - scrollableRef={scrollableRef} - monthMarkers={monthMarkers} - className="absolute top-0 right-0 h-full w-4" - /> + {monthMarkers.length > 0 && ( + <TimelineScrollbar + scrollableRef={scrollableRef} + monthMarkers={monthMarkers} + className="absolute top-0 right-0 h-full w-6" + /> + )}
72-74: Optional: mirror MediaView usage with Home or remove onClose there for consistency.Minor consistency nit only.
frontend/src/components/Media/ChronologicalGallery.tsx (1)
66-79: Minor: Clear refs when the month set changes.Avoids stale entries if months are removed.
Apply this diff:
if (!images.length) { + monthHeaderRefs.current.clear(); return (frontend/src/components/Timeline/TimelineScrollbar.tsx (5)
48-61: Consider using a const assertion or enum for month abbreviations.The month abbreviations mapping could be more type-safe and maintainable using a const assertion to ensure immutability and better type inference.
- const monthAbbreviations: Record<string, string> = { + const monthAbbreviations = { January: 'Jan', February: 'Feb', March: 'Mar', April: 'Apr', May: 'May', June: 'Jun', July: 'Jul', August: 'Aug', September: 'Sept', October: 'Oct', November: 'Nov', December: 'Dec', - }; + } as const;
66-113: Potential memory leak with timer cleanup.The scroll tooltip timer might not be properly cleaned up if the component unmounts while a timer is active, since the cleanup in the effect dependency array changes could leave orphaned timers.
Consider moving the timer cleanup to a dedicated cleanup function:
useEffect(() => { if (isDragging || isMarkerHovered || !scrollableRef.current) { setIsScrollTooltipVisible(false); + if (scrollTooltipTimer.current) { + clearTimeout(scrollTooltipTimer.current); + scrollTooltipTimer.current = null; + } return; }
84-87: Extract magic number for tooltip half height.The tooltip half height value (14) is repeated multiple times. Consider extracting it as a named constant for better maintainability.
+ const TOOLTIP_HALF_HEIGHT = 14; // Line 83-87 - const tooltipHalfHeight = 14; const top = Math.max( - tooltipHalfHeight, - Math.min(scrollProgress * trackHeight, trackHeight - tooltipHalfHeight), + TOOLTIP_HALF_HEIGHT, + Math.min(scrollProgress * trackHeight, trackHeight - TOOLTIP_HALF_HEIGHT), ); // Similar changes for lines 169-173 and 240-244Also applies to: 170-173, 241-244
115-132: ResizeObserver cleanup might not handle all edge cases.The ResizeObserver setup looks good, but consider handling the case where the parent element changes during the component lifecycle.
Add a dependency on the parent element reference:
useEffect(() => { - if (!containerRef.current?.parentElement) return; + const container = containerRef.current; + const parent = container?.parentElement; + if (!parent) return; - const parent = containerRef.current.parentElement; const measure = () => setTrackHeight(parent.clientHeight); // ... rest of the code - }, []); + }, [containerRef.current?.parentElement]);
260-262: Extract tooltip styles to a constant or CSS class.The tooltip base class string is quite long and could be extracted for better maintainability and potential reuse.
- const tooltipBaseClass = - 'text-primary-foreground bg-primary absolute left-[-75px] rounded-md px-2 py-1 text-xs shadow-md'; + const TOOLTIP_BASE_CLASS = 'text-primary-foreground bg-primary absolute left-[-75px] rounded-md px-2 py-1 text-xs shadow-md';Or better yet, consider moving this to a CSS module or Tailwind component class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/app/database/images.py(4 hunks)backend/app/routes/images.py(2 hunks)docs/backend/backend_python/openapi.json(1 hunks)frontend/jest.setup.ts(1 hunks)frontend/src/App.css(1 hunks)frontend/src/components/Media/ChronologicalGallery.tsx(1 hunks)frontend/src/components/Media/MediaInfoPanel.tsx(1 hunks)frontend/src/components/Timeline/TimelineScrollbar.tsx(1 hunks)frontend/src/layout/layout.tsx(1 hunks)frontend/src/pages/AITagging/AITagging.tsx(3 hunks)frontend/src/pages/Home/Home.tsx(3 hunks)frontend/src/types/Media.ts(1 hunks)frontend/src/utils/dateUtils.ts(2 hunks)frontend/src/utils/timelineUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
frontend/src/components/Media/ChronologicalGallery.tsx (4)
frontend/src/types/Media.ts (1)
Image(11-20)frontend/src/features/imageSelectors.ts (1)
selectImages(5-5)frontend/src/utils/dateUtils.ts (2)
groupImagesByYearMonthFromMetadata(31-55)createImageIndexMap(58-66)frontend/src/components/Media/ImageCard.tsx (1)
ImageCard(20-111)
frontend/src/utils/timelineUtils.ts (1)
frontend/src/components/Media/ChronologicalGallery.tsx (1)
MonthMarker(11-15)
frontend/src/utils/dateUtils.ts (1)
frontend/src/types/Media.ts (1)
Image(11-20)
frontend/src/components/Timeline/TimelineScrollbar.tsx (2)
frontend/src/components/Media/ChronologicalGallery.tsx (1)
MonthMarker(11-15)frontend/src/utils/timelineUtils.ts (4)
TooltipState(4-8)useScroll(10-31)useWheel(33-52)getMarkerForScrollPosition(54-61)
backend/app/database/images.py (1)
frontend/src/types/Media.ts (1)
Image(11-20)
frontend/src/pages/Home/Home.tsx (2)
frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(11-15)ChronologicalGallery(25-148)frontend/src/components/Timeline/TimelineScrollbar.tsx (1)
TimelineScrollbar(22-376)
frontend/src/pages/AITagging/AITagging.tsx (3)
frontend/src/features/imageSelectors.ts (2)
selectIsImageViewOpen(22-25)selectTaggedImages(37-39)frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(11-15)ChronologicalGallery(25-148)frontend/src/components/Timeline/TimelineScrollbar.tsx (1)
TimelineScrollbar(22-376)
🔇 Additional comments (7)
docs/backend/backend_python/openapi.json (1)
1862-1864: Schema update to object looks good; flagging downstream consistency.Changing
ImageData.metadatatoobjectmatches the backend response and frontend types.Please confirm all endpoints that return images now use
objectmetadata for consistency.backend/app/database/images.py (2)
213-225: Robust JSON parsing: LGTM.Safe parsing with fallbacks to
{}avoids crashes and matches the new API contract.Also applies to: 231-231
236-238: Good de-duplication of tags.Avoiding duplicate tag aggregation prevents noisy payloads.
backend/app/routes/images.py (1)
16-16: Type change toDict[str, Any]matches new schema.This aligns route models with the updated metadata shape.
frontend/src/utils/dateUtils.ts (1)
58-66: LGTM: fast id→index map.Straightforward and efficient for frequent lookups.
frontend/src/components/Timeline/TimelineScrollbar.tsx (2)
1-376: Overall: Well-implemented timeline scrollbar with minor improvements needed.The component successfully implements the timeline scrollbar functionality with month markers, tooltips, and drag interactions. The integration with the timeline utilities and the chronological gallery is clean and follows React best practices.
Key strengths:
- Good separation of concerns with utilities
- Proper event handler cleanup
- Responsive height measurement
- Smooth scrolling behavior
The main areas for improvement are around error handling during drag operations and some performance optimizations for the marker rendering.
42-46: Verify wheel event handling doesn't block native inertial scrolling
- Inspect useWheel in frontend/src/components/Timeline/TimelineScrollbar.tsx (around lines 42–46): confirm it does not call e.preventDefault() or attach non-passive wheel listeners (passive: false). If it does, remove or narrowly scope preventDefault so native touchpad momentum isn't lost.
- Search the repo for other "wheel" / "onWheel" listeners that might also call preventDefault or update the same container's scrollTop.
- Manually test two-finger touchpad/precision-scroll behavior (macOS and Windows) to ensure inertial scrolling and smoothness are preserved.
| const monthHeaderRefs = useRef<(HTMLDivElement | null)[]>([]); | ||
|
|
There was a problem hiding this comment.
Compute marker offsets relative to the scroll container; filter/sort refs; clear stale refs.
offsetTop may be relative to a different offsetParent and produce mismatched offsets vs scrollTop. Also, stale/null refs should be filtered and markers sorted by DOM position.
Apply this diff:
- const monthHeaderRefs = useRef<(HTMLDivElement | null)[]>([]);
+ // Map of 'YYYY-MM' -> header element
+ const monthHeaderRefs = useRef<Map<string, HTMLDivElement | null>>(new Map()); useEffect(() => {
- if (onMonthOffsetsChange) {
- const markers = monthHeaderRefs.current.map((ref) => {
- const year = ref?.parentElement?.parentElement?.dataset.year || '';
- const month =
- ref?.parentElement?.dataset.timelineMonth?.split('-')[1] || '';
- return {
- offset: ref?.offsetTop ?? 0,
- month: new Date(Number(year), Number(month) - 1).toLocaleString(
- 'default',
- { month: 'long' },
- ),
- year,
- };
- });
- onMonthOffsetsChange(markers);
- }
- }, [images, onMonthOffsetsChange]);
+ if (!onMonthOffsetsChange) return;
+ // Rebuild markers from current refs
+ const entries = Array.from(monthHeaderRefs.current.entries()).flatMap(
+ ([key, el]) => {
+ if (!el) return [];
+ const [y, m] = key.split('-');
+ const monthName = new Date(Number(y), Number(m) - 1).toLocaleString(
+ 'default',
+ { month: 'long' },
+ );
+ // Compute offset relative to provided scroll container when available
+ // @ts-ignore: prop added in this PR; see props diff below
+ const scroller = scrollContainerRef?.current as HTMLElement | undefined;
+ const offset = scroller
+ ? el.getBoundingClientRect().top -
+ scroller.getBoundingClientRect().top +
+ scroller.scrollTop
+ : el.offsetTop;
+ return [{ offset, month: monthName, year: y }];
+ },
+ );
+ // Sort by offset ascending to match scroll coordinates
+ entries.sort((a, b) => a.offset - b.offset);
+ onMonthOffsetsChange(entries);
+ }, [images, onMonthOffsetsChange /* , scrollContainerRef */]);And update props to accept the scroller:
type ChronologicalGalleryProps = {
images: Image[];
showTitle?: boolean;
title?: string;
className?: string;
onMonthOffsetsChange?: (markers: MonthMarker[]) => void;
+ scrollContainerRef?: React.RefObject<HTMLElement | null>;
}; export const ChronologicalGallery = ({
images,
showTitle = false,
title = 'Image Gallery',
className = '',
onMonthOffsetsChange,
+ scrollContainerRef,
}: ChronologicalGalleryProps) => {Also applies to: 47-64
🤖 Prompt for AI Agents
frontend/src/components/Media/ChronologicalGallery.tsx around lines 33-34 and
47-64: monthHeaderRefs is currently an unsanitized array of div refs and offsets
are computed with offsetTop (which may use a different offsetParent); update the
component to accept a scroller prop (HTMLElement | null) and compute marker
offsets relative to that scroller by using getBoundingClientRect() on each
header and the scroller and subtracting their tops, filter out null/stale refs
before using them, sort the refs by DOM order (e.g., compareDocumentPosition or
by the computed top) so markers are ordered, and ensure you clear stale refs
(e.g., set monthHeaderRefs.current = filteredRefs) whenever refs change or on
cleanup so future calculations only use valid elements.
| const handleScroll = () => { | ||
| const { scrollTop, scrollHeight, clientHeight } = scrollable; | ||
| const progress = scrollTop / (scrollHeight - clientHeight); | ||
| setScrollProgress(progress); | ||
| }; | ||
|
|
||
| scrollable.addEventListener('scroll', handleScroll, { passive: true }); |
There was a problem hiding this comment.
Guard division by zero and clamp progress to [0,1].
When content fits the viewport, (scrollHeight − clientHeight) is 0, yielding NaN and breaking styles.
Apply this diff:
- const handleScroll = () => {
- const { scrollTop, scrollHeight, clientHeight } = scrollable;
- const progress = scrollTop / (scrollHeight - clientHeight);
- setScrollProgress(progress);
- };
+ const handleScroll = () => {
+ const { scrollTop, scrollHeight, clientHeight } = scrollable;
+ const denom = Math.max(scrollHeight - clientHeight, 1);
+ const progress = Math.min(1, Math.max(0, scrollTop / denom));
+ setScrollProgress(progress);
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleScroll = () => { | |
| const { scrollTop, scrollHeight, clientHeight } = scrollable; | |
| const progress = scrollTop / (scrollHeight - clientHeight); | |
| setScrollProgress(progress); | |
| }; | |
| scrollable.addEventListener('scroll', handleScroll, { passive: true }); | |
| const handleScroll = () => { | |
| const { scrollTop, scrollHeight, clientHeight } = scrollable; | |
| const denom = Math.max(scrollHeight - clientHeight, 1); | |
| const progress = Math.min(1, Math.max(0, scrollTop / denom)); | |
| setScrollProgress(progress); | |
| }; | |
| scrollable.addEventListener('scroll', handleScroll, { passive: true }); |
🤖 Prompt for AI Agents
In frontend/src/utils/timelineUtils.ts around lines 17 to 23, the scroll
progress calculation can divide by zero when scrollHeight − clientHeight is 0
and produce NaN; change the handler to compute denominator = Math.max(1,
scrollHeight - clientHeight) or explicitly check if denominator <= 0 and set
progress = 0, then compute progress = scrollTop / denominator and clamp the
result to the [0,1] range (e.g., progress = Math.min(1, Math.max(0, progress)))
before calling setScrollProgress.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/database/images.py (1)
289-299: Return type mismatch: db_get_untagged_images claims ImageRecord but omits isTagged.Either add isTagged: False to each row or relax the return type.
-def db_get_untagged_images() -> List[ImageRecord]: +def db_get_untagged_images() -> List[ImageRecord]: @@ untagged_images.append( { "id": image_id, "path": path, "folder_id": folder_id, "thumbnailPath": thumbnail_path, - "metadata": md, + "metadata": md, + "isTagged": False, } )Also applies to: 327-334
♻️ Duplicate comments (4)
frontend/src/pages/Home/Home.tsx (1)
50-56: Resolved: offsets now computed in the correct coordinate space.Passing scrollContainerRef to ChronologicalGallery fixes the earlier offsetParent mismatch for month markers. Looks good.
frontend/src/pages/AITagging/AITagging.tsx (1)
64-70: Resolved: scroll container passed to ChronologicalGallery.This addresses the earlier offset mismatch for month markers.
backend/app/database/images.py (2)
96-105: Consistent minimal object on outer exceptions — nice.This addresses the earlier feedback to never return {}.
180-190: Switched from OR IGNORE to UPSERT — good upgrade.This will keep timeline markers fresh as metadata changes.
🧹 Nitpick comments (9)
frontend/src/pages/Home/Home.tsx (1)
60-64: Width mismatch: container w-4 vs track w-6 leads to clipping/overflow.TimelineScrollbar’s internal track uses w-6; here the wrapper is w-4. Align widths (either bump wrapper to w-6 or reduce the track to w-4) to avoid cropping and stray padding.
Apply either change:
- <TimelineScrollbar + <TimelineScrollbar scrollableRef={scrollableRef} monthMarkers={monthMarkers} - className="absolute top-0 right-0 h-full w-4" + className="absolute top-0 right-0 h-full w-6" />—or adjust the track width inside TimelineScrollbar (see separate comment).
Also applies to: 44-44
frontend/src/components/Media/ChronologicalGallery.tsx (2)
118-121: Avoid retaining null refs; delete on unmount to prevent stale entries.When a section unmounts, React calls the ref with null. Delete the key to keep the map clean.
ref={(el) => { const key = `${year}-${month}`; - monthHeaderRefs.current.set(key, el); + if (el) { + monthHeaderRefs.current.set(key, el); + } else { + monthHeaderRefs.current.delete(key); + } }}
11-15: Prefer locale‑agnostic payloads for MonthMarker; format in the view.Storing a localized month name ties consumers to one locale. Consider adding monthIndex (0–11) or a timestamp and format in TimelineScrollbar via Intl.DateTimeFormat('default', { month: 'short' }).
frontend/src/pages/AITagging/AITagging.tsx (2)
76-80: Width mismatch with internal track (w-6).Same as Home: update wrapper width to avoid clipping or adjust the track width.
- <TimelineScrollbar + <TimelineScrollbar scrollableRef={scrollableRef} monthMarkers={monthMarkers} - className="absolute top-0 right-0 h-full w-4" + className="absolute top-0 right-0 h-full w-6" />
76-80: Minor a11y: add role and keyboard support in TimelineScrollbar.Consider role="scrollbar", tabIndex=0, and Arrow/Page/Home/End key handling to move the scrollableRef for keyboard users; set aria-controls to the scroll container id.
backend/app/database/images.py (4)
16-18: Type alias mismatch: FolderId should be int.DB schema defines folder_id INTEGER; code uses List[int] elsewhere. Align the alias.
Apply:
-FolderId = str +FolderId = int
39-49: Differentiate missing files in metadata.For non-existent paths, set item_type to "missing" (not "unknown") so callers can distinguish.
- "item_type": "unknown", + "item_type": "missing",
153-175: Avoid mutating input records; build DB rows explicitly.Prevents surprising side effects upstream and ensures stable JSON encoding.
- # Make sure it's stored as a JSON string in DB - record["metadata"] = json.dumps(metadata) - - prepared_records.append(record) + # Make sure it's stored as a JSON string in DB + prepared_records.append( + { + **record, + "metadata": json.dumps(metadata, ensure_ascii=False), + } + )
214-227: Optional — add server-side chronological sort (SQLite JSON1 verified)File: backend/app/database/images.py
Lines: 214-227Verified sqlite3 JSON1 support in Python (script returned 1). Returning date-sorted rows reduces client work and stabilizes pagination.
- ORDER BY i.path, m.name + ORDER BY + COALESCE(json_extract(i.metadata, '$.date_created'), '') DESC, + i.path, + m.name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/database/images.py(5 hunks)frontend/src/components/Media/ChronologicalGallery.tsx(1 hunks)frontend/src/components/Timeline/TimelineScrollbar.tsx(1 hunks)frontend/src/pages/AITagging/AITagging.tsx(3 hunks)frontend/src/pages/Home/Home.tsx(3 hunks)frontend/src/utils/timelineUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/utils/timelineUtils.ts
- frontend/src/components/Timeline/TimelineScrollbar.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
backend/app/database/images.py (1)
frontend/src/types/Media.ts (1)
Image(11-20)
frontend/src/components/Media/ChronologicalGallery.tsx (4)
frontend/src/types/Media.ts (1)
Image(11-20)frontend/src/features/imageSelectors.ts (1)
selectImages(5-5)frontend/src/utils/dateUtils.ts (2)
groupImagesByYearMonthFromMetadata(31-55)createImageIndexMap(58-66)frontend/src/components/Media/ImageCard.tsx (1)
ImageCard(20-111)
frontend/src/pages/AITagging/AITagging.tsx (3)
frontend/src/features/imageSelectors.ts (2)
selectIsImageViewOpen(22-25)selectTaggedImages(37-39)frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(11-15)ChronologicalGallery(26-155)frontend/src/components/Timeline/TimelineScrollbar.tsx (1)
TimelineScrollbar(22-385)
frontend/src/pages/Home/Home.tsx (2)
frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(11-15)ChronologicalGallery(26-155)frontend/src/components/Timeline/TimelineScrollbar.tsx (1)
TimelineScrollbar(22-385)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Tests
🔇 Additional comments (2)
frontend/src/components/Media/ChronologicalGallery.tsx (1)
37-47: LGTM: efficient grouping and index map.Good use of useMemo and shared Redux index mapping to keep ImageCard selection consistent across filtered views.
backend/app/database/images.py (1)
3-6: Imports look good for EXIF work.
…rollbar and Gallery components updated
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
frontend/src/components/Media/ChronologicalGallery.tsx (1)
143-146: Avoid stale refs accumulating in the Map.Delete the key when the element unmounts (el === null).
- ref={(el) => { - const key = `${year}-${month}`; - monthHeaderRefs.current.set(key, el); - }} + ref={(el) => { + const key = `${year}-${month}`; + if (el) { + monthHeaderRefs.current.set(key, el); + } else { + monthHeaderRefs.current.delete(key); + } + }}
🧹 Nitpick comments (13)
frontend/src/pages/Home/Home.tsx (2)
60-66: Render the timeline only when markers exist.Prevents showing an empty scrollbar when images lack valid dates.
- {images.length > 0 && ( + {monthMarkers.length > 0 && ( <TimelineScrollbar scrollableRef={scrollableRef} monthMarkers={monthMarkers} className="absolute top-0 right-0 h-full w-4" /> )}
40-42: Remove no-op onClose for MediaView (keep consistent with AITagging).Slight cleanup; MediaView closes via Redux.
- const handleCloseMediaView = () => { - // MediaView will handle closing via Redux - }; ... - {isImageViewOpen && <MediaView onClose={handleCloseMediaView} />} + {isImageViewOpen && <MediaView />}Also applies to: 69-69
frontend/src/components/Media/ChronologicalGallery.tsx (2)
97-99: Use disconnect() for ResizeObserver cleanup.Safer if the observed element changes.
- return () => { - observer.unobserve(elementToObserve); - }; + return () => { + observer.disconnect(); + };
61-83: Minor: skip callback when no headers.Saves a redundant state update when the Map is empty.
const recomputeMarkers = useCallback(() => { if (!onMonthOffsetsChange) return; + if (monthHeaderRefs.current.size === 0) { + onMonthOffsetsChange([]); + return; + }frontend/src/pages/AITagging/AITagging.tsx (1)
76-82: Gate TimelineScrollbar on markers, not just images.Matches Home and hides empty track when no dated images.
- {taggedImages.length > 0 && ( + {monthMarkers.length > 0 && ( <TimelineScrollbar scrollableRef={scrollableRef} monthMarkers={monthMarkers} className="absolute top-0 right-0 h-full w-4" /> )}frontend/src/components/Timeline/TimelineScrollbar.tsx (3)
154-161: Avoid re-sorting markers on every lookup.Pre-sort once and reuse for tooltip/drag lookups.
+ const sortedMarkers = useMemo( + () => monthMarkers.slice().sort((a, b) => a.offset - b.offset), + [monthMarkers], + ); ... - const currentMarker = getMarkerForScrollPosition( - scrollable.scrollTop, - monthMarkers, - ); + const currentMarker = getMarkerForScrollPosition(scrollable.scrollTop, sortedMarkers); ... - const draggedMarker = getMarkerForScrollPosition( - correspondingScrollTop, - monthMarkers, - ); + const draggedMarker = getMarkerForScrollPosition(correspondingScrollTop, sortedMarkers); ... - const hoveredMarker = getMarkerForScrollPosition( - correspondingScrollTop, - monthMarkers, - ); + const hoveredMarker = getMarkerForScrollPosition(correspondingScrollTop, sortedMarkers);Also applies to: 211-214, 288-291
284-287: Clamp hover scroll percentage.Protects against minor float errors and edge math.
- const scrollPercentage = hoverY / height; + const scrollPercentage = Math.min(Math.max(hoverY / height, 0), 1);
107-111: Wheel handler: guard against detached ref.Tiny safety to avoid rare NPE if the node unmounts mid-wheel.
- useWheel(containerRef, (deltaY) => { - if (scrollableRef.current) { - scrollableRef.current.scrollTop += deltaY; - } - }); + useWheel(containerRef, (deltaY) => { + const scroller = scrollableRef.current; + if (scroller) scroller.scrollTop += deltaY; + });backend/app/database/images.py (5)
35-39: Enable a sane busy timeout to reduce “database is locked” errors.Foreign keys ON is great. Consider adding a small busy timeout.
Apply:
def _connect() -> sqlite3.Connection: conn = sqlite3.connect(DATABASE_PATH) # Ensure ON DELETE CASCADE and other FKs are enforced conn.execute("PRAGMA foreign_keys = ON") + # Reduce contention errors under concurrent access + conn.execute("PRAGMA busy_timeout = 5000") return conn
46-56: Nonexistent file sentinel: consider item_type="missing".Returning a stable minimal object is good. Labeling missing files distinctly can aid debugging and cleanup tooling.
- "item_type": "unknown", + "item_type": "missing",
88-103: Timezone semantics for date_created are ambiguous.EXIF times are naïve; mtime is from local clock. For consistent month bucketing across hosts/timezones, consider normalizing mtime to UTC and documenting that EXIF is naïve local time.
Example tweak:
- date_created = datetime.datetime.fromtimestamp( - stats.st_mtime - ).isoformat() + date_created = datetime.datetime.fromtimestamp( + stats.st_mtime, tz=datetime.timezone.utc + ).isoformat()Optionally add a metadata flag like date_source = "exif" | "mtime" to help clients disambiguate.
181-206: Avoid mutating caller-provided records when JSON-stringifying metadata.You overwrite record["metadata"] in place. Prefer building a DB-only dict to prevent surprising callers.
Apply:
- prepared_records = [] + prepared_records = [] for record in image_records: metadata = record.get("metadata") @@ - # Make sure it's stored as a JSON string in DB - record["metadata"] = json.dumps(metadata) - - prepared_records.append(record) + # Build DB payload without mutating the input record + prepared_records.append({ + "id": record["id"], + "path": record["path"], + "folder_id": record["folder_id"], + "thumbnailPath": record["thumbnailPath"], + "metadata": json.dumps(metadata), + "isTagged": record.get("isTagged", False), + })
274-286: Parsing metadata and de-duping tags: OK; minor perf nit.Logic is correct. If tag lists can be long, consider a set during aggregation and convert to list at the end.
Also applies to: 297-299, 309-309
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/database/images.py(11 hunks)frontend/src/components/Media/ChronologicalGallery.tsx(1 hunks)frontend/src/components/Timeline/TimelineScrollbar.tsx(1 hunks)frontend/src/pages/AITagging/AITagging.tsx(3 hunks)frontend/src/pages/Home/Home.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/src/components/Media/ChronologicalGallery.tsx (4)
frontend/src/types/Media.ts (1)
Image(11-20)frontend/src/features/imageSelectors.ts (1)
selectImages(5-5)frontend/src/utils/dateUtils.ts (2)
groupImagesByYearMonthFromMetadata(31-55)createImageIndexMap(58-66)frontend/src/components/Media/ImageCard.tsx (1)
ImageCard(20-111)
backend/app/database/images.py (1)
frontend/src/types/Media.ts (1)
Image(11-20)
frontend/src/pages/Home/Home.tsx (2)
frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(11-15)ChronologicalGallery(26-182)frontend/src/components/Timeline/TimelineScrollbar.tsx (1)
TimelineScrollbar(39-422)
frontend/src/components/Timeline/TimelineScrollbar.tsx (2)
frontend/src/components/Media/ChronologicalGallery.tsx (1)
MonthMarker(11-15)frontend/src/utils/timelineUtils.ts (4)
TooltipState(4-8)useScroll(10-32)useWheel(34-53)getMarkerForScrollPosition(55-67)
frontend/src/pages/AITagging/AITagging.tsx (3)
frontend/src/features/imageSelectors.ts (2)
selectIsImageViewOpen(22-25)selectTaggedImages(37-39)frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(11-15)ChronologicalGallery(26-182)frontend/src/components/Timeline/TimelineScrollbar.tsx (1)
TimelineScrollbar(39-422)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Tests
🔇 Additional comments (7)
backend/app/database/images.py (7)
3-7: Imports look correct and minimal.The new dependencies align with the added functionality.
28-28: Good: metadata type now matches DB TEXT serialization.Union[Mapping[str, Any], str] prevents mypy drift between in‑memory dicts and stored JSON.
67-87: EXIF handling is robust.Using getexif with a safe fallback and defensive decoding looks solid.
114-126: Graceful fallback on Pillow open failure.Keeps file stats and avoids empty metadata; good.
239-239: Consistent connection helper usage.Good centralization of FK enforcement.
416-425: INSERT OR IGNORE is fine here.For a pure junction table with no extra columns, ignoring duplicates is acceptable.
488-507: Deletion path looks good.FKs enabled per-connection ensures CASCADE on image_classes.
| INSERT INTO images (id, path, folder_id, thumbnailPath, metadata, isTagged) | ||
| VALUES (:id, :path, :folder_id, :thumbnailPath, :metadata, :isTagged) | ||
| """, | ||
| image_records, | ||
| ON CONFLICT(path) DO UPDATE SET | ||
| folder_id=excluded.folder_id, | ||
| thumbnailPath=excluded.thumbnailPath, | ||
| metadata=excluded.metadata, | ||
| isTagged=excluded.isTagged | ||
| """, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t downgrade isTagged on upsert.
Re-ingest flows often pass isTagged=False; this would wipe a previously tagged image. Preserve True once set; only escalate to True.
Apply:
ON CONFLICT(path) DO UPDATE SET
folder_id=excluded.folder_id,
thumbnailPath=excluded.thumbnailPath,
- metadata=excluded.metadata,
- isTagged=excluded.isTagged
+ metadata=excluded.metadata,
+ isTagged=CASE
+ WHEN excluded.isTagged THEN 1
+ ELSE images.isTagged
+ ENDPlease verify call sites don’t rely on resetting isTagged to False unintentionally:
🏁 Script executed:
#!/bin/bash
# Inspect builders/callers of db_bulk_insert_images for isTagged usage
rg -n -C2 "db_bulk_insert_images\(|isTagged\s*[:=]" --type=py --type=ts --type=tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for calls to db_bulk_insert_images and usages of isTagged..."
rg -n -C2 "db_bulk_insert_images\(" || true
rg -n -C2 "\bisTagged\b" || true
echo
echo "Searching for images table schema, INSERTs, and ON CONFLICT upserts..."
rg -n -C2 "CREATE TABLE images" || true
rg -n -C2 "INSERT INTO images" || true
rg -n -C2 "ON CONFLICT\(path\) DO UPDATE SET" || true
echo
echo "Showing backend/app/database/images.py (if present) up to 500 lines for manual inspection..."
if [ -f backend/app/database/images.py ]; then
sed -n '1,500p' backend/app/database/images.py || true
else
echo "backend/app/database/images.py not found"
fiLength of output: 23414
Don't downgrade isTagged on upsert — preserve True.
ON CONFLICT currently sets isTagged = excluded.isTagged; a re-ingest with isTagged=False will clear a previously-tagged image. Change the upsert to only escalate to True.
Apply:
ON CONFLICT(path) DO UPDATE SET
folder_id=excluded.folder_id,
thumbnailPath=excluded.thumbnailPath,
- metadata=excluded.metadata,
- isTagged=excluded.isTagged
+ metadata=excluded.metadata,
+ isTagged=CASE
+ WHEN excluded.isTagged THEN 1
+ ELSE images.isTagged
+ ENDVerify callers that might intentionally rely on resetting isTagged to False:
- backend/app/database/images.py (current upsert at ~lines 212-219)
- backend/app/utils/images.py (db_bulk_insert_images call ~line 67; prepared records set "isTagged": False around ~lines 160-164)
- backend/app/routes/images.py (routes pass isTagged around ~lines 44-47)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| INSERT INTO images (id, path, folder_id, thumbnailPath, metadata, isTagged) | |
| VALUES (:id, :path, :folder_id, :thumbnailPath, :metadata, :isTagged) | |
| """, | |
| image_records, | |
| ON CONFLICT(path) DO UPDATE SET | |
| folder_id=excluded.folder_id, | |
| thumbnailPath=excluded.thumbnailPath, | |
| metadata=excluded.metadata, | |
| isTagged=excluded.isTagged | |
| """, | |
| INSERT INTO images (id, path, folder_id, thumbnailPath, metadata, isTagged) | |
| VALUES (:id, :path, :folder_id, :thumbnailPath, :metadata, :isTagged) | |
| ON CONFLICT(path) DO UPDATE SET | |
| folder_id=excluded.folder_id, | |
| thumbnailPath=excluded.thumbnailPath, | |
| metadata=excluded.metadata, | |
| isTagged=CASE | |
| WHEN excluded.isTagged THEN 1 | |
| ELSE isTagged | |
| END | |
| """, |
🤖 Prompt for AI Agents
In backend/app/database/images.py around lines 212 to 219 the ON CONFLICT upsert
unconditionally sets isTagged = excluded.isTagged which allows a re-ingest with
isTagged=False to clear a previously-tagged image; change the upsert so it only
escalates to True (e.g., set isTagged = isTagged OR excluded.isTagged, or use
CASE to preserve True) so existing True values cannot be downgraded, and then
audit the listed callers (backend/app/database/images.py upsert,
backend/app/utils/images.py where records are prepared for db_bulk_insert_images
around lines ~160-164, and backend/app/routes/images.py where isTagged is passed
~lines 44-47) to ensure no code intentionally relies on resetting isTagged to
False and adjust those callers if needed.
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/database/images.py (1)
333-385: Possible None metadata leaks to clients; coerce to dict.json.loads("null") returns None; current branch can return metadata=None, violating the TypedDict and risking UI errors.
Apply:
- md: dict = {} - if metadata: - try: - md = ( - json.loads(metadata) - if isinstance(metadata, str) - else (metadata or {}) - ) - except Exception: - md = {} + md: dict = {} + if metadata: + try: + parsed = json.loads(metadata) if isinstance(metadata, str) else metadata + md = parsed if isinstance(parsed, dict) else {} + except Exception: + md = {}
♻️ Duplicate comments (1)
frontend/src/pages/AITagging/AITagging.tsx (1)
62-70: Great: Passing scrollContainerRef keeps marker offsets in the correct space.This addresses the earlier concern about mismatched coordinate systems.
🧹 Nitpick comments (14)
frontend/src/pages/Home/Home.tsx (3)
59-66: Fix width/position duplication; align container width with track widthTimelineScrollbar internally renders an absolute container and a track of width w-6. Here the outer className repeats absolute/top/right and sets w-4, causing the track (w-6) to overflow the container and potentially misalign pointer hit‑targets. Keep positioning inside the component and match container width to the track.
Apply this diff:
- <TimelineScrollbar - scrollableRef={scrollableRef} - monthMarkers={monthMarkers} - className="absolute top-0 right-0 h-full w-4" - /> + <TimelineScrollbar + scrollableRef={scrollableRef} + monthMarkers={monthMarkers} + className="w-6" + />
46-49: A11y: label the primary scroll regionAdd an accessible label so screen readers can identify the main scrollable gallery area.
- <div - ref={scrollableRef} - className="hide-scrollbar flex-1 overflow-x-hidden overflow-y-auto" - > + <div + ref={scrollableRef} + className="hide-scrollbar flex-1 overflow-x-hidden overflow-y-auto" + role="region" + aria-label="Chronological image gallery" + >
44-44: Keep padding in sync with timeline widthYou use pr-6 on the container while the timeline track is w-6. If the track width changes later, remember to update this padding to avoid content overlap. Consider extracting a shared variable or utility class for the timeline gutter to prevent drift.
backend/app/database/images.py (6)
45-49: Set row_factory on connections for safer column access.Optional, but returning sqlite3.Row reduces dict unpacking errors and eases future refactors.
Apply:
def _connect() -> sqlite3.Connection: conn = sqlite3.connect(DATABASE_PATH) + conn.row_factory = sqlite3.Row # Ensure ON DELETE CASCADE and other FKs are enforced conn.execute("PRAGMA foreign_keys = ON") return conn
52-148: Normalize date_created to timezone-aware UTC; keep EXIF fallbacks.Naive datetimes can sort inconsistently across hosts/timezones, which risks wrong month bucketing in the timeline. Emit ISO-8601 with UTC tz.
Apply:
- date_created = datetime.datetime.strptime( - dt_original.strip().split("\x00", 1)[0], - "%Y:%m:%d %H:%M:%S", - ).isoformat() + date_created = datetime.datetime.strptime( + dt_original.strip().split("\x00", 1)[0], + "%Y:%m:%d %H:%M:%S", + ).replace(tzinfo=datetime.timezone.utc).isoformat() except ValueError: - date_created = datetime.datetime.fromtimestamp( - stats.st_mtime - ).isoformat() + date_created = datetime.datetime.fromtimestamp( + stats.st_mtime, tz=datetime.timezone.utc + ).isoformat() else: - date_created = datetime.datetime.fromtimestamp( - stats.st_mtime - ).isoformat() + date_created = datetime.datetime.fromtimestamp( + stats.st_mtime, tz=datetime.timezone.utc + ).isoformat()Optional: also check EXIF "DateTimeDigitized" and "DateTime" when "DateTimeOriginal" is missing.
150-183: Add indexes to match query patterns (folder scans, untagged queries).Will speed up joins and the AI-tagging selection.
Apply after table creation:
cursor.execute( """ CREATE TABLE IF NOT EXISTS image_classes ( @@ ) """ ) + + # Helpful indexes + cursor.execute("CREATE INDEX IF NOT EXISTS idx_images_folder ON images(folder_id)") + cursor.execute("CREATE INDEX IF NOT EXISTS idx_images_isTagged ON images(isTagged)") + cursor.execute("CREATE INDEX IF NOT EXISTS idx_ic_image ON image_classes(image_id)") + cursor.execute("CREATE INDEX IF NOT EXISTS idx_ic_class ON image_classes(class_id)") + cursor.execute("CREATE INDEX IF NOT EXISTS idx_folders_ai ON folders(AI_Tagging)")
186-234: Make metadata JSON stable; avoid downgrading thumbnailPath to NULL; confirm path-as-key invariant.
- Preserve non-ASCII and smaller payloads when serializing.
- Prevent accidental NULL overwrite of thumbnailPath on upsert.
- Double-check that “path” is the canonical key and renames are treated as new images.
Apply:
- record["metadata"] = json.dumps(metadata) + record["metadata"] = json.dumps(metadata, ensure_ascii=False, separators=(",", ":"))ON CONFLICT(path) DO UPDATE SET folder_id=excluded.folder_id, - thumbnailPath=excluded.thumbnailPath, + thumbnailPath=COALESCE(excluded.thumbnailPath, images.thumbnailPath), metadata=excluded.metadata, isTagged=CASE WHEN excluded.isTagged THEN 1 ELSE images.isTagged ENDPlease confirm:
- Ingest treats “path” as the stable identity; if a file moves/renames, do we intentionally create a new row?
- No code relies on clearing thumbnailPath by passing null.
245-331: Tag de-dup can be O(n²); use a set internally.Minor micro-optimization: build tags in a set, then convert to list once at the end.
468-479: Deterministic ordering for folder_id queries.Add ORDER BY for stable pagination and tests.
Apply:
cursor.execute( f""" SELECT id, path, thumbnailPath FROM images WHERE folder_id IN ({placeholders}) + ORDER BY path """, folder_ids, )frontend/src/pages/AITagging/AITagging.tsx (5)
24-26: Fix ref typing to allow null (TS strict).useRef should include null in the generic to match the initial value and avoid downstream type casting.
- const scrollableRef = useRef<HTMLDivElement>(null); + const scrollableRef = useRef<HTMLDivElement | null>(null);
14-17: Import MonthMarker as a type-only import.Slight tree‑shaking/clarity win and avoids accidental value import.
-import { - ChronologicalGallery, - MonthMarker, -} from '@/components/Media/ChronologicalGallery'; +import { ChronologicalGallery } from '@/components/Media/ChronologicalGallery'; +import type { MonthMarker } from '@/components/Media/ChronologicalGallery';
37-47: Ensure global loader always hides on unmount and avoids flicker.If the component unmounts mid‑load, the loader may remain shown. Track local show state and hide in cleanup.
useEffect(() => { - if (imagesLoading) { - dispatch(showLoader('Loading AI tagging data')); - } else if (imagesError) { - dispatch(hideLoader()); - } else if (imagesSuccess) { - const images = imagesData?.data as Image[]; - dispatch(setImages(images)); - dispatch(hideLoader()); - } - }, [imagesData, imagesSuccess, imagesError, imagesLoading, dispatch]); + let didShow = false; + if (imagesLoading) { + dispatch(showLoader('Loading AI tagging data')); + didShow = true; + } else { + // Always clear loader when not loading + dispatch(hideLoader()); + if (imagesSuccess) { + const images = imagesData?.data as Image[]; + dispatch(setImages(images)); + } + // (optional) surface error UI when imagesError is true + } + return () => { + if (didShow) dispatch(hideLoader()); + }; + }, [imagesData, imagesSuccess, imagesError, imagesLoading, dispatch]);
43-45: Avoid unsafe cast; validate response shape before dispatch.Casting imagesData?.data as Image[] can mask server/schema drift. Prefer a narrow mapper/validator before setImages.
I can provide a minimal zod schema and mapper if helpful.
50-55: Spacing parity confirmed — optional refactor recommended.Both frontend/src/pages/AITagging/AITagging.tsx and frontend/src/pages/Home/Home.tsx use pr-6 while TimelineScrollbar is w-4, so there’s no immediate mismatch; content won’t be obscured. Optional: centralize the spacing or add responsive parity (e.g., pr-4 md:pr-6 + w-3 md:w-4) to prevent future drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/database/images.py(12 hunks)frontend/src/components/Media/ChronologicalGallery.tsx(1 hunks)frontend/src/components/Timeline/TimelineScrollbar.tsx(1 hunks)frontend/src/pages/AITagging/AITagging.tsx(3 hunks)frontend/src/pages/Home/Home.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/Timeline/TimelineScrollbar.tsx
- frontend/src/components/Media/ChronologicalGallery.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/pages/Home/Home.tsx (2)
frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(11-15)ChronologicalGallery(26-190)frontend/src/components/Timeline/TimelineScrollbar.tsx (1)
TimelineScrollbar(39-421)
frontend/src/pages/AITagging/AITagging.tsx (3)
frontend/src/features/imageSelectors.ts (2)
selectIsImageViewOpen(22-25)selectTaggedImages(37-39)frontend/src/components/Media/ChronologicalGallery.tsx (2)
MonthMarker(11-15)ChronologicalGallery(26-190)frontend/src/components/Timeline/TimelineScrollbar.tsx (1)
TimelineScrollbar(39-421)
backend/app/database/images.py (1)
frontend/src/types/Media.ts (1)
Image(11-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Backend Tests
🔇 Additional comments (5)
frontend/src/pages/Home/Home.tsx (2)
50-56: Good: Gallery wired to timeline source of truthPassing both onMonthOffsetsChange and scrollContainerRef into ChronologicalGallery ensures markers are computed in the same scroll coordinate space. This addresses the prior offsetParent issue.
20-22: State wiring for timeline is cleanLocal state for monthMarkers and a stable scroll ref keep the page concerns minimal and composable.
backend/app/database/images.py (2)
3-7: Type widening and imports look good.Using Union[Mapping[str, Any], str] for metadata aligns with DB TEXT storage; Pillow/EXIF imports are appropriate for the new extraction logic.
Also applies to: 28-28
32-41: UntaggedImageRecord type fits the API surface.Clear separation from the full ImageRecord and matches the untagged query payload.
frontend/src/pages/AITagging/AITagging.tsx (1)
62-70: Confirmed — all ChronologicalGallery usages pass scrollContainerRef and onMonthOffsetsChange.Found in frontend/src/pages/AITagging/AITagging.tsx (lines 64–70) and frontend/src/pages/Home/Home.tsx (lines 50–56); both include scrollContainerRef and onMonthOffsetsChange.
| images_dict[image_id] = { | ||
| "id": image_id, | ||
| "path": path, | ||
| "folder_id": folder_id, | ||
| "thumbnailPath": thumbnail_path, | ||
| "metadata": metadata, | ||
| "metadata": metadata_dict, | ||
| "isTagged": bool(is_tagged), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
folder_id type mismatch between DB (INTEGER) and frontend (string). Cast on return.
Frontend types (Media.ts) expect folder_id: string. Backend currently returns int. Cast to str to avoid subtle client bugs.
Apply:
- "folder_id": folder_id,
+ "folder_id": str(folder_id),- "folder_id": folder_id,
+ "folder_id": str(folder_id),Run to confirm frontend expects string:
Also applies to: 373-378
🏁 Script executed:
#!/bin/bash
rg -nC2 'folder_id\s*:\s*string' --type=ts --type=tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for 'folder_id: string' in .ts/.tsx files..."
rg -nC2 "folder_id\s*:\s*string" -g '!**/node_modules/**' -g '**/*.ts' -g '**/*.tsx' || true
echo "Searching for 'folderId: string' in .ts/.tsx files..."
rg -nC2 "folderId\s*:\s*string" -g '!**/node_modules/**' -g '**/*.ts' -g '**/*.tsx' || true
echo "Searching for any occurrence of 'folder_id' or 'folderId' in .ts/.tsx files..."
rg -nC2 "\b(folder_id|folderId)\b" -g '!**/node_modules/**' -g '**/*.ts' -g '**/*.tsx' || true
echo "Searching for interface/type named Media in .ts/.tsx files..."
rg -nC2 "(interface|type)\s+Media" -g '!**/node_modules/**' -g '**/*.ts' -g '**/*.tsx' || trueLength of output: 12108
Cast folder_id to string in image responses
Frontend types (frontend/src/types/Media.ts, frontend/src/types/Folder.ts) declare folder_id as string; backend returns integers — cast to string before returning. Location: backend/app/database/images.py lines 300-306 and 373-378.
Apply:
- "folder_id": folder_id,
+ "folder_id": str(folder_id),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| images_dict[image_id] = { | |
| "id": image_id, | |
| "path": path, | |
| "folder_id": folder_id, | |
| "thumbnailPath": thumbnail_path, | |
| "metadata": metadata, | |
| "metadata": metadata_dict, | |
| "isTagged": bool(is_tagged), | |
| images_dict[image_id] = { | |
| "id": image_id, | |
| "path": path, | |
| "folder_id": str(folder_id), | |
| "thumbnailPath": thumbnail_path, | |
| "metadata": metadata_dict, | |
| "isTagged": bool(is_tagged), |
🤖 Prompt for AI Agents
In backend/app/database/images.py around lines 300-306 and 373-378 the image
response currently returns folder_id as an integer while frontend types expect a
string; update the response objects to cast folder_id to a string (e.g.,
folder_id = str(folder_id) or wrap the value with str(...) where building the
dicts) so both places return folder_id as a string.
|
@rohan-pandeyy I need some time to properly review this. Kindly wait for one more week :) |
rahulharpal1603
force-pushed
the
main
branch
from
f589c41 to
00d7a55
Compare
3 participants
Fixes: #495
This PR is divided into 3 major parts:
1. Image metadata retrieval:
Status: Done ✅
2. Chronological gallery view
Status: Done ✅
3. Timeline scrollbar integration
Status: Done ✅
Recording.2025-09-14.195728.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests
Documentation