Feat/lazy image loading #998
Conversation
📝 WalkthroughWalkthroughAdds a reusable IntersectionObserver hook and a LazyImage component that defers image loading until visible, shows a skeleton while loading, handles load errors, and applies a fade-in. Replaces img usage with LazyImage in ImageCard and MediaThumbnails. Changes
Sequence DiagramsequenceDiagram
participant User
participant BrowserViewport
participant IntersectionObserver
participant LazyImage
participant ImageElement as <img>
User->>BrowserViewport: Scrolls / navigates
BrowserViewport->>IntersectionObserver: layout/visibility updates
IntersectionObserver->>LazyImage: notify visible
LazyImage->>LazyImage: set isVisible = true
LazyImage->>ImageElement: set src, attach onLoad/onError
ImageElement->>ImageElement: fetch resource
ImageElement->>LazyImage: onLoad / onError
LazyImage->>LazyImage: set isLoaded or hasError
LazyImage->>ImageElement: apply fade-in (if loaded) or show error UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/Media/ImageCard.tsx (1)
58-67: Replace hardcoded alt text with dynamic content.The alt attribute is hardcoded to
'Sample Title', which doesn't describe the actual image content. This impacts accessibility for screen reader users.♻️ Proposed fix
<LazyImage src={convertFileSrc( image.thumbnailPath || image.path || '/placeholder.svg', )} - alt={'Sample Title'} + alt={image.name || image.path.split('/').pop() || 'Image thumbnail'} className={cn( 'h-full w-full object-cover transition-transform group-hover:scale-105', isSelected ? 'opacity-95' : '', )} />
🧹 Nitpick comments (2)
frontend/src/components/Media/MediaThumbnails.tsx (1)
120-125: Remove trailing whitespace.Line 124 contains only whitespace. Clean it up for consistency.
🧹 Proposed fix
<LazyImage src={convertFileSrc(image.thumbnailPath) || '/placeholder.svg'} alt={`thumbnail-${index}`} className="h-full w-full object-cover" - />frontend/src/components/LazyImage/LazyImage.tsx (1)
8-8: Remove unused placeholder prop.The
placeholderprop is defined in the interface and destructured but never used in the component's rendering logic. The component uses a generic skeleton loader instead.♻️ Proposed fix
Remove the unused prop from the interface and function signature:
interface LazyImageProps { src: string; alt: string; className?: string; - placeholder?: string; rootMargin?: string; threshold?: number; } export function LazyImage({ src, alt, className = "", - placeholder, rootMargin = "200px", threshold = 0.1, }: LazyImageProps) {If you plan to use custom placeholder images in the future, you can keep the prop and implement it like this:
{/* Skeleton loader */} {!isLoaded && !hasError && ( - <div className="absolute inset-0 animate-pulse bg-muted" /> + placeholder ? ( + <img src={placeholder} alt="" className="absolute inset-0 h-full w-full object-cover" /> + ) : ( + <div className="absolute inset-0 animate-pulse bg-muted" /> + ) )}Also applies to: 17-17
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/components/LazyImage/LazyImage.test.tsxfrontend/src/components/LazyImage/LazyImage.tsxfrontend/src/components/Media/ImageCard.tsxfrontend/src/components/Media/MediaThumbnails.tsxfrontend/src/hooks/useIntersectionObserver.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/Media/MediaThumbnails.tsx (1)
frontend/src/components/LazyImage/LazyImage.tsx (1)
LazyImage(13-61)
frontend/src/components/LazyImage/LazyImage.tsx (1)
frontend/src/hooks/useIntersectionObserver.ts (1)
useIntersectionObserver(12-42)
frontend/src/components/Media/ImageCard.tsx (1)
frontend/src/components/LazyImage/LazyImage.tsx (1)
LazyImage(13-61)
🔇 Additional comments (4)
frontend/src/components/Media/ImageCard.tsx (1)
10-10: LGTM! LazyImage import is correct.The import statement properly references the new LazyImage component.
frontend/src/components/Media/MediaThumbnails.tsx (1)
3-3: LGTM! LazyImage import is correct.The import properly references the new LazyImage component for thumbnail lazy loading.
frontend/src/hooks/useIntersectionObserver.ts (1)
1-42: LGTM! Hook implementation is correct and efficient.The
useIntersectionObserverhook correctly:
- Observes the element when it exists and hasn't been seen yet
- Sets visibility and unobserves immediately on intersection (one-time trigger)
- Cleans up the observer on unmount
- Uses appropriate default values (200px margin, 0.1 threshold)
The dependency array including
isVisibleis intentional and safe: whenisVisiblebecomes true, the effect re-runs but returns early, and the previous observer is properly disconnected in cleanup.frontend/src/components/LazyImage/LazyImage.tsx (1)
30-60: LGTM! LazyImage rendering logic is well-structured.The component correctly:
- Shows a skeleton placeholder while the image loads
- Fades in the image smoothly once loaded (opacity transition)
- Handles errors with a clear message
- Uses absolute positioning to overlay states without layout shift
The combination of
isVisible,isLoaded, andhasErrorstates ensures proper visual feedback at each stage.
|
|
2 similar comments
|
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @frontend/src/components/Media/MediaThumbnails.tsx:
- Around line 120-124: The src fallback is ineffective because
convertFileSrc(image.thumbnailPath) always returns a truthy string; update the
LazyImage src to compute the fallback before conversion by passing thumbnailPath
|| '/placeholder.svg' into convertFileSrc (i.e., call convertFileSrc with the
fallback applied to image.thumbnailPath) so the placeholder is used when
thumbnailPath is empty, keeping the change localized to the LazyImage prop where
convertFileSrc and image.thumbnailPath are referenced; verify this aligns with
LazyImage's new error UX that shows "Failed to load image" on errors.
🧹 Nitpick comments (1)
frontend/src/components/Media/ImageCard.tsx (1)
62-68: Simplify alt text extraction and remove empty lines.The dynamic alt text is a great accessibility improvement over the static 'Sample Title'. However, the logic can be simplified, and the trailing empty lines (67-68) should be removed.
♻️ Simplified implementation
- alt={ - image.path - ? image.path.split('/').pop() ?? 'Image thumbnail' - : 'Image thumbnail' -} - - + alt={image.path?.split('/').pop() ?? 'Image thumbnail'} className={cn(
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/LazyImage/LazyImage.tsxfrontend/src/components/Media/ImageCard.tsxfrontend/src/components/Media/MediaThumbnails.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/LazyImage/LazyImage.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/Media/ImageCard.tsx (1)
frontend/src/components/LazyImage/LazyImage.tsx (1)
LazyImage(12-60)
frontend/src/components/Media/MediaThumbnails.tsx (1)
frontend/src/components/LazyImage/LazyImage.tsx (1)
LazyImage(12-60)
🔇 Additional comments (3)
frontend/src/components/Media/MediaThumbnails.tsx (1)
3-3: LGTM! Clean import of LazyImage component.The relative import path is correct and follows the project structure.
frontend/src/components/Media/ImageCard.tsx (2)
10-10: LGTM! Consistent import pattern.The LazyImage import follows the same pattern as MediaThumbnails and is correctly placed.
58-73: Verify convertFileSrc fallback behavior.The LazyImage integration preserves the existing hover effects and styling correctly. However, similar to MediaThumbnails, verify the fallback logic:
src={convertFileSrc( image.thumbnailPath || image.path || '/placeholder.svg', )}The three-level fallback looks robust, but if
thumbnailPathorpathcontain empty strings (rather thannull/undefined), those empty strings will be passed toconvertFileSrc. Confirm thatconvertFileSrchandles empty strings appropriately and returns a falsy value that triggers the fallback chain.Also note that the error handling UX has changed: instead of showing a placeholder image on load errors, LazyImage now displays "Failed to load image" text.
The verification script from the MediaThumbnails review will help confirm
convertFileSrcbehavior patterns across the codebase.
| <LazyImage | ||
| src={convertFileSrc(image.thumbnailPath) || '/placeholder.svg'} | ||
| alt={`thumbnail-${index}`} | ||
| className="h-full w-full object-cover" | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find convertFileSrc definition
echo "=== Searching for convertFileSrc definition ==="
rg -n "export.*convertFileSrc|function convertFileSrc" --type=ts --type=tsx
# Search in common locations
echo -e "\n=== Checking common utility files ==="
fd "convertFileSrc|file.*src" -e ts -e tsx | head -20
# Search for imports of convertFileSrc
echo -e "\n=== Searching for convertFileSrc imports ==="
rg "import.*convertFileSrc" --type=ts --type=tsx | head -20Repository: AOSSIE-Org/PictoPy
Length of output: 267
🏁 Script executed:
#!/bin/bash
# Adjust search - don't use tsx file type
echo "=== Searching for convertFileSrc definition ==="
rg -n "export.*convertFileSrc|function convertFileSrc"
echo -e "\n=== Find all .tsx and .ts files containing convertFileSrc ==="
rg -l "convertFileSrc" | head -20
echo -e "\n=== Check MediaThumbnails.tsx imports ==="
head -30 frontend/src/components/Media/MediaThumbnails.tsx
echo -e "\n=== Search for convertFileSrc usage patterns ==="
rg -B2 -A2 "convertFileSrc" | head -50Repository: AOSSIE-Org/PictoPy
Length of output: 4836
🏁 Script executed:
#!/bin/bash
# Check LazyImage component
echo "=== LazyImage component implementation ==="
find . -name "LazyImage*" -type f
cat ./frontend/src/components/LazyImage/LazyImage.tsx
echo -e "\n=== Check ImageCard pattern more closely ==="
rg -B5 -A5 "thumbnailPath || image.path" frontend/src/components/Media/ImageCard.tsx
echo -e "\n=== Check if there are any comments about convertFileSrc behavior ==="
rg -B3 -A3 "convertFileSrc" frontend/src/hooks/UseVideos.tsRepository: AOSSIE-Org/PictoPy
Length of output: 5878
🏁 Script executed:
#!/bin/bash
# Search for any test files that might show convertFileSrc behavior
echo "=== Search for convertFileSrc tests or edge cases ==="
find . -name "*.test.*" -o -name "*.spec.*" | xargs grep -l "convertFileSrc" 2>/dev/null
echo -e "\n=== Check if MediaThumbnails or related files have any error handling ==="
rg -B10 -A10 "convertFileSrc.*placeholder" frontend/src/components/Media/MediaThumbnails.tsx
echo -e "\n=== Search for any validation of paths before convertFileSrc ==="
rg -B5 "convertFileSrc" frontend/src/components/Media/MediaThumbnails.tsx | head -40
echo -e "\n=== Check if there's any conditional logic around thumbnailPath ==="
rg "thumbnailPath" frontend/src/components/Media/MediaThumbnails.tsx -B2 -A2Repository: AOSSIE-Org/PictoPy
Length of output: 1636
🌐 Web query:
Tauri convertFileSrc behavior empty string null invalid path return value
💡 Result:
Short answer
- convertFileSrc simply encodes the given string path and returns a protocol URL (e.g. asset://<url‑encoded‑path> on Unix, http(s)://asset.localhost/<url‑encoded‑path> on Windows). It does not check that the file exists. [1][2]
- Because it only transforms the input string, calling it on an already‑converted URL will double‑encode/ nest the protocol (e.g. asset://asset%3A%2F%2F...); this is a known source of bugs. [3]
- The API always returns a string URL for the supplied input; invalid filesystem paths produce a valid (but pointing‑nowhere) asset URL rather than an error. The function expects a string input — passing non‑strings (null/undefined) is a type mismatch in JS/TS and can produce unexpected results (coercion to "null"/"undefined" or a runtime error depending on usage). [1][2][3]
Sources
- convertFileSrc doc in Tauri API (description, params, return type). [1]
- Rust/wasm wrappers and tauri_sys docs showing convert_file_src semantics (returns Url; no existence check). [2]
- GitHub issue describing double‑conversion problem and examples. [3]
References
[1] Tauri convertFileSrc (API docs).
[2] tauri convert_file_src (tauri_sys / tauri-wasm docs).
[3] tauri-apps/tauri issue: "Avoid calling convertFileSrc twice" (discussing double‑encoding).
Simplify the fallback logic for consistency and clarity.
The fallback pattern convertFileSrc(image.thumbnailPath) || '/placeholder.svg' is ineffective because convertFileSrc always returns a truthy string value (a protocol URL like asset://), even for invalid or empty paths. The || '/placeholder.svg' branch will never execute.
For consistency with the pattern used elsewhere in the codebase (e.g., ImageCard), place the fallback before converting:
src={convertFileSrc(image.thumbnailPath || '/placeholder.svg')}This ensures that if thumbnailPath is empty or invalid, the placeholder is used before conversion.
Note: The error handling UX has changed—LazyImage now displays "Failed to load image" text on load errors rather than swapping to the placeholder image. Ensure this aligns with your design requirements.
🤖 Prompt for AI Agents
In @frontend/src/components/Media/MediaThumbnails.tsx around lines 120 - 124,
The src fallback is ineffective because convertFileSrc(image.thumbnailPath)
always returns a truthy string; update the LazyImage src to compute the fallback
before conversion by passing thumbnailPath || '/placeholder.svg' into
convertFileSrc (i.e., call convertFileSrc with the fallback applied to
image.thumbnailPath) so the placeholder is used when thumbnailPath is empty,
keeping the change localized to the LazyImage prop where convertFileSrc and
image.thumbnailPath are referenced; verify this aligns with LazyImage's new
error UX that shows "Failed to load image" on errors.
1 participant
📋 Summary
This PR implements lazy loading for gallery images to improve performance, reduce memory usage, and enhance the user experience for large image collections.
✨ What’s Changed
useIntersectionObserverhookLazyImagecomponent with:ImageCardMediaThumbnails🚀 Benefits
🧪 Testing
🔗 Related Issue
Fixed #998
📝 Notes
LazyImageSummary by CodeRabbit
New Features
Changes
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.