diff --git a/CHANGELOG.md b/CHANGELOG.md index b5b55938..1732a408 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,15 +84,20 @@ archived by series under [docs/changelog/](docs/changelog/); see the release that shipped the method, and the transport ones are reachable by a remote peer rather than only by the application's own code. -- **`forwardMessage` hangs on iOS in debug builds** rather than forwarding. +- **`forwardMessage` hung forever on iOS debug builds instead of forwarding** + ([#417](https://github.com/Offline-Protocol/offline-protocol-sdk/issues/417)). React Native forces every `NSNumber` argument to non-null, because numbers are not nullable on Android, and refuses a null one before the Swift method - is entered, so neither the resolver nor the rejecter ever runs. The - TypeScript passes `null` whenever a caller omits the priority. No declaration - in the bridge can fix this; it needs a contract change across TypeScript, - Swift and Kotlin, and is tracked in - [#417](https://github.com/Offline-Protocol/offline-protocol-sdk/issues/417). - Release builds are unaffected, as the check is compiled out. + is entered, so neither the resolver nor the rejecter ran and the promise + never settled. The TypeScript passed `null` whenever a caller omitted the + priority, which was the only nullable number in the bridge. No spelling of + the declaration repairs that, so the nullability is gone instead: an omitted + priority now resolves to `MessagePriority.Medium` in TypeScript, exactly as + `sendMessage` has always done, and crosses to Swift and Kotlin as a required + integer. No caller sees a behaviour change on either platform, because the + core already resolved an absent priority to Medium and the null therefore + carried no information. The check that refused it is compiled out of release + builds, so only development was affected. ## [0.24.0] — 2026-08-24 diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/OfflineProtocolModule.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/OfflineProtocolModule.kt index 1b5c51b3..96b5bfc3 100644 --- a/bindings/react-native/android/src/main/java/com/offlineprotocol/OfflineProtocolModule.kt +++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/OfflineProtocolModule.kt @@ -1606,17 +1606,15 @@ class OfflineProtocolModule(reactContext: ReactApplicationContext) : } @ReactMethod - fun forwardMessage(originalMessageJson: String, newRecipient: String, priority: Int?, promise: Promise) { + fun forwardMessage(originalMessageJson: String, newRecipient: String, priority: Int, promise: Promise) { try { val proto = protocol ?: throw IllegalStateException("Protocol not initialized") - val msgPriority = priority?.let { - when (it) { - 0 -> MessagePriority.LOW - 1 -> MessagePriority.MEDIUM - 2 -> MessagePriority.HIGH - 3 -> MessagePriority.CRITICAL - else -> null - } + val msgPriority = when (priority) { + 0 -> MessagePriority.LOW + 1 -> MessagePriority.MEDIUM + 2 -> MessagePriority.HIGH + 3 -> MessagePriority.CRITICAL + else -> MessagePriority.MEDIUM } val messageId = proto.forwardMessage(originalMessageJson, newRecipient, msgPriority) promise.resolve(messageId) diff --git a/bindings/react-native/ios/BRIDGE_MAINTENANCE.md b/bindings/react-native/ios/BRIDGE_MAINTENANCE.md index 9f3bf92f..d3f931de 100644 --- a/bindings/react-native/ios/BRIDGE_MAINTENANCE.md +++ b/bindings/react-native/ios/BRIDGE_MAINTENANCE.md @@ -75,17 +75,29 @@ point one. Nothing is logged either way. This row read `Int` to `nonnull NSNumber *` from v0.3.3 until this release, and seven methods followed it. -Take an `NSNumber` on the Swift side only where the argument is genuinely -optional, and know that React Native does not really support that: it forces -every `NSNumber` argument to non-null whatever you declare, because numbers are -not nullable on Android. A null one is then refused before the Swift method is -entered, so neither the resolver nor the rejecter runs and the promise never -settles. `forwardMessage` is the one method in this bridge that relies on a -nullable number, and it hangs on iOS debug builds for that reason; there is no -spelling of the declaration that fixes it, so it needs a contract change across -all three languages. That is tracked in -[#417](https://github.com/Offline-Protocol/offline-protocol-sdk/issues/417). -Until it lands, do not add a second nullable-number argument. +**A nullable number never crosses this bridge.** React Native forces every +`NSNumber` argument to non-null whatever you declare, because numbers are not +nullable on Android. A null one is then refused before the Swift method is +entered, so neither the resolver nor the rejecter runs, the promise never +settles, and the caller waits forever behind a redbox. The whole check sits +inside `#if RCT_DEBUG`, so the release build works and only development hangs. +That makes the failure confusing rather than harmless, and no spelling of the +declaration avoids it. + +An optional number is therefore not modelled as a nullable one. Resolve its +documented default in TypeScript and declare a required primitive in the shim +and on both native sides. `forwardMessage`'s priority is the precedent: TypeScript sends +`params.priority ?? MessagePriority.Medium`, the shim takes `NSInteger`, Swift +and Kotlin take `Int`, and each maps an unrecognised value back to Medium. It +costs nothing, because the core already resolves an absent priority to Medium, +so the null carried no information to begin with. That shape shipped as the fix +for [#417](https://github.com/Offline-Protocol/offline-protocol-sdk/issues/417), +where the argument was a nullable `NSNumber` until this release. + +The selector guard cannot catch a regression here. A nullable number and a +nullable object have the same ABI class on both sides, so the two halves agree +and the test passes; this rule is the only thing holding it. Take an `NSNumber` +on the Swift side only where the argument is genuinely required. **Note**: All `@objc` methods must include `resolver` and `rejecter` parameters (React Native Promise pattern). diff --git a/bindings/react-native/ios/OfflineProtocolModule.m b/bindings/react-native/ios/OfflineProtocolModule.m index 83cd6076..46dc4c3f 100644 --- a/bindings/react-native/ios/OfflineProtocolModule.m +++ b/bindings/react-native/ios/OfflineProtocolModule.m @@ -62,7 +62,7 @@ @interface RCT_EXTERN_MODULE(OfflineProtocolModule, RCTEventEmitter) RCT_EXTERN_METHOD(forwardMessage:(NSString *)originalMessageJson newRecipient:(NSString *)newRecipient - priority:(NSNumber *)priority + priority:(NSInteger)priority resolver:(RCTPromiseResolveBlock)resolve rejecter:(RCTPromiseRejectBlock)reject) diff --git a/bindings/react-native/ios/OfflineProtocolModule.swift b/bindings/react-native/ios/OfflineProtocolModule.swift index 9e20b1f5..0342abe9 100644 --- a/bindings/react-native/ios/OfflineProtocolModule.swift +++ b/bindings/react-native/ios/OfflineProtocolModule.swift @@ -1480,7 +1480,7 @@ class OfflineProtocolModule: RCTEventEmitter { /// Forwards a message to a new recipient with original sender attribution. @objc func forwardMessage(_ originalMessageJson: String, newRecipient: String, - priority: NSNumber?, + priority: Int, resolver: @escaping RCTPromiseResolveBlock, rejecter: @escaping RCTPromiseRejectBlock) { do { @@ -1489,16 +1489,15 @@ class OfflineProtocolModule: RCTEventEmitter { userInfo: [NSLocalizedDescriptionKey: "Protocol not initialized"]) } - var msgPriority: MessagePriority? = nil - if let p = priority { - switch p.intValue { - case 0: msgPriority = .low - case 1: msgPriority = .medium - case 2: msgPriority = .high - case 3: msgPriority = .critical - default: break + let msgPriority: MessagePriority = { + switch priority { + case 0: return .low + case 1: return .medium + case 2: return .high + case 3: return .critical + default: return .medium } - } + }() let messageId = try proto.forwardMessage(originalMessageJson: originalMessageJson, newRecipient: newRecipient, priority: msgPriority) resolver(messageId) diff --git a/bindings/react-native/js-ci-harness/README.md b/bindings/react-native/js-ci-harness/README.md index 8a048e3f..8755d9f6 100644 --- a/bindings/react-native/js-ci-harness/README.md +++ b/bindings/react-native/js-ci-harness/README.md @@ -69,4 +69,5 @@ Two traps, both of which produce a test that passes while proving nothing: | --- | --- | | `one-shot-hold.test.js` | The JS-layer one-shot event hold: hold, replay, and the `start()` / `enableTransport('internet')` / `destroy()` staleness transitions (`src/index.ts`, `ONE_SHOT_EVENT_TYPES`). See `docs/react-native-integration.md` §6.1. | | `local-address.test.js` | The cache of this device's derived address (`src/index.ts`, `cachedLocalAddress`): populated eagerly by `start()` and by `identity_ready`, cleared by `destroy()` so it cannot outlive the identity it names, and the session attribution that depends on knowing which half of a pair is us. | +| `forward-priority.test.js` | The priority argument `forwardMessage` hands to native (`src/index.ts`): always a number and never `null`, because React Native refuses a null number argument before the Swift method runs and the promise then never settles (#417), and `MessagePriority.Low` is 0 so the default must be resolved with `??` rather than `||`. | | `relay-config.test.js` | The relay and DORS config payloads this layer hands to native: the whole `relay` section crossing at create time (not just `relayPriority`), the legacy `low`/`medium`/`high` spelling mapping to the engine vocabulary, and a runtime update naming only the fields it was given — which is what makes the native-side merge a partial update rather than a full overwrite. Its other half is the Rust guard `react_native_bridges_merge_dors_updates_from_the_live_config`. | diff --git a/bindings/react-native/js-ci-harness/forward-priority.test.js b/bindings/react-native/js-ci-harness/forward-priority.test.js new file mode 100644 index 00000000..ad92ee82 --- /dev/null +++ b/bindings/react-native/js-ci-harness/forward-priority.test.js @@ -0,0 +1,235 @@ +#!/usr/bin/env node +/** + * Behavioral tests for the priority argument `forwardMessage` hands to the + * native module (`src/index.ts`). + * + * A nullable number cannot cross this bridge. React Native forces every + * `NSNumber` argument to non-null, because numbers are not nullable on + * Android, and refuses a null one before the Swift method is entered, so + * neither the resolver nor the rejecter runs and the promise never settles. + * This layer passed `null` for an omitted priority until #417, which hung + * `forwardMessage` forever on iOS debug builds. The repair was to resolve the + * documented default here instead, so what needs pinning is on this side of + * the bridge: the argument is always a number, and it is the right one. + * + * The Rust guard cannot cover this. A nullable number and a nullable object + * share an ABI class, so both bridge halves agree while React Native rejects + * the call anyway. + * + * See README.md for why the package has no other JS test setup. + */ +'use strict'; + +const assert = require('node:assert/strict'); +const { execFileSync } = require('node:child_process'); +const fs = require('node:fs'); +const Module = require('node:module'); +const os = require('node:os'); +const path = require('node:path'); + +const PACKAGE_DIR = path.resolve(__dirname, '..'); + +// --------------------------------------------------------------------------- +// Build +// --------------------------------------------------------------------------- + +/** Compiles `src/` to a scratch dir. See one-shot-hold.test.js for why. */ +function compileSdk() { + const tsc = path.join(PACKAGE_DIR, 'node_modules', 'typescript', 'bin', 'tsc'); + if (!fs.existsSync(tsc)) { + throw new Error(`TypeScript not found at ${tsc} — run \`npm ci\` in ${PACKAGE_DIR} first.`); + } + const outDir = fs.mkdtempSync(path.join(os.tmpdir(), 'op-rn-fwd-')); + execFileSync( + process.execPath, + [tsc, '--outDir', outDir, '--declaration', 'false', '--declarationMap', 'false'], + { cwd: PACKAGE_DIR, stdio: 'inherit' } + ); + return outDir; +} + +// --------------------------------------------------------------------------- +// The native stub +// --------------------------------------------------------------------------- + +let nativeOverrides = {}; + +/** The argument list of every `forwardMessage` call the SDK made. */ +let forwardCalls = []; + +const nativeModule = new Proxy( + {}, + { + get(_target, method) { + if (typeof method !== 'string') return undefined; + return (...args) => { + if (method === 'forwardMessage') forwardCalls.push(args); + const override = nativeOverrides[method]; + return override ? override(...args) : Promise.resolve(); + }; + }, + } +); + +class StubNativeEventEmitter { + constructor() { + this.listeners = new Map(); + } + + addListener(channel, handler) { + let handlers = this.listeners.get(channel); + if (!handlers) { + handlers = new Set(); + this.listeners.set(channel, handlers); + } + handlers.add(handler); + return { remove: () => handlers.delete(handler) }; + } +} + +const realLoad = Module._load; +Module._load = function loadWithReactNativeStub(request) { + if (request === 'react-native') { + return { + NativeModules: { OfflineProtocolModule: nativeModule }, + NativeEventEmitter: StubNativeEventEmitter, + }; + } + return realLoad.apply(this, arguments); +}; + +// --------------------------------------------------------------------------- +// Scaffolding +// --------------------------------------------------------------------------- + +const realConsole = { log: console.log, warn: console.warn, error: console.error }; + +function captureConsole() { + console.log = () => {}; + console.warn = () => {}; + console.error = () => {}; +} + +function releaseConsole() { + Object.assign(console, realConsole); +} + +const tests = []; +const test = (name, fn) => tests.push({ name, fn }); + +const RECIPIENT = 'off1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqa'; +const ORIGINAL_JSON = '{"id":"m1","content":"hello"}'; + +/** The slot the priority occupies in the native call. */ +const PRIORITY_ARG = 2; + +let OfflineProtocol; +let MessagePriority; + +function newSdk() { + return new OfflineProtocol({ appId: 'harness', profile: 'harness-profile' }); +} + +function forwardWith(params) { + return newSdk().forwardMessage({ + originalMessageJson: ORIGINAL_JSON, + newRecipient: RECIPIENT, + ...params, + }); +} + +// --------------------------------------------------------------------------- +// The argument is a number, never null +// --------------------------------------------------------------------------- + +test('an omitted priority crosses as the Medium integer, not null', async () => { + await forwardWith({}); + + const priority = forwardCalls[0][PRIORITY_ARG]; + assert.equal( + priority, + MessagePriority.Medium, + 'the core resolves an absent priority to Medium, so this layer sends it explicitly' + ); + assert.equal( + typeof priority, + 'number', + 'React Native refuses a null number argument before the Swift method runs, and the promise then never settles (#417)' + ); +}); + +test('every priority in the enum crosses as a number', async () => { + for (const name of ['Low', 'Medium', 'High', 'Critical']) { + forwardCalls = []; + await forwardWith({ priority: MessagePriority[name] }); + + const priority = forwardCalls[0][PRIORITY_ARG]; + assert.equal(priority, MessagePriority[name], `${name} must cross unchanged`); + assert.equal(typeof priority, 'number', `${name} must cross as a number`); + } +}); + +// --------------------------------------------------------------------------- +// Zero is a priority, not an absence +// --------------------------------------------------------------------------- + +test('Low survives the default rather than being read as absent', async () => { + await forwardWith({ priority: MessagePriority.Low }); + + // `MessagePriority.Low` is 0. Resolving the default with `||` instead of + // `??` silently upgrades every Low forward to Medium, which no caller can + // see: the message sends either way, just at the wrong priority. + assert.equal( + forwardCalls[0][PRIORITY_ARG], + MessagePriority.Low, + 'Low is 0 and must not be treated as an unset priority' + ); +}); + +// --------------------------------------------------------------------------- +// The rest of the call is unchanged +// --------------------------------------------------------------------------- + +test('the message and recipient reach native alongside the priority', async () => { + nativeOverrides.forwardMessage = () => Promise.resolve('new-id'); + + const messageId = await forwardWith({ priority: MessagePriority.High }); + + assert.deepEqual(forwardCalls[0], [ORIGINAL_JSON, RECIPIENT, MessagePriority.High]); + assert.equal(messageId, 'new-id', 'the native message id is returned to the caller'); +}); + +// --------------------------------------------------------------------------- +// Runner +// --------------------------------------------------------------------------- + +(async () => { + const outDir = compileSdk(); + try { + ({ OfflineProtocol, MessagePriority } = require(path.join(outDir, 'index.js'))); + + let failed = 0; + for (const { name, fn } of tests) { + nativeOverrides = { isMlsInitialized: () => Promise.resolve(true) }; + forwardCalls = []; + captureConsole(); + try { + await fn(); + releaseConsole(); + realConsole.log(` ✓ ${name}`); + } catch (error) { + failed += 1; + releaseConsole(); + realConsole.log(` ✗ ${name}\n ${error.message}`); + } + } + + realConsole.log( + failed === 0 ? `\n${tests.length} passed.` : `\n${failed} of ${tests.length} FAILED.` + ); + process.exitCode = failed === 0 ? 0 : 1; + } finally { + releaseConsole(); + fs.rmSync(outDir, { recursive: true, force: true }); + } +})(); diff --git a/bindings/react-native/package.json b/bindings/react-native/package.json index dd0d8fea..f97a2a41 100644 --- a/bindings/react-native/package.json +++ b/bindings/react-native/package.json @@ -28,7 +28,7 @@ "scripts": { "prepare": "tsc", "build": "tsc", - "test:js": "node js-ci-harness/one-shot-hold.test.js && node js-ci-harness/local-address.test.js && node js-ci-harness/relay-config.test.js && node js-ci-harness/data-config.test.js && node js-ci-harness/security-config.test.js", + "test:js": "node js-ci-harness/one-shot-hold.test.js && node js-ci-harness/local-address.test.js && node js-ci-harness/forward-priority.test.js && node js-ci-harness/relay-config.test.js && node js-ci-harness/data-config.test.js && node js-ci-harness/security-config.test.js", "build:ios": "bash scripts/build-ios.sh", "build:android": "bash scripts/build-android.sh", "build:all": "bash scripts/build-all.sh", diff --git a/bindings/react-native/src/index.ts b/bindings/react-native/src/index.ts index 88a28ffa..ebee44d3 100644 --- a/bindings/react-native/src/index.ts +++ b/bindings/react-native/src/index.ts @@ -1265,7 +1265,7 @@ export class OfflineProtocol { * @throws Error if forwarding fails */ async forwardMessage(params: ForwardMessageParams): Promise { - const priority = params.priority ?? null; + const priority = params.priority ?? MessagePriority.Medium; const messageId = await OfflineProtocolNativeModule.forwardMessage( params.originalMessageJson, params.newRecipient, diff --git a/docs/bridges/swift.md b/docs/bridges/swift.md index d796d9a8..438b77ab 100644 --- a/docs/bridges/swift.md +++ b/docs/bridges/swift.md @@ -96,11 +96,19 @@ own scan finds nothing to check. `react_native_ios_bridge_bounds_every_byte_it_builds_from_javascript` fails on any byte conversion whose argument does not carry its own bound. -One gap is known and unfixable here: React Native forces every `NSNumber` -argument to non-null, so `forwardMessage`'s optional priority is refused before -the Swift method runs and its promise never settles in a debug build. It needs -a contract change across all three languages, tracked in -[#417](https://github.com/Offline-Protocol/offline-protocol-sdk/issues/417). +**A nullable number is not something this bridge can express**, and the guard +above cannot say so. React Native forces every `NSNumber` argument to non-null +whatever the declaration says, because numbers are not nullable on Android, and +it refuses a null one before entering the Swift method, so neither the resolver +nor the rejecter runs and the promise never settles. The check lives inside +`#if RCT_DEBUG`, so the release build passes the null through and only +developers meet the hang. `forwardMessage`'s priority was declared that way +until this release. The repair is not a spelling of the declaration but the +removal of the nullability: TypeScript resolves the documented default, and the +shim, Swift and Kotlin all take a required integer. That costs nothing, because +the core already turns an absent priority into Medium. The guard misses this class +because a nullable number and a nullable object share an ABI class, so both +halves agree while React Native rejects the call anyway. ## S2. Five registration points per new Swift file