ReviewOS

also looking at this

craft-native/craft

fix(sdk): schedule a notification for its scheduleAt, not immediately

#263
Merged glennmichael123 wants to merge fix/ios-notification-schedule-at into main
3 files +73 -3
9 days ago

Closes #261.

The SDK documents scheduleAt for scheduling:

await notifications.schedule({ title: 'Reminder', body: 'Time to take a break', scheduleAt: Date.now() + 3600000 })

notifications.schedule handed its options to the bridge unchanged, but neither bridge reads scheduleAt. iOS schedules from timestamp or delay, and Android from delay only. So the reminder fired immediately on both platforms.

Change

bridgeNotification(options, now) in api/mobile.ts translates the SDK's option into the bridges' field, and notifications.schedule calls it:

  • A future scheduleAt becomes delay = ceil(scheduleAt - now), and scheduleAt isn't sent.
  • A moment already here or past sends no delay, which both bridges deliver now. A zero delay would raise inside UserNotifications on the Swift shim, which the Zig module's header documents as an uncatchable exception.
  • A sub-millisecond wait rounds up to 1, because Android reads whole milliseconds and would otherwise treat it as "now".

Choices

  • The SDK, not the bridges. scheduleAt is an SDK name: the bridge's own typed API (NotificationData in craft.d.ts) has only timestamp and delay. Translating in the one layer that owns the name leaves the Swift, Zig and Kotlin readers alone, and fixes Android as well.
  • delay rather than iOS's timestamp. Android reads only delay. An interval also matches what an epoch instant means. The iOS timestamp path uses a calendar trigger built from local date components, which would move with the device's time zone.
  • The delay path itself is covered end to end on both iOS schedulers by the local-notification stage from #262, which schedules with delay: 8000, kills the app, and taps the banner once it fires.

Also: craft.d.ts's NotificationData now types data, which #262 made work.

Tests

mobile.test.ts has 4 new cases (566 SDK tests in all). Each of these mutations fails one of them:

MutationFails
schedule passes options through unchanged (the bug)"schedules through the bridge with the delay, not scheduleAt"
a zero delay is allowed"delivers now for a moment already here or past…"
no rounding upthe same test's sub-millisecond case

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

Sign in to comment on this pull request.