feat: add notification settings screen with push notification preference (phase 4) #408
Conversation
…nces Add NotificationSettingsScreen with support for push notification configuration: - Add route for /notification_settings in app router - Implement UI for push notifications toggle with platform detection - Add notification preferences for sound and vibration settings - Display privacy information card explaining encrypted token approach - Add Settings model fields for pushNotificationsEnabled, notificationSoundEnabled, not
WalkthroughAdds a notification settings feature: new NotificationSettingsScreen and route, three persisted settings (push, sound, vibration), notifier methods to update/unregister tokens, push service gating/unregistration, main wiring for push integration, and localization entries (EN/ES/IT). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as NotificationSettingsScreen
participant Notifier as SettingsNotifier
participant PushSvc as PushNotificationService
participant FCM as FCMService
User->>UI: Toggle "Push Notifications" (on/off)
UI->>Notifier: updatePushNotificationsEnabled(newValue)
alt newValue == true
Notifier->>Notifier: persist setting
Notifier-->>UI: state updated
Note right of Notifier: token registration occurs via SessionNotifier / PushSvc on next session save or existing registration flow
else newValue == false
Notifier->>Notifier: persist setting
Notifier->>PushSvc: unregisterAllTokens()
Notifier->>FCM: (if available) unregisterToken(...)
Notifier-->>UI: state updated
end
sequenceDiagram
autonumber
participant AppInit
participant Container as ProviderContainer
participant Session as SessionNotifier
participant Settings as SettingsNotifier
participant PushSvc as PushNotificationService
participant FCM as FCMService
AppInit->>Container: create providers
AppInit->>PushSvc: instantiate PushNotificationService
AppInit->>FCM: instantiate FCMService
AppInit->>Session: SessionNotifier.setPushService(PushSvc)
AppInit->>Settings: SettingsNotifier.setPushServices(PushSvc, FCM)
AppInit->>PushSvc: PushSvc.isPushEnabledInSettings = () => settings.pushNotificationsEnabled
Session->>PushSvc: after saving session -> registerToken(...) (fire-and-forget)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
|
…cation fields - Replace activeTrackColor and activeThumbColor with single activeColor property in Switch widgets - Add missing notification-related getters to MockSettings (isLoggingEnabled, pushNotificationsEnabled, notificationSoundEnabled, notificationVibrationEnabled) - Update MockSettings copyWith method to include new notification preference parameters
…anagement - Add setPushServices method to SettingsNotifier to connect with PushNotificationService and FCMService - Implement automatic token unregistration when user disables push notifications in settings - Add setPushNotificationService method to SessionNotifier for automatic token registration - Register push tokens automatically when new sessions are saved - Initialize push notification integration in main.dart with settings
|
@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: 2
🤖 Fix all issues with AI agents
In `@lib/features/settings/settings_notifier.dart`:
- Around line 170-198: The enable path in updatePushNotificationsEnabled
currently only sets state and saves prefs but does not re-register tokens for
existing sessions, leaving notifications silent; modify
updatePushNotificationsEnabled so that when newValue is true it calls a new
helper (or extend _unregisterPushTokens) to iterate the app's existing
sessions/trade pubkeys (e.g., from state.sessions or the session store) and call
_pushService.registerToken(...) for each pubkey, awaiting or handling each
future and logging success/failure; ensure you check _pushService != null before
registering and add robust error handling/logging similar to the unregister path
(use _logger.i/_logger.w) and persist any necessary mapping so future toggles
can re-register.
In `@lib/shared/notifiers/session_notifier.dart`:
- Around line 141-155: The success log in _registerPushToken uses
tradePubkey.substring(0, 16) which can throw if the pubkey is shorter; guard by
computing a safe display string (e.g., if tradePubkey.length > 16 use
tradePubkey.substring(0,16) + '...' else use tradePubkey) and use that display
variable in the _logger.i call so no RangeError occurs during logging.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/features/settings/notification_settings_screen.dartlib/features/settings/settings_notifier.dartlib/main.dartlib/services/push_notification_service.dartlib/shared/notifiers/session_notifier.darttest/mocks.mocks.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/settings/notification_settings_screen.dart
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/notifiers/session_notifier.dartlib/features/settings/settings_notifier.dartlib/services/push_notification_service.dartlib/main.darttest/mocks.mocks.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/notifiers/session_notifier.dartlib/features/settings/settings_notifier.dartlib/services/push_notification_service.dartlib/main.darttest/mocks.mocks.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/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
**/*.{g.dart,mocks.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
Never manually edit generated files (
.g.dart,.mocks.dart) or add individual ignore comments - regenerate withdart run build_runner build -dinstead
Files:
test/mocks.mocks.dart
test/mocks.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Never manually modify
test/mocks.mocks.dart- it already has file-level// ignore_for_file: must_be_immutableignore directives
Files:
test/mocks.mocks.dart
🧠 Learnings (24)
📓 Common learnings
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.
📚 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/features/settings/settings_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/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/features/settings/settings_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/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/shared/notifiers/session_notifier.dartlib/features/settings/settings_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 : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
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-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/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 **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/shared/notifiers/session_notifier.dartlib/features/settings/settings_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/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/shared/notifiers/session_notifier.dartlib/features/settings/settings_notifier.darttest/mocks.mocks.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.dartlib/main.darttest/mocks.mocks.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/notifiers/session_notifier.dartlib/features/settings/settings_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/data/repositories/**/*.dart : Implement Repository pattern for all data access - all data operations must go through repository classes
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-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/notifiers/session_notifier.dartlib/main.dart
📚 Learning: 2026-01-09T22:09:13.071Z
Learnt from: BraCR10
Repo: MostroP2P/mobile PR: 403
File: lib/features/settings/settings_notifier.dart:153-156
Timestamp: 2026-01-09T22:09:13.071Z
Learning: In lib/features/settings/settings_notifier.dart, the updateLoggingEnabled method intentionally does NOT call _saveToPrefs() because the logging toggle should not persist across app restarts. The isLoggingEnabled field is deliberately excluded from Settings.toJson() and always returns false in fromJson() for security and performance reasons, requiring users to explicitly enable logging each session.
Applied to files:
lib/features/settings/settings_notifier.dartlib/services/push_notification_service.darttest/mocks.mocks.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/settings/settings_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/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/settings/settings_notifier.darttest/mocks.mocks.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/settings/settings_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: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/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
📚 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:
test/mocks.mocks.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 **/*_test.dart : Place unit tests beside their feature counterparts using the `*_test.dart` suffix and prefer Mockito fakes generated via `build_runner`
Applied to files:
test/mocks.mocks.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/relays_notifier.dart : Use `removeRelayWithBlacklist()` for Mostro/default relays (adds to blacklist) and `removeRelay()` for permanent user relay deletion
Applied to files:
test/mocks.mocks.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)
lib/shared/notifiers/session_notifier.dart (2)
10-37: Clean service injection hook.Setter-based injection keeps the notifier decoupled and is easy to wire during app initialization.
130-139: Registration hook placement is sensible.Registering after persistence + state emit keeps side-effects out of critical save flow.
lib/main.dart (2)
17-71: Push integration wiring looks solid.The conditional setup is safe and keeps initialization isolated when services aren’t available.
93-117: Initialization helper is clear and well-scoped.Service wiring and settings callback are straightforward and resilient to failures.
Also applies to: 161-163
lib/services/push_notification_service.dart (3)
39-41: Settings callback hook is a good extension point.
113-117: Settings gate before registration makes sense.
194-213: Bulk unregister logic is straightforward and safe.lib/features/settings/settings_notifier.dart (2)
8-26: Push service wiring is clean and minimal.
200-208: Notification sound/vibration toggles look good.test/mocks.mocks.dart (1)
2001-2083: No review comment (generated file).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| Future<void> updatePushNotificationsEnabled(bool newValue) async { | ||
| state = state.copyWith(pushNotificationsEnabled: newValue); | ||
| await _saveToPrefs(); | ||
| _logger.i('Push notifications ${newValue ? 'enabled' : 'disabled'}'); | ||
|
|
||
| // When disabling, unregister all tokens and delete FCM token | ||
| if (!newValue) { | ||
| _unregisterPushTokens(); | ||
| } | ||
| } | ||
|
|
||
| /// Unregister all push tokens when user disables notifications | ||
| void _unregisterPushTokens() { | ||
| if (_pushService != null) { | ||
| _pushService!.unregisterAllTokens().then((_) { | ||
| _logger.i('All push tokens unregistered'); | ||
| }).catchError((e) { | ||
| _logger.w('Failed to unregister push tokens: $e'); | ||
| }); | ||
| } | ||
|
|
||
| if (_fcmService != null) { | ||
| _fcmService!.deleteToken().then((_) { | ||
| _logger.i('FCM token deleted'); | ||
| }).catchError((e) { | ||
| _logger.w('Failed to delete FCM token: $e'); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Enabling push doesn’t re-register existing sessions.
Right now only the disable path triggers token cleanup. If a user re-enables push (or enables it after having open sessions while it was off), no tokens are registered for those existing trade pubkeys. This leaves notifications silently disabled until a new session is saved.
Please add a re-registration step on enable (e.g., iterate existing sessions and call registerToken, or persist known trade pubkeys so an enable path can re-register them).
🤖 Prompt for AI Agents
In `@lib/features/settings/settings_notifier.dart` around lines 170 - 198, The
enable path in updatePushNotificationsEnabled currently only sets state and
saves prefs but does not re-register tokens for existing sessions, leaving
notifications silent; modify updatePushNotificationsEnabled so that when
newValue is true it calls a new helper (or extend _unregisterPushTokens) to
iterate the app's existing sessions/trade pubkeys (e.g., from state.sessions or
the session store) and call _pushService.registerToken(...) for each pubkey,
awaiting or handling each future and logging success/failure; ensure you check
_pushService != null before registering and add robust error handling/logging
similar to the unregister path (use _logger.i/_logger.w) and persist any
necessary mapping so future toggles can re-register.
| /// Register push notification token for a trade pubkey | ||
| void _registerPushToken(String tradePubkey) { | ||
| if (_pushService == null) { | ||
| _logger.d('Push service not available, skipping token registration'); | ||
| return; | ||
| } | ||
|
|
||
| // Fire and forget - don't block session save on push registration | ||
| _pushService!.registerToken(tradePubkey).then((success) { | ||
| if (success) { | ||
| _logger.i('Push token registered for trade: ${tradePubkey.substring(0, 16)}...'); | ||
| } | ||
| }).catchError((e) { | ||
| _logger.w('Failed to register push token: $e'); | ||
| }); |
There was a problem hiding this comment.
Guard pubkey truncation to avoid RangeError.
Line 151 uses substring(0, 16) without checking length. If a shorter key ever arrives, this will throw during logging.
🧩 Suggested fix
- _logger.i('Push token registered for trade: ${tradePubkey.substring(0, 16)}...');
+ final shortKey =
+ tradePubkey.length <= 16 ? tradePubkey : tradePubkey.substring(0, 16);
+ _logger.i('Push token registered for trade: $shortKey...');🤖 Prompt for AI Agents
In `@lib/shared/notifiers/session_notifier.dart` around lines 141 - 155, The
success log in _registerPushToken uses tradePubkey.substring(0, 16) which can
throw if the pubkey is shorter; guard by computing a safe display string (e.g.,
if tradePubkey.length > 16 use tradePubkey.substring(0,16) + '...' else use
tradePubkey) and use that display variable in the _logger.i call so no
RangeError occurs during logging.
* standardize brand green color across application * replace hardcoded background and utility colors with AppTheme constants * replace remaining harcoded colors * add doc with phases
- Add infoEventKind constant for new Mostro info event kind - Update subscription filter to include kind 38385 - Change info event detection from z-tag to kind-based filtering
…nd future encryption plans - Update phase descriptions to clarify current unencrypted implementation (Phase 3) vs future encrypted approach (Phase 5) - Mark Phase 4 (user settings and opt-out) as complete - Move encryption implementation details from Phase 3 to new Phase 5 (future work) - Add privacy note explaining current plaintext token registration over HTTPS - Update PushNotificationService to remove encryption code and use
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 (3)
lib/features/order/screens/payment_confirmation_screen.dart (2)
25-27: Localize hardcoded user-facing strings.Per coding guidelines, use
S.of(context)!.yourKeyfor all user-facing strings. The following strings should be localized:
- Line 26:
'PAYMENT'- Line 78:
'received'- Line 99:
'CONTINUE'Also applies to: 77-83, 99-99
114-120: Fix typo and localize error messages.Line 117 has a typo:
'Unkown Action'→'Unknown Action'. Additionally, both error messages (lines 110 and 117) should be localized.Proposed fix
- 'Unkown Action: ${state.action}', + 'Unknown Action: ${state.action}',Consider adding localization keys for these error messages as well.
docs/FCM_IMPLEMENTATION.md (1)
16-31: Encryption status is contradicted and misrepresented.Multiple sections still claim encrypted token registration is implemented, while Phase 3/5 explicitly state plaintext tokens today. This is a security/privacy documentation correctness issue and can mislead users and reviewers. Please align all “implemented” and “privacy properties” sections with the current plaintext flow, and confine encryption details to Phase 5 (future) only.
Also applies to: 124-129, 160-163, 506-512, 575-579
🤖 Fix all issues with AI agents
In `@docs/architecture/DESIGN_SYSTEM.md`:
- Around line 1121-1126: The Changelog table header "| Date | Version | Changes
| Author |" declares four columns but the data row "| 2026-01-14 | 1.0.0 |
Initial document creation, Phase 1 complete |" only supplies three cells; fix by
either adding the missing author value to that data row (e.g., append "| Your
Name |") or remove the "Author" column from the header and its corresponding
separator so header and rows have matching column counts; update the separator
line accordingly to keep table syntax consistent.
In `@docs/FCM_IMPLEMENTATION.md`:
- Around line 448-453: Several fenced code blocks in FCM_IMPLEMENTATION.md are
missing language identifiers (MD040); update the affected triple-backtick blocks
(the diagram blocks around the Ephemeral Pubkey/Nonce/Ciphertext and
Platform/Token Length/Device Token/Random Padding tables) to include a language
hint like "text" by changing ``` to ```text so the Markdown linter no longer
flags them; locate the fenced blocks in the sections referenced (the blocks
shown in the diff with the ASCII table diagrams) and add the identifier
consistently for both occurrences.
🧹 Nitpick comments (4)
lib/data/repositories/open_orders_repository.dart (1)
52-60: Inconsistent event identification approach.The handler uses
event.type == 'order'for order events butevent.kind == infoEventKindfor info events. Consider using a consistent approach, either both bytypeor both bykind, for maintainability.Additionally, the
event.pubkey == _settings.mostroPublicKeycheck on line 57 is redundant since the subscription filter already constrainsauthorsto_settings.mostroPublicKey.♻️ Suggested refactor for consistency
_subscription = _nostrService.subscribeToEvents(request).listen((event) { - if (event.type == 'order') { + if (event.kind == orderEventKind) { _events[event.orderId!] = event; _eventStreamController.add(_events.values.toList()); - } else if (event.kind == infoEventKind && - event.pubkey == _settings.mostroPublicKey) { + } else if (event.kind == infoEventKind) { _logger.i('Mostro instance info loaded: $event'); _mostroInstance = event; }lib/features/disputes/widgets/dispute_message_bubble.dart (1)
100-116: Consider localizing time format strings.The relative time strings ('now', 'm ago', 'h ago', 'd ago') are hard-coded in English. Per coding guidelines, user-facing strings should use
S.of(context)!.yourKey. Consider using localized ARB entries or thetimeagopackage which is already configured in the codebase for locale-aware formatting.docs/architecture/DESIGN_SYSTEM.md (1)
46-52: Add language specifiers to fenced code blocks.Several code blocks lack language specifiers. Adding them improves syntax highlighting and documentation quality.
Example fixes
-``` +```text mostroGreen: ████ `#9CD651` (156, 214, 81) ← "Official" brand colorFor the phase overview block:
-``` +```text ✅ Phase 1: Green Unification + Hardcoded Cleanup (COMPLETE)Also applies to: 83-87, 111-114, 144-152, 160-171, 255-260, 585-590
lib/services/push_notification_service.dart (1)
171-190: Consider clearing local state on unregistration failures.When
unregisterTokenfails for a specific trade pubkey, that pubkey remains in_registeredTradePubkeys. If the server has already processed some unregistrations but not others, the local state may become inconsistent with server state.Consider whether failed unregistrations should also clear the local tracking to ensure the user can re-enable push notifications cleanly, or alternatively, track which unregistrations failed for retry.
Option: Clear local state regardless of server response
Future<void> unregisterAllTokens() async { if (!isSupported || _registeredTradePubkeys.isEmpty) { return; } debugPrint( 'PushService: Unregistering all ${_registeredTradePubkeys.length} tokens...'); // Copy the set to avoid modification during iteration final pubkeys = Set<String>.from(_registeredTradePubkeys); for (final tradePubkey in pubkeys) { try { await unregisterToken(tradePubkey); } catch (e) { _logger.e('Error unregistering token for $tradePubkey: $e'); + // Clear local state even on failure to allow clean re-registration + _registeredTradePubkeys.remove(tradePubkey); } } }
| ``` | ||
| ┌─────────────────────┬────────────┬─────────────────────────────────┐ | ||
| │ Ephemeral Pubkey │ Nonce │ Ciphertext │ | ||
| │ (33 bytes) │ (12 bytes) │ (220 + 16 = 236 bytes) │ | ||
| └─────────────────────┴────────────┴─────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (MD040).
Proposed fix
-```
+```text
┌─────────────────────┬────────────┬─────────────────────────────────┐
│ Ephemeral Pubkey │ Nonce │ Ciphertext │
│ (33 bytes) │ (12 bytes) │ (220 + 16 = 236 bytes) │
└─────────────────────┴────────────┴─────────────────────────────────┘
-```
+```
-```
+```text
┌──────────┬──────────────┬─────────────────┬───────────────────────┐
│ Platform │ Token Length │ Device Token │ Random Padding │
│ (1 byte) │ (2 bytes) │ (variable) │ (remainder) │
│ 0x01/02 │ big-endian │ UTF-8 string │ random bytes │
└──────────┴──────────────┴─────────────────┴───────────────────────┘
-```
+```Also applies to: 457-463
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
448-448: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/FCM_IMPLEMENTATION.md` around lines 448 - 453, Several fenced code
blocks in FCM_IMPLEMENTATION.md are missing language identifiers (MD040); update
the affected triple-backtick blocks (the diagram blocks around the Ephemeral
Pubkey/Nonce/Ciphertext and Platform/Token Length/Device Token/Random Padding
tables) to include a language hint like "text" by changing ``` to ```text so the
Markdown linter no longer flags them; locate the fenced blocks in the sections
referenced (the blocks shown in the diff with the ASCII table diagrams) and add
the identifier consistently for both occurrences.
Add NotificationSettingsScreen with support for push notification configuration:
Summary by CodeRabbit
New Features
Improvements
Localization
Style
Chore
✏️ Tip: You can customize this high-level summary in your review settings.