feat: upload input v3 image preview improvement#263
Conversation
|
Warning Review limit reached
More reviews will be available in 50 minutes and 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates upload thumbnail previews to create and cache blob URLs for new image uploads, render local URLs synchronously in ChangesBlob URL Preview Caching and Lifecycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Around line 182-194: The current preview assignment uses FIFO shift on
pendingPreviewsRef.current and assumes server-returned value order matches
upload order, causing mis-matches; update the logic to match previews to files
by a stable identifier instead of insertion order: ensure preview entries in
pendingPreviewsRef include the original client filename (e.g.,
preview.originalName or preview.clientFilename) at enqueue time, build a map
from that original name to dataURL, then iterate newFiles (from value) and for
each file try to match by comparing f.filename or f.originalName to the preview
map key (and use a heuristic like contains/endsWith if the server adds
prefixes); assign matched previews into updates and only fall back to shift()
for any remaining unmatched files; update the code paths around
pendingPreviewsRef, newFiles, and setFilePreviews to use this name-based
matching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d9718e3-5485-4d80-a2ed-9b8ff5300c12
📒 Files selected for processing (3)
src/components/inputs/upload-input-v3/dropzone-v3.jssrc/components/inputs/upload-input-v3/index.jssrc/components/progressive-img/index.js
priscila-moneo
force-pushed
the
feature/upload-input-v3-image-preview-improvement
branch
from
4610604 to
32e967f
Compare
| // Once the parent updates value, remove all completed files from uploadingFiles | ||
| useEffect(() => { | ||
| // Once the parent updates value, remove completed uploading files and assign local previews to new files | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
I don't see the use case for this change, can you explain ?
There was a problem hiding this comment.
useLayoutEffect runs synchronously before the browser paints, so when value updates, the completed uploading row is removed before the user sees anything. With useEffect it runs after paint, causing a one-frame flash where the file appears twice — once as "Complete" in the uploading section and once in the committed value section.
There was a problem hiding this comment.
this just hides the symptom but does not address the re-render . which is caused by this
There was a problem hiding this comment.
Done the refactor you mention and prevValueRef, pendingPreviewsRef and the batched matching are all gone. But useLayoutEffect is still needed though: when value updates, both the completed uploading row and the new value row land in the same render, and without it the user sees them both for one frame before the cleanup runs. It's not hiding a symptom, it's preventing a real visual duplicate.
There was a problem hiding this comment.
I'm working on the last of your comments and will update this PR with the latest code, will tag you so you can review it.
There was a problem hiding this comment.
code already updated
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/inputs/upload-input-v3/index.js (1)
210-214:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreview mapping is still ambiguous for same-size concurrent uploads.
At Line 212, matching uses only
response.size, so two different files with identical sizes can receive swapped previews. Please map with a stable per-file identifier (client upload id echoed by server, or an explicit correlation token) instead of size-only matching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/inputs/upload-input-v3/index.js` around lines 210 - 214, The current preview-assignment logic in the setUploadingFiles callback uses response.size to match upload entries (response.name/response.size and entry.previewUrl), which causes wrong previews for concurrent same-size files; change the protocol to use a stable per-file identifier (e.g. response.uploadId or response.correlationId) returned by the server and store that id on the client-side upload entries, then in setUploadingFiles find the entry by that uploadId instead of size and call setFilePreviews with the stable key (e.g. setFilePreviews(p => ({ ...p, [response.uploadId]: entry.previewUrl }))). Update any code that creates upload entries to include the client upload id so matching is deterministic.
🧹 Nitpick comments (2)
src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js (2)
350-358: ⚡ Quick winPreserve and restore original
URLmethods instead of deleting globals.At Line 356–Line 357, deleting global methods can pollute other tests. Store originals and restore them in
afterEachfor isolation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js` around lines 350 - 358, The tests are deleting global URL methods which can leak into other tests; modify the beforeEach/afterEach so beforeEach saves the originals (e.g., const originalCreateObjectURL = URL.createObjectURL; const originalRevokeObjectURL = URL.revokeObjectURL) then mocks with jest.fn in beforeEach, and in afterEach restore them (URL.createObjectURL = originalCreateObjectURL; URL.revokeObjectURL = originalRevokeObjectURL) instead of using delete; update the beforeEach/afterEach in upload-input-v3.test.js around the existing beforeEach/afterEach blocks referencing URL.createObjectURL and URL.revokeObjectURL.
414-434: ⚡ Quick winAdd a collision test for parallel uploads with identical file sizes.
Current coverage validates out-of-order responses but only with unique sizes. Add a case where two image uploads have the same
sizeto catch preview mis-assignment regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js` around lines 414 - 434, Add a new unit test alongside the existing "correctly maps previews for parallel uploads using response size" that reproduces a collision where two uploads have identical size: use the same pattern of calling dropzoneCallbacks.onAddedFile for two distinct filenames (e.g., 'sunset.jpg' and 'portrait.jpg') with identical size values, call dropzoneCallbacks.onFileCompleted for both, then simulate server responses in reverse order via dropzoneCallbacks.onUploadComplete with server filenames (e.g., '246_portrait_abc123.jpg' and '246_sunset_def456.jpg'), rerender UploadInputV3 with value containing those server filenames and sizes, and assert that screen.getByRole('img', { name: ... }) returns the expected blob src mapping to the original preview names (e.g., 'blob:portrait.jpg' and 'blob:sunset.jpg') to catch preview mis-assignment when sizes collide.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Line 70: Add a useEffect that revokes cached blob URLs on unmount and whenever
filePreviews changes: implement useEffect(() => { return () => {
Object.values(filePreviews).forEach(url => { if (url) URL.revokeObjectURL(url)
}) } }, [filePreviews]); this ensures any object URLs created earlier (look for
places that call URL.createObjectURL in the upload preview code around the
filePreviews setter) are revoked; also ensure any code path that replaces an
individual preview calls URL.revokeObjectURL on the old URL before overwriting
filePreviews via setFilePreviews.
---
Duplicate comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Around line 210-214: The current preview-assignment logic in the
setUploadingFiles callback uses response.size to match upload entries
(response.name/response.size and entry.previewUrl), which causes wrong previews
for concurrent same-size files; change the protocol to use a stable per-file
identifier (e.g. response.uploadId or response.correlationId) returned by the
server and store that id on the client-side upload entries, then in
setUploadingFiles find the entry by that uploadId instead of size and call
setFilePreviews with the stable key (e.g. setFilePreviews(p => ({ ...p,
[response.uploadId]: entry.previewUrl }))). Update any code that creates upload
entries to include the client upload id so matching is deterministic.
---
Nitpick comments:
In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js`:
- Around line 350-358: The tests are deleting global URL methods which can leak
into other tests; modify the beforeEach/afterEach so beforeEach saves the
originals (e.g., const originalCreateObjectURL = URL.createObjectURL; const
originalRevokeObjectURL = URL.revokeObjectURL) then mocks with jest.fn in
beforeEach, and in afterEach restore them (URL.createObjectURL =
originalCreateObjectURL; URL.revokeObjectURL = originalRevokeObjectURL) instead
of using delete; update the beforeEach/afterEach in upload-input-v3.test.js
around the existing beforeEach/afterEach blocks referencing URL.createObjectURL
and URL.revokeObjectURL.
- Around line 414-434: Add a new unit test alongside the existing "correctly
maps previews for parallel uploads using response size" that reproduces a
collision where two uploads have identical size: use the same pattern of calling
dropzoneCallbacks.onAddedFile for two distinct filenames (e.g., 'sunset.jpg' and
'portrait.jpg') with identical size values, call
dropzoneCallbacks.onFileCompleted for both, then simulate server responses in
reverse order via dropzoneCallbacks.onUploadComplete with server filenames
(e.g., '246_portrait_abc123.jpg' and '246_sunset_def456.jpg'), rerender
UploadInputV3 with value containing those server filenames and sizes, and assert
that screen.getByRole('img', { name: ... }) returns the expected blob src
mapping to the original preview names (e.g., 'blob:portrait.jpg' and
'blob:sunset.jpg') to catch preview mis-assignment when sizes collide.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 060f9ed7-3bec-49bb-a60b-9791fd9adcfc
📒 Files selected for processing (4)
src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.jssrc/components/inputs/upload-input-v3/index.jssrc/components/progressive-img/__tests__/progressive-img.test.jssrc/components/progressive-img/index.js
✅ Files skipped from review due to trivial changes (1)
- src/components/progressive-img/tests/progressive-img.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/progressive-img/index.js
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
LGTM, thank you @priscila-moneo
|
Memory leak: blob URLs not revoked on unmount
In a long-running SPA session where the user uploads images across multiple visits to this component without a full page reload, each unmount leaks the memory for all previewed images. Suggested fix — add a cleanup effect using a ref to capture latest state at unmount time: const filePreviewsRef = useRef(filePreviews);
filePreviewsRef.current = filePreviews;
const uploadingFilesRef = useRef(uploadingFiles);
uploadingFilesRef.current = uploadingFiles;
useEffect(() => {
return () => {
Object.values(filePreviewsRef.current).forEach(url => URL.revokeObjectURL(url));
uploadingFilesRef.current.forEach(f => f.previewUrl && URL.revokeObjectURL(f.previewUrl));
};
}, []);(Using refs instead of deps on |
|
Zero/undefined Two issues compound each other when a file's 1. const formatFileSize = (bytes) => {
if (!bytes) return '0 KB'; // catches 0, undefined, null, NaN
...
};A file whose 2. The size-based preview matching in const entry = prev.find(f => f.size === response.size && f.previewUrl);If Why these two are connected: the Suggested fix: guard the preview mapping against falsy sizes before attempting the match: const entry = response?.size
? prev.find(f => f.size === response.size && f.previewUrl)
: null;This ensures that a missing or zero size from the server never triggers a spurious preview assignment. |
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please re review
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/upload-input-v3-image-preview-improvement
branch
from
ca65214 to
ee35ca1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Around line 182-185: The upload cleanup in upload-input-v3 is removing every
completed row whenever value changes, which can hide other finished uploads that
the parent has not received yet. Update wrappedOnUploadComplete and the
useLayoutEffect cleanup to track each row’s upload/response identity, then only
clear the specific uploading row whose completed response is actually present in
value. Use the existing uploading row state in upload-input-v3 and the
completion handler logic around wrappedOnUploadComplete to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fd40445-f0a8-473b-a7f9-862a57f894c4
📒 Files selected for processing (4)
src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.jssrc/components/inputs/upload-input-v3/index.jssrc/components/progressive-img/__tests__/progressive-img.test.jssrc/components/progressive-img/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/progressive-img/tests/progressive-img.test.js
- src/components/inputs/upload-input-v3/tests/upload-input-v3.test.js
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/upload-input-v3-image-preview-improvement
branch
from
860645a to
0061872
Compare
3 participants
ref: https://app.clickup.com/t/86ba8wmgh
Summary by CodeRabbit
New Features
Improvements
data:/blob:sources)Tests