fix: @uppy/retriever state reset on reload #5955
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Murderlon
reviewed
Sep 16, 2025
Member
Murderlon
left a comment
Murderlon
left a comment
There was a problem hiding this comment.
Since this used to work I can't help but feel we are conditionally patching at the end of the logic chain instead of the root cause.
You even describe it yourself:
complete event immediately fired with empty successful array
Why not look into removing that?
Contributor
|
I’m working on a fix for this in my (not yet pushed) branch |
Collaborator
Author
|
Closing this as @mifi already working on a refactor which would fix this. |
mifi
added a commit
that referenced
this pull request
Sep 17, 2025
- clean up stored files on `complete` event *only* if *all* files succeeded (no failed files). this allows the user to retry failed files if they get interrupted - fixes #5927, closes #5955 - merge methods `#restoreBlobs` and `#restoreState` and `#onBlobsLoaded` into `#restore` - simplify the logic here - this also allow removing instance variable `#savedPluginData` - only set `isGhost` for non-successful files. it doesn't make sense for successfully uploaded files to be ghosted because they're already done. #5930 - rename `state-update` event handler `saveFilesStateToLocalStorageThrottled` to `handleStateUpdateThrottled` - rename `file-removed` event handler `removeBlobFromStores` to `handleFileRemoved` - rename `complete` event handler `handleComplete` to `handleUploadComplete` - add `upload-success` event handler `handleFileUploaded`: this handler will remove blobs of files that have successfully uploaded. this prevents leaking blobs when an upload with multiple files gets interrupted (but some files have uploaded successfully), because `#handleUploadComplete` (which normally does the cleanup) doesn't get called untill *all* files are complete. - fix `#replaceBlobInStores` potential race condition: it would delete and add at the same time (without first awaiting delete operation) - don't double `setState` in `#restore` - don't add `isRestored: true` in `saveFilesStateToLocalStorage` - this is redundant because we already add it in `#restore` - simplify `filesToSaveWithoutData`: values `null` are invalid for `data` and `preview` - remove the props instead - improve types in golden retriever and MetaDataStore - MetaDataStore: move old state expiry to from `constructor` to `load()`
mifi
added a commit
that referenced
this pull request
Oct 17, 2025
Probably best reviewed commit by commit. I also split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile Also: - Removed the TagFile type - Don't re-upload completed files - fixes #5930 - Clean up stored files on `complete` event *only* if *all* files succeeded (no failed files). this allows the user to retry failed files if the browser & upload get interrupted - fixes #5927, closes #5955 - Only set `isGhost` for non-successful files. it doesn't make sense for successfully uploaded files to be ghosted because they're already done. #5930 fixes #6013 --------- Co-authored-by: Prakash <[email protected]>
Merged
Murderlon
pushed a commit
that referenced
this pull request
Oct 17, 2025
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Make `file.data` nullable - Because for ghosts it will be `undefined` and we don't have any type to distinguish ghosts from other (local) files. This caused a crash, because we didn't check for `undefined` everywhere (when trying to store a blob that was `undefined`). This means we have to add null checks in some packages - Split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 6a60ee5: Reject request early instead of crashing on missing `filename` for /s3/multipart and /s3/params endpoints ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Make `file.data` nullable - Because for ghosts it will be `undefined` and we don't have any type to distinguish ghosts from other (local) files. This caused a crash, because we didn't check for `undefined` everywhere (when trying to store a blob that was `undefined`). This means we have to add null checks in some packages - Move `restore-confirmed` from `onUploadStart` event listener to `startUpload`, else it would cause `restore-confirmed` to be triggered even if there is no `recoveredState` to recover - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Make `file.data` nullable - Because for ghosts it will be `undefined` and we don't have any type to distinguish ghosts from other (local) files. This caused a crash, because we didn't check for `undefined` everywhere (when trying to store a blob that was `undefined`). This means we have to add null checks in some packages - Split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Make `file.data` nullable - Because for ghosts it will be `undefined` and we don't have any type to distinguish ghosts from other (local) files. This caused a crash, because we didn't check for `undefined` everywhere (when trying to store a blob that was `undefined`) - Introduce new field `progress`.`complete`: if there is a post-processing step, set it to `true` once post processing is complete. If not, set it to `true` once upload has finished. - Throw a proper `Nonexistent upload` error message if trying to upload a non-existent upload, instead of TypeError - Rewrite `Uppy.upload()` - this fixes two bugs: 1. No more duplicate emit call when this.#restricter.validateMinNumberOfFiles throws (`#informAndEmit` and `this.emit('error')`) 2. 'restriction-failed' now also gets correctly called when `checkRequiredMetaFields` check errors. - Don't re-upload completed files #5930 - Split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile - Remove TagFile type - Use UppyFile instead. - Make `name` required on UppyFile (it is in reality always set) - Fix bug: `RestrictionError` sometimes thrown with a `file` property that was _not_ a `UppyFile`, but a `File`. This would happen if someone passed a `File` instead of a `MinimalRequiredUppyFile` into `core.addFile` (which is valid to do according to our API) - Improve some log messages - Simplify Uppy `postprocess-complete` handler - Updated dependencies [0c16fe4] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Remove `restore-canceled` event as it was not being used. - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - **Internal inter-package breaking change:** Remove hacky internal event `restore:get-data` that would send a function as its event data (to golden retriever for it to call the function to receive data from it). Add instead `restore:plugin-data-changed` that publishes data when it changes. This means that **old versions of `@uppy/transloadit` are not compatible with newest version of `@uppy/golden-retriever` (and vice versa)**. - Large internal refactor of Golden Retriever - Use `state-update` handler to trigger save to local storage and blobs, instead of doing it in various other event handlers (`complete`, `upload-success`, `file-removed`, `file-editor:complete`, `file-added`). this way we don't miss any state updates. also simplifies the code a lot. this fixes: - Always store blob when it changes - this fixes the bug when using the compressor plugin, it would store the uncompressed original blob (like when using image editor plugin) - Add back throttle: but throttling must happen on the actual local storage save calls inside MetaDataStore, _not_ the handleStateUpdate function, so we don't miss any state updates (and end up with inconsistent data). Note that there is still a race condition where if the user removes a file (causing the blob to be deleted), then quickly reloads the page before the throttled save has happened, the file will be restored but the blob will be missing, so it will become a ghost. this is probably not a big problem though. need to disable throttling when running tests (add it as an option to the plugin) - Fix implicit `any` types in #restore filesWithBlobs - Don't error when saving indexedDB file that already exists (make it idempotent) - Fix bug: Golden Retriever was not deleting from IndexedDbStore if ServiceWorkerStore exists, causing a storage leak - Remove unused Golden Retriever cleanup.ts - Clean up stored files on `complete` event _only_ if _all_ files succeeded (no failed files). this allows the user to retry failed files if they get interrupted - fixes #5927, closes #5955 - Only set `isGhost` for non-successful files - it doesn't make sense for successfully uploaded files to be ghosted because they're already done. #5930 - Add `upload-success` event handler `handleFileUploaded`: this handler will remove blobs of files that have successfully uploaded. this prevents leaking blobs when an upload with multiple files gets interrupted (but some files have uploaded successfully), because `#handleUploadComplete` (which normally does the cleanup) doesn't get called untill _all_ files are complete. - Fix `file-editor:complete` potential race condition: it would delete and add at the same time (without first awaiting delete operation) - Fix: Don't double `setState` when restoring - Improve types in golden retriever and MetaDataStore - MetaDataStore: move old state expiry to from `constructor` to `load()` - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Make `file.data` nullable - Because for ghosts it will be `undefined` and we don't have any type to distinguish ghosts from other (local) files. This caused a crash, because we didn't check for `undefined` everywhere (when trying to store a blob that was `undefined`). This means we have to add null checks in some packages - Split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Rename `getTagFile` to `companionFileToUppyFile` - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Make `file.data` nullable - Because for ghosts it will be `undefined` and we don't have any type to distinguish ghosts from other (local) files. This caused a crash, because we didn't check for `undefined` everywhere (when trying to store a blob that was `undefined`). This means we have to add null checks in some packages - Split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - **Internal inter-package breaking change:** Remove hacky internal event `restore:get-data` that would send a function as its event data (to golden retriever for it to call the function to receive data from it). Add instead `restore:plugin-data-changed` that publishes data when it changes. This means that **old versions of `@uppy/transloadit` are not compatible with newest version of `@uppy/golden-retriever` (and vice versa)**. - Minor internal refactoring in order to make sure that we will always emit `restore:plugin-data-changed` whenever assembly state changes - Split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Make `file.data` nullable - Because for ghosts it will be `undefined` and we don't have any type to distinguish ghosts from other (local) files. This caused a crash, because we didn't check for `undefined` everywhere (when trying to store a blob that was `undefined`). This means we have to add null checks in some packages - Split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Make `file.data` nullable - Because for ghosts it will be `undefined` and we don't have any type to distinguish ghosts from other (local) files. This caused a crash, because we didn't check for `undefined` everywhere (when trying to store a blob that was `undefined`). This means we have to add null checks in some packages - Split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] ## @uppy/[email protected] ### Patch Changes - 0c16fe4: - Make `file.data` nullable - Because for ghosts it will be `undefined` and we don't have any type to distinguish ghosts from other (local) files. This caused a crash, because we didn't check for `undefined` everywhere (when trying to store a blob that was `undefined`) - Introduce new field `progress`.`complete`: if there is a post-processing step, set it to `true` once post processing is complete. If not, set it to `true` once upload has finished. - Throw a proper `Nonexistent upload` error message if trying to upload a non-existent upload, instead of TypeError - Rewrite `Uppy.upload()` - this fixes two bugs: 1. No more duplicate emit call when this.#restricter.validateMinNumberOfFiles throws (`#informAndEmit` and `this.emit('error')`) 2. 'restriction-failed' now also gets correctly called when `checkRequiredMetaFields` check errors. - Don't re-upload completed files #5930 - Split UppyFile into two intefaces distinguished by the `isRemote` boolean: - LocalUppyFile - RemoteUppyFile - Remove TagFile type - Use UppyFile instead. - Make `name` required on UppyFile (it is in reality always set) - Fix bug: `RestrictionError` sometimes thrown with a `file` property that was _not_ a `UppyFile`, but a `File`. This would happen if someone passed a `File` instead of a `MinimalRequiredUppyFile` into `core.addFile` (which is valid to do according to our API) - Improve some log messages - Simplify Uppy `postprocess-complete` handler ## [email protected] ### Patch Changes - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - Updated dependencies [0c16fe4] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] - @uppy/[email protected] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this fixes #5927
Root Cause :
AbortControllercompleteevent immediately fired with emptysuccessfularrayhandleCompletetriggers cleanup → localStorage clearedBefore:
gr_bug.mov
After:
gr_fix.mov