FCM implementation for push notification #357
Conversation
…round and fcm service
- Add Lightning address usage detection in HoldInvoicePaymentAccepted
- Search previous order confirmation messages for buyerInvoice
- Display lightningAddressUsed notification for buy order makers
- Use existing notification system for consistent UX
- Only applies to buyers who used their configured Lightning address
#354) * feat: improve countdown timer with order_expires_at and dynamic scaling in pending orders - Use exact timestamps from order_expires_at tag for precise calculations - Add dynamic day/hour scaling: 14d 20h 06m (>24h) or HH:MM:SS (≤24h) - Refactor: Create shared DynamicCountdownWidget to remove duplication - Apply consistent behavior across TakeOrderScreen and TradeDetailScreen * fix: use int.tryParse for order_expires_at parsing and update documentation * fix: safe parsing for order countdown timestamps * fix: prevent countdown crashes with zero totals from short orders * update documentation * coderabbit suggestion * use expires_at
WalkthroughAdds Firebase/FCM integration: project config and platform Firebase options, client FCM service and background handler, push registration/encryption service, Cloud Functions poller to send data-only silent pushes, notification deduplication storage, lifecycle hooks to process pending pushes, Gradle/plugin updates, TypeScript/Dart configs, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CF as Cloud Function (keepAlive)
participant NR as Nostr Relays
participant FCM as Firebase Cloud Messaging
participant Device as Mobile Device
participant SP as SharedPreferences
Note over CF: Scheduled poll (keepAlive)
CF->>NR: Open WS, REQ kind=1059 since lastCheckTimestamp
NR-->>CF: Events per relay
CF->>CF: Aggregate events, update lastCheckTimestamp
alt New events found
CF->>FCM: Send data-only silent push to topic
FCM-->>Device: Deliver silent push
Device->>SP: Set fcm.pending_fetch = true
else No new events
CF->>CF: Log zero events
end
sequenceDiagram
autonumber
participant App as Mobile App
participant LM as Lifecycle Manager / App
participant Settings as Settings Service
participant BG as Background Notification Service
participant NR as Nostr Relays
participant DB as notification_state Store
participant SP as SharedPreferences
App->>LM: App resumes (foreground)
LM->>SP: Read fcm.pending_fetch
alt pending_fetch == true
LM->>Settings: Read relays
alt relays configured
LM->>BG: fetchAndProcessNewEvents(relays)
loop per session
BG->>NR: Fetch kind=1059 since last timestamp
NR-->>BG: Return events
BG->>DB: isProcessed(eventId)?
alt not processed
BG->>BG: Decrypt/extract recipient, show local notification
DB->>DB: markAsProcessed(eventId)
end
end
BG->>SP: Update timestamps
LM->>SP: Clear fcm.pending_fetch
else No relays configured
LM->>LM: Log & skip
end
else
LM->>LM: Continue normal resume
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)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (13)
lib/data/models/nostr_event.dart (1)
42-42: NewexpiresAtgetter is consistent with existing tag accessorsExposing the raw
expires_attag viaexpiresAtfits nicely with the other tag-based getters and keeps parsing concerns in higher layers (e.g.,Order.fromEvent). If you find yourself repeatedly converting this toDateTime, consider adding a parallel typed getter later for symmetry withexpirationDate, but it’s not required for this PR.lib/features/order/notfiers/abstract_mostro_notifier.dart (1)
267-294: Lightning-address usage detection works but may double‑notify; confirm protocol assumptionsThis post‑
holdInvoicePaymentAcceptedcheck for anewOrdermessage with abuyerInvoicethat looks like a Lightning address is reasonable and safely wrapped in atry/catch. Two things worth verifying:
Double notification risk for buyers:
_handleAddInvoiceWithAutoLightningAddressalready showslightningAddressUsedwhen a default Lightning address is auto‑sent. With this new block, the same buyer flow could see that message again when the hold invoice is accepted. If that’s not desired, you might want a small guard (e.g., a flag in session or storage) to ensure the message is only shown once per order.Semantics of
buyerInvoice:
This logic assumesbuyerInvoicecontains a Lightning address (matchinguser@domain), not just a BOLT11 invoice. If in some cases it is a standard invoice string,_isValidLightningAddresswill always fail and the notification will never fire. Please confirm that on the Mostro sidebuyerInvoiceis indeed populated with the Lightning address in the scenarios you’re targeting.If both points align with the intended UX/protocol, the implementation here looks good.
PLAN_FCM.md (1)
1-108: Planning document language consideration.This planning document is written in Spanish, while the coding guidelines specify that all code comments and documentation should be in English. Although this is a planning document rather than code comments, for consistency across the project and to ensure all team members can easily access the documentation, consider creating an English version or translating this document.
The content itself is comprehensive and well-structured, covering all critical aspects of FCM implementation including security, architecture, phases, and important considerations about Firebase project setup and costs.
lib/data/models/order.dart (1)
184-199: SaferexpiresAtparsing from Nostr event looks goodUsing
int.tryParse(event.expiresAt!)with a null fallback avoids runtime cast errors when the tag is missing or malformed and keepsOrder.expiresAtconsistent with the JSON parsing logic. If you ever need observability around badexpires_attags, consider adding logging at the extension level, but that’s optional for now.functions/.eslintrc.js (1)
1-33: TypeScript ESLint setup for Cloud Functions is sensibleConfig looks good: Node env, TS parser with
projectset, and ignoring/liband/generatedwill keep linting focused on sources. This should play nicely with yourtsconfig.json/tsconfig.dev.json.One optional hardening tweak: if you ever run ESLint from the monorepo root instead of the
functionsdirectory, addingtsconfigRootDir: __dirnameunderparserOptionshelps avoid “failed to load parserOptions.project” path issues:parserOptions: { project: ["tsconfig.json", "tsconfig.dev.json"], tsconfigRootDir: __dirname, sourceType: "module", },lib/features/trades/screens/trade_detail_screen.dart (2)
87-93: PassingexpiresAtTimestampin milliseconds is correct; consider createdAt consistencyUsing
orderPayload.expiresAt! * 1000to pass a millisecond timestamp into_CountdownWidgetmatches howOrder.createdAtis treated elsewhere (seconds → ms). That keeps the countdown aligned with Nostr/Mostro timestamps and the newOrder.fromEventparsing.One follow-up to consider in
_CountdownWidget(see below): for cases whereorderPayload.createdAtis null or 0, you may want the same fallback you use in_buildSellerAmount(session start time or now) instead of implicitly defaulting to the Unix epoch when derivingcreatedAt.
970-1000: Pending-state countdown now correctly uses event expiration; improvecreatedAtfallback and tidy the provider usageThe new pending branch in
_buildCountDownTime:
- Properly guards against missing/invalid
expiresAtTimestampand avoids showing a countdown in those cases.- Converts
expiresAtTimestamp(ms) andtradeState.order?.createdAt(seconds → ms) intoDateTimes and feeds them intoDynamicCountdownWidget, which aligns with the model changes inOrder.fromEvent.Two small improvements:
Safer
createdAtderivationCurrently:
final createdAt = DateTime.fromMillisecondsSinceEpoch( (tradeState.order?.createdAt ?? 0) * 1000, );If
createdAtis null or 0, this becomes1970-01-01, which is inconsistent with the rest of the screen where you fall back tosession?.startTime ?? DateTime.now(). Consider something along these lines inside_buildCountDownTime:final createdAtSeconds = tradeState.order?.createdAt; final createdAt = (createdAtSeconds != null && createdAtSeconds > 0) ? DateTime.fromMillisecondsSinceEpoch(createdAtSeconds * 1000) : DateTime.now();Or, if you’d like exact parity with
_buildSellerAmount, pass the already-chosen created date down into_CountdownWidgetinstead of recomputing here.Unused
currentTimefromcountdownTimeProviderIn
build, you’re usingcountdownTimeProviderpurely as a ticker, but the value itself (currentTime) is unused. To avoid analyzer warnings, you can mark it as unused:return timeAsync.when( data: (_) { // ... }, // ... );That keeps the intent (drive rebuilds) while keeping the code warning-free.
Overall, the migration to
DynamicCountdownWidgetforStatus.pendingis a nice step toward consistent countdown handling across screens.lib/services/fcm_service.dart (2)
139-171: Manage FCM stream subscriptions indisposeto avoid leaks/duplicates
_setupTokenRefreshListenerand_setupForegroundMessageHandlerattach listeners to long‑lived streams but theirStreamSubscriptions are not stored or cancelled indispose(). If the service/provider is ever recreated, this can lead to multiple listeners and prevent instances from being GC’d.Recommend storing both subscriptions in fields and cancelling them in
dispose():+ StreamSubscription<String>? _tokenRefreshSub; + StreamSubscription<RemoteMessage>? _foregroundMessageSub; ... void _setupTokenRefreshListener() { - _messaging.onTokenRefresh.listen( + _tokenRefreshSub = _messaging.onTokenRefresh.listen( (newToken) { _logger.i('FCM token refreshed: ${newToken.substring(0, 20)}...'); _saveToken(newToken); }, onError: (error) { _logger.e('Error in token refresh listener: $error'); }, ); } ... void _setupForegroundMessageHandler() { - FirebaseMessaging.onMessage.listen( + _foregroundMessageSub = FirebaseMessaging.onMessage.listen( (RemoteMessage message) { ... }, onError: (error) { _logger.e('Error in foreground message handler: $error'); }, ); } ... void dispose() { + _tokenRefreshSub?.cancel(); + _foregroundMessageSub?.cancel(); _isInitialized = false; }Also applies to: 209-212
153-170: Consider reducing/guarding foreground message logging for privacyLogging full
message.dataand notification title/body on every foreground message may expose user or trade details in production logs. Consider:
- Lowering log level for payloads (e.g.,
donly in debug builds), or- Logging only derived/high‑level info instead of raw data.
This keeps observability while reducing privacy and noise risks.
lib/shared/widgets/dynamic_countdown_widget.dart (1)
31-35: Confirm UX intent for hiding expirations older than 1 hourThe widget returns
SizedBox.shrink()whenexpirationis more than 1 hour in the past. That’s reasonable if PENDING orders should never legitimately be older than an hour; otherwise users might see no countdown with no indication that the order simply expired.If there are valid flows with >1h‑ago expirations, consider rendering an explicit “expired” state instead of hiding.
lib/features/notifications/services/background_notification_service.dart (1)
312-418:fetchAndProcessNewEventsflow is coherent; consider a couple of refinementsThe silent-push flow is well structured: load sessions → derive
sincefrom shared prefs → init Nostr client → fetch kind 1059 events per session (viaptag) → pass through existing notification pipeline → updatefcm.last_processed_timestamp. This should play nicely with the FCM topic‑based wake-up.A few non-blocking refinements to consider:
- Reuse session data in decryption:
_decryptAndProcessEventreloads sessions again to find a matching session by trade key. SincefetchAndProcessNewEventsalready hassessionin hand, you could extendshowLocalNotification/_decryptAndProcessEventto accept the knownSessionto avoid extra DB hits per event.- Last-processed timestamp semantics: on partial relay failures or transient errors, you update
fcm.last_processed_timestamptonowanyway. That can skip some events if they arrive only on a relay that was temporarily failing. If this is a concern, you might:
- Only advance the timestamp when at least one relay succeeded, or
- Track per-relay last-processed timestamps.
These are mostly resilience/perf tweaks; the current implementation is functionally consistent.
functions/src/index.ts (1)
223-269: GloballastCheckTimestampis simple but may miss events on relay failures
keepAliveuses a single in‑memorylastCheckTimestampshared across all relays and advances it tocheckStartTimeafter each run, even when some relays fail. That’s acceptable if:
- You rely on a single Mostro relay (current config), and
- Missing a small window of events in rare failure scenarios is an acceptable trade‑off.
If you plan to add more relays or want stronger guarantees, consider:
- Tracking per‑relay last timestamps, or
- Only advancing the global timestamp when all relays succeed (or at least one successfully reports events).
This is an enhancement, not a blocker, but worth deciding explicitly.
lib/features/order/screens/take_order_screen.dart (1)
467-487: LGTM - Robust countdown logic with proper validation.The implementation correctly:
- Validates
expiresAtandcreatedAtavailability- Handles dynamic types safely using
toString()+tryParse()(appropriate for Nostr event data)- Converts Unix timestamps (seconds) to DateTime (milliseconds)
- Gracefully handles invalid data by returning an empty widget
Consider adding validation to catch data inconsistencies during development:
Widget _buildCountDownTime( BuildContext context, WidgetRef ref, NostrEvent order) { if (order.expiresAt == null) { return const SizedBox.shrink(); } final expiresAtSeconds = int.tryParse(order.expiresAt.toString()); if (expiresAtSeconds == null || expiresAtSeconds <= 0) { return const SizedBox.shrink(); } final expiration = DateTime.fromMillisecondsSinceEpoch(expiresAtSeconds * 1000); final createdAt = order.createdAt; if (createdAt == null) { return const SizedBox.shrink(); } + + // Optional: Validate timestamp consistency + assert(expiration.isAfter(createdAt), + 'Expiration ($expiration) must be after creation ($createdAt)'); return DynamicCountdownWidget( expiration: expiration, createdAt: createdAt, ); }This assertion would help catch invalid Nostr event data during development without affecting production builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
functions/package-lock.jsonis excluded by!**/package-lock.jsonpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.firebaserc(1 hunks).gitignore(1 hunks)CLAUDE.md(2 hunks)PLAN_FCM.md(1 hunks)android/app/build.gradle(1 hunks)android/settings.gradle(1 hunks)docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md(4 hunks)firebase.json(1 hunks)functions/.eslintrc.js(1 hunks)functions/.gitignore(1 hunks)functions/package.json(1 hunks)functions/src/index.ts(1 hunks)functions/tsconfig.dev.json(1 hunks)functions/tsconfig.json(1 hunks)lib/data/models/nostr_event.dart(1 hunks)lib/data/models/order.dart(1 hunks)lib/features/notifications/services/background_notification_service.dart(5 hunks)lib/features/order/notfiers/abstract_mostro_notifier.dart(1 hunks)lib/features/order/screens/take_order_screen.dart(4 hunks)lib/features/trades/screens/trade_detail_screen.dart(2 hunks)lib/firebase_options.dart(1 hunks)lib/main.dart(1 hunks)lib/services/fcm_service.dart(1 hunks)lib/shared/providers/app_init_provider.dart(2 hunks)lib/shared/widgets/dynamic_countdown_widget.dart(1 hunks)pubspec.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-05-08T16:29:52.154Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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:
PLAN_FCM.mdlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/features/trades/screens/trade_detail_screen.dartlib/shared/providers/app_init_provider.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/features/trades/screens/trade_detail_screen.dartlib/shared/providers/app_init_provider.dartCLAUDE.mdlib/features/order/screens/take_order_screen.dartlib/features/notifications/services/background_notification_service.dartlib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/trades/screens/trade_detail_screen.dartlib/shared/providers/app_init_provider.dartlib/features/order/screens/take_order_screen.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-08-15T01:37:12.243Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.
Applied to files:
lib/features/trades/screens/trade_detail_screen.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/shared/providers/app_init_provider.dartdocs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.mdCLAUDE.mdlib/features/notifications/services/background_notification_service.dartlib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/shared/providers/app_init_provider.dart
📚 Learning: 2025-06-26T15:03:23.529Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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:
docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.mdCLAUDE.mdlib/features/notifications/services/background_notification_service.dartlib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-05-06T15:49:55.079Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:97-103
Timestamp: 2025-05-06T15:49:55.079Z
Learning: In the Mostro protocol, an order cannot be canceled unless it has a valid orderId, so null checks on orderId during cancel operations are unnecessary.
Applied to files:
docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md
📚 Learning: 2025-06-04T19:35:20.209Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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:
docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.mdlib/features/order/screens/take_order_screen.dart
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 272
File: lib/features/relays/relays_notifier.dart:41-49
Timestamp: 2025-08-19T17:54:15.016Z
Learning: In the Mostro mobile app relay synchronization system, the hardcoded 'wss://relay.mostro.network' relay in RelaysNotifier._loadRelays() is intentional for bootstrapping. Config.nostrRelays only contains this single active relay anyway (other entries are commented-out dev relays), so hardcoding is functionally equivalent and more explicit about the bootstrap requirement for fetching kind 10002 relay list events.
Applied to files:
functions/src/index.ts
📚 Learning: 2025-05-06T15:46:08.942Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/nostr_service.dart:3-4
Timestamp: 2025-05-06T15:46:08.942Z
Learning: The file in dart_nostr library is named "ease.dart" (not "eose.dart" as might be expected), as confirmed by the project maintainer.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/notifications/services/background_notification_service.dartlib/features/order/notfiers/abstract_mostro_notifier.dart
🪛 Gitleaks (8.29.0)
lib/firebase_options.dart
[high] 44-44: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
[high] 53-53: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
[high] 61-61: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
[high] 70-70: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
[high] 79-79: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
🪛 LanguageTool
PLAN_FCM.md
[grammar] ~1-~1: Corrige la mayúscula.
Context: Plan de Implementación: Firebase Cloud Messaging (FCM) Objetivo...
(QB_NEW_ES_OTHER_ERROR_IDS_REPLACEMENT_ORTHOGRAPHY_UPPERCASE)
[grammar] ~4-~4: Corrige la mayúscula.
Context: ...uridad del sistema actual. Arquitectura Propuesta 1. Modelo de Seguridad FCM Payload: Sol...
(QB_NEW_ES_OTHER_ERROR_IDS_REPLACEMENT_ORTHOGRAPHY_UPPERCASE)
[grammar] ~5-~5: Corrige la mayúscula.
Context: ...al. Arquitectura Propuesta 1. Modelo de Seguridad FCM Payload: Solo event_id + recipient_...
(QB_NEW_ES_OTHER_ERROR_IDS_REPLACEMENT_ORTHOGRAPHY_UPPERCASE)
[grammar] ~7-~7: Agrega un signo de puntuación.
Context: ...mpre en el dispositivo usando trade keys Privacy: Nunca enviar contenido sensible...
(QB_NEW_ES_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~48-~48: Agrega un signo de puntuación.
Context: ...izerProvider: Agregar inicialización de FCM Solicitar permisos de notificación Obte...
(QB_NEW_ES_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~49-~49: Agrega un signo de puntuación.
Context: ...ialización de FCM Solicitar permisos de notificación Obtener token inicial Modificar Backgro...
(QB_NEW_ES_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~50-~50: Agrega un signo de puntuación.
Context: ... permisos de notificación Obtener token inicial Modificar BackgroundNotificationService...
(QB_NEW_ES_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~101-~101: Cambia la palabra o signo.
Context: ...? Si no existe, habría que implementarlo ¿El backend Mostro está bajo tu control ...
(QB_NEW_ES_OTHER_ERROR_IDS_REPLACEMENT_OTHER)
[grammar] ~103-~103: Elimina el articulo
Context: ... envíe a un backend tuyo o directamente al Mostro instance? ¿Quieres implementar F...
(QB_NEW_ES_OTHER_ERROR_IDS_UNNECESSARY_DETERMINER)
[grammar] ~104-~104: Agrega un signo de puntuación.
Context: ...lytics/Crashlytics también, o solo FCM? Estimación Con backend existente: 2-3 días de desa...
(QB_NEW_ES_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~106-~106: Aquí puede haber un error.
Context: ...d existente: 2-3 días de desarrollo + 1 día testing Sin backend: +3-5 días para bac...
(QB_NEW_ES)
[grammar] ~106-~106: Se encontraron 2 posibles errores
Context: ...istente: 2-3 días de desarrollo + 1 día testing Sin backend: +3-5 días para backend FCM...
(QB_NEW_ES_OTHER_ERROR_IDS_MERGED_MATCHES___MISSING_ADPOSITION___MISSING_PUNCTUATION)
[grammar] ~107-~107: Agrega un signo de puntuación.
Context: ...ckend: +3-5 días para backend FCM relay listener Complejidad: Media-Alta (manejo de isol...
(QB_NEW_ES_OTHER_ERROR_IDS_MISSING_PUNCTUATION)
[grammar] ~108-~108: Corrige el error ortográfico.
Context: ...d FCM relay listener Complejidad: Media-Alta (manejo de isolates, crypto en backgrou...
(QB_NEW_ES_OTHER_ERROR_IDS_REPLACEMENT_ORTHOGRAPHY_OTHERCASE)
🔇 Additional comments (20)
.firebaserc (1)
1-5: Default Firebase project is test-only; ensure prod switch before releaseUsing
mostro-testas the default project is fine for development, but it’s easy to forget when cutting a production build. Consider documenting or scripting the prod.firebaserc(or using separate configs per env) so release builds can’t accidentally point at the test project.pubspec.yaml (1)
19-19: Version bump and Firebase deps look coherent; verify FlutterFire compatibility
version: 1.0.4is a reasonable bump for adding FCM + background changes, and addingfirebase_core/firebase_messaginghere matches the rest of the PR. Please double‑check that:
- These versions are compatible with your current Flutter SDK (3.5.x) and Android/iOS Gradle/Xcode stack.
firebase_options.dart/ platform configs were generated with the same Firebase project and plugin versions.Running
flutter pub outdatedand checking the FlutterFire docs against these versions before merging would help avoid subtle build/runtime issues.Also applies to: 87-90
.gitignore (1)
74-78: Cloud Functions ignore patterns are appropriateIgnoring
functions/node_modules/, compiledfunctions/lib/, and.runtimeconfig.jsonis standard for Firebase Functions and avoids noisy or sensitive artifacts in Git. This overlaps slightly withfunctions/.gitignore, but the redundancy is harmless and helps when working only from the repo root.functions/.gitignore (1)
1-10: Local functions ignores are sensible and aligned with TS/Node workflowIgnoring compiled JS (
lib/**/*.js*), typings,node_modules/, and*.localmatches a standard TypeScript Firebase Functions setup and keeps only source + config in version control.CLAUDE.md (2)
128-135: LGTM! Method references updated correctly.The documentation now properly references the explicit method names (
startSessionTimeoutCleanup()andstartSessionTimeoutCleanupForRequestId()) instead of line ranges, improving clarity and maintainability.
248-259: LGTM! Comprehensive Dynamic Countdown Widget documentation.The new documentation clearly explains the DynamicCountdownWidget system with its automatic day/hour scaling, data sources, and integration points. This will help developers understand the countdown timer architecture.
docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md (1)
309-378: LGTM! Excellent architectural documentation for DynamicCountdownWidget.The new Dynamic Countdown Timer System section provides comprehensive coverage of the widget architecture, including:
- Clear explanation of automatic scaling between day/hour formats
- Code examples showing the implementation
- Integration points for both TakeOrderScreen and TradeDetailScreen
- Scope and limitations clearly defined
This documentation will be valuable for understanding the countdown timer system.
functions/tsconfig.json (1)
1-17: LGTM! Solid TypeScript configuration for Firebase Functions.The configuration uses appropriate settings for Firebase Functions:
- NodeNext modules align with Node 22 engine specified in package.json
- Strict mode and lint-friendly options (noImplicitReturns, noUnusedLocals) ensure code quality
- Output to
libdirectory matches the main entry point in package.jsonfunctions/package.json (2)
17-21: I need to verify if these versions are current. Let me check the latest available versions:Dependencies are secure and current.
The verification confirms all dependencies are free from known security vulnerabilities:
- firebase-admin latest version is 13.6.0, while the codebase uses 12.6.0. No known security advisories exist for firebase-admin.
- firebase-functions latest version is 7.0.0, while the codebase uses 6.0.1. No known security advisories exist for firebase-functions. The version choice may be intentional for compatibility considerations.
- ws latest version is 8.18.3, and the codebase specifies ^8.18.3, which is the latest available. The dependency is protected against the known HIGH severity DoS vulnerability affecting earlier versions.
All three dependencies are secure and appropriate for use.
13-15: Node.js 22 is officially supported for these Firebase Functions.Firebase Cloud Functions (v2) supports Node.js 22, and the codebase is confirmed to use Gen2 functions based on the
firebase-functions/v2/*imports infunctions/src/index.tsand thedisallowLegacyRuntimeConfig: truesetting infirebase.json. The engine specification is correct and requires no changes.android/app/build.gradle (1)
1-9: Google Services plugin integration is wired correctlyAdding
id 'com.google.gms.google-services'in thepluginsblock aftercom.android.applicationis the expected setup for Firebase/FCM. This should work fine together with the corresponding plugin declaration insettings.gradleand thegoogle-services.jsonconfigured viafirebase.json.Please just verify on CI/device that the Gradle sync picks up the generated
android/app/google-services.jsonwithout additionalapply pluginstatements at the bottom of the file.firebase.json (1)
1-39: Firebase CLI config is consistent with the test project / Functions setupThe
fluttersection correctly wires the Androidgoogle-services.jsonoutput andlib/firebase_options.dartplatform mappings to themostro-testproject. Thefunctionsblock withcodebase: "default"and the ignore patterns is standard for a single Cloud Functions codebase living underfunctions/.Since this is pointed at the test project, ensure that:
.firebasercclearly separates dev/test vs future prod projects, and- your release pipeline explicitly targets the prod project before shipping to users.
Functionally, this file is good to go for the current test environment.
lib/main.dart (1)
76-86: Firebase init ordering and background handler registration inmain()look goodCalling
WidgetsFlutterBinding.ensureInitialized();first, thenawait Firebase.initializeApp(...), and only then registeringFirebaseMessaging.onBackgroundMessage(firebaseMessagingBackgroundHandler);is a clean startup sequence. This should keep FCM happy across cold starts and app restarts.lib/features/trades/screens/trade_detail_screen.dart (1)
26-27: Dynamic countdown import is consistent with new pending-order UXThe new
dynamic_countdown_widget.dartimport fits with the pending-status refactor below; no issues here.lib/services/fcm_service.dart (1)
25-65: Initialization flow and failure handling look solidThe init sequence (permissions → token fetch/persist → topic subscription → listeners) is consistent and fails gracefully without breaking app startup. Good use of timeouts and non‑fatal logging around FCM initialization.
lib/features/notifications/services/background_notification_service.dart (1)
420-508: LegacyprocessFCMBackgroundNotificationis safely deprecated and reuses new pipelineThe legacy background handler is clearly marked
@Deprecatedand still routes fetched events through the centralshowLocalNotificationpath, so it benefits from the new dedup logic without diverging behavior.Once all clients are migrated to the silent-push path, you can consider:
- Removing this function and its calling wiring, and
- Cleaning up any now-unused FCM payload fields (
eventId,recipientPubkey) on the backend.functions/src/index.ts (1)
54-142:pollRelayWebSocket lifecycle and timeout handling look robustThe relay polling logic opens a connection, sends a scoped REQ, counts kind‑1059 events, and resolves on EOSE or a 30‑second timeout while ensuring the socket is closed and timeouts are cleared. Using a fixed
SUBSCRIPTION_IDper connection is fine here since each WebSocket is isolated and you filter on that ID in the EVENT handler.No changes needed from a correctness perspective.
lib/features/order/screens/take_order_screen.dart (3)
21-21: LGTM - Centralized countdown widget import.The import of
DynamicCountdownWidgetaligns with the PR's goal to centralize countdown rendering across multiple screens.
89-90: LGTM - Widget instantiation updated correctly.The parameter change from
expirationDate: order.expirationDatetoorder: orderis consistent with the widget's new API that extracts expiration directly from the NostrEvent.
445-448: LGTM - Widget API refactored to accept full order object.The change from accepting
DateTime expirationDatetoNostrEvent orderprovides access to bothexpiresAtandcreatedAttimestamps, enabling the newDynamicCountdownWidgetto render accurate countdowns.
lib/features/notifications/services/background_notification_service.dart
Outdated
Show resolved
Hide resolved
lib/features/notifications/services/background_notification_service.dart
Show resolved
Hide resolved
|
Hi @AndreaDiazCorreia I'm not being able to compile it, here the log: |
…nd error handling
…cation with EventStorage
|
@grunch @Catrya @BraCR10 en debug las notificaciones background me andaban bien pero en release me falla (capaz pise algo y no me di cuenta), si pueden prueben y me dicen. Para probar la implementacion de FCM es con la app en release porque tienen que poder cerrarla por completo y que FCM cuando el backend detecte algun evento la despierte. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
functions/src/index.ts (2)
14-14: Global state is not persisted across Cloud Function instances.Cloud Functions are stateless—each invocation may run on a different instance, and instances can be cold-started at any time. The global
lastCheckTimestampwill reset toDate.now()on each cold start, potentially causing missed events or duplicate processing.Persist
lastCheckTimestampin Firestore:-let lastCheckTimestamp = Math.floor(Date.now() / 1000); +const stateRef = admin.firestore().collection("config").doc("pollerState"); + +async function getLastCheckTimestamp(): Promise<number> { + const doc = await stateRef.get(); + return doc.exists ? (doc.data()?.lastCheckTimestamp ?? Math.floor(Date.now() / 1000)) : Math.floor(Date.now() / 1000); +} + +async function setLastCheckTimestamp(ts: number): Promise<void> { + await stateRef.set({ lastCheckTimestamp: ts }, { merge: true }); +}Then update
keepAliveto use these functions instead of the global variable.
140-160: HTTP endpoints lack authentication.
sendTestNotificationandgetStatusare publicly accessible. WhilegetStatusonly exposes configuration,sendTestNotificationcould be abused to spam all subscribed devices.Consider removing these before production or adding authentication:
-export const sendTestNotification = onRequest(async (_req, res) => { +export const sendTestNotification = onRequest(async (req, res) => { + const authHeader = req.headers.authorization; + if (!authHeader || authHeader !== `Bearer ${process.env.ADMIN_SECRET}`) { + res.status(401).json({ success: false, error: "Unauthorized" }); + return; + }lib/main.dart (1)
96-125: Missingreload()and premature flag clearing.Same issues as in
lifecycle_manager.dart:
- Line 104 reads SharedPreferences without
reload()- may get stale values from before background isolate wrote.- Line 109 clears the flag before processing completes - if
fetchAndProcessNewEventsfails, events are lost.Future<void> _checkPendingEventsOnResume( SharedPreferencesAsync sharedPrefs, SettingsNotifier settings, ) async { final logger = Logger(); try { + await sharedPrefs.reload(); final hasPending = await sharedPrefs.getBool('fcm.pending_fetch') ?? false; if (hasPending) { logger.i('Pending events detected - processing now'); - await sharedPrefs.setBool('fcm.pending_fetch', false); - final relays = settings.settings.relays; if (relays.isEmpty) { logger.w('No relays configured - cannot fetch events'); + await sharedPrefs.setBool('fcm.pending_fetch', false); return; } logger.i('Fetching new events from ${relays.length} relays'); await fetchAndProcessNewEvents(relays: relays); + await sharedPrefs.setBool('fcm.pending_fetch', false); logger.i('Successfully processed pending events'); } } catch (e, stackTrace) { logger.e('Error processing pending events: $e'); logger.e('Stack trace: $stackTrace'); } }lib/features/notifications/services/background_notification_service.dart (1)
111-113:retryNotificationstill never retries becauseshowLocalNotificationswallows errors
retryNotificationrelies onshowLocalNotificationthrowing on failure, butshowLocalNotificationcatches all exceptions and just logs them. As a result:
await showLocalNotification(event);almost never throws.successis set totrueon the first loop.- The backoff/retry path is effectively dead code.
This matches the earlier review concern and remains unresolved.
A minimal fix is to let
showLocalNotificationrethrow after logging, soretryNotificationcan actually handle failures:Future<void> showLocalNotification(NostrEvent event) async { try { ... - Logger().i('Shown: ${notificationText.title} - ${notificationText.body}'); - } catch (e) { - Logger().e('Notification error: $e'); - } + Logger().i('Shown: ${notificationText.title} - ${notificationText.body}'); + } catch (e, stackTrace) { + Logger().e('Notification error: $e', e, stackTrace); + rethrow; + } }With this change,
retryNotification’s loop and exponential backoff will actually trigger on real failures, both here and at other call sites that wrapshowLocalNotificationin try/catch.Also applies to: 285-305
🧹 Nitpick comments (5)
lib/shared/providers/app_init_provider.dart (1)
27-37: Callback logs warning but doesn't use the same logger instance.The
onMessageReceivedcallback accessesloggerfrom the outer scope, which is correct. However, ifrelays.isEmpty, the callback returns early without any subsequent action, which is appropriate for the "no relays" scenario.Consider whether an empty relays list is a recoverable state or if it warrants notifying the user.
lib/main.dart (1)
86-86: Fire-and-forget async call may silently fail.
_checkPendingEventsOnResumeis called withoutawait, so any errors are caught internally but the main thread won't know if processing succeeded. This is acceptable since:
- The function has its own try/catch
- Blocking
runAppfor event processing could delay UIHowever, consider using
unawaited()to signal this is intentional and avoid linter warnings:+import 'dart:async'; ... - _checkPendingEventsOnResume(sharedPreferences, settings); + unawaited(_checkPendingEventsOnResume(sharedPreferences, settings));lib/services/fcm_service.dart (1)
106-114: Inconsistency in SharedPreferences usage exists but with less urgency than stated.The codebase does mix
SharedPreferences.getInstance()(synchronous cached API) andSharedPreferencesAsync()(asynchronous I/O API), which Flutter documentation advises against due to potential cache inconsistencies across isolates. However, the specific concern in this comment—that a background handler writes withSharedPreferencesAsyncwhilefcm_servicereads withSharedPreferenceson the same keys—is not accurate:
fcm_service.dartaccesses the'fcm_token'keybackground_notification_service.dartaccesses'fcm.last_processed_timestamp', not'fcm_token'- The FCM background handler (
firebaseMessagingBackgroundHandlerinmain.dart) does not directly access either keyThat said, using
SharedPreferencesAsyncconsistently throughout the codebase (as recommended by Flutter best practices) remains a valid refactoring to avoid subtle bugs and ensure consistency. This is best practice advice rather than a bug fix.lib/features/notifications/services/background_notification_service.dart (2)
143-145: Guardsubstring(0, 16)to avoid potentialRangeErrorToday
recipient/recipientPubkeyare expected to be full public keys, but if malformed/short values ever slip through,substring(0, 16)will throw.You can make the logging safer with a small guard:
- if (matchingSession == null) { - Logger().d('No matching session found for recipient: ${recipient.substring(0, 16)}...'); - return null; - } + if (matchingSession == null) { + final preview = recipient.length > 16 ? recipient.substring(0, 16) : recipient; + Logger().d('No matching session found for recipient: $preview...'); + return null; + }- if (matchingSession == null) { - logger.w('No matching session found for recipient: ${recipientPubkey.substring(0, 16)}...'); - return; - } + if (matchingSession == null) { + final preview = recipientPubkey.length > 16 + ? recipientPubkey.substring(0, 16) + : recipientPubkey; + logger.w('No matching session found for recipient: $preview...'); + return; + }Also applies to: 409-411
307-389: Consider basinglast_processed_timestampon max eventcreatedAt, notnowUsing
nowforfcm.last_processed_timestampcan skip events whosecreatedAtis earlier thannowbut which are first seen later (e.g., when adding a new relay). Persisting the maximumcreatedAtamong processed events is a safer watermark.One possible refinement:
- int processedCount = 0; - final now = DateTime.now(); + int processedCount = 0; + final now = DateTime.now(); + DateTime latestCreatedAt = since; @@ - for (final event in events) { + for (final event in events) { final nostrEvent = NostrEvent( id: event.id, kind: event.kind, content: event.content, tags: event.tags, createdAt: event.createdAt, @@ - await showLocalNotification(nostrEvent); - processedCount++; + await showLocalNotification(nostrEvent); + processedCount++; + + if (event.createdAt != null && event.createdAt!.isAfter(latestCreatedAt)) { + latestCreatedAt = event.createdAt!; + } } @@ - final newTimestamp = (now.millisecondsSinceEpoch / 1000).floor(); + final cutoff = processedCount > 0 ? latestCreatedAt : now; + final newTimestamp = (cutoff.millisecondsSinceEpoch / 1000).floor();This keeps behavior the same when no events are processed, but advances the watermark only up to the latest event timestamp when new events were actually handled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.fvmrc(1 hunks).gitignore(2 hunks)functions/src/index.ts(1 hunks)lib/background/background.dart(1 hunks)lib/features/notifications/services/background_notification_service.dart(4 hunks)lib/firebase_options.dart(1 hunks)lib/main.dart(3 hunks)lib/services/fcm_service.dart(1 hunks)lib/services/lifecycle_manager.dart(5 hunks)lib/shared/providers/app_init_provider.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/firebase_options.dart
- lib/background/background.dart
- .gitignore
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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: Catrya
Repo: MostroP2P/mobile PR: 272
File: lib/features/relays/relays_notifier.dart:41-49
Timestamp: 2025-08-19T17:54:15.016Z
Learning: In the Mostro mobile app relay synchronization system, the hardcoded 'wss://relay.mostro.network' relay in RelaysNotifier._loadRelays() is intentional for bootstrapping. Config.nostrRelays only contains this single active relay anyway (other entries are commented-out dev relays), so hardcoding is functionally equivalent and more explicit about the bootstrap requirement for fetching kind 10002 relay list events.
📚 Learning: 2025-08-15T01:37:12.243Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.
Applied to files:
.fvmrclib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/shared/providers/app_init_provider.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/shared/providers/app_init_provider.dartlib/main.dartlib/features/notifications/services/background_notification_service.dartlib/services/lifecycle_manager.dart
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 272
File: lib/features/relays/relays_notifier.dart:41-49
Timestamp: 2025-08-19T17:54:15.016Z
Learning: In the Mostro mobile app relay synchronization system, the hardcoded 'wss://relay.mostro.network' relay in RelaysNotifier._loadRelays() is intentional for bootstrapping. Config.nostrRelays only contains this single active relay anyway (other entries are commented-out dev relays), so hardcoding is functionally equivalent and more explicit about the bootstrap requirement for fetching kind 10002 relay list events.
Applied to files:
functions/src/index.ts
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/main.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/main.dartlib/features/notifications/services/background_notification_service.dartlib/services/lifecycle_manager.dart
📚 Learning: 2025-05-08T16:29:52.154Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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.dartlib/services/lifecycle_manager.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/features/notifications/services/background_notification_service.dartlib/services/lifecycle_manager.dart
📚 Learning: 2025-05-08T16:31:29.582Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-06-04T19:35:20.209Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/services/lifecycle_manager.dart
📚 Learning: 2025-06-26T15:03:23.529Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/services/lifecycle_manager.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 (10)
.fvmrc (1)
1-3: Flutter version pin in .fvmrc is consistent with the project baselinePinning Flutter to
3.35.7via FVM is fine and remains above your documented>=3.27.0requirement; just ensure this stays in sync with the Flutter constraint inpubspec.yamland any CI/build images so all environments use the same SDK version. Based on learnings, this matches your expected minimum.functions/src/index.ts (2)
101-127: APNs configuration now follows best practices.The silent push configuration correctly uses
apns-priority: "5",apns-push-type: "background", andcontentAvailable: truewithout alert/badge/sound keys. This aligns with Apple's requirements for background pushes.
162-204: Scheduled function logic is sound.The
keepAlivefunction correctly:
- Uses
Promise.allSettledto handle partial relay failures gracefully- Only sends notifications when new events are found
- Logs comprehensively for debugging
However,
lastCheckTimestamp = checkStartTimeat line 200 writes to the ephemeral global variable. This should write to Firestore once persistence is implemented.lib/shared/providers/app_init_provider.dart (1)
24-41: Good error handling for FCM initialization.The try/catch wrapper correctly prevents FCM failures from blocking app startup, addressing the previous concern. The error is logged with full stack trace for debugging.
lib/services/lifecycle_manager.dart (1)
56-56: Good placement of pending events check.Calling
_checkPendingFCMEvents()early in_switchToForeground()ensures background-flagged events are processed before other foreground services reinitialize. This prevents race conditions with subscription managers.lib/main.dart (1)
22-43: Background handler correctly simplified to minimal work.The handler now only:
- Initializes Firebase (required for background isolate)
- Sets a flag in SharedPreferences
- Records timestamp
This addresses the previous concern about heavy plugin usage in background isolates. Heavy lifting is deferred to foreground/lifecycle handlers.
lib/services/fcm_service.dart (2)
20-62: Well-structured initialization with proper idempotency and error handling.The
initialize()method:
- Checks
_isInitializedto prevent duplicate initialization- Follows a clear sequence: permissions → token → topic → listeners
- Logs comprehensively at each step
- Uses timeouts for network operations
- Rethrows errors to let callers decide how to handle failures
174-195: Good defensive error handling in deleteToken().The nested try/catch ensures local storage cleanup even if the remote token deletion fails. This prevents orphaned local tokens.
lib/features/notifications/services/background_notification_service.dart (2)
121-136: Recipient extraction and session matching logic look solidThe new flow for resolving the recipient (direct field +
ptag for kind 1059), validating it, and then matching sessions bytradeKey.publicis coherent and should correctly route gift‑wrapped events to the right session. Early returns on missing recipient or session keep decryption costs down.Also applies to: 139-139
391-471: DeprecatedprocessFCMBackgroundNotificationpath is clear and consistentThe legacy FCM background path correctly:
- Resolves the session by
recipientPubkey,- Initializes
NostrServicewith the provided relays,- Fetches the specific 1059 event by ID, and
- Reuses
showLocalNotificationto display it.Given it’s marked deprecated in favor of
fetchAndProcessNewEvents, keeping this as a thin compatibility wrapper is reasonable.
lib/features/notifications/services/background_notification_service.dart
Outdated
Show resolved
Hide resolved
| Future<void> _checkPendingFCMEvents() async { | ||
| try { | ||
| final sharedPrefs = SharedPreferencesAsync(); | ||
| final hasPending = await sharedPrefs.getBool('fcm.pending_fetch') ?? false; | ||
|
|
||
| if (hasPending) { | ||
| _logger.i('Pending FCM events detected - processing now'); | ||
|
|
||
| await sharedPrefs.setBool('fcm.pending_fetch', false); | ||
|
|
||
| final settings = ref.read(settingsProvider); | ||
| final relays = settings.relays; | ||
|
|
||
| if (relays.isEmpty) { | ||
| _logger.w('No relays configured - cannot fetch events'); | ||
| return; | ||
| } | ||
|
|
||
| _logger.i('Fetching new events from ${relays.length} relays'); | ||
| await fetchAndProcessNewEvents(relays: relays); | ||
| _logger.i('Successfully processed pending FCM events'); | ||
| } | ||
| } catch (e, stackTrace) { | ||
| _logger.e('Error processing pending FCM events: $e'); | ||
| _logger.e('Stack trace: $stackTrace'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing reload() call and premature flag clearing.
Two issues in _checkPendingFCMEvents:
-
Missing reload(): When the background isolate writes to SharedPreferences, the foreground may have cached stale values. Call
reload()before reading. -
Premature flag clearing: The flag is cleared at line 96 before
fetchAndProcessNewEventsexecutes. If processing fails, events won't be retried.
Future<void> _checkPendingFCMEvents() async {
try {
final sharedPrefs = SharedPreferencesAsync();
+ await sharedPrefs.reload();
final hasPending = await sharedPrefs.getBool('fcm.pending_fetch') ?? false;
if (hasPending) {
_logger.i('Pending FCM events detected - processing now');
- await sharedPrefs.setBool('fcm.pending_fetch', false);
-
final settings = ref.read(settingsProvider);
final relays = settings.relays;
if (relays.isEmpty) {
_logger.w('No relays configured - cannot fetch events');
+ await sharedPrefs.setBool('fcm.pending_fetch', false);
return;
}
_logger.i('Fetching new events from ${relays.length} relays');
await fetchAndProcessNewEvents(relays: relays);
+ await sharedPrefs.setBool('fcm.pending_fetch', false);
_logger.i('Successfully processed pending FCM events');
}
} catch (e, stackTrace) {
_logger.e('Error processing pending FCM events: $e');
_logger.e('Stack trace: $stackTrace');
+ // Flag remains set - will retry on next resume
}
}🤖 Prompt for AI Agents
In lib/services/lifecycle_manager.dart around lines 88 to 114, the method reads
a potentially stale SharedPreferences value and clears the pending flag too
early; to fix it, await sharedPrefs.reload() before reading
getBool('fcm.pending_fetch'), and move/perform the call to
sharedPrefs.setBool('fcm.pending_fetch', false) only after
fetchAndProcessNewEvents completes successfully (i.e., await
fetchAndProcessNewEvents first, then clear the flag), ensuring any exceptions
prevent the flag from being cleared so failed work will be retried.
…oderabbit suggest)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/features/notifications/services/background_notification_service.dart (1)
282-302:retryNotificationstill cannot effectively retry and is currently unused
retryNotificationwrapsshowLocalNotificationin a retry loop, butshowLocalNotificationcatches all exceptions and never rethrows (Lines 54–111), soawait showLocalNotification(event);will not throw here. As a result:
successis set totrueon the first iteration.- The catch block and exponential backoff never run.
- There are no call sites for
retryNotificationyet, so this remains effectively dead code.Either wire this up properly (by letting
showLocalNotificationrethrow after logging, so real failures trigger the retry), or removeretryNotificationuntil you have a concrete failure mode to handle.
🧹 Nitpick comments (4)
lib/services/fcm_service.dart (1)
33-67: Clarify whether FCM initialization failures should be fatalThe catch block logs that “App will continue without FCM push notifications” but then
rethrows (Line 65). If upstream code doesn’t catch this, transient FCM/network errors could break the app init flow rather than just disabling push.Consider either:
- Treating init failures as non-fatal by removing
rethrow;, or- Keeping the rethrow but updating call sites to explicitly handle FCM init failures, and adjusting the log message to reflect that behavior.
lib/features/notifications/services/background_notification_service.dart (3)
134-137: Avoid reloading sessions for each event in the FCM fetch path
_decryptAndProcessEventloads all sessions from the database and searches them for a matchingtradeKey.public(Lines 134–137, 161–173). InfetchAndProcessNewEvents, you already loadsessionsonce (Line 311) and then fetch events per-session (Lines 343–371), but eachshowLocalNotificationcall triggers another_loadSessionsFromDatabase().To reduce DB and key-manager overhead in the FCM polling path, consider:
- Passing the already-loaded
Session(or a map ofrecipientPubkey → Session) into_decryptAndProcessEvent, or- Adding a variant like
_decryptAndProcessEventForSession(event, session)used fromfetchAndProcessNewEvents, while keeping the current version for other call sites.This keeps the behavior the same but avoids repeated DB opens and key derivation in tight loops.
Also applies to: 161-173, 343-371
304-385: Timestamp update strategy infetchAndProcessNewEventsmay skip very old late-arriving events
fetchAndProcessNewEventsreadsfcm.last_processed_timestamp(seconds), builds aDateTime sincefrom it, and after processing all sessions sets the new timestamp tonow(Lines 320–323, 341, 377–379). This works for most flows, but has a subtle edge case:
- If an event is created far in the past (low
createdAt) but only becomes available after you’ve advanced the stored timestamp tonow, subsequent runs withsince = newTimestampwill never see it.A more robust pattern is to advance the checkpoint based on the latest processed event’s
createdAt, e.g.:
- Track
maxCreatedAtacross all fetched events (default to existingsinceif none).- Store
maxCreatedAt(ormaxCreatedAt + ε) instead ofnow.This keeps the behavior monotonic while reducing the risk of missing very old, late-arriving events.
4-4: Consider aliasingdart_nostrto avoid potential type-name collisionsThis file imports both
package:dart_nostr/dart_nostr.dart(Line 4) andpackage:mostro_mobile/data/models/nostr_event.dart(Line 13). Ifnostr_event.dartalso defines aNostrEventtype, you can end up with ambiguous references once you useNostrEventfrom both contexts (local model vs. library event), especially as you’re now directly constructingNostrFilterinstances (Lines 345–350, 430–433) that conceptually pair with the library’s event type.To keep things unambiguous and self-documenting, consider:
import 'package:dart_nostr/dart_nostr.dart' as nostr; // ... final filter = nostr.NostrFilter(...);and keep your domain model as
NostrEventfromnostr_event.dart. This makes it clear which types come from the protocol library and avoids future naming clashes as the file evolves.Also applies to: 345-350, 430-433
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/notifications/services/background_notification_service.dart(4 hunks)lib/services/fcm_service.dart(1 hunks)lib/services/lifecycle_manager.dart(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/services/lifecycle_manager.dart
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/services/fcm_service.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-08T16:29:52.154Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/notifications/services/background_notification_service.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:46:08.942Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/nostr_service.dart:3-4
Timestamp: 2025-05-06T15:46:08.942Z
Learning: The file in dart_nostr library is named "ease.dart" (not "eose.dart" as might be expected), as confirmed by the project maintainer.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-08-15T01:37:12.243Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-08T16:31:29.582Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
Applied to files:
lib/features/notifications/services/background_notification_service.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/services/fcm_service.dart (1)
18-22: Stream subscription lifecycle is now handled correctlyThe
_tokenRefreshSuband_foregroundMsgSubfields are created, any existing subscriptions are cancelled before reassigning in_setupTokenRefreshListenerand_setupForegroundMessageHandler, and both are cancelled and nulled indispose().This closes the earlier leak/duplicate-handler gap and makes
initialize()/dispose()idempotent from a subscription perspective.Also applies to: 135-173, 208-215
| // Extract recipient from event | ||
| String? recipient = event.recipient; | ||
|
|
||
| // For kind 1059 (gift-wrapped), recipient might be in 'p' tag | ||
| if ((recipient == null || recipient.isEmpty) && event.kind == 1059 && event.tags != null) { | ||
| final pTags = event.tags!.where((tag) => tag.isNotEmpty && tag[0] == 'p'); | ||
| if (pTags.isNotEmpty && pTags.first.length > 1) { | ||
| recipient = pTags.first[1]; | ||
| } | ||
| } | ||
|
|
||
| if (recipient == null || recipient.isEmpty) { | ||
| Logger().d('No recipient found for event ${event.id}'); | ||
| return null; | ||
| } | ||
|
|
||
| final sessions = await _loadSessionsFromDatabase(); | ||
| final matchingSession = sessions.cast<Session?>().firstWhere( | ||
| (s) => s?.tradeKey.public == event.recipient, | ||
| (s) => s?.tradeKey.public == recipient, | ||
| orElse: () => null, | ||
| ); | ||
|
|
||
| if (matchingSession == null) return null; | ||
| if (matchingSession == null) { | ||
| Logger().d('No matching session found for recipient: ${recipient.substring(0, 16)}...'); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Guard against substring(0, 16) RangeError when logging recipients
In both _decryptAndProcessEvent (Line 141) and processFCMBackgroundNotification (Line 407), substring(0, 16) is called on values derived from external data (recipient / recipientPubkey). If for any reason those strings are shorter than 16 characters, this will throw RangeError and break background processing.
A small defensive helper avoids that:
- if (matchingSession == null) {
- Logger().d('No matching session found for recipient: ${recipient.substring(0, 16)}...');
- return null;
- }
+ if (matchingSession == null) {
+ final preview = recipient.length > 16 ? recipient.substring(0, 16) : recipient;
+ Logger().d('No matching session found for recipient: $preview...');
+ return null;
+ }And similarly for recipientPubkey in processFCMBackgroundNotification:
- if (matchingSession == null) {
- logger.w('No matching session found for recipient: ${recipientPubkey.substring(0, 16)}...');
- return;
- }
+ if (matchingSession == null) {
+ final preview =
+ recipientPubkey.length > 16 ? recipientPubkey.substring(0, 16) : recipientPubkey;
+ logger.w('No matching session found for recipient: $preview...');
+ return;
+ }Also applies to: 406-408
🤖 Prompt for AI Agents
lib/features/notifications/services/background_notification_service.dart around
lines 118 to 143 — the logging uses recipient.substring(0, 16) which can throw a
RangeError if recipient is shorter than 16 chars; change the log call to safely
truncate by computing a safe length (min(16, recipient.length)) or use a helper
that returns the first N chars or the whole string when shorter, then log that
safe value; apply the same defensive truncation approach for recipientPubkey in
processFCMBackgroundNotification (around lines 406-408).
| { | ||
| "project_info": { | ||
| "project_number": "679654468306", | ||
| "project_id": "mostro-test", |
There was a problem hiding this comment.
can you remove the test from the name?
| "oauth_client": [], | ||
| "api_key": [ | ||
| { | ||
| "current_key": "AIzaSyBAwF5XEuNOC-TQqhLrFJQhNVzbFYSc1sI" |
There was a problem hiding this comment.
it is safe to publish this key?
|
|
||
| admin.initializeApp(); | ||
|
|
||
| const NOSTR_RELAYS = ["wss://relay.mostro.network"]; |
There was a problem hiding this comment.
we have this values on lib/core/config.dart, we should probably have them centralized somewhere
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
@AndreaDiazCorreia compilé la app en release y tambien en debug pero no me sale ninguna notificación.
Tampoco me funciona en linux, me da error de que firebase no se usa en linux, habria que deshabilitarlo para linux porque si no luego no vamos a poder probar desde ahi, aunque es más urgente que funione bien en android
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/features/notifications/services/background_notification_service.dart (3)
122-148: Recipient extraction logic looks good, but substring RangeError remains unaddressed.The new logic to extract recipient from
event.recipientor p-tags for kind 1059 events is well-structured. However, thesubstring(0, 16)call at line 146 can still throwRangeErrorifrecipientis shorter than 16 characters.Apply the fix from the previous review:
if (matchingSession == null) { - Logger().d('No matching session found for recipient: ${recipient.substring(0, 16)}...'); + final preview = recipient.length > 16 ? recipient.substring(0, 16) : recipient; + Logger().d('No matching session found for recipient: $preview...'); return null; }
398-478: Deprecated function has the same issues.
Substring RangeError at line 417: Same issue as flagged in past review—apply the defensive length check.
NostrService not disposed: Same leak concern as
fetchAndProcessNewEvents.Since this function is deprecated, consider whether it should be removed entirely rather than maintained with bug fixes. If keeping it for backwards compatibility:
if (matchingSession == null) { - logger.w('No matching session found for recipient: ${recipientPubkey.substring(0, 16)}...'); + final preview = recipientPubkey.length > 16 ? recipientPubkey.substring(0, 16) : recipientPubkey; + logger.w('No matching session found for recipient: $preview...'); return; }
291-312: Retry logic remains ineffective.As noted in the previous review,
showLocalNotificationcatches all exceptions internally and never rethrows, soretryNotificationalways succeeds on the first attempt. The exponential backoff logic is dead code.Either:
- Make
showLocalNotificationrethrow after logging, or- Remove the retry wrapper entirely since it provides no value
📜 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 (5)
.gitignore(2 hunks)android/app/build.gradle(1 hunks)lib/background/background.dart(1 hunks)lib/features/notifications/services/background_notification_service.dart(4 hunks)pubspec.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/build.gradle
- pubspec.yaml
- .gitignore
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/features/notifications/services/background_notification_service.dartlib/background/background.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/features/notifications/services/background_notification_service.dartlib/background/background.dart
🧠 Learnings (26)
📓 Common learnings
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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: Catrya
Repo: MostroP2P/mobile PR: 272
File: lib/features/relays/relays_notifier.dart:41-49
Timestamp: 2025-08-19T17:54:15.016Z
Learning: In the Mostro mobile app relay synchronization system, the hardcoded 'wss://relay.mostro.network' relay in RelaysNotifier._loadRelays() is intentional for bootstrapping. Config.nostrRelays only contains this single active relay anyway (other entries are commented-out dev relays), so hardcoding is functionally equivalent and more explicit about the bootstrap requirement for fetching kind 10002 relay list events.
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
lib/features/notifications/services/background_notification_service.dartlib/background/background.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/features/notifications/services/background_notification_service.dartlib/background/background.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/**/*.dart : Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Applied to files:
lib/features/notifications/services/background_notification_service.dartlib/background/background.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/features/notifications/services/background_notification_service.dartlib/background/background.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-08T16:29:52.154Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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.dartlib/background/background.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : Use SubscriptionManager with `fireImmediately: false` during SessionNotifier initialization to prevent premature execution
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/notifications/services/background_notification_service.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/features/notifications/services/background_notification_service.dartlib/background/background.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Persistence, APIs, and background jobs should live in `lib/data/` and `lib/background/`; generated localization output must be in `lib/generated/` and must stay untouched
Applied to files:
lib/features/notifications/services/background_notification_service.dartlib/background/background.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Application code should be organized under `lib/`, grouped by domain with `lib/features/<feature>/` structure, shared utilities in `lib/shared/`, dependency wiring in `lib/core/`, and services in `lib/services/`
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Resolve every analyzer warning in Dart code
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Remove unused imports and dependencies to maintain code cleanliness and reduce build size
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/core/app_routes.dart : Use GoRouter for all navigation configuration instead of Navigator API - maintain all routes in app_routes.dart
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Localize all user-facing strings via ARB files and access them with `S.of(context)` rather than hard-coded literals
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Use dual storage strategy: store Mostro/default relays in `settings.relays` and user relays in `settings.userRelays` with full JSON metadata via `toJson()`/`fromJson()`
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/settings/settings.dart : Use null-aware operators (`??`) in Settings `copyWith()` method to preserve existing values for selectedLanguage and defaultLightningAddress when not explicitly overridden
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Use `S.of(context)!.yourKey` for all user-facing strings instead of hardcoded text
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement two-tier relay validation: primary Nostr protocol test (REQ/EVENT/EOSE) with WebSocket fallback for connectivity testing
Applied to files:
lib/features/notifications/services/background_notification_service.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 (2)
lib/features/notifications/services/background_notification_service.dart (1)
4-4: LGTM on import changes.The import updates align with the new functionality:
dart_nostrforNostrFilter,Settingsfor relay configuration, andNostrServicefor fetching events.Also applies to: 22-22, 28-28
lib/background/background.dart (1)
16-21: Correct the documentation to accurately reflect the deduplication mechanism.The comment on line 19 claims the service "Uses separate notification_state store for deduplication," but verification shows the actual deduplication is done via
eventStore.hasItem()(line 78 in this file), not a separate store. There is nonotification_statestore in the codebase.Update the architecture comment to match the actual implementation:
- Deduplication uses
eventStore.hasItem()to check if events were already processed- The read-only boundary claim is correct—only
hasItem()is called onevents.dbLikely an incorrect or invalid review comment.
lib/features/notifications/services/background_notification_service.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/main.dart (1)
97-148: Pending events check only runs at startup, misses actual app resumes.The
_checkPendingEventsOnResumefunction at line 140 is called only once duringmain(). If the background handler sets thefcm.pending_fetchflag while the app is suspended (but not killed), there's no lifecycle observer to trigger the check when the app transitions from background to foreground.As suggested in past review comments, add a
WidgetsBindingObserverto detectAppLifecycleState.resumedand call the pending check, or integrate this into your existing app lifecycle management. This ensures pending events are processed whenever the user brings the app to the foreground, not just on cold starts.Based on past review comments.
♻️ Duplicate comments (1)
lib/main.dart (1)
150-190: Missingreload()and incorrect flag clearing order in pending events handler.Two issues remain unresolved despite past review comments:
Missing
reload()call: Line 165 reads from SharedPreferences without first callingreload(), which can return stale values if the background isolate recently wrote to the flag.Flag cleared before processing: Line 170 clears the
fcm.pending_fetchflag BEFORE callingfetchAndProcessNewEventsat line 180. A past review comment (marked as "addressed") recommended clearing the flag only AFTER successful processing to ensure failed processing can be retried. The current code will lose events if processing fails.Apply this fix:
try { + // Reload to get latest values from background isolate + await sharedPrefs.reload(); + final hasPending = await sharedPrefs.getBool('fcm.pending_fetch') ?? false; if (hasPending) { logger.i('Pending events detected (background processing failed) - processing now'); - await sharedPrefs.setBool('fcm.pending_fetch', false); - final relays = settings.settings.relays; if (relays.isEmpty) { logger.w('No relays configured - cannot fetch events'); + await sharedPrefs.setBool('fcm.pending_fetch', false); return; } logger.i('Fetching new events from ${relays.length} relays'); // Process without limits since app is now active await fetchAndProcessNewEvents( relays: relays, // No limits when app is active ); + + // Clear flag only after successful processing + await sharedPrefs.setBool('fcm.pending_fetch', false); logger.i('Successfully processed pending events'); } } catch (e, stackTrace) { logger.e('Error processing pending events: $e'); logger.e('Stack trace: $stackTrace'); + // Flag remains set for retry on next resume/FCM wake }Based on past review comments.
🧹 Nitpick comments (2)
docs/FCM_TESTING_GUIDE.md (1)
319-440: Well-defined success metrics and future planning.The success metrics section provides clear, measurable targets. The test results template will ensure consistent reporting across team members.
Static analysis correctly identifies missing language specifiers on several fenced code blocks (lines 39, 96, 125, 161, 198, 216, 223) and emphasis-as-heading usage (lines 268, 273, 278, 283, 302, 308, 313). These are markdown linting issues that don't affect functionality but could be cleaned up for better documentation hygiene:
-**Check 1: FCM Token Registered** +#### Check 1: FCM Token RegisteredAnd add language specifiers to code blocks:
-``` +```text [FCM] === FCM BACKGROUND WAKE START ===docs/FCM_IMPLEMENTATION.md (1)
212-382: Comprehensive implementation documentation with realistic limitations and security analysis.The document provides a complete picture of the FCM implementation including practical limitations (30s background timeout), monitoring guidance, and thoughtful security considerations (timing analysis, device fingerprinting risks).
Similar to the testing guide, static analysis flags markdown linting issues (missing language specifiers on fenced code blocks at lines 39, 96, 125, 161, 198, 216, 223 and emphasis-as-heading at lines 268, 273, 278, 283, 302, 308, 313). These are minor and don't affect functionality but could be cleaned up for documentation consistency.
📜 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 (6)
docs/FCM_IMPLEMENTATION.md(1 hunks)docs/FCM_TESTING_GUIDE.md(1 hunks)functions/src/index.ts(1 hunks)lib/features/notifications/services/background_notification_service.dart(4 hunks)lib/main.dart(3 hunks)lib/shared/providers/app_init_provider.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- functions/src/index.ts
- lib/features/notifications/services/background_notification_service.dart
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/shared/providers/app_init_provider.dartlib/main.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/shared/providers/app_init_provider.dartlib/main.dart
lib/shared/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Files:
lib/shared/providers/app_init_provider.dart
lib/main.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Configure timeago package locales in app initialization for proper relative time formatting (e.g., 'hace X horas' vs 'hours ago')
Files:
lib/main.dart
🧠 Learnings (14)
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Run `flutter test` before pushing code and add targeted test scenarios when expanding complex logic or async workflows
Applied to files:
lib/shared/providers/app_init_provider.dartdocs/FCM_TESTING_GUIDE.md
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : Use SubscriptionManager with `fireImmediately: false` during SessionNotifier initialization to prevent premature execution
Applied to files:
lib/shared/providers/app_init_provider.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
lib/shared/providers/app_init_provider.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/shared/providers/app_init_provider.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/shared/providers/app_init_provider.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Always check `mounted` before using BuildContext after async operations to prevent errors on disposed widgets
Applied to files:
lib/main.dart
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/main.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
lib/main.dart
📚 Learning: 2025-09-17T20:45:32.468Z
Learnt from: grunch
Repo: MostroP2P/mobile PR: 306
File: docs/architecture/REQUEST_ID_ANALYSIS.md:176-183
Timestamp: 2025-09-17T20:45:32.468Z
Learning: For PR #306 in MostroP2P/mobile repository: This is a documentation-only PR intended to explain how the current requestId system works, not to fix any bugs or issues in the code. The documentation should accurately reflect existing behavior.
Applied to files:
docs/FCM_IMPLEMENTATION.md
📚 Learning: 2025-09-17T20:45:07.179Z
Learnt from: grunch
Repo: MostroP2P/mobile PR: 306
File: docs/architecture/REQUEST_ID_ANALYSIS.md:114-118
Timestamp: 2025-09-17T20:45:07.179Z
Learning: For the MostroP2P/mobile project, grunch prefers to separate documentation PRs from code improvement PRs. Documentation PRs should focus on explaining how the current system works, while code fixes and improvements should be handled in separate PRs.
Applied to files:
docs/FCM_IMPLEMENTATION.md
📚 Learning: 2025-05-08T16:29:52.154Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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:
docs/FCM_IMPLEMENTATION.md
🪛 markdownlint-cli2 (0.18.1)
docs/FCM_TESTING_GUIDE.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
198-198: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
216-216: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
223-223: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
268-268: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
273-273: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
278-278: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
283-283: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
302-302: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
308-308: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
313-313: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/FCM_IMPLEMENTATION.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
198-198: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
216-216: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
223-223: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
268-268: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
273-273: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
278-278: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
283-283: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
302-302: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
308-308: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
313-313: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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 (6)
docs/FCM_TESTING_GUIDE.md (2)
1-136: Excellent testing documentation structure.The testing guide provides comprehensive, actionable test scenarios with clear success criteria and troubleshooting steps. The expected log formats align with the implementation patterns observed in the codebase.
207-318: Comprehensive debugging and troubleshooting guidance.The debugging commands and troubleshooting steps are practical and cover the major failure modes (battery optimization, background restrictions, connectivity issues).
lib/shared/providers/app_init_provider.dart (3)
1-22: Platform guard correctly excludes web and restricts to mobile platforms.The
_isFirebaseSupportedcheck properly gates Firebase initialization to Android and iOS, avoiding unsupported web platform calls.
32-60: FCM initialization with proper error handling and foreground message callback.The implementation addresses the previous concern about error handling by wrapping FCM initialization in try-catch. The foreground message handler correctly reads relay configuration, validates it, and processes events without background limits.
The
onMessageReceivedcallback is properly marked async and awaits the fetchAndProcessNewEvents call. The error handling logs failures without crashing app startup, allowing graceful degradation if FCM is unavailable.
62-87: Existing initialization flow preserved with added logging.The FCM integration is cleanly inserted into the initialization sequence without disrupting existing app startup logic. The completion log provides useful visibility.
docs/FCM_IMPLEMENTATION.md (1)
123-211: Accurate flow diagrams and configuration details.The flow diagrams correctly represent the three processing paths (background, foreground, fallback) implemented in the codebase. Configuration details are specific and actionable.
…notification service
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
lib/features/notifications/services/background_notification_service.dart (3)
145-148: Previous review comment still applies:substring(0, 16)can throw RangeError.The
recipient.substring(0, 16)at line 146 was flagged in a previous review. Ifrecipientis shorter than 16 characters, this throwsRangeError.Apply the fix from the previous review:
if (matchingSession == null) { - Logger().d('No matching session found for recipient: ${recipient.substring(0, 16)}...'); + final preview = recipient.length > 16 ? recipient.substring(0, 16) : recipient; + Logger().d('No matching session found for recipient: $preview...'); return null; }
291-312: Previous review comment still applies: retry logic is ineffective.As flagged previously,
showLocalNotificationcatches all exceptions internally (lines 110-112) and never rethrows. The try/catch at line 299 will never execute, making the exponential backoff logic dead code.
472-475: Previous review comment still applies:substring(0, 16)can throw RangeError.Same issue as line 146 -
recipientPubkey.substring(0, 16)will throw if the string is shorter than 16 characters.if (matchingSession == null) { - logger.w('No matching session found for recipient: ${recipientPubkey.substring(0, 16)}...'); + final preview = recipientPubkey.length > 16 ? recipientPubkey.substring(0, 16) : recipientPubkey; + logger.w('No matching session found for recipient: $preview...'); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/notifications/services/background_notification_service.dart(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/features/notifications/services/background_notification_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/features/notifications/services/background_notification_service.dart
🧠 Learnings (27)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/**/*.dart : Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : Use SubscriptionManager with `fireImmediately: false` during SessionNotifier initialization to prevent premature execution
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-08T16:29:52.154Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/notifications/services/background_notification_service.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Persistence, APIs, and background jobs should live in `lib/data/` and `lib/background/`; generated localization output must be in `lib/generated/` and must stay untouched
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Application code should be organized under `lib/`, grouped by domain with `lib/features/<feature>/` structure, shared utilities in `lib/shared/`, dependency wiring in `lib/core/`, and services in `lib/services/`
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Resolve every analyzer warning in Dart code
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Remove unused imports and dependencies to maintain code cleanliness and reduce build size
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement two-tier relay validation: primary Nostr protocol test (REQ/EVENT/EOSE) with WebSocket fallback for connectivity testing
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-06-26T15:03:23.529Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/core/app_routes.dart : Use GoRouter for all navigation configuration instead of Navigator API - maintain all routes in app_routes.dart
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Localize all user-facing strings via ARB files and access them with `S.of(context)` rather than hard-coded literals
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Use dual storage strategy: store Mostro/default relays in `settings.relays` and user relays in `settings.userRelays` with full JSON metadata via `toJson()`/`fromJson()`
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/settings/settings.dart : Use null-aware operators (`??`) in Settings `copyWith()` method to preserve existing values for selectedLanguage and defaultLightningAddress when not explicitly overridden
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Use `S.of(context)!.yourKey` for all user-facing strings instead of hardcoded text
Applied to files:
lib/features/notifications/services/background_notification_service.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 (2)
lib/features/notifications/services/background_notification_service.dart (2)
4-4: Import additions align with new functionality.The new imports for
dart_nostr,Settings, andNostrServicecorrectly support thefetchAndProcessNewEventsimplementation.Also applies to: 22-22, 28-28
361-446: Good improvements addressing past concerns; relay cleanup and timestamp tracking now correct.The implementation now properly:
- Cleans up
NostrServiceconnections in thefinallyblock (lines 442-446)- Tracks
latestEventTimestampbased on actual event timestamps rather than wall-clock time (line 400-403)- Applies per-session timeouts to prevent blocking (lines 378-384)
lib/features/notifications/services/background_notification_service.dart
Outdated
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: 2
♻️ Duplicate comments (3)
lib/features/notifications/services/background_notification_service.dart (2)
149-149: Guard againstRangeErrorwhen logging recipient substring.If
recipientis shorter than 16 characters,substring(0, 16)will throw aRangeError, potentially crashing background processing.Apply defensive truncation:
if (matchingSession == null) { - Logger().d('No matching session found for recipient: ${recipient.substring(0, 16)}...'); + final preview = recipient.length > 16 ? recipient.substring(0, 16) : recipient; + Logger().d('No matching session found for recipient: $preview...'); return null; }Based on past review comments.
470-470: Guard againstRangeErrorwhen logging recipientPubkey substring.Same issue as line 149: if
recipientPubkeyis shorter than 16 characters,substring(0, 16)will throw aRangeError.Apply defensive truncation:
if (matchingSession == null) { - logger.w('No matching session found for recipient: ${recipientPubkey.substring(0, 16)}...'); + final preview = recipientPubkey.length > 16 ? recipientPubkey.substring(0, 16) : recipientPubkey; + logger.w('No matching session found for recipient: $preview...'); return; }Based on past review comments.
lib/main.dart (1)
168-186: Flag cleared before processing—events lost on failure.The pending flag is cleared at line 173 before
fetchAndProcessNewEventsexecutes at line 183-186. If processing fails, the flag is already cleared and events won't be retried on the next resume.Apply this fix to clear the flag only after successful processing:
if (hasPending) { logger.i('Pending events detected (background processing failed) - processing now'); - await sharedPrefs.setBool('fcm.pending_fetch', false); - final relays = settings.settings.relays; if (relays.isEmpty) { logger.w('No relays configured - cannot fetch events'); + await sharedPrefs.setBool('fcm.pending_fetch', false); return; } logger.i('Fetching new events from ${relays.length} relays'); // Process without limits since app is now active await fetchAndProcessNewEvents( relays: relays, // No limits when app is active ); + + // Clear flag only after successful processing + await sharedPrefs.setBool('fcm.pending_fetch', false); logger.i('Successfully processed pending events'); }Note: The catch block at line 189 should NOT clear the flag so failed processing leaves it set for retry.
🧹 Nitpick comments (2)
docs/FCM_TESTING_GUIDE.md (2)
39-39: Add language specifiers to fenced code blocks for better readability.Several code blocks are missing language identifiers, which prevents syntax highlighting. Consider adding appropriate identifiers (e.g.,
```textor```log) to improve readability.Also applies to: 96-96, 125-125, 161-161, 198-198, 216-216, 223-223
268-268: Replace bold emphasis with proper Markdown headings.Using bold text (
**Check 1:**) instead of proper headings (#### Check 1:) reduces semantic structure and accessibility. Consider converting these to proper heading levels.Also applies to: 273-273, 278-278, 283-283, 302-302, 308-308, 313-313
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/FCM_TESTING_GUIDE.md(1 hunks)lib/features/notifications/services/background_notification_service.dart(7 hunks)lib/main.dart(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/main.dartlib/features/notifications/services/background_notification_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/main.dartlib/features/notifications/services/background_notification_service.dart
lib/main.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Configure timeago package locales in app initialization for proper relative time formatting (e.g., 'hace X horas' vs 'hours ago')
Files:
lib/main.dart
🧠 Learnings (34)
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/main.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/main.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : Use SubscriptionManager with `fireImmediately: false` during SessionNotifier initialization to prevent premature execution
Applied to files:
lib/main.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Always check `mounted` before using BuildContext after async operations to prevent errors on disposed widgets
Applied to files:
lib/main.dart
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/main.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Use dual storage strategy: store Mostro/default relays in `settings.relays` and user relays in `settings.userRelays` with full JSON metadata via `toJson()`/`fromJson()`
Applied to files:
lib/main.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/settings/settings.dart : Use null-aware operators (`??`) in Settings `copyWith()` method to preserve existing values for selectedLanguage and defaultLightningAddress when not explicitly overridden
Applied to files:
lib/main.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
lib/main.dartlib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/**/*.dart : Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-08T16:29:52.154Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Persistence, APIs, and background jobs should live in `lib/data/` and `lib/background/`; generated localization output must be in `lib/generated/` and must stay untouched
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Application code should be organized under `lib/`, grouped by domain with `lib/features/<feature>/` structure, shared utilities in `lib/shared/`, dependency wiring in `lib/core/`, and services in `lib/services/`
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Resolve every analyzer warning in Dart code
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to test/mocks.mocks.dart : Never manually modify `test/mocks.mocks.dart` - it already has file-level `// ignore_for_file: must_be_immutable` ignore directives
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Remove unused imports and dependencies to maintain code cleanliness and reduce build size
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement two-tier relay validation: primary Nostr protocol test (REQ/EVENT/EOSE) with WebSocket fallback for connectivity testing
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/orders/notifiers/add_order_notifier.dart : Start session timeout cleanup in `submitOrder()` method to prevent orphan sessions when Mostro doesn't respond within 10 seconds
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Use `startSessionTimeoutCleanupForRequestId()` for order creation timeout protection and cancel timer automatically when any Mostro response received
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/core/app_routes.dart : Use GoRouter for all navigation configuration instead of Navigator API - maintain all routes in app_routes.dart
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Localize all user-facing strings via ARB files and access them with `S.of(context)` rather than hard-coded literals
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:46:08.942Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/nostr_service.dart:3-4
Timestamp: 2025-05-06T15:46:08.942Z
Learning: The file in dart_nostr library is named "ease.dart" (not "eose.dart" as might be expected), as confirmed by the project maintainer.
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Use `S.of(context)!.yourKey` for all user-facing strings instead of hardcoded text
Applied to files:
lib/features/notifications/services/background_notification_service.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/notifications/services/background_notification_service.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Run `flutter test` before pushing code and add targeted test scenarios when expanding complex logic or async workflows
Applied to files:
docs/FCM_TESTING_GUIDE.md
🪛 markdownlint-cli2 (0.18.1)
docs/FCM_TESTING_GUIDE.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
198-198: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
216-216: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
223-223: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
268-268: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
273-273: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
278-278: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
283-283: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
302-302: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
308-308: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
313-313: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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 (6)
lib/main.dart (2)
25-29: LGTM!The platform check correctly gates Firebase initialization to supported platforms (Android/iOS), excluding web.
103-109: LGTM!Firebase initialization is correctly gated by platform support and the background message handler is registered following Flutter Firebase Messaging best practices.
lib/features/notifications/services/background_notification_service.dart (4)
3-3: LGTM!Import changes correctly add dependencies needed for the new relay-based event fetching functionality.
Also applies to: 21-21, 27-27
53-116: LGTM!The return type change from
Future<void>toFuture<bool>correctly enables callers to detect notification failures and implement retry logic. The function returnstrueon success andfalseon failure/skip conditions.
294-309: LGTM!The retry logic now works correctly with the updated
showLocalNotificationsignature returningbool. The exponential backoff and retry attempts are properly implemented.
311-449: Excellent implementation addressing all past review concerns.This new function resolves multiple issues flagged in previous reviews:
- NostrService cleanup: The
finallyblock at lines 439-443 ensures relay connections are properly disposed.- Timestamp race condition fixed: Lines 397-400 track the latest event timestamp per event, and lines 427-430 only update the stored timestamp after successful processing, preventing event loss.
- Accurate metrics: Using the
boolreturn fromshowLocalNotificationat lines 413-419 provides accurate success/failure counts.- Timeout protection: Per-session timeout at lines 375-381 prevents hanging on slow relays.
The implementation follows best practices with proper error handling, logging, and resource management.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/core/app.dart (1)
49-89: Defer clearingfcm.pending_fetchuntil after successful processing to preserve retries.Right now the flag is cleared before
fetchAndProcessNewEventsruns:if (hasPending) { logger.i('Pending events detected on resume - processing now'); await sharedPrefs.setBool('fcm.pending_fetch', false); final relays = settings.settings.relays; ... await fetchAndProcessNewEvents(...); logger.i('Successfully processed pending events on resume'); }If
fetchAndProcessNewEventsthrows (or the process is interrupted), the flag is already false and the app will not retry on future resumes, defeating this fallback path.Consider only clearing after a successful run and leaving it set on failure so the next resume can retry:
if (hasPending) { logger.i('Pending events detected on resume - processing now'); - - await sharedPrefs.setBool('fcm.pending_fetch', false); - final relays = settings.settings.relays; if (relays.isEmpty) { logger.w('No relays configured - cannot fetch events'); return; } logger.i('Fetching new events from ${relays.length} relays'); await fetchAndProcessNewEvents( relays: relays, // No limits when app is active ); logger.i('Successfully processed pending events on resume'); + await sharedPrefs.setBool('fcm.pending_fetch', false); }This keeps the retry signal intact if processing fails.
🧹 Nitpick comments (4)
docs/FCM_TESTING_GUIDE.md (2)
39-51: Add languages to fenced code blocks to satisfy markdownlint (MD040).Several code/log examples use bare ``` fences; markdownlint flags these. Consider specifying explicit languages (e.g.
bash, `json`, or `text`) for consistency and better rendering.Example adjustments:
-``` +[```text] [FCM] === FCM BACKGROUND WAKE START === ... -[FCM] === FCM BACKGROUND WAKE END === -``` +[FCM] === FCM BACKGROUND WAKE END === +```Apply similar changes to the other fenced blocks at the referenced lines.
Also applies to: 96-101, 125-129, 161-169, 198-205, 216-218, 223-225
268-281: Replace bold “Check N:” lines with headings to fix MD036.Lines like
**Check 1: FCM Token Registered**are used as headings and are flagged by MD036. Consider converting them to proper Markdown headings:-**Check 1: FCM Token Registered** +#### Check 1: FCM Token RegisteredRepeat for “Check 2/3” and the similar “Check” lines in the Events Not Fetched section.
Also applies to: 283-286, 302-316
lib/main.dart (2)
31-98: Validate background handler plugin usage and workload in FCM isolate.The background handler does full event processing (network + storage + notifications) via
fetchAndProcessNewEventsand writes SharedPreferences from the FCM isolate. This is powerful but also where subtle plugin issues tend to surface (MissingPluginException, OEM background limits, long-running work).Concrete suggestions:
- Explicitly verify on multiple devices (debug + release) that all plugins used transitively by
fetchAndProcessNewEvents(DB, notifications, crypto, etc.) are safe to call from the FCM background isolate and don’t throw in headless mode.- Keep the background work as short as feasible (your current
maxEventsPerSessionandtimeoutPerSessionare good guardrails; consider tightening or offloading heavy work to your existingBackgroundServicewhen possible).- Monitor logs around the
try { fetchAndProcessNewEvents(...) } catchblock in real-world tests to ensure you’re not hitting timeouts or plugin errors silently.If any plugin turns out not to be background-safe, you may need to reduce this handler to “set a pending flag + minimal bookkeeping” and let the foreground/lifecycle-resume path (_checkPendingEventsOnResume in
MostroApp) perform the heavy lifting instead.
51-60: ConfirmrelaysJSON shape matches Settings storage; consider reusing the Settings model.Here you deserialize relays directly from raw JSON:
final settingsJson = await sharedPrefs.getString('mostro_settings'); ... final settingsMap = jsonDecode(settingsJson) as Map<String, dynamic>; final relaysList = settingsMap['relays'] as List<dynamic>?; if (relaysList != null && relaysList.isNotEmpty) { relays = relaysList.cast<String>(); }Per existing guidelines,
settings.relaysis typically stored with full JSON metadata viatoJson()/fromJson(), not as bare strings. If the stored value is actually a list of relay objects (maps),relaysList.cast<String>()will fail at runtime and/or drop needed metadata. Based on learnings, it would be safer to either:
- Reuse the existing
Settings.fromJsonmodel to load settings and then passsettings.relaysintofetchAndProcessNewEvents, or- Explicitly map from the stored relay object structure to the URL type that
fetchAndProcessNewEventsexpects.Example sketch:
final settings = Settings.fromJson(settingsMap); final relays = settings.relays; // or settings.relays.map((r) => r.url).toList();Please double-check the actual JSON shape of
'mostro_settings'and adjust this parsing accordingly so it stays in sync with the main Settings model.Based on learnings, relays/userRelays are expected to be stored with full JSON metadata.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/FCM_TESTING_GUIDE.md(1 hunks)functions/src/index.ts(1 hunks)lib/core/app.dart(4 hunks)lib/main.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- functions/src/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/main.dartlib/core/app.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/main.dartlib/core/app.dart
lib/main.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Configure timeago package locales in app initialization for proper relative time formatting (e.g., 'hace X horas' vs 'hours ago')
Files:
lib/main.dart
🧠 Learnings (22)
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
lib/main.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/main.dartlib/core/app.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/main.dartlib/core/app.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : Use SubscriptionManager with `fireImmediately: false` during SessionNotifier initialization to prevent premature execution
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Always check `mounted` before using BuildContext after async operations to prevent errors on disposed widgets
Applied to files:
lib/main.dart
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Use dual storage strategy: store Mostro/default relays in `settings.relays` and user relays in `settings.userRelays` with full JSON metadata via `toJson()`/`fromJson()`
Applied to files:
lib/main.dartlib/core/app.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/settings/settings.dart : Use null-aware operators (`??`) in Settings `copyWith()` method to preserve existing values for selectedLanguage and defaultLightningAddress when not explicitly overridden
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/core/mostro_fsm.dart : Use MostroFSM for managing order state transitions - all state changes must go through FSM state methods
Applied to files:
lib/core/app.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/core/app.dart
📚 Learning: 2025-06-26T15:03:23.529Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/core/app.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/core/app.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/core/app_routes.dart : Use GoRouter for all navigation configuration instead of Navigator API - maintain all routes in app_routes.dart
Applied to files:
lib/core/app.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/core/app.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
lib/core/app.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/core/app.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Name Riverpod providers using the `<Feature>Provider` or `<Feature>Notifier` convention
Applied to files:
lib/core/app.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/core/app.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
lib/core/app.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Persistence, APIs, and background jobs should live in `lib/data/` and `lib/background/`; generated localization output must be in `lib/generated/` and must stay untouched
Applied to files:
lib/core/app.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Run `flutter test` before pushing code and add targeted test scenarios when expanding complex logic or async workflows
Applied to files:
docs/FCM_TESTING_GUIDE.md
🪛 markdownlint-cli2 (0.18.1)
docs/FCM_TESTING_GUIDE.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
96-96: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
198-198: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
216-216: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
223-223: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
268-268: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
273-273: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
278-278: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
283-283: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
302-302: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
308-308: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
313-313: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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
Implement privacy-preserving push notification system inspired by MIP-05: - Add PushNotificationService for encrypted device token registration - Encrypt FCM tokens with server's public key using ECDH + ChaCha20-Poly1305 - Register tokens per trade pubkey for targeted notifications - Add debug notification to verify push source (cloud functions vs new server) - Update default Mostro pubkey and add configurable push server URL - Integrate
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/main.dart (1)
80-101: Background handler performs heavy processing; past review concerns remain.A past review flagged that
fetchAndProcessNewEvents()performs heavy operations (network calls, decryption, database access, notification display) in the background isolate, which violates best practices for FCM background handlers. While event limits and timeouts have been added, the fundamental concern about plugin safety in background isolates remains.For production reliability, consider:
- Setting the
fcm.pending_fetchflag and deferring processing to app resume, or- Using platform background job APIs (WorkManager/BGTask) for the heavy lifting
This is flagged as a duplicate since it was raised in past reviews.
🧹 Nitpick comments (4)
lib/shared/notifiers/session_notifier.dart (2)
118-119: Push token registration is fire-and-forget; consider awaiting for reliability.The
_registerPushTokencall is not awaited, meaning the session is returned before registration completes. If registration fails silently, the user won't receive push notifications for this trade. Consider awaiting to ensure registration success before returning the session, or at minimum log the registration attempt for debugging.🔎 Proposed fix to await registration
// Register push notification token for this trade - _registerPushToken(tradeKey.public); + await _registerPushToken(tradeKey.public); _emitState();
202-214: Consider awaiting unregister before storage deletion for consistency.The
_unregisterPushTokencall is fire-and-forget. While this is acceptable since the session is already removed locally, awaiting would ensure the server-side cleanup completes before storage deletion. This is a minor consistency improvement.lib/main.dart (1)
202-241: Debug notification code should be removed before production release.The
_showDebugSourceNotificationfunction and its call at line 50 are clearly marked for removal after testing. Given the PR status mentions "needs more testing and bug fixes before production deployment," this is acceptable for now. Create a follow-up task to remove this before merging to production.Do you want me to open an issue to track removing the debug notification code before production release?
lib/services/push_notification_service.dart (1)
250-276: HKDF implementation only supports single-block output (≤32 bytes).The expand step produces only one block of output, which is fine since
lengthis always 32 in current usage. However, if future use requires more than 32 bytes, this will silently produce incorrect output. Consider adding an assertion or extending to support multi-block expansion.🔎 Proposed fix: Add assertion for single-block limit
/// HKDF-SHA256 key derivation Uint8List _hkdfDerive(Uint8List ikm, int length) { + // This implementation only supports single-block output (≤32 bytes) + assert(length <= 32, 'HKDF expand supports max 32 bytes with this implementation'); + const salt = 'mostro-push-v1'; const info = 'mostro-token-encryption';
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/core/config.dart(2 hunks)lib/main.dart(3 hunks)lib/services/push_notification_service.dart(1 hunks)lib/shared/notifiers/session_notifier.dart(3 hunks)lib/shared/providers/push_notification_provider.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/shared/providers/push_notification_provider.dartlib/core/config.dartlib/shared/notifiers/session_notifier.dartlib/services/push_notification_service.dartlib/main.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/shared/providers/push_notification_provider.dartlib/core/config.dartlib/shared/notifiers/session_notifier.dartlib/services/push_notification_service.dartlib/main.dart
lib/shared/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Files:
lib/shared/providers/push_notification_provider.dartlib/shared/notifiers/session_notifier.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/push_notification_service.dart
lib/main.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Configure timeago package locales in app initialization for proper relative time formatting (e.g., 'hace X horas' vs 'hours ago')
Files:
lib/main.dart
🧠 Learnings (24)
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Name Riverpod providers using the `<Feature>Provider` or `<Feature>Notifier` convention
Applied to files:
lib/shared/providers/push_notification_provider.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
lib/shared/providers/push_notification_provider.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/shared/providers/push_notification_provider.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/shared/providers/push_notification_provider.dartlib/shared/notifiers/session_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/shared/providers/push_notification_provider.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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/shared/providers/push_notification_provider.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/shared/providers/push_notification_provider.dartlib/core/config.dartlib/shared/notifiers/session_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/shared/notifiers/session_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/shared/notifiers/session_notifier.dartlib/main.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/shared/notifiers/session_notifier.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/orders/notifiers/add_order_notifier.dart : Start session timeout cleanup in `submitOrder()` method to prevent orphan sessions when Mostro doesn't respond within 10 seconds
Applied to files:
lib/shared/notifiers/session_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : Use SubscriptionManager with `fireImmediately: false` during SessionNotifier initialization to prevent premature execution
Applied to files:
lib/shared/notifiers/session_notifier.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Use `startSessionTimeoutCleanupForRequestId()` for order creation timeout protection and cancel timer automatically when any Mostro response received
Applied to files:
lib/shared/notifiers/session_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
lib/shared/notifiers/session_notifier.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/**/*.dart : Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Applied to files:
lib/shared/notifiers/session_notifier.dart
📚 Learning: 2025-06-08T23:54:09.987Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 110
File: lib/features/trades/widgets/mostro_message_detail_widget.dart:134-141
Timestamp: 2025-06-08T23:54:09.987Z
Learning: In the OrderState refactor, public keys should be accessed via `tradeState.peer?.publicKey` from the OrderState instance rather than through `session?.peer?.publicKey`, as the OrderState encapsulates peer information directly.
Applied to files:
lib/shared/notifiers/session_notifier.dart
📚 Learning: 2025-11-27T12:10:12.081Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.081Z
Learning: Run `flutter test integration_test/` only for significant changes affecting core services and main user flows
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Persistence, APIs, and background jobs should live in `lib/data/` and `lib/background/`; generated localization output must be in `lib/generated/` and must stay untouched
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Always check `mounted` before using BuildContext after async operations to prevent errors on disposed widgets
Applied to files:
lib/main.dart
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 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/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Use dual storage strategy: store Mostro/default relays in `settings.relays` and user relays in `settings.userRelays` with full JSON metadata via `toJson()`/`fromJson()`
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/settings/settings.dart : Use null-aware operators (`??`) in Settings `copyWith()` method to preserve existing values for selectedLanguage and defaultLightningAddress when not explicitly overridden
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/main.dart : Configure timeago package locales in app initialization for proper relative time formatting (e.g., 'hace X horas' vs 'hours ago')
Applied to files:
lib/main.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 (9)
lib/core/config.dart (2)
46-51: LGTM! Push server configuration follows established patterns.The
pushServerUrlconfiguration uses the sameString.fromEnvironmentpattern as other config values, allowing environment-based overrides while providing a sensible default.
15-16: Verify the updated Mostro public key is correct.The default
mostroPubKeyhas been changed. Ensure this new public key (0a537332f2d5...) corresponds to the intended Mostro daemon instance for production use.lib/shared/providers/push_notification_provider.dart (1)
1-10: LGTM! Provider follows naming conventions and properly wires the service.The provider correctly uses the
<Feature>Providernaming convention and injects the configuration dependency. Based on learnings about Riverpod provider naming.lib/shared/notifiers/session_notifier.dart (1)
126-143: LGTM! Error handling prevents push failures from breaking session management.The try-catch wrappers with warning logs ensure that push notification failures don't crash the core session lifecycle, which is the right trade-off for a non-critical enhancement.
lib/main.dart (2)
113-119: LGTM! Firebase initialization follows recommended patterns.The conditional initialization on supported platforms and early registration of the background message handler follows Firebase Messaging best practices.
56-59: No issues found. The comment about SharedPreferencesAsync behavior is accurate—no reload() is needed because the API queries the platform directly on each call without a local cache.lib/services/push_notification_service.dart (3)
12-18: Well-documented privacy-preserving design.The class-level documentation clearly explains the MIP-05 inspired privacy approach. This helps future maintainers understand the security model.
163-228: LGTM! Cryptographic implementation follows sound practices.The encryption implementation uses:
- Ephemeral ECDH for forward secrecy
- ChaCha20-Poly1305 AEAD with 128-bit auth tag
- Proper random number generation via
Random.secure()- secp256k1 curve (consistent with Nostr ecosystem)
71-126: LGTM! Token registration has proper validation and error handling.The method correctly:
- Validates preconditions (platform support, server pubkey initialized)
- Handles null FCM token gracefully
- Uses timeouts for HTTP requests
- Logs truncated keys for privacy
- Parses response for success confirmation
| /// Initialize push notification service for targeted notifications | ||
| Future<void> _initializePushService() async { | ||
| if (!_isFirebaseSupported) return; | ||
|
|
||
| try { | ||
| final pushService = PushNotificationService( | ||
| pushServerUrl: Config.pushServerUrl, | ||
| ); | ||
| await pushService.initialize(); | ||
| } catch (e) { | ||
| debugPrint('Failed to initialize push service: $e'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Redundant PushNotificationService instantiation.
This function creates and initializes a standalone PushNotificationService instance that is immediately discarded. Meanwhile, pushNotificationServiceProvider creates a separate instance when accessed by SessionNotifier. This results in:
- Two separate service instances with independent state
- The provider's instance never has
initialize()called (no server pubkey fetched) - Token registration will fail since
_serverPubkeyis null in the provider's instance
Either use the provider here or ensure the provider's instance is initialized.
🔎 Proposed fix: Initialize the provider's instance instead
/// Initialize push notification service for targeted notifications
Future<void> _initializePushService() async {
if (!_isFirebaseSupported) return;
try {
- final pushService = PushNotificationService(
- pushServerUrl: Config.pushServerUrl,
- );
- await pushService.initialize();
+ // Note: Initialization should happen through the provider
+ // The provider instance will be initialized when first accessed
+ // Consider moving this to a proper app initialization flow with ProviderContainer
+ debugPrint('Push service will be initialized on first use');
} catch (e) {
debugPrint('Failed to initialize push service: $e');
}
}Or better, modify the provider to auto-initialize:
final pushNotificationServiceProvider = FutureProvider<PushNotificationService>((ref) async {
final service = PushNotificationService(pushServerUrl: Config.pushServerUrl);
await service.initialize();
return service;
});🤖 Prompt for AI Agents
lib/main.dart lines 188-200: The function currently constructs a throwaway
PushNotificationService instance and initializes it, leaving the provider's
instance uninitialized which causes null server pubkey and token registration
failures; either obtain and initialize the provider's instance here (e.g., await
the existing pushNotificationServiceProvider so its service.initialize() is
called) or change the provider to auto-initialize by making it return an
already-initialized service (convert it to a FutureProvider that constructs the
service, calls initialize(), then returns it); implement one of these fixes so
only the provider's instance is used and initialized.
| /// Unregister a device token for a specific trade | ||
| Future<bool> unregisterToken(String tradePubkey) async { | ||
| if (!isSupported) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| final response = await http | ||
| .post( | ||
| Uri.parse('$_pushServerUrl/api/unregister'), | ||
| headers: {'Content-Type': 'application/json'}, | ||
| body: jsonEncode({ | ||
| 'trade_pubkey': tradePubkey, | ||
| }), | ||
| ) | ||
| .timeout(const Duration(seconds: 10)); | ||
|
|
||
| if (response.statusCode == 200) { | ||
| _logger.i( | ||
| 'Token unregistered for trade ${tradePubkey.substring(0, 16)}...'); | ||
| return true; | ||
| } | ||
|
|
||
| _logger.w('Failed to unregister token: ${response.body}'); | ||
| return false; | ||
| } catch (e) { | ||
| _logger.e('Error unregistering token: $e'); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the push notification service file
find . -name "push_notification_service.dart" -type f 2>/dev/null | head -20Repository: MostroP2P/mobile
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Get file size and then examine it
if [ -f "lib/services/push_notification_service.dart" ]; then
wc -l lib/services/push_notification_service.dart
echo "---"
cat -n lib/services/push_notification_service.dart
else
echo "File not found at lib/services/push_notification_service.dart"
# Search for it
find . -name "*push*" -type f 2>/dev/null | grep -E "\.(dart|yaml)$"
fiRepository: MostroP2P/mobile
Length of output: 12116
🏁 Script executed:
#!/bin/bash
# Look for registerToken method to compare
rg "registerToken|unregisterToken" --type dart -B 3 -A 15Repository: MostroP2P/mobile
Length of output: 5148
🏁 Script executed:
#!/bin/bash
# Search for authentication patterns in service files
rg "signature|signed|auth|_serverPubkey|_pushServer" lib/services/ --type dart -B 2 -A 2Repository: MostroP2P/mobile
Length of output: 13823
Unregister endpoint lacks ownership verification - enables denial of service.
The unregisterToken method sends only the trade_pubkey without any proof of ownership. An attacker can unregister other users' trade pubkeys, preventing them from receiving push notifications. While this doesn't leak data, it's an availability risk.
The asymmetry is evident: registerToken uses encryption (via _encryptToken()) to secure the operation, but unregisterToken sends a plain request. Consider requiring the same encrypted payload or a cryptographic signature to prove ownership before allowing unregistration, consistent with the privacy-preserving model documented in the class.
🤖 Prompt for AI Agents
In lib/services/push_notification_service.dart around lines 128 to 157, the
unregisterToken call sends only the trade_pubkey allowing
unauthenticated/unverified unregistrations; update the method to include the
same ownership proof used by registerToken (e.g. call and attach _encryptToken()
output or a cryptographic signature in the request body/headers), send the
encrypted payload instead of or alongside trade_pubkey, preserve timeout and
content-type handling, and update logging/error paths to reflect failed
encryption or missing proof so the server can verify ownership before
unregistration.
| /// Create padded payload: platform_byte || token_length (2 bytes BE) || token || random_padding | ||
| Uint8List _createPaddedPayload(String deviceToken, Random random) { | ||
| final tokenBytes = utf8.encode(deviceToken); | ||
| final platformByte = Platform.isIOS ? _platformIos : _platformAndroid; | ||
|
|
||
| final payload = Uint8List(_paddedPayloadSize); | ||
| payload[0] = platformByte; | ||
| payload[1] = (tokenBytes.length >> 8) & 0xFF; | ||
| payload[2] = tokenBytes.length & 0xFF; | ||
| payload.setRange(3, 3 + tokenBytes.length, tokenBytes); | ||
|
|
||
| // Fill rest with random padding | ||
| final paddingStart = 3 + tokenBytes.length; | ||
| for (var i = paddingStart; i < _paddedPayloadSize; i++) { | ||
| payload[i] = random.nextInt(256); | ||
| } | ||
|
|
||
| return payload; | ||
| } |
There was a problem hiding this comment.
Missing validation for token length exceeding payload capacity.
If deviceToken exceeds 217 bytes (220 - 3 header bytes), setRange at line 239 will write beyond the payload buffer, causing a runtime error. FCM tokens are typically ~163 chars, but this should be validated defensively.
🔎 Proposed fix: Add length validation
Uint8List _createPaddedPayload(String deviceToken, Random random) {
final tokenBytes = utf8.encode(deviceToken);
+
+ // Validate token fits in payload (220 - 3 header bytes = 217 max)
+ if (tokenBytes.length > _paddedPayloadSize - 3) {
+ throw ArgumentError('Device token too long: ${tokenBytes.length} bytes (max: ${_paddedPayloadSize - 3})');
+ }
+
final platformByte = Platform.isIOS ? _platformIos : _platformAndroid;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Create padded payload: platform_byte || token_length (2 bytes BE) || token || random_padding | |
| Uint8List _createPaddedPayload(String deviceToken, Random random) { | |
| final tokenBytes = utf8.encode(deviceToken); | |
| final platformByte = Platform.isIOS ? _platformIos : _platformAndroid; | |
| final payload = Uint8List(_paddedPayloadSize); | |
| payload[0] = platformByte; | |
| payload[1] = (tokenBytes.length >> 8) & 0xFF; | |
| payload[2] = tokenBytes.length & 0xFF; | |
| payload.setRange(3, 3 + tokenBytes.length, tokenBytes); | |
| // Fill rest with random padding | |
| final paddingStart = 3 + tokenBytes.length; | |
| for (var i = paddingStart; i < _paddedPayloadSize; i++) { | |
| payload[i] = random.nextInt(256); | |
| } | |
| return payload; | |
| } | |
| /// Create padded payload: platform_byte || token_length (2 bytes BE) || token || random_padding | |
| Uint8List _createPaddedPayload(String deviceToken, Random random) { | |
| final tokenBytes = utf8.encode(deviceToken); | |
| // Validate token fits in payload (220 - 3 header bytes = 217 max) | |
| if (tokenBytes.length > _paddedPayloadSize - 3) { | |
| throw ArgumentError('Device token too long: ${tokenBytes.length} bytes (max: ${_paddedPayloadSize - 3})'); | |
| } | |
| final platformByte = Platform.isIOS ? _platformIos : _platformAndroid; | |
| final payload = Uint8List(_paddedPayloadSize); | |
| payload[0] = platformByte; | |
| payload[1] = (tokenBytes.length >> 8) & 0xFF; | |
| payload[2] = tokenBytes.length & 0xFF; | |
| payload.setRange(3, 3 + tokenBytes.length, tokenBytes); | |
| // Fill rest with random padding | |
| final paddingStart = 3 + tokenBytes.length; | |
| for (var i = paddingStart; i < _paddedPayloadSize; i++) { | |
| payload[i] = random.nextInt(256); | |
| } | |
| return payload; | |
| } |
🤖 Prompt for AI Agents
In lib/services/push_notification_service.dart around lines 230 to 248, the
method _createPaddedPayload does not validate deviceToken length and can write
past the payload buffer if tokenBytes.length > _paddedPayloadSize - 3; add a
defensive check right after computing tokenBytes (e.g., if (tokenBytes.length >
_paddedPayloadSize - 3) throw ArgumentError.value(deviceToken, 'deviceToken',
'encoded token too long for padded payload')); this prevents buffer overflow and
surfaces a clear error to callers; keep existing behavior otherwise and return
the padded payload as before.
This PR implements Firebase Cloud Messaging (FCM) for push notifications in Mostro Mobile. The implementation migrates from WebSocket listeners to a polling-based architecture using Firebase Cloud Functions.
Implementation Details
Current Status
Important Considerations
Firebase Project Setup
Cost & Infrastructure Evaluation
Firebase Cloud Functions provide a quick implementation path, but come with important considerations:
Pros:
Cons:
Alternative Architecture
For decentralization and cost control, consider deploying a self-hosted push server:
Proposed Architecture:
Work in Progress:
Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.