Repository navigation
Wait for FCM token before replying to push enable, add ORNotificationHandler - #40
Merged
Merged
Conversation
| return Int64(notificationIdString) | ||
| } | ||
|
|
||
| // TODO: geofence provider should also be adapted to store "per project" |
MartinaeyNL
reviewed
Oct 13, 2026
MartinaeyNL
left a comment
Member
There was a problem hiding this comment.
Initial AI assisted Review
Verdict: the approach is right — waiting in enable() is the only hook ORLib owns (console.ts
drops late replies and has no token-update channel), and ORNotificationHandler as a forwarding
helper matches #21.
Before merge
hasPermission: granted ?? true(PushNotificationProvider.swift:122) is an optimistic
guess when the token beats the user's tap. Registering for remote notifications before
requestAuthorizationmakes this more likely. Wait for both signals within the budget (or
document the trade-off in "Behaviour notes").- README prerequisites are incomplete — the fix only works if the app also forwards the APNs
token (Messaging.messaging().apnsToken = deviceToken) and configures Firebase on every launch
path (console-ios-app#46 does both). Same gap in theORNotificationHandlerdoc comment. Fixes #39is partial — first-launch race is fixed (≤4.5 s); late (>4.5 s) and rotated
tokens still only reach the backend on the next page load. Keep #39 open or file a follow-up for
theconsole.tschange.
Behaviour changes / side effects
PROVIDER_ENABLEnow takes up to 4.5 s when no cached token (every first enable per launch,
and any launch where the cache is empty, e.g. changed app group).console.tsawaits providers
sequentially, so this also delays the geofence enable, the registration POST and the app's
enableCompletecallback.- Remote registration now happens even when permission is denied → backend can hold tokens for
devices that can't show notifications (delivery/ack stats get holes). Upside: enabling later in
iOS Settings works without re-registering. - No early exit when registration fails — nothing observes
didFailToRegisterForRemoteNotifications; a device without APNs/Firebase (incl. simulator)
always burns the full 4.5 s then repliestoken: null. Consider forwarding that failure to the
handler. - Pending callback retains
ORViewcontrollerup to 4.5 s (ORViewControllerwires
self.sendDatastrongly) — was ~instant before;[weak self]would fix. console.ts"Message already pending" collision window grew from ms to 4.5 s — manual
enableProvider("push")during auto-enable now throws easily.- API break for subclasses:
init()becameconvenience init()and the designated init is
internal— external subclasses ofPushNotificationProviderare no longer writable. Release
notes should mention it.
Smaller items
- The 4.5 s ↔
console.ts5000 ms coupling is an undocumented cross-repo constant — comment both
ends, consider making it tunable. didReceiveRegistrationToken(nil)doesn't clear the stored token (deleteTokencase) — stale
token is re-sent.activeORViewController()only walkspresentedViewController(no child containment → nav/tab
hosts silently no-op) and isinternal; apps still usingwindow?.topController(e.g. geofence
deep links) can't reuse it. Make itpublic, consider a containment walk.enable()itself is untested (UNUserNotificationCenternot injectable) — the new
permission/hasPermissionbranches are only covered viawaitForToken. Small injection seam
would fix.
Miggets7
force-pushed
the
fix/push-token-late-upload
branch
2 times, most recently
from
October 13, 2026 12:01
2f6f09c to
fc543c5
Compare
…ionHandler On a fresh install the push provider replied to PROVIDER_ENABLE with the cached FCM token, which Firebase had not delivered yet. The console registration then had no token and push notifications only worked after an app restart (#39). Like the Android console, the provider now registers for remote notifications straight away and waits for the token before replying. The permission prompt is shown in parallel, so its duration does not count against the web console's 5 second provider timeout. The wait is capped at 4.5 seconds. A token that arrives later, or a rotated token, is sent the next time the console enables push, which happens on every page load. ORNotificationHandler moves the notification and FCM token handling that every console app duplicated into ORLib (#21). Apps forward their UNUserNotificationCenterDelegate and MessagingDelegate callbacks to it; it stores the token and notifies a waiting push provider. It finds the console view controller through the connected scenes, which also makes "open in app" work for scene based apps.
Miggets7
force-pushed
the
fix/push-token-late-upload
branch
from
October 13, 2026 12:12
fc543c5 to
de6084d
Compare
MartinaeyNL
approved these changes
Oct 14, 2026
Member
|
Did a thorough review to understand what was going on here, because I'd like to prevent any unwanted bugs. |
Merged
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.
Fixes #21. Refs #39 (first-launch race only, see Behaviour notes). Root cause of openremote/ourgrid#227.
Problem
On a fresh iOS install the push provider answered
PROVIDER_ENABLEwith the FCM token cached inUserDefaults. Firebase had not delivered one yet, so the console registered without a token and push only worked after an app restart.Changes
PushNotificationProvider.enable()now does what the Android console does: it registers for remote notifications right away and waits for the token before replying. The permission prompt runs in parallel, so a slow tap on the prompt no longer counts against the web console's 5 s provider timeout. The wait is capped at 4.5 s.ORNotificationHandlertakes over the notification and Firebase token handling every app duplicated in itsAppDelegate. Apps forward three callbacks to it (snippet in the README). It stores the token and wakes a waiting push provider.FirebaseApp.configure()and the messaging delegate on every launch path, and forwardingapnsTokenwhen swizzling is off.Behaviour notes
console.ts.console.tsenables providers one after another, so the geofence enable and the registration POST wait as well. Android waits ongetToken()with no cap, so a reply after 5 s is dropped byconsole.tsand no registration is sent. The 4.5 s cap avoids that on iOS.hasPermission: true. Android always reportstrue, and the backend does not read the field..deniedbranch still skips remote registration.ORViewcontrollerholds the enable callback weakly, so the wait doesn't keep it alive.Testing
PushNotificationProviderTestandORNotificationHandlerTest. Full suite passes on the simulator (118 tests).Follow-up
A release is needed after merge. openremote/console-ios-app and openremote/ourgrid PRs bump to it.
#39 stays open: tokens that arrive after 4.5 s, and rotated tokens, only reach the backend on the next page load. Closing that needs
console.tsto accept a token update from the provider.