feat(uploads-manager): add upload progress and time left to uploads manager#4716
Conversation
|
pgolebiowski seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughAdds opt-in upload ETA tracking with smoothed progress estimates, extends upload item metadata, wires the data into the modernized uploads manager, upgrades its dependency, and adds tests and Storybook coverage. ChangesUpload ETA tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant UploadsManager
participant ContentUploader
participant ETAUtilities
participant ItemMapper
UploadsManager->>ContentUploader: report upload progress
ContentUploader->>ETAUtilities: update loaded bytes and timestamp
ETAUtilities-->>ContentUploader: return smoothed remainingMs
ContentUploader->>ItemMapper: map items with ETA enabled
ItemMapper-->>UploadsManager: return byte progress and ETA metadata
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
pierre-pigeon
changed the title
Co-authored-by: Cursor <cursoragent@cursor.com>
45e3968 to
e9a462a
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-uploader/ContentUploader.tsx (1)
1265-1287: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winGate ETA tracking behind
isUploadEtaEnabled.
handleUploadProgressunconditionally runsupdateEtaand updatesetaByItem/bytesUploaded/totalBytes/remainingMson every progress tick, even whenisUploadEtaEnabledisfalse(the default). OnlymapToModernizedUploadItemhonors the flag by stripping these fields before render, so the UI is correct, but the EMA computation and WeakMap bookkeeping run on every progress event for free — wasted work for the common (flag-off) case, and it also meansContentUploader.test.js'shandleUploadProgress()tests pass without ever settingisUploadEtaEnabled: true, masking that the "kill switch" isn't applied at the source.⚡ Proposed guard
item.progress = Math.min(Math.round((event.loaded / event.total) * 100), 100); item.status = item.progress === 100 ? STATUS_STAGED : STATUS_IN_PROGRESS; - // Track byte-level progress and a smoothed ETA for the modernized manager. - const nextEta = updateEta(this.etaByItem.get(item), event.loaded, event.total, Date.now()); - this.etaByItem.set(item, nextEta); - item.bytesUploaded = event.loaded; - item.totalBytes = event.total; - item.remainingMs = getRemainingMs(nextEta); + // Track byte-level progress and a smoothed ETA for the modernized manager. + if (this.props.isUploadEtaEnabled) { + const nextEta = updateEta(this.etaByItem.get(item), event.loaded, event.total, Date.now()); + this.etaByItem.set(item, nextEta); + item.bytesUploaded = event.loaded; + item.totalBytes = event.total; + item.remainingMs = getRemainingMs(nextEta); + }🤖 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 `@src/elements/content-uploader/ContentUploader.tsx` around lines 1265 - 1287, Gate the ETA-specific work in handleUploadProgress behind the isUploadEtaEnabled prop or configuration flag. Only call updateEta, write etaByItem, and assign bytesUploaded, totalBytes, and remainingMs when the flag is enabled; preserve the existing progress, status, callback, and collection updates for all uploads.
🤖 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.
Outside diff comments:
In `@src/elements/content-uploader/ContentUploader.tsx`:
- Around line 1265-1287: Gate the ETA-specific work in handleUploadProgress
behind the isUploadEtaEnabled prop or configuration flag. Only call updateEta,
write etaByItem, and assign bytesUploaded, totalBytes, and remainingMs when the
flag is enabled; preserve the existing progress, status, callback, and
collection updates for all uploads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef802a16-d441-4cfd-98d2-b454765cdda3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
package.jsonsrc/common/types/upload.jssrc/elements/content-uploader/ContentUploader.tsxsrc/elements/content-uploader/__tests__/ContentUploader.test.jssrc/elements/content-uploader/stories/ContentUploader.stories.jssrc/elements/content-uploader/utils/__tests__/mapToModernizedUploadItem.test.tssrc/elements/content-uploader/utils/__tests__/uploadEta.test.tssrc/elements/content-uploader/utils/mapToModernizedUploadItem.tssrc/elements/content-uploader/utils/uploadEta.ts
pierre-pigeon
changed the title
2 participants
This PR enriches the modernized uploads manager with per-item byte progress and a smoothed time-remaining (ETA) estimate.
ContentUploadernow tracksbytesUploaded/totalBytesfrom upload progress events and computes an EMA-smoothed ETA.Summary by CodeRabbit
New Features
Bug Fixes
Chores