Skip to content

Fixed: notifications issue as the token registration is gated in preferences condition causing the device to not getting registered and subscription fails - #859

Merged
ymaheshwari1 merged 1 commit into
hotwax:mainfrom
ymaheshwari1:fix/notifications
Sep 22, 2026

Conversation

@ymaheshwari1

Copy link
Copy Markdown
Contributor

Related Issues

Short Description and Why It's Useful

Removed the preferences check from the notification util and moved it to the login flow, so that if notifications preferences are not found on login do not register the device, but if someone is subscribing from the settings page, the device registration should be done.

Screenshots of Visual Changes before/after (If There Are Any)

Contribution and Currently Important Rules Acceptance

…erences condition causing the device to not getting registered and subscription fails

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

dt2patel added a commit that referenced this pull request Sep 17, 2026
…#878)

* Fixed: register the device when subscribing from the preference modal

subscribe#Topic only attaches registration tokens the backend already
knows about. The notification preference modal, reached from the bell
icon, subscribes topics and never registers the device, so a user who
sets their preferences there creates subscriptions with no token behind
them and receives nothing until their next login, when the login path
finally registers and back-fills. That is the shape of the "log out and
log back in" folklore around these notifications.

The settings screen already registers, and #859's move of the preference
gate out of initialiseFirebaseMessaging is already on this branch, so
this is the one remaining path that can subscribe without registering.

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

Deliberately NOT changed here, both worth a decision of their own:

- The subscribe-then-register ordering in Settings is left alone.
  store#ClientRegistrationToken back-fills a new token into topics the
  user already holds, so that order does work. I previously claimed it
  was the bug and was wrong.
- The isFirebaseInitialised early return in firebaseUtil is currently
  commented out on this branch. Leaving it commented re-runs
  initialiseFirebaseApp on every call and re-attaches onMessage, which
  duplicates in-app notifications. It should be restored once
  accxui#165 stops that flag being persisted, since persistence is
  presumably why it was disabled.

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

* Fixed: do not reinitialise messaging for an already-registered device

The preference modal called initialiseFirebaseMessaging on every save.
Its initialisation guard is commented out in firebaseUtil on this
branch, so each call attached another foreground onMessage listener and
an already-registered user accumulated one per save, with later
notifications landing in the store several times over.

Registration now runs only when this device has no token yet.
getFirebaseDeviceId is set once a token has been stored, so it answers
"has this device registered" without re-entering initialisation, and it
does not depend on the persisted isFirebaseInitialised flag, which
rehydrates as true in a context that has no Firebase app.

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

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@ymaheshwari1
ymaheshwari1 merged commit 610e110 into hotwax:main Sep 22, 2026
2 checks passed
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.

1 participant