Skip to content

Properly handle fcm error#556

Merged
dewabisma merged 1 commit into
mainfrom
beast/proper-handle-fcm-error
Jul 3, 2026
Merged

Properly handle fcm error#556
dewabisma merged 1 commit into
mainfrom
beast/proper-handle-fcm-error

Conversation

@dewabisma

@dewabisma dewabisma commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Small fix for handling fcm error instead of throwing unhandled.


Note

Low Risk
Defensive error handling around push setup with no change to happy-path behavior; reduces risk of stuck notification-enable state on failure.

Overview
FCM failures no longer surface as unhandled async errors. Remote notification enablement (lazy Firebase init, init(), tap handlers) is wrapped in try/catch/finally so failures are logged and sent to telemetry as fcm_enable_remote_notifications_failed, and the _isEnablingRemoteNotifications guard is always cleared in finally (previously it could stay stuck on error).

Device token fetch in getDeviceToken() is similarly guarded: errors are logged, reported as fcm_get_device_token_failed, and the method returns null instead of throwing.

Reviewed by Cursor Bugbot for commit 2420cb4. Configure here.

@dewabisma dewabisma requested a review from n13 July 3, 2026 03:02

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Reviewed both changed files in full plus all callers of getDeviceToken() and the enablement flow. This PR fixes a real bug and both new catch blocks satisfy the repo's "no silent failures" convention (debug log + telemetry report).

Findings

Real bug fixed — stuck guard prevented retry. Previously, any throw in _enableRemoteNotificationsIfNeeded (mobile-app/lib/providers/remote_config_provider.dart) left _isEnablingRemoteNotifications = true permanently, blocking all future enable attempts, and the exception escaped through unawaited(...) as an unhandled async error. The finally now clears the guard, so the next remote-config change can retry, and the failure is logged and reported as fcm_enable_remote_notifications_failed.

null return from getDeviceToken() is handled by all callers. All four call sites (init() via registerDeviceIfPossible(), unregisterDevice(), insertNewAddress()) already null-check and log the skip. Because _cachedToken stays null on failure, every subsequent call re-attempts the fetch, and the onTokenRefresh listener registered in init() provides eventual recovery + device registration once FCM produces a token. No silent failure downstream — the fetch failure itself is telemetry-reported at the source.

Minor, non-blocking: since getDeviceToken() no longer throws, init() now completes and sets _isInitialized = true even when the initial token fetch fails, so device registration is deferred to onTokenRefresh or a later registerDeviceIfPossible() call rather than retried via init(). Acceptable for a best-effort push path, and strictly better than the previous behavior (enablement aborted with the guard stuck forever).

Broad catch is acceptable here. catch (e, st) also catches programming errors, but both catches log and telemetry-report so bugs stay visible, and this matches the existing pattern in registerForRemoteNotificationsBestEffort. Around Firebase platform-channel calls a broad catch is the pragmatic choice. Note quantusDebugPrint is debug-only, so telemetry is the release-mode error channel — both new catches send it.

Verdict: Approve

@dewabisma dewabisma merged commit f4648bf into main Jul 3, 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.

2 participants