feat(uploads-manager): add upload progress and time left to uploads manager#4720
feat(uploads-manager): add upload progress and time left to uploads manager#4720pierre-pigeon wants to merge 10 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
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. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughContentUploader now supports optional upload ETA and byte-progress telemetry. ETA state is sampled and smoothed per upload, reset between attempts, and conditionally mapped into modernized upload items. Types, dependency versions, tests, and a Storybook example were updated. ChangesUpload ETA integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant UploadEvent
participant ContentUploader
participant updateEta
participant mapToModernizedUploadItems
participant UploadsManager
UploadEvent->>ContentUploader: loaded and total progress
ContentUploader->>updateEta: update per-item ETA state
updateEta-->>ContentUploader: smoothed remainingMs
ContentUploader->>mapToModernizedUploadItems: upload items and ETA flag
mapToModernizedUploadItems-->>UploadsManager: byte and ETA fields
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)package.jsonTraceback (most recent call last): 🔧 Biome (2.5.3)src/common/types/upload.jsFile contains syntax errors that prevent linting: Line 10: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 13: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 21: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 29: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 31: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 39: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 44: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 49: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 53: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 65: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 84: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 93: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 100: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 104: 'export type' declarations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax. 🔧 ast-grep (0.44.1)src/elements/content-uploader/__tests__/ContentUploader.test.jsast-grep timed out on this file src/elements/content-uploader/stories/ContentUploader.stories.jsast-grep retry budget exhausted before isolating this batch src/elements/content-uploader/utils/__tests__/mapToModernizedUploadItem.test.tsast-grep retry budget exhausted before isolating this batch
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/elements/content-uploader/utils/mapToModernizedUploadItem.ts`:
- Around line 63-65: Update mapToModernizedUploadItem so items with
item.isFolder do not populate totalBytes, bytesUploaded, or remainingMs,
regardless of upload status or ETA configuration. Preserve the existing byte and
ETA calculations for non-folder file items.
- Around line 64-66: Update the remainingMs mapping in mapToModernizedUploadItem
so errored and canceled statuses return undefined rather than forwarding the
stale fileItem.remainingMs value. Preserve the existing ETA for active,
non-terminal upload statuses and keep staged/complete behavior unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7753101d-bae6-4a3b-a09d-255e8550dfac
⛔ 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
| const totalBytes = fileItem.totalBytes ?? item.size; | ||
| const isFullyUploaded = status === 'staged' || status === 'complete'; | ||
| const bytesUploaded = isFullyUploaded && totalBytes != null ? totalBytes : fileItem.bytesUploaded; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether progressCallback / handleUploadProgress is wired for folder uploads
# with real aggregate totals rather than the placeholder size of 1.
rg -n -A10 'progressCallback' src/elements/content-uploader/ContentUploader.tsx
rg -n 'class FolderUpload' -A30 src/elements/content-uploaderRepository: box/box-ui-elements
Length of output: 1242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files in content-uploader:"
git ls-files 'src/elements/content-uploader/*' | sed -n '1,200p'
echo
echo "Locate handleUploadProgress and size/bytes fields:"
rg -n 'handleUploadProgress|totalBytes|bytesUploaded|isUploadEtaEnabled|isFolder|STATUS_IN_PROGRESS|STATUS_STAGED|complete' src/elements/content-uploader
echo
echo "Outline ContentUploader.tsx around relevant lines:"
wc -l src/elements/content-uploader/ContentUploader.tsx
sed -n '540,670p' src/elements/content-uploader/ContentUploader.tsx
echo
sed -n '920,1020p' src/elements/content-uploader/ContentUploader.tsx
echo
echo "Outline mapToModernizedUploadItem.ts:"
wc -l src/elements/content-uploader/utils/mapToModernizedUploadItem.ts
cat -n src/elements/content-uploader/utils/mapToModernizedUploadItem.tsRepository: box/box-ui-elements
Length of output: 48163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate FolderUpload class:"
rg -n 'class FolderUpload|function FolderUpload|export class FolderUpload|FolderUpload' src -g '*.ts' -g '*.tsx' -g '*.js'
echo
for f in $(rg -l 'class FolderUpload|folderUploads\.setBytesUploaded|setBytesUploaded|totalUploadedBytes|totalBytes|bytesUploaded|progressCallback' src/common src/elements/content-uploader); do
echo "===== $f ====="
wc -l "$f"
rg -n 'class FolderUpload|function FolderUpload|setBytesUploaded|totalUploadedBytes|totalBytes|bytesUploaded|progressCallback|emitProgress|setComplete|complete' "$f" || true
done
echo
echo "Inspect candidate FolderUpload files:"
find src -path '*FolderUpload*' -o -path '*folderUploads*' | while read -r f; do
echo "---- $f"
sed -n '1,220p' "$f"
doneRepository: box/box-ui-elements
Length of output: 49676
Skip byte/ETA fields for isFolder items.
Folder roots are queued with size: 1 and never receive real folder aggregate progress via handleUploadProgress; with isUploadEtaEnabled enabled this mapper can expose “0 of 1 bytes” for that row. Treat folder items as non-file data and don’t populate bytesUploaded, totalBytes, or remainingMs for item.isFolder.
🤖 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/utils/mapToModernizedUploadItem.ts` around
lines 63 - 65, Update mapToModernizedUploadItem so items with item.isFolder do
not populate totalBytes, bytesUploaded, or remainingMs, regardless of upload
status or ETA configuration. Preserve the existing byte and ETA calculations for
non-folder file items.
| const isFullyUploaded = status === 'staged' || status === 'complete'; | ||
| const bytesUploaded = isFullyUploaded && totalBytes != null ? totalBytes : fileItem.bytesUploaded; | ||
| const remainingMs = !isFullyUploaded ? fileItem.remainingMs : undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate STATUS_MAP definition to confirm the modernized status literals.
rg -n -B2 -A15 'STATUS_MAP' src/elements/content-uploader --type=tsRepository: box/box-ui-elements
Length of output: 4007
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the mapper implementation and nearby status-related code.
wc -l src/elements/content-uploader/utils/mapToModernizedUploadItem.ts
sed -n '1,120p' src/elements/content-uploader/utils/mapToModernizedUploadItem.ts
# Search for legacy status constants and remainingMs handling.
rg -n "STATUS_PENDING|STATUS_IN_PROGRESS|STATUS_STAGED|STATUS_COMPLETE|STATUS_ERROR|STATUS_CANCELED|remainingMs|bytesUploaded|totalBytes" src/elements/content-uploader --type=ts | sed -n '1,220p'Repository: box/box-ui-elements
Length of output: 26143
Stale remainingMs leaks through for errored/canceled items.
remainingMs is forwarded for any status except staged/complete, but handleUploadError and cancellation do not clear the legacy item.remainingMs. This sends the last computed ETA through for error and canceled status values, so the modernized manager can display a stale "time remaining".
🐛 Proposed fix
- const remainingMs = !isFullyUploaded ? fileItem.remainingMs : undefined;
+ const remainingMs = status === 'uploading' ? fileItem.remainingMs : undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isFullyUploaded = status === 'staged' || status === 'complete'; | |
| const bytesUploaded = isFullyUploaded && totalBytes != null ? totalBytes : fileItem.bytesUploaded; | |
| const remainingMs = !isFullyUploaded ? fileItem.remainingMs : undefined; | |
| const isFullyUploaded = status === 'staged' || status === 'complete'; | |
| const bytesUploaded = isFullyUploaded && totalBytes != null ? totalBytes : fileItem.bytesUploaded; | |
| const remainingMs = status === 'uploading' ? fileItem.remainingMs : undefined; |
🤖 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/utils/mapToModernizedUploadItem.ts` around
lines 64 - 66, Update the remainingMs mapping in mapToModernizedUploadItem so
errored and canceled statuses return undefined rather than forwarding the stale
fileItem.remainingMs value. Preserve the existing ETA for active, non-terminal
upload statuses and keep staged/complete behavior unchanged.
821a657 to
71f4dc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-uploader/ContentUploader.tsx (1)
1273-1275: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip ETA sampling when the feature is disabled.
isUploadEtaEnabledonly controls mapping, so every upload still invokesupdateEtaand allocates sampling arrays while the mapper drops the result. Guard this telemetry block with the feature flag to avoid adding work to the existing progress hot path.🤖 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 1273 - 1275, Guard the ETA telemetry block in the upload progress handler with isUploadEtaEnabled so updateEta and etaByItem.set are skipped when the feature is disabled. Preserve the existing progress handling and only sample or store ETA data when the flag is enabled.
🤖 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 `@src/elements/content-uploader/ContentUploader.tsx`:
- Around line 1276-1278: Update handleUploadError and markItemCanceled to clear
item.remainingMs and remove the corresponding etaByItem entry when an upload
fails or is canceled. Preserve ETA updates for in-progress uploads and ensure
failed/canceled rows cannot retain stale time remaining.
---
Nitpick comments:
In `@src/elements/content-uploader/ContentUploader.tsx`:
- Around line 1273-1275: Guard the ETA telemetry block in the upload progress
handler with isUploadEtaEnabled so updateEta and etaByItem.set are skipped when
the feature is disabled. Preserve the existing progress handling and only sample
or store ETA data when the flag is enabled.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9847b8e-6c4f-4824-bd01-3829e251a1ed
⛔ 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
🚧 Files skipped from review as they are similar to previous changes (8)
- package.json
- src/elements/content-uploader/utils/mapToModernizedUploadItem.ts
- src/elements/content-uploader/tests/ContentUploader.test.js
- src/elements/content-uploader/utils/uploadEta.ts
- src/common/types/upload.js
- src/elements/content-uploader/utils/tests/uploadEta.test.ts
- src/elements/content-uploader/utils/tests/mapToModernizedUploadItem.test.ts
- src/elements/content-uploader/stories/ContentUploader.stories.js
| item.bytesUploaded = event.loaded; | ||
| item.totalBytes = event.total; | ||
| item.remainingMs = getRemainingMs(nextEta); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear ETA when an upload fails or is canceled.
After this block stores remainingMs, handleUploadError and markItemCanceled leave it intact. Since the modernized mapper forwards ETA for every status except staged/complete, failed and canceled rows can show stale time remaining. Clear item.remainingMs and delete the corresponding etaByItem entry in both transitions, or restrict mapping to STATUS_IN_PROGRESS.
🤖 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 1276 - 1278,
Update handleUploadError and markItemCanceled to clear item.remainingMs and
remove the corresponding etaByItem entry when an upload fails or is canceled.
Preserve ETA updates for in-progress uploads and ensure failed/canceled rows
cannot retain stale time remaining.
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