Add android.noNotification for silent uploads - #9
Closed
elliottkember wants to merge 99 commits into
Closed
elliottkember wants to merge 99 commits into
elliottkember wants to merge 99 commits into
Conversation
Sync kotlin and java version to 17
Fix upload issues on Android
* form data test * lint * errors not exist * ts fix * better log * fix pod --------- Co-authored-by: thomasvo <thomas.vo@openspace.ai>
* fix * doc * fix * simpler * fix * Address Daniel's PR review comments - Name response variable in use block (UploadUtils.kt) - Fix shadowed it in takeIf lambda (UploadUtils.kt) - Rename set() to setIfNotNull() (UploadProgress.kt) - Add complete() method to Progress class (UploadProgress.kt) - Rename clearIfNeeded() to clearIfCompleted() (UploadProgress.kt) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * set * package --------- Co-authored-by: thomasvo <thomas.vo@openspace.ai> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
## v8.0.0 — reliable upload outcomes on a New Architecture TurboModule Makes upload outcomes impossible to lose and impossible to misread, and replaces the legacy-bridge native module with a codegen TurboModule on both platforms. This is the foundation for the axios-like `request()`/`onSettled` API planned for Phase 1 — that layer's durability rests on the journal shipped here. Design context: openspacelabs/diana#8875. > **Reviewing this**: the diff is large because it is a rewrite of both native layers. > It is kept together on this branch so Diana's `package.json` can point at one ref. > A stack of small, individually readable PRs covering the same content is linked in > the comments — read those instead of this diff. ### Why The library's model was "start an upload, listen to a global event firehose." Events were dropped whenever JS wasn't running (app killed, JS reload, background relaunch), statuses were ambiguous (`completed` fired for 4xx/5xx; `cancelled` meant user-cancel *or* system-kill), and there was no way to ask what happened after the fact. Diana compensated with ~2,000 lines of redux rebuilding durability and reconciliation, and lost real customer data to a 400 that retried silently for days (diana#8235). Separately, the module was legacy-bridge code running under React Native's TurboModule **interop shim**. That shim works today but is explicitly temporary, and 0.85+ has begun removing legacy internals — so "it still runs" was not a foundation worth building on. ### What changed **Reliability core (both platforms)** - Durable native **event journal**: terminal events are persisted *before* being emitted and deleted only when JS acknowledges them — at-least-once delivery. New JS API: `getUnacknowledgedEvents()` / `ackEvents(ids)`. - `getAllUploads()` to reconcile in-flight uploads on boot. - Outcome classification: `completed` only for 2xx or a per-request `acceptStatus`; every other HTTP response is an `error` with `errorKind: 'http'` and the response attached. Typed `errorKind: 'http' | 'network' | 'file' | 'unknown'`. - `cancelled` events carry `cancelReason: 'user' | 'system'`. **New Architecture TurboModule** - `src/NativeRNFileUploader.ts` is the codegen spec: six methods plus five event emitters. Variant payloads are `UnsafeObject` because codegen cannot model `Partial<>`, intersections or index signatures; the precise types stay in `src/types.ts` and are applied at the JS edge, so the public API is unchanged. - No interop-layer dependency, no `codegenConfig`-less legacy registration. **iOS — Swift, split in two** - The split is forced by the toolchain: the generated spec header is Obj-C++ only, and a consumer's `AppDelegate.m` is plain Obj-C, so they cannot meet. - `RNFileUploader.h/.mm` — the TurboModule shell. Every header is `private_header_files` so it never enters the pod's public umbrella; otherwise a consumer's plain-Obj-C `@import` fails to compile. - `RNBackgroundUpload.swift` — `@objc public` singleton owning the background `URLSession` delegate, the journal, and the task map. Session configuration is carried over verbatim from the original Obj-C. - Events travel Swift → `@objc` delegate → `.mm` → the generated emitters, replacing the `RCTEventEmitter` subclass and its weak `latestInstance` indirection. - `responseHeaders` now delivered on iOS (was Android-only); progress throttled to 500ms; removed the non-functional multipart / `assets-library` / `appGroup` paths. **Android** - `UploaderModule` extends the generated spec; `UploaderReactPackage` is a Kotlin `BaseReactPackage` advertising `isTurboModule`; `EventReporter` emits through the codegen emitters. Journal + classification (`UploadOutcome`, unit-tested), `getAllUploads` via id-tagged WorkManager query, user-vs-system cancel attribution, optional notification options with a library-created channel. - `build.gradle` applies `com.facebook.react` for codegen and now inherits the host app's SDK and toolchain instead of pinning AGP 3.5.4 / Kotlin 1.6.20. - **Ships `consumer-rules.pro`.** `Upload` and `EventJournal.Entry` are Gson-persisted and read back across app restarts *and app updates*; without the `Signature` attribute R8 turns `acceptStatus: List<Int>` into `List<Double>` (so a configured 409 is reported as an error) and renames journal fields so entries written by an older build are silently dropped. Neither is reproducible in an unminified build. **Housekeeping** - Removed all Vydia references (Android package → `ai.openspace.backgroundupload`). LICENSE/CHANGELOG keep the original copyright. - Deleted the committed `lib/` (types served from `src`); fixed the pre-commit hook that auto-committed build output; CI runs lint + typecheck + jest + android tests. ### Breaking (v8.0.0) - **Requires React Native ≥ 0.84 with the New Architecture, and React ≥ 19.** The legacy bridge is no longer supported. - The iOS AppDelegate hook moved to `RNBackgroundUpload`: `[RNBackgroundUpload setBackgroundSessionCompletionHandler:forIdentifier:]`. `RNFileUploader` is now the TurboModule and is intentionally unreachable from plain Obj-C. See the README. - Events are delivered through the codegen emitters, so they are no longer observable under the raw `RNFileUploader-*` `DeviceEventEmitter` names. `Upload.addListener` is unchanged. - `completed` fires only for 2xx (+ `acceptStatus`); other statuses are `error`s. - `cancelUpload` resolves `false` when no matching in-flight upload was found. - Deep imports of `lib/*` break — import from the package root. - Minimum iOS deployment target 15.1; minimum Android SDK 29. ### Verification Real-world: **iOS and Android both build and run inside Diana on physical devices.** Field note create (online *and* offline), attachment upload, and field note update all work on both platforms, with events arriving through the codegen emitters. - RN 0.84 codegen accepts the spec on both platforms; on Android it generates into `ai.openspace.backgroundupload` as configured. - Android: compiles via Diana's Gradle build; 15/15 JVM unit tests (`EventJournal`, `UploadOutcome`). - JS: `tsc` clean, 7 Jest tests, eslint clean. - Earlier simulator pass confirmed the journal survives an app kill (re-delivered on next launch), `ackEvents` clears it, and a 404 classifies as `error`/`errorKind: 'http'`. **Not yet verified** — all need a physical device and deliberate scenarios: - true background continuation while suspended/terminated, and system relaunch on background completion; - force-quit → `cancelReason: 'system'` and user cancel → `cancelReason: 'user'`; - iOS event delivery after a JS reload; - Android emit from the WorkManager worker thread (the codegen emitter callback is invoked off the JS thread — outcomes are journaled either way, so a miss degrades rather than loses data); - a release/minified Android build, which is what `consumer-rules.pro` guards. **Known CI gap:** the android-unit-test job runs from `example/RNBGUExample`, which is still on RN 0.75 / old architecture, where codegen does not support event emitters in a TurboModule spec. That job stays red until the example app is bumped to RN 0.84 — which is also the fix for the harness blind spot that let an iOS visibility bug reach Diana in the first place. ### Diana adoption Diana's listener epic has already been updated for the new contract (non-2xx now arrives as `error`, so the 409 / delete-already-gone shortcuts, response-body parsing and log-noise suppression moved out of the `completed` listener). Remaining: 1. On boot: drain `getUnacknowledgedEvents()` → existing listener logic → `ackEvents()`; delete the blind app-killed marking and most server reconciliation. 2. Pass `acceptStatus` where a non-2xx is legitimately success, then use `errorKind` / `cancelReason` instead of the string-parsed taxonomy — in particular, make `errorKind: 'file'` terminal so a missing payload stops being retried forever. 3. Drop the notifee channel-creation retry (the library creates its own channel). --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
A consumer whose queue carries both user-visible media and housekeeping payloads had no way to keep the housekeeping ones out of the notification shade: every upload posts a progress notification, because posting one is how the worker enters foreground mode. noNotification skips the channel registration, the foreground promotion and the notify calls. Such an upload runs as an ordinary background worker, so the OS may defer or stop it — a path the worker already handles by letting WorkManager re-run it. Phrased as an opt-out because WorkManager persists the upload as JSON: a job enqueued by 8.0.0 and replayed by 8.1.0 has no such key, and an absent Gson Boolean is false, so it keeps its notification. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Opened against the wrong base repo; reopening on openspacelabs. |
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
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.
A consumer whose upload queue carries both user-visible media and housekeeping
payloads has no way to keep the housekeeping ones out of the notification shade.
Every upload posts a progress notification, because posting one is how the worker
enters foreground mode. In OpenSpace's app that means a field note syncing on
launch shows "Uploading Captures…", which reads to the user as a capture upload
stuck at 0%.
android: { noNotification: true }uploads a file without a notification. Theworker skips the channel registration, the foreground promotion and the
notifycalls; nothing else about the upload changes. Default
false.The tradeoff, and why it is in the docs
The notification is the foreground service's notification, so a silent upload
runs as an ordinary background worker. The OS may defer it, or stop it mid-flight
for WorkManager to re-run later. That retry path already exists — a system stop
leaves the WorkManager row
RUNNINGand journals nothing, so JS never sees afalse terminal event. It does mean
noNotificationis wrong for anything thattakes real time to upload, so the option's doc comment, the README section and
the changelog all say to reserve it for small payloads.
Why an opt-out rather than
showNotificationWorkManager persists this model as JSON, so a job enqueued by 8.0.0 can be run by
8.1.0 after an app upgrade. That JSON has no key for the new option, and Gson
leaves an absent
Booleanfieldfalse— as an opt-out, absence means "notify",so a replayed job keeps its notification and its foreground protection. As
showNotificationit would have silently lost both.UploadTestcovers exactlythis by stripping the key from serialized JSON.
The double negative stops at the model: the worker reads
upload.showsNotification.Progress accounting is unchanged
UploadProgressstays device-wide, so a visible notification's bar still countsevery in-flight upload, silent ones included. The bar reports what the device is
uploading, not what the notification is titled after; the README says so.
Verification
Ran locally:
yarn typecheck,yarn lint:ci(3 pre-existing warnings in theexample app, 0 errors),
yarn test(7 pass), and./gradlew :react-native-background-upload:testDebugUnitTest— 29 tasks, allgreen, with the 3 new
UploadTestcases confirmed in the result XML.Not verified on hardware. No device or emulator run, so the silent path has
not been observed end to end. Worth checking on an Android device before this is
relied on:
android: { noNotification: true }. Expect nonotification,
progressevents still arriving, and acompletedevent.percentage moves for both uploads.