Fix file upload truncation on Safari#2645
Merged
Merged
Conversation
On Safari iOS, XHR with a file-backed Blob as request body sends successfully but never reaches readyState 4 (DONE), so onload never fires and the upload loop stalls after the first chunk. Only the first 20 MB of any file was written. This is a WebKit bug specific to file-backed Blobs as XHR request bodies. Fix: convert each file slice to an ArrayBuffer via blob.arrayBuffer() before passing it to xhr.send(). This uses a different WebKit code path that correctly reaches readyState 4. To avoid a sequential disk-read bottleneck, the next chunk's arrayBuffer() call is started immediately when the current chunk begins sending, so the disk read for chunk N+1 overlaps with the network transfer of chunk N. Tested: Chrome 1 GB upload ~70 MB/s (no regression), Safari iOS confirmed working.
Contributor
WalkthroughThe ChangesUpload performance and reliability
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
mgutt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/Browse.page (1)
1284-1317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor cancellation before chaining the next upload.
cancelis only handled on the same-file continuation path. If the user cancels after a chunk finishes but before this callback advances, the prefetch branch can return withoutstopUpload(), and the next-file branch can still startfiles[index + 1].Suggested fix
xhr.onload = function() { + if (cancel === 1) { + stopUpload(file.name, false); + return; + } + if (xhr.status < 200 || xhr.status >= 300) { stopUpload(file.name, true, 'http:' + xhr.status); return; @@ $('`#dfm_uploadStatus`').html("_(Uploading)_: <span class='dfm_percent'>"+percent+"%</span><span class='dfm_speed'>Speed: "+speed+"</span><span class='orange-text'> ["+(index+1)+'/'+files.length+'] '+escapeHtml(file.name)+"</span>"); - if (cancel === 1) return; + if (cancel === 1) { + stopUpload(file.name, false); + return; + } nextBufferPromise.then(function(nextBuffer) { - if (cancel === 1) return; + if (cancel === 1) { + stopUpload(file.name, false); + return; + } uploadFile(files,index,next,time,nextBuffer); - }).catch(function() { stopUpload(file.name, true, 'read'); }); + }).catch(function() { + if (cancel !== 1) stopUpload(file.name, true, 'read'); + }); } else if (index < files.length-1) { // Clean up temp file for completed upload before starting next file $.post('/webGui/include/Control.php',{mode:'stop',file:encodeURIComponent(file.name)}); uploadFile(files,index+1,0,time);🤖 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 `@emhttp/plugins/dynamix/Browse.page` around lines 1284 - 1317, The callback must honor the cancel flag before chaining further work: in the nextBufferPromise.then(...) handler and in the else-if branch that starts the next file (the branch with uploadFile(files,index+1,0,time)), check if cancel === 1 and call stopUpload(file.name, true) (or otherwise abort) before calling uploadFile or posting the stop temp-file request; likewise, ensure the nextBufferPromise.catch path still calls stopUpload when cancel is set so no subsequent upload starts after cancellation (refer to cancel, nextBufferPromise.then, uploadFile, and stopUpload).
🤖 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 `@emhttp/plugins/dynamix/Browse.page`:
- Around line 1310-1313: The catch handlers on nextBufferPromise call
stopUpload(file.name, true, 'read') but that leaves the UI showing the generic
"File is removed" dialog; update those catch blocks (the ones that call
nextBufferPromise.then(...).catch(...)) to pass a clear, user-facing error
message to stopUpload indicating the read/arrayBuffer failure (e.g., "Failed to
read file data — could not convert to ArrayBuffer") or change stopUpload to
interpret errorType==='read' and display that specific message; apply the same
change to both occurrences that currently call stopUpload(..., 'read').
---
Outside diff comments:
In `@emhttp/plugins/dynamix/Browse.page`:
- Around line 1284-1317: The callback must honor the cancel flag before chaining
further work: in the nextBufferPromise.then(...) handler and in the else-if
branch that starts the next file (the branch with
uploadFile(files,index+1,0,time)), check if cancel === 1 and call
stopUpload(file.name, true) (or otherwise abort) before calling uploadFile or
posting the stop temp-file request; likewise, ensure the nextBufferPromise.catch
path still calls stopUpload when cancel is set so no subsequent upload starts
after cancellation (refer to cancel, nextBufferPromise.then, uploadFile, and
stopUpload).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d5d4b6fb-fd05-43e4-9555-1ad42c250f4d
📒 Files selected for processing (1)
emhttp/plugins/dynamix/Browse.page
Contributor
Author
|
@coderabbitai status |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 21156
📋 PR Status — Fix file upload truncation on Safari
🔍 Open Review Comments (2)
The major cancellation issue is the most important one to address before this is ready to merge. Would you like me to provide a more detailed look at the suggested fix? |
Contributor
Author
|
@coderabbitai |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2338 🏁 Script executed: Length of output: 2511 Let me read the complete relevant block to fully verify. 🏁 Script executed: Length of output: 5602
1. ✅ Cancellation fully honored at every branching point:
2. ✅ Descriptive error message on read failure:
Everything looks good — I'll resolve both review comments now. [resolve] |
Member
|
Note that this fix is for 7.3 only, it does not apply to 7.2 I was unable to repro the initial problem with Safari on macOS, and I don't have iOS to test with. But I can confirm the changes did not cause a regression... after installing the PR plugin on 7.3.0 I was still able to upload 300+MB files using Safari/Chrome on macOS as well as Chrome/Firefox on Windows. LGTM |
elibosley
added a commit
that referenced
this pull request
May 22, 2026
## Summary - Pins `actions/checkout` to the immutable commit currently referenced by `v6.0.2`. - Bounds the no-previous-tag path to `TAG^..TAG` when possible so the workflow does not scan full tag ancestry. - Restricts PR number extraction to explicit merge commit messages like `Merge pull request #123`, avoiding accidental issue references like `(#123)`. - Adds Linear-side GitHub PR link resolution: discovered PR URLs are passed to the sync script, which resolves Linear attachments for those URLs and attaches the linked issues to the release. - Fixes manual `workflow_dispatch` testing against historical tags by checking out the workflow branch while still resolving the requested `tag_name`. - Carries active QA work across OS prereleases: internal tag sync now attaches active issues to the exact prerelease, the stable companion release, and the next planned prerelease. It removes next-prerelease membership only after the issue reaches an internal/completed state. - Sweeps existing issues already attached to the exact prerelease through the same policy, so tickets are carried forward even when they were not mentioned by the latest WebGUI tag diff. ## Review Notes - Valid: mutable checkout tag. Fixed by pinning to `de0fac2e4500dabe0009e67214ff5f5447ce83dd`. - Valid: empty `PREVIOUS_TAG` range walked too much history. Fixed with parent-bounded range and a parentless fallback. - Valid: broad parenthetical PR extraction could treat issue references as PR numbers. Fixed by restricting extraction to `Merge pull request #[0-9]+` while preserving numeric extraction, `sort -u`, and `|| true` behavior. - Follow-up support: if the GitHub PR only links to Linear from the Linear side, `attachmentsForURL` now resolves the issue from the PR URL. - Manual test finding: checking out `tag_name` during `workflow_dispatch` failed for historical tags because the tag did not contain the new script. Fixed by checking out `github.ref` for manual runs. - Skipped: none. All reported findings applied to the current code. ## Validation - `git diff --check` - `node --check .github/scripts/sync-linear-release.mjs` - `ruby -e 'require "yaml"; YAML.load_file(".github/workflows/linear-release.yml"); puts "yaml ok"'` - Shell probe confirmed PR extraction returns `2645` from `Merge pull request #2645 ...` and ignores parenthetical `(#2646)` / `(#123)` references. - Shell probe confirmed the first-tag fallback uses a single-commit style range such as `6.2.2^..6.2.2` when a parent exists. - Live Linear validation: `attachmentsForURL(https://github.com/unraid/webgui/pull/2645)` resolved `OS-239`, and the sync script attached `OS-239` to `7.3.1-rc.0.3` without changing its `QA Ready` state. - Manual workflow test passed: https://github.com/unraid/webgui/actions/runs/26299583415 for `tag_name=7.3.1-rc.0.3`; it synced the existing Linear Release and attached `OS-239` idempotently. - Local Linear validation after carry-forward change: `OS-239` is `QA Ready` and attached to `7.3.1-rc.0.3`, `7.3.1-rc.0.4`, and `Unraid OS 7.3.1 Stable`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Chores * Improved release automation and synchronization between GitHub PRs and Linear issues, ensuring PR links are properly captured and tracked throughout the release cycle * Strengthened issue state management across multiple release versions, providing better visibility into which changes are included in each release * Enhanced overall reliability of the release workflow <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/unraid/webgui/pull/2647?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/unraid/webgui/pull/2647?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
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
4 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.
Fixes: https://product.unraid.net/p/7-3-0-file-manager-large-uploads-truncate-in-web-ui
Problem
On Safari, uploading files via the file manager resulted in only the first 20 MB being written. All subsequent chunks were silently dropped.
Root cause
When
xhr.send()is called with a file-backedBlob, Safari sends the data correctly and receives the server's 200 response, but the XHR never transitions toreadyState 4(DONE). As a result,onloadnever fires, the upload loop never advances to the next chunk, and the connection hangs indefinitely. This is a WebKit bug specific to file-backed Blobs as XHR request bodies.Fix
Convert each file slice to an
ArrayBufferviablob.arrayBuffer()before passing it toxhr.send(). This uses a different WebKit code path that correctly reachesreadyState 4.To avoid a sequential disk-read bottleneck (read chunk N, then send chunk N, then read chunk N+1, ...), the next chunk's
arrayBuffer()call is started immediately when the current chunk begins sending, so the disk read for chunk N+1 overlaps with the network transfer of chunk N.Testing
Summary by CodeRabbit