Push Notifications Integration with Background Support #297
Conversation
WalkthroughReplaces the legacy notification service with a new background notification service, adds a top-level Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Relay as Nostr Relay
participant BG as BackgroundNotificationService
participant DB as Key DB / Sessions
participant EX as NotificationDataExtractor
participant LOC as Localizations (S)
participant FL as FlutterLocalNotificationsPlugin
participant App as App Router
Relay-->>BG: Encrypted NostrEvent (kind 4/1059)
BG->>DB: Load sessions & keys
BG->>BG: Decrypt & parse to MostroMessage
BG->>EX: extractFromMostroMessage(msg, ref?, session)
EX-->>BG: NotificationData? (action, values, isTemporary, ids)
alt has NotificationData
BG->>LOC: Resolve title/body (parameterized by values)
BG->>BG: Build expanded text + NotificationDetails
BG->>FL: show(id, title, body, payload=MostroMessage.id)
else decrypt/parse fail or no data
BG->>FL: show(generic notification)
end
User->>FL: Tap notification
FL->>BG: onSelectNotification(payload)
BG->>App: Navigate / deep-link (notifications or order)
sequenceDiagram
autonumber
participant Stream as Mostro Event Stream
participant AN as AbstractMostroNotifier
participant EX as NotificationDataExtractor
participant NN as NotificationsNotifier
participant Repo as NotificationsRepository
Stream-->>AN: MostroMessage
AN->>AN: Dedup check (event.id+action+timestamp)
alt not processed
AN->>EX: extractFromMostroMessage(...)
EX-->>AN: NotificationData? (values, isTemporary)
opt has data
AN->>NN: sendNotification(action, values, isTemporary, eventId)
NN->>NN: notify() shows temporary notification
NN->>Repo: addToHistory(deterministicId)
Repo-->>NN: notificationExists? true → skip store
end
else duplicate
AN->>AN: Skip handling
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/notifications/notifiers/notifications_notifier.dart (1)
101-109: Deterministic ID can change due to unordered nested maps; canonicalize recursivelyOnly top-level keys are sorted. Nested
valuesmap/list order can vary, producing different hashes for identical payloads. Canonicalize recursively before hashing.Apply this diff:
- // Sort keys for deterministic serialization - final sortedContent = Map.fromEntries( - content.entries.toList()..sort((a, b) => a.key.compareTo(b.key)) - ); - - final contentString = jsonEncode(sortedContent); + // Canonicalize recursively so nested maps/lists are deterministically ordered + final canonical = _canonicalize(content); + final contentString = jsonEncode(canonical);Add this helper inside the class:
// Canonicalize JSON recursively (sort map keys, normalize lists) static Object _canonicalize(Object? input) { if (input is Map) { final keys = input.keys.map((k) => k.toString()).toList()..sort(); return { for (final k in keys) k: _canonicalize(input[k]), }; } if (input is List) { return input.map(_canonicalize).toList(); } if (input is DateTime) return input.toIso8601String(); return input ?? ''; }lib/background/background.dart (1)
28-39: Initialize notifications in the background isolate before showing any.Without initializing FlutterLocalNotificationsPlugin in this isolate, show() calls can fail. Initialize during 'start'.
Apply:
final settings = Settings.fromJson(settingsMap); - currentLanguage = settings.selectedLanguage ?? 'en'; + currentLanguage = settings.selectedLanguage ?? 'en'; + // Initialize local notifications in the background isolate + await notification_service.initializeNotifications(); await nostrService.init(settings);
🧹 Nitpick comments (9)
lib/features/notifications/utils/notification_message_mapper.dart (2)
194-205: DRY: extract parameterized-key handling to a shared helperThe switch for parameterized keys is duplicated here and below. Centralize to avoid drift.
Apply this diff:
- // Handle special keys that require parameters - switch (messageKey) { - case 'released': - final sellerName = values?['seller_npub'] ?? ''; - return s.released(sellerName); - case 'holdInvoicePaymentSettled': - final buyerName = values?['buyer_npub'] ?? ''; - return s.holdInvoicePaymentSettled(buyerName); - default: - return _resolveLocalizationKey(s, messageKey); - } + return _resolveParameterizedMessage(s, messageKey, values);Add this helper in the class:
static String _resolveParameterizedMessage(S s, String key, Map<String, dynamic>? values) { switch (key) { case 'released': final sellerName = values?['seller_npub'] ?? ''; return s.released(sellerName); case 'holdInvoicePaymentSettled': final buyerName = values?['buyer_npub'] ?? ''; return s.holdInvoicePaymentSettled(buyerName); default: return _resolveLocalizationKey(s, key); } }
340-361: Apply the same DRY helper in the instance-based APIReuse the same
_resolveParameterizedMessageto keep both code paths in sync.Apply this diff:
- // Handle special keys that require parameters - switch (messageKey) { - case 'released': - final sellerName = values?['seller_npub'] ?? ''; - return localizations.released(sellerName); - case 'holdInvoicePaymentSettled': - final buyerName = values?['buyer_npub'] ?? ''; - return localizations.holdInvoicePaymentSettled(buyerName); - default: - return _resolveLocalizationKey(localizations, messageKey); - } + return _resolveParameterizedMessage(localizations, messageKey, values);lib/main.dart (2)
10-10: Harden notification init with error handlingInitialization failures should not block app startup. Wrap in try/catch and log.
- await initializeNotifications(); + try { + await initializeNotifications(); + } catch (e, st) { + debugPrint('Failed to initialize notifications: $e'); + debugPrintStack(stackTrace: st); + }Also applies to: 33-33
76-84: Also register short timeago localesShort formats are common in UI; registering them avoids fallback to English.
// Set Spanish locale for timeago timeago.setLocaleMessages('es', timeago.EsMessages()); + timeago.setLocaleMessages('es_short', timeago.EsShortMessages()); // Set Italian locale for timeago timeago.setLocaleMessages('it', timeago.ItMessages()); + timeago.setLocaleMessages('it_short', timeago.ItShortMessages());lib/features/order/notfiers/add_order_notifier.dart (1)
46-49: Order of operations: reset before async side-effects (optional)Consider running
_resetForRetry()before the fire-and-forget handler to avoid racing state updates.- // Reset for retry if out_of_range_sats_amount - final cantDo = msg.getPayload<CantDo>(); - if (cantDo?.cantDoReason == CantDoReason.outOfRangeSatsAmount) { - _resetForRetry(); - } + // Reset for retry if out_of_range_sats_amount + final cantDo = msg.getPayload<CantDo>(); + if (cantDo?.cantDoReason == CantDoReason.outOfRangeSatsAmount) { + _resetForRetry(); + } + // Fire-and-forget after local reset + // (leave as-is if super.handleEvent already handles this case)Would you confirm whether
super.handleEventalso issues a reset foroutOfRangeSatsAmount? If yes, we should consolidate the behavior to a single place to avoid double work.lib/background/background.dart (1)
13-15: Unused state: isAppForeground.If it no longer gates notifications, remove it; otherwise, use it to suppress foreground duplicates.
-bool isAppForeground = true; -String currentLanguage = 'en'; +String currentLanguage = 'en';lib/features/notifications/utils/notification_data_extractor.dart (1)
6-6: Remove unused import.mostro_instance.dart isn’t referenced directly here.
-import 'package:mostro_mobile/features/mostro/mostro_instance.dart';lib/features/order/notfiers/abstract_mostro_notifier.dart (1)
21-22: Bound the dedupe set to avoid unbounded growth.For long-lived notifiers, _processedEventIds can grow indefinitely.
Consider a size-capped LRU or time-based cleanup (e.g., retain last N or last 24h).
lib/features/notifications/services/background_notification_service.dart (1)
140-156: Avoid reloading DB and keys on every event (hot path)._openMostroDatabase + KeyManager.init + SessionStorage.getAll() per event is heavy.
- Cache DB handle, KeyManager, and sessions list in this isolate.
- Refresh sessions on 'update-settings'/'start' or on a timer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
lib/background/background.dart(4 hunks)lib/features/notifications/notifiers/notifications_notifier.dart(1 hunks)lib/features/notifications/services/background_notification_service.dart(1 hunks)lib/features/notifications/utils/notification_data_extractor.dart(1 hunks)lib/features/notifications/utils/notification_message_mapper.dart(3 hunks)lib/features/order/notfiers/abstract_mostro_notifier.dart(10 hunks)lib/features/order/notfiers/add_order_notifier.dart(1 hunks)lib/features/order/notfiers/order_notifier.dart(1 hunks)lib/main.dart(1 hunks)lib/notifications/notification_service.dart(0 hunks)test/mocks.mocks.dart(1 hunks)
💤 Files with no reviewable changes (1)
- lib/notifications/notification_service.dart
🧰 Additional context used
📓 Path-based instructions (7)
**/*.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.mocks.dart: Never manually edit Mockito-generated*.mocks.dartfiles
Do not add ignore comments to*.mocks.dart; regenerate instead if analyzer issues appear
Files:
test/mocks.mocks.dart
test/mocks.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
test/mocks.mocks.dart: Do not manually modifytest/mocks.mocks.dart(auto-generated by Mockito)
Do not add additional ignore directives; file already has file-level ignores
Files:
test/mocks.mocks.dart
{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}: Checkmountedbefore usingBuildContextafterawait(async gaps)
Remove unused imports and dependencies
Files:
test/mocks.mocks.dartlib/main.dartlib/features/order/notfiers/add_order_notifier.dartlib/features/notifications/utils/notification_data_extractor.dartlib/features/notifications/notifiers/notifications_notifier.dartlib/background/background.dartlib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/notifications/utils/notification_message_mapper.dartlib/features/order/notfiers/order_notifier.dartlib/features/notifications/services/background_notification_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
All code comments, variable names, and function names must be in English
Files:
test/mocks.mocks.dartlib/main.dartlib/features/order/notfiers/add_order_notifier.dartlib/features/notifications/utils/notification_data_extractor.dartlib/features/notifications/notifiers/notifications_notifier.dartlib/background/background.dartlib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/notifications/utils/notification_message_mapper.dartlib/features/order/notfiers/order_notifier.dartlib/features/notifications/services/background_notification_service.dart
test/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under
test/
Files:
test/mocks.mocks.dart
{lib/*.dart,lib/!(generated)/**/*.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
{lib/*.dart,lib/!(generated)/**/*.dart}: Use localized strings; replace hardcoded user-facing text withS.of(context).keyName
Preferconstconstructors andconstwhere possible
Use latest Flutter/Dart APIs (e.g.,withValues()instead of deprecatedwithOpacity())
Files:
lib/main.dartlib/features/order/notfiers/add_order_notifier.dartlib/features/notifications/utils/notification_data_extractor.dartlib/features/notifications/notifiers/notifications_notifier.dartlib/background/background.dartlib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/notifications/utils/notification_message_mapper.dartlib/features/order/notfiers/order_notifier.dartlib/features/notifications/services/background_notification_service.dart
lib/main.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Configure timeago locales in main startup (use
timeago.setLocaleMessages(...))
Files:
lib/main.dart
🧠 Learnings (11)
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
PR: MostroP2P/mobile#215
File: lib/features/order/notfiers/order_notifier.dart:0-0
Timestamp: 2025-07-20T23:33:17.689Z
Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Applied to files:
test/mocks.mocks.dartlib/features/order/notfiers/add_order_notifier.dartlib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/order/notfiers/order_notifier.dart
📚 Learning: 2025-06-04T19:35:20.209Z
Learnt from: chebizarro
PR: MostroP2P/mobile#110
File: test/notifiers/take_order_notifier_test.dart:72-74
Timestamp: 2025-06-04T19:35:20.209Z
Learning: MostroService methods like takeBuyOrder() and takeSellOrder() return Future<void> and trigger side effects through other mechanisms rather than direct return values. When testing these methods, focus on verifying method calls and testing state changes through the provider system rather than mocking return values.
Applied to files:
test/mocks.mocks.dartlib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-08-28T20:32:34.625Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Applies to test/mocks.mocks.dart : Do not manually modify `test/mocks.mocks.dart` (auto-generated by Mockito)
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-08-28T20:32:34.625Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Applies to test/mocks.dart : When changing Mockito annotations in `test/mocks.dart`, regenerate mocks with `dart run build_runner build -d`
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
lib/main.dart
📚 Learning: 2025-06-26T15:03:23.529Z
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/order/notfiers/order_notifier.dart
📚 Learning: 2025-08-28T20:32:34.625Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Use Riverpod for state management with Notifiers encapsulating business logic; access data only through repositories
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.974Z
Learning: In the Mostro mobile app's RelaySelector widget (lib/features/relays/widgets/relay_selector.dart), watching relaysProvider.notifier correctly triggers rebuilds because the relaysProvider itself depends on settingsProvider (line 8 in relays_provider.dart). When blacklist changes via toggleMostroRelayBlacklist(), the settingsProvider updates, causing relaysProvider to rebuild, which then notifies widgets watching the notifier. The UI correctly reflects active/inactive states in real-time through this dependency chain.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-08-28T20:32:34.625Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Applies to {lib/*.dart,lib/!(generated)/**/*.dart} : Use localized strings; replace hardcoded user-facing text with `S.of(context).keyName`
Applied to files:
lib/features/notifications/utils/notification_message_mapper.dart
📚 Learning: 2025-05-08T16:29:52.154Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/background/background.dart:74-77
Timestamp: 2025-05-08T16:29:52.154Z
Learning: In the Mostro Mobile background service architecture, events aren't stored by the background process. Instead, the background service only checks if events exist in the eventStore and sends notifications for new ones, while the foreground process is responsible for storing and processing events. This is an intentional design decision to separate concerns.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
🔇 Additional comments (9)
lib/features/notifications/utils/notification_message_mapper.dart (1)
121-121: ARB keys present; generated localization not found — regenerate or commit generated S
- lib/l10n/intl_en.arb, lib/l10n/intl_es.arb, lib/l10n/intl_it.arb contain "released" and "holdInvoicePaymentSettled".
- No generated localization class/file was found (no lib/generated/l10n.dart or S/AppLocalizations detected). Verify generated getters S.released(...) and S.holdInvoicePaymentSettled(...) exist with matching placeholders; run the project’s localization generator (gen-l10n / flutter_intl) or commit the generated output.
lib/features/order/notfiers/add_order_notifier.dart (1)
43-43: Prevent unhandled Future errors when using unawaitedIf
handleEventthrows, errors may surface as unhandled. Catch and log.- unawaited(handleEvent(msg)); + unawaited( + handleEvent(msg).catchError( + (e, st) => logger.e('handleEvent failed in AddOrderNotifier', error: e, stackTrace: st), + ), + );⛔ Skipped due to learnings
Learnt from: Catrya PR: MostroP2P/mobile#215 File: lib/features/order/notfiers/order_notifier.dart:0-0 Timestamp: 2025-07-20T23:33:17.689Z Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.Learnt from: Catrya PR: MostroP2P/mobile#215 File: lib/features/order/notfiers/order_notifier.dart:0-0 Timestamp: 2025-07-20T23:33:17.689Z Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.Learnt from: Catrya PR: MostroP2P/mobile#215 File: lib/features/order/notfiers/order_notifier.dart:0-0 Timestamp: 2025-07-20T23:33:17.689Z Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.lib/features/order/notfiers/order_notifier.dart (1)
26-32: Awaiting base handler is correct — audit non‑awaiting callerslib/features/order/notfiers/order_notifier.dart already uses
await super.handleEvent(event);— keep that.
- Found
unawaited(handleEvent(msg))in lib/features/order/notfiers/add_order_notifier.dart:43 and lib/features/order/notfiers/abstract_mostro_notifier.dart:56 — confirm these are intentionally fire‑and‑forget and that errors/ordering are handled.- Tests/mocks declare an async
handleEvent(test/mocks.mocks.dart:2789) — update mocks/expectations if behavior changed.test/mocks.mocks.dart (1)
2789-2797: Revert manual edits to Mockito-generated mocks and regenerate locally.test/mocks.mocks.dart (lines 2789–2797) shows a manual change — revert it and regenerate mocks locally (couldn't run build_runner here:
dart: command not found).Regenerate:
#!/bin/bash dart run build_runner build -dVerify only generated diffs remain:
#!/bin/bash rg -n "mockito" test | sed -n '1,50p'lib/features/notifications/utils/notification_data_extractor.dart (1)
192-198: Verify orderId vs eventId mapping.Both fields are set to event.id. If event.id represents the order ID, provide a distinct event identifier when available (e.g., event.eventId). Otherwise deep links and dedupe may misbehave.
Would you like me to scan the codebase to confirm MostroMessage fields and usages?
lib/features/order/notfiers/abstract_mostro_notifier.dart (2)
56-57: Good: fire-and-forget with unawaited for async handleEvent().Prevents blocking the stream while enabling async processing.
133-139: Confirm pay-invoice navigation uses the correct identifier.Route uses '/pay_invoice/${event.id!}'. If event.id is the order ID, OK; if it’s a message/event ID, use orderId instead.
I can grep usages and definitions to verify mapping.
lib/features/notifications/services/background_notification_service.dart (2)
88-95: Critical alerts on iOS require entitlement.Using InterruptionLevel.critical will be ignored or cause review issues without the entitlement. Prefer .active unless the app has approval.
Would you like me to gate this by a config flag and default to .active?
112-121: Verify recipient matching logic for decryption.Matching sessions via s.tradeKey.public == event.recipient assumes recipient is the session pubkey. Confirm for kinds 4/1059; otherwise decryption will fail intermittently.
I can search the nostr lib docs and our models if needed.
lib/features/notifications/services/background_notification_service.dart
Show resolved
Hide resolved
lib/features/notifications/services/background_notification_service.dart
Show resolved
Hide resolved
lib/features/notifications/services/background_notification_service.dart
Show resolved
Hide resolved
lib/features/notifications/services/background_notification_service.dart
Show resolved
Hide resolved
lib/features/notifications/services/background_notification_service.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/features/order/notfiers/abstract_mostro_notifier.dart (3)
33-38: Fix LateInitializationError risk: session may be uninitialized.
If oldSession is null, session remains unset but is later accessed (e.g., extractor, _isUserCreator), which will crash at runtime.Apply one of these approaches (preferred A):
final oldSession = ref.read(sessionNotifierProvider.notifier).getSessionByOrderId(orderId); if (oldSession != null) { session = oldSession; - } + } else { + // A) Ensure a session is created/provisioned by the notifier + final sn = ref.read(sessionNotifierProvider.notifier); + session = sn.ensureSession(orderId); // implement ensureSession(...) if missing + // B) If no helper exists, minimally initialize an empty session for this order + // session = Session(orderId: orderId); // use the correct constructor for your model + }I can draft ensureSession(orderId) in the session notifier if you want.
134-138: Avoid null-bang on event.id during navigation.
event.id may be null; protect the navigation.- if (event.payload is PaymentRequest) { - navProvider.go('/pay_invoice/${event.id!}'); - } + if (event.payload is PaymentRequest) { + final id = event.id; + if (id != null) { + navProvider.go('/pay_invoice/$id'); + } else { + logger.w('payInvoice received without event.id; skipping navigation'); + } + }
201-209: Don't rely on ref.invalidateSelf() for StateNotifier — make invalidation explicit.StateNotifierProviderRef exposes invalidateSelf(), but StateNotifier-based providers can remain "mounted" (watching their internal notifier) and may not dispose/refresh as expected.
- Replace with explicit invalidation: ref.invalidate(yourProvider(/id/)) to target the provider instance.
- Or mark the provider autoDispose so invalidate/invalidateSelf disposes reliably.
- Long-term: migrate to Notifier/AsyncNotifier (recommended) for dependable self-invalidation.
Location: lib/features/order/notfiers/abstract_mostro_notifier.dart:206–209 (current call: ref.invalidateSelf()). Review other occurrences (add_order_notifier.dart, order_notifier.dart).
🧹 Nitpick comments (5)
lib/features/order/notfiers/abstract_mostro_notifier.dart (5)
85-91: Dedup key may overfit and the set can grow unbounded.
Using id+action+timestamp can miss true dups (if timestamp drifts) and allows unbounded growth per notifier.- final eventKey = '${event.id}_${event.action}_${event.timestamp}'; + // Prefer stable IDs; fall back to action+timestamp only if id is null. + final eventKey = event.id ?? '${event.action}_${event.timestamp}'; if (_processedEventIds.contains(eventKey)) { logger.d('Skipping duplicate event: $eventKey'); return; } _processedEventIds.add(eventKey); + // Optional: cap size to prevent unbounded growth (tune threshold per order lifecycle) + if (_processedEventIds.length > 2048) _processedEventIds.clear();If the protocol guarantees unique ids, consider using just event.id.
119-126: Keep local session field in sync after updateSession.
You update the stored session via provider but not the notifier’s session field, leaving it stale for later logic/extractor calls.final sessionProvider = ref.read(sessionNotifierProvider.notifier); final peer = order.buyerTradePubkey != null ? Peer(publicKey: order.buyerTradePubkey!) : null; sessionProvider.updateSession(orderId, (s) => s.peer = peer); state = state.copyWith(peer: peer); + // Refresh local cache + session = sessionProvider.getSessionByOrderId(orderId) ?? session;Apply similar refresh in other branches where the session is mutated (e.g., holdInvoicePaymentAccepted).
146-161: Mirror session refresh after updating peer.
Same staleness concern as buyerTookOrder; refresh local session field after update.sessionProvider.updateSession(orderId, (s) => s.peer = peer); state = state.copyWith(peer: peer); + session = sessionProvider.getSessionByOrderId(orderId) ?? session;
171-178: Prevent unintended navigation when order is not yet hydrated.
If state.order is null (first messages), isBuy/Sell evaluation defaults to false, triggering navigation unintentionally.- case Action.waitingSellerToPay: + case Action.waitingSellerToPay: + if (state.order == null) break; // Navigation logic final isUserCreator = _isUserCreator(); final isBuyOrder = state.order?.kind == OrderType.buy; if (!(isUserCreator && isBuyOrder)) { navProvider.go('/trade_detail/$orderId'); } break; @@ - case Action.waitingBuyerInvoice: + case Action.waitingBuyerInvoice: + if (state.order == null) break; // Navigation logic final isUserCreator = _isUserCreator(); final isSellOrder = state.order?.kind == OrderType.sell; if (!(isUserCreator && isSellOrder)) { navProvider.go('/trade_detail/$orderId'); } break;Also applies to: 180-187
51-56: Normalize MostroMessage.timestamp to milliseconds before comparing. MostroMessage.timestamp is declared as int? (lib/data/models/mostro_message.dart) and many code paths already use milliseconds (mostro_storage, background_notification_service), but MostroMessage.fromJson assigns json['timestamp'] raw — add normalization there (e.g. if ts < 10_000_000_000 then ts *= 1000) or normalize before the 60s comparison in lib/features/order/notfiers/abstract_mostro_notifier.dart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/order/notfiers/abstract_mostro_notifier.dart(9 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{lib/*.dart,lib/!(generated)/**/*.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
{lib/*.dart,lib/!(generated)/**/*.dart}: Use localized strings; replace hardcoded user-facing text withS.of(context).keyName
Preferconstconstructors andconstwhere possible
Use latest Flutter/Dart APIs (e.g.,withValues()instead of deprecatedwithOpacity())
Files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}: Checkmountedbefore usingBuildContextafterawait(async gaps)
Remove unused imports and dependencies
Files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
All code comments, variable names, and function names must be in English
Files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
🧠 Learnings (7)
📓 Common learnings
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/background/background.dart:74-77
Timestamp: 2025-05-08T16:29:52.154Z
Learning: In the Mostro Mobile background service architecture, events aren't stored by the background process. Instead, the background service only checks if events exist in the eventStore and sends notifications for new ones, while the foreground process is responsible for storing and processing events. This is an intentional design decision to separate concerns.
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
📚 Learning: 2025-06-26T15:03:23.529Z
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
PR: MostroP2P/mobile#215
File: lib/features/order/notfiers/order_notifier.dart:0-0
Timestamp: 2025-07-20T23:33:17.689Z
Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-08-28T20:32:34.625Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Use Riverpod for state management with Notifiers encapsulating business logic; access data only through repositories
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.974Z
Learning: In the Mostro mobile app's RelaySelector widget (lib/features/relays/widgets/relay_selector.dart), watching relaysProvider.notifier correctly triggers rebuilds because the relaysProvider itself depends on settingsProvider (line 8 in relays_provider.dart). When blacklist changes via toggleMostroRelayBlacklist(), the settingsProvider updates, causing relaysProvider to rebuild, which then notifies widgets watching the notifier. The UI correctly reflects active/inactive states in real-time through this dependency chain.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-06-04T19:35:20.209Z
Learnt from: chebizarro
PR: MostroP2P/mobile#110
File: test/notifiers/take_order_notifier_test.dart:72-74
Timestamp: 2025-06-04T19:35:20.209Z
Learning: MostroService methods like takeBuyOrder() and takeSellOrder() return Future<void> and trigger side effects through other mechanisms rather than direct return values. When testing these methods, focus on verifying method calls and testing state changes through the provider system rather than mocking return values.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
🔇 Additional comments (4)
lib/features/order/notfiers/abstract_mostro_notifier.dart (4)
1-2: Import of dart:async is appropriate for unawaited.
Needed for unawaited usage below. LGTM.
73-81: Notification routing looks good.
Centralizing via notificationActionsProvider with eventId support is clean.
56-56: Unawaited future without error trap.
If handleEvent throws asynchronously, the exception may surface as an unhandled zone error. Wrap handleEvent body with try/catch and route to handleError.- Future<void> handleEvent(MostroMessage event) async { - // Skip if we've already processed this exact event + Future<void> handleEvent(MostroMessage event) async { + try { + // Skip if we've already processed this exact event ... - } + } catch (e, st) { + handleError(e, st); + } }⛔ Skipped due to learnings
Learnt from: Catrya PR: MostroP2P/mobile#215 File: lib/features/order/notfiers/order_notifier.dart:0-0 Timestamp: 2025-07-20T23:33:17.689Z Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.Learnt from: Catrya PR: MostroP2P/mobile#215 File: lib/features/order/notfiers/order_notifier.dart:0-0 Timestamp: 2025-07-20T23:33:17.689Z Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.Learnt from: Catrya PR: MostroP2P/mobile#215 File: lib/features/order/notfiers/order_notifier.dart:0-0 Timestamp: 2025-07-20T23:33:17.689Z Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
83-83: handleEvent changed to async — no action requiredNo remaining overrides use
void; overrides areFuture<void>and call sites eitherawaitthe super call (lib/features/order/notfiers/order_notifier.dart) or intentionally fire-and-forget withunawaited(...)(lib/features/order/notfiers/abstract_mostro_notifier.dart, lib/features/order/notfiers/add_order_notifier.dart).
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Great job!
There is something that is not ok, I have my mostro app on the default system language, in my case is spanish but the push notifications are being displayed on english, it must be displaying the push notification in the same language the user have selected
|
About firebase, yes that would be outside the scope of this PR, I'm curious on how that would impact on the privacy of our users? On the other hand, I think we can keep working with the app like we are now, knowing that when the app is terminated no notifications will be display, and start a discussion on the next step improving this situation, but for now I think the best thing to do is just keep it. |
BraCR10
force-pushed
the
feat/br/push-notifications
branch
from
c7f7606 to
7d4ff8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/l10n/intl_es.arb (1)
989-991: ES i18n: Use “Calificación” for consistency with existing UI.Elsewhere you use “Calificación” (e.g., rating labels/buttons). Align this key to avoid mixed terminology.
Apply:
- "notificationRate": "Valoración", + "notificationRate": "Calificación",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/features/notifications/services/background_notification_service.dart(1 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_es.arb(1 hunks)lib/l10n/intl_it.arb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/notifications/services/background_notification_service.dart
🧰 Additional context used
📓 Path-based instructions (1)
lib/l10n/intl_{en,es,it}.arb
📄 CodeRabbit inference engine (CLAUDE.md)
Add new localization keys to all three ARB files (en, es, it)
Files:
lib/l10n/intl_en.arblib/l10n/intl_it.arblib/l10n/intl_es.arb
🧠 Learnings (2)
📓 Common learnings
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/background/background.dart:74-77
Timestamp: 2025-05-08T16:29:52.154Z
Learning: In the Mostro Mobile background service architecture, events aren't stored by the background process. Instead, the background service only checks if events exist in the eventStore and sends notifications for new ones, while the foreground process is responsible for storing and processing events. This is an intentional design decision to separate concerns.
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
📚 Learning: 2025-08-28T20:32:34.625Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Applies to lib/l10n/intl_{en,es,it}.arb : Add new localization keys to all three ARB files (en, es, it)
Applied to files:
lib/l10n/intl_en.arblib/l10n/intl_it.arblib/l10n/intl_es.arb
🔇 Additional comments (2)
lib/l10n/intl_it.arb (1)
1021-1023: IT i18n: New notification detail labels look correct and consistent.Translations align with existing rating terminology.
lib/l10n/intl_en.arb (1)
1043-1045: EN i18n: New notification detail labels added correctly (verified).
Confirmed notificationToken, notificationExpiresIn, and notificationRate exist and are non-empty in lib/l10n/intl_en.arb, lib/l10n/intl_es.arb, and lib/l10n/intl_it.arb.
Yes sir, I think that if implemented on Mostro there would be no privacy problems because the messages are encrypted where FCM only delivers them and wakes up the app, and is decrypted locally in the app. The only drawback I see is that it requires centralization by Firebase because of the need for a running server to send FCM notifications and wake up the app |
|
Hello @grunch , the PR is ready to review, pls let me know if there is any problem |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/features/notifications/utils/notification_message_mapper.dart (2)
194-202: Add a localized fallback when buyer name is missing.Avoid rendering an empty placeholder in s.holdInvoicePaymentSettled(''). Fallback to the static message when buyer_npub is absent/blank.
Apply:
- switch (messageKey) { - case 'holdInvoicePaymentSettled': - final buyerName = values?['buyer_npub'] ?? ''; - return s.holdInvoicePaymentSettled(buyerName); - default: - return _resolveLocalizationKey(s, messageKey); - } + switch (messageKey) { + case 'holdInvoicePaymentSettled': + final buyerName = (values?['buyer_npub'] ?? '').toString().trim(); + if (buyerName.isNotEmpty) { + return s.holdInvoicePaymentSettled(buyerName); + } + return s.notification_hold_invoice_payment_settled_message; + default: + return _resolveLocalizationKey(s, messageKey); + }
337-355: DRY + same fallback for background path.Mirror the above fallback in the instance-based method and consider extracting a private helper to remove duplication.
Apply:
- static String getLocalizedMessageWithInstance(S localizations, mostro.Action action, {Map<String, dynamic>? values}) { + static String getLocalizedMessageWithInstance(S localizations, mostro.Action action, {Map<String, dynamic>? values}) { final messageKey = getMessageKeyWithContext(action, values); - - // Handle special keys that require parameters - switch (messageKey) { - case 'holdInvoicePaymentSettled': - final buyerName = values?['buyer_npub'] ?? ''; - return localizations.holdInvoicePaymentSettled(buyerName); - default: - return _resolveLocalizationKey(localizations, messageKey); - } + + // Handle special keys that require parameters + switch (messageKey) { + case 'holdInvoicePaymentSettled': + final buyerName = (values?['buyer_npub'] ?? '').toString().trim(); + if (buyerName.isNotEmpty) { + return localizations.holdInvoicePaymentSettled(buyerName); + } + return localizations.notification_hold_invoice_payment_settled_message; + default: + return _resolveLocalizationKey(localizations, messageKey); + } }Optional consolidation:
+ static String _resolveParameterizedMessage(S s, String key, Map<String, dynamic>? values) { + switch (key) { + case 'holdInvoicePaymentSettled': + final buyerName = (values?['buyer_npub'] ?? '').toString().trim(); + return buyerName.isNotEmpty + ? s.holdInvoicePaymentSettled(buyerName) + : s.notification_hold_invoice_payment_settled_message; + default: + return _resolveLocalizationKey(s, key); + } + }And call _resolveParameterizedMessage from both methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/notifications/utils/notification_message_mapper.dart(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{lib/*.dart,lib/!(generated)/**/*.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
{lib/*.dart,lib/!(generated)/**/*.dart}: Use localized strings; replace hardcoded user-facing text withS.of(context).keyName
Preferconstconstructors andconstwhere possible
Use latest Flutter/Dart APIs (e.g.,withValues()instead of deprecatedwithOpacity())
Files:
lib/features/notifications/utils/notification_message_mapper.dart
{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}: Checkmountedbefore usingBuildContextafterawait(async gaps)
Remove unused imports and dependencies
Files:
lib/features/notifications/utils/notification_message_mapper.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
All code comments, variable names, and function names must be in English
Files:
lib/features/notifications/utils/notification_message_mapper.dart
🧠 Learnings (2)
📓 Common learnings
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/background/background.dart:74-77
Timestamp: 2025-05-08T16:29:52.154Z
Learning: In the Mostro Mobile background service architecture, events aren't stored by the background process. Instead, the background service only checks if events exist in the eventStore and sends notifications for new ones, while the foreground process is responsible for storing and processing events. This is an intentional design decision to separate concerns.
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
📚 Learning: 2025-08-28T20:32:34.625Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Applies to {lib/*.dart,lib/!(generated)/**/*.dart} : Use localized strings; replace hardcoded user-facing text with `S.of(context).keyName`
Applied to files:
lib/features/notifications/utils/notification_message_mapper.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
lib/features/notifications/utils/notification_message_mapper.dart (1)
129-129: Stabilize dynamic key and verify l10n artifacts (manual verification required)Return a named constant instead of the bare string 'holdInvoicePaymentSettled' and confirm ARB entries + generated getters exist across locales — quick check failed because lib/generated/l10n.dart was missing, so getters/ARB presence couldn't be verified.
Actions:
Replace literal with a constant (example): const kNotificationHoldInvoicePaymentSettled = 'holdInvoicePaymentSettled'; use it in lib/features/notifications/utils/notification_message_mapper.dart.
Verify localization artifacts (run from repo root):
rg -n --hidden "holdInvoicePaymentSettled" || true
fd -e arb -0 | xargs -0 rg -n "holdInvoicePaymentSettled" || true
rg -nP '\bnotification_hold_invoice_payment_settled_message\b' || true
rg -n "l10n.dart|class S\b" lib || trueIf generated files are missing, run the project l10n generator (e.g., flutter gen-l10n) and re-run the checks.
Remove the old ARB key 'notification_hold_invoice_payment_settled_message' if it's unused to avoid translator drift.
|
@BraCR10 great job! please add a lightning invoice with amount of 300000 sats in this PR as a comment |
|
Hello fine! pleased to contribute, I hope to continue contributing actively to this project Address: bracr10@pay.bitcoinjungle.app |
Payment hash: 1de58e28cc7ee2491f25b931879284e101c0398a3ff1b0d0540e2ecf576b5b7b |
3 participants
Overview
Closes #223
This PR integrates descriptive push notifications using background processing capabilities.
Note that this PR is not ready for review as it is still in testing phase.
Current Implementation Status
The current implementation successfully handles push notifications in foreground when the app is active and in background when the app is running in second plane. However, push notifications do not work when the app is completely terminated.
Research & Investigation
After extensive research and multiple implementation attempts, I have found that there is no native way to synchronize push notifications when the application is in terminated state. Flutter cannot handle background synchronization in terminated state due to strict restrictions imposed by both Android and iOS platforms.
We are currently using the flutter_background package (https://pub.dev/packages/flutter_background), but according to its documentation, it has significant limitations on both platforms. The package only functions in background and foreground states and does not support terminated state due to strict platform restrictions and limitations imposed by Android and iOS background execution policies.
Alternative Solutions
During research, I found that Firebase Cloud Messaging (FCM) appears to be the only viable solution for handling push notifications in terminated state on Flutter. FCM (https://pub.dev/packages/firebase_messaging and https://firebase.google.com/docs/cloud-messaging) works across all application states including terminated state by leveraging platform-native notification systems. However, implementing FCM would be outside the current scope of this PR, as a service like FCM would require a central server that triggers the application remotely. This would necessitate a complete rethinking of our current flow and application architecture.
I would appreciate @grunch feedback on this approach or if you have suggestions for alternative solutions I might be missing.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores