Conversation
isFirebaseInitialised records whether the Firebase SDK was initialised in the current page context. The SDK does not survive a reload, but a persisted flag does, so after any reload the flag rehydrated as true against a freshly empty SDK. Every caller then took the early return in initialiseFirebaseMessaging and the app ended up with no onMessage handler, no background message listener and no registration token, while still reporting itself as initialised. This is not a rare edge case. VitePWA is configured with registerType "autoUpdate", so a store device reloads without being asked and reaches this state on its own. It also silently defeats any token refresh work, because the refresh path is behind the same guard. The flag is session state rather than user data, so it is excluded from persistence. The plugin applies omit on hydration as well as on write, so devices that already hold the flag from an earlier build have it ignored on their next load and need no storage reset. Verified in a browser against the real store: persistence still writes firebaseDeviceId, allNotificationPrefs, notificationPrefs, notifications and hasUnreadNotifications, and no longer writes isFirebaseInitialised, while the flag continues to work in memory within a session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
The bug
isFirebaseInitialisedrecords whether the Firebase SDK was initialised in the current page context. The SDK does not survive a reload. A persisted flag does.So after any reload the flag rehydrates as
trueagainst a freshly empty SDK,initialiseFirebaseMessagingtakes its early return, and the app ends up with:onMessagehandler, so no foreground notificationsBroadcastChannellistener, so background notifications never reach the in-app bellwhile still reporting itself as initialised. The service worker keeps drawing OS banners independently, so from the outside it looks like "notifications sort of work but the bell is always empty", which is exactly what was reported from the field.
This is not a rare edge case.
VitePWA({ registerType: "autoUpdate" })reloads the app without asking, so a store tablet reaches this state on its own. It also silently defeats token-refresh work, because that path sits behind the same guard.The fix
One field excluded from persistence. It is session state, not user data.
omitis applied on hydration as well as on write (hydrateStore, before$patch), so devices that already hold the flag from an earlier build have it ignored on their next load. No storage reset or app reinstall is needed to recover them.Verified, not assumed
Exercised in a real browser against the real store definition, not a mock:
isFirebaseInitialisedwrittenfirebaseDeviceIdsurvivesallNotificationPrefssurvivesI tried to add a unit test first and could not make one that proves anything here. The persistence plugin does not run under this workspace's vitest setup at all: jsdom provides no
Storage, and with an in-memory stand-in neitherpersist: truenorpersist: { omit }writes anything. Rather than land a test that passes without exercising the behaviour, I verified in a browser and am saying so plainly. If someone knows how persistence is meant to be tested here, a regression test would be worth adding.Blast radius
commonis shared, so this affects every app that uses the notification store (BOPIS, fulfillment, receiving). The change is safe for all of them: any app relying on this flag surviving a reload was already broken, because the SDK it refers to does not survive either.Note for deployment: app repos resolve
@commonby path into anaccxuicheckout, so a BOPIS build only picks this up once its build uses anaccxuicontaining it.🤖 Generated with Claude Code