Refactor: State Management Overhaul (#77) #110
Conversation
…igrate to new SessionNotifier
… with request IDs
… order state management
WalkthroughThis change refactors order and trade state management by introducing a unified Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant OrderNotifier
participant SessionNotifier
participant MostroService
participant Storage
UI->>OrderNotifier: User initiates order action
OrderNotifier->>SessionNotifier: Create/lookup session (by orderId/requestId)
OrderNotifier->>MostroService: Submit/take order
MostroService->>Storage: Store message
MostroService-->>OrderNotifier: Emits message (stream)
OrderNotifier->>OrderState: Update with new message
OrderNotifier-->>UI: Notifies state change
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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: 19
🔭 Outside diff range comments (2)
lib/features/key_manager/key_management_screen.dart (1)
56-57:⚠️ Potential issueFix the typo in variable name.
There's a typo in the variable name
sessionNotifer- it should besessionNotifier.Apply this diff to fix the typo:
- final sessionNotifer = ref.read(sessionNotifierProvider.notifier); - await sessionNotifer.reset(); + final sessionNotifier = ref.read(sessionNotifierProvider.notifier); + await sessionNotifier.reset();lib/features/order/notfiers/order_notifier.dart (1)
44-57: 🛠️ Refactor suggestionAdd error handling for session creation
Session creation could fail but there's no error handling. This could cause subsequent operations to fail unexpectedly.
Future<void> takeSellOrder( String orderId, int? amount, String? lnAddress) async { + try { final sessionNotifier = ref.read(sessionNotifierProvider.notifier); session = await sessionNotifier.newSession( orderId: orderId, role: Role.buyer, ); mostroService.subscribe(session); await mostroService.takeSellOrder( orderId, amount, lnAddress, ); + } catch (e) { + // Propagate error to caller + rethrow; + } }
🧹 Nitpick comments (8)
lib/shared/notifiers/session_notifier.dart (1)
89-95: Simplify error handling in getSessionByRequestId.The method catches
StateErrorbut map access doesn't throwStateError- it returnsnullfor missing keys. The try-catch is unnecessary here.Session? getSessionByRequestId(int requestId) { - try { return _requestIdToSession[requestId]; - } on StateError { - return null; - } }lib/core/mostro_fsm.dart (1)
189-193: Consider improving documentation for null return cases.The method correctly returns nullable
Status?, but the documentation should clarify when null is returned (i.e., when the action is not valid for the given role and current status)./// Returns the next status for a given current status, role, and action. + /// Returns null if the action is not valid for the given role and status. static Status? nextStatus(Status current, Role role, Action action) { return _transitions[current]?[role]?[action]; }lib/features/order/models/order_state.dart (3)
82-96: Consider adding documentation for the complex update logic.The
updateWithmethod contains important business logic for state updates. Consider adding documentation to explain the special cases, particularly thecantDoaction handling and order extraction from PaymentRequest.+ /// Updates the current state with data from a new MostroMessage. + /// - Preserves current action if the new action is cantDo + /// - Extracts order from PaymentRequest if present + /// - Updates all other fields with new values if available OrderState updateWith(MostroMessage message) {
98-100: Consider adding validation for action lookups.The current implementation silently returns an empty list for invalid role/status/action combinations. Consider logging a warning or throwing an exception for invalid lookups during development to catch configuration errors early.
List<Action> getActions(Role role) { - return actions[role]![status]![action] ?? []; + final roleActions = actions[role]; + if (roleActions == null) { + // Log warning or throw in debug mode + return []; + } + final statusActions = roleActions[status]; + if (statusActions == null) { + // Log warning or throw in debug mode + return []; + } + return statusActions[action] ?? []; }
139-139: Remove extra blank line.Minor formatting issue.
Action.cooperativeCancelInitiatedByPeer: [ Action.cancel, ], - },lib/features/trades/screens/trade_detail_screen.dart (2)
365-368: Verify implementation for holdInvoicePaymentAccepted actionThis case only adds a contact button. Please verify if this is the intended behavior or if additional actions are needed.
Also remove the trailing whitespace on line 367:
case actions.Action.holdInvoicePaymentAccepted: widgets.add(_buildContactButton(context)); - break;
390-393: Consider logging unknown actions in default caseThe default case silently ignores unknown actions. Consider logging these for debugging purposes.
default: - // Optionally handle unknown or unimplemented actions + // Log unknown actions for debugging + debugPrint('Unknown action in trade detail: $action'); break;lib/features/order/notfiers/order_notifier.dart (1)
26-31: Improve null safety clarity in sync methodThe code checks
msg?.payload is Orderbut then force unwrapsmsg!. While this is safe due to the type check, it could be clearer.final msg = messages.firstWhereOrNull((m) => m.action != Action.cantDo); - if (msg?.payload is Order) { + if (msg != null && msg.payload is Order) { state = OrderState( - status: msg!.getPayload<Order>()!.status, + status: msg.getPayload<Order>()!.status, action: msg.action, - order: msg!.getPayload<Order>()!, + order: msg.getPayload<Order>()!, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
android/app/src/main/AndroidManifest.xml(1 hunks)lib/background/background.dart(0 hunks)lib/core/mostro_fsm.dart(1 hunks)lib/data/enums.dart(1 hunks)lib/data/models.dart(1 hunks)lib/data/repositories/base_storage.dart(1 hunks)lib/data/repositories/mostro_storage.dart(0 hunks)lib/features/chat/notifiers/chat_room_notifier.dart(1 hunks)lib/features/chat/notifiers/chat_rooms_notifier.dart(1 hunks)lib/features/chat/providers/chat_room_providers.dart(1 hunks)lib/features/chat/screens/chat_room_screen.dart(1 hunks)lib/features/chat/screens/chat_rooms_list.dart(1 hunks)lib/features/home/providers/home_order_providers.dart(1 hunks)lib/features/home/screens/home_screen.dart(3 hunks)lib/features/home/widgets/order_list_item.dart(6 hunks)lib/features/key_manager/key_management_screen.dart(1 hunks)lib/features/key_manager/key_manager_provider.dart(0 hunks)lib/features/key_manager/key_notifier.dart(0 hunks)lib/features/order/models/order_state.dart(1 hunks)lib/features/order/notfiers/abstract_mostro_notifier.dart(3 hunks)lib/features/order/notfiers/add_order_notifier.dart(4 hunks)lib/features/order/notfiers/cant_do_notifier.dart(0 hunks)lib/features/order/notfiers/dispute_notifier.dart(0 hunks)lib/features/order/notfiers/order_notifier.dart(3 hunks)lib/features/order/notfiers/payment_request_notifier.dart(0 hunks)lib/features/order/providers/order_notifier_provider.dart(2 hunks)lib/features/order/providers/order_status_provider.dart(0 hunks)lib/features/order/screens/pay_lightning_invoice_screen.dart(1 hunks)lib/features/order/screens/payment_confirmation_screen.dart(3 hunks)lib/features/order/screens/take_order_screen.dart(3 hunks)lib/features/order/widgets/action_button_config.dart(1 hunks)lib/features/trades/models/trade_state.dart(0 hunks)lib/features/trades/providers/trade_state_provider.dart(0 hunks)lib/features/trades/providers/trades_provider.dart(1 hunks)lib/features/trades/screens/trade_detail_screen.dart(7 hunks)lib/features/trades/widgets/mostro_message_detail_widget.dart(8 hunks)lib/features/trades/widgets/trades_list_item.dart(1 hunks)lib/services/mostro_service.dart(5 hunks)lib/shared/notifiers/session_notifier.dart(4 hunks)lib/shared/providers.dart(1 hunks)lib/shared/providers/app_init_provider.dart(2 hunks)lib/shared/providers/mostro_service_provider.dart(1 hunks)lib/shared/providers/session_providers.dart(0 hunks)lib/shared/utils/nostr_utils.dart(1 hunks)lib/shared/widgets/mostro_reactive_button.dart(2 hunks)test/mocks.mocks.dart(6 hunks)test/notifiers/add_order_notifier_test.dart(8 hunks)test/notifiers/take_order_notifier_test.dart(6 hunks)test/services/mostro_service_test.dart(2 hunks)test/services/mostro_service_test.mocks.dart(26 hunks)
💤 Files with no reviewable changes (11)
- lib/background/background.dart
- lib/features/order/notfiers/payment_request_notifier.dart
- lib/features/order/notfiers/dispute_notifier.dart
- lib/features/trades/providers/trade_state_provider.dart
- lib/features/order/providers/order_status_provider.dart
- lib/features/order/notfiers/cant_do_notifier.dart
- lib/data/repositories/mostro_storage.dart
- lib/features/key_manager/key_notifier.dart
- lib/features/trades/models/trade_state.dart
- lib/shared/providers/session_providers.dart
- lib/features/key_manager/key_manager_provider.dart
🧰 Additional context used
🧠 Learnings (2)
lib/shared/providers.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, `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#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.
🔇 Additional comments (55)
android/app/src/main/AndroidManifest.xml (1)
49-50: Consistent indentation for permission entries.These
<uses-permission>declarations have been realigned to match the surrounding entries. This is a cosmetic formatting change only, with no impact on functionality.lib/features/chat/screens/chat_rooms_list.dart (1)
10-10: Import updated to the new session notifier provider
The import has been switched tosession_notifier_provider.dart. Confirm that this module exportssessionProviderand that no lingering references to the oldsession_manager_provider.dartremain.lib/features/chat/notifiers/chat_room_notifier.dart (1)
11-11: Update import to session_notifier_provider.dart
The session management import has been migrated to the new notifier-based provider. Ensure thatsessionProvideris correctly exported here and that subsequent calls (e.g.,ref.read(sessionProvider(orderId))) work with the refactored API.lib/features/trades/widgets/trades_list_item.dart (1)
10-10: Replace session manager import with new notifier provider
The import now points tosession_notifier_provider.dartfor trade sessions. Verify thatsessionProvider(trade.orderId)resolves correctly and aligns with the new state management pattern.lib/features/chat/screens/chat_room_screen.dart (1)
13-13: Switch to session_notifier_provider.dart
The session provider import has been updated to the notifier-based module. Double-check thatref.read(sessionProvider(widget.orderId))continues to yield the expectedSessiondata under the new implementation.lib/features/chat/notifiers/chat_rooms_notifier.dart (1)
6-6: Add import for session_notifier_provider.dart
Added the new provider import to accesssessionNotifierProvider. Ensure thatref.read(sessionNotifierProvider.notifier).sessionscorrectly retrieves the active sessions list per the refactored provider API.lib/features/trades/providers/trades_provider.dart (1)
8-8: Clean migration to session notifier provider.The import and provider reference updates are consistent with the state management overhaul. The session data access patterns remain unchanged, indicating good API compatibility between the old session manager and new session notifier.
Also applies to: 14-14
lib/features/chat/providers/chat_room_providers.dart (1)
5-5: Consistent session provider migration.The import update aligns with the systematic migration from session manager to session notifier across the codebase. The provider usage remains unchanged, demonstrating good backward compatibility.
lib/features/home/providers/home_order_providers.dart (1)
7-7: Consistent session notifier migration pattern.The changes follow the same clean migration pattern seen across other providers. Session data access remains compatible, ensuring the order filtering logic continues to work correctly with the new session notifier architecture.
Also applies to: 14-14
lib/shared/providers/mostro_service_provider.dart (2)
5-5: Proper session notifier integration in service provider.The migration correctly accesses
sessionNotifierProvider.notifier, indicating the new session provider follows the StateNotifierProvider pattern. This is the appropriate way to inject the notifier instance into the MostroService constructor and aligns with Riverpod best practices.Also applies to: 17-17
1-24:Details
✅ Verification successful
Verify complete migration from session manager to session notifier.
The systematic migration looks good across all reviewed files. To ensure the refactor is complete and no stale references remain, please verify that all session manager usages have been updated.
🏁 Script executed:
#!/bin/bash # Description: Verify complete migration from session_manager_provider to session_notifier_provider # Expected: No remaining references to session_manager_provider in the codebase echo "Checking for remaining session_manager_provider imports..." rg "session_manager_provider" --type dart echo -e "\nChecking for remaining sessionManagerProvider usages..." rg "sessionManagerProvider" --type dart echo -e "\nVerifying session_notifier_provider is properly defined..." fd "session_notifier_provider.dart" --type fLength of output: 565
All session manager references removed; migration is complete.
No occurrences of
session_manager_providerorsessionManagerProviderwere found in the codebase, and the newsession_notifier_provider.dartexists at:
- lib/shared/providers/session_notifier_provider.dart
You can confidently proceed with this refactor.
lib/features/home/widgets/order_list_item.dart (1)
39-39: LGTM! Correct Flutter API update.The changes correctly update from the deprecated
withOpacity()method to the newerwithValues(alpha: alpha)API for setting color transparency. All modifications are consistent and maintain the same alpha values.Also applies to: 53-54, 73-73, 79-79, 171-171, 177-177, 215-215, 221-221, 275-275
lib/features/key_manager/key_management_screen.dart (1)
8-8: LGTM! Session provider import updated correctly.The import update from
session_manager_provider.darttosession_notifier_provider.dartaligns with the broader session management refactor described in the PR objectives.lib/shared/utils/nostr_utils.dart (1)
194-194: LGTM! Privacy enhancement through timestamp randomization.The change from
DateTime.now()torandomNow()for wrap event creation enhances privacy by randomizing event timestamps within a 48-hour window. This helps prevent timing correlation attacks while maintaining event functionality.lib/data/models.dart (1)
9-9: LGTM! Added peer model export.The addition of the
peer.dartexport supports the enhanced OrderState model and aligns with the model reorganization described in the PR objectives.lib/data/repositories/base_storage.dart (1)
79-81: LGTM! Improved data consistency by filtering deleted records.The addition of the deleted filter in
getAll()properly excludes logically deleted records, which aligns with the broader state management improvements and helps prevent inconsistencies from deleted data appearing in queries.test/notifiers/take_order_notifier_test.dart (1)
138-138: LGTM! Updated to use direct property access with OrderState.The change from
state.getPayload<Order>()tostate.ordercorrectly reflects the migration to the newOrderStatemodel where order data is directly accessible as a property.lib/features/order/screens/take_order_screen.dart (2)
155-157: LGTM! Improved reactive state management.Switching from
ref.readtoref.watchenables the UI to reactively update when the order notifier state changes, which is essential for the newOrderStatepattern and provides a better user experience.
164-164: LGTM! Better navigation behavior for close button.Using
context.pop()instead ofcontext.go('/')is more appropriate for a "CLOSE" button as it returns to the previous screen rather than navigating to the root, providing a more intuitive user experience.lib/features/order/screens/pay_lightning_invoice_screen.dart (1)
24-27: LGTM! Consistent migration to reactive OrderState pattern.The changes correctly implement the new state management approach:
- Using
ref.watchfor reactive updates to both the order state and notifier- Accessing payment request data directly from
order.paymentRequest?.lnInvoiceinstead of payload extraction- This aligns with the architectural shift from
MostroMessagetoOrderStateand improves UI reactivitytest/services/mostro_service_test.dart (3)
4-4: LGTM: Proper dependency injection refactorThe addition of
flutter_riverpodimport aligns with the shift to using Riverpod'sRefinterface for dependency injection.
19-19: LGTM: Updated mock generation for new architectureThe change from mocking
MostroStorageandSettingsto mockingRefproperly reflects the new dependency injection pattern where services receive aRefinterface instead of direct dependencies.
25-25: LGTM: Consistent mock setup and service instantiationThe mock setup and simplified
MostroServiceconstructor call correctly implement the new dependency injection pattern using Riverpod'sRefinterface.Also applies to: 32-33
lib/shared/providers.dart (1)
1-7: LGTM: Well-structured barrel file for provider centralizationThis barrel file properly centralizes provider exports, which improves import organization and aligns with the broader state management refactor. The exported providers cover the core functionality areas: services, session management, storage, repositories, navigation, and notifications.
test/notifiers/add_order_notifier_test.dart (4)
36-40: LGTM: Proper alignment with new service interfaceThe changes from
publishOrdertosubmitOrderand commenting out theMostroMessageusage correctly align with the newOrderStatemodel and updated service interface.
87-87: LGTM: Consistent use of addOrderNotifierProviderThe updates to use
addOrderNotifierProvidercorrectly reflect the new provider structure for order submission functionality.Also applies to: 154-154, 216-216, 278-278
96-96: LGTM: Correct property access for OrderStateThe change from
getPayload<Order>()to direct.orderproperty access correctly reflects the newOrderStatemodel structure.Also applies to: 160-160, 222-222, 284-284
71-71: LGTM: Updated provider overridesThe change from
mostroRepositoryProvidertomostroServiceProvidercorrectly aligns with the new centralized provider exports and service architecture.Also applies to: 137-137, 201-201, 262-262
lib/features/home/screens/home_screen.dart (2)
99-99: LGTM: Updated to modern Flutter color APIThe migration from
withOpacity()towithValues(alpha:)correctly uses the newer Flutter API, improving forward compatibility and following current best practices.Also applies to: 187-187, 219-219
190-190: LGTM: Consistent color API usageThe update to use
Colors.black.withValues(alpha: 0.2)maintains consistency with the other color API updates in this file.lib/shared/widgets/mostro_reactive_button.dart (1)
75-78:Details
❓ Verification inconclusive
Clarify the commented conditional logic and hybrid state approach.
The widget is now watching
OrderStatebut the conditional logic based onorderState.actionis commented out. This creates uncertainty about when the button should be rendered. Additionally, the widget uses a hybrid approach - watchingOrderStatebut listening toMostroMessagestream.Please clarify:
- Should the conditional logic on lines 76-78 be active or removed entirely?
- Is the hybrid approach of using both
OrderStateandMostroMessageintentional, or should theref.listenalso migrate to useOrderState?
🏁 Script executed:
#!/bin/bash # Description: Check if other reactive components fully migrated to OrderState or also use hybrid approach # Search for other widgets that might be using both OrderState and MostroMessage rg -A 5 -B 5 "orderNotifierProvider.*mostroMessageStreamProvider" # Search for similar reactive button patterns rg -A 10 "ref\.listen.*mostroMessageStreamProvider"Length of output: 121
🏁 Script executed:
#!/bin/bash # Find all occurrences of mostroMessageStreamProvider rg -n "mostroMessageStreamProvider" # Find all occurrences of orderNotifierProvider rg -n "orderNotifierProvider"Length of output: 5012
Clarify conditional rendering and stream handling in MostroReactiveButton
The
MostroReactiveButtonin lib/shared/widgets/mostro_reactive_button.dart (lines 75–78) currently watchesOrderStateviaorderNotifierProviderbut still subscribes tomostroMessageStreamProviderin initState, and its build-time guard onorderState.actionis commented out. This hybrid pattern isn’t used elsewhere.Please confirm:
- Should we restore the early-return on
orderState.action(uncomment lines 76–78) or remove that check entirely?- Is it intentional for this widget to subscribe directly to the message stream, or should message handling be moved into the
OrderNotifierso the widget only relies onOrderState?lib/features/order/screens/payment_confirmation_screen.dart (2)
5-5: LGTM: Clean migration to OrderState.The import change and parameter type update properly migrate from
MostroMessagetoOrderState, maintaining consistency with the broader refactoring.Also applies to: 39-39
107-110: LGTM: Improved error handling with OrderState.The updated error handling properly accesses the
cantDoproperty onOrderStateand extracts thecantDoReason, which is cleaner than the previous generic payload extraction approach.lib/data/enums.dart (1)
1-5: LGTM: Clean enum consolidation.This barrel file properly consolidates enum exports, which improves maintainability and provides a single import point for all enums. The file follows standard Dart conventions for barrel files.
lib/shared/providers/app_init_provider.dart (1)
14-14: LGTM: Consistent session provider migration.The import change from
session_manager_providertosession_notifier_provideris consistent with the broader refactoring across the codebase.lib/features/order/widgets/action_button_config.dart (1)
9-67: LGTM! Well-designed configuration class.The
MostroButtonConfigclass follows excellent Flutter/Dart practices with immutable design, comprehensive documentation, and a propercopyWithmethod. The configuration encapsulates all necessary button properties cleanly.lib/features/order/providers/order_notifier_provider.dart (2)
4-4: LGTM! Import aligns with state management refactoring.The addition of the
OrderStateimport supports the migration fromMostroMessageto the unifiedOrderStatemodel.
12-12: LGTM! Provider type updates are consistent.The state type changes from
MostroMessagetoOrderStatefor both providers align with the overall state management refactoring described in the PR objectives.Also applies to: 22-22
lib/services/mostro_service.dart (2)
100-101: LGTM! Simplified order publishing flow.The removal of session creation and subscription management from these methods aligns with the state management refactoring. The service now focuses on publishing orders rather than managing state.
Also applies to: 105-111, 126-132, 187-193
196-196: LGTM! Simplified return type.Changing the return type from
Future<Session>toFuture<void>simplifies the API since session management is now handled elsewhere.test/mocks.mocks.dart (1)
1-3: Auto-generated file looks correct.The mock updates appropriately reflect the changes in the service interfaces, particularly the
publishOrdermethod signature change and import alias corrections.lib/core/mostro_fsm.dart (3)
5-12: Good use of private constructor pattern.The private constructor prevents instantiation of this utility class, which is a good practice for classes containing only static members.
74-187: Well-structured role-based state transitions.The nested transition map provides clear role-based state management with good separation of concerns. The sectioning comments improve readability significantly.
194-198: Good addition of role-aware action retrieval.The
possibleActionsmethod is a valuable addition that allows querying valid actions based on current state and role, with proper null-safety.lib/features/order/models/order_state.dart (4)
4-22: Well-structured immutable state model.The
OrderStateclass properly encapsulates all order-related state with appropriate nullable fields and required parameters.
23-33: Factory constructor handles message conversion properly.Good use of null-aware operators and appropriate fallback to
Status.pendingwhen order status is not available.
39-60: Correct implementation of equality and hash code.The equals operator and hashCode properly include all fields, following Dart best practices for value objects.
62-80: Standard copyWith implementation for immutability.The method correctly allows selective field updates while maintaining immutability.
lib/features/order/notfiers/add_order_notifier.dart (3)
28-52: LGTM! Well-structured event handling.The subscribe method properly handles different message types and actions, with appropriate logging for debugging purposes.
54-65: Clean implementation of order confirmation flow.The method properly updates state, persists the session, sets up order monitoring, navigates to confirmation, and cleans up resources. The use of
ref.invalidateSelf()after confirmation is a good practice to prevent memory leaks.
67-86: Excellent session management and state handling.The method correctly establishes a session with appropriate role assignment, ensures subscription before order submission to prevent race conditions, and properly updates the state to reflect the pending status.
lib/features/order/notfiers/abstract_mostro_notifier.dart (3)
11-30: Good architectural improvements with the OrderState model.The migration to
OrderStateprovides a more structured approach to state management. The session retrieval in the constructor ensures proper state restoration for existing orders.
81-87: Excellent cleanup implementation for order cancellation.The canceled action properly handles all cleanup tasks: deleting messages from storage, removing the session, navigating away, showing notification, and invalidating the notifier. This ensures no stale data remains after cancellation.
88-116: Well-implemented cooperative cancellation and dispute handling.The new action handlers properly extract relevant data from payloads and provide appropriate user notifications. The inclusion of dispute IDs in notifications will help users track their disputes effectively.
test/services/mostro_service_test.mocks.dart (1)
1-856: Generated mock file - no manual changes needed.This is an auto-generated mock file that correctly reflects the architectural changes in the codebase, particularly the shift from storage-based to Riverpod-based state management. The mock updates are appropriate and necessary for testing.
| when(mockMostroService.takeSellOrder(any, any, any)).thenAnswer((_) async { | ||
| final msg = MostroMessage.fromJson(confirmationJsonTakeSell); | ||
| return Stream.value(msg); | ||
| //final msg = MostroMessage.fromJson(confirmationJsonTakeSell); | ||
| //return Stream.value(msg); | ||
| }); |
There was a problem hiding this comment.
Critical: Update all commented mock implementations.
Similar to the takeBuyOrder test, this mock implementation is commented out and needs to be updated for the new state management pattern.
🤖 Prompt for AI Agents
In test/notifiers/take_order_notifier_test.dart around lines 119 to 122, the
mock implementation for mockMostroService.takeSellOrder is commented out and
outdated due to the new state management pattern. Update this mock to return a
Stream of the appropriate MostroMessage object using the new pattern, similar to
how takeBuyOrder is mocked. Ensure the mock returns a valid Stream that matches
the expected behavior in the test.
| //final msg = MostroMessage.fromJson(confirmationJsonTakeBuy); | ||
| //return Stream.value(msg); | ||
| }); |
There was a problem hiding this comment.
Critical: Commented out mock implementations will cause test failures.
The mock return values are commented out, which means these tests are not properly stubbing the service methods. This will likely cause the tests to fail or produce unreliable results.
Uncomment and update the mock implementations to work with the new OrderState pattern:
- when(mockMostroService.takeBuyOrder(any, any)).thenAnswer((_) async {
- //final msg = MostroMessage.fromJson(confirmationJsonTakeBuy);
- //return Stream.value(msg);
- });
+ when(mockMostroService.takeBuyOrder(any, any)).thenAnswer((_) async {
+ // Return appropriate OrderState or Stream<OrderState> based on new signature
+ // This needs to be updated based on the actual service method signature
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/notifiers/take_order_notifier_test.dart around lines 72 to 74, the mock
return values are commented out, causing the tests to fail or behave unreliably.
Uncomment the mock implementations and update them to return values consistent
with the new OrderState pattern, ensuring the mocked service methods correctly
simulate expected behavior for the tests.
There was a problem hiding this comment.
MostroService.takeBuyOrder() etc now return Future so they don't need to return anything, but is there a better way to handle this in the tests?
There was a problem hiding this comment.
🧩 Analysis chain
@chebizarro You're absolutely right! Since takeBuyOrder() and takeSellOrder() now return Future<void>, the mocking approach should be much simpler. Here's how to handle this properly:
For Future<void> methods, you can simply mock them like this:
// Simple void method mocking
when(mockMostroService.takeBuyOrder(any, any))
.thenAnswer((_) async {});
when(mockMostroService.takeSellOrder(any, any, any))
.thenAnswer((_) async {});Since these methods are now void, the state updates likely happen through other mechanisms (like event streams or provider notifications). Here's a better testing approach:
- Mock the void methods simply:
when(mockMostroService.takeBuyOrder(testOrderId, any))
.thenAnswer((_) async {});- Focus on verifying the method calls:
verify(mockMostroService.takeBuyOrder(testOrderId, 0)).called(1);- Test state changes through the actual state management mechanism:
// If state updates happen via providers/notifiers, you might need to:
// - Mock the underlying event streams
// - Test the provider state directly
// - Use integration tests for end-to-end flows- Consider testing at different levels:
- Unit tests: Mock the service and verify calls
- Integration tests: Test the full flow with real state management
- Widget tests: Test UI responses to state changes
Would you like me to examine how the state updates actually work now to suggest a more specific testing strategy?
🏁 Script executed:
# Let's check the current service method signatures to confirm the return types
ast-grep --pattern 'Future<$_> takeBuyOrder($$$)'
ast-grep --pattern 'Future<$_> takeSellOrder($$$)'
Length of output: 104
🏁 Script executed:
# Let's search for these methods more broadly
rg "takeBuyOrder|takeSellOrder" -A 3 -B 1
# Also check the MostroService class definition
rg "class MostroService" -A 20
# Look for Future<void> method signatures in the service
rg "Future<void>" lib/services/mostro_service.dart -A 2 -B 1
Length of output: 15303
Perfect! The search confirms that these methods indeed return Future<void>. Here's the much better approach for testing these void methods:
1. Simple Void Method Mocking
Since the methods return Future<void>, you can mock them very simply:
// Just ensure they complete successfully
when(mockMostroService.takeBuyOrder(any, any))
.thenAnswer((_) async {});
when(mockMostroService.takeSellOrder(any, any, any))
.thenAnswer((_) async {});2. Focus on Verification and Side Effects
The real value in testing these methods is:
test('Taking a Buy Order calls service method with correct parameters', () async {
// Arrange
when(mockMostroService.takeBuyOrder(any, any))
.thenAnswer((_) async {});
container = ProviderContainer(overrides: [
mostroServiceProvider.overrideWithValue(mockMostroService),
]);
// Act
final notifier = container.read(orderNotifierProvider(testOrderId).notifier);
await notifier.takeBuyOrder(testOrderId, 100);
// Assert
verify(mockMostroService.takeBuyOrder(testOrderId, 100)).called(1);
});3. Test State Changes Through the Provider System
For testing the actual state changes, you might need to:
test('Taking order updates state correctly', () async {
// This depends on how state updates actually work in your new system
// You might need to mock the underlying streams or events that trigger state changes
// If state updates happen through the notifier's internal logic:
final notifier = container.read(orderNotifierProvider(testOrderId).notifier);
// Trigger the action
await notifier.takeBuyOrder(testOrderId, 100);
// Check the resulting state
final state = container.read(orderNotifierProvider(testOrderId));
expect(state.status, equals(expectedStatus));
});4. Consider Integration Testing
Since the methods are now void and side effects happen through other channels, you might want to consider:
- Unit tests: Mock the service, verify calls
- Integration tests: Test the full flow with real providers
- Mock the event streams: If state updates come through streams, mock those instead
You can remove the old configureMockMethod helper and the commented JSON setup since they're no longer needed. The tests become much simpler and more focused on the actual behavior you want to verify.
✏️ Learnings added
Learnt from: chebizarro
PR: MostroP2P/mobile#110
File: test/notifiers/take_order_notifier_test.dart:72-74
Timestamp: 2025-06-04T19:35:20.188Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| container.read(addOrderNotifierProvider(testUuid).notifier); | ||
| await notifier.submitOrder(newSellRangeOrder); | ||
|
|
||
| final state = container.read(orderNotifierProvider(testUuid)); |
There was a problem hiding this comment.
Fix inconsistent provider usage
These lines still use orderNotifierProvider while other parts of the tests use addOrderNotifierProvider. This inconsistency could lead to test failures or incorrect behavior.
Apply this diff to maintain consistency:
- final state = container.read(orderNotifierProvider(testUuid));
+ final state = container.read(addOrderNotifierProvider(testUuid));Also applies to: 281-281
🤖 Prompt for AI Agents
In test/notifiers/add_order_notifier_test.dart at lines 157 and 281, the code
uses orderNotifierProvider instead of addOrderNotifierProvider, causing
inconsistency with the rest of the tests. Replace orderNotifierProvider with
addOrderNotifierProvider in these lines to ensure consistent provider usage
throughout the test file.
| ref.listen( | ||
| mostroMessageStreamProvider(widget.orderId), | ||
| (_, next) { | ||
| next.whenData( | ||
| (msg) { | ||
| if (msg == null || msg.action == _lastSeenAction) return; | ||
| _lastSeenAction = msg.action; | ||
| if (!_loading) return; | ||
| if (msg.action == actions.Action.cantDo || | ||
| msg.action == widget.action) { | ||
| setState(() { | ||
| _loading = false; | ||
| _showSuccess = | ||
| widget.showSuccessIndicator && msg.action == widget.action; | ||
| }); | ||
| } | ||
| }, | ||
| ); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating state management to use OrderState consistently.
The ref.listen call still uses mostroMessageStreamProvider and handles MostroMessage objects, while the widget now also watches OrderState. This hybrid approach may lead to inconsistencies.
Consider migrating the listener to also use OrderState for consistency:
- ref.listen(
- mostroMessageStreamProvider(widget.orderId),
- (_, next) {
- next.whenData(
- (msg) {
- if (msg == null || msg.action == _lastSeenAction) return;
- _lastSeenAction = msg.action;
- if (!_loading) return;
- if (msg.action == actions.Action.cantDo ||
- msg.action == widget.action) {
- setState(() {
- _loading = false;
- _showSuccess =
- widget.showSuccessIndicator && msg.action == widget.action;
- });
- }
- },
- );
- },
- );
+ ref.listen(
+ orderNotifierProvider(widget.orderId),
+ (_, next) {
+ if (next.action == _lastSeenAction) return;
+ _lastSeenAction = next.action;
+ if (!_loading) return;
+ if (next.action == actions.Action.cantDo ||
+ next.action == widget.action) {
+ setState(() {
+ _loading = false;
+ _showSuccess =
+ widget.showSuccessIndicator && next.action == widget.action;
+ });
+ }
+ },
+ );📝 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.
| ref.listen( | |
| mostroMessageStreamProvider(widget.orderId), | |
| (_, next) { | |
| next.whenData( | |
| (msg) { | |
| if (msg == null || msg.action == _lastSeenAction) return; | |
| _lastSeenAction = msg.action; | |
| if (!_loading) return; | |
| if (msg.action == actions.Action.cantDo || | |
| msg.action == widget.action) { | |
| setState(() { | |
| _loading = false; | |
| _showSuccess = | |
| widget.showSuccessIndicator && msg.action == widget.action; | |
| }); | |
| } | |
| }, | |
| ); | |
| }, | |
| ); | |
| ref.listen( | |
| orderNotifierProvider(widget.orderId), | |
| (_, next) { | |
| if (next.action == _lastSeenAction) return; | |
| _lastSeenAction = next.action; | |
| if (!_loading) return; | |
| if (next.action == actions.Action.cantDo || | |
| next.action == widget.action) { | |
| setState(() { | |
| _loading = false; | |
| _showSuccess = | |
| widget.showSuccessIndicator && next.action == widget.action; | |
| }); | |
| } | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In lib/shared/widgets/mostro_reactive_button.dart around lines 80 to 99, the
ref.listen call currently listens to mostroMessageStreamProvider and handles
MostroMessage objects, which is inconsistent with the widget's use of
OrderState. To fix this, refactor the listener to listen to the OrderState
provider instead, updating the callback logic to handle OrderState changes
consistently. This will unify state management and prevent potential
inconsistencies between message and state handling.
| case Action.paymentFailed: | ||
| notifProvider.showInformation(state.action, values: { | ||
| notifProvider.showInformation(event.action, values: { | ||
| 'payment_attempts': -1, | ||
| 'payment_retries_interval': -1000 | ||
| }); | ||
| break; |
There was a problem hiding this comment.
Use appropriate default values instead of negative placeholders.
The negative values appear to be placeholders that should be replaced with meaningful defaults.
case Action.paymentFailed:
notifProvider.showInformation(event.action, values: {
- 'payment_attempts': -1,
- 'payment_retries_interval': -1000
+ 'payment_attempts': 0,
+ 'payment_retries_interval': Config.paymentRetryInterval ?? 30000
});📝 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.
| case Action.paymentFailed: | |
| notifProvider.showInformation(state.action, values: { | |
| notifProvider.showInformation(event.action, values: { | |
| 'payment_attempts': -1, | |
| 'payment_retries_interval': -1000 | |
| }); | |
| break; | |
| case Action.paymentFailed: | |
| notifProvider.showInformation(event.action, values: { | |
| 'payment_attempts': 0, | |
| 'payment_retries_interval': Config.paymentRetryInterval ?? 30000 | |
| }); | |
| break; |
🤖 Prompt for AI Agents
In lib/features/order/notfiers/abstract_mostro_notifier.dart between lines 199
and 204, replace the negative placeholder values for 'payment_attempts' and
'payment_retries_interval' with appropriate, meaningful default values that
reflect valid states or defaults for these fields instead of using -1 and -1000.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
Hi @chebizarro
With this PR, an order can be completed, but the user should still see the statuses waiting buyer invoice waiting seller to pay and active in the order details, and when "action": "fiat-sent-ok" is received, it should show fiat sent
These statuses were modified in event 38383 to preserve user privacy from outside observers. The user should have more information about the status of their order; in-progress isn't enough.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
When an order is completed, it should show the rate button, but it no longer does, even though Mostro has sent the message of rate the counterparty.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
When a user creates a sales order, which is taken and the buyer puts his invoice, when it goes to waiting-payment, the seller (maker) does not receive the notification to pay the invoice, but if he goes to the order and selects Pay Invoice button, the QR that is shown is empty
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
When I post a purchase order, and is taken, it asks me to add an invoice for 0 sats
When I go back to see if the message comes out again the "Add Invoice" button doesn't appear.
If I close and reopen the app, the message appears when the order is in pending status
- Created `new_order_with_mocks_test.dart` to test the creation of BUY and SELL orders with mocked providers. - Added `test_helpers.dart` to provide fake implementations for shared preferences and secure storage. - Updated `mostro_storage.dart` to log messages with order IDs instead of payload types. - Enhanced `order_state.dart` to log updates with action types. - Modified `abstract_mostro_notifier.dart` to log received messages. - Fixed potential null issues in `add_lightning_invoice_screen.dart` by providing default values. - Updated UI components in various screens to include keys for widget testing. - Removed the default widget test file as it was not relevant. - Updated mocks to include `SharedPreferencesAsync` for better testing coverage.
|
The latest commit What's still missing is this:
|
The display of the current order status is modified by PR #116 so I would suggest we merge this PR as is and continue the discussion on issue #86 |
This PR delivers a comprehensive refactor of the app's state management layer to address long-standing issues with order/session lifecycle, notification handling, and provider invalidation.
Key Changes
OrderStatepattern.Closes #77:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Style