feat: silent refresh when receiving notification#536
Conversation
n13
left a comment
There was a problem hiding this comment.
Review: silent refresh when receiving notification
Nicely scoped change — it hooks the silent refresh into FirebaseMessaging.onMessage (the foreground path), which is the correct gap to close since background/terminated resume is already handled by AppLifecycleManager. It also reuses the existing HistoryPollingManager.triggerSilentRefresh() rather than duplicating refresh logic, so it's DRY and minimal. One error-handling concern worth addressing per the project's fail-early rule.
Findings
-
Major — silent refresh failures are never logged. The call is fire-and-forget via
unawaited(...), andtriggerSilentRefresh()itself has no try/catch. IfsilentRefreshActiveAccountor the multisigFuture.waitthrows (e.g. a network blip while the notification arrives), the error is dropped to the zone's uncaught handler with no contextual log — a "silent refresh" that fails silently. The project rule is that all failures must produce error logging. Best fix (also DRY, sinceAppLifecycleManagercalls the same method on lines 61 & 97 with no handling either) is to wrap the body oftriggerSilentRefresh()in try/catch and log viaquantusDebugPrint, so every caller benefits. Alternatively, attach.catchError(...)at this call site. -
Minor — no debounce / concurrency guard. Each foreground FCM message triggers a full refresh (balance invalidate + paginated history network fetch + multisig proposals). A burst of notifications fires several overlapping refreshes. Low practical impact for a wallet, but a guard would avoid redundant network calls.
-
Nit — refresh fires for every foreground message, including irrelevant ones. The refresh runs before
_remoteMessageToNotificationData(...)returns/null-checks, so even a message that doesn't deserialize to a known tx event triggers a refresh. Probably fine (any FCM likely signals new on-chain activity), but moving it after the relevance check would avoid spurious refreshes. -
Nit — the added 5-line doc comment is a touch verbose for the project's minimal-comment style, though it does capture useful intent (why foreground-only). Optional trim.
Positives
- Correct callback (foreground
onMessage), no double-refresh with the lifecycle resume path. - Reuses existing
triggerSilentRefresh()— no duplicated logic. - Tiny, focused diff.
Verdict: COMMENT — non-blocking; please add error logging to the silent-refresh path before merge.
Automated review via Cursor agent.
|
Can we add some sort of debounce, ie, if we're in the middle of refreshing, don't do it again - this might already be in the provider... otherwise, great, minimal! |
Summary
Silent refresh!!!
Note
Low Risk
Single fire-and-forget call on foreground FCM with existing refresh logic; no auth or push registration changes.
Overview
When the app is open and receives an FCM message, it now kicks off the same silent refresh path used on resume (
HistoryPollingManager.triggerSilentRefresh()), so balance, activity history, and multisig proposals update without loading spinners before the local notification is shown.Background/terminated delivery is unchanged—resume still refreshes via the app lifecycle manager; this PR only closes the foreground gap where lifecycle resume does not run.
Reviewed by Cursor Bugbot for commit cd688b4. Configure here.