Skip to content

Fixed: register the device when subscribing from the notification preference modal (targets v5.3.0-ls) - #872

Closed
dt2patel wants to merge 2 commits into
v5.3.0-lsfrom
fix/notification-device-registration-ls
Closed

dt2patel wants to merge 2 commits into
v5.3.0-lsfrom
fix/notification-device-registration-ls

Conversation

@dt2patel

Copy link
Copy Markdown
Contributor

Same code as #870, targeted at v5.3.0-ls for Lovers UAT retesting. The rationale is corrected relative to #870, which I have also commented on.

What is actually broken

subscribe#Topic in hotwax-moqui-firebase only subscribes registration tokens that already exist for the user:

<entity-find entity-name="co.hotwax.firebase.UserLoginFirebaseClient" list="userLoginClientTokens">
    <econdition field-name="userLoginId" from="ec.user.getUsername()"/>
    <econdition field-name="applicationId" from="applicationId"/>
</entity-find>

NotificationPreferenceModal, reached from the bell icon, subscribes topics and never registers the device at all. A user with no token who sets their preferences there creates topic subscriptions with nothing behind them. The backend then shows a healthy-looking subscription for a user who can never receive anything from that path.

initialiseFirebaseMessaging also refused to run for exactly these users, because it required getAllNotificationPrefs to be non-empty, which is only true once a subscription exists.

Change

  • Register the device before subscribing, in both Settings.vue and NotificationPreferenceModal.vue.
  • Add a force parameter to initialiseFirebaseMessaging. The default is unchanged so users who never asked for notifications are not prompted on login, but a caller about to subscribe overrides it. Existing no-argument callers in App.vue and store/user.ts are unaffected.
  • Stop an unset or malformed VITE_FIREBASE_CONFIG throwing out of JSON.parse, which previously rejected the whole post-login flow instead of degrading to notifications being unavailable.

Correction to what I originally claimed

I first said the Settings ordering itself was the bug. It is not. store#ClientRegistrationToken back-fills, subscribing a newly created token to every topic the user already holds, so the old order worked there. The reordering in Settings.vue is neutral and kept only for consistency with the modal. The modal fix is the substantive one.

The larger defect is in the backend, not this PR

store#ClientRegistrationToken looks its row up on (userLoginId, deviceId, applicationId) and ignores registrationToken. When FCM rotates a token for a device that already has a row, the service returns early, never updates the stored token and never subscribes the new one. The database keeps a dead token, the live token belongs to no topic, and the device goes silent permanently with no error raised. That fits "worked for a few orders then stopped" far better than anything in this PR, and it cannot be fixed from the frontend.

🤖 Generated with Claude Code

A user turning on their first notification preference never receives
anything, even though the app and the backend both look correct
afterwards.

The toggle handler subscribed the topic first and only then called
initialiseFirebaseMessaging, so at the moment of subscribing the user
had no registration token at all. The token was created immediately
afterwards and never joined the topic. The result is a user with a
stored device token and a stored topic subscription, both of which look
healthy in the backend, receiving nothing.

initialiseFirebaseMessaging also refused to run for exactly these users.
It required getAllNotificationPrefs to be non-empty, which is only true
once a subscription exists, so a user's very first subscription could
never register the device. Registration is now skipped by default, as
before, so users who never asked for notifications are not prompted on
login, but a caller that is about to subscribe passes force to override
it.

NotificationPreferenceModal never registered the device at all. It
subscribed topics and stopped, so configuring notifications from that
screen produced subscriptions with no token behind them.

Also stopped an unset or malformed VITE_FIREBASE_CONFIG throwing out of
JSON.parse. That rejection propagated out of the post-login flow rather
than degrading to notifications being unavailable.

Note for review: the claim that the backend attaches a topic only to the
tokens it already knows about is inferred from the client ordering and
from observed devices that hold both a token and a subscription while
receiving nothing. It has not been confirmed against the firebase/topic
service. Worth verifying there before this is considered closed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-16T04:07:42.309722Z 4881401 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@dt2patel

Copy link
Copy Markdown
Contributor Author

Superseded by the PR above. This branch was cut from a v5.3.0-ls that is now 28 commits stale, so it would have reverted unrelated work. More importantly @ymaheshwari1's #859 approach has since landed on this branch, which removes the preference gate from initialiseFirebaseMessaging more cleanly than the force parameter here, and my claim that the subscribe-then-register ordering was the root cause was wrong: store#ClientRegistrationToken back-fills. The replacement keeps only what is genuinely still missing, the preference modal never registering the device, and the unguarded config parse.

@dt2patel dt2patel closed this Sep 17, 2026
@dt2patel
dt2patel deleted the fix/notification-device-registration-ls branch September 17, 2026 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants