fix(files): report real upload progress (XHR with progress events)#333
Open
shukiv wants to merge 2 commits into
Open
fix(files): report real upload progress (XHR with progress events)#333shukiv wants to merge 2 commits into
shukiv wants to merge 2 commits into
Conversation
The Files page UI sat at 0% throughout an upload because uploadBlob() uses fetch(), which does not surface upload progress events. The store set loaded=0 before the call and loaded=file.size after it, so users saw the progress bar jump from 0% straight to 100% on completion -- and on slow connections (or large files) it appeared frozen. Switch uploadBlob() to XHR when the caller passes onProgress or an AbortSignal, so progress events from xhr.upload.onprogress can drive the UI. Callers that don't pass either keep the fetch path so we preserve the existing 401-retry behaviour in authenticatedFetch(). Wire the file store to pass both onProgress (updates uploadProgress in real time) and the existing AbortController's signal (so cancel now actually aborts the network request, not just the post-upload createFileNode step). uploadBlob() is part of IJMAPClient so the signature change is also applied to the demo client (synthesises 0% then 100%).
Resolved conflict in lib/jmap/client.ts around uploadBlob():
* Upstream added a second positional accountId argument to support
multi-account JMAP setups.
* This branch added an opts bag for progress callback + AbortSignal.
Combined the two by accepting either signature:
uploadBlob(file, accountId) // upstream form
uploadBlob(file, { accountId, onProgress, signal }) // new form
Existing call sites compile unchanged; the new fields are reachable
via the options object. client-interface.ts updated to match.
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
1 participant
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
On the Files page, uploading a large file shows the progress bar stuck at 0% for the entire upload, then it jumps to 100% on completion. On slow connections or large files the UI looks frozen even though bytes are flowing on the wire.
Root cause:
JMAPClient.uploadBlob()usesfetch(), andfetch()does not expose upload progress events (no equivalent toxhr.upload.onprogress). Thefile-storesetloaded: 0before callinguploadBlob, thenloaded: file.sizeafter it resolved, so there were no intermediate updates.The repo already uses the XHR-with-progress pattern in
lib/webdav/client.ts:138, so this PR mirrors that approach for JMAP blob uploads.Change
IJMAPClient.uploadBlob(file)→uploadBlob(file, opts?: { onProgress?, signal? }). Backwards-compatible:optsis optional.JMAPClient: when the caller passesonProgressorsignal, route through a new privatexhrUpload()helper that usesXMLHttpRequestand wiresxhr.upload.onprogress→opts.onProgress. Otherwise the fetch path is unchanged so we keep the existing 401-retry behaviour inauthenticatedFetch()for non-Files callers.DemoJMAPClient.uploadBlob: same signature, synthesises a 0% then 100% callback for parity.stores/file-store.ts(uploadFile+uploadFiles): passes bothonProgress: (loaded, total) => set({ uploadProgress: { ..., loaded, total } }), andsignal: abortController.signal, so Cancel now actually aborts the network request instead of only short-circuiting the post-uploadcreateFileNodestep.fetch()does have a streaming option (ReadableStreambody +duplex: 'half'), but browser support is still inconsistent enough (Safari) that XHR is the safer choice for upload progress today.Verification
npx tsc --noEmitcleannpx eslinton the four changed files cleanjmap-client-resilience.test.ts: 16/16)Files
lib/jmap/client-interface.ts— signaturelib/jmap/client.ts— XHR path +xhrUploadhelperlib/demo/demo-client.ts— signaturestores/file-store.ts— wireonProgress+signal