Repository navigation
fix(share): Do not add a share item when the file transfer failed - #2758
Merged
SystemKeeper merged 2 commits intoOct 14, 2026
Merged
Conversation
prepareFileForUploadingAtURL ignored the result of the move into the temporary directory and only looked at the file coordinator error. If the move failed (no space left, source not removable), an item pointing to a non-existing file was added to the share list. Report the transfer error as a failure, so the existing fallback and the "Failed to prepare file for sharing" path apply. With NSFileCoordinatorReadingWithoutChanges the coordinator hands out the original file, which must not be moved away from its owner anymore, so copy it. The snapshot of NSFileCoordinatorReadingForUploading is still moved, as the coordinator unlinks it after the accessor block. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
…ok edit updateItem:withURL: removed the item's file before moving the edited file in and ignored a failed move, which left the item without a file. Replace the file atomically and log failures. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
Collaborator
|
Thanks for the PR! The changes look good and sane. I assume (based on your last message in the other PR) it's more of a correctness fix than an issue you ran into? |
Contributor
Author
|
It's a correctness fix. On Android we ran into a real problem: the app queried the content provider of shared files on the main thread, and that froze the UI (fixed in nextcloud/talk-android#6857 and related PRs). I then checked whether iOS has the same issue. It doesn't, but while reading that code path I found this one. I haven't hit it on a device. It needs the move to fail (e.g. no free space) or the |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Problem
ShareItemController prepareFileForUploadingAtURL:…moves the file provided by the file coordinator into the temporary directory, but ignores the result ofmoveItemAtPath:and only returns the coordinator error. If the move fails (no space left, source can't be removed, …), the method still reports success andaddItemWithURLAndName:adds aShareItemwhose file does not exist. Preview and upload of that item then fail.Besides, with
NSFileCoordinatorReadingWithoutChanges(the fallback) the URL given to the accessor is the original file. Moving it takes the file away from its owner, or fails if the source is not removable.Fix
Failed to prepare file for sharingpath apply.NSFileCoordinatorReadingForUploading: keep moving, the URL is a snapshot that the coordinator unlinks after the accessor block (Apple documents relocating it within the block).NSFileCoordinatorReadingWithoutChanges: copy the original instead of moving it.updateItem:withURL:(replacing the item's file with the QuickLook edit) has the same defect class, and a worse outcome: it removes the item's file first and then ignores a failedmoveItemAtPath:, so on failure the item loses its file. It now usesreplaceItemAtURL:withItemAtURL:…, which keeps the original until the replacement succeeded, and logs the replace and coordinator errors (second commit).Tests
Added
UnitShareItemControllerTest(item with a local copy for an existing file, no item for a missing file). I could not run the build and tests locally, only CI.AI-assisted: Claude Code (Sonnet 5.5), reviewed by the author.