Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Deploying drive-web with
|
| Latest commit: |
f5db2fe
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0e3bc913.drive-web.pages.dev |
| Branch Preview URL: | https://feat-pb-7047-photos-thumbnai.drive-web.pages.dev |
|
|
||
| async getThumbnail(photo: Photo, thumbnail: PhotoThumbnail, abortController: AbortController): Promise<Blob> { | ||
| const cachedPreview = await getDatabaseFilePreviewData({ fileId: photo.id }); | ||
| if (cachedPreview?.preview) return cachedPreview.preview; |
There was a problem hiding this comment.
getThumbnail returns the cached blob keyed only by fileId, with no check against the thumbnail id or updatedAt. Once cached, a stale pre-edit thumbnail is served forever, contradicting the PR's own "latest one is used" behavior.
Maybe you can key the cache check on thumbnail id (or compare updatedAt) before returning the cached blob.
| if (cachedPreview?.preview) return cachedPreview.preview; | ||
|
|
||
| const thumbnailBlob = await downloadThumbnail(thumbnail, false, abortController); | ||
| void updateDatabaseFilePreviewData({ |
There was a problem hiding this comment.
updateDatabaseFilePreviewData is fire-and-forget with no .catch. A failed IndexedDB write becomes an unhandled promise rejection instead of going through errorService, and silently disables caching for that photo.
adding .catch(errorService.reportError) should be enough, right? 🤔
| return () => abortController.abort(); | ||
| }, [photo, shouldLoad]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Thumbnail state is split across two useState + two effects (blob, then derived URL), adding an extra render per load. But PublicSharedItemList.tsx's equivalent creates the object URL directly on download resolution 🤔
| const thumbnail = usePhotoThumbnail(photo, isNearViewport); | ||
| const FileIcon = iconService.getItemIcon(false, photo.type); | ||
|
|
||
| const content = { |
There was a problem hiding this comment.
iconService.getItemIcon and all three content branches are computed on every render, even though only one is shown. Wasted work that scales with grid size, it could be avoidable with a switch/ternary on thumbnail.status, right?
…into feat/PB-7047-photos-thumbnails # Conflicts: # src/views/Photos/services/photoFolders.service.ts
|
Description
The photos in the Photos gallery now show their thumbnail instead of the file type icon. A thumbnail starts downloading just before its photo scrolls into view, and the download is cancelled if the user scrolls past it before it finishes. While it loads the tile shows a placeholder. Thumbnails already downloaded are taken from the local previews cache that Drive uses, so they are not downloaded again. If a photo has no thumbnail or it cannot be downloaded, the file type icon is shown. When a photo has several thumbnails (for example after being edited on the phone), the latest one is used.
Drive's thumbnail download can now be cancelled. Drive keeps its current behaviour.
Related Issues
Jira: PB-7047 (parent PB-6876)
Related Pull Requests
Checklist
Testing Process
Additional Notes
The page is still not linked from the sidebar.