Nitro module expo uploadthing - #9
Merged
Merged
Conversation
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a new workspace package Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,235,255,0.5)
participant App as React Native App
participant JS as JS Upload Flow
participant UT as UploadThing Server
participant Native as Native Module
participant Store as Native Task Store
participant Query as React Query
end
App->>JS: request upload (route, files)
JS->>UT: POST presign (actionType=upload&slug)
UT-->>JS: presigned targets
JS->>Native: enqueueBackgroundUpload(taskId, fileUri, url, meta)
Native->>Store: persist task (queued)
JS-->>App: return tasks + completion promise
par Native background processing
Native->>Native: perform upload (URLSession / WorkManager)
Native->>Store: update progress/status
end
App->>Native: listBackgroundUploadTasks() (on mount/active)
Native-->>App: tasks
App->>Native: markBackgroundUploadTaskObserved()/removeBackgroundUploadTask(taskId)
Native->>Store: mark observed / remove record
App->>Query: invalidate queries (friends/messages/groups...)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
AugusDogus
marked this pull request as ready for review
March 19, 2026 02:35
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (12)
packages/react-native-uploadthing-background/.gitignore (2)
5-6: Minor: Duplicate.expo/entry.
.expo/appears on both line 6 and line 72. Consider removing the duplicate.Also applies to: 71-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/.gitignore` around lines 5 - 6, Remove the duplicated `.expo/` ignore entry in the .gitignore so it only appears once; locate the two `.expo/` lines (one near the top and one duplicated later) and delete the redundant occurrence to avoid duplicate entries.
45-48: Consider removing unusedexample/patterns.The patterns
example/ios/Podsandexample/vendor/reference anexampledirectory. If this package doesn't include an example app, these patterns are unnecessary and could be removed to keep the gitignore clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/.gitignore` around lines 45 - 48, The .gitignore currently contains unused patterns "example/ios/Pods" and "example/vendor/"; if this package does not include an example app, remove those two lines from packages/react-native-uploadthing-background/.gitignore to keep the file minimal and avoid referencing a non-existent example directory.packages/react-native-uploadthing-background/package.json (1)
58-63: Consider specifying minimum peer dependency versions.Using
"*"for peer dependencies is flexible but provides no guidance on compatibility. Consider specifying minimum versions (e.g.,"react-native": ">=0.70.0") to help consumers understand supported versions and catch incompatibilities early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/package.json` around lines 58 - 63, The peerDependencies in package.json currently use wildcards ("*") which gives no compatibility guidance; update the "peerDependencies" object to specify sensible minimum version ranges (for example set "react-native": ">=0.70.0", "react": ">=18.0.0", "expo": ">=48.0.0" or other supported minima) so consumers get compatibility hints and npm/yarn can warn on incompatible installs—modify the "peerDependencies" entry in packages/react-native-uploadthing-background/package.json accordingly, keeping any upper bounds or ranges you deem appropriate.packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundNotificationHelper.kt (1)
36-44: Minor: hashCode collision risk for notification ID.Using
taskId.hashCode()as the notification ID could theoretically collide for different task IDs, causing one notification to replace another. This is unlikely in practice with UUID-style task IDs and the impact is minimal (UI-only), but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundNotificationHelper.kt` around lines 36 - 44, The notification ID currently uses record.taskId.hashCode(), which can collide; replace that with a stable unique integer generator instead of relying on hashCode. Implement a small helper (e.g., NotificationIdGenerator.getIdForTask(taskId)) that maintains a ConcurrentHashMap<String,Integer> mapping from taskId to a generated incremental id (or assigns and stores a random/atomic integer on first request) and use that id in the ForegroundInfo calls inside UploadthingBackgroundNotificationHelper (where ForegroundInfo(record.taskId.hashCode(), ...) is created) so each task gets a distinct notification id.packages/react-native-uploadthing-background/NitroUploadthingBackground.podspec (1)
14-14: Minor: Preferto_sover string interpolation.Per RuboCop convention, use
s.version.to_sinstead of"#{s.version}"for simple conversions.Proposed fix
- s.source = { :git => "https://github.com/AugusDogus/whisp.git", :tag => "#{s.version}" } + s.source = { :git => "https://github.com/AugusDogus/whisp.git", :tag => s.version.to_s }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/NitroUploadthingBackground.podspec` at line 14, Replace the string-interpolated version in the podspec's s.source assignment by converting s.version to a string using to_s; specifically update the usage around s.source so it uses s.version.to_s instead of "#{s.version}" to satisfy RuboCop convention.packages/react-native-uploadthing-background/plugin/index.js (1)
9-24: Redundant with library's AndroidManifest.xml.The library's
AndroidManifest.xmlalready declaresFOREGROUND_SERVICEandFOREGROUND_SERVICE_DATA_SYNCpermissions. The Android manifest merger will deduplicate these, but having both the plugin and the static manifest declare them is redundant.Consider either:
- Removing these from the static manifest and relying solely on the plugin, or
- Removing them from the plugin since the library manifest already provides them
The current setup works but creates maintenance overhead if permissions need to change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/plugin/index.js` around lines 9 - 24, The plugin is redundantly adding permissions that the library's AndroidManifest.xml already declares; update the plugin to stop injecting the library-owned permissions instead of duplicating them: remove the code paths (calls that pass "android.permission.FOREGROUND_SERVICE" and "android.permission.FOREGROUND_SERVICE_DATA_SYNC" into ensurePermission) or change the logic around ensurePermission in index.js so it no longer pushes those specific permission entries into manifest.manifest["uses-permission"]; keep ensurePermission for other permissions but do not add the two library-managed constants.apps/expo/src/App.tsx (1)
51-108: Race condition with concurrent task removal is mitigated by idempotent implementations.Both this reconciliation handler and the
cleanupBackgroundTasksinmedia-upload.tscan attempt to remove the same task concurrently when a background upload completes while the app is backgrounded and the user then foregrounds the app.However, the native
removeTaskimplementations on both Android and iOS are idempotent—they safely handle removal of non-existent tasks by returning null rather than throwing exceptions. This means the race condition itself is not a failure risk, but the concurrent removal calls represent redundant work that could be optimized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/App.tsx` around lines 51 - 108, reconcileBackgroundUploads currently calls removeBackgroundUploadTask for every terminal task which can duplicate work with cleanupBackgroundTasks; fix by deduplicating task removals and making removals tolerant of concurrency: after getting tasks from listBackgroundUploadTasks in reconcileBackgroundUploads (and similarly in cleanupBackgroundTasks), build a unique set of taskId values and only call removeBackgroundUploadTask once per unique id, and use Promise.allSettled (or otherwise ignore non-critical failures) when awaiting removals to avoid throwing on concurrent removals.packages/react-native-uploadthing-background/ios/BackgroundUploadStore.swift (1)
203-220: Consider elevating persistence failures beyond NSLog for observability.Persistence failures are logged via
NSLogbut otherwise silently ignored. While this prevents crashes, upload state could be lost on disk write failures. Consider whether this warrants additional observability (e.g., incrementing a counter or emitting an event) for production monitoring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/ios/BackgroundUploadStore.swift` around lines 203 - 220, persistLocked currently swallows disk write errors after logging with NSLog; update it to surface failures for observability by both recording a metric and broadcasting an event: inside persistLocked(_:), in the catch block (where error.localizedDescription is logged), call a telemetry/metrics increment (e.g., increment a Failure counter via your telemetry client) and post a NotificationCenter notification or invoke an optional errorHandler closure/property on BackgroundUploadStore with the error so production monitoring can pick it up; keep the existing NSLog but add these two observable actions and reference persistLocked, cachedRecords, encoder, and storageURL when locating the code to change.packages/react-native-uploadthing-background/android/build.gradle (1)
106-108:lintOptionsis deprecated; uselintblock instead.The
lintOptionsDSL has been deprecated since AGP 7.0. Consider migrating to thelintblock for forward compatibility.♻️ Proposed fix
- lintOptions { - disable "GradleCompatible" - } + lint { + disable += "GradleCompatible" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/android/build.gradle` around lines 106 - 108, The build.gradle uses the deprecated lintOptions block (lintOptions { disable "GradleCompatible" }) which should be migrated to the newer lint DSL; replace the lintOptions usage with a lint { disable "GradleCompatible" } block (or equivalent lint { checks = [...] baseline = file(...) } pattern) so the project no longer relies on the deprecated lintOptions symbol and is compatible with AGP 7+ while preserving the disabled "GradleCompatible" rule.packages/react-native-uploadthing-background/src/uploadthing.ts (1)
225-246: Potential race condition with parallel enqueue calls.When
Promise.allenqueues uploads in parallel (lines 225-246), if any single enqueue fails, the already-enqueued tasks will remain in the background queue but the returnedtasksarray will be incomplete. Consider usingPromise.allSettledto handle partial failures gracefully, or ensure cleanup of enqueued tasks on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/src/uploadthing.ts` around lines 225 - 246, The parallel Promise.all enqueues via getUploadthingBackground().enqueueUpload can leave partially-enqueued tasks if one call fails; change the implementation to either use Promise.allSettled and detect rejected entries so you can call the background client's cancellation/removal API for any successfully-enqueued taskIds, or switch to a sequential for/async loop that enqueue each upload (using createTaskId(), ensureFileUri(), normalizeHeaders()) and on any error roll back previously-enqueued tasks by calling the background client's delete/cancel method for those taskIds; ensure you surface a clear error after cleanup.packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/BackgroundUploadStore.kt (1)
7-92: SharedPreferences-based storage is functional but has scalability limits.The current implementation stores all tasks as a single JSON blob in SharedPreferences. This works for moderate task counts but has limitations:
- Each read/write deserializes/serializes all tasks
- SharedPreferences has a practical size limit (~1MB)
- No indexing for efficient queries
This is acceptable for an initial implementation, but consider migrating to Room/SQLite if task volume grows significantly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/BackgroundUploadStore.kt` around lines 7 - 92, The store currently serializes all tasks into one JSON blob in SharedPreferences (see BackgroundUploadStore, readAll, persist, TASKS_KEY, PREFS_NAME and StoredBackgroundUploadTaskRecord), which will hit performance and size limits; migrate to a proper database (Room/SQLite) by creating a Room `@Entity` for StoredBackgroundUploadTaskRecord, a DAO with methods for list/get/insert/update/delete, and update BackgroundUploadStore to delegate activeTaskCount, listTasks, getTask, getRecord, upsert, update, and remove to the DAO (keeping the same method signatures and synchronized(lock) behavior for thread-safety), removing the single-blob readAll/persist logic and replacing nowMs usage with the same timestamp behavior in DAO updates.packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift (1)
206-210: Synchronous dispatch inside delegate callback may deadlock.Line 207 uses
queue.syncwithin a URLSession delegate callback. If the delegate queue happens to be the same asself.queue(or serialized with it), this could deadlock. Consider using async access or a different synchronization strategy:♻️ Proposed fix
- let responseBody = queue.sync { - let data = responseData.removeValue(forKey: task.taskIdentifier) - return data.flatMap { String(data: $0, encoding: .utf8) } - } + var responseBody: String? + queue.async { [self] in + let data = responseData.removeValue(forKey: task.taskIdentifier) + responseBody = data.flatMap { String(data: $0, encoding: .utf8) } + } + queue.sync {} // barrier to ensure async block completesAlternatively, ensure the URLSession delegate queue is never the same as
self.queueby explicitly settingdelegateQueuein the session configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift` around lines 206 - 210, The delegate callback currently uses queue.sync to read and remove responseData (responseBody = queue.sync { responseData.removeValue(forKey: task.taskIdentifier) ... }), which can deadlock if the URLSession delegate queue is the same as self.queue; change this to avoid synchronous dispatch: perform the read/remove on self.queue asynchronously (queue.async) and move any logic that depends on responseBody/responseCode into that async closure (or otherwise use a thread-safe data structure or lock) so you never call queue.sync from inside the URLSession delegate callback; reference the symbols responseBody, queue, responseData, task.taskIdentifier and ensure post-read processing is executed inside the async block (or use a separate dedicated serial queue for the URLSession delegate to guarantee they are never the same).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/src/utils/media-upload.ts`:
- Around line 177-206: The cleanup is currently inside the .then() callback so
it is skipped when backgroundBatch.completion rejects; fix by moving cleanup to
a promise.finally handler: declare a scoped variable (e.g. let tasks) before
calling backgroundBatch.completion, assign it inside the .then((t) => { tasks =
t; ... }) and remove the inner finally, then add .finally(async () => { await
cleanupBackgroundTasks(tasks ?? []); }) after the .catch so
cleanupBackgroundTasks always runs; keep applyFailedUploadSideEffects and
applySuccessfulUploadSideEffects usage unchanged.
In
`@packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/BackgroundUploadStore.kt`:
- Around line 209-215: Parsing currently uses JSONObject.optDouble(...) which
yields Double.NaN for missing keys; update the JSON parsing in
BackgroundUploadStore.kt to call the optDouble overload with a sane default
(e.g., optDouble("bytesSent", 0.0), optDouble("totalBytes", 0.0), and similarly
for createdAt/updatedAt if you want 0.0 instead of NaN) or perform an explicit
has/isNull check and map to 0.0 (or nullable) so that bytesSent/totalBytes (and
createdAt/updatedAt if applicable) are not NaN; keep the existing null-handling
logic for responseCode/responseBody/errorMessage.
In
`@packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.kt`:
- Line 78: In UploadthingBackgroundWorker.kt, replace the int overload call that
causes overflow by using the Long overload of
HttpURLConnection.setFixedLengthStreamingMode: remove the .toInt() cast on
totalBytes and call setFixedLengthStreamingMode(totalBytes) instead (the
variable totalBytes is already Long); ensure this change is made inside the
UploadthingBackgroundWorker class where the streaming mode is set and note the
Long overload requires API 19+ (no other logic changes needed).
In `@packages/react-native-uploadthing-background/babel.config.js`:
- Around line 1-3: The file's export (module.exports) is failing the oxfmt check
due to styling (likely a missing trailing semicolon); run the formatter (bun
format) to apply the correct code style, or manually ensure the module.exports
object ends with the proper semicolon and spacing consistent with the project's
Babel config convention so presets: ['module:`@react-native/babel-preset`'] is
formatted correctly.
In
`@packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift`:
- Around line 302-320: The InputStream opened from InputStream(url:) may fail
silently; after calling inputStream.open() check inputStream.streamStatus and/or
inputStream.streamError on the InputStream instance and if the status is not
.open (or is .error) throw the streamError (or a descriptive error) before
entering the read loop; update the logic around
InputStream/open/hasBytesAvailable in BackgroundUploadManager.swift (references:
InputStream, inputStream.open(), inputStream.streamStatus,
inputStream.streamError, inputStream.hasBytesAvailable, read(_:maxLength:),
write(data:to:)) so uploads fail loudly instead of producing an empty body when
open() fails.
- Around line 57-101: Summary: enqueueUpload always builds a multipart/form-data
body even for PUT presigned URLs, breaking uploads that expect raw file bytes.
Fix: In enqueueUpload (BackgroundUploadManager.enqueueUpload) branch on
request.method (or default "PUT") and only call createMultipartBody when using
multipart (e.g., POST); for PUT build the URLRequest with the raw file: set
Content-Type to the file's actual MIME (from request.headers or detect from
sourceFileURL), set Content-Length to the source file size, and call
session.uploadTask(with: urlRequest, fromFile: sourceFileURL) using
sourceFileURL (not multipartBody.fileURL). Also update
StoredBackgroundUploadTaskRecord to use totalBytes = source file size and
multipartFilePath = sourceFileURL.path (or nil/empty if you prefer), and remove
multipart-specific header setting (multipartBody.contentType) in the PUT path;
keep existing multipart logic only in the non-PUT path that still calls
createMultipartBody.
In
`@packages/react-native-uploadthing-background/NitroUploadthingBackground.podspec`:
- Line 14: The podspec's s.source currently points to the wrong repository
(s.source in NitroUploadthingBackground.podspec uses
"https://github.com/mrousavy/nitro.git"); update s.source to reference the
correct repository (e.g., "https://github.com/AugusDogus/whisp.git" while
keeping the tag interpolation "#{s.version}") or switch to a local path-based
source for development (use :path => "../relative/path") so CocoaPods fetches
the correct package.
In `@packages/react-native-uploadthing-background/README.md`:
- Around line 121-124: Update the compound adjective "event-emitter based" to
the hyphenated form "event-emitter-based" in the README text entry that
currently reads "task event delivery to JavaScript is polling-based rather than
push/event-emitter based" so the line becomes "task event delivery to JavaScript
is polling-based rather than push/event-emitter-based"; ensure the same
hyphenation is applied wherever the exact phrase "event-emitter based" appears
in the repository.
In `@packages/react-native-uploadthing-background/src/uploadthing.ts`:
- Around line 174-188: The polling loop in waitForBackgroundUploadTask can spin
forever if getUploadthingBackground().getTask(taskId) returns null (task
removed); update waitForBackgroundUploadTask to accept an options.maxWaitMs (or
maxRetries) and implement a timeout: track start time, on each poll call
getUploadthingBackground().getTask(taskId), if task is null treat that as
terminal by throwing a clear error (e.g., "task not found/removed") or
returning/propagating a terminal result per your type choices, and if elapsed
time exceeds options.maxWaitMs throw a timeout error; keep using
DEFAULT_POLL_INTERVAL_MS and sleep for polling. Ensure references:
waitForBackgroundUploadTask, getUploadthingBackground().getTask, isTerminalTask,
DEFAULT_POLL_INTERVAL_MS, and sleep are used to locate and modify the code.
In `@packages/react-native-uploadthing-background/tsconfig.json`:
- Around line 1-15: The tsconfig.json formatting failed CI's oxfmt check; run
the formatter (bun format) to reformat this JSON file so keys like "extends",
"compilerOptions", "jsx", "lib", "noEmit", "types", and the "include"/"exclude"
arrays are correctly styled; commit the formatted tsconfig.json so the oxfmt
check passes.
---
Nitpick comments:
In `@apps/expo/src/App.tsx`:
- Around line 51-108: reconcileBackgroundUploads currently calls
removeBackgroundUploadTask for every terminal task which can duplicate work with
cleanupBackgroundTasks; fix by deduplicating task removals and making removals
tolerant of concurrency: after getting tasks from listBackgroundUploadTasks in
reconcileBackgroundUploads (and similarly in cleanupBackgroundTasks), build a
unique set of taskId values and only call removeBackgroundUploadTask once per
unique id, and use Promise.allSettled (or otherwise ignore non-critical
failures) when awaiting removals to avoid throwing on concurrent removals.
In `@packages/react-native-uploadthing-background/.gitignore`:
- Around line 5-6: Remove the duplicated `.expo/` ignore entry in the .gitignore
so it only appears once; locate the two `.expo/` lines (one near the top and one
duplicated later) and delete the redundant occurrence to avoid duplicate
entries.
- Around line 45-48: The .gitignore currently contains unused patterns
"example/ios/Pods" and "example/vendor/"; if this package does not include an
example app, remove those two lines from
packages/react-native-uploadthing-background/.gitignore to keep the file minimal
and avoid referencing a non-existent example directory.
In `@packages/react-native-uploadthing-background/android/build.gradle`:
- Around line 106-108: The build.gradle uses the deprecated lintOptions block
(lintOptions { disable "GradleCompatible" }) which should be migrated to the
newer lint DSL; replace the lintOptions usage with a lint { disable
"GradleCompatible" } block (or equivalent lint { checks = [...] baseline =
file(...) } pattern) so the project no longer relies on the deprecated
lintOptions symbol and is compatible with AGP 7+ while preserving the disabled
"GradleCompatible" rule.
In
`@packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/BackgroundUploadStore.kt`:
- Around line 7-92: The store currently serializes all tasks into one JSON blob
in SharedPreferences (see BackgroundUploadStore, readAll, persist, TASKS_KEY,
PREFS_NAME and StoredBackgroundUploadTaskRecord), which will hit performance and
size limits; migrate to a proper database (Room/SQLite) by creating a Room
`@Entity` for StoredBackgroundUploadTaskRecord, a DAO with methods for
list/get/insert/update/delete, and update BackgroundUploadStore to delegate
activeTaskCount, listTasks, getTask, getRecord, upsert, update, and remove to
the DAO (keeping the same method signatures and synchronized(lock) behavior for
thread-safety), removing the single-blob readAll/persist logic and replacing
nowMs usage with the same timestamp behavior in DAO updates.
In
`@packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundNotificationHelper.kt`:
- Around line 36-44: The notification ID currently uses
record.taskId.hashCode(), which can collide; replace that with a stable unique
integer generator instead of relying on hashCode. Implement a small helper
(e.g., NotificationIdGenerator.getIdForTask(taskId)) that maintains a
ConcurrentHashMap<String,Integer> mapping from taskId to a generated incremental
id (or assigns and stores a random/atomic integer on first request) and use that
id in the ForegroundInfo calls inside UploadthingBackgroundNotificationHelper
(where ForegroundInfo(record.taskId.hashCode(), ...) is created) so each task
gets a distinct notification id.
In
`@packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift`:
- Around line 206-210: The delegate callback currently uses queue.sync to read
and remove responseData (responseBody = queue.sync {
responseData.removeValue(forKey: task.taskIdentifier) ... }), which can deadlock
if the URLSession delegate queue is the same as self.queue; change this to avoid
synchronous dispatch: perform the read/remove on self.queue asynchronously
(queue.async) and move any logic that depends on responseBody/responseCode into
that async closure (or otherwise use a thread-safe data structure or lock) so
you never call queue.sync from inside the URLSession delegate callback;
reference the symbols responseBody, queue, responseData, task.taskIdentifier and
ensure post-read processing is executed inside the async block (or use a
separate dedicated serial queue for the URLSession delegate to guarantee they
are never the same).
In
`@packages/react-native-uploadthing-background/ios/BackgroundUploadStore.swift`:
- Around line 203-220: persistLocked currently swallows disk write errors after
logging with NSLog; update it to surface failures for observability by both
recording a metric and broadcasting an event: inside persistLocked(_:), in the
catch block (where error.localizedDescription is logged), call a
telemetry/metrics increment (e.g., increment a Failure counter via your
telemetry client) and post a NotificationCenter notification or invoke an
optional errorHandler closure/property on BackgroundUploadStore with the error
so production monitoring can pick it up; keep the existing NSLog but add these
two observable actions and reference persistLocked, cachedRecords, encoder, and
storageURL when locating the code to change.
In
`@packages/react-native-uploadthing-background/NitroUploadthingBackground.podspec`:
- Line 14: Replace the string-interpolated version in the podspec's s.source
assignment by converting s.version to a string using to_s; specifically update
the usage around s.source so it uses s.version.to_s instead of "#{s.version}" to
satisfy RuboCop convention.
In `@packages/react-native-uploadthing-background/package.json`:
- Around line 58-63: The peerDependencies in package.json currently use
wildcards ("*") which gives no compatibility guidance; update the
"peerDependencies" object to specify sensible minimum version ranges (for
example set "react-native": ">=0.70.0", "react": ">=18.0.0", "expo": ">=48.0.0"
or other supported minima) so consumers get compatibility hints and npm/yarn can
warn on incompatible installs—modify the "peerDependencies" entry in
packages/react-native-uploadthing-background/package.json accordingly, keeping
any upper bounds or ranges you deem appropriate.
In `@packages/react-native-uploadthing-background/plugin/index.js`:
- Around line 9-24: The plugin is redundantly adding permissions that the
library's AndroidManifest.xml already declares; update the plugin to stop
injecting the library-owned permissions instead of duplicating them: remove the
code paths (calls that pass "android.permission.FOREGROUND_SERVICE" and
"android.permission.FOREGROUND_SERVICE_DATA_SYNC" into ensurePermission) or
change the logic around ensurePermission in index.js so it no longer pushes
those specific permission entries into manifest.manifest["uses-permission"];
keep ensurePermission for other permissions but do not add the two
library-managed constants.
In `@packages/react-native-uploadthing-background/src/uploadthing.ts`:
- Around line 225-246: The parallel Promise.all enqueues via
getUploadthingBackground().enqueueUpload can leave partially-enqueued tasks if
one call fails; change the implementation to either use Promise.allSettled and
detect rejected entries so you can call the background client's
cancellation/removal API for any successfully-enqueued taskIds, or switch to a
sequential for/async loop that enqueue each upload (using createTaskId(),
ensureFileUri(), normalizeHeaders()) and on any error roll back
previously-enqueued tasks by calling the background client's delete/cancel
method for those taskIds; ensure you surface a clear error after cleanup.
🪄 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
Run ID: cdfe204c-c009-4b33-bef4-f927e3ff0f72
⛔ Files ignored due to path filters (47)
bun.lockis excluded by!**/*.lockpackages/react-native-uploadthing-background/nitrogen/generated/.gitattributesis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/NitroUploadthingBackground+autolinking.cmakeis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/NitroUploadthingBackground+autolinking.gradleis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/NitroUploadthingBackgroundOnLoad.cppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/NitroUploadthingBackgroundOnLoad.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JBackgroundUploadHeader.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JBackgroundUploadRequest.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JBackgroundUploadTask.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JBackgroundUploadTaskStatus.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JHybridUploadthingBackgroundSpec.cppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JHybridUploadthingBackgroundSpec.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JVariant_NullType_BackgroundUploadTask.cppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JVariant_NullType_BackgroundUploadTask.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/kotlin/com/margelo/nitro/uploadthingbackground/BackgroundUploadHeader.ktis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/kotlin/com/margelo/nitro/uploadthingbackground/BackgroundUploadRequest.ktis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/kotlin/com/margelo/nitro/uploadthingbackground/BackgroundUploadTask.ktis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/kotlin/com/margelo/nitro/uploadthingbackground/BackgroundUploadTaskStatus.ktis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/kotlin/com/margelo/nitro/uploadthingbackground/HybridUploadthingBackgroundSpec.ktis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/kotlin/com/margelo/nitro/uploadthingbackground/NitroUploadthingBackgroundOnLoad.ktis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/kotlin/com/margelo/nitro/uploadthingbackground/Variant_NullType_BackgroundUploadTask.ktis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/NitroUploadthingBackground+autolinking.rbis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/NitroUploadthingBackground-Swift-Cxx-Bridge.cppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/NitroUploadthingBackground-Swift-Cxx-Bridge.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/NitroUploadthingBackground-Swift-Cxx-Umbrella.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/NitroUploadthingBackgroundAutolinking.mmis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/NitroUploadthingBackgroundAutolinking.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/c++/HybridUploadthingBackgroundSpecSwift.cppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/c++/HybridUploadthingBackgroundSpecSwift.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/BackgroundUploadHeader.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/BackgroundUploadRequest.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/BackgroundUploadTask.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/BackgroundUploadTaskStatus.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/Func_void.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/Func_void_BackgroundUploadTask.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/Func_void_std__exception_ptr.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/Func_void_std__variant_nitro__NullType__BackgroundUploadTask_.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/Func_void_std__vector_BackgroundUploadTask_.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/HybridUploadthingBackgroundSpec.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/HybridUploadthingBackgroundSpec_cxx.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/Variant_NullType_BackgroundUploadTask.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/shared/c++/BackgroundUploadHeader.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/shared/c++/BackgroundUploadRequest.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/shared/c++/BackgroundUploadTask.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/shared/c++/BackgroundUploadTaskStatus.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/shared/c++/HybridUploadthingBackgroundSpec.cppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/shared/c++/HybridUploadthingBackgroundSpec.hppis excluded by!**/generated/**
📒 Files selected for processing (36)
apps/expo/app.config.tsapps/expo/package.jsonapps/expo/src/App.tsxapps/expo/src/utils/media-upload.tsapps/expo/src/utils/uploadthing.tspackages/react-native-uploadthing-background/.gitignorepackages/react-native-uploadthing-background/.watchmanconfigpackages/react-native-uploadthing-background/NitroUploadthingBackground.podspecpackages/react-native-uploadthing-background/README.mdpackages/react-native-uploadthing-background/android/CMakeLists.txtpackages/react-native-uploadthing-background/android/build.gradlepackages/react-native-uploadthing-background/android/fix-prefab.gradlepackages/react-native-uploadthing-background/android/gradle.propertiespackages/react-native-uploadthing-background/android/src/main/AndroidManifest.xmlpackages/react-native-uploadthing-background/android/src/main/cpp/cpp-adapter.cpppackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/BackgroundUploadStore.ktpackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/HybridUploadthingBackground.ktpackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/NitroUploadthingBackgroundPackage.ktpackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundNotificationHelper.ktpackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.ktpackages/react-native-uploadthing-background/app.plugin.jspackages/react-native-uploadthing-background/babel.config.jspackages/react-native-uploadthing-background/expo-module.config.jsonpackages/react-native-uploadthing-background/ios/BackgroundUploadManager.swiftpackages/react-native-uploadthing-background/ios/BackgroundUploadStore.swiftpackages/react-native-uploadthing-background/ios/Bridge.hpackages/react-native-uploadthing-background/ios/HybridUploadthingBackground.swiftpackages/react-native-uploadthing-background/ios/UploadthingBackgroundAppDelegateSubscriber.swiftpackages/react-native-uploadthing-background/nitro.jsonpackages/react-native-uploadthing-background/package.jsonpackages/react-native-uploadthing-background/plugin/index.jspackages/react-native-uploadthing-background/react-native.config.jspackages/react-native-uploadthing-background/src/index.tspackages/react-native-uploadthing-background/src/specs/uploadthing-background.nitro.tspackages/react-native-uploadthing-background/src/uploadthing.tspackages/react-native-uploadthing-background/tsconfig.json
Owner
Author
|
@cursoragent please fix these issues |
|
Fixed and pushed in What I addressed:
Validation run:
Branch is clean and pushed.   |
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/src/App.tsx`:
- Around line 73-75: Terminal task records are being removed immediately
(removeBackgroundUploadTasks(terminalTasks.map(...))) which can race with live
completion waiters; instead, for each task from uploadFilesWithInputInBackground
use the live completion/observation API (e.g., await the completion promise
returned by uploadFilesWithInputInBackground or call
waitForBackgroundUploadTask(taskId) to observe a non-null terminal state) and
only call removeBackgroundUploadTasks for taskIds after those waiters have
resolved. Update the code to await each task's completion/observation (or filter
to tasks whose getTask() is confirmed terminal) before invoking
removeBackgroundUploadTasks to avoid "removed before it reached a terminal
state" errors.
In `@packages/react-native-uploadthing-background/android/build.gradle`:
- Around line 55-62: The CMake per-build-type flags were placed inside
defaultConfig.externalNativeBuild.cmake (ExternalNativeCmakeOptions) which is
invalid; move the cppFlags lines out of defaultConfig and into each build type's
externalNativeBuild.cmake block (e.g.,
buildTypes.debug.externalNativeBuild.cmake and
buildTypes.release.externalNativeBuild.cmake) so that cppFlags "-O1 -g" is set
under buildTypes.debug.externalNativeBuild.cmake and cppFlags "-O2" is set under
buildTypes.release.externalNativeBuild.cmake, removing them from
defaultConfig.externalNativeBuild.cmake.
In
`@packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.kt`:
- Around line 199-237: The progress persistence is too frequent
(PROGRESS_UPDATE_CHUNK_BYTES = 64KB) causing expensive
BackgroundUploadStore.update() and notification rebuilds via updateProgress();
change the hot path to batch/throttle updates: in the upload loop (where
lastReportedBytesSent is used) keep in-memory counters and a lastPersistTime
timestamp and only call updateProgress(record, bytesSent, totalBytes) when
either bytesSent - lastPersistBytes >= LARGER_CHUNK_BYTES (e.g., 1–5MB) OR
System.currentTimeMillis() - lastPersistTime >= PERSIST_INTERVAL_MS (e.g.,
1–5s), and always call updateProgress once at completion; adjust/introduce
constants (e.g., LARGER_CHUNK_BYTES, PERSIST_INTERVAL_MS) and update
lastPersistBytes/lastPersistTime when you persist, leaving
BackgroundUploadStore.update, setForeground, and setProgress usage unchanged
inside updateProgress().
- Around line 54-66: The current worker should abort if it fails to claim the
task and must not proceed with the fallback initialRecord; change the claim
logic around BackgroundUploadStore.update(...) so that a null return causes an
immediate abort/stop (do not use the ?: initialRecord.copy(...) fallback) and
ensure all subsequent BackgroundUploadStore.update(taskId) calls are gated by a
per-worker attempt ID or expected prior status embedded in the record (e.g., add
an attemptId field to initialRecord and include that when calling
BackgroundUploadStore.update) so stale workers (including ones replaced via
enqueueUniqueWork(..., REPLACE) in HybridUploadthingBackground.kt) cannot
overwrite newer attempts; apply the same pattern for the other update blocks
referenced (lines ~118-169).
- Around line 30-45: The filename sanitization in UploadthingBackgroundWorker.kt
(variable escapedFileName used when building the multipart Content-Disposition
in the code that constructs the multipart header) only replaces quotes and still
allows CR/LF which can break the multipart body; update the sanitization to also
strip or replace '\r' and '\n' characters (and optionally other control
characters) from initialRecord.fileName before inserting it into the
Content-Disposition header so the boundary and header lines cannot be injected
or broken.
In
`@packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift`:
- Around line 147-150: Only mark the upload record cancelled when a live
URLSessionTask was actually cancelled: before calling store.update(taskId: ...)
check the current session tasks from getAllTasks() for a task that matches the
URLSessionTaskIdentifier (or your matching logic) and confirm its state
indicates cancellation (e.g., .canceling/.cancelled) and/or that the store's
current record status is not already .completed or .failed; only then set
record.status = .cancelled and record.errorMessage. Reference getAllTasks(),
URLSessionTask (task.identifier/state), store.update(taskId:) and taskId to find
where to add this guard so late cancel requests do not overwrite final results.
- Around line 232-252: The code unconditionally sets record.bytesSent =
max(record.bytesSent, record.totalBytes) before determining status, causing
cancelled/failed uploads to report 100% progress; fix by moving or making that
assignment conditional so bytesSent is only set to totalBytes when the upload
actually completed (i.e., when you set record.status = .completed for successful
2xx responses and no error). Update the closure in store.update (referencing
record.bytesSent, record.totalBytes, record.status, and the error/responseCode
checks) to determine status and errorMessage first, then only set bytesSent to
totalBytes (or take max) when the status is .completed; for
failures/cancellations leave bytesSent as-is so partial progress is preserved.
In
`@packages/react-native-uploadthing-background/ios/BackgroundUploadStore.swift`:
- Around line 196-207: The current catch block swallows all read/parse errors
and resets cachedRecords to [:]; change it so only a missing file results in an
empty store: after catching, check FileManager.default.fileExists(atPath:
storageURL().path) — if false set cachedRecords = [:] and return [:]; otherwise
do NOT overwrite cachedRecords, and surface the decode/I/O error via the
existing error hook/notification path (invoke your error handler/notification
with the caught error) and either rethrow or return the prior cachedRecords so
running URLSession tasks retain their sessionTaskIdentifier -> taskId mapping;
update the code referencing storageURL(), decoder.decode(...,
StoredBackgroundUploadTaskRecord.self), and cachedRecords accordingly.
🪄 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
Run ID: 60897a89-32f8-456b-8e3e-c71671bbd213
📒 Files selected for processing (22)
apps/expo/src/App.tsxapps/expo/src/utils/media-upload.tsapps/expo/src/utils/uploadthing.tspackages/react-native-uploadthing-background/.gitignorepackages/react-native-uploadthing-background/.watchmanconfigpackages/react-native-uploadthing-background/NitroUploadthingBackground.podspecpackages/react-native-uploadthing-background/README.mdpackages/react-native-uploadthing-background/android/build.gradlepackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/BackgroundUploadStore.ktpackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundNotificationHelper.ktpackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.ktpackages/react-native-uploadthing-background/babel.config.jspackages/react-native-uploadthing-background/ios/BackgroundUploadManager.swiftpackages/react-native-uploadthing-background/ios/BackgroundUploadStore.swiftpackages/react-native-uploadthing-background/nitro.jsonpackages/react-native-uploadthing-background/package.jsonpackages/react-native-uploadthing-background/plugin/index.jspackages/react-native-uploadthing-background/react-native.config.jspackages/react-native-uploadthing-background/src/index.tspackages/react-native-uploadthing-background/src/specs/uploadthing-background.nitro.tspackages/react-native-uploadthing-background/src/uploadthing.tspackages/react-native-uploadthing-background/tsconfig.json
✅ Files skipped from review due to trivial changes (9)
- packages/react-native-uploadthing-background/.watchmanconfig
- packages/react-native-uploadthing-background/babel.config.js
- packages/react-native-uploadthing-background/react-native.config.js
- packages/react-native-uploadthing-background/tsconfig.json
- packages/react-native-uploadthing-background/.gitignore
- packages/react-native-uploadthing-background/nitro.json
- packages/react-native-uploadthing-background/package.json
- packages/react-native-uploadthing-background/README.md
- packages/react-native-uploadthing-background/src/specs/uploadthing-background.nitro.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react-native-uploadthing-background/NitroUploadthingBackground.podspec
- apps/expo/src/utils/media-upload.ts
- packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/BackgroundUploadStore.kt
Owner
Author
|
@cursoragent there are some new comments that need to be resolved now as well as one outstanding issue that didn't get resolved previously. |
|
Fixed and pushed in What I resolved:
Validation run:
Branch is clean and pushed. |
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/expo/src/App.tsx (1)
75-85:⚠️ Potential issue | 🟠 MajorAge-based cleanup still races suspended completion waiters.
The 5-minute retention window reduces the odds, but it still doesn't prove every JS waiter has already observed the terminal snapshot. If the app resumes after that window, foreground reconciliation can still remove the record before suspended completion logic polls again, turning a real terminal result into a spurious “task disappeared” path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/App.tsx` around lines 75 - 85, The age-based cleanup using staleTerminalTasks and TERMINAL_TASK_RETENTION_MS can still race suspended JS completion waiters; instead, ensure terminal records are not removed until JS has definitely observed them by adding a durable handshake: update the terminal record with a "finalized"/"tombstone" flag or lastObservedByJS timestamp when the JS waiter reads the terminal snapshot, and only call removeBackgroundUploadTasks for taskIds whose record shows that handshake has completed (or extend retention per-record until that flag is present); modify the code paths that read terminal snapshots to write the handshake marker and change removeBackgroundUploadTasks to check that marker before deleting tasks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.kt`:
- Around line 221-233: The final unconditional call updateProgress(record,
totalBytes, totalBytes) prematurely reports 100% before footerBytes are written
and before the response/HTTP success is confirmed; remove that final promotion
and instead call updateProgress(record, bytesSentAfterFooter, totalBytes) only
after writing the footer and after the HTTP/stream response has succeeded (or on
confirmed completion), using the actual bytesSent (including footerBytes) to set
lastPersistedBytesSent and lastPersistTimeMs consistently; locate and change
calls around updateProgress, bytesSent, footerBytes, lastPersistedBytesSent, and
lastPersistTimeMs so 100% is only reported on confirmed completion.
In
`@packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift`:
- Around line 149-154: The current removeTask(taskId:) implementation removes
the persisted record and deletes the multipart temp file immediately, which can
race with an active URLSessionTask; change removeTask(taskId:) so it first
checks the task's state via the session (or the in-memory activeTasks map) and
if the task is running or suspended, call cancel() on that URLSessionTask and
return (or mark for removal) so that cleanup happens in URLSessionTaskDelegate's
didCompleteWithError; only remove the store entry and delete multipartFilePath
when the task is in a terminal state (completed/cancelled/failed) or from
didCompleteWithError, and avoid deleting files before delegate reconciliation
(references: removeTask(taskId:), store.remove(taskId:),
URLSessionTask.cancel(), URLSessionTaskDelegate.didCompleteWithError).
- Around line 294-300: The multipart filename needs CR/LF stripped as well as
quotes to avoid corrupting the Content-Disposition header; update the
escapedFileName transformation (where request.fileName is used to build the
header for Content-Disposition in BackgroundUploadManager.swift) to remove "\r"
and "\n" in addition to replacing "\"" — e.g., normalize by replacing CR and LF
characters (and quotes) before interpolating into the header string so the
Content-Disposition: form-data; name="file"; filename="\(escapedFileName)" line
cannot be broken by newline characters.
---
Duplicate comments:
In `@apps/expo/src/App.tsx`:
- Around line 75-85: The age-based cleanup using staleTerminalTasks and
TERMINAL_TASK_RETENTION_MS can still race suspended JS completion waiters;
instead, ensure terminal records are not removed until JS has definitely
observed them by adding a durable handshake: update the terminal record with a
"finalized"/"tombstone" flag or lastObservedByJS timestamp when the JS waiter
reads the terminal snapshot, and only call removeBackgroundUploadTasks for
taskIds whose record shows that handshake has completed (or extend retention
per-record until that flag is present); modify the code paths that read terminal
snapshots to write the handshake marker and change removeBackgroundUploadTasks
to check that marker before deleting tasks.
🪄 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
Run ID: cf1b2995-d0f1-4782-8e66-f49e277049d7
📒 Files selected for processing (6)
apps/expo/src/App.tsxpackages/react-native-uploadthing-background/android/build.gradlepackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/BackgroundUploadStore.ktpackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.ktpackages/react-native-uploadthing-background/ios/BackgroundUploadManager.swiftpackages/react-native-uploadthing-background/ios/BackgroundUploadStore.swift
✅ Files skipped from review due to trivial changes (1)
- packages/react-native-uploadthing-background/android/build.gradle
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-native-uploadthing-background/ios/BackgroundUploadStore.swift
Owner
Author
|
@cursoragent please fix the 5 open issues |
|
Fixed and pushed in What I addressed for the remaining open issues:
Validation run:
Branch is clean and pushed. |
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react-native-uploadthing-background/src/index.ts (1)
18-31: Share one hybrid-object cache across the package.
packages/react-native-uploadthing-background/src/uploadthing.tsdefines the sameuploadthingBackgroundsingleton again, so callers that mix the low-level helpers andcreateUploadthingBackgroundClient()can end up with two wrappers for the same native module. Extract this getter/cache into one shared internal module so every entrypoint uses the same instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/src/index.ts` around lines 18 - 31, There are two separate singletons for the native hybrid object (uploadthingBackground) causing duplicate wrappers; extract the cache/getter into a single internal module and have all entrypoints import it. Create one module that exports getUploadthingBackground (and the uploadthingBackground variable privately) and update the other file(s) (e.g., the existing getUploadthingBackground usage and createUploadthingBackgroundClient in uploadthing.ts) to import and call that shared getUploadthingBackground so every consumer uses the same UploadthingBackground instance from NitroModules.createHybridObject("UploadthingBackground").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-native-uploadthing-background/src/uploadthing.ts`:
- Around line 191-194: When handling a terminal task in the isTerminalTask(task)
branch, make the call to getUploadthingBackground().markTaskObserved(taskId)
best-effort: wrap the await in a try/catch (or use promise settling) so any
error from markTaskObserved does not reject completion; on error simply fall
back to returning the original task (task) rather than throwing. Ensure this
change touches the isTerminalTask branch where markTaskObserved is invoked and
does not change downstream return behavior other than swallowing
markTaskObserved failures.
- Around line 154-165: The code parses response.text() into JSON before checking
response.ok, so a non-JSON error body can throw and hide the intended HTTP
error; update the logic in the response handling around text, payload, and
response.ok (the block that computes text, payload, and throws on !response.ok)
to avoid parsing errors masking failures—either move JSON.parse(text) into the
success branch (only parse when response.ok) or wrap JSON.parse(text) in a
try/catch and fall back to raw text for the error message so the thrown Error
uses the HTTP status fallback (`UploadThing request failed with status
${response.status}`) when parsing fails.
---
Nitpick comments:
In `@packages/react-native-uploadthing-background/src/index.ts`:
- Around line 18-31: There are two separate singletons for the native hybrid
object (uploadthingBackground) causing duplicate wrappers; extract the
cache/getter into a single internal module and have all entrypoints import it.
Create one module that exports getUploadthingBackground (and the
uploadthingBackground variable privately) and update the other file(s) (e.g.,
the existing getUploadthingBackground usage and
createUploadthingBackgroundClient in uploadthing.ts) to import and call that
shared getUploadthingBackground so every consumer uses the same
UploadthingBackground instance from
NitroModules.createHybridObject("UploadthingBackground").
🪄 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
Run ID: 42a1b2c2-b331-47c9-aa4a-a85263e10b86
⛔ Files ignored due to path filters (12)
packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JBackgroundUploadTask.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JHybridUploadthingBackgroundSpec.cppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/c++/JHybridUploadthingBackgroundSpec.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/kotlin/com/margelo/nitro/uploadthingbackground/BackgroundUploadTask.ktis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/android/kotlin/com/margelo/nitro/uploadthingbackground/HybridUploadthingBackgroundSpec.ktis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/c++/HybridUploadthingBackgroundSpecSwift.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/BackgroundUploadTask.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/HybridUploadthingBackgroundSpec.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/ios/swift/HybridUploadthingBackgroundSpec_cxx.swiftis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/shared/c++/BackgroundUploadTask.hppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/shared/c++/HybridUploadthingBackgroundSpec.cppis excluded by!**/generated/**packages/react-native-uploadthing-background/nitrogen/generated/shared/c++/HybridUploadthingBackgroundSpec.hppis excluded by!**/generated/**
📒 Files selected for processing (11)
apps/expo/src/App.tsxapps/expo/src/utils/uploadthing.tspackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/BackgroundUploadStore.ktpackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/HybridUploadthingBackground.ktpackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.ktpackages/react-native-uploadthing-background/ios/BackgroundUploadManager.swiftpackages/react-native-uploadthing-background/ios/BackgroundUploadStore.swiftpackages/react-native-uploadthing-background/ios/HybridUploadthingBackground.swiftpackages/react-native-uploadthing-background/src/index.tspackages/react-native-uploadthing-background/src/specs/uploadthing-background.nitro.tspackages/react-native-uploadthing-background/src/uploadthing.ts
✅ Files skipped from review due to trivial changes (1)
- packages/react-native-uploadthing-background/src/specs/uploadthing-background.nitro.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/react-native-uploadthing-background/ios/HybridUploadthingBackground.swift
- packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/HybridUploadthingBackground.kt
- apps/expo/src/App.tsx
- packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift
- apps/expo/src/utils/uploadthing.ts
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.env.example (1)
22-25: Consider alphabetical ordering of environment variables.Static analysis flagged that
ENABLE_BACKGROUND_UPLOAD_TEST_PAGEshould come beforeEXPO_PUBLIC_ALLOW_SELF_MESSAGESalphabetically. However, the current grouping (server-side flag followed by its client-side counterpart) is also a reasonable organizational approach.🔧 Optional: Alphabetical ordering
# Optional testing flags ALLOW_SELF_MESSAGES="false" +ENABLE_BACKGROUND_UPLOAD_TEST_PAGE="false" EXPO_PUBLIC_ALLOW_SELF_MESSAGES="false" -ENABLE_BACKGROUND_UPLOAD_TEST_PAGE="false" EXPO_PUBLIC_ENABLE_BACKGROUND_UPLOAD_TEST_PAGE="false"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 22 - 25, Reorder the four environment variables in .env.example so they are alphabetically ordered (move ENABLE_BACKGROUND_UPLOAD_TEST_PAGE before EXPO_PUBLIC_ALLOW_SELF_MESSAGES), or if you prefer to keep server/client pairs, explicitly document that grouping; update the block containing ALLOW_SELF_MESSAGES, EXPO_PUBLIC_ALLOW_SELF_MESSAGES, ENABLE_BACKGROUND_UPLOAD_TEST_PAGE, and EXPO_PUBLIC_ENABLE_BACKGROUND_UPLOAD_TEST_PAGE accordingly to reflect the chosen ordering.packages/api/src/uploadthing/router.ts (1)
95-95: Consider extracting theufsKeytype assertion to a helper.The same type assertion pattern
(file as unknown as { ufsKey?: string }).ufsKeyis used in both routes (lines 95 and 207). This could be extracted to a typed helper for better maintainability.♻️ Optional helper extraction
// At the top of the file or in a utils module function getFileKey(file: { key: string; ufsUrl: string }): string { return (file as unknown as { ufsKey?: string }).ufsKey ?? file.key; }Then use
getFileKey(file)in both routes.Also applies to: 207-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/uploadthing/router.ts` at line 95, Extract the repeated type assertion (file as unknown as { ufsKey?: string }).ufsKey into a small typed helper (e.g., getFileKey) and use it in both route handlers instead of duplicating the cast; implement getFileKey to accept the original file shape (e.g., { key: string; ufsUrl: string } or the actual file type used in router.ts) and return the ufsKey if present or fall back to file.key, then replace occurrences where fileKey is set (the lines using file variable in the router handlers) to call getFileKey(file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/src/app/background-upload-test.tsx`:
- Around line 127-133: handleRefresh currently awaits
listBackgroundUploadTasks() but never updates the nativeTasks state, so the UI
only refreshes on the next interval; replace the direct call with the existing
refresh helper (e.g., refreshBackgroundTasks or whichever function updates
nativeTasks) or capture the result of listBackgroundUploadTasks() and call
setNativeTasks(updatedTasks) after successful fetch, preserving the same error
handling used by the helper; update handleRefresh to await that helper (or
perform setNativeTasks) before clearing isRefreshing so the task cards update
immediately.
- Around line 141-158: The code eagerly reads all selected assets into memory by
using Promise.all over result.assets and createFile — this can OOM on mobile for
large videos; change to process assets sequentially or in small batches (e.g.,
for-loop awaiting createFile for each asset or chunked processing) instead of
Promise.all so only one (or a few) Blobs are in memory at a time; update the
ImagePicker handling code that maps result.assets to files and the createFile
call to use sequential/batched awaits and keep the existing selectionLimit
behavior.
- Around line 59-63: The onSuccess handler for
trpc.backgroundUploadTest.delete.useMutation currently assumes a successful
delete; change the handler signature to inspect the first argument (the mutation
result) instead of ignoring it, e.g. in the deleteUpload onSuccess callback
check if result.ok is false and append a failure log with result.reason (or a
default message) and skip refetch; only when result.ok is true append the
"Deleted uploaded file ..." message and call uploadsQuery.refetch() so
races/repeated taps are reported correctly (refer to deleteUpload,
trpc.backgroundUploadTest.delete.useMutation, uploadsQuery.refetch, and
appendLog).
In `@packages/api/src/router/background-upload-test.ts`:
- Around line 11-14: Replace the plain Error in
assertBackgroundUploadTestEnabled with a tRPC error: import TRPCError from
"@trpc/server" and throw new TRPCError({ code: "FORBIDDEN", message: "Background
upload test page is disabled." }) when
process.env.ENABLE_BACKGROUND_UPLOAD_TEST_PAGE !== "true"; this will surface a
FORBIDDEN response to clients instead of an INTERNAL_SERVER_ERROR.
- Around line 50-60: The call to UTApi.deleteFiles currently swallows errors via
.catch(() => undefined), allowing the DB deletion
(ctx.db.delete(BackgroundUploadTestFile)... ) to proceed even if UploadThing
deletion fails; remove the trailing .catch so that await
utapi.deleteFiles(file.fileKey) will propagate errors and prevent the subsequent
deletion of BackgroundUploadTestFile on failure, ensuring the mutation fails and
the DB row is not hard-deleted when file deletion fails.
---
Nitpick comments:
In @.env.example:
- Around line 22-25: Reorder the four environment variables in .env.example so
they are alphabetically ordered (move ENABLE_BACKGROUND_UPLOAD_TEST_PAGE before
EXPO_PUBLIC_ALLOW_SELF_MESSAGES), or if you prefer to keep server/client pairs,
explicitly document that grouping; update the block containing
ALLOW_SELF_MESSAGES, EXPO_PUBLIC_ALLOW_SELF_MESSAGES,
ENABLE_BACKGROUND_UPLOAD_TEST_PAGE, and
EXPO_PUBLIC_ENABLE_BACKGROUND_UPLOAD_TEST_PAGE accordingly to reflect the chosen
ordering.
In `@packages/api/src/uploadthing/router.ts`:
- Line 95: Extract the repeated type assertion (file as unknown as { ufsKey?:
string }).ufsKey into a small typed helper (e.g., getFileKey) and use it in both
route handlers instead of duplicating the cast; implement getFileKey to accept
the original file shape (e.g., { key: string; ufsUrl: string } or the actual
file type used in router.ts) and return the ufsKey if present or fall back to
file.key, then replace occurrences where fileKey is set (the lines using file
variable in the router handlers) to call getFileKey(file).
🪄 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
Run ID: cdbc2825-e70f-4787-933e-203a3e04d8a7
📒 Files selected for processing (11)
.env.exampleapps/expo/src/app/background-upload-test.tsxapps/expo/src/app/profile.tsxapps/expo/src/navigation/RootNavigator.tsxapps/expo/src/navigation/types.tspackages/api/src/index.tspackages/api/src/root.tspackages/api/src/router/background-upload-test.tspackages/api/src/uploadthing/router.tspackages/db/src/schema.tsturbo.json
✅ Files skipped from review due to trivial changes (1)
- turbo.json
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/src/utils/media-upload.ts`:
- Around line 169-175: Trim and normalize groupId once before using it: compute
a normalizedGroupId = groupId?.trim() and derive isGroupSend from
Boolean(normalizedGroupId), then use normalizedGroupId in the validation check
and when building uploadInput (replace uses of groupId with normalizedGroupId)
so empty/whitespace-only IDs are treated as absent and non-empty IDs are sent
trimmed; update the validation error to reference normalizedGroupId and keep the
uploadInput ternary using { groupId: normalizedGroupId, mimeType, thumbhash } or
{ recipients, mimeType, thumbhash }.
In
`@packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift`:
- Around line 361-368: The multipart header currently mixes explicit "\r"
characters inside the multiline string and also calls .replacingOccurrences(of:
"\n", with: "\r\n"), which produces double carriage returns; fix by removing the
embedded "\r" characters from the multiline header (use normal line breaks in
the header string that references boundary, escapedFileName and escapedMimeType)
so the existing .replacingOccurrences(of: "\n", with: "\r\n") produces correct
CRLFs, and make the footer use the same CRLF approach (i.e., ensure footer uses
plain "\n" line breaks or already-CRLF text consistent with the header
conversion).
🪄 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
Run ID: 57690a78-e700-45c1-bde2-41284069c2ec
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
apps/expo/src/app/background-upload-test.tsxapps/expo/src/utils/media-upload.tspackages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/expo/src/app/background-upload-test.tsx
Co-authored-by: Augie <AugusDogus@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift (1)
473-475:⚠️ Potential issue | 🔴 CriticalDo not continue the write loop after a negative write result.
At Line 473,
bytesWritten < 0only throws whenstreamErroris present. If absent, Line 492 adds a negative value tototalBytesWritten, which can cause non-terminating writes and invalid pointer arithmetic at Line 469.🛠️ Suggested fix
- if bytesWritten < 0, let error = outputStream.streamError { - throw error - } + if bytesWritten < 0 { + throw outputStream.streamError + ?? BackgroundUploadManagerError.missingMultipartStream + }Also applies to: 490-493
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift` around lines 473 - 475, In the write loop inside BackgroundUploadManager (the block using bytesWritten, totalBytesWritten and outputStream), stop treating a negative bytesWritten as harmless; if bytesWritten < 0 you must not continue the loop or add it to totalBytesWritten—throw an error (create/throw a generic NSError if outputStream.streamError is nil) or otherwise return/fail immediately so pointer arithmetic and the loop termination are never performed with a negative value; apply the same fix to the other occurrence around the bytesWritten handling (the 490-493 block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift`:
- Around line 398-402: The read loop using inputStream.hasBytesAvailable reads
into buffer and only throws when bytesRead < 0 && streamError != nil, but if
bytesRead < 0 and streamError == nil you later build Data(buffer[0..<bytesRead])
and crash; change the logic in the loop (the code around
inputStream.read(&buffer, maxLength: bufferSize), bytesRead variable and the
Data(buffer[0..<bytesRead]) usage) to unconditionally throw or return on
bytesRead < 0 (use a generic error if streamError is nil) and only proceed to
create Data when bytesRead > 0, ensuring bytesRead == 0 breaks the loop safely.
---
Duplicate comments:
In
`@packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift`:
- Around line 473-475: In the write loop inside BackgroundUploadManager (the
block using bytesWritten, totalBytesWritten and outputStream), stop treating a
negative bytesWritten as harmless; if bytesWritten < 0 you must not continue the
loop or add it to totalBytesWritten—throw an error (create/throw a generic
NSError if outputStream.streamError is nil) or otherwise return/fail immediately
so pointer arithmetic and the loop termination are never performed with a
negative value; apply the same fix to the other occurrence around the
bytesWritten handling (the 490-493 block).
🪄 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
Run ID: 4e134dbd-247f-4bf2-878c-1dbc073d292a
📒 Files selected for processing (2)
apps/expo/src/utils/media-upload.tspackages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/expo/src/utils/media-upload.ts
Treat timeout and task-removal polling failures as transient so send state is not falsely marked failed, and harden iOS stream error handling to throw on negative reads and writes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift`:
- Around line 57-121: enqueueUpload currently calls store.upsert(record)
(creating StoredBackgroundUploadTaskRecord) which lets a second enqueue with the
same taskId overwrite an in-flight record and its multipart file (multipart file
paths are derived deterministically from taskId); fix by detecting existing
records before writing: inside BackgroundUploadManager.enqueueUpload(check for
existing = store.task(taskId: request.taskId)), and if an existing record exists
with status .uploading (or any non-terminal state) then either throw/return an
error or generate a truly unique multipartFilePath (e.g., append a UUID) and a
new task id, rather than calling upsert blindly; update any code paths that
create multipart body (createMultipartBody) to accept an explicit unique
filename/multipartFilePath so deterministic collisions are avoided.
- Around line 245-257: In urlSession(_:task:didCompleteWithError:) ensure
buffered response bytes are always cleared: call
responseData.removeValue(forKey: task.taskIdentifier) (or otherwise clear the
buffer) regardless of whether store.record(forSessionTaskIdentifier:
task.taskIdentifier) returns a record; i.e., perform the removeValue before the
guard returns or add a cleanup branch after the guard to avoid leaking data.
Reference the urlSession(_:task:didCompleteWithError:) method,
store.record(forSessionTaskIdentifier:), and responseData.removeValue(forKey:)
when making the change.
🪄 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
Run ID: af2f78bc-c4a2-4c33-96f7-89bce68aad71
📒 Files selected for processing (4)
apps/expo/src/utils/media-upload.tsapps/expo/src/utils/uploadthing.tspackages/react-native-uploadthing-background/ios/BackgroundUploadManager.swiftpackages/react-native-uploadthing-background/src/uploadthing.ts
✅ Files skipped from review due to trivial changes (1)
- apps/expo/src/utils/uploadthing.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/expo/src/utils/media-upload.ts
Reject enqueuing a task ID that is already active to avoid record/body collisions, and always clear buffered response bytes in task completion even when no store record is found.
Align the custom native uploader with UploadThing's expected upload semantics, and unblock iOS/Android background upload troubleshooting by fixing native build/runtime issues plus adding better test-page controls for large-file validation.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.kt (1)
159-178: Retry logic usesrunAttemptCountbut WorkManager isn't configured with matching backoff.The retry logic at line 160 checks
runAttemptCount < 2, but per the context snippet fromHybridUploadthingBackground.kt, theOneTimeWorkRequestBuilderdoesn't configuresetBackoffCriteria(). This means WorkManager will use its default exponential backoff (starting at 10 seconds), which may not align with expectations.Consider either:
- Documenting that default WorkManager backoff applies
- Configuring explicit backoff policy in
HybridUploadthingBackground.kt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.kt` around lines 159 - 178, The retry decision in UploadthingBackgroundWorker (the runAttemptCount < 2 check inside the IOException catch that updates BackgroundUploadStore and returns Result.retry()/Result.failure()) assumes a specific backoff behavior but WorkManager's OneTimeWorkRequestBuilder (in HybridUploadthingBackground.kt) doesn't setBackoffCriteria, so default exponential backoff will apply; fix by either (preferred) configuring the OneTimeWorkRequestBuilder used to enqueue these uploads to call setBackoffCriteria(BackoffPolicy.LINEAR or EXPONENTIAL, <desired duration>, TimeUnit.MILLISECONDS) with a duration that matches the intended retry timing, or adjust the retry-count logic in UploadthingBackgroundWorker to explicitly account for WorkManager's default backoff; reference OneTimeWorkRequestBuilder in HybridUploadthingBackground.kt and runAttemptCount / BackgroundUploadStore update logic in UploadthingBackgroundWorker.kt when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.kt`:
- Around line 159-178: The retry decision in UploadthingBackgroundWorker (the
runAttemptCount < 2 check inside the IOException catch that updates
BackgroundUploadStore and returns Result.retry()/Result.failure()) assumes a
specific backoff behavior but WorkManager's OneTimeWorkRequestBuilder (in
HybridUploadthingBackground.kt) doesn't setBackoffCriteria, so default
exponential backoff will apply; fix by either (preferred) configuring the
OneTimeWorkRequestBuilder used to enqueue these uploads to call
setBackoffCriteria(BackoffPolicy.LINEAR or EXPONENTIAL, <desired duration>,
TimeUnit.MILLISECONDS) with a duration that matches the intended retry timing,
or adjust the retry-count logic in UploadthingBackgroundWorker to explicitly
account for WorkManager's default backoff; reference OneTimeWorkRequestBuilder
in HybridUploadthingBackground.kt and runAttemptCount / BackgroundUploadStore
update logic in UploadthingBackgroundWorker.kt when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 917b080d-b6b2-48f7-9ddc-887542c91ee2
⛔ Files ignored due to path filters (1)
packages/react-native-uploadthing-background/nitrogen/generated/ios/NitroUploadthingBackground-Swift-Cxx-Umbrella.hppis excluded by!**/generated/**
📒 Files selected for processing (9)
apps/expo/src/app/background-upload-test.tsxapps/expo/src/utils/uploadthing.tspackages/api/src/uploadthing/router.tspackages/react-native-uploadthing-background/android/build.gradlepackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/HybridUploadthingBackground.ktpackages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/UploadthingBackgroundWorker.ktpackages/react-native-uploadthing-background/ios/BackgroundUploadManager.swiftpackages/react-native-uploadthing-background/ios/BackgroundUploadStore.swiftpackages/react-native-uploadthing-background/src/uploadthing.ts
✅ Files skipped from review due to trivial changes (1)
- apps/expo/src/app/background-upload-test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-native-uploadthing-background/android/src/main/java/com/margelo/nitro/uploadthingbackground/HybridUploadthingBackground.kt
…uploads - Build multipart/form-data with explicit CRLF (header ends with \r\n\r\n before file bytes; footer \r\n--boundary--\r\n) to match Android and UploadThing ingest. - Resolve MIME from filename when RN reports empty or octet-stream so presign matches PUT. - Nitro pod: add ExpoModulesCore Swift compatibility header search path for Nitro subclass. - Add @uploadthing/mime-types to Expo and background package. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/expo/src/utils/uploadthing.ts (1)
89-105: Extract the MIME fallback into a shared helper.The unreliable-MIME rules here now mirror
getMimeTypeForUpload()inpackages/react-native-uploadthing-background/src/uploadthing.ts. Those paths need to stay aligned for presign vs. native PUT behavior, so keeping two copies makes the next MIME fix easy to land in only one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/utils/uploadthing.ts` around lines 89 - 105, The MIME fallback logic (rawBlobType/unreliableMime/inferredFromName/resolvedMimeType) should be extracted into a shared helper and reused (the same logic as getMimeTypeForUpload in packages/react-native-uploadthing-background/src/uploadthing.ts) so presign and native PUT stay aligned; create a single exported helper (e.g., getMimeTypeForUpload or use the existing one from the background package), replace the inline computation here to call that helper with the blob.type and fileName (and type), and remove the duplicated logic from apps/expo/src/utils/uploadthing.ts so both codepaths call the shared function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/src/utils/uploadthing.ts`:
- Around line 116-153: The createUriBackedFile function currently constructs new
File([]...) which yields a 0-byte size when size is omitted or when
Object.defineProperty fails; update createUriBackedFile to require a valid size
(typeof size === "number", Number.isFinite(size), size >= 0) and throw a
descriptive error if size is missing/invalid, and also re-throw or surface an
error if Object.defineProperty on rnFormDataCompatibleFile.size fails (do not
silently swallow the failure)—this ensures callers of createUriBackedFile (and
later code that reads file.size) cannot proceed with an incorrect 0-byte size.
In `@packages/react-native-uploadthing-background/src/uploadthing.ts`:
- Around line 272-278: The presign POST is being made before validating local
file URIs; run ensureFileUri on each entry in params.files and fail fast if any
file is not RN FormData-compatible before calling requestUploadTargets so the
server never creates upload targets for invalid inputs; specifically, validate
params.files (iterate and call ensureFileUri for each file object) in the same
function that calls requestUploadTargets and return/throw an error when
validation fails to abort the flow early.
- Around line 311-317: The catch block currently runs tasks.map(async (task) =>
{ await getUploadthingBackground().cancelUpload(task.taskId); await
getUploadthingBackground().removeTask(task.taskId); }) so if cancelUpload
rejects the per-task removeTask is never called; change the per-task cleanup to
be best-effort by wrapping each task's cancel/remove in its own try/catch (or
use Promise.allSettled on two promises) so removeTask is always attempted
regardless of cancelUpload failure; reference the async callback in the
tasks.map, getUploadthingBackground().cancelUpload, and
getUploadthingBackground().removeTask to locate where to add the per-task error
handling.
---
Nitpick comments:
In `@apps/expo/src/utils/uploadthing.ts`:
- Around line 89-105: The MIME fallback logic
(rawBlobType/unreliableMime/inferredFromName/resolvedMimeType) should be
extracted into a shared helper and reused (the same logic as
getMimeTypeForUpload in
packages/react-native-uploadthing-background/src/uploadthing.ts) so presign and
native PUT stay aligned; create a single exported helper (e.g.,
getMimeTypeForUpload or use the existing one from the background package),
replace the inline computation here to call that helper with the blob.type and
fileName (and type), and remove the duplicated logic from
apps/expo/src/utils/uploadthing.ts so both codepaths call the shared function.
🪄 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
Run ID: 2a294ac2-9721-4d9d-9b5e-f5fd99b4e5c8
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/expo/package.jsonapps/expo/src/utils/uploadthing.tspackages/react-native-uploadthing-background/NitroUploadthingBackground.podspecpackages/react-native-uploadthing-background/ios/BackgroundUploadManager.swiftpackages/react-native-uploadthing-background/package.jsonpackages/react-native-uploadthing-background/src/uploadthing.ts
✅ Files skipped from review due to trivial changes (2)
- apps/expo/package.json
- packages/react-native-uploadthing-background/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift
Fail fast when uri-backed files are missing valid size or uri metadata, keep MIME resolution consistent across upload paths, and simplify the background upload test screen to use only the normal media picker. Made-with: Cursor
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
apps/expo/src/app/background-upload-test.tsx (1)
1-1:⚠️ Potential issue | 🟡 MinorAddress formatting issues flagged by CI pipeline.
The pipeline reports
oxfmt --checkfound formatting issues. Run the formatter to resolve:bun format🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/app/background-upload-test.tsx` at line 1, Run the project formatter to fix CI formatting errors: execute "bun format" and commit the resulting changes so oxfmt passes; specifically reformat the file containing the import line referencing NativeStackNavigationProp in the BackgroundUploadTest component (apps/expo/src/app/background-upload-test.tsx) to resolve whitespace/line-breaks and any other style issues flagged by the pipeline, then re-run the CI check.packages/react-native-uploadthing-background/src/uploadthing.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorAddress formatting issues flagged by CI pipeline.
The pipeline reports
oxfmt --checkfound formatting issues. Run the formatter to resolve:bun format🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/src/uploadthing.ts` at line 1, CI flagged formatting issues in uploadthing.ts (the import line "import { lookup } from \"@uploadthing/mime-types\";" is one affected spot); run the repository formatter to fix them by running the formatter (bun format) locally, verify with the CI check (oxfmt --check) passes, and commit the resulting changes so the import formatting and any other style issues are resolved.apps/expo/src/utils/uploadthing.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorAddress formatting issues flagged by CI pipeline.
The pipeline reports
oxfmt --checkfound formatting issues. Run the formatter to resolve:bun format🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/utils/uploadthing.ts` at line 1, CI flagged formatting errors in the uploadthing.ts module (import block) — run the project formatter and commit the results: execute `bun format` at the repo root to fix oxfmt issues, inspect and stage the updated uploadthing.ts changes, commit and push the branch so the pipeline can re-run and clear the formatting error.
🧹 Nitpick comments (7)
apps/expo/src/utils/uploadthing.ts (1)
19-32: Cookie header check is more restrictive than other usages in codebase.In
apps/expo/src/utils/api.tsx:37-40, the pattern isif (cookies) { headers.set("Cookie", cookies); }which handles any truthy value. Here, the checktypeof cookies === "string" && cookies.length > 0is stricter but functionally equivalent sincegetCookie()returnsstring | null | undefined. The current implementation is correct but slightly inconsistent with the codebase pattern.Consider aligning with codebase pattern for consistency
export function uploadthingFetch(input: RequestInfo | URL, init?: RequestInit) { const cookies = authClient.getCookie(); const headers = new Headers(init?.headers); - if (typeof cookies === "string" && cookies.length > 0) { + if (cookies) { headers.set("Cookie", cookies); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/utils/uploadthing.ts` around lines 19 - 32, The cookie presence check in uploadthingFetch is stricter than the rest of the codebase; replace the current `typeof cookies === "string" && cookies.length > 0` check with a simple truthy check `if (cookies)` so uploadthingFetch (which calls authClient.getCookie()) matches the pattern used elsewhere (e.g., api.tsx) and still handles string | null | undefined correctly; update the condition inside the uploadthingFetch function that sets the "Cookie" header.packages/react-native-uploadthing-background/src/uploadthing.ts (3)
96-105: Type assertion inisTerminalTaskis overly complex.The cast
task.status as typeof TERMINAL_STATUSES extends Set<infer T> ? T : neveris verbose. SinceTERMINAL_STATUSES.has()acceptsstring, the cast is unnecessary for runtime correctness.Simplify the type guard
function isTerminalTask( task: BackgroundUploadTask | null, ): task is BackgroundUploadTask { return ( task != null && - TERMINAL_STATUSES.has( - task.status as typeof TERMINAL_STATUSES extends Set<infer T> ? T : never, - ) + TERMINAL_STATUSES.has(task.status) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/src/uploadthing.ts` around lines 96 - 105, The isTerminalTask type guard uses an overly complex type assertion for task.status; simplify it by removing the conditional type cast and call TERMINAL_STATUSES.has with the status directly (or a simple cast like task.status as string if TS still complains). Update the isTerminalTask function to return task != null && TERMINAL_STATUSES.has(task.status) (or task.status as string) and keep the function name and TERMINAL_STATUSES reference unchanged.
82-88: Fallback UUID generation has weak entropy.The fallback when
crypto.randomUUIDis unavailable usesMath.random(), which is not cryptographically secure. While task IDs are not security-critical, collisions in high-throughput scenarios are theoretically possible.Consider using a more robust fallback
If collision resistance matters, consider importing a lightweight UUID library or using a longer random suffix:
function createTaskId(): string { if (typeof globalThis.crypto?.randomUUID === "function") { return globalThis.crypto.randomUUID(); } - return `utbg-${Date.now()}-${Math.random().toString(36).slice(2, 10)}`; + return `utbg-${Date.now()}-${Math.random().toString(36).slice(2, 10)}-${Math.random().toString(36).slice(2, 10)}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/src/uploadthing.ts` around lines 82 - 88, The fallback in createTaskId uses Math.random() which has weak entropy; replace it with a stronger source by detecting and using globalThis.crypto.getRandomValues (or a small UUID library if getRandomValues is unavailable) to generate the random suffix for the `utbg-<timestamp>-<random>` ID; update createTaskId to build the suffix from secure bytes (e.g., a few bytes hex/base36 from Uint8Array via crypto.getRandomValues) so collisions are far less likely in high-throughput scenarios.
148-150: Return normalized MIME type, not originalfile.type.When
unreliableis false, the function returnsfile.type as string(the original, possibly mixed-case value), butrawis already trimmed and lowercased. Returningrawwould be more consistent.Return the normalized value
if (!unreliable) { - return file.type as string; + return raw; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-native-uploadthing-background/src/uploadthing.ts` around lines 148 - 150, The function currently returns the original file.type when unreliable is false, which can be mixed-case; instead return the already normalized value raw (which is trimmed and lowercased). Update the return in the branch that checks unreliable (where it currently returns file.type as string) to return raw, keeping the existing normalization logic that produces raw intact and referenced by the variables unreliable, file.type and raw.apps/expo/src/app/background-upload-test.tsx (3)
84-105: State comparison string could miss status-only changes.The
currentStatestring format${task.status}:${task.bytesSent}:${task.totalBytes}will log changes when bytes change, but if onlyerrorMessageorresponseCodechanges, it won't be logged. This is likely acceptable for a test screen, but worth noting.Consider including error state in change detection
nextStates.set( task.taskId, - `${task.status}:${task.bytesSent}:${task.totalBytes}`, + `${task.status}:${task.bytesSent}:${task.totalBytes}:${task.errorMessage ?? ""}`, ); const previousState = previousTaskStates.current.get(task.taskId); - const currentState = `${task.status}:${task.bytesSent}:${task.totalBytes}`; + const currentState = `${task.status}:${task.bytesSent}:${task.totalBytes}:${task.errorMessage ?? ""}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/app/background-upload-test.tsx` around lines 84 - 105, applyNativeTasksSnapshot currently compares previousTaskStates.current to a state string built from task.status, bytesSent and totalBytes, so changes to errorMessage or responseCode won't trigger logs; update the comparison to include those fields as well (e.g., include task.errorMessage and task.responseCode in the state string or serialize a minimal object of {status, bytesSent, totalBytes, errorMessage, responseCode}) when building currentState and nextStates so status-only error/response changes are detected and logged; the relevant symbols are applyNativeTasksSnapshot, previousTaskStates.current, nextStates, and appendLog.
245-248:useMemofor a static string is unnecessary.The
sectionTitleClassNamestring never changes. UsinguseMemowith an empty dependency array is effectively a constant definition. A simpleconstoutside the component or at the top of the function would suffice.Simplify to a constant
+const SECTION_TITLE_CLASS_NAME = + "pb-2 text-sm font-semibold uppercase tracking-wide text-muted"; + export default function BackgroundUploadTestScreen() { // ... state declarations ... - - const sectionTitleClassName = useMemo( - () => "pb-2 text-sm font-semibold uppercase tracking-wide text-muted", - [], - );Then use
SECTION_TITLE_CLASS_NAMEin place ofsectionTitleClassName.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/app/background-upload-test.tsx` around lines 245 - 248, Replace the unnecessary useMemo for sectionTitleClassName with a plain constant by defining SECTION_TITLE_CLASS_NAME = "pb-2 text-sm font-semibold uppercase tracking-wide text-muted" (preferably at module scope or at the top of the component) and then replace all uses of sectionTitleClassName with SECTION_TITLE_CLASS_NAME; remove the useMemo import/usage to simplify the code and avoid creating a constant via a hook.
209-229: Fire-and-forget completion watcher may leave tasks in limbo on errors.If
batch.completionrejects (e.g., due to timeout or task removal), the.catch()only logs the error. According to the relevant code snippets,media-upload.tshandles this by settingshouldCleanupTasks = falsefor these specific errors, but this test screen doesn't perform any cleanup at all, leaving tasks potentially stuck in native storage.For a test screen this is likely acceptable since manual cleanup is possible, but consider documenting this behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/app/background-upload-test.tsx` around lines 209 - 229, The completion watcher currently only logs rejections and can leave native tasks stuck; update the batch.completion.catch handler to actively resync and clean up: after logging the error, call uploadsQuery.refetch() to refresh state and then invoke a cleanup helper (e.g., cleanupStaleBackgroundUploads() — create it if missing) to remove or reconcile stuck native tasks; reference the existing batch.completion, appendLog, and uploadsQuery.refetch symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/expo/src/app/background-upload-test.tsx`:
- Line 1: Run the project formatter to fix CI formatting errors: execute "bun
format" and commit the resulting changes so oxfmt passes; specifically reformat
the file containing the import line referencing NativeStackNavigationProp in the
BackgroundUploadTest component (apps/expo/src/app/background-upload-test.tsx) to
resolve whitespace/line-breaks and any other style issues flagged by the
pipeline, then re-run the CI check.
In `@apps/expo/src/utils/uploadthing.ts`:
- Line 1: CI flagged formatting errors in the uploadthing.ts module (import
block) — run the project formatter and commit the results: execute `bun format`
at the repo root to fix oxfmt issues, inspect and stage the updated
uploadthing.ts changes, commit and push the branch so the pipeline can re-run
and clear the formatting error.
In `@packages/react-native-uploadthing-background/src/uploadthing.ts`:
- Line 1: CI flagged formatting issues in uploadthing.ts (the import line
"import { lookup } from \"@uploadthing/mime-types\";" is one affected spot); run
the repository formatter to fix them by running the formatter (bun format)
locally, verify with the CI check (oxfmt --check) passes, and commit the
resulting changes so the import formatting and any other style issues are
resolved.
---
Nitpick comments:
In `@apps/expo/src/app/background-upload-test.tsx`:
- Around line 84-105: applyNativeTasksSnapshot currently compares
previousTaskStates.current to a state string built from task.status, bytesSent
and totalBytes, so changes to errorMessage or responseCode won't trigger logs;
update the comparison to include those fields as well (e.g., include
task.errorMessage and task.responseCode in the state string or serialize a
minimal object of {status, bytesSent, totalBytes, errorMessage, responseCode})
when building currentState and nextStates so status-only error/response changes
are detected and logged; the relevant symbols are applyNativeTasksSnapshot,
previousTaskStates.current, nextStates, and appendLog.
- Around line 245-248: Replace the unnecessary useMemo for sectionTitleClassName
with a plain constant by defining SECTION_TITLE_CLASS_NAME = "pb-2 text-sm
font-semibold uppercase tracking-wide text-muted" (preferably at module scope or
at the top of the component) and then replace all uses of sectionTitleClassName
with SECTION_TITLE_CLASS_NAME; remove the useMemo import/usage to simplify the
code and avoid creating a constant via a hook.
- Around line 209-229: The completion watcher currently only logs rejections and
can leave native tasks stuck; update the batch.completion.catch handler to
actively resync and clean up: after logging the error, call
uploadsQuery.refetch() to refresh state and then invoke a cleanup helper (e.g.,
cleanupStaleBackgroundUploads() — create it if missing) to remove or reconcile
stuck native tasks; reference the existing batch.completion, appendLog, and
uploadsQuery.refetch symbols when making the change.
In `@apps/expo/src/utils/uploadthing.ts`:
- Around line 19-32: The cookie presence check in uploadthingFetch is stricter
than the rest of the codebase; replace the current `typeof cookies === "string"
&& cookies.length > 0` check with a simple truthy check `if (cookies)` so
uploadthingFetch (which calls authClient.getCookie()) matches the pattern used
elsewhere (e.g., api.tsx) and still handles string | null | undefined correctly;
update the condition inside the uploadthingFetch function that sets the "Cookie"
header.
In `@packages/react-native-uploadthing-background/src/uploadthing.ts`:
- Around line 96-105: The isTerminalTask type guard uses an overly complex type
assertion for task.status; simplify it by removing the conditional type cast and
call TERMINAL_STATUSES.has with the status directly (or a simple cast like
task.status as string if TS still complains). Update the isTerminalTask function
to return task != null && TERMINAL_STATUSES.has(task.status) (or task.status as
string) and keep the function name and TERMINAL_STATUSES reference unchanged.
- Around line 82-88: The fallback in createTaskId uses Math.random() which has
weak entropy; replace it with a stronger source by detecting and using
globalThis.crypto.getRandomValues (or a small UUID library if getRandomValues is
unavailable) to generate the random suffix for the `utbg-<timestamp>-<random>`
ID; update createTaskId to build the suffix from secure bytes (e.g., a few bytes
hex/base36 from Uint8Array via crypto.getRandomValues) so collisions are far
less likely in high-throughput scenarios.
- Around line 148-150: The function currently returns the original file.type
when unreliable is false, which can be mixed-case; instead return the already
normalized value raw (which is trimmed and lowercased). Update the return in the
branch that checks unreliable (where it currently returns file.type as string)
to return raw, keeping the existing normalization logic that produces raw intact
and referenced by the variables unreliable, file.type and raw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62dede5f-7de9-46c8-8387-0660359a97e7
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/expo/package.jsonapps/expo/src/app/background-upload-test.tsxapps/expo/src/utils/uploadthing.tspackages/react-native-uploadthing-background/NitroUploadthingBackground.podspecpackages/react-native-uploadthing-background/ios/BackgroundUploadManager.swiftpackages/react-native-uploadthing-background/package.jsonpackages/react-native-uploadthing-background/src/uploadthing.ts
✅ Files skipped from review due to trivial changes (3)
- apps/expo/package.json
- packages/react-native-uploadthing-background/package.json
- packages/react-native-uploadthing-background/ios/BackgroundUploadManager.swift
Track more native task fields in the test screen, resync stale terminal tasks after completion failures, and tighten a few upload helper consistency and ID generation details. Made-with: Cursor
Keep the original runtime error attached when setting the synthetic File.size fails so debugging upload metadata issues is easier. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/expo/src/app/background-upload-test.tsx (1)
223-235:⚠️ Potential issue | 🟠 MajorThis still retains the whole selection in JS memory.
On Lines 223-235, the loop is sequential now, but each
createFile()still materializes a Blob-backedFile, andfiles.push(...)keeps all of them alive untiluploadFilesWithInputInBackground()runs. With a 10-item selection, large videos can still exhaust the test screen. Prefer a URI-backed path here (for example, a compressed-URI helper layered oncreateUriBackedFile) so the batch only keeps metadata in JS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/app/background-upload-test.tsx` around lines 223 - 235, The code is still holding Blob-backed File objects in memory via the files array (created by createFile) which can OOM for large selections; change the flow so you don't materialize blobs for every asset: replace createFile usage with a URI-backed approach (e.g., use createUriBackedFile or a compressed-URI helper that returns lightweight metadata/URI objects) and stream or lazily construct the actual upload payload inside uploadFilesWithInputInBackground so only one file buffer is materialized at a time; update the loop that currently pushes await createFile(...) into files to instead push URI-backed descriptors (or compressed URIs) and adjust uploadFilesWithInputInBackground to accept and resolve those descriptors into a real File/stream per upload.apps/expo/src/utils/uploadthing.ts (1)
133-147:⚠️ Potential issue | 🟡 MinorReject fractional byte sizes here too.
On Lines 133-147,
Number.isFinite(size)still accepts impossible values like1.5. That value is assigned tofile.sizeand forwarded unchanged in the presign request, so callers can still construct invalid upload metadata. Tighten this to an integer check.🛠️ Suggested fix
- if (!(typeof size === "number" && Number.isFinite(size) && size >= 0)) { + if ( + !(typeof size === "number" && Number.isSafeInteger(size) && size >= 0) + ) { throw new Error( - `[Upload] createUriBackedFile requires a finite non-negative size for "${fileName}". Received ${String(size)}.`, + `[Upload] createUriBackedFile requires a non-negative integer size for "${fileName}". Received ${String(size)}.`, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/src/utils/uploadthing.ts` around lines 133 - 147, The validation for size in createUriBackedFile is too permissive because Number.isFinite allows non-integer values (e.g., 1.5); update the check to require a non-negative integer (e.g., ensure typeof size === "number" && Number.isFinite(size) && Number.isInteger(size) && size >= 0) before assigning to rnFormDataCompatibleFile.size and before proceeding with presign; keep the existing error message but mention the integer requirement for fileName in the thrown Error to make failures clear.packages/react-native-uploadthing-background/src/uploadthing.ts (1)
244-246:⚠️ Potential issue | 🟠 Major
getTask() === nullcan now be a false failure.On Lines 244-246, this treats every
nulllookup as "removed before terminal state". With the new app-level reconciliation also marking/removing terminal tasks, a successfully finished upload can disappear between polls and still land here, which makescompletionreject for an already-finished batch.Verify whether another reconciler is removing terminal tasks independently:
#!/bin/bash set -euo pipefail file="apps/expo/src/App.tsx" sed -n '1,260p' "$file" | nl -ba | sed -n '1,260p' echo rg -n -C4 'listBackgroundUploadTasks|markBackgroundUploadTaskObserved|removeBackgroundUploadTask' "$file"If that file is doing its own observe/remove pass, this branch needs coordination or a grace window instead of treating every
nullas a hard failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/src/app/background-upload-test.tsx`:
- Around line 136-161: cleanupStaleBackgroundUploads currently lists and mutates
all terminal background upload tasks globally; change its signature to accept
the current batch's taskIds and, before calling
markBackgroundUploadTaskObserved/removeBackgroundUploadTask or applying
applyNativeTasksSnapshot, filter tasks by those taskIds so only tasks belonging
to the failing batch are observed/removed; update callers (including the
catch-path that invokes cleanupStaleBackgroundUploads) to pass the current
batch's taskIds and ensure listBackgroundUploadTasks results are intersected
with that set prior to any mutation.
In `@packages/react-native-uploadthing-background/src/uploadthing.ts`:
- Around line 144-162: getMimeTypeForUpload currently can return an empty string
when raw is "" and lookup(file.name) returns false; change the final return in
that function (after the lookup check) to never return raw — instead return
fallbackMimeType if present, otherwise the safe default
"application/octet-stream" (i.e. replace the expression fallbackMimeType ?? raw
?? "application/octet-stream" with fallbackMimeType ??
"application/octet-stream") so getMimeTypeForUpload always yields a non-empty
MIME; keep references to variables raw, fallbackMimeType, and the
lookup(file.name) result when locating the code to update.
---
Duplicate comments:
In `@apps/expo/src/app/background-upload-test.tsx`:
- Around line 223-235: The code is still holding Blob-backed File objects in
memory via the files array (created by createFile) which can OOM for large
selections; change the flow so you don't materialize blobs for every asset:
replace createFile usage with a URI-backed approach (e.g., use
createUriBackedFile or a compressed-URI helper that returns lightweight
metadata/URI objects) and stream or lazily construct the actual upload payload
inside uploadFilesWithInputInBackground so only one file buffer is materialized
at a time; update the loop that currently pushes await createFile(...) into
files to instead push URI-backed descriptors (or compressed URIs) and adjust
uploadFilesWithInputInBackground to accept and resolve those descriptors into a
real File/stream per upload.
In `@apps/expo/src/utils/uploadthing.ts`:
- Around line 133-147: The validation for size in createUriBackedFile is too
permissive because Number.isFinite allows non-integer values (e.g., 1.5); update
the check to require a non-negative integer (e.g., ensure typeof size ===
"number" && Number.isFinite(size) && Number.isInteger(size) && size >= 0) before
assigning to rnFormDataCompatibleFile.size and before proceeding with presign;
keep the existing error message but mention the integer requirement for fileName
in the thrown Error to make failures clear.
🪄 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
Run ID: 237beb12-d52d-469e-a218-eaeb5bbc6db1
📒 Files selected for processing (3)
apps/expo/src/app/background-upload-test.tsxapps/expo/src/utils/uploadthing.tspackages/react-native-uploadthing-background/src/uploadthing.ts
Keep the regression test screen lightweight by avoiding blob-backed files, scope stale-task cleanup to the current batch, and harden MIME and size validation for uri-backed uploads. Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new Nitro module
react-native-uploadthing-backgroundto enable native background uploads for Expo apps using UploadThing.This module allows the
whispExpo app to perform robust background file uploads on Android (viaWorkManager) and iOS (viaURLSession), ensuring uploads complete even when the app is not in the foreground. It integrates with the existing UploadThing flow by requesting presigned URLs and then offloading the actual file transfer to the native background services.Summary by CodeRabbit
New Features
Documentation
Chores