Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ui): prevent removeThumbnail from desyncing thumbnail array in bulkUpload #11596

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akhrarovsaid
Copy link
Contributor

@akhrarovsaid akhrarovsaid commented Mar 7, 2025

What?

This PR fixes a pair of issues around thumbnails and their removal in the BulkUpload component. Namely:

  1. Adding non-image files followed by an image files simultaneously in the BulkUpload drawer causes a runtime error due to the Thumbnail component potentially rendering an img element with an empty string, which, as the error points out, could cause Next to redownload the whole page supposedly.

  2. Adding non-image files with images simultaneously in the BulkUpload drawer and then removing them via the XIcon button causes the thumbnails array to desync, showing incorrect thumbnails for subsequent forms. This is because the thumbnailUrlsRef array is sparse and using .filter on a sparse array omits empty holes in the array, causing it to desync.

Why?

To properly preserve the thumbnail order and show the correct thumbnails for existing forms in the BulkUpload drawer, and to prevent a runtime error.

How?

  1. Solved by checking if src is truthy.

  2. Solved by preserving the sparseness of the array containing the thumbnail urls instead of using .filter. This PR also changes how the reducer adds new forms to the BulkUpload component as the existing way added new forms in the beginning of the array. This is problematic as thumbnails are generated and added sequentially to the array potentially causing another area of desynchronization between these two arrays.

Before

Media-bulk-before--Payload.webm

After

Media-bulk-after--Payload.webm

Notes:

  • In general, I try to avoid sparse arrays. I think one was used here to minimize memory usage in a component that might need to generate many thumbnails, which makes sense to me. However, synchronizing sparse and dense arrays come with a few gotchas. It might be worth exploring swapping out the thumbnail array for a Map<string, string> to map filenames to their urls, or permitting the thumbnail array to be dense.

If this array was dense, then the fix for desynchronizing thumbnails becomes very simple. You would populate non-image indexes with undefined in the thumbnailUrlsRef array instead of skipping them.

  • Initially, when implementing removeThumbnails, I didn't realize that thumbnailUrlsRef was indeed sparse. My mistake.
  • Test coverage here should be considered, I can add this if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant