Skip to content

feat(notifications): wire Firebase/FCM push notifications and sembast - #94

Merged
grunch merged 7 commits into
mainfrom
feat/push-notifications
Apr 4, 2026
Merged

feat(notifications): wire Firebase/FCM push notifications and sembast#94
grunch merged 7 commits into
mainfrom
feat/push-notifications

Conversation

@grunch

@grunch grunch commented Apr 4, 2026

Copy link
Copy Markdown
Member
  • Add firebase_core, firebase_messaging, flutter_local_notifications deps
  • Create placeholder firebase_options.dart and docs/firebase-setup.md
  • Add web/firebase-messaging-sw.js service worker for PWA push
  • Replace PushNotificationService stubs with full FCM implementation
    (permission request, foreground/background handlers, token management,
    push server registration, pending route for tap-to-navigate)
  • Wire NotificationListenerWidget into app root via ConsumerStatefulWidget
  • Initialize PushNotificationService in MostroApp.initState
  • Add Firebase.initializeApp in main.dart with graceful placeholder fallback
  • Replace sembast in-memory web DB with databaseFactoryWeb via conditional
    import (sembast_factory_web.dart / sembast_factory_stub.dart)
  • Persist notification settings toggles via SharedPreferences

Summary by CodeRabbit

  • New Features

    • Full push notification support (foreground, background, taps) with routing and token management.
    • App initializes Firebase at startup when available and continues gracefully if not configured.
  • Improvements

    • Notification preference toggles now persist across launches.
  • Documentation

    • Added a step-by-step Firebase setup guide, including web service worker instructions.
  • Chores

    • Added Firebase-related platform/plugin registrations and runtime dependencies.

grunch added 2 commits April 4, 2026 19:23
… web persistence

- Add firebase_core, firebase_messaging, flutter_local_notifications deps
- Create placeholder firebase_options.dart and docs/firebase-setup.md
- Add web/firebase-messaging-sw.js service worker for PWA push
- Replace PushNotificationService stubs with full FCM implementation
  (permission request, foreground/background handlers, token management,
  push server registration, pending route for tap-to-navigate)
- Wire NotificationListenerWidget into app root via ConsumerStatefulWidget
- Initialize PushNotificationService in MostroApp.initState
- Add Firebase.initializeApp in main.dart with graceful placeholder fallback
- Replace sembast in-memory web DB with databaseFactoryWeb via conditional
  import (sembast_factory_web.dart / sembast_factory_stub.dart)
- Persist notification settings toggles via SharedPreferences
- Replace dart:io with package:http for cross-platform web compat
- Use ProviderContainer instead of WidgetRef in long-lived closure
- Defer push init to post-frame callback with try/catch
- Guard deleteToken with _initialized flag for unsupported platforms
- Safe-truncate orderId in _defaultBody to prevent RangeError
- Use jsonEncode and check HTTP response status in register/unregister
- Add runtime warning for unconfigured VAPID key on web
@coderabbitai

coderabbitai Bot commented Apr 4, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

Adds Firebase configuration and initialization, implements full FCM-based PushNotificationService with token lifecycle and deep-link routing, persists notification toggles via SharedPreferences, provides platform-gated Sembast factory stubs, and adds a web service worker plus native plugin wiring and docs.

Changes

Cohort / File(s) Summary
Firebase bootstrap & docs
lib/firebase_options.dart, lib/main.dart, docs/firebase-setup.md, firebase.json
Adds generated DefaultFirebaseOptions, calls Firebase.initializeApp(...) with UnsupportedError fallback, includes Firebase CLI config and setup doc.
Push notification runtime
lib/features/notifications/services/push_notification_service.dart, lib/core/app.dart, lib/features/notifications/.../notifications_provider.dart
Implements FCM init (permissions, background handler, foreground/on-open handling), token register/unregister/re-register flows, pending route consumption; switches MostroApp to ConsumerStatefulWidget and initializes push post-frame.
Notification persistence UI
lib/features/settings/screens/notification_settings_screen.dart
Adds SharedPreferences-backed persistence for notification toggle switches with async load/save and error logging.
Sembast platform factories
lib/features/notifications/providers/sembast_factory_io.dart, .../sembast_factory_web.dart, .../sembast_factory_stub.dart, .../notifications_provider.dart
Introduces platform-specific factory modules and conditional import adjustments so web uses sembast_web and non-web uses IO/memory stubs; fixes missing-item handling in markAsRead.
Web service worker & web assets
web/firebase-messaging-sw.js
Adds FCM service worker for background messages and notification click routing (placeholder config values present).
Dependencies & native plugin wiring
pubspec.yaml, macos/Flutter/GeneratedPluginRegistrant.swift, windows/.../generated_plugin_registrant.cc, windows/.../generated_plugins.cmake, android/app/build.gradle.kts, android/app/google-services.json, android/settings.gradle.kts
Adds firebase_core, firebase_messaging, flutter_local_notifications, and http; wires native plugin registrations for macOS/Windows, adds Android google-services files and Gradle plugin/desugaring config.

Sequence Diagram

sequenceDiagram
    participant App as App Startup
    participant Firebase as Firebase SDK
    participant PushSvc as PushNotificationService
    participant FCM as FCM Runtime
    participant Backend as Push Backend
    participant Sembast as Local Store / Sembast
    App->>Firebase: initializeApp(options)
    Firebase-->>App: initialized / UnsupportedError
    App->>PushSvc: init (post-frame) with container
    PushSvc->>FCM: requestPermission(), setBackgroundHandler(), getToken()
    FCM-->>PushSvc: permission result, token, messages
    PushSvc->>Backend: registerToken(token, platform, tradePubkey)
    Backend-->>PushSvc: ack
    FCM-->>PushSvc: onMessage (foreground)
    PushSvc->>Sembast: persist NotificationModel
    PushSvc->>App: queue pending route / consumePendingRoute()
    App->>App: NotificationListenerWidget routes tap -> UI navigation
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Poem

🐰 I hopped through code at break of dawn,
Firebase lights and tokens carried on,
Background bells and clicks that steer,
Settings saved and routes appear,
Tiny rabbit cheers — notifications here! 🥕🔔

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately summarizes the main change: adding Firebase/FCM push notifications and Sembast database integration.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/push-notifications

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mostronatorcoder mostronatorcoder Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review

Solid implementation. FCM wiring, service worker, Sembast web persistence, and notification settings all work correctly. Two issues before merge.


🔴 Blocker — notification_settings_screen.dart respects preferences in UI but does NOT gate push delivery

The four SharedPreferences flags (notify_trade_updates, etc.) are persisted and loaded on screen open. However, nothing in the codebase reads them. When _handleForeground in PushNotificationService receives a message, it creates a NotificationModel and adds it to the provider unconditionally — the user's preference flags are never checked.

A user who disables "Trade updates" still receives trade update notifications in-app. The settings screen is cosmetic only.

Fix: read the preferences before creating the in-app notification in _handleForeground:

void _handleForeground(RemoteMessage message, {ProviderContainer? container}) {
  final data = message.data;
  if (data.isEmpty) return;
  final type = data['type'] as String?;
  if (type == null) return;

  // Respect per-type notification preferences.
  if (!_isTypeEnabled(type)) return;

  // ... rest of the method unchanged
}

bool _isTypeEnabled(String type) {
  // Read synchronously from SharedPreferences. The prefs instance is
  // available synchronously after the first async load in the settings screen.
  // Use the same keys as notification_settings_screen.dart.
  final prefs = _cachedPrefs;
  if (prefs == null) return true; // default: allow until prefs are loaded
  return switch (type) {
    'tradeUpdate' => prefs.getBool('notify_trade_updates') ?? true,
    'invoiceRequest' || 'paymentReceived' => prefs.getBool('notify_payments') ?? true,
    'orderTaken' => prefs.getBool('notify_trade_updates') ?? true,
    'dispute' => prefs.getBool('notify_disputes') ?? true,
    _ => true,
  };
}

SharedPreferences? _cachedPrefs;

// Call this during initialize() after loading prefs:
Future<void> _cachePrefs() async {
  _cachedPrefs = await SharedPreferences.getInstance();
}

Add await _cachePrefs() inside initialize(). This ensures the prefs instance is available synchronously in _handleForeground.


🟡 Major — _saveBool in notification_settings_screen.dart has a missing mounted check

Future<void> _saveBool(String key, bool value) async {
  try {
    final prefs = await SharedPreferences.getInstance();
    await prefs.setBool(key, value);   // ← no mounted check
  } catch (e) { ... }
}

SharedPreferences.getInstance() is async. If the widget is disposed between the toggle and the await, prefs.setBool still runs (harmless, but the framework may warn). More importantly, if you add any setState inside _saveBool in the future, this becomes a real bug.

Fix: add a mounted check after the await:

Future<void> _saveBool(String key, bool value) async {
  try {
    final prefs = await SharedPreferences.getInstance();
    if (!mounted) return;
    await prefs.setBool(key, value);
  } catch (e) {
    debugPrint('[notification_settings] save failed: $e');
  }
}

✅ What's good

  • Conditional import pattern for Sembast (sembast_factory_web.dart / sembast_factory_stub.dart) is the correct approach for targeting dart.library.io vs dart.library.html. Clean.
  • firebase_options.dart placeholder throws UnsupportedError with a clear message pointing to docs/firebase-setup.md. Good — fails fast with actionable error.
  • Firebase init wrapped in try/catch UnsupportedError in main.dart — app boots without push in non-Firebase builds. Correct.
  • PushNotificationService.initialize() bails early if _isDesktop. Correct.
  • initialize() now uses ProviderContainer instead of WidgetRef — correct for a singleton service that lives outside the widget tree.
  • _token == null guard in registerToken/unregisterToken prevents sending null tokens to the server.
  • reRegisterAllTokens() uses Set.of(_registeredTradePubkeys) to avoid ConcurrentModificationException. Correct.
  • _defaultBody uses math.min(8, orderId.length) to avoid RangeError on short IDs. Good catch.
  • Service worker version (10.14.0) matches firebase_messaging: ^15.1.3. Keep in sync when upgrading.
  • web/firebase-messaging-sw.js uses payload.notification ?? {} with nullish coalescing — correct for browsers that don't support optional chaining in SW context.
  • NotificationListenerWidget correctly uses addPostFrameCallback to consume the pending route — waits for the navigator to be ready. Correct.
  • docs/firebase-setup.md is clear and actionable.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Nitpick comments (2)
lib/features/settings/screens/notification_settings_screen.dart (2)

2-10: SharedPreferences usage noted; guideline deviation is documented.

The coding guidelines specify Sembast for UI-layer state management. The doc comment explains this is a temporary solution pending the Rust settings API—acceptable given the explicit migration path. Consider adding a TODO comment or tracking issue to ensure this migration happens.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/features/settings/screens/notification_settings_screen.dart` around lines
2 - 10, The comment notes SharedPreferences is used temporarily instead of
Sembast; add an explicit TODO with a tracking issue reference to the top of
notification_settings_screen.dart near the existing doc comment so future
maintainers know to migrate to the Rust settings API or Sembast-based UI state
when those fields (e.g., notify_trade_updates) exist; mention SharedPreferences
usage and include a short task ID or GitHub issue link and an estimated priority
to ensure it isn't forgotten.

52-59: Consider error recovery for failed saves.

If setBool fails, the UI shows the new state but storage retains the old value. On app restart, the toggle reverts unexpectedly. For a more robust user experience, consider reverting the UI state on failure or showing a snackbar.

♻️ Optional: Add error recovery
-  Future<void> _saveBool(String key, bool value) async {
+  Future<bool> _saveBool(String key, bool value) async {
     try {
       final prefs = await SharedPreferences.getInstance();
       await prefs.setBool(key, value);
+      return true;
     } catch (e) {
       debugPrint('[notification_settings] save failed: $e');
+      return false;
     }
   }

Then in onChanged handlers, you could revert on failure:

onChanged: (v) async {
  setState(() => _tradeUpdates = v);
  final saved = await _saveBool(_kTradeUpdates, v);
  if (!saved && mounted) {
    setState(() => _tradeUpdates = !v);
  }
},
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/features/settings/screens/notification_settings_screen.dart` around lines
52 - 59, The _saveBool method currently swallows errors and returns void, so UI
toggles can lie if persistence fails; change _saveBool to return a bool success
flag (e.g., Future<bool> _saveBool(String key, bool value)) that returns true on
success and false on failure, and update all onChanged handlers (e.g., the
handlers that setState for _tradeUpdates/_someOtherToggle) to await the save,
and if it returns false revert the UI state (setState back to previous value) or
show a snackbar when mounted; ensure you still log the exception inside
_saveBool before returning false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/core/app.dart`:
- Around line 49-65: NotificationListenerWidget is placed above
MaterialApp.router so its BuildContext can't access GoRouter, causing
context.push(...) in NotificationListenerWidget.initState() to fail; fix by
either moving NotificationListenerWidget so it is built inside the
MaterialApp.router subtree (so its context is a descendant of the router) or
modify NotificationListenerWidget.initState() to call the router instance
directly (e.g., use the appRouter or Router.of(context).routerDelegate/GoRouter
instance to call push/go) instead of context.push; update references in
push_notification_service.dart (initState and any context.push calls) or
relocate the NotificationListenerWidget in the widget tree to be a child of the
MaterialApp.router.

In `@lib/features/notifications/providers/notifications_provider.dart`:
- Around line 13-14: The unconditional import of package:sembast/sembast_io.dart
causes Flutter web compilation failures; change the imports so the IO backend is
only imported when dart.library.io is available — either replace the
unconditional import with a conditional import like "if (dart.library.io)
'package:sembast/sembast_io.dart' else 'package:sembast_web/sembast_web.dart'"
or instead import the neutral "package:sembast/sembast.dart" unconditionally and
keep the factory conditional import (sembast_factory_web.dart /
sembast_factory_stub.dart); update the import statement(s) near the top where
sembast_io.dart is referenced so the code no longer pulls in dart:io on web.

In `@lib/features/notifications/services/push_notification_service.dart`:
- Around line 66-69: The code currently prints the raw FCM token (variable
_token) via debugPrint in push_notification_service.dart; remove that sensitive
output and instead log a non-secret message or a redacted token. Replace the
debugPrint('[push] FCM token: $_token') call used after _fcm.getToken(...) with
something like debugPrint('[push] FCM token acquired') or log a masked value
(e.g., show only the first/last few chars and replace the rest with ellipses) so
the real token is never written to logs; ensure the change is applied to any
other places that log _token.
- Around line 61-68: The code currently passes the placeholder vapidKey to
_fcm.getToken on web which will throw; update the init so that when kIsWeb and
vapidKey == 'YOUR_VAPID_KEY' you do not call _fcm.getToken — either return early
from the web token setup path (so message/tap listeners still get wired) or
surround the _fcm.getToken(...) call with a try/catch that logs the real
exception and leaves _token null; target the vapidKey constant, the kIsWeb
guard, the _fcm.getToken call and the _token assignment when making this change.

In `@pubspec.yaml`:
- Around line 44-57: The dependencies list in pubspec.yaml is missing the
required Nostr SDK; add a new dependency entry nostr_sdk: ^0.44.0 to the
dependencies section (near http, share_plus, file_picker, firebase_core, etc.)
so the project includes the Nostr SDK (use the caret version ^0.44.0 to match
project guidelines).

In `@web/firebase-messaging-sw.js`:
- Around line 20-27: The service worker shows notifications via
messaging.onBackgroundMessage and stores payload.data in showNotification but
lacks a notificationclick handler; add a
self.addEventListener('notificationclick', ...) implementation that reads
event.notification.data, closes the notification, focuses or opens the relevant
client window (using clients.matchAll and clients.openWindow) and routes the
payload data (e.g., URL or action) into the page so clicks are handled on web
like native onMessageOpenedApp; ensure you reference the same data shape stored
by the existing self.registration.showNotification call and guard for missing
data before opening/focusing clients.

---

Nitpick comments:
In `@lib/features/settings/screens/notification_settings_screen.dart`:
- Around line 2-10: The comment notes SharedPreferences is used temporarily
instead of Sembast; add an explicit TODO with a tracking issue reference to the
top of notification_settings_screen.dart near the existing doc comment so future
maintainers know to migrate to the Rust settings API or Sembast-based UI state
when those fields (e.g., notify_trade_updates) exist; mention SharedPreferences
usage and include a short task ID or GitHub issue link and an estimated priority
to ensure it isn't forgotten.
- Around line 52-59: The _saveBool method currently swallows errors and returns
void, so UI toggles can lie if persistence fails; change _saveBool to return a
bool success flag (e.g., Future<bool> _saveBool(String key, bool value)) that
returns true on success and false on failure, and update all onChanged handlers
(e.g., the handlers that setState for _tradeUpdates/_someOtherToggle) to await
the save, and if it returns false revert the UI state (setState back to previous
value) or show a snackbar when mounted; ensure you still log the exception
inside _saveBool before returning false.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5e60033d-7a44-456f-b01c-ed913e17a7b8

📥 Commits

Reviewing files that changed from the base of the PR and between 7f46487 and ce1ca02.

📒 Files selected for processing (14)
  • docs/firebase-setup.md
  • lib/core/app.dart
  • lib/features/notifications/providers/notifications_provider.dart
  • lib/features/notifications/providers/sembast_factory_stub.dart
  • lib/features/notifications/providers/sembast_factory_web.dart
  • lib/features/notifications/services/push_notification_service.dart
  • lib/features/settings/screens/notification_settings_screen.dart
  • lib/firebase_options.dart
  • lib/main.dart
  • macos/Flutter/GeneratedPluginRegistrant.swift
  • pubspec.yaml
  • web/firebase-messaging-sw.js
  • windows/flutter/generated_plugin_registrant.cc
  • windows/flutter/generated_plugins.cmake

Comment thread lib/core/app.dart Outdated
Comment thread lib/features/notifications/providers/notifications_provider.dart Outdated
Comment thread lib/features/notifications/services/push_notification_service.dart Outdated
Comment thread lib/features/notifications/services/push_notification_service.dart Outdated
Comment thread lib/features/notifications/services/push_notification_service.dart
Comment thread pubspec.yaml
Comment on lines +44 to +57
# Cross-platform HTTP client (push server registration)
http: ^1.2.2

# System share sheet (invoice sharing)
share_plus: ^12.0.1

# File operations (attachment picker)
file_picker: ^8.3.7

# Firebase (push notifications)
firebase_core: ^3.6.0
firebase_messaging: ^15.1.3
flutter_local_notifications: ^17.2.3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether a nostr sdk dependency is declared in pubspec.yaml
rg -n '^\s*(nostr[-_]sdk|nostr_sdk|nostr):\s*' pubspec.yaml

Repository: MostroP2P/app

Length of output: 39


🌐 Web query:

What is the correct Flutter/Dart package name and version format to satisfy “nostr-sdk 0.44+” in pubspec.yaml?

💡 Result:

The Flutter/Dart package is nostr_sdk, available on pub.dev with API documentation confirming it's the Nostr SDK for Flutter. No explicit version 0.44 is listed in pub.dev results, but given the matching name and the existence of Nostr.Sdk 0.44.0 in .NET (likely corresponding via the rust-nostr/nostr-sdk-flutter repo, last updated 2026-01-29), use the version constraint "nostr_sdk: ^0.44.0" or "nostr_sdk: '>=0.44.0'" in pubspec.yaml to satisfy "nostr-sdk 0.44+". In pubspec.yaml under dependencies: dependencies: nostr_sdk: ^0.44.0 Note: The package is WIP and not production-ready per docs. The repo https://github.com/rust-nostr/nostr-sdk-flutter may require git dependency if not published to pub.dev with that version: dependencies: nostr_sdk: git: url: https://github.com/rust-nostr/nostr-sdk-flutter.git ref: precompiled_db329c3629654ff5ec89b7e93da920d1 # or appropriate ref for 0.44

Citations:


Add the required Nostr SDK dependency.

This PR updates dependencies, but nostr_sdk (0.44+) is not declared in pubspec.yaml. Add nostr_sdk: ^0.44.0 under the dependencies section to comply with project guidelines.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pubspec.yaml` around lines 44 - 57, The dependencies list in pubspec.yaml is
missing the required Nostr SDK; add a new dependency entry nostr_sdk: ^0.44.0 to
the dependencies section (near http, share_plus, file_picker, firebase_core,
etc.) so the project includes the Nostr SDK (use the caret version ^0.44.0 to
match project guidelines).

Comment thread web/firebase-messaging-sw.js
grunch added 2 commits April 4, 2026 19:45
…ser prefs

- Enable isCoreLibraryDesugaringEnabled + desugar_jdk_libs dependency
  to fix flutter_local_notifications Android build failure
- Read SharedPreferences in _handleForeground to respect per-type
  notification toggles (trade updates, payments, disputes)
- Cache SharedPreferences instance during initialize() for synchronous
  access in the foreground handler
- Add mounted check in _saveBool to prevent post-dispose writes
… add click handler

- Move NotificationListenerWidget inside MaterialApp.router builder
  so its context can reach GoRouter for push navigation
- Replace unconditional sembast_io.dart import with conditional
  sembast_factory_io.dart/sembast_factory_web.dart to avoid pulling
  dart:io on web
- Redact FCM token from logs, skip getToken when VAPID key is placeholder
- Add notificationclick handler to service worker for web push taps

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
lib/features/settings/screens/notification_settings_screen.dart (1)

19-60: Consider moving notification settings state to Riverpod + Sembast-backed UI state.

Current UI-local state and direct SharedPreferences access works, but it diverges from the repository state-management conventions for lib/**.

As per coding guidelines: "**/*.dart: Use Riverpod for state management in Flutter applications" and "lib/**/*.dart: Use Sembast for UI-layer state management in Dart across all platforms".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/features/settings/screens/notification_settings_screen.dart` around lines
19 - 60, The screen currently manages notification toggles locally in
_NotificationSettingsScreenState and directly uses SharedPreferences in
_loadSettings and _saveBool; refactor this by creating a Riverpod state
notifier/provider (e.g.,
NotificationSettingsNotifier/notificationSettingsProvider) that persists to
Sembast, move the constants (_kTradeUpdates, _kNewMessages, _kPaymentAlerts,
_kDisputeUpdates) into that provider's implementation, replace
_loadSettings/_saveBool calls with provider reads/updates and have the widget
consume the provider (remove direct SharedPreferences usage from
_NotificationSettingsScreenState), and update the UI to call the notifier's
update methods so state and persistence follow the project's Riverpod + Sembast
convention.
lib/features/notifications/providers/notifications_provider.dart (1)

146-157: firstWhere throws if id doesn't exist; consider guarding.

If markAsRead is called with a non-existent id, line 152 throws StateError. While caught by the try/catch, the error message ("failed to persist markAsRead") is misleading since persistence wasn't attempted.

🛡️ Suggested guard with firstWhereOrNull
   Future<void> markAsRead(String id) async {
     state = [
       for (final n in state)
         if (n.id == id) n.copyWith(isRead: true) else n,
     ];
     try {
-      final updated = state.firstWhere((n) => n.id == id);
-      await store?.save(updated);
+      final updated = state.where((n) => n.id == id).firstOrNull;
+      if (updated != null) {
+        await store?.save(updated);
+      }
     } catch (e) {
       debugPrint('NotificationsNotifier: failed to persist markAsRead: $e');
     }
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/features/notifications/providers/notifications_provider.dart` around
lines 146 - 157, markAsRead currently uses state.firstWhere((n) => n.id == id)
which will throw StateError when id isn't present; change markAsRead to first
check for existence (e.g., use firstWhereOrNull or state.any((n) => n.id == id))
and return early if not found, and only call await store?.save(updated) when an
updated notification exists; also adjust the catch/logging around store?.save to
report persistence failures separately from “not found” cases (use
NotificationsNotifier.markAsRead, state, store?.save, and the updated local
variable name to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/features/settings/screens/notification_settings_screen.dart`:
- Around line 52-57: The current _saveBool method aborts after awaiting
SharedPreferences.getInstance() if the widget is unmounted, which can drop the
user's last toggle; instead, always perform the persistence write (call
prefs.setBool(key, value)) regardless of mounted and only guard UI updates or
setState with the mounted check. Update _saveBool so it acquires prefs, calls
prefs.setBool(key, value) unconditionally (still inside the try/catch), and only
uses mounted for any subsequent UI/state interactions; reference _saveBool,
SharedPreferences.getInstance, prefs.setBool, and mounted to locate the change.

