Fix synchronization issue with android notification #21
Conversation
| NoWifi, NoInternet, Ok; | ||
|
|
||
| companion object { | ||
| fun fetch(context: Context, wifiOnly: Boolean): Connectivity { |
There was a problem hiding this comment.
moved from UploadWorker.ts no logical change
| } | ||
|
|
||
| // builds the notification required to enable Foreground mode | ||
| fun build(context: Context): Notification { |
There was a problem hiding this comment.
moved from UploadWorker.ts. Not much has changed except for val wifiOnly = getActiveUpload()?.wifiOnly ?: false
| if (this.activeUpload?.id == upload.id) this.activeUpload = null | ||
| } | ||
|
|
||
| fun setOptions(opts: ReadableMap) { |
There was a problem hiding this comment.
these options are moved over from the Upload class, so they are now global options instead of per-Upload options
| setProgress(undefined); | ||
| console.log('Upload error!', err); | ||
| }); | ||
| for (let i = 0; i < 100; i++) { |
There was a problem hiding this comment.
Can we add a comment explaining the magic number 100 and why we have to perform Upload.startUpload 100 times?
There was a problem hiding this comment.
ah this is just the test app, but it's good to add a comment still
- 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>
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
| ios?: IOSOnlyUploadOptions; | ||
| } & RawUploadOptions; | ||
|
|
||
| type AndroidOnlyUploadOptions = { |
There was a problem hiding this comment.
maxRetries option ignored in initialization
Medium Severity
The maxRetries property in AndroidInitializationOptions type is declared but never read by UploadNotification.setOptions(). Meanwhile, Upload.fromReadableMap() still expects maxRetries from per-upload options, but UploadOptions no longer includes this property. This makes maxRetries effectively non-configurable—users passing it to initialize() will have it silently ignored, and the default value of 5 will always be used.
Additional Locations (1)
|
|
||
| // Don't bother to run on an invalid network | ||
| if (!validateAndReportConnectivity()) return null | ||
| if (Connectivity.fetch(context, upload.wifiOnly) != Connectivity.Ok) return null |
There was a problem hiding this comment.
Notification not updated when connectivity check fails
Medium Severity
When the connectivity check fails in upload(), the code returns null without updating the notification. Previously, validateAndReportConnectivity() always updated the notification to reflect connectivity state. Now, a wifi-only upload waiting for wifi shows "Uploading" instead of "Waiting for WiFi" because UploadNotification.build() uses the active upload's wifiOnly (defaulting to false when no active upload), and the notification is never updated when connectivity fails pre-semaphore.
| // mark as active upload for notification | ||
| UploadNotification.setActiveUpload(upload) | ||
| UploadNotification.update(context) | ||
|
|
There was a problem hiding this comment.
Semaphore leak if notification update throws exception
High Severity
If UploadNotification.setActiveUpload() or UploadNotification.update() throws an exception after semaphore.acquire() but before entering the try block, the finally block containing semaphore.release() will never execute. With MAX_CONCURRENCY = 1, this would permanently leak the only permit, causing all subsequent uploads to hang indefinitely waiting for the semaphore. The old code had semaphore.acquire() immediately followed by try, avoiding this window.
Fixes notification race condition where queued uploads could overwrite the active upload's notification settings. The bug occurred because each Upload object carried its own notification config—when a Wi-Fi-only upload was queued while a regular upload was running, the queued upload could touch the shared system notification and apply its wifiOnly=true preference before actually executing.
Centralization solves this by making UploadNotification the single source of truth for notification state. It tracks exactly one activeUpload at a time (synchronized), and notification content is built by reading getActiveUpload()?.wifiOnly rather than arbitrary upload configs. Queued uploads can exist in memory with their settings, but they can't affect the notification until they call setActiveUpload() when they start executing. This eliminates the race condition—only the actively running upload determines what the user sees.
Test Plan:
Note
Introduces centralized, connectivity-aware Android notification handling and a new initialization flow.
Written by Cursor Bugbot for commit 7063263. Configure here.