ReviewOS

also looking at this

craft-native/craft

iOS Phase 6: notifications, background tasks, Watch — ratchet 70 → 64

#88
Merged glennmichael123 wants to merge feat/ios-phase6-notif into main
10 files +3613 -12
10 days ago

Follow-on to #87. Six actions cross, and the review pass caught a fabricated success in my own code.

What landed

getPendingNotifications · scheduleBackgroundTask · cancelBackgroundTask · cancelAllBackgroundTasks · updateWatchContext · isWatchReachable

Three assigned actions were deliberately not served. scheduleNotification, registerBackgroundTask and sendToWatch each hand iOS a callback that fires later — at relaunch, or on a reply handler — which needs an event channel iOS doesn't have yet. They fall through to the Swift shim that serves them correctly today, rather than being declared .unavailable, which would make Zig dispatch and refuse them.

Coverage counted by actions-in-a-table would have looked better. The app would have looked worse.

The bug in my own foundation

ios_async.deliverJson — written an hour before, in the file built specifically to stop this — fabricated success on OOM:

slots[i].json = allocator.dupe(u8, json) catch null;   // failure indistinguishable from "BOOL completion"
...
json orelse if (granted) "\"granted\"" else "\"denied\""  // granted defaults false

A caller awaiting an array got the string "denied", resolved — so its catch never ran. A failed flag now separates "no json because this was a BOOL completion" from "no json because shaping it failed", and the latter sends an error. deliverError is exported for modules whose own completion can't shape an answer, which also makes that path testable: the reviewer had to delete a correct test because the only alternative was an unreplyable log.err.

Two claims that outran their guards

ios_dispatch's route comment asserted "the conformance test guarantees no action appears in two modules' tables." It did not — collectZigActions folded every module into one set, so a duplicate was invisible and dispatch order (a comptime tuple's order) would silently pick a winner. The check exists now, verified by injecting a duplicate:

action 'getPendingNotifications' is declared by more than one mobile module.
  Dispatch order would silently pick one.

requestNotifications reached currentNotificationCenter with no bundle-identifier guard, its comment arguing the exposure was unreachable from a page and matched Swift's. Both true, and neither a reason to inherit an uncatchable NSInternalInconsistencyException that takes the process down with the page's promise still pending. Guarded now, the same way notifcancel does.

Verification

Twelve fixture assertions on an iPhone 17 Pro simulator. The new one proves the async escape hatch end to end — an NSArray completion shaped by the module, delivered through ios_async's main-queue hop with the request id captured back at dispatch:

ok: async NSArray completion delivered via ios_async (i=14 seen 1x)
PASS

Plus zig build test / test:ios / build-ios / build-ios-simulator / build-android, zig fmt --check, verify:cimports, pickier.

10 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.