---

Nitpick comments:
In `@lib/features/notifications/providers/notifications_provider.dart`:
- Around line 146-157: markAsRead currently uses state.firstWhere((n) => n.id ==
id) which will throw StateError when id isn't present; change markAsRead to
first check for existence (e.g., use firstWhereOrNull or state.any((n) => n.id
== id)) and return early if not found, and only call await store?.save(updated)
when an updated notification exists; also adjust the catch/logging around
store?.save to report persistence failures separately from “not found” cases
(use NotificationsNotifier.markAsRead, state, store?.save, and the updated local
variable name to locate the code).

In `@lib/features/settings/screens/notification_settings_screen.dart`:
- Around line 19-60: The screen currently manages notification toggles locally
in _NotificationSettingsScreenState and directly uses SharedPreferences in
_loadSettings and _saveBool; refactor this by creating a Riverpod state
notifier/provider (e.g.,
NotificationSettingsNotifier/notificationSettingsProvider) that persists to
Sembast, move the constants (_kTradeUpdates, _kNewMessages, _kPaymentAlerts,
_kDisputeUpdates) into that provider's implementation, replace
_loadSettings/_saveBool calls with provider reads/updates and have the widget
consume the provider (remove direct SharedPreferences usage from
_NotificationSettingsScreenState), and update the UI to call the notifier's
update methods so state and persistence follow the project's Riverpod + Sembast
convention.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 28195d77-574f-40d4-a3b2-cf2733a2a95a

📥 Commits

Reviewing files that changed from the base of the PR and between ce1ca02 and a773283.

📒 Files selected for processing (8)
  • android/app/build.gradle.kts
  • lib/core/app.dart
  • lib/features/notifications/providers/notifications_provider.dart
  • lib/features/notifications/providers/sembast_factory_io.dart
  • lib/features/notifications/providers/sembast_factory_web.dart
  • lib/features/notifications/services/push_notification_service.dart
  • lib/features/settings/screens/notification_settings_screen.dart
  • web/firebase-messaging-sw.js
✅ Files skipped from review due to trivial changes (1)
  • lib/features/notifications/providers/sembast_factory_io.dart
🚧 Files skipped from review as they are similar to previous changes (4)
  • lib/features/notifications/providers/sembast_factory_web.dart
  • web/firebase-messaging-sw.js
  • lib/features/notifications/services/push_notification_service.dart
  • lib/core/app.dart

Comment on lines +27 to +29
static const _kNewMessages = 'notify_new_messages';
static const _kPaymentAlerts = 'notify_payments';
static const _kDisputeUpdates = 'notify_disputes';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

New messages preference appears disconnected from runtime gating.

This screen persists notify_new_messages (Line 27 / Line 106), but the current notification-type gate in lib/features/notifications/services/push_notification_service.dart (Line 143-156 in provided snippet) has no branch reading that key. Result: this toggle may not affect incoming push behavior.

Also applies to: 104-107

Comment thread lib/features/settings/screens/notification_settings_screen.dart
grunch added 3 commits April 4, 2026 19:53
Wrap IdentityService.initialize() in try/catch so a platform channel
error (e.g. flutter_secure_storage not ready on Android 16) is
non-fatal instead of crashing the app on launch.
…AsRead

Remove mounted guard from _saveBool so the SharedPreferences write
always completes even if the widget is disposed mid-await.

Replace firstWhere with firstOrNull in markAsRead to avoid a
misleading error log when the id is not in state.
@grunch
grunch merged commit 2cd1b4b into main Apr 4, 2026
1 check was pending
@grunch
grunch deleted the feat/push-notifications branch April 4, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant