Fix: Improve subscription management and event handling #127
Conversation
… SubscriptionManager
WalkthroughThis update introduces a centralized subscription management system for Nostr events, refactors chat and order state management to use this system, and enhances data model validation and error handling throughout the codebase. It also updates UI localization keys, button logic, and background service behavior, while removing outdated or redundant notifiers and models. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SubscriptionManager
participant NostrService
participant MostroService
participant ChatRoomNotifier
participant OrderNotifier
App->>SubscriptionManager: Initialize (via Provider)
SubscriptionManager->>NostrService: Subscribe to events (orders/chat)
NostrService-->>SubscriptionManager: Stream<NostrEvent>
SubscriptionManager-->>MostroService: Stream orders events
SubscriptionManager-->>ChatRoomNotifier: Stream chat events
MostroService->>OrderNotifier: Update order state on event
ChatRoomNotifier->>ChatRoomNotifier: Update chat state on event
App->>SubscriptionManager: On app foreground/background, subscribe/unsubscribe all
Estimated code review effort4 (~90 minutes) Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
lib/shared/notifiers/session_notifier.dart (1)
43-62: Good timer management for periodic cleanup.The implementation correctly cancels existing timers and handles session expiration. Consider extracting the expiration logic to reduce duplication between
init()and_cleanup().lib/features/subscriptions/subscription_manager.dart (3)
21-21: Consider usinglate finalfor_sessionListenerSince
_sessionListeneris initialized in_initSessionListenerwhich is called from the constructor, it could be declared aslate finalinstead of nullable to avoid null checks.- ProviderSubscription? _sessionListener; + late final ProviderSubscription _sessionListener;Then update the dispose method:
- if (_sessionListener != null) { - _sessionListener!.close(); - _sessionListener = null; - } + _sessionListener.close();
38-48: Consider usingfireImmediatelyfor initial state handlingThe session listener doesn't handle the initial state. Consider using
fireImmediately: trueto process current sessions immediately when the listener is set up._sessionListener = ref.listen<List<Session>>( sessionNotifierProvider, (previous, current) { _logger.i('Sessions changed, updating subscriptions'); _updateAllSubscriptions(current); }, + fireImmediately: true, onError: (error, stackTrace) {This would eliminate the need for the manual initial update on lines 51-52.
101-101: Improve logging output for subscriptionsLogging the entire
_subscriptionsmap directly may not provide readable output. Consider logging more specific information.- _logger.i('Updated $type subscription with $_subscriptions'); + _logger.i('Updated $type subscription. Active subscriptions: ${_subscriptions.keys.toList()}');integration_test/test_helpers.dart (1)
320-328: Complete the implementation of unsubscribe and dispose methodsThe
unsubscribeanddisposemethods have TODO comments indicating incomplete implementation. These should be implemented even if they're no-ops for testing.Would you like me to help implement these methods or create an issue to track this task?
@override void unsubscribe(Session session) { - // TODO: implement unsubscribe + // No-op for fake service } @override void dispose() { - // TODO: implement dispose + // No-op for fake service }lib/services/mostro_service.dart (1)
40-61: Consider adding @deprecated annotations to backward compatibility methods.These methods are kept for backward compatibility but don't perform any actions. To make the API clearer, consider adding
@deprecatedannotations with migration instructions.+ @Deprecated('SubscriptionManager now automatically handles subscriptions. This method will be removed in a future version.') void subscribe(Session session) { _logger.i('Manual subscription not needed for: ${session.tradeKey.public}'); // No action needed - SubscriptionManager handles subscriptions automatically } + @Deprecated('SubscriptionManager now automatically handles subscriptions. This method will be removed in a future version.') void unsubscribe(Session session) { _logger .i('Manual unsubscription not needed for: ${session.tradeKey.public}'); // No action needed - SubscriptionManager handles subscriptions automatically }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
integration_test/test_helpers.dart(6 hunks)lib/core/config.dart(1 hunks)lib/data/repositories/mostro_storage.dart(2 hunks)lib/features/chat/notifiers/chat_room_notifier.dart(3 hunks)lib/features/chat/notifiers/chat_rooms_notifier.dart(4 hunks)lib/features/chat/providers/chat_room_providers.dart(1 hunks)lib/features/chat/screens/chat_rooms_list.dart(2 hunks)lib/features/order/models/order_state.dart(6 hunks)lib/features/order/notfiers/abstract_mostro_notifier.dart(7 hunks)lib/features/order/notfiers/add_order_notifier.dart(3 hunks)lib/features/order/notfiers/order_notifier.dart(1 hunks)lib/features/order/screens/add_order_screen.dart(1 hunks)lib/features/subscriptions/subscription.dart(1 hunks)lib/features/subscriptions/subscription_manager.dart(1 hunks)lib/features/subscriptions/subscription_manager_provider.dart(1 hunks)lib/features/subscriptions/subscription_type.dart(1 hunks)lib/features/trades/screens/trade_detail_screen.dart(2 hunks)lib/features/trades/widgets/mostro_message_detail_widget.dart(0 hunks)lib/main.dart(1 hunks)lib/services/lifecycle_manager.dart(4 hunks)lib/services/mostro_service.dart(3 hunks)lib/services/nostr_service.dart(1 hunks)lib/shared/notifiers/order_action_notifier.dart(0 hunks)lib/shared/notifiers/session_notifier.dart(2 hunks)lib/shared/providers/app_init_provider.dart(2 hunks)lib/shared/providers/mostro_service_provider.dart(2 hunks)lib/shared/providers/mostro_service_provider.g.dart(1 hunks)lib/shared/providers/session_notifier_provider.dart(1 hunks)lib/shared/utils/notification_permission_helper.dart(1 hunks)test/mocks.dart(5 hunks)test/mocks.mocks.dart(15 hunks)test/notifiers/add_order_notifier_test.dart(2 hunks)test/notifiers/take_order_notifier_test.dart(6 hunks)test/services/mostro_service_test.dart(7 hunks)test/services/mostro_service_test.mocks.dart(26 hunks)
💤 Files with no reviewable changes (2)
- lib/features/trades/widgets/mostro_message_detail_widget.dart
- lib/shared/notifiers/order_action_notifier.dart
🧰 Additional context used
🧠 Learnings (3)
lib/shared/providers/mostro_service_provider.g.dart (1)
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
lib/shared/providers/mostro_service_provider.dart (2)
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `@Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
lib/features/order/notfiers/abstract_mostro_notifier.dart (1)
Learnt from: chebizarro
PR: MostroP2P/mobile#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.
🔇 Additional comments (49)
lib/features/subscriptions/subscription_type.dart (1)
1-4: Clean enum implementation for subscription categorization.The
SubscriptionTypeenum is well-defined with clear, domain-specific values that align with the app's subscription management needs. This provides type safety for the centralized subscription architecture.lib/core/config.dart (1)
31-34: Good practice: Converting to immutable constants and adding session management config.The changes improve configuration management by:
- Making values immutable with
const(good practice)- Adding reasonable defaults for cleanup intervals (30 min) and session expiration (36 hours)
- Supporting centralized session lifecycle management
lib/shared/providers/mostro_service_provider.g.dart (1)
25-25: Expected hash update for generated code.The hash change reflects the underlying refactoring of
mostroServiceProviderimplementation. This is expected behavior for generated Riverpod provider code.lib/main.dart (1)
19-19: Good refactoring: Simplifying main.dart initialization.Moving the platform and environment checking logic into
requestNotificationPermissionIfNeeded()itself is a good architectural decision. This makes the main initialization cleaner and properly separates concerns.lib/features/subscriptions/subscription_manager_provider.dart (1)
1-7: Clean provider implementation for centralized subscription management.The
subscriptionManagerProvideris well-implemented, following Riverpod best practices by passing therefto enable theSubscriptionManagerto access other providers. This supports the architectural goal of centralizing subscription handling.lib/features/chat/screens/chat_rooms_list.dart (2)
8-8: Good architectural improvement - centralizing data accessThe shift from session-based to order-based data access aligns well with the centralized subscription management approach described in the PR objectives.
76-77: Verify null safety for peer accessThe change from
orderState.session!.peer!.publicKeytoorderState.peer!.publicKeysimplifies the data access path, but please ensure thatorderState.peercannot be null in this context to avoid potential runtime exceptions.#!/bin/bash # Description: Check if OrderState.peer can be null and verify null safety handling # Search for OrderState model definition and peer field ast-grep --pattern 'class OrderState { $$$ peer $$$ }' # Search for any null checks on peer in other parts of the codebase rg -A 3 -B 3 "\.peer(\s*==\s*null|\s*!=\s*null|\?)"lib/features/order/notfiers/order_notifier.dart (1)
18-20: Verify that removed filtering and sorting logic is not neededThe sync method was significantly simplified by removing message filtering (cantDo actions), sorting by timestamp, and early return for empty messages. Please confirm that:
- Filtering out
cantDoactions is no longer needed or handled elsewhere- Message ordering doesn't affect state updates
- The performance optimization for empty messages is not required
#!/bin/bash # Description: Verify if message filtering and sorting logic is handled elsewhere # Check if cantDo action filtering is done in updateWith method or elsewhere ast-grep --pattern 'updateWith($msg) { $$$ }' # Check for any cantDo action handling rg -A 5 -B 5 "cantDo|Action\.cantDo" # Check if message ordering is important in state updates rg -A 10 -B 5 "timestamp.*sort|sort.*timestamp"lib/data/repositories/mostro_storage.dart (2)
17-17: Good optimization - prevents duplicate message insertsAdding the duplicate check prevents unnecessary database operations and potential conflicts when the same message is processed multiple times. This aligns well with the deduplication logic mentioned in the PR objectives.
171-180: Enhanced query with proper sorting and limitingThe improvements to
watchByRequestIdwith sorting by timestamp, limiting to 1 result, and robust stream mapping enhance the reliability of retrieving the latest message. The explicit null handling in the stream mapping is a good defensive programming practice.lib/features/chat/providers/chat_room_providers.dart (1)
9-9: Approve dependency simplificationRemoving the SessionNotifier dependency aligns with the centralized subscription management approach and simplifies the provider construction. This change supports the architectural goal of decoupling session management from chat room state management.
#!/bin/bash # Description: Verify ChatRoomsNotifier constructor accepts only ref parameter # Check ChatRoomsNotifier constructor signature ast-grep --pattern 'class ChatRoomsNotifier { $$$ ChatRoomsNotifier($$$) { $$$ } $$$ }' # Verify no references to sessionNotifier in ChatRoomsNotifier rg -A 3 -B 3 "sessionNotifier" lib/features/chat/notifiers/chat_rooms_notifier.dartlib/shared/utils/notification_permission_helper.dart (2)
1-1: Appropriate import for platform detectionAdding the
dart:ioimport enables platform-specific behavior, which is necessary for the Android-only notification permission logic.
6-11: Excellent platform-specific and test-aware implementationThe addition of platform guards (
Platform.isAndroid) and test environment detection (!Platform.environment.containsKey('FLUTTER_TEST')) is a best practice that:
- Prevents unnecessary permission requests on iOS where they're handled differently
- Avoids test failures when running Flutter tests
- Maintains clean separation of platform-specific concerns
This change enhances the robustness of the notification permission handling.
test/notifiers/take_order_notifier_test.dart (2)
35-35: LGTM! Test setup correctly adapted to new dependency injection pattern.The addition of
MockRefand its usage in theMockSessionNotifierconstructor properly reflects the architectural changes whereSessionNotifiernow receives dependencies through Riverpod'sRefinstead of direct injection.Also applies to: 47-47, 52-52
131-131: Good practice: Standardized mostroPublicKey across tests.The consistent
mostroPublicKeyvalue across all test configurations improves maintainability and reduces test setup complexity.Also applies to: 195-195, 258-258, 307-307
lib/shared/providers/session_notifier_provider.dart (2)
3-3: Import update aligns with dependency injection refactoring.The change from importing
key_manager_provider.darttosettings.dartcorrectly reflects the new pattern where dependencies are accessed through theRefobject.
14-24: Excellent reactive pattern implementation.The SessionNotifier construction now uses
reffor dependency access, and the settings listener ensures automatic updates when settings change. This pattern maintains consistency across the application and supports the centralized subscription management architecture.lib/features/order/notfiers/add_order_notifier.dart (2)
54-54: Improved state management using updateWith method.The replacement of direct
OrderStateconstruction withstate.updateWith(message)aligns with the centralized state management improvements and ensures consistent state handling across the application.Also applies to: 78-78
22-24: Verify request ID uniqueness with timestamp-based generation.The change from XORing UUID segments to using
DateTime.now().microsecondsSinceEpochcould potentially create collisions if multiple orders are created in rapid succession, especially in high-frequency scenarios.Consider whether microsecond precision provides sufficient uniqueness for your use case. If collision risk is a concern, consider adding additional entropy or reverting to the UUID-based approach.
#!/bin/bash # Search for other request ID generation patterns in the codebase ast-grep --pattern '_requestIdFromOrderId($_) { $$$ }' # Check if there are other timestamp-based ID generators rg -A 5 "microsecondsSinceEpoch"lib/features/order/screens/add_order_screen.dart (1)
288-299: Good sanitization implementation for payment method input.The sanitization logic properly handles problematic characters that could cause issues with NIP-69 formatting. The approach of replacing problematic characters with spaces and normalizing whitespace is sound.
Consider documenting the specific characters being sanitized for future maintenance:
+ // Sanitize payment method to comply with NIP-69 formatting + // Remove characters that could break JSON parsing: , " \ [ ] { } final problematicChars = RegExp(r'[,"\\\[\]{}]');lib/features/trades/screens/trade_detail_screen.dart (2)
389-391: Enhanced user communication with sendDm action.Adding the
sendDmaction case to display a contact button improves user experience by providing clear communication options during trade interactions.
452-452: Improved navigation determinism.Changing from
context.pop()tocontext.go('/order_book')provides more predictable navigation behavior, especially useful for deep-linked scenarios and ensures users always land on the expected screen.test/notifiers/add_order_notifier_test.dart (2)
36-36: LGTM: Proper mock setup for the new Ref dependency.The addition of
MockRefaligns with the architectural change whereSessionNotifiernow takes aRefparameter for accessing providers instead of direct dependencies.
50-53: LGTM: Test setup correctly updated for new constructor signature.The changes properly update the test setup to use
MockSettingsand include theMockRefas the first parameter in theMockSessionNotifierconstructor, matching the refactoredSessionNotifierimplementation.lib/shared/providers/mostro_service_provider.dart (1)
18-29: LGTM: Excellent lifecycle management and reactive settings handling.The refactored provider properly implements:
- Explicit service initialization
- Reactive settings updates via listener
- Proper disposal cleanup
This aligns well with the centralized subscription management architecture and follows Riverpod best practices for provider lifecycle management.
lib/shared/providers/app_init_provider.dart (2)
23-23: LGTM: Proper initialization of the centralized subscription manager.The subscription manager initialization ensures that the centralized subscription handling is available during app startup.
29-39: LGTM: Simplified and improved session filtering logic.The changes represent a significant improvement:
- Uses configurable
Config.sessionExpirationHoursinstead of hardcoded 24-hour cutoff- Simplified logic delegates complex subscription management to the centralized
SubscriptionManager- Cleaner, more maintainable code structure
This aligns well with the architectural goal of centralizing subscription management.
lib/features/subscriptions/subscription.dart (1)
4-36: LGTM: Well-designed subscription encapsulation class.The
Subscriptionclass demonstrates excellent design principles:
- Clean encapsulation of subscription state and lifecycle
- Proper resource cleanup via
cancel()method- Immutability support with
copyWith()- Clear separation of concerns with
onCancelcallbackThis provides a solid foundation for the centralized subscription management system.
lib/features/order/models/order_state.dart (3)
89-89: LGTM: Fixed cantDo action handling.The fix properly updates the
cantDofield instead of returning the unchanged state, ensuring the UI can display cantDo information correctly.
108-122: LGTM: Comprehensive peer extraction logic.The enhanced peer extraction logic properly handles different payload types:
- Direct
Peerpayload extraction- Extracting peer from
Orderpayload via trade pubkeys- Preserving existing peer when no new peer is found
This ensures accurate peer information for order state management.
236-236: LGTM: Added sendDm action support for enhanced messaging.The addition of
Action.sendDmto the action maps for both buyer and seller roles in active states enables direct messaging functionality, enhancing communication capabilities during active trades.Also applies to: 303-303, 313-313
lib/services/lifecycle_manager.dart (4)
8-8: Good architectural improvement with centralized subscription management.The refactoring to use a centralized
SubscriptionManageris a solid design decision that improves modularity and reduces code duplication across the application.Also applies to: 13-13
61-62: LGTM! Clean delegation to SubscriptionManager.The foreground transition correctly delegates subscription reactivation to the centralized manager.
90-115: Well-implemented background transition with proper filter collection.The method correctly:
- Iterates through all subscription types
- Collects active filters
- Only transitions to background if there are active subscriptions
- Provides comprehensive logging
123-127: Good deprecation handling with clear migration path.The deprecated method properly logs a warning directing users to the new SubscriptionManager approach.
lib/features/chat/notifiers/chat_room_notifier.dart (3)
16-19: Proper subscription lifecycle management in reload.Good use of null-aware cancellation to prevent memory leaks when reloading.
49-82: Well-structured event handling with proper error management.The
_onChatEventmethod demonstrates good practices:
- Comprehensive null safety checks
- Efficient deduplication using Map
- Proper error logging with stack traces
- Chronological message sorting
106-108: Proper resource cleanup in dispose.Good practice to cancel subscriptions and log disposal for debugging.
lib/features/order/notfiers/abstract_mostro_notifier.dart (2)
97-97: Improved navigation with context-specific routes.The navigation changes provide better user flow by directing to appropriate trade detail pages based on the order action.
Also applies to: 159-159, 162-162, 174-174, 201-201
142-144: Good null safety improvement for Peer creation.The conditional check prevents creating invalid Peer objects with null public keys.
lib/shared/notifiers/session_notifier.dart (3)
27-42: Well-implemented session initialization with expiration handling.The init method properly:
- Removes expired sessions from storage and memory
- Uses configurable expiration time
- Updates state to reflect cleaned sessions
- Schedules ongoing cleanup
70-72: Clean dependency injection using Ref.The refactoring to use
ref.read(keyManagerProvider)follows Riverpod best practices and improves testability.
157-157: Proper timer cleanup in dispose.Good practice to cancel the cleanup timer to prevent resource leaks.
test/services/mostro_service_test.dart (2)
125-129: Good test hygiene with tearDown.Proper resource cleanup prevents test interference and memory leaks.
184-186: Clean mock simplification aligned with architecture.The reduction to only mocking
publishEventreflects the proper separation of concerns with static methods moved toNostrUtils.Also applies to: 243-245
lib/features/chat/notifiers/chat_rooms_notifier.dart (1)
119-126: Uncomment the loadChats() callThe
loadChats()call is commented out in_refreshAllSubscriptions, which may be needed to refresh the chat list when subscriptions are refreshed.void _refreshAllSubscriptions() { // No need to manually refresh subscriptions // SubscriptionManager now handles this automatically based on SessionNotifier changes _logger.i('Subscription management is now handled by SubscriptionManager'); // Just reload the chat rooms from the current sessions - //loadChats(); + loadChats(); }Please verify if this call is still needed or if it can be removed entirely along with the method.
lib/services/nostr_service.dart (1)
16-17: Clean refactoring of NostrService.The removal of cryptographic utility wrapper methods and the simplified constructor align well with the architectural changes to centralize subscription management. The core relay management functionality remains intact.
test/services/mostro_service_test.mocks.dart (1)
1-4: Generated mocks correctly reflect implementation changes.The Mockito-generated mocks properly align with the refactored NostrService and SessionNotifier implementations, including the removal of cryptographic methods and addition of the ref getter.
test/mocks.mocks.dart (1)
8-8: Comprehensive mock updates support new architecture.The generated mocks properly include the new SubscriptionManager and updated service interfaces. The consolidated dart_nostr import and new mock classes align perfectly with the centralized subscription management architecture.
Also applies to: 1788-1927
| ChatRoomsNotifier(this.ref) : super(const []) { | ||
| loadChats(); | ||
| //_setupChatSubscription(); | ||
| } |
There was a problem hiding this comment.
Critical: Chat subscription is not initialized
The _setupChatSubscription() method is commented out in the constructor, which means the chat subscription is never set up and chat events won't be received.
ChatRoomsNotifier(this.ref) : super(const []) {
loadChats();
- //_setupChatSubscription();
+ _setupChatSubscription();
}📝 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.
| ChatRoomsNotifier(this.ref) : super(const []) { | |
| loadChats(); | |
| //_setupChatSubscription(); | |
| } | |
| ChatRoomsNotifier(this.ref) : super(const []) { | |
| loadChats(); | |
| _setupChatSubscription(); | |
| } |
🤖 Prompt for AI Agents
In lib/features/chat/notifiers/chat_rooms_notifier.dart around lines 19 to 22,
the _setupChatSubscription() method call is commented out in the constructor,
preventing chat subscriptions from being initialized and chat events from being
received. Uncomment the _setupChatSubscription() call in the constructor to
ensure the chat subscription is properly set up when the ChatRoomsNotifier is
instantiated.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
lib/features/subscriptions/subscription_manager.dart (2)
152-156: Consider adding subscription type validation.The code cancels existing subscriptions before creating new ones, but there's no validation that the subscription type is being replaced correctly. While this works, adding a debug log could help with troubleshooting.
if (_subscriptions.containsKey(type)) { + _logger.d('Replacing existing $type subscription'); _subscriptions[type]!.cancel(); }
186-188: Remove redundant method delegation.The
unsubscribeSessionmethod is just delegating tounsubscribeByTypewithout adding any value. This creates unnecessary API surface area and potential confusion.Consider removing this method entirely and using
unsubscribeByTypedirectly, or add specific session-based logic if needed:- void unsubscribeSession(SubscriptionType type) { - unsubscribeByType(type); - }test/mocks.dart (1)
151-155: Simplify mock dispose method.The mock calls
super.dispose()but this is unnecessary in a mock and could potentially cause issues if the parent dispose method tries to access real dependencies.@override void dispose() { _ordersController.close(); _chatController.close(); - super.dispose(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/features/chat/notifiers/chat_room_notifier.dart(3 hunks)lib/features/order/notfiers/add_order_notifier.dart(3 hunks)lib/features/order/notfiers/order_notifier.dart(1 hunks)lib/features/subscriptions/subscription_manager.dart(1 hunks)lib/services/mostro_service.dart(3 hunks)test/mocks.dart(5 hunks)test/services/mostro_service_test.dart(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/features/order/notfiers/order_notifier.dart
- lib/features/order/notfiers/add_order_notifier.dart
- lib/features/chat/notifiers/chat_room_notifier.dart
- lib/services/mostro_service.dart
- test/services/mostro_service_test.dart
🔇 Additional comments (2)
lib/features/subscriptions/subscription_manager.dart (2)
1-31: Well-structured class initialization with proper dependencies.The class setup follows good practices with dependency injection via
Ref, proper stream controller initialization, and immediate session listener setup.
33-45: Robust session listener implementation.The session listener setup includes proper error handling and uses
fireImmediately: trueto ensure subscriptions are initialized on startup. The error logging provides good debugging capability.
…iption ID parameter
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/mocks.dart (1)
85-85: Fix undefined MockRef dependency.The constructor calls
super(MockRef())butMockRefmay not be properly imported or defined, which could cause compilation errors.Ensure that
MockRefis properly generated in the@GenerateMocksannotation (line 32) and imported frommocks.mocks.dart. The mock should be generated automatically when you rundart run build_runner build.
🧹 Nitpick comments (3)
lib/features/subscriptions/subscription_manager.dart (2)
185-187: Consider removing redundant wrapper method.The
unsubscribeSessionmethod is just a wrapper aroundunsubscribeByTypewithout adding any value. This could be confusing as the method name suggests session-specific unsubscription but it unsubscribes the entire type.Consider removing this method or making it more explicit:
- void unsubscribeSession(SubscriptionType type) { - unsubscribeByType(type); - }
54-56: Verify performance impact of subscribing to all types.The current implementation subscribes to all
SubscriptionType.valueswhenever sessions change, even if some subscription types aren't actively being used. This could create unnecessary network traffic.Consider implementing on-demand subscription or tracking which types are actually needed:
void _updateAllSubscriptions(List<Session> sessions) { if (sessions.isEmpty) { _logger.i('No sessions available, clearing all subscriptions'); _clearAllSubscriptions(); return; } - for (final type in SubscriptionType.values) { + for (final type in _getActiveSubscriptionTypes()) { _updateSubscription(type, sessions); } }test/mocks.dart (1)
104-108: Improve mock subscription for better test control.The current mock creates a subscription with an empty stream that doesn't allow test control over event simulation.
Consider making the stream subscription more controllable for testing:
final subscription = Subscription( request: request, - streamSubscription: const Stream<NostrEvent>.empty().listen((_) {}), + streamSubscription: (type == SubscriptionType.orders ? _ordersController.stream : _chatController.stream).listen((_) {}), onCancel: () {}, );This would allow the mock to properly simulate real subscription behavior where events flow through the same streams that are returned by the
subscribemethod.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/features/subscriptions/subscription_manager.dart(1 hunks)test/mocks.dart(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.502Z
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.
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/background/background.dart:74-77
Timestamp: 2025-05-08T16:29:52.154Z
Learning: In the Mostro Mobile background service architecture, events aren't stored by the background process. Instead, the background service only checks if events exist in the eventStore and sends notifications for new ones, while the foreground process is responsible for storing and processing events. This is an intentional design decision to separate concerns.
lib/features/subscriptions/subscription_manager.dart (5)
Learnt from: chebizarro
PR: MostroP2P/mobile#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.
Learnt from: chebizarro
PR: MostroP2P/mobile#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.
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `@Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.502Z
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.
test/mocks.dart (4)
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `@Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Learnt from: chebizarro
PR: MostroP2P/mobile#110
File: test/notifiers/take_order_notifier_test.dart:72-74
Timestamp: 2025-06-04T19:35:20.209Z
Learning: MostroService methods like takeBuyOrder() and takeSellOrder() return Future<void> and trigger side effects through other mechanisms rather than direct return values. When testing these methods, focus on verifying method calls and testing state changes through the provider system rather than mocking return values.
Learnt from: chebizarro
PR: MostroP2P/mobile#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.
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
🔇 Additional comments (6)
lib/features/subscriptions/subscription_manager.dart (3)
89-106: LGTM: Chat filter logic has been corrected.The filter creation properly handles the inconsistency mentioned in previous reviews by filtering sessions where
sharedKey?.public != nulland then safely mapping tosharedKey!.public.
17-31: Solid centralized subscription management design.The class structure properly encapsulates subscription state with broadcast streams for orders and chat events, and automatically manages subscriptions based on session changes. The initialization with session listener setup is well-designed.
151-155: Proper subscription lifecycle management.The code correctly cancels existing subscriptions before creating new ones, preventing subscription leaks and ensuring only one active subscription per type.
test/mocks.dart (3)
96-113: Mock subscribe method signature is now correct.The method signature properly matches the real implementation without the extra
idparameter that was flagged in previous reviews.
135-137: Mock hasActiveSubscription method is now correct.The method signature and implementation properly match the real implementation without the unused
idparameter that was flagged in previous reviews.
140-146: Good test helper method for event simulation.The
addEventhelper method provides a clean way to simulate events in tests, properly routing them to the appropriate stream controllers.
…g and remove unused methods
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
Hi @chebizarro I'm getting tons of notifications at once, but I only posted two orders that haven't been taken.
If I clear the notifications and back into the app, when I exit the app, all 25 notifications come back at once.
…ectly (Fixes: Error displaying the date and time of order creation #83)
|
Hi @chebizarro please rebase and fix the conflicts |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.gitignore (1)
57-57: Consider unifying the pattern for all platformsYou broadened the ignore rule to
generated_plugin_registrant.*for Windows (👍).
For consistency, you might want to:
- Apply the same wildcard rule for macOS (
macos/Flutter/GeneratedPluginRegistrant.*) so every platform follows the same convention.- Remove the now-redundant platform-specific extension comment to keep the file shorter.
Not critical, but it keeps the ignore rules symmetrical and easier to scan.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)lib/features/chat/screens/chat_rooms_list.dart(2 hunks)lib/features/order/models/order_state.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/chat/screens/chat_rooms_list.dart
- lib/features/order/models/order_state.dart
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/background/background.dart:74-77
Timestamp: 2025-05-08T16:29:52.154Z
Learning: In the Mostro Mobile background service architecture, events aren't stored by the background process. Instead, the background service only checks if events exist in the eventStore and sends notifications for new ones, while the foreground process is responsible for storing and processing events. This is an intentional design decision to separate concerns.
🔇 Additional comments (1)
.gitignore (1)
63-63: Double-check that no hand-written source files share this prefixThe new glob will ignore any file named
generated_plugin_registrant.*underlinux/flutter/.
If you ever check in a custom shim (e.g.generated_plugin_registrant_manual.cc) it will also be ignored. Verify this is acceptable for your workflow.
|
@chebizarro siguen saliendo el montón de notificaciones a la vez, esta vez salieron 50 notificaciones a los pocos segundos de salir de la app despues de crear una orden (solo fui para otra app, pero no cerré la de mostro) |
|
This works for me, any remaining issues with notifications, which are not the focus of this issue or PR should be handled in a separate issue |
This PR addresses several related issues:
Fixes #57: App requiring restart to update order status and chat messages
Addresses #54: Duplicate notifications for already completed orders
Addresses #70: Automatic screen transition after hold invoice payment
Additional improvements:
These changes significantly improve the app's reliability by preventing subscription limit issues, enhancing responsiveness to real-time updates, and providing a more seamless user experience during order flows.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Chores
.gitignoreand analysis options for better development workflow.Tests