Ligher android uploader #20
Conversation
thomasttvo
requested review from
1DanielC,
dhruv-dangi-openspace and
elliottkember
| val result = response.use { // close the response asap | ||
| UploadResponse( | ||
| response.code, | ||
| response.body?.string().takeIf { !it.isNullOrBlank() } ?: response.message, |
There was a problem hiding this comment.
looks like you're shadowing it here. you should either name the response you have opened via use, or name this string you are conditionally taking:
| response.body?.string().takeIf { !it.isNullOrBlank() } ?: response.message, | |
| response.body?.string()?.takeIf { str -> str.isNotEmpty() } ?: response.message, |
| override fun onResponse(call: Call, response: Response) = | ||
| continuation.resumeWith(Result.success(response)) | ||
| override fun onResponse(call: Call, response: Response) { | ||
| val result = response.use { // close the response asap |
There was a problem hiding this comment.
This will technically work, but best practices would recommend naming the response you have opened:
val result = response.use { res ->
UploadResponse(
res.code,
...
| .sumOf { storage.getLong(it, 0L) } | ||
| @Synchronized | ||
| fun set(uploadId: String, bytesUploaded: Long) { | ||
| map[uploadId]?.bytesUploaded = bytesUploaded |
There was a problem hiding this comment.
This only sets if map[uploadId] exists, which is not conventional in a set function. I would rename this to something like setIfNotNull
| return (totalBytesUploaded.toDouble() * 100 / totalFileSize) | ||
| @Synchronized | ||
| fun complete(uploadId: String) { | ||
| map[uploadId]?.let { |
There was a problem hiding this comment.
nit you could add this as a complete() function in Progress and get some encapsulation:
| map[uploadId]?.let { | |
| map[uploadId]?.complete() |
| storage.edit().clear().apply() | ||
| } | ||
| @Synchronized | ||
| private fun clearIfNeeded() { |
There was a problem hiding this comment.
nit i would call this clearIfCompleted()
| // workers may be killed abruptly for whatever reasons, | ||
| // so they might not have had a chance to clear the progress data. | ||
| UploadProgress.clearIfNeeded(context) |
There was a problem hiding this comment.
Has no effect because the progress lives and dies with the application
| private fun handleProgress(bytesSentTotal: Long, fileSize: Long) { | ||
| UploadProgress.set(upload.id, bytesSentTotal) | ||
| EventReporter.progress(upload.id, bytesSentTotal, fileSize) | ||
| setForeground(getForegroundInfo()) |
There was a problem hiding this comment.
Pairing note: We only need to call setForeground once, no need to call it all the time like this
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Name response variable in use block (UploadUtils.kt) - Fix shadowed it in takeIf lambda (UploadUtils.kt) - Rename set() to setIfNotNull() (UploadProgress.kt) - Add complete() method to Progress class (UploadProgress.kt) - Rename clearIfNeeded() to clearIfCompleted() (UploadProgress.kt) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
UploadProgress. Workers live and die with the application instance so we don't need persistencenotificationManager.notifyinstead ofsetForegroundUploadProgress.clearIfNeededno longer fetches worker statusesNote
Switch to in-memory progress with updated notification handling, return
UploadResponsefrom uploads, refactorEventReporter, and bump example minSdk to 29.UploadProgress(add/set/complete/remove/total) and auto-clear when all complete.okhttpUploadnow returnsUploadResponse(code/body/headers) and closes theResponseearly; maps headers viatoMultimap.EventReporter.successupdated to consumeUploadResponse;EventReporterrefactored to anobject.NotificationManager.notifyand newbuildNotification();getForegroundInfo()builds from it.Upload.notificationIdis nowInt(hash of provided string).UploaderModule.minSdkVersionto29.Written by Cursor Bugbot for commit e3c135c. Configure here.