Add remote notifications

[dewabisma]

Summary

I have finished adding FCM and permission needed in iOS. I have tested that it works in iOS but I haven't tested for android. To test it, we need real device. Later we will just need to handle the kind of message and behavior we want, all the ground works are done.

Changes

• Add necessary permission to iOS
• Add firebase configurations
• Add FCM service
• Add senoti service to register device

[n13]

Here's my review of PR #382: Add remote notifications.

---

Overall

Solid groundwork for FCM integration. The iOS/Android setup, permission handling, token lifecycle, and foreground/background message handling all look correct. A few issues to address though, primarily around DRY and some leftover debug code.

---

DRY Violations

1. Notification tap navigation logic -- duplicated 3 times

The pattern of "deserialize tx event -> set intent provider -> navigate to /transactions" appears in three places:

local_notifications_service.dart lines 80-87 (handleLaunchByNotification):

    final json = jsonDecode(payload);
    final txService = _ref.read(transactionServiceProvider);
    final event = txService.deserializeTxEventFromJsonIfPossible(json);

    if (event != null) {
      _ref.read(transactionIntentProvider.notifier).state = event;
      navigatorKey.currentState?.pushNamed('/transactions');
    }

local_notifications_service.dart lines 137-144 (setupNotificationsClickListener):

      final json = jsonDecode(payload);
      final txService = _ref.read(transactionServiceProvider);
      final event = txService.deserializeTxEventFromJsonIfPossible(json);

      if (event != null) {
        _ref.read(transactionIntentProvider.notifier).state = event;
        navigatorKey.currentState?.pushNamed('/transactions');
      }

And now the new FirebaseMessagingService._handleNotificationTap adds a third copy (from the diff):

void _handleNotificationTap(RemoteMessage message, GlobalKey<NavigatorState> navigatorKey) {
    final data = message.data;
    if (data.isEmpty) return;
    final txService = _ref.read(transactionServiceProvider);
    final event = txService.deserializeTxEventFromJsonIfPossible(data);
    if (event != null) {
      _ref.read(transactionIntentProvider.notifier).state = event;
      navigatorKey.currentState?.pushNamed('/transactions');
    }
  }

Suggestion: Extract a shared helper method (e.g., on TransactionService or a standalone function) like navigateToTransactionFromPayload(Ref ref, dynamic data, GlobalKey<NavigatorState> navigatorKey) and call it from all three places.

---

2. SenotiService.getMainAccount() is a copy of TaskmasterService.getMainAccount()

New code in senoti_service.dart:

Future<Account> getMainAccount() async {
    final account = await _settingsService.getAccount(walletIndex: 0, index: 0);
    if (account == null) {
      throw Exception('No main account - ...');
    }
    return account;
  }

This is identical to:

  Future<Account> getMainAccount() async {
    final account = await _settingsService.getAccount(walletIndex: 0, index: 0);
    if (account == null) {
      throw Exception('No main account - this method should probably not be called when logged out');
    }
    return account;
  }

Suggestion: Either reuse TaskmasterService.getMainAccount(), or extract this to SettingsService as a shared utility since both services already depend on it. Also note: getMainAccount() doesn't appear to be called anywhere in this PR -- it may be dead code.

---

Other Issues

3. Debug print() statements left in senoti_service.dart

[deleted - not an issue]

4. Senoti endpoint is hardcoded to localhost

static const String senotiEndpoint = 'http://localhost:3100/api';

