feat: Provide progress updates for multiple range differential downloads #9448
Conversation
🦋 Changeset detectedLatest commit: 871216f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Add emission of download-progress events when multiple range differential downloads are performed. Previously, no progress update events were emitted if this download method was used.
eliotschu
force-pushed
the
feat--multiple-range-progress-updates
branch
from
1a3354a to
871216f
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements progress reporting for multiple range differential downloads in the electron-updater. Previously, progress events were only supported for single-range downloads (as indicated by the removed TODO comment). The implementation adds byte tracking and timed progress emission to the DataSplitter class, which handles multipart HTTP responses.
Key changes:
- Track total expected download bytes across all download ranges
- Emit progress events every 1 second during download and upon completion
- Update transferred and delta byte counters as data chunks are processed
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| multipleRangeDownloader.ts | Calculate total download bytes and pass progress callback to DataSplitter |
| DifferentialDownloader.ts | Remove TODO comment about missing multiple range progress support |
| DataSplitter.ts | Add progress tracking state, emit progress events during data processing |
| .changeset/three-pens-sit.md | Document the feature addition for version tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| total: this.grandTotalBytes, | ||
| delta: this.delta, | ||
| transferred: this.transferred, | ||
| percent: (this.transferred / this.grandTotalBytes) * 100, |
There was a problem hiding this comment.
Division by zero can occur if grandTotalBytes is 0. While this scenario might be unlikely, if all tasks are COPY operations or if there are no download tasks, grandTotalBytes could be 0, causing the percent calculation to result in NaN or Infinity.
| delta: this.delta, | ||
| transferred: this.transferred, | ||
| percent: (this.transferred / this.grandTotalBytes) * 100, | ||
| bytesPerSecond: Math.round(this.transferred / ((now - this.start) / 1000)), |
There was a problem hiding this comment.
Division by zero can occur if the download completes in less than 1 millisecond. The calculation (now - this.start) / 1000 could result in 0 if now equals this.start, causing bytesPerSecond to be Infinity. Consider adding a guard condition or using Math.max to ensure a minimum elapsed time of 1 millisecond.
mmaietta
left a comment
mmaietta
left a comment
There was a problem hiding this comment.
This is awesome. Thanks for submitting this PR
| }) | ||
| this.delta = 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
I like the simplicity of this PR in that it's throttling/firing a callback when data is received (as opposed to a NodeJS.Timer approach), but I'm concerned with the number of properties that are being added.
private start = Date.now()
private nextUpdate = this.start + 1000
private transferred = 0
private delta = 0
Feels like we're recreating a wheel here but I can't think of a better approach. Can you at least please add a short tsdoc as to what each is doing (or expected to be used as/for)?
2 participants
Summary of Changes