Every other endpoint in app_constants.dart points to a real URL (e.g., https://quests.quantus.com/api). This will not work on a real device. Needs a production URL before merge, or at minimum a TODO/env-based toggle.

5. Firebase API keys committed to source control

GoogleService-Info.plist and firebase_options.dart contain API keys, app IDs, etc. These aren't in .gitignore. While Firebase API keys are considered "public-facing" (restricted by bundle ID), it's still worth confirming this is intentional. Many teams exclude these and inject them via CI.

6. Stale comment in notification_provider.dart

The addRemoteNotification method still says:

/// Stub for remote notifications (to be implemented later)
void addRemoteNotification(NotificationData notification) {
    // This is a placeholder for future Firebase/APNs integration

This PR is the Firebase integration -- the comment should be updated.

7. NotificationSource.remote case in addNotification inconsistency

In notification_provider.dart line 182-184, the remote case still has no handling:

case NotificationSource.remote:
    // Remote notifications arriving via FCM foreground listener are
    // shown as local push notifications by FirebaseMessagingService.
    // No additional handling needed here.
    break;

But FirebaseMessagingService calls both notifier.addRemoteNotification() AND localNotificationService.showOrScheduleNotification() separately. This means the "show local notification" logic for remote messages lives in FirebaseMessagingService, while for push source it lives inside addNotification. The split is inconsistent -- consider moving showOrScheduleNotification into the remote case of addNotification instead, matching how push works.

8. Info.plist whitespace churn

The Info.plist diff shows every line removed and re-added with only indentation changes (tabs vs spaces). The actual content change is just two additions (UIBackgroundModes and FirebaseAppDelegateProxyEnabled). This makes the diff noisy and hard to review.

---

Summary

Category	Items
DRY violations	Notification tap handler duplicated 3x; getMainAccount() duplicated
Debug leftovers	Raw print() in senoti_service.dart
Config issue	Senoti endpoint is localhost
Security	Firebase keys in source control (verify intentional)
Stale comments	"placeholder for future" is no longer future
Consistency	Remote vs push notification flow split across different layers

The core FCM integration is well-structured. Addressing the DRY items and cleaning up the debug artifacts would make this ready to merge.

[n13]

Here's the updated review of PR #382 after the changes.

---

What was addressed from the first review

Nice work -- several key items were fixed:

1. DRY: Notification tap navigation extracted -- navigateToTransactionFromPayload was added to TransactionService and is now called from LocalNotificationsService.handleLaunchByNotification, setupNotificationsClickListener, and FirebaseMessagingService._handleNotificationTap. The triplicated logic is now in one place.

2. NotificationSource.remote consistency fixed -- push and remote cases now share the same code path via fall-through:

case NotificationSource.push:
case NotificationSource.remote:
  _localNotificationsService.showOrScheduleNotification(notification);
  break;

3. Stale addRemoteNotification removed -- The dead stub and its "placeholder for future" comment are gone. Foreground FCM messages now flow through addNotification directly.

4. Firebase files added to .gitignore -- Both **/lib/firebase_options.dart and **/ios/Runner/GoogleService-Info.plist are excluded.

5. Dead code getMainAccount() removed from SenotiService.

---

New bug introduced

navigateToTransactionFromPayload will crash on FCM notification taps

void navigateToTransactionFromPayload(Ref ref, dynamic payload, GlobalKey<NavigatorState> navigatorKey) {
    final json = jsonDecode(payload);

This calls jsonDecode(payload) unconditionally, but the callers pass different types:

• Local notifications pass a String (JSON-encoded payload) -- works fine
• FCM passes message.data which is already a Map<String, dynamic> -- jsonDecode(Map) will throw a TypeError at runtime

The FCM caller:

void _handleNotificationTap(RemoteMessage message, GlobalKey<NavigatorState> navigatorKey) {
    final data = message.data;  // Map<String, dynamic>
    ...
    txService.navigateToTransactionFromPayload(_ref, data, navigatorKey);
}

Fix: Handle both types:

void navigateToTransactionFromPayload(dynamic payload, GlobalKey<NavigatorState> navigatorKey) {
    final json = payload is String ? jsonDecode(payload) : payload;
    final event = deserializeTxEventFromJsonIfPossible(json);
    if (event != null) {
      _ref.read(transactionIntentProvider.notifier).state = event;
      navigatorKey.currentState?.pushNamed('/transactions');
    }
  }

Also note the method accepts a Ref ref parameter but uses _ref internally -- the parameter is unused and should be removed.

---

Remaining items from first review

1. print() statements in senoti_service.dart

Still present -- 5 raw print() calls:

print('request challenge');
print('challenge: $ch');
print('msg: $msg');
print('verify ${ch['temp_session_id']!} $senotiEndpointUrl');
print('verify response: ${r.body}');

Some of these also leak sensitive data (signature message, session IDs). Should be removed or converted to debugPrint.

2. Senoti endpoint is still localhost

static const String senotiEndpoint = 'http://localhost:3100/api';

This won't work on a real device. Needs a production URL or env-based toggle before merge.

3. Info.plist whitespace churn

Still a full remove-and-readd of every line due to indentation change (tabs to spaces). Noisy but functional.

---

Summary

Status	Item
Fixed	DRY: Tap navigation extracted to shared method
Fixed	push/remote cases unified
Fixed	Stale addRemoteNotification stub removed
Fixed	Firebase files gitignored
Fixed	Dead getMainAccount() removed
New bug	jsonDecode on Map will crash FCM tap handling
New minor	Unused Ref ref parameter in navigateToTransactionFromPayload
Remaining	print() with sensitive data in senoti_service.dart
Remaining	Senoti endpoint hardcoded to localhost

The jsonDecode type mismatch is a blocker -- tapping an FCM notification will crash. The rest are cleanup items.

[n13]

print is fine but the crash might be an issue

[n13]

Great, I now have the full picture with all 12 commits. Let me compile the updated review.

---

PR #382 Review -- Third Pass (Latest)

The PR has gone through significant iteration since my last two reviews. Let me go through what's been addressed and what remains.

---

Previously Raised Issues -- Now Fixed

1. DRY: Notification tap navigation -- navigateToTransactionFromPayloadIfPossible is now extracted to TransactionService and called from all three locations. Clean.

2. jsonDecode crash on FCM tap -- Fixed. The method now takes Map<String, dynamic>? directly. Local notification callers decode the String first, FCM callers pass message.data directly. No more type mismatch.

3. Unused Ref ref parameter -- Removed. The method uses the class's own _ref.

4. push/remote cases unified -- Clean fall-through:

case NotificationSource.push:
case NotificationSource.remote:
  _localNotificationsService.showOrScheduleNotification(notification);
  break;

5. Dead addRemoteNotification stub removed -- Gone, along with the stale "placeholder for future" comment.

6. Dead getMainAccount() removed from SenotiService.

7. Firebase files added to .gitignore -- Both **/lib/firebase_options.dart and **/ios/Runner/GoogleService-Info.plist are excluded.

8. Feature flag -- Everything is gated behind FeatureFlags.enableRemoteNotifications (currently false) in main.dart, app_initializer.dart, and app.dart. This is a great addition -- it means the feature is safely dormant until ready.

---

Remaining Items

1. Senoti endpoint is still localhost

static const String senotiEndpoint = 'http://localhost:3100/api';

Since the feature flag is false, this won't be hit at runtime right now. But when the flag is flipped, this will fail on real devices. Needs a production URL before enabling.

Severity: Low (blocked by feature flag), but a blocker for turning the flag on.

2. print() statements in senoti_service.dart

Five raw print() calls remain, some logging sensitive data:

print('request challenge');
print('challenge: $ch');
print('msg: $msg');                    // logs the full signature message
print('verify ${ch['temp_session_id']!} $senotiEndpointUrl');
print('verify response: ${r.body}');

These will show in release builds (unlike debugPrint). The signature message and session IDs are sensitive. Again behind the feature flag, but should be cleaned up before enabling.

Severity: Low (behind flag), but a security concern when enabled.

3. No error handling on registerDevice in token refresh listener

In _setupTokenRefreshListener():

_messaging.onTokenRefresh.listen((newToken) {
  _senotiService.registerDevice(newToken, Platform.operatingSystem);
});

If senoti is unreachable, this throws an unhandled async error from a stream listener, which could crash the app. The init() call is wrapped in AppInitializer's try-catch so _getToken() failures are caught, but the refresh listener has no such protection.

Suggestion: Wrap in try-catch:

_messaging.onTokenRefresh.listen((newToken) async {
  try {
    await _senotiService.registerDevice(newToken, Platform.operatingSystem);
  } catch (e) {
    debugPrint('Failed to re-register device: $e');
  }
});

Severity: Medium -- this can crash in production when the flag is on.

4. Missing Android google-services.json

The firebase.json references android/app/google-services.json but this file isn't in the PR. Android builds will fail when the flag is turned on and Firebase tries to initialize. iOS has GoogleService-Info.plist but Android is missing its equivalent.

Severity: Low (feature is off + not tested on Android per the PR description), but needs to be added before Android testing.

5. Info.plist whitespace churn

Still a full rewrite of every line (tabs to spaces). The actual content additions are just UIBackgroundModes and FirebaseAppDelegateProxyEnabled. Cosmetic but makes the diff noisy.

Severity: Cosmetic only.

---

Summary

Status	Item
Fixed	DRY: tap navigation extracted to TransactionService
Fixed	jsonDecode crash on FCM Map payload
Fixed	Unused Ref ref parameter removed
Fixed	push/remote cases unified
Fixed	Dead addRemoteNotification stub removed
Fixed	Dead getMainAccount() removed
Fixed	Firebase files gitignored
New (good)	Feature flag gates all FCM code (currently off)
Remaining	Senoti endpoint is localhost (blocker for flag-on)
Remaining	print() with sensitive data (security concern for flag-on)
Remaining	Unhandled error in token refresh listener (potential crash)
Remaining	Missing Android google-services.json
Remaining	Info.plist whitespace churn (cosmetic)

Verdict

The feature flag makes this safe to merge as-is -- nothing will execute at runtime with enableRemoteNotifications = false. All the blocking bugs from previous reviews are fixed. The remaining items (#1-#4) must be addressed before the flag gets flipped to true, but they don't block this PR from landing. I'd approve this with a note that items 1-3 need resolution before enabling the feature.

[n13]

PR #382 Review: Add Remote Notifications

Branch: beast/remote-notification -> n13/v3_design
Commits: 14
Reviewed: 2026-02-16 (fourth pass -- latest)

---

What This PR Does

Adds Firebase Cloud Messaging (FCM) for remote push notifications. Key additions:

• Firebase configuration for iOS and Android
• FirebaseMessagingService for FCM lifecycle (permissions, token, foreground/background messages, tap handling)
• SenotiService (in quantus_sdk) for device registration with challenge-response auth
• Feature flag enableRemoteNotifications (currently false) gating all FCM code

---

Issues Fixed Since Previous Reviews

All blocking bugs and DRY violations from prior reviews have been addressed:

Item	Status
Notification tap navigation duplicated 3x	Fixed -- extracted to TransactionService.navigateToTransactionFromPayloadIfPossible
jsonDecode crash on FCM Map payload	Fixed -- method now takes Map<String, dynamic>? directly
Unused Ref ref parameter	Fixed -- removed
push/remote notification cases split across layers	Fixed -- unified via fall-through in addNotification
Dead addRemoteNotification stub with stale comments	Fixed -- removed
Dead getMainAccount() in SenotiService	Fixed -- removed
Firebase config files not gitignored	Fixed -- both added to .gitignore
No feature flag for safe rollout	Fixed -- FeatureFlags.enableRemoteNotifications gates all FCM code
print() statements leaking sensitive data in senoti_service.dart	Mostly fixed -- removed from registerDevice, 1 remains in requestChallenge
Unhandled error in token refresh / device registration	Fixed -- _registerDevice wrapper with try-catch
No early return when permission denied	Fixed -- init() checks AuthorizationStatus and returns early

---

Remaining Items

1. Senoti endpoint hardcoded to localhost

static const String senotiEndpoint = 'http://localhost:3100/api';

Every other endpoint in AppConstants points to a production URL. This will fail on real devices. Needs a production URL before the feature flag is turned on.

Severity: Blocker for enabling the feature. Safe while flag is off.

2. One leftover print() in SenotiAuthClient.requestChallenge

Future<Map<String, String>> requestChallenge() async {
    print('request challenge');  // <-- still here

Minor, but print() outputs in release builds unlike debugPrint.

Severity: Low.

3. Missing Android google-services.json

The firebase.json references android/app/google-services.json but this file is not included in the PR. Android builds will fail when Firebase initializes. Per the PR description, Android hasn't been tested yet.

Severity: Blocker for Android. Not relevant while flag is off or iOS-only testing.

4. Info.plist whitespace churn

The entire Info.plist shows as removed and re-added due to indentation change (tabs to spaces). The actual content additions are just UIBackgroundModes (remote-notification, fetch) and FirebaseAppDelegateProxyEnabled = NO. Functional but noisy diff.

Severity: Cosmetic.

---

Code Quality Notes

Good patterns observed:

• Clean separation: SenotiAuthClient (HTTP) vs SenotiService (business logic)
• Proper @pragma('vm:entry-point') on the background handler
• FirebaseAppDelegateProxyEnabled = NO with manual APNs token forwarding in AppDelegate.swift -- avoids swizzling conflicts with flutter_local_notifications
• Feature flag pattern is well-applied across all three integration points (main.dart, app_initializer.dart, app.dart)
• _remoteMessageToNotificationData cleanly maps FCM payloads to the existing NotificationData model

---

Verdict

Approve -- safe to merge with the feature flag off. All previous blocking bugs are resolved. The remaining items (localhost endpoint, missing Android config) must be addressed before flipping enableRemoteNotifications to true, but they don't block landing this PR.

[n13]

GTG!