Restore Session Implementation #324
Conversation
WalkthroughThis pull request implements a complete session restore feature that enables users to import a mnemonic phrase and restore previously active sessions, orders, and disputes. It includes new message models, a service orchestrating the restore workflow, UI components for mnemonic import and progress overlay, state management for tracking restoration steps, and localization strings across multiple languages. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant KeyMgmt as Key Management<br/>Screen
participant MnemonicDlg as Import Mnemonic<br/>Dialog
participant RestoreOvly as Restore<br/>Overlay
participant RestoreSvc as RestoreService
participant NostrSvc as Nostr Event<br/>Service
participant MostroSvc as Mostro<br/>Service
participant Storage as Session &<br/>Message Storage
User->>KeyMgmt: Tap Import/Refresh Button
KeyMgmt->>MnemonicDlg: Show dialog (if import)
MnemonicDlg->>User: Prompt for 12-word mnemonic
User->>MnemonicDlg: Enter mnemonic
MnemonicDlg->>MnemonicDlg: Validate (12 words, A-Z only)
alt Validation Failed
MnemonicDlg->>User: Show error message
else Validation Passed
MnemonicDlg->>RestoreSvc: importMnemonicAndRestore(mnemonic)
end
RestoreSvc->>RestoreSvc: Validate master key & Mostro config
RestoreSvc->>RestoreOvly: Show overlay (requesting state)
rect rgb(200, 220, 255)
note over RestoreSvc,Storage: Restore Workflow
RestoreSvc->>RestoreSvc: Derive temporary trade key
RestoreSvc->>NostrSvc: Subscribe to kind 1059 events
RestoreSvc->>MostroSvc: Send restore-session request<br/>(wrapped with Mostro keys)
RestoreSvc->>RestoreOvly: Update progress (requestingData)
loop Incoming Events
NostrSvc->>RestoreSvc: Receive kind 1059 event
RestoreSvc->>RestoreSvc: Unwrap & decode content
RestoreSvc->>RestoreSvc: Process action<br/>(restore-session, orders, last-trade-index)
alt Action: restore-session
RestoreSvc->>RestoreOvly: Update (receivingOrders)
RestoreSvc->>RestoreSvc: Extract orders & disputes
RestoreSvc->>MostroSvc: Request order details
RestoreSvc->>MostroSvc: Request last-trade-index
else Action: orders
RestoreSvc->>RestoreOvly: Update (loadingDetails)
RestoreSvc->>RestoreSvc: Accumulate order details
else Action: last-trade-index
RestoreSvc->>RestoreOvly: Update (processingRoles)
end
end
RestoreSvc->>RestoreSvc: All data received?
RestoreSvc->>Storage: Restore sessions with derived keys
RestoreSvc->>Storage: Save order details as messages
RestoreSvc->>RestoreSvc: Update key index
RestoreSvc->>RestoreOvly: Update (finalizing → completed)
end
RestoreSvc->>NostrSvc: Unsubscribe & cleanup
RestoreOvly->>User: Auto-hide after 1s
User->>KeyMgmt: Sessions/orders restored
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention during review:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
integration_test/test_helpers.dart(1 hunks)lib/data/models.dart(1 hunks)lib/data/models/enums/action.dart(1 hunks)lib/data/models/mostro_message.dart(1 hunks)lib/data/models/payload.dart(2 hunks)lib/data/models/restore_data.dart(1 hunks)lib/features/key_manager/key_management_screen.dart(6 hunks)lib/features/notifications/utils/notification_message_mapper.dart(2 hunks)lib/features/notifications/widgets/notification_item.dart(1 hunks)lib/features/subscriptions/subscription_manager.dart(7 hunks)lib/features/subscriptions/subscription_type.dart(1 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_es.arb(1 hunks)lib/l10n/intl_it.arb(1 hunks)lib/services/mostro_service.dart(4 hunks)test/mocks.mocks.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (15)
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place feature-specific Dart code under lib/features// (screens, providers, widgets)
Files:
lib/features/subscriptions/subscription_type.dartlib/features/subscriptions/subscription_manager.dartlib/features/notifications/utils/notification_message_mapper.dartlib/features/notifications/widgets/notification_item.dartlib/features/key_manager/key_management_screen.dart
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Format code with the Dart formatter (two-space indentation, trailing commas where beneficial)
Keep the analyzer (flutter_lints) at zero warnings across the codebase
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
Files:
lib/features/subscriptions/subscription_type.dartintegration_test/test_helpers.dartlib/data/models.darttest/mocks.mocks.dartlib/data/models/payload.dartlib/data/models/restore_data.dartlib/services/mostro_service.dartlib/data/models/mostro_message.dartlib/features/subscriptions/subscription_manager.dartlib/data/models/enums/action.dartlib/features/notifications/utils/notification_message_mapper.dartlib/features/notifications/widgets/notification_item.dartlib/features/key_manager/key_management_screen.dart
lib/features/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Name Riverpod providers as Provider or Notifier within their feature directory
Files:
lib/features/subscriptions/subscription_type.dartlib/features/subscriptions/subscription_manager.dartlib/features/notifications/utils/notification_message_mapper.dartlib/features/notifications/widgets/notification_item.dartlib/features/key_manager/key_management_screen.dart
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possible
Files:
lib/features/subscriptions/subscription_type.dartlib/data/models.dartlib/data/models/payload.dartlib/data/models/restore_data.dartlib/services/mostro_service.dartlib/data/models/mostro_message.dartlib/features/subscriptions/subscription_manager.dartlib/data/models/enums/action.dartlib/features/notifications/utils/notification_message_mapper.dartlib/features/notifications/widgets/notification_item.dartlib/features/key_manager/key_management_screen.dart
integration_test/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
integration_test/**/*.dart: Integration scenarios belong in integration_test/ and exercise full user flows
Include end-to-end tests under integration_test/ for flows that touch native bridgesPlace integration tests under
integration_test/
Files:
integration_test/test_helpers.dart
lib/data/**
📄 CodeRabbit inference engine (AGENTS.md)
Place persistence and API abstractions under lib/data/
Files:
lib/data/models.dartlib/data/models/payload.dartlib/data/models/restore_data.dartlib/data/models/mostro_message.dartlib/data/models/enums/action.dart
**/*.arb
📄 CodeRabbit inference engine (AGENTS.md)
Keep user-facing strings in localization ARB files; do not hard-code copy in Dart code
Files:
lib/l10n/intl_it.arblib/l10n/intl_en.arblib/l10n/intl_es.arb
lib/l10n/intl_{en,es,it}.arb
📄 CodeRabbit inference engine (CLAUDE.md)
Add new localization keys to all three ARB files (en, es, it)
Files:
lib/l10n/intl_it.arblib/l10n/intl_en.arblib/l10n/intl_es.arb
lib/l10n/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
For parameterized strings, include proper ARB metadata
Files:
lib/l10n/intl_it.arblib/l10n/intl_en.arblib/l10n/intl_es.arb
test/**
📄 CodeRabbit inference engine (AGENTS.md)
Unit tests should mirror the feature layout under test/
Files:
test/mocks.mocks.dart
**/*.{g.dart,mocks.dart}
📄 CodeRabbit inference engine (AGENTS.md)
Do not modify generated files (*.g.dart, *.mocks.dart) by hand
Files:
test/mocks.mocks.dart
**/*.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.mocks.dart: Never manually edit generated*.mocks.dartfiles
Do not add ignore comments to*.mocks.dart; regenerate instead
Files:
test/mocks.mocks.dart
test/mocks.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Never modify
test/mocks.mocks.dart(has file-level ignores; regenerate via build_runner)
Files:
test/mocks.mocks.dart
test/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under
test/
Files:
test/mocks.mocks.dart
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep app-wide services under lib/services/
Files:
lib/services/mostro_service.dart
🧠 Learnings (1)
📚 Learning: 2025-10-01T14:10:17.742Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-01T14:10:17.742Z
Learning: Applies to lib/l10n/intl_{en,es,it}.arb : Add new localization keys to all three ARB files (en, es, it)
Applied to files:
lib/l10n/intl_it.arblib/l10n/intl_es.arb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
lib/data/models/restore_data.dart (1)
72-85: LGTM with one note.The graceful handling of missing
ordersanddisputesarrays with?? []is good defensive programming. However, the nested calls toRestoreOrderInfo.fromJsonandRestoreDisputeInfo.fromJsonon lines 79 and 82 will still throw if individual items have type issues, as noted in previous comments.Once the type safety improvements are applied to
RestoreOrderInfo.fromJsonandRestoreDisputeInfo.fromJson, this method will properly handle malformed data.lib/features/notifications/widgets/notification_item.dart (1)
125-125: LGTM!The
restoreSessioncase is correctly handled by breaking without navigation, consistent with other action types that don't require user interaction from notifications (liketradePubkey,invoiceUpdated, etc.).lib/data/models/mostro_message.dart (1)
100-100: LGTM!The addition of
json['restore']as a fallback source aligns with the existing pattern fororderandcant-do. This enables proper extraction ofrequest_idand other fields from restore-related messages.lib/data/models.dart (1)
16-16: LGTM!The export of
restore_data.dartproperly extends the public API surface to include the new restore functionality.lib/features/subscriptions/subscription_type.dart (1)
5-5: LGTM!The addition of
masterKeyto theSubscriptionTypeenum properly extends the subscription system to support master key event handling for the restore session flow.lib/data/models/payload.dart (2)
9-9: LGTM!The import of
restore_data.dartenables the new restore payload type integration.
32-33: LGTM!The addition of
restore_datapayload handling follows the established pattern for other payload types. The check forjson.containsKey('restore_data')and call toRestoreData.fromJson(json)is consistent with the rest of the factory method.integration_test/test_helpers.dart (1)
310-311: LGTM!The empty implementation of
requestRestoreSession()is appropriate for the test helper mock. This allows integration tests to exercise the UI flow for restore session without requiring actual backend interaction.lib/l10n/intl_en.arb (1)
862-865: Localization completeness confirmedAll four mnemonicValidation keys appear once in intl_es.arb and intl_it.arb; no further changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/services/mostro_service.dart (1)
355-363: Stop hard-coding the restored session role.Every restored session is saved as
Role.seller, so buyer trades will be mislabeled and routed through the wrong FSM/UI paths. Please populaterolefrom the backend payload (or leave it null/unknown until the real role is known) instead of forcingRole.seller.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/data/models/restore_data.dart(1 hunks)lib/features/key_manager/key_management_screen.dart(6 hunks)lib/services/mostro_service.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/data/models/restore_data.dart
🧰 Additional context used
📓 Path-based instructions (5)
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep app-wide services under lib/services/
Files:
lib/services/mostro_service.dart
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Format code with the Dart formatter (two-space indentation, trailing commas where beneficial)
Keep the analyzer (flutter_lints) at zero warnings across the codebase
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
Files:
lib/services/mostro_service.dartlib/features/key_manager/key_management_screen.dart
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possible
Files:
lib/services/mostro_service.dartlib/features/key_manager/key_management_screen.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place feature-specific Dart code under lib/features// (screens, providers, widgets)
Files:
lib/features/key_manager/key_management_screen.dart
lib/features/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Name Riverpod providers as Provider or Notifier within their feature directory
Files:
lib/features/key_manager/key_management_screen.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
| final validCharacters = RegExp(r'^[a-zA-Z\s]+$'); | ||
| if (!validCharacters.hasMatch(trimmed)) { | ||
| return S.of(context)!.mnemonicValidationInvalidFormat; | ||
| } | ||
|
|
||
| final words = trimmed.split(RegExp(r'\s+')); | ||
| if (words.length != 12 && words.length != 24) { | ||
| return S.of(context)!.mnemonicValidationInvalidWordCount; | ||
| } | ||
|
|
There was a problem hiding this comment.
Allow non-ASCII BIP-39 characters in mnemonic validation.
The regex only permits [a-zA-Z], so valid Spanish mnemonics (e.g., “ábaco”, “niñez”) fail validation, blocking imports for ES users. Expand the character class to accept all Unicode letters.
- final validCharacters = RegExp(r'^[a-zA-Z\s]+$');
- if (!validCharacters.hasMatch(trimmed)) {
+ final validCharacters =
+ RegExp(r'^[\p{L}\s]+$', unicode: true);
+ if (!validCharacters.hasMatch(trimmed)) {📝 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.
| final validCharacters = RegExp(r'^[a-zA-Z\s]+$'); | |
| if (!validCharacters.hasMatch(trimmed)) { | |
| return S.of(context)!.mnemonicValidationInvalidFormat; | |
| } | |
| final words = trimmed.split(RegExp(r'\s+')); | |
| if (words.length != 12 && words.length != 24) { | |
| return S.of(context)!.mnemonicValidationInvalidWordCount; | |
| } | |
| final validCharacters = | |
| RegExp(r'^[\p{L}\s]+$', unicode: true); | |
| if (!validCharacters.hasMatch(trimmed)) { | |
| return S.of(context)!.mnemonicValidationInvalidFormat; | |
| } | |
| final words = trimmed.split(RegExp(r'\s+')); | |
| if (words.length != 12 && words.length != 24) { | |
| return S.of(context)!.mnemonicValidationInvalidWordCount; | |
| } |
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
hola @BraCR10 me está funcionando la importación de las sesiones con el mostro que hay por defecto, pero si cambio de mostro no restaura ninguna de las sesiones que tenía.
aquí tenía varias órdenes usando otro mostro
luego de que regresé al mostro por defecto, y creé un nuevo usuario, sí restauró las sesiones del mostro por defecto, pero al cambiar al otro mostro que estaba usando antes no lo hizo, tampoco veo en los logs del otro mostro nada sobre restauracion de sesiones alli
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
lib/services/mostro_service.dart (2)
343-359: Consider per-batch error handling.The batching loop sends multiple requests but doesn't handle individual batch failures. If one batch request fails to publish, the completers for those orders will timeout (line 407), which provides a safety net. However, you might want to add error handling within the loop to log which batches failed to send.
Consider wrapping the publish call:
for (var i = 0; i < orderIds.length; i += maxOrdersPerRequest) { final batch = orderIds.skip(i).take(maxOrdersPerRequest).toList(); final message = MostroMessage( action: Action.orders, payload: OrdersRequest(ids: batch), ); final event = await message.wrap( tradeKey: masterKey, recipientPubKey: _settings.mostroPublicKey, masterKey: masterKey, keyIndex: null, ); - await ref.read(nostrServiceProvider).publishEvent(event); + try { + await ref.read(nostrServiceProvider).publishEvent(event); + } catch (e) { + _logger.e('Failed to publish order details batch ${i ~/ maxOrdersPerRequest}', error: e); + } }
418-420: Handle null buyer/seller pubkeys explicitly
buyerTradePubkey and sellerTradePubkey are nullable in lib/data/models/order.dart, so when both are null the code just logs a warning—consider mapping that to a defined order state (e.g., pending) or handling it explicitly instead of falling through.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
integration_test/test_helpers.dart(1 hunks)lib/data/models.dart(2 hunks)lib/data/models/enums/action.dart(1 hunks)lib/data/models/orders_request.dart(1 hunks)lib/data/models/orders_response.dart(1 hunks)lib/data/models/payload.dart(3 hunks)lib/features/notifications/utils/notification_message_mapper.dart(2 hunks)lib/features/notifications/widgets/notification_item.dart(1 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_es.arb(1 hunks)lib/l10n/intl_it.arb(1 hunks)lib/services/mostro_service.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/l10n/intl_en.arb
- lib/l10n/intl_es.arb
- lib/features/notifications/utils/notification_message_mapper.dart
- lib/l10n/intl_it.arb
- lib/data/models/enums/action.dart
🧰 Additional context used
📓 Path-based instructions (6)
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possibleName Riverpod providers as Provider or Notifier
Files:
lib/data/models/orders_request.dartlib/features/notifications/widgets/notification_item.dartlib/data/models/orders_response.dartlib/services/mostro_service.dartlib/data/models.dartlib/data/models/payload.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
Files:
lib/data/models/orders_request.dartintegration_test/test_helpers.dartlib/features/notifications/widgets/notification_item.dartlib/data/models/orders_response.dartlib/services/mostro_service.dartlib/data/models.dartlib/data/models/payload.dart
lib/data/**
📄 CodeRabbit inference engine (AGENTS.md)
Store persistence and API code under lib/data/
Files:
lib/data/models/orders_request.dartlib/data/models/orders_response.dartlib/data/models.dartlib/data/models/payload.dart
integration_test/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place integration tests under
integration_test/
integration_test/**/*.dart: Place end-to-end integration flows under integration_test/
Ensure integration coverage for flows involving networking, background work, or session recovery
Files:
integration_test/test_helpers.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/notifications/widgets/notification_item.dart
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Place service-layer code in lib/services/
Files:
lib/services/mostro_service.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (11)
lib/data/models/payload.dart (2)
5-11: LGTM! Clean payload model integration.The new imports for
OrdersRequest,OrdersResponse, andRestoreDataare properly integrated to support the restore session feature.
20-23: LGTM! Factory pattern extended correctly.The new conditional branches properly handle the restore and orders payloads by checking for the expected JSON keys and delegating to the appropriate model constructors.
Also applies to: 38-39
lib/services/mostro_service.dart (9)
23-24: LGTM! Proper subscription and coordination infrastructure.The new
_masterKeySubscriptionfield enables processing restore responses, and_orderDetailCompletersprovides a clean async coordination mechanism for matching order detail requests with responses.Also applies to: 25-25
40-49: LGTM! Master key subscription lifecycle managed correctly.The master key subscription is properly initialized and disposed, mirroring the pattern used for orders subscription.
Also applies to: 54-54
71-87: LGTM! Elegant master key fallback logic.The code now gracefully handles both session-based messages (using trade keys) and master-key-based messages (for restore/orders actions). The fallback to master key when no session matches is exactly what's needed for the restore flow.
91-100: LGTM! Defensive validation improves robustness.The added null checks and type validation prevent processing malformed events and provide clear diagnostics via logging.
104-122: LGTM! Clean action routing separates concerns.Routing
restoreSessionandordersactions to dedicated handlers before the normal message storage path is correct—these master-key messages don't belong to any specific session.
417-425: Excellent fix! Role now determined from order ownership.This addresses the critical issue from previous reviews. By comparing the derived trade key against
buyerTradePubkeyandsellerTradePubkeyin the order details, the code correctly identifies whether the user was the buyer or seller—no more hardcodedRole.seller. The logic also safely skips orders that don't match either role, preventing incorrect restoration.
362-465: Well-structured restore flow with good error handling.The restore response handler properly:
- Validates payload type and handles empty responses
- Uses completers with timeouts to coordinate async order detail requests
- Derives trade keys from indices and determines role from order details
- Manages key index advancement based on restored count
- Cleans up completers in all code paths (success, individual failure, and global failure)
The batching of order detail requests (max 20 per batch) is a sensible performance optimization.
454-456: Confirmed fix for key index advancement.The condition now correctly uses
restoredCount > 0instead ofmaxTradeIndex > 0, ensuring the key index advances even when the only restored trade had index 0. This addresses the issue flagged in previous reviews.
467-494: LGTM! Clean orders response handler.The handler properly validates the payload type, iterates through orders, and completes the corresponding completers. Good defensive check for
!completer.isCompletedto avoid completing twice.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/mostro_service.dart (1)
61-69: Don’t mark events as processed before handling; can drop messages.You write to eventStore before decrypt/routing. If a DM arrives before a session exists, it’s skipped now and won’t be processed later after restore. Mark as processed only after you actually handle it (restore/orders or matched session).
final eventStore = ref.read(eventStorageProvider); - if (await eventStore.hasItem(event.id!)) return; - await eventStore.putItem( - event.id!, - { - 'id': event.id, - 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, - }, - ); + if (await eventStore.hasItem(event.id!)) return; + var handled = false;if (msg.action == Action.restoreSession) { - await _handleRestoreResponse(msg); - return; + await _handleRestoreResponse(msg); + await eventStore.putItem(event.id!, { + 'id': event.id, + 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, + }); + return; } if (msg.action == Action.orders) { - await _handleOrdersResponse(msg); - return; + await _handleOrdersResponse(msg); + await eventStore.putItem(event.id!, { + 'id': event.id, + 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, + }); + return; } if (matchingSession != null) { final messageStorage = ref.read(mostroStorageProvider); await messageStorage.addMessage(decryptedEvent.id!, msg); _logger.i( - 'Received DM, Event ID: ${decryptedEvent.id} with payload: ${decryptedEvent.content}', + 'Received DM, Event ID: ${decryptedEvent.id}', ); await _maybeLinkChildOrder(msg, matchingSession); + handled = true; } } catch (e, stackTrace) { _logger.e('Error processing event', error: e, stackTrace: stackTrace); } +if (handled) { + await eventStore.putItem(event.id!, { + 'id': event.id, + 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, + }); +}Also applies to: 104-112, 114-123, 123-126
♻️ Duplicate comments (4)
lib/data/models/orders_request.dart (1)
46-54: Equality/hash now correct and order-aware.Using ListEquality for == and hash aligns equality with hash. LGTM.
Based on learnings
lib/services/mostro_service.dart (3)
316-321: Master key null-guard added — good.Prevents crashes when master key is missing.
344-348: Master key null-guard for order details — good.Fails fast with clear log.
425-433: Correct role derivation — issue resolved.Derives buyer/seller from order details; no longer hard-coded.
🧹 Nitpick comments (3)
lib/data/models/orders_request.dart (2)
7-11: Make ids immutable to prevent external mutations.Defensively copy to an unmodifiable list in the constructor.
- OrdersRequest({required this.ids}) { + OrdersRequest({required List<String> ids}) + : ids = List.unmodifiable(ids) { if (ids.isEmpty) { throw ArgumentError('Order IDs list cannot be empty'); } }
50-54: Minor nit: add generic parameter to ListEquality.Be explicit on the type parameter.
- const ListEquality().equals(other.ids, ids); + const ListEquality<String>().equals(other.ids, ids);- int get hashCode => const ListEquality().hash(ids); + int get hashCode => const ListEquality<String>().hash(ids);Based on learnings
lib/services/mostro_service.dart (1)
415-421: Use const Duration.Tiny allocation win.
- final orderDetails = await completer.future.timeout( - Duration(seconds: 10), + final orderDetails = await completer.future.timeout( + const Duration(seconds: 10), onTimeout: () {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/data/models/orders_request.dart(1 hunks)lib/data/models/orders_response.dart(1 hunks)lib/services/mostro_service.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/data/models/orders_response.dart
🧰 Additional context used
📓 Path-based instructions (4)
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possibleName Riverpod providers as Provider or Notifier
Files:
lib/data/models/orders_request.dartlib/services/mostro_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
Files:
lib/data/models/orders_request.dartlib/services/mostro_service.dart
lib/data/**
📄 CodeRabbit inference engine (AGENTS.md)
Store persistence and API code under lib/data/
Files:
lib/data/models/orders_request.dart
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Place service-layer code in lib/services/
Files:
lib/services/mostro_service.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
lib/services/mostro_service.dart (1)
41-49: Master-key subscription wiring looks good.Subscribes and cancels properly. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/settings/settings_notifier.dart (1)
43-46: Likely wrong field in copyWith for privacy modecopyWith(privacyModeSetting: newValue) looks inconsistent with Settings.default using fullPrivacyMode. If the model doesn’t expose privacyModeSetting, this won’t compile or silently do nothing. Switch to fullPrivacyMode.
- Future<void> updatePrivacyMode(bool newValue) async { - state = state.copyWith(privacyModeSetting: newValue); + Future<void> updatePrivacyMode(bool newValue) async { + state = state.copyWith(fullPrivacyMode: newValue); await _saveToPrefs(); }lib/services/mostro_service.dart (1)
100-103: Guard against empty decoded list.After confirming the decoded result is a
List, the code accessesresult[0]without checking if the list is empty, which will throw anIndexError.Apply this defensive check:
final result = jsonDecode(decryptedEvent.content!); if (result is! List) return; + if (result.isEmpty) { + _logger.w('Decoded result list is empty'); + return; + } final msg = MostroMessage.fromJson(result[0]);
🧹 Nitpick comments (10)
lib/features/settings/settings_notifier.dart (1)
65-68: Avoid redundant write when Mostro pubkey is unchangedIf oldPubkey == newValue, we’re persisting identical state. Early-return to reduce needless I/O.
} else { - // Only update pubkey if it's the same (without reset) - state = state.copyWith(mostroPublicKey: newValue); - await _saveToPrefs(); + _logger.d('Mostro pubkey unchanged; skipping persistence'); + return; }lib/features/notifications/widgets/notification_item.dart (2)
74-131: New actions are no-ops on tap; verify intended UXrestoreSession, orders, lastTradeIndex just break. If the intent is to surface the restoration UI (overlay) on tap, consider triggering the relevant flow even when orderId is null, or move these cases outside the orderId guard.
- if (notification.orderId != null) { - switch (notification.action) { + if (notification.action == mostro_action.Action.restoreSession || + notification.action == mostro_action.Action.orders || + notification.action == mostro_action.Action.lastTradeIndex) { + // Option: trigger restoration overlay or navigate accordingly + // ref.read(restorationNotifierProvider.notifier).start(); + return; + } + if (notification.orderId != null) { + switch (notification.action) { // ... } }
158-199: Dialog side-effect: wrap in post-frame to match UI side-effect guidelineTo align with “Use post-frame callbacks for UI side effects,” dispatch showDialog in a post-frame callback.
- void _showDeleteConfirmationDialog(BuildContext context, WidgetRef ref) { - showDialog( + void _showDeleteConfirmationDialog(BuildContext context, WidgetRef ref) { + WidgetsBinding.instance.addPostFrameCallback((_) { + if (!context.mounted) return; + showDialog( context: context, builder: (context) => AlertDialog( // ... - ); + ); + }); }lib/features/settings/restoration_provider.dart (1)
5-8: Provider wiring looks goodName matches convention; minimal, clear setup. If RestorationNotifier later needs dependencies, switch to
RestorationNotifier(ref)and inject via ref.lib/core/app.dart (1)
216-220: Localize error message per guidelinesReplace hardcoded 'Initialization Error' with a localized string via S.of(context), and add delegates to this MaterialApp if needed.
- error: (err, stack) => MaterialApp( - home: Scaffold( - body: Center(child: Text('Initialization Error: $err')), - ), - ), + error: (err, stack) => MaterialApp( + localizationsDelegates: const [ + S.delegate, + GlobalMaterialLocalizations.delegate, + GlobalWidgetsLocalizations.delegate, + GlobalCupertinoLocalizations.delegate, + ], + supportedLocales: S.supportedLocales, + home: Builder( + builder: (context) => Scaffold( + body: Center(child: Text(S.of(context)!.initializationError(err.toString()))), + ), + ), + ),Note: add a new intl key like initializationError(message).
integration_test/test_helpers.dart (1)
310-318: Confirm method signatures against MostroServicerequestOrderDetails takes List non-nullable here, but mocks elsewhere use nullable List? If the interface expects nullable, this override won’t compile. Align signature and add a null/empty guard.
- Future<void> requestOrderDetails(List<String> orderIds) async {} + Future<void> requestOrderDetails(List<String>? orderIds) async { + if (orderIds == null || orderIds.isEmpty) return; + // no-op in fake + }lib/features/settings/restoration_overlay.dart (2)
37-38: Avoid duplicate status icons (Completed/Failed).Both _buildStatusIcon and _buildProgressIndicator render icons for completed/failed, leading to two icons. Show only one.
Apply this diff to keep the top icon and suppress the second one for terminal states; also show determinate progress when possible:
Widget _buildProgressIndicator(RestorationState state) { - if (state.status == RestorationStatus.completed) { - return const Icon( - Icons.check_circle, - color: Colors.green, - size: 48, - ); - } - - if (state.status == RestorationStatus.failed) { - return const Icon( - Icons.error, - color: Colors.red, - size: 48, - ); - } + // Suppress secondary icon for terminal states; the header icon already conveys status. + if (state.status == RestorationStatus.completed || + state.status == RestorationStatus.failed) { + return const SizedBox.shrink(); + } - // Show generic circular loading indicator for all loading states - return const CircularProgressIndicator( - color: AppTheme.activeColor, - ); + // Show determinate progress when loading order details + if (state.status == RestorationStatus.loadingDetails && + state.totalOrders > 0) { + return Column( + mainAxisSize: MainAxisSize.min, + children: [ + LinearProgressIndicator( + color: AppTheme.activeColor, + value: state.progress, + backgroundColor: Colors.white.withValues(alpha: 0.08), + minHeight: 6, + ), + const SizedBox(height: 8), + Text( + '${state.restoredOrders + state.skippedOrders}/${state.totalOrders}', + style: const TextStyle(color: AppTheme.textSecondary), + ), + ], + ); + } + + // Fallback indeterminate loader + return const CircularProgressIndicator(color: AppTheme.activeColor); }Also applies to: 91-112
114-129: Drop unnecessary null-assert on S.of(context).S.of(context) is non-null in generated l10n; the bang operator may trigger analyzer warnings.
- return S.of(context)!.restorationCleaningTitle; + return S.of(context).restorationCleaningTitle; @@ - return S.of(context)!.restorationRequestingTitle; + return S.of(context).restorationRequestingTitle; @@ - return S.of(context)!.restorationLoadingDetailsTitle; + return S.of(context).restorationLoadingDetailsTitle; @@ - return S.of(context)!.restorationCompletedTitle; + return S.of(context).restorationCompletedTitle; @@ - return S.of(context)!.restorationFailedTitle; + return S.of(context).restorationFailedTitle; @@ - return S.of(context)!.restorationCleaningMessage; + return S.of(context).restorationCleaningMessage; @@ - return S.of(context)!.restorationRequestingMessage; + return S.of(context).restorationRequestingMessage; @@ - return S.of(context)!.restorationLoadingDetailsMessage; + return S.of(context).restorationLoadingDetailsMessage; @@ - return S.of(context)!.restorationCompletedMessage; + return S.of(context).restorationCompletedMessage; @@ - return state.errorMessage ?? S.of(context)!.restorationFailedMessage; + return state.errorMessage ?? S.of(context).restorationFailedMessage;As per coding guidelines.
Also applies to: 131-146
lib/features/settings/restoration_notifier.dart (1)
29-41: DRY the auto-reset logic and make delay explicit.Reduce duplication and centralize the delayed reset behavior.
class RestorationNotifier extends StateNotifier<RestorationState> { - final _logger = Logger(); + final _logger = Logger(); + static const _autoResetDelay = Duration(seconds: 3); + + void _autoResetWhenStable(RestorationStatus expected) { + Future.delayed(_autoResetDelay, () { + if (!mounted) return; + if (state.status == expected) reset(); + }); + } @@ void complete(int totalOrders, int restoredOrders, int skippedOrders) { @@ - Future.delayed(const Duration(seconds: 3), () { - if (mounted && state.status == RestorationStatus.completed) { - reset(); - } - }); + _autoResetWhenStable(RestorationStatus.completed); } @@ void fail(String errorMessage) { @@ - Future.delayed(const Duration(seconds: 3), () { - if (mounted && state.status == RestorationStatus.failed) { - reset(); - } - }); + _autoResetWhenStable(RestorationStatus.failed); }Also applies to: 43-52, 5-7
lib/features/settings/restoration_state.dart (1)
84-87: Clamp progress to [0,1] and guard zero/negative totals.Prevents accidental >100% due to race conditions or rounding.
- double get progress { - if (totalOrders == 0) return 0.0; - return (restoredOrders + skippedOrders) / totalOrders; - } + double get progress { + if (totalOrders <= 0) return 0.0; + return ((restoredOrders + skippedOrders) / totalOrders).clamp(0.0, 1.0); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
integration_test/test_helpers.dart(1 hunks)lib/core/app.dart(2 hunks)lib/data/models/enums/action.dart(1 hunks)lib/features/notifications/utils/notification_message_mapper.dart(2 hunks)lib/features/notifications/widgets/notification_item.dart(1 hunks)lib/features/settings/restoration_notifier.dart(1 hunks)lib/features/settings/restoration_overlay.dart(1 hunks)lib/features/settings/restoration_provider.dart(1 hunks)lib/features/settings/restoration_state.dart(1 hunks)lib/features/settings/settings_notifier.dart(2 hunks)lib/l10n/intl_en.arb(2 hunks)lib/l10n/intl_es.arb(2 hunks)lib/l10n/intl_it.arb(2 hunks)lib/services/mostro_service.dart(6 hunks)lib/shared/providers/app_init_provider.dart(2 hunks)test/mocks.mocks.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/data/models/enums/action.dart
- lib/features/notifications/utils/notification_message_mapper.dart
- test/mocks.mocks.dart
🧰 Additional context used
📓 Path-based instructions (9)
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
Files:
integration_test/test_helpers.dartlib/features/settings/restoration_notifier.dartlib/features/settings/settings_notifier.dartlib/features/settings/restoration_overlay.dartlib/services/mostro_service.dartlib/features/notifications/widgets/notification_item.dartlib/features/settings/restoration_state.dartlib/core/app.dartlib/shared/providers/app_init_provider.dartlib/features/settings/restoration_provider.dart
integration_test/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place integration tests under
integration_test/
integration_test/**/*.dart: Place end-to-end integration flows under integration_test/
Ensure integration coverage for flows involving networking, background work, or session recovery
Files:
integration_test/test_helpers.dart
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possibleName Riverpod providers as Provider or Notifier
Files:
lib/features/settings/restoration_notifier.dartlib/features/settings/settings_notifier.dartlib/features/settings/restoration_overlay.dartlib/services/mostro_service.dartlib/features/notifications/widgets/notification_item.dartlib/features/settings/restoration_state.dartlib/core/app.dartlib/shared/providers/app_init_provider.dartlib/features/settings/restoration_provider.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/settings/restoration_notifier.dartlib/features/settings/settings_notifier.dartlib/features/settings/restoration_overlay.dartlib/features/notifications/widgets/notification_item.dartlib/features/settings/restoration_state.dartlib/features/settings/restoration_provider.dart
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Place service-layer code in lib/services/
Files:
lib/services/mostro_service.dart
lib/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep dependency wiring in lib/core/
Files:
lib/core/app.dart
lib/l10n/intl_{en,es,it}.arb
📄 CodeRabbit inference engine (CLAUDE.md)
Add new localization keys to all three ARB files (en, es, it)
Files:
lib/l10n/intl_es.arblib/l10n/intl_it.arblib/l10n/intl_en.arb
lib/l10n/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
For parameterized strings, include proper ARB metadata
Files:
lib/l10n/intl_es.arblib/l10n/intl_it.arblib/l10n/intl_en.arb
lib/shared/**
📄 CodeRabbit inference engine (AGENTS.md)
Put shared utilities in lib/shared/
Files:
lib/shared/providers/app_init_provider.dart
🧠 Learnings (5)
📚 Learning: 2025-06-04T19:35:20.209Z
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.
Applied to files:
integration_test/test_helpers.dart
📚 Learning: 2025-10-21T21:47:03.440Z
Learnt from: Catrya
PR: MostroP2P/mobile#327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.440Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/features/settings/restoration_notifier.dart
📚 Learning: 2025-10-01T14:10:17.742Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-01T14:10:17.742Z
Learning: Applies to lib/l10n/intl_{en,es,it}.arb : Add new localization keys to all three ARB files (en, es, it)
Applied to files:
lib/l10n/intl_es.arblib/l10n/intl_it.arb
📚 Learning: 2025-05-06T15:49:26.443Z
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.
Applied to files:
lib/shared/providers/app_init_provider.dartlib/features/settings/restoration_provider.dart
📚 Learning: 2025-10-07T17:31:12.117Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T17:31:12.117Z
Learning: Applies to lib/**/*.dart : Name Riverpod providers as <Feature>Provider or <Feature>Notifier
Applied to files:
lib/features/settings/restoration_provider.dart
🔇 Additional comments (18)
lib/features/settings/settings_notifier.dart (1)
14-14: Constructor simplification LGTMRemoval of Ref from the constructor narrows responsibilities and matches how it’s instantiated in tests. No issues.
lib/features/notifications/widgets/notification_item.dart (1)
55-58: Good use ofwithValues(alpha: ...)Conforms to latest Flutter API preference over
withOpacity.lib/core/app.dart (1)
197-205: Overlay integration LGTMStack + builder is a clean way to add RestorationOverlay app‑wide. Ensure the overlay internally uses IgnorePointer/Visibility when idle to avoid intercepting taps.
lib/l10n/intl_it.arb (1)
418-428: LGTM — Italian translations provided for restoration and mnemonic validation.Also applies to: 929-932
lib/l10n/intl_es.arb (1)
389-399: LGTM — Spanish translations provided for restoration and mnemonic validation.Also applies to: 873-876
lib/l10n/intl_en.arb (1)
466-477: Verification passed — all keys present across en/es/it locales.The 12 new localization keys (restoration and mnemonic validation strings) are confirmed present in all three language files. Parity is maintained; no issues found.
lib/shared/providers/app_init_provider.dart (1)
31-69: Await is unnecessary; reinitializeMasterKeySubscription() is void.The verification confirms that
reinitializeMasterKeySubscription()atlib/features/subscriptions/subscription_manager.dart:101is declared asvoidand returns synchronously. The diff already comments out the suggestedawait, which is correct—no awaiting is needed.The optional guard pattern (checking
isRestoring) is technically valid since RestorationState includes anisRestoringgetter that returns true for cleaning, requesting, or loadingDetails statuses. However, per learnings on this codebase, Riverpod StateNotifiers should avoid adding extra defensive guards in business logic flows; Riverpod handles concurrent state updates and provider disposal automatically. The guard adds complexity for marginal benefit in an unlikely race scenario.No further changes needed; the code is correct as-is.
lib/services/mostro_service.dart (11)
24-31: LGTM!The new fields appropriately support restore orchestration with correct typing and naming conventions.
35-51: LGTM!The dual subscription setup with distinct error handling and
cancelOnError: falseensures resilience.
53-58: LGTM!Proper resource cleanup with safe null-aware cancellation of subscriptions and timers.
60-66: LGTM!Event deduplication prevents reprocessing cached events efficiently.
78-94: LGTM!Private key resolution correctly handles session matching, master key fallback, and graceful skip when neither applies.
319-360: LGTM!Restore session request properly handles master key validation, state initialization, timeout setup, and failure paths.
362-394: LGTM!Order details batching (max 20 per request) with proper master key validation and clear progress logging.
396-583: LGTM!Comprehensive restore handling with proper role detection, dispute restoration via synthetic messages, batched detail loading with timeouts, and deferred trade index updates.
585-646: LGTM!Last trade index request/response flow with completer-based deduplication, appropriate timeout, and proper key index advancement.
648-669: LGTM!Batch response handler safely completes pending order detail requests with proper validation and logging.
703-719: LGTM!Settings update properly clears all restoration state when Mostro instance changes, preventing stale data.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/mostro_service.dart (1)
119-121: Remove decrypted payload from logs.Line 120 logs the full decrypted content, which can leak sensitive data like invoices. Log only metadata (event ID, action, order ID) instead.
Apply this fix:
- _logger.i( - 'Received DM, Event ID: ${decryptedEvent.id ?? event.id} with payload: ${decryptedEvent.content}', - ); + _logger.i( + 'Received DM, Event ID: ${decryptedEvent.id ?? event.id}, action: ${msg.action}, orderId: ${msg.id}', + );
♻️ Duplicate comments (1)
lib/features/key_manager/key_management_screen.dart (1)
683-686: Allow non-ASCII BIP-39 characters in mnemonic validation.The regex
^[a-zA-Z\s]+$only permits ASCII letters, so valid Spanish mnemonics (e.g., "ábaco", "época"), French mnemonics (e.g., "être", "événement"), and Italian mnemonics (e.g., "città", "però") will fail validation, blocking imports for non-English users.Apply this diff to accept Unicode letters:
- final validCharacters = RegExp(r'^[a-zA-Z\s]+$'); + final validCharacters = RegExp(r'^[\p{L}\s]+$', unicode: true); if (!validCharacters.hasMatch(trimmed)) {
🧹 Nitpick comments (3)
lib/features/settings/settings_notifier.dart (1)
54-59: Consider removing redundant inline comments.The inline comments on lines 57-58 repeat information already conveyed by the comprehensive comment on line 54. Consider removing them to reduce redundancy.
// COMPLETE RESET: Clear blacklist and user relays when changing Mostro state = state.copyWith( mostroPublicKey: newValue, - blacklistedRelays: const [], // Clear blacklist - userRelays: const [], // Clear user relays + blacklistedRelays: const [], + userRelays: const [], );lib/features/key_manager/key_management_screen.dart (2)
99-155: Consider improving error handling for partial failures.The current implementation imports the mnemonic before clearing state (which addresses the previous review concern), but if the import succeeds and then a subsequent operation fails (e.g.,
deleteAll()throws), the system is left in an inconsistent state with a new key but old sessions/storage possibly remaining.While the try-catch will show an error, users won't know whether the import succeeded or failed, making recovery difficult.
Consider adding more granular error handling:
Future<void> _importKeyAndRestore() async { final importValue = _importController.text.trim(); if (importValue.isEmpty) { if (mounted) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text(S.of(context)!.mnemonicValidationEmpty), backgroundColor: Colors.red, duration: const Duration(seconds: 3), ), ); } return; } try { final keyManager = ref.read(keyManagerProvider); await keyManager.importMnemonic(importValue); await _loadKeys(); + } catch (e) { + if (mounted) { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text(S.of(context)!.importFailed(e.toString())), + backgroundColor: Colors.red, + duration: const Duration(seconds: 5), + ), + ); + } + return; + } + + try { final sessionNotifier = ref.read(sessionNotifierProvider.notifier); await sessionNotifier.reset(); final mostroStorage = ref.read(mostroStorageProvider); await mostroStorage.deleteAll(); final eventStorage = ref.read(eventStorageProvider); await eventStorage.deleteAll(); final notificationsRepo = ref.read(notificationsRepositoryProvider); await notificationsRepo.clearAll(); ref.read(subscriptionManagerProvider).reinitializeMasterKeySubscription(); await ref.read(mostroServiceProvider).requestRestoreSession(); if (mounted) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text(S.of(context)!.keyImportedSuccessfully), backgroundColor: Colors.green, duration: const Duration(seconds: 3), ), ); } } catch (e) { if (mounted) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( - content: Text(S.of(context)!.importFailed(e.toString())), + content: Text(S.of(context)!.restorationFailed(e.toString())), backgroundColor: Colors.red, duration: const Duration(seconds: 5), ), ); } } }
654-671: Add tooltip or label to restore button for clarity.The restore button only displays an icon without any text label. Users may not understand its purpose, especially since it triggers destructive operations (clearing all sessions, storage, and notifications).
Add a tooltip to improve discoverability:
Expanded( flex: 3, child: OutlinedButton( onPressed: _restoreCurrentUser, style: OutlinedButton.styleFrom( side: const BorderSide(color: AppTheme.activeColor), shape: RoundedRectangleBorder( borderRadius: BorderRadius.circular(8), ), padding: const EdgeInsets.symmetric(vertical: 16), ), - child: const Icon( - LucideIcons.refreshCw, - size: 20, - color: AppTheme.activeColor, + child: Tooltip( + message: S.of(context)!.restoreSession, // Add to localizations + child: const Icon( + LucideIcons.refreshCw, + size: 20, + color: AppTheme.activeColor, + ), ), ), ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/data/models/enums/action.dart(1 hunks)lib/data/models/mostro_message.dart(1 hunks)lib/data/models/payload.dart(3 hunks)lib/features/key_manager/key_management_screen.dart(5 hunks)lib/features/notifications/utils/notification_message_mapper.dart(2 hunks)lib/features/notifications/widgets/notification_item.dart(1 hunks)lib/features/settings/settings_notifier.dart(2 hunks)lib/l10n/intl_en.arb(2 hunks)lib/l10n/intl_es.arb(2 hunks)lib/l10n/intl_it.arb(2 hunks)lib/services/mostro_service.dart(8 hunks)test/mocks.mocks.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/l10n/intl_es.arb
- lib/data/models/payload.dart
- lib/l10n/intl_it.arb
🧰 Additional context used
📓 Path-based instructions (10)
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/notifications/utils/notification_message_mapper.dartlib/features/key_manager/key_management_screen.dartlib/features/settings/settings_notifier.dartlib/features/notifications/widgets/notification_item.dart
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
**/*.dart: Remove unused imports and unused dependencies
Do not add // ignore: must_be_immutable to classes within generated files; rely on regeneration and existing file-level ignores
Files:
lib/features/notifications/utils/notification_message_mapper.dartlib/data/models/mostro_message.dartlib/features/key_manager/key_management_screen.dartlib/features/settings/settings_notifier.dartlib/data/models/enums/action.dartlib/features/notifications/widgets/notification_item.darttest/mocks.mocks.dartlib/services/mostro_service.dart
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Name Riverpod providers as Provider or Notifier
lib/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Use clear, concise English for variable names, function names, and code comments; all comments must be in English
Target zero flutter analyze issues
Use latest Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
After async operations, check mounted before using BuildContext
Use const constructors where possible
Files:
lib/features/notifications/utils/notification_message_mapper.dartlib/data/models/mostro_message.dartlib/features/key_manager/key_management_screen.dartlib/features/settings/settings_notifier.dartlib/data/models/enums/action.dartlib/features/notifications/widgets/notification_item.dartlib/services/mostro_service.dart
lib/data/**
📄 CodeRabbit inference engine (AGENTS.md)
Store persistence and API code under lib/data/
Files:
lib/data/models/mostro_message.dartlib/data/models/enums/action.dart
lib/l10n/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
lib/l10n/*.arb: Maintain ARB files for en/es/it in lib/l10n/ (intl_en.arb, intl_es.arb, intl_it.arb)
When adding a new localization key, update all three ARB files (en, es, it)
Use proper ARB metadata for parameterized strings
After modifying ARB files, run dart run build_runner build -d to regenerate localization
Files:
lib/l10n/intl_en.arb
test/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*.dart: Mirror feature layout for tests under test/
Prefer Mockito fakes generated via build_runner in tests
test/**/*.dart: Unit tests belong in test/; integration tests in integration_test/
Mocks are generated via Mockito; regenerate with dart run build_runner build -d after mock changes
Files:
test/mocks.mocks.dart
test/mocks.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Do not manually edit test/mocks.mocks.dart (Mockito-generated)
Files:
test/mocks.mocks.dart
**/*.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Do not manually edit any *.mocks.dart generated Mockito files
Files:
test/mocks.mocks.dart
**/*.{g.dart,mocks.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add per-item ignore comments to generated files; rely on file-level ignores or regenerate
Files:
test/mocks.mocks.dart
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Place service-layer code in lib/services/
Files:
lib/services/mostro_service.dart
🧠 Learnings (2)
📚 Learning: 2025-10-22T12:29:26.958Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.958Z
Learning: Applies to lib/features/relays/relays_notifier.dart : Implement relay operations: syncWithMostroInstance, removeRelayWithBlacklist, addRelayWithSmartValidation, _handleMostroRelayListUpdate
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.958Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.958Z
Learning: Applies to test/mocks.mocks.dart : Do not manually edit test/mocks.mocks.dart (Mockito-generated)
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 (19)
lib/l10n/intl_en.arb (1)
467-478: LGTM! Restoration and validation strings are clear and well-structured.The new localization keys for the restoration flow and mnemonic validation follow ARB conventions and provide clear, user-friendly messages in proper English.
Also applies to: 895-898
lib/features/notifications/widgets/notification_item.dart (1)
124-127: LGTM! Internal restoration actions handled correctly.The three new restoration-related actions (
restoreSession,orders,lastTradeIndex) intentionally break without navigation, which is appropriate since these are internal system actions rather than user-actionable notifications.lib/data/models/mostro_message.dart (1)
45-45: LGTM! Restore payload support added correctly.The additional
json['restore']fallback extends parsing to handle restoration payloads while preserving the existing fallback chain fororderandcant-dokeys.lib/features/notifications/utils/notification_message_mapper.dart (1)
86-89: LGTM! Consistent notification mapping for restoration actions.The three new restoration-related actions are consistently mapped to generic order update title and message keys, which is appropriate for these internal system actions.
Also applies to: 181-184
lib/data/models/enums/action.dart (1)
41-44: LGTM! New action enum values follow established conventions.The three new restoration-related actions (
restoreSession,orders,lastTradeIndex) follow the existing kebab-case string value pattern and integrate cleanly into the enum.test/mocks.mocks.dart (1)
737-766: Generated mock methods for restoration flow.These three new mock method overrides (
requestRestoreSession,requestOrderDetails,requestLastTradeIndex) were auto-generated by Mockito to support the new restoration methods inMostroService. As this is a generated file, no manual edits should be made here.Based on learnings
lib/services/mostro_service.dart (10)
24-31: Good state management structure for restoration.The new fields properly track restoration state with clear purposes: event deduplication, order detail coordination, timeout management, and progress tracking.
36-50: Dual subscription setup looks solid.Subscribing to both
ordersandmasterKeystreams with separate error handlers and proper cleanup indispose()provides the coverage needed for both regular trading and restoration flows.Also applies to: 55-56
61-92: Event deduplication and private key resolution improved.The addition of
_processedEventIdsprevents duplicate processing, and the refactored private key resolution correctly tries matching session first, then falls back to master key when appropriate.
101-104: Empty list guard properly implemented.Checking both
result is! Listandresult.isEmptybefore accessingresult[0]prevents potential index-out-of-bounds crashes.
334-374: Restoration request method is well-guarded.Good defensive checks: clears stale state, validates master key availability, and sets up a 60-second timeout with proper failure handling.
376-408: Batch request logic is sound.Guarding against empty lists and null master key, then batching up to 20 order IDs per request, is a sensible approach to avoid overwhelming the server or hitting message size limits.
514-523: Role determination now correctly matches trade keys.The role is properly derived by comparing the order's
buyerTradePubkeyandsellerTradePubkeyagainst the derived trade key, addressing the previous issue where all restored sessions were hardcoded toRole.seller.
410-608: Complex restoration flow—ensure thorough testing.The
_handleRestoreResponsemethod orchestrates many async operations: completers, timeouts, role derivation, synthetic message creation, and progress updates. While the logic appears sound, this complexity increases the risk of subtle bugs under edge cases (e.g., partial failures, rapid reconnections, concurrent restores).Ensure integration tests cover:
- Partial restore failures (some orders succeed, others timeout)
- Concurrent restore attempts
- Role mismatch scenarios
- Timeout and completer cleanup under various failure modes
610-671: Last trade index request and response handling look solid.Proper null guards, completer lifecycle management, and timeout handling ensure the key index is updated reliably after restoration completes.
729-743: Good instance-change cleanup.Clearing restoration state and resetting the UI when the Mostro public key changes prevents stale data from persisting across instance switches.
lib/features/settings/settings_notifier.dart (3)
51-52: LGTM: Improved log message clarity.The updated log message "Mostro instance change detected" is clearer and more specific than the previous wording.
61-63: LGTM: Proper async handling added.Good addition of
awaitfor_saveToPrefs()and the success log message. This ensures the save operation completes before the method returns and provides better observability.
14-14: Constructor simplification verified—no breaking changes detected.The removal of the optional
refparameter is safe. The class fields confirm noreffield exists, and the codebase instantiation inintegration_test/test_helpers.dartonly passes the_prefsparameter, matching the updated constructor signature. Therefappearing in provider overrides is a standard Riverpod callback parameter, unrelated to the removed constructor parameter.
Saludos @Catrya Cambios listos para revisiónNuevas funcionalidades
Consideraciones en el backend detectadas al depurar1. Órdenes con estado success no se restauranDespués de que una orden se marca como success, al solicitar la restauración al backend, este no devuelve las órdenes con ese estado, por lo que no es posible restablecerlas en la seccion de my trades en la UI. Antes de la confirmación: Momento del settled: Después de restaurar nuevamente: 2. Órdenes en disputa continúan marcadas como tal tras el releaseAl tener una disputa y posteriormente liberar el pago (released), si se solicitan los datos para la restauración, la orden sigue apareciendo como “en disputa”, aunque ya se haya confirmado. Orden después del settled: Resultado tras la restauración: Quedo atento a cualquier ajuste o solución que sea necesario. |
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
@BraCR10 cuando restauro un usuario que tiene ordenes active o fiatsent, no las muestra en el status correcto, sino que, o no las muestra, o lo hace en el status anterior. Para que las muestre debo dar al botón de reintentar restaurar órdenes, pero debería hacerlo bien a la primera.
- Tenía 1 orden en Active y 2 Pending
- Creé un nuevo usuario
- Restauré el anterior
- En My Trades solo aperecían las 2 Pending
- Luego le di al boton de reintentar restauracion y la orden en Active apareció en Active.
Después de eso continué el intercambio y la orden fue a fiat sent
- volví a crear un nuevo usuario y a restaurar el anterior
- la orden ahora aparecía en Active
- tuve que volver a dar al boton de reintentar para que se pusiera en Fiat-sent
Además de eso, como en 30 min me han salido 2 veces las mismas notificaciones en el movil con la última sesión restaurada, de que contacte a la contraparte, y de que ya está en fiat-sent
|
Además a @grunch al restaurar el usuario, una orden que tenia en status active le salio en status waiting-buyer-invoice |
BraCR10
force-pushed
the
feat/br/restoredata
branch
from
3661b6b to
0d595c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/settings/settings_notifier.dart (1)
58-62: Use English-only commentsReplace Spanish inline comments with English per guidelines.
- blacklistedRelays: const [], // Blacklist vacío - userRelays: const [], // User relays vacíos + blacklistedRelays: const [], // Clear blacklist + userRelays: const [], // Clear user relaysAs per coding guidelines.
♻️ Duplicate comments (2)
lib/services/mostro_service.dart (1)
124-126: Stop logging decrypted payloads (sensitive data leak)Logging decryptedEvent.content may expose invoices/PII. Log metadata only.
Apply this diff:
- _logger.i( - 'Received DM, Event ID: ${decryptedEvent.id ?? event.id} with payload: ${decryptedEvent.content}', - ); + final type = (result[0] is Map && (result[0] as Map).isNotEmpty) + ? (result[0] as Map).keys.first + : 'unknown'; + _logger.i('Received DM, Event ID: ${decryptedEvent.id ?? event.id}, type: $type');Run to find any remaining sensitive logs:
#!/bin/bash rg -n -C2 -e 'decryptedEvent\.content' -e 'with payload:' --type=dart lib/lib/features/settings/settings_notifier.dart (1)
85-88: Remove no-op else branch to avoid unnecessary writes/notificationsWhen oldPubkey == newValue, copying the same value and saving prefs is unnecessary.
- } else { - state = state.copyWith(mostroPublicKey: newValue); - await _saveToPrefs(); - } + }As per coding guidelines.
🧹 Nitpick comments (10)
lib/data/models/restore_message.dart (1)
3-21: Use Action enum constant instead of hardcoded string.The action string
'restore-session'is hardcoded, but based on the changes in notification_message_mapper.dart, there's a correspondingAction.restoreSessionenum value. Consider using a constant from the Action enum or defining a string constant to ensure consistency across the codebase and reduce the risk of typos.+import 'package:mostro_mobile/data/models/enums/action.dart'; + class RestoreMessage { final int version; final String action; RestoreMessage({ this.version = 1, - this.action = 'restore-session', + this.action = 'restore-session', // TODO: Consider using Action.restoreSession.value or a constant });lib/data/models/last_trade_index_message.dart (1)
3-24: Use Action enum constant instead of hardcoded string.Similar to RestoreMessage, the action
'last-trade-index'is hardcoded. For consistency and maintainability, consider using a constant or enum value to define this action string.class LastTradeIndexRequest { final int version; final String action; LastTradeIndexRequest({ this.version = 1, - this.action = 'last-trade-index', + this.action = 'last-trade-index', // TODO: Consider using a constant });lib/services/mostro_service.dart (1)
328-329: Avoid logging full outgoing payloadsLogging order.toJson() can leak invoices or amounts. Log action/id only.
- _logger.i('Sending DM, Event ID: ${event.id} with payload: ${order.toJson()}'); + _logger.i('Sending DM, Event ID: ${event.id}, action: ${order.action}, id: ${order.id}, requestId: ${order.requestId}');lib/features/restore/restore_overlay.dart (1)
30-72: Improve accessibility and insets handling (optional)Wrap content in SafeArea and announce live status for screen readers.
- return Material( + return Material( color: Colors.black.withValues(alpha: 0.85), - child: Center( - child: Container( + child: Center( + child: SafeArea( + child: Container( ... - child: Column( + child: Semantics( + liveRegion: true, + label: S.of(context)!.restoringOrders, + child: Column( mainAxisSize: MainAxisSize.min, children: [ _buildStatusIcon(state.step, iconSize), ... - ], - ), - ), + ], + ), + ), + ), ), );lib/features/key_manager/import_mnemonic_dialog.dart (1)
111-165: Harden TextField for secretsDisable autocorrect/suggestions and capitalization to reduce leakage risk.
TextField( controller: _mnemonicController, minLines: 4, maxLines: 6, + enableSuggestions: false, + autocorrect: false, + textCapitalization: TextCapitalization.none, + keyboardType: TextInputType.visiblePassword, style: const TextStyle(lib/features/key_manager/key_management_screen.dart (2)
79-100: Remove unused private method_cleanup: _importKey is unused and annotated to ignore; delete to keep analyzer clean.
- // ignore: unused_element - Future<void> _importKey() async { - final keyManager = ref.read(keyManagerProvider); - final importValue = _importController.text.trim(); - if (importValue.isNotEmpty) { - try { - await keyManager.importMnemonic(importValue); - await _loadKeys(); - if (mounted) { - ScaffoldMessenger.of(context).showSnackBar( - SnackBar(content: Text(S.of(context)!.keyImportedSuccessfully)), - ); - } - } catch (e) { - if (mounted) { - ScaffoldMessenger.of(context).showSnackBar( - SnackBar(content: Text(S.of(context)!.importFailed(e.toString()))), - ); - } - } - } - }
315-322: Add explicit type for settings parameter (style nit)Annotate
_buildPrivacyCard’s second parameter asSettingsfor clarity.- Widget _buildPrivacyCard(BuildContext context, settings) { + Widget _buildPrivacyCard(BuildContext context, Settings settings) {lib/services/restore_service.dart (1)
57-63: Avoid hardcoded user-facing error strings in servicesService writes English strings to UI via overlay. Prefer localized mapping (e.g., pass error codes or use a notifier API that the UI localizes).
As per coding guidelines.
lib/features/restore/restore_progress_notifier.dart (1)
62-66: Consider using cancellable timers for auto-hide delays.The
Future.delayedcallback will execute even if the notifier is disposed before the delay completes. While themountedcheck prevents issues, using a cancellableTimer(like_timeoutTimer) would provide more consistent lifecycle management and allow immediate cleanup on dispose.Example approach:
Timer? _autoHideTimer; void completeRestore() { _logger.i('Restore completed successfully'); _cancelTimeoutTimer(); _cancelAutoHideTimer(); state = state.copyWith( step: RestoreStep.completed, ); _autoHideTimer = Timer(const Duration(seconds: 1), () { if (mounted) { hide(); } }); } void _cancelAutoHideTimer() { _autoHideTimer?.cancel(); _autoHideTimer = null; } @override void dispose() { _cancelTimeoutTimer(); _cancelAutoHideTimer(); super.dispose(); }lib/features/restore/restore_progress_state.dart (1)
42-45: Consider clamping progress percentage.If
currentProgressexceedstotalProgressdue to a bug,progressPercentagewill return a value greater than 1.0, which might cause UI issues depending on how the progress indicator is implemented.Apply this diff to clamp the value:
double get progressPercentage { if (totalProgress == 0) return 0.0; - return currentProgress / totalProgress; + return (currentProgress / totalProgress).clamp(0.0, 1.0); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
lib/core/app.dart(2 hunks)lib/data/models/enums/action.dart(1 hunks)lib/data/models/last_trade_index_message.dart(1 hunks)lib/data/models/mostro_message.dart(1 hunks)lib/data/models/payload.dart(2 hunks)lib/data/models/restore_data.dart(1 hunks)lib/data/models/restore_message.dart(1 hunks)lib/features/key_manager/import_mnemonic_dialog.dart(1 hunks)lib/features/key_manager/key_management_screen.dart(5 hunks)lib/features/notifications/utils/notification_data_extractor.dart(1 hunks)lib/features/notifications/utils/notification_message_mapper.dart(2 hunks)lib/features/notifications/widgets/notification_item.dart(1 hunks)lib/features/restore/restore_overlay.dart(1 hunks)lib/features/restore/restore_progress_notifier.dart(1 hunks)lib/features/restore/restore_progress_state.dart(1 hunks)lib/features/settings/settings_notifier.dart(3 hunks)lib/features/settings/settings_provider.dart(1 hunks)lib/l10n/intl_en.arb(3 hunks)lib/l10n/intl_es.arb(3 hunks)lib/l10n/intl_it.arb(3 hunks)lib/main.dart(1 hunks)lib/services/last_trade_index_service.dart(1 hunks)lib/services/mostro_service.dart(2 hunks)lib/services/restore_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/data/models/enums/action.dart
- lib/features/notifications/widgets/notification_item.dart
- lib/core/app.dart
- lib/data/models/mostro_message.dart
🧰 Additional context used
📓 Path-based instructions (7)
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/notifications/utils/notification_message_mapper.dartlib/features/key_manager/key_management_screen.dartlib/features/restore/restore_progress_state.dartlib/features/restore/restore_progress_notifier.dartlib/features/settings/settings_provider.dartlib/features/restore/restore_overlay.dartlib/features/notifications/utils/notification_data_extractor.dartlib/features/settings/settings_notifier.dartlib/features/key_manager/import_mnemonic_dialog.dart
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
**/*.dart: Remove unused imports and unused dependencies
Do not add // ignore: must_be_immutable to classes within generated files; rely on regeneration and existing file-level ignores
Files:
lib/features/notifications/utils/notification_message_mapper.dartlib/main.dartlib/features/key_manager/key_management_screen.dartlib/data/models/payload.dartlib/services/last_trade_index_service.dartlib/features/restore/restore_progress_state.dartlib/features/restore/restore_progress_notifier.dartlib/services/mostro_service.dartlib/features/settings/settings_provider.dartlib/data/models/restore_message.dartlib/features/restore/restore_overlay.dartlib/features/notifications/utils/notification_data_extractor.dartlib/data/models/last_trade_index_message.dartlib/services/restore_service.dartlib/data/models/restore_data.dartlib/features/settings/settings_notifier.dartlib/features/key_manager/import_mnemonic_dialog.dart
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Name Riverpod providers as Provider or Notifier
lib/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Use clear, concise English for variable names, function names, and code comments; all comments must be in English
Target zero flutter analyze issues
Use latest Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
After async operations, check mounted before using BuildContext
Use const constructors where possible
Files:
lib/features/notifications/utils/notification_message_mapper.dartlib/main.dartlib/features/key_manager/key_management_screen.dartlib/data/models/payload.dartlib/services/last_trade_index_service.dartlib/features/restore/restore_progress_state.dartlib/features/restore/restore_progress_notifier.dartlib/services/mostro_service.dartlib/features/settings/settings_provider.dartlib/data/models/restore_message.dartlib/features/restore/restore_overlay.dartlib/features/notifications/utils/notification_data_extractor.dartlib/data/models/last_trade_index_message.dartlib/services/restore_service.dartlib/data/models/restore_data.dartlib/features/settings/settings_notifier.dartlib/features/key_manager/import_mnemonic_dialog.dart
lib/main.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Configure timeago locales and app initialization in main.dart
Files:
lib/main.dart
lib/data/**
📄 CodeRabbit inference engine (AGENTS.md)
Store persistence and API code under lib/data/
Files:
lib/data/models/payload.dartlib/data/models/restore_message.dartlib/data/models/last_trade_index_message.dartlib/data/models/restore_data.dart
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Place service-layer code in lib/services/
Files:
lib/services/last_trade_index_service.dartlib/services/mostro_service.dartlib/services/restore_service.dart
lib/l10n/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
lib/l10n/*.arb: Maintain ARB files for en/es/it in lib/l10n/ (intl_en.arb, intl_es.arb, intl_it.arb)
When adding a new localization key, update all three ARB files (en, es, it)
Use proper ARB metadata for parameterized strings
After modifying ARB files, run dart run build_runner build -d to regenerate localization
Files:
lib/l10n/intl_es.arblib/l10n/intl_en.arblib/l10n/intl_it.arb
🧠 Learnings (6)
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/core/app_routes.dart : Use GoRouter for navigation configured in core/app_routes.dart
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/data/models.dart : Export models through lib/data/models.dart
Applied to files:
lib/data/models/payload.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/shared/providers/mostro_database_provider.dart : Initialize Sembast database in shared/providers/mostro_database_provider.dart
Applied to files:
lib/services/mostro_service.dart
📚 Learning: 2025-10-07T17:31:12.117Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T17:31:12.117Z
Learning: Applies to lib/**/*.dart : Name Riverpod providers as <Feature>Provider or <Feature>Notifier
Applied to files:
lib/features/settings/settings_provider.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
PR: MostroP2P/mobile#327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/features/notifications/utils/notification_data_extractor.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/relays/relays_notifier.dart : Implement relay operations: syncWithMostroInstance, removeRelayWithBlacklist, addRelayWithSmartValidation, _handleMostroRelayListUpdate
Applied to files:
lib/features/settings/settings_notifier.dart
🔇 Additional comments (27)
lib/features/settings/settings_provider.dart (1)
9-9: LGTM!The addition of the
refparameter correctly passes the Riverpod reference to the SettingsNotifier, enabling it to interact with other providers as needed for the restoration flow.lib/l10n/intl_en.arb (4)
463-467: LGTM!The refresh-related localization keys are properly formatted and provide clear user-facing messages for the user data refresh feature.
891-893: LGTM!The refresh dialog localization keys are well-structured and provide clear context for the user about what the refresh operation does.
902-910: LGTM!The restore overlay localization keys provide comprehensive status messages for each stage of the restore process, enhancing user experience by keeping them informed of progress.
1163-1174: LGTM!The import mnemonic dialog localization keys are well-organized and provide clear guidance and validation messages for the mnemonic import flow.
lib/l10n/intl_it.arb (4)
414-418: LGTM!The Italian translations for the refresh feature are accurate and follow proper localization conventions.
926-928: LGTM!The Italian translations for the refresh dialog are clear and properly convey the purpose of the operation to Italian-speaking users.
936-945: LGTM!The Italian translations for the restore overlay provide comprehensive status updates throughout the restoration process.
1196-1207: LGTM!The Italian translations for the import mnemonic dialog are accurate and provide clear guidance and error messages for Italian-speaking users.
lib/features/notifications/utils/notification_message_mapper.dart (2)
85-86: LGTM!Correctly handles the new
restoreSessionaction by returning an empty string, which is appropriate since restore operations are internal and don't require user-facing notification titles.
179-180: LGTM!Consistent with the title handling, returning an empty string for the message key is appropriate for internal restore operations.
lib/features/notifications/utils/notification_data_extractor.dart (1)
22-24: LGTM!Correctly prevents notification generation for the internal
restoreSessionaction by returning null. The comment clearly explains the rationale, and this is consistent with the handling in notification_message_mapper.dart.lib/data/models/payload.dart (1)
21-22: ****The payload extraction pattern is correct.
RestoreData.fromJson(json)properly receives the full JSON object and internally extractsjson['restore_data'](as seen inrestore_data.dartline 15), matching the implementation pattern used byCantDo,Dispute, andTextMessage. The pattern is consistent and appropriate.lib/main.dart (1)
42-45: Provider override correctly injects Ref into SettingsNotifierThis fixes the previous inability to access ref inside SettingsNotifier and keeps a single instance in the container. LGTM.
lib/services/mostro_service.dart (3)
74-77: Good defensive check for empty/invalid payloadsGuarding against non-List and empty results prevents index access errors. LGTM.
84-101: Restore and last-trade-index dispatch looks correctBranching to RestoreService and LastTradeIndexService based on action keeps _onData cohesive. LGTM.
102-115: Orders details routing is clearExtracting payload.orders and delegating to RestoreService is the right split. LGTM.
lib/features/key_manager/key_management_screen.dart (3)
174-187: Import and Refresh controls: good UX additionClear split between import and refresh. LGTM.
739-824: Refresh flow looks safeConfirmation + restore call with snackbars and logging reads well. LGTM.
315-431: No SDK compatibility issues—WidgetStateProperty is fully supportedYour project's minimum Flutter version is 3.27.0 (per
pubspec.yaml), which exceeds Flutter 3.19.0 whereWidgetStatePropertywas introduced. The code at lines 415–423 is compatible with your SDK constraints. The implementation is correct as-is.lib/services/last_trade_index_service.dart (1)
45-52: Advancing key index and cleaning temp session is correctSetting index to tradeIndex+1 and deleting 'restore' aligns with restore flow. LGTM.
lib/features/restore/restore_progress_notifier.dart (3)
102-106: LGTM!The conditional timer reset ensures timers are only active when the overlay is visible, preventing unnecessary resource usage.
113-118: LGTM!The dispose method properly cancels the timeout timer before calling super.dispose(), ensuring clean resource management.
9-9: The review comment is unsupported by available project information.The claim that "the PR description states a 60-second timeout" cannot be verified. The git commit message is "fix: resolve BuildContext async gaps in key management" and contains no mention of timeout changes. The code consistently implements a 30-second timeout throughout
RestoreProgressNotifier, and it is properly managed (cancelled and nullified). Without concrete evidence from the PR description or codebase requirements documenting a 60-second timeout expectation, the suggested diff should not be applied.Likely an incorrect or invalid review comment.
lib/features/restore/restore_progress_state.dart (1)
47-52: LGTM!The initial state factory provides a clean, const-optimized starting point for the restore progress flow.
lib/l10n/intl_es.arb (2)
1141-1152: LGTM!The Spanish translations for the import mnemonic dialog and validation errors are clear, grammatically correct, and maintain a consistent tone with the rest of the application.
385-385: No action required—all localization files have been properly updated.All 25 new localization keys have been confirmed in all three ARB files (intl_en.arb, intl_es.arb, intl_it.arb). The coding guidelines have been followed correctly.
Likely an incorrect or invalid review comment.
BraCR10
force-pushed
the
feat/br/restoredata
branch
from
77dc943 to
e83c6dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/subscriptions/subscription_manager.dart (1)
300-304: Admin subscription is dropped by subscribeAll() and never re-initialized.unsubscribeAll() cancels admin, but subscribeAll() only re-subscribes session-based types. Admin channel dies until app restart, breaking restore/last-trade-index flows.
Apply this minimal fix to re-init admin after resubscribing sessions:
void subscribeAll() { unsubscribeAll(); final currentSessions = ref.read(sessionNotifierProvider); _updateAllSubscriptions(currentSessions); + // Re-create admin subscription (was canceled above) + _initializeAdminSubscription(); }Based on learnings.
Also applies to: 306-309
♻️ Duplicate comments (1)
lib/services/restore_service.dart (1)
107-107: Null-guard master key before use.Line 107 force-unwraps
masterKeyPairwithout checking for null. If the master key is unavailable, this will crash the restore flow.Apply this fix:
+ final masterKey = keyManager.masterKeyPair; + if (masterKey == null) { + _logger.w('Cannot process restore data: master key not available'); + progressNotifier.showError('Master key not available'); + return; + } + final List<String> orderIds = []; - final masterKey = keyManager.masterKeyPair!;
🧹 Nitpick comments (3)
lib/features/subscriptions/subscription_manager.dart (3)
80-104: Make admin subscription reactive to master key changes (import/generate).Currently initialized once. If the master key becomes available later or changes, admin won’t update.
Consider listening to keyManagerProvider and re-initializing admin when masterKeyPair transitions from null→non-null or changes:
+ late final ProviderSubscription _keyManagerListener; SubscriptionManager(this.ref) { _initSessionListener(); ref.onDispose(dispose); _initializeExistingSessions(); + _keyManagerListener = ref.listen( + keyManagerProvider, + (prev, next) { + final prevPub = prev?.masterKeyPair?.public; + final currPub = next.masterKeyPair?.public; + if (currPub != null && currPub != prevPub) { + unsubscribeByType(SubscriptionType.admin); + _initializeAdminSubscription(); + } + }, + fireImmediately: false, + ); } @@ void dispose() { _sessionListener.close(); unsubscribeAll(); _ordersController.close(); _chatController.close(); _relayListController.close(); + _keyManagerListener.close(); _adminController.close(); }Based on learnings.
220-246: Guard against null subscriptionId in onCancel.request.subscriptionId! can be null if the ID wasn’t set; avoid a crash during unsubscribe.
final subscription = Subscription( request: request, streamSubscription: streamSubscription, onCancel: () { - ref.read(nostrServiceProvider).unsubscribe(request.subscriptionId!); + final id = request.subscriptionId; + if (id != null) { + ref.read(nostrServiceProvider).unsubscribe(id); + } else { + _logger.w('Unsubscribe called without subscriptionId for $type'); + } }, );
180-189: Avoid duplicating admin filter construction.Admin filter logic exists both in _initializeAdminSubscription and _createFilterForType. Prefer a single source of truth to prevent drift.
Refactor _initializeAdminSubscription to reuse _createFilterForType(SubscriptionType.admin, ...) and only proceed when a filter is returned.
Also applies to: 80-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/core/app.dart(2 hunks)lib/features/restore/restore_overlay.dart(1 hunks)lib/features/restore/restore_progress_notifier.dart(1 hunks)lib/features/restore/restore_progress_state.dart(1 hunks)lib/features/subscriptions/subscription_manager.dart(8 hunks)lib/features/subscriptions/subscription_type.dart(1 hunks)lib/l10n/intl_en.arb(3 hunks)lib/l10n/intl_es.arb(3 hunks)lib/l10n/intl_it.arb(3 hunks)lib/services/last_trade_index_service.dart(1 hunks)lib/services/mostro_service.dart(3 hunks)lib/services/restore_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/restore/restore_progress_notifier.dart
- lib/services/last_trade_index_service.dart
🧰 Additional context used
📓 Path-based instructions (7)
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Place service-layer code in lib/services/
Files:
lib/services/mostro_service.dartlib/services/restore_service.dart
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
**/*.dart: Remove unused imports and unused dependencies
Do not add // ignore: must_be_immutable to classes within generated files; rely on regeneration and existing file-level ignores
Files:
lib/services/mostro_service.dartlib/core/app.dartlib/features/subscriptions/subscription_type.dartlib/features/restore/restore_overlay.dartlib/services/restore_service.dartlib/features/subscriptions/subscription_manager.dartlib/features/restore/restore_progress_state.dart
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Name Riverpod providers as Provider or Notifier
lib/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Use clear, concise English for variable names, function names, and code comments; all comments must be in English
Target zero flutter analyze issues
Use latest Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
After async operations, check mounted before using BuildContext
Use const constructors where possible
Files:
lib/services/mostro_service.dartlib/core/app.dartlib/features/subscriptions/subscription_type.dartlib/features/restore/restore_overlay.dartlib/services/restore_service.dartlib/features/subscriptions/subscription_manager.dartlib/features/restore/restore_progress_state.dart
lib/core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep dependency wiring in lib/core/
Files:
lib/core/app.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/subscriptions/subscription_type.dartlib/features/restore/restore_overlay.dartlib/features/subscriptions/subscription_manager.dartlib/features/restore/restore_progress_state.dart
lib/features/subscriptions/subscription_manager.dart
📄 CodeRabbit inference engine (CLAUDE.md)
SubscriptionManager manages subscriptions, including relay list stream; ensure proper initialization and cleanup
Files:
lib/features/subscriptions/subscription_manager.dart
lib/l10n/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
lib/l10n/*.arb: Maintain ARB files for en/es/it in lib/l10n/ (intl_en.arb, intl_es.arb, intl_it.arb)
When adding a new localization key, update all three ARB files (en, es, it)
Use proper ARB metadata for parameterized strings
After modifying ARB files, run dart run build_runner build -d to regenerate localization
Files:
lib/l10n/intl_it.arblib/l10n/intl_es.arblib/l10n/intl_en.arb
🧠 Learnings (2)
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : SubscriptionManager manages subscriptions, including relay list stream; ensure proper initialization and cleanup
Applied to files:
lib/features/subscriptions/subscription_manager.dart
📚 Learning: 2025-05-06T15:49:26.443Z
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.
Applied to files:
lib/features/subscriptions/subscription_manager.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
lib/services/mostro_service.dart (2)
25-126: LGTM! Admin stream handling looks solid.The new admin subscription and message routing are well-implemented:
- Master key null guard prevents crashes
- Empty/invalid payload guard added
- Admin actions properly routed to corresponding services (restore, last-trade-index)
- Batch orders response correctly delegated to RestoreService
128-179: LGTM! Order data handling improvements applied.The renaming to
_onOrderDataimproves clarity, and the empty list guard (line 156) addresses previous review feedback.lib/features/subscriptions/subscription_type.dart (1)
5-5: LGTM! Clear enum addition.The
adminenum member is well-documented and aligns with the new admin subscription flow.lib/core/app.dart (1)
167-174: LGTM! RestoreOverlay integration is clean.The Stack-based overlay approach ensures the restore progress UI appears above all screens. The overlay conditionally renders via
isVisible, so performance impact is minimal when not in use.lib/features/restore/restore_overlay.dart (1)
1-172: LGTM! Well-structured restore overlay UI.The implementation follows Flutter best practices:
- Uses latest
Color.withValues()API- All user-facing text properly localized via
S.of(context)- Responsive design with screen-width-based sizing
- Clean separation of concerns with helper methods
lib/services/restore_service.dart (3)
30-37: Verify the clearing order for importMnemonicAndRestore.The current implementation calls
restore()(which calls_clearAll()at line 40) after importing the mnemonic. However, this means:
- Import mnemonic (line 34)
- Clear all data (via restore → _clearAll at line 40)
This order seems counterintuitive. If the import fails, we've already cleared everything. Consider moving the clearing step inside
importMnemonicAndRestorebefore the import to match the typical pattern:Future<void> importMnemonicAndRestore(String mnemonic) async { _logger.i('Importing mnemonic and restoring session'); await _clearAll(); // Clear first final keyManager = ref.read(keyManagerProvider); await keyManager.importMnemonic(mnemonic); // Then import await restore(); // Then restore (which won't clear again) }And update
restore()to optionally skip clearing when called from import flow.
163-220: LGTM! Order details handling is well-structured.The
_requestOrderDetailsmethod properly constructs and publishes the request, andprocessOrderDetailscorrectly assigns session roles based on trade pubkeys.
222-244: LGTM! Comprehensive cleanup with proper error handling.The
_clearAllmethod clears all relevant stores (sessions, storage, events, notifications) with appropriate error handling.lib/features/restore/restore_progress_state.dart (1)
1-53: LGTM! Clean and well-structured state model.The
RestoreStepenum andRestoreProgressStateclass follow Flutter/Riverpod best practices:
- Immutable state with const constructor
- Proper copyWith for state updates
- Computed getter for progress percentage
- Factory method for initial state
lib/l10n/intl_en.arb (1)
463-468: LGTM! New localization keys properly defined.All new keys for refresh, import mnemonic, and restore overlay flows are well-structured with appropriate placeholders. The English strings are clear and concise.
Also applies to: 891-893, 1153-1174
lib/l10n/intl_it.arb (1)
414-418: LGTM! Italian translations complete.All new localization keys from the English file are present with Italian translations, satisfying the guideline to update all three ARB files (en, es, it).
Also applies to: 926-928, 1186-1207
lib/l10n/intl_es.arb (2)
870-873: LGTM for new ES strings (refresh dialog, restore overlay, timeout, refreshUser).Also applies to: 1145-1153, 1131-1132, 385-386
385-386: Fix missing metadata for placeholder keys in ARB files.All 25 target keys exist across en/es/it locales. However,
refreshFailedcontains the{error}placeholder but is missing its@refreshFailedmetadata entry in intl_en.arb. Per ARB specification, any key with placeholders must have corresponding metadata defining the placeholder types and descriptions.After adding the metadata, regenerate the localization files:
dart run build_runner build -d⛔ Skipped due to learnings
Learnt from: CR PR: MostroP2P/mobile#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-22T12:29:26.971Z Learning: Applies to lib/l10n/*.arb : After modifying ARB files, run dart run build_runner build -d to regenerate localizationLearnt from: CR PR: MostroP2P/mobile#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-22T12:29:26.971Z Learning: Applies to lib/l10n/*.arb : When adding a new localization key, update all three ARB files (en, es, it)Learnt from: CR PR: MostroP2P/mobile#0 File: AGENTS.md:0-0 Timestamp: 2025-10-07T17:31:12.117Z Learning: Run dart run build_runner build -d when ARB or annotated files change to regenerate localization, mocks, and models
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/key_manager/key_management_screen.dart (1)
30-30: Remove unused import field and method.The
_importControllerfield (line 30) and_importKeymethod (lines 80-100) are no longer used after refactoring the import flow to useImportMnemonicDialog. This creates unnecessary dead code and a potential resource leak since_importControlleris never disposed.Apply this diff to remove the unused code:
- final TextEditingController _importController = TextEditingController(); - @override void initState() { ... - // ignore: unused_element - Future<void> _importKey() async { - final keyManager = ref.read(keyManagerProvider); - final importValue = _importController.text.trim(); - if (importValue.isNotEmpty) { - try { - await keyManager.importMnemonic(importValue); - await _loadKeys(); - if (mounted) { - ScaffoldMessenger.of(context).showSnackBar( - SnackBar(content: Text(S.of(context)!.keyImportedSuccessfully)), - ); - } - } catch (e) { - if (mounted) { - ScaffoldMessenger.of(context).showSnackBar( - SnackBar(content: Text(S.of(context)!.importFailed(e.toString()))), - ); - } - } - } - } -Also applies to: 80-100
🧹 Nitpick comments (2)
lib/features/key_manager/key_management_screen.dart (1)
790-795: Consider sanitizing error messages for better UX.Both error handlers expose raw exception details via
e.toString()to users, which may include technical stack traces or implementation details. While some error context is helpful, consider showing user-friendly messages while logging full details via the logger.For example:
} catch (e) { _logger.e('Refresh failed: $e'); if (!mounted || !context.mounted) return; ScaffoldMessenger.of(context).showSnackBar( - SnackBar(content: Text(S.of(context)!.refreshFailed(e.toString()))), + SnackBar(content: Text(S.of(context)!.refreshFailed( + e is Exception ? e.toString() : S.of(context)!.unexpectedError + ))), ); }Apply similar logic to the import error handler.
Also applies to: 838-843
lib/data/models/orders_request_message.dart (1)
3-28: Consider adding deserialization and documentation.The model currently supports only serialization (toJson). Consider these optional enhancements:
Add fromJson factory: If order responses need to be parsed, a
factory OrdersRequestMessage.fromJson(Map<String, dynamic> json)would be useful.Add documentation: Brief class-level and method-level doc comments would improve maintainability.
Input validation: Consider validating that
orderIdsis non-empty andrequestIdis non-negative, either in the constructor or via an assertion.Example documentation:
/// Represents a request message to fetch multiple orders from the Mostro daemon. /// /// Constructs a nested JSON structure with order metadata and a payload /// containing the list of order IDs to retrieve. class OrdersRequestMessage { // ... existing code ... /// Converts this message to a JSON map structure. /// /// Returns a nested map with 'order' as the root key containing /// version, request_id, action, and a payload with order IDs. Map<String, dynamic> toJson() => { // ... existing code ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/data/models/orders_request_message.dart(1 hunks)lib/features/key_manager/key_management_screen.dart(5 hunks)lib/services/restore_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/services/restore_service.dart
🧰 Additional context used
📓 Path-based instructions (4)
lib/data/**
📄 CodeRabbit inference engine (AGENTS.md)
Store persistence and API code under lib/data/
Files:
lib/data/models/orders_request_message.dart
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
**/*.dart: Remove unused imports and unused dependencies
Do not add // ignore: must_be_immutable to classes within generated files; rely on regeneration and existing file-level ignores
Files:
lib/data/models/orders_request_message.dartlib/features/key_manager/key_management_screen.dart
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Name Riverpod providers as Provider or Notifier
lib/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Use clear, concise English for variable names, function names, and code comments; all comments must be in English
Target zero flutter analyze issues
Use latest Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
After async operations, check mounted before using BuildContext
Use const constructors where possible
Files:
lib/data/models/orders_request_message.dartlib/features/key_manager/key_management_screen.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/key_manager/key_management_screen.dart
🧠 Learnings (1)
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/core/app_routes.dart : Use GoRouter for navigation configured in core/app_routes.dart
Applied to files:
lib/features/key_manager/key_management_screen.dart
🔇 Additional comments (3)
lib/features/key_manager/key_management_screen.dart (2)
174-187: LGTM!The button layout effectively balances the Import button (with text) and the Refresh button (icon-only) using appropriate flex ratios and spacing.
739-818: LGTM!Both dialog methods follow Flutter best practices:
- Proper mounted checks after async operations (lines 786, 792, 834, 840)
- Try-catch error handling with user feedback
- Consistent use of localized strings
- Appropriate logger usage for debugging
The refresh confirmation dialog prevents accidental data loss, and the import flow delegates validation to
ImportMnemonicDialog.Also applies to: 820-846
lib/data/models/orders_request_message.dart (1)
27-27: No changes needed—implementation correctly follows the Mostro protocol.The
[toJson(), null]array format matches the Mostro protocol specification, where messages are JSON-serialized arrays with two elements: [wrapper-object, signature]. The null signature is valid in full-privacy mode, and this pattern is correctly applied across RestoreMessage, OrdersRequestMessage, and LastTradeIndexMessage.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/l10n/intl_es.arb (1)
1160-1161: Update text to reflect support for 12 or 24 word mnemonics.The UI strings still reference only "12 palabras" but the validation logic accepts both 12 and 24 word BIP-39 mnemonics (as stated in PR objectives). This misleads users and may prevent valid 24-word mnemonic imports.
Apply this diff:
- "secretWordsPlaceholder": "Ingresa tus 12 palabras secretas separadas por espacios", - "errorNotTwelveWords": "Debes ingresar exactamente 12 palabras", + "secretWordsPlaceholder": "Ingresa tus 12 o 24 palabras secretas separadas por espacios", + "errorNotTwelveWords": "Debes ingresar exactamente 12 o 24 palabras",As per PR objectives.
🧹 Nitpick comments (1)
lib/features/subscriptions/subscription_manager.dart (1)
80-104: Eliminate code duplication in admin filter creation.The admin filter creation logic (lines 89-92) is duplicated in
_createFilterForType()at lines 187-190. Consider refactoring to use_createFilterForType()directly.Apply this diff to eliminate duplication:
void _initializeAdminSubscription() { try { - final masterKey = ref.read(keyManagerProvider).masterKeyPair; - if (masterKey == null) { + final filter = _createFilterForType(SubscriptionType.admin, []); + if (filter == null) { _logger.i('No master key available, skipping admin subscription'); return; } - final filter = NostrFilter( - kinds: [1059], - p: [masterKey.public], - ); - subscribe( type: SubscriptionType.admin, filter: filter, ); _logger.i('Admin subscription initialized for master key'); } catch (e, stackTrace) { _logger.e('Failed to initialize admin subscription', error: e, stackTrace: stackTrace); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/features/subscriptions/subscription_manager.dart(9 hunks)lib/l10n/intl_en.arb(3 hunks)lib/l10n/intl_es.arb(3 hunks)lib/l10n/intl_it.arb(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/l10n/intl_it.arb
🧰 Additional context used
📓 Path-based instructions (5)
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/subscriptions/subscription_manager.dart
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
**/*.dart: Remove unused imports and unused dependencies
Do not add // ignore: must_be_immutable to classes within generated files; rely on regeneration and existing file-level ignores
Files:
lib/features/subscriptions/subscription_manager.dart
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Name Riverpod providers as Provider or Notifier
lib/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Use clear, concise English for variable names, function names, and code comments; all comments must be in English
Target zero flutter analyze issues
Use latest Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
After async operations, check mounted before using BuildContext
Use const constructors where possible
Files:
lib/features/subscriptions/subscription_manager.dart
lib/features/subscriptions/subscription_manager.dart
📄 CodeRabbit inference engine (CLAUDE.md)
SubscriptionManager manages subscriptions, including relay list stream; ensure proper initialization and cleanup
Files:
lib/features/subscriptions/subscription_manager.dart
lib/l10n/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
lib/l10n/*.arb: Maintain ARB files for en/es/it in lib/l10n/ (intl_en.arb, intl_es.arb, intl_it.arb)
When adding a new localization key, update all three ARB files (en, es, it)
Use proper ARB metadata for parameterized strings
After modifying ARB files, run dart run build_runner build -d to regenerate localization
Files:
lib/l10n/intl_es.arblib/l10n/intl_en.arb
🧠 Learnings (2)
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : SubscriptionManager manages subscriptions, including relay list stream; ensure proper initialization and cleanup
Applied to files:
lib/features/subscriptions/subscription_manager.dart
📚 Learning: 2025-05-06T15:49:26.443Z
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.
Applied to files:
lib/features/subscriptions/subscription_manager.dart
🔇 Additional comments (9)
lib/features/subscriptions/subscription_manager.dart (6)
8-8: LGTM: Admin infrastructure properly added.The import of
key_manager_provider,_adminControllerdeclaration, andadminstream getter follow established patterns for orders/chat/relayList subscriptions.Also applies to: 28-28, 33-33
58-78: LGTM: Correct initialization order.Initializing the admin subscription before session-based subscriptions is appropriate since admin functionality is independent of user sessions.
107-127: LGTM: Admin properly excluded from session-based flows.The admin subscription is correctly kept independent of session lifecycle by skipping it in
_updateAllSubscriptions()and_clearAllSubscriptions().
197-219: LGTM: Admin event routing implemented correctly.The admin case in
_handleEvent()follows the same pattern as orders and chat subscriptions.
221-266: LGTM: Admin stream properly returned.The admin case in
subscribe()correctly returns theadminstream, consistent with other subscription types.
307-313: LGTM: Proper admin lifecycle management.Admin subscription is correctly preserved in
unsubscribeAll()and_adminControlleris properly closed indispose().Also applies to: 380-387
lib/l10n/intl_es.arb (1)
397-403: LGTM! Metadata issue resolved.The ARB metadata for the
refreshFailedplaceholder has been correctly added as requested in the previous review.lib/l10n/intl_en.arb (2)
475-481: LGTM! Metadata correctly defined.The ARB metadata for the
refreshFailedplaceholder is properly structured with the error placeholder.
1176-1196: No issues found—Italian localization file was properly updated.Verification confirms that
lib/l10n/intl_it.arbincludes all new localization keys added in this PR, complying with the coding guidelines requiring all three ARB files (en, es, it) to be updated.
| "secretWordsPlaceholder": "Enter your 12 secret words separated by spaces", | ||
| "errorNotTwelveWords": "You must enter exactly 12 words", |
There was a problem hiding this comment.
Update text to reflect support for 12 or 24 word mnemonics.
The UI strings still reference only "12 words" but the validation logic accepts both 12 and 24 word BIP-39 mnemonics (as stated in PR objectives). This misleads users and may prevent valid 24-word mnemonic imports.
Apply this diff:
- "secretWordsPlaceholder": "Enter your 12 secret words separated by spaces",
- "errorNotTwelveWords": "You must enter exactly 12 words",
+ "secretWordsPlaceholder": "Enter your 12 or 24 secret words separated by spaces",
+ "errorNotTwelveWords": "You must enter exactly 12 or 24 words",As per PR objectives.
🤖 Prompt for AI Agents
In lib/l10n/intl_en.arb around lines 1182 to 1183, the strings currently
reference only "12 words" while the app accepts 12 or 24-word BIP-39 mnemonics;
update the two entries so the placeholder reads something like "Enter your 12 or
24 secret words separated by spaces" and the error reads "You must enter exactly
12 or 24 words" (or equivalent phrasing that clearly indicates support for both
lengths).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/mostro_service.dart (1)
182-183: Avoid logging decrypted payloads in production.Logging
${decryptedEvent.content}exposes decrypted order data (potentially including invoices, amounts, and other sensitive information) to logs. Consider logging only metadata (event ID, action, order ID) instead.Apply this diff to reduce sensitive data exposure:
- _logger.i( - 'Received DM, Event ID: ${decryptedEvent.id ?? event.id} with payload: ${decryptedEvent.content}', - ); + _logger.i( + 'Received DM, Event ID: ${decryptedEvent.id ?? event.id}, action: ${msg.action}, order: ${msg.id}', + );
♻️ Duplicate comments (1)
lib/services/restore_service.dart (1)
106-177: Add null check for master key at line 123 in processRestoreData().The force unwrap is inconsistent with the defensive pattern used 10 lines earlier in the same method (line 55: null check before access). More importantly,
processRestoreData()is invoked asynchronously as an admin message callback (mostro_service.dart:102), not within therestore()flow that validates the master key. Between the initial validation and the async callback execution, the master key could theoretically become unavailable.The codebase establishes a defensive null-check pattern in the service layer (mostro_service.dart:77-79, last_trade_index_service.dart:19-21, subscription_manager.dart:83-85), so the suggested fix aligns with existing conventions.
final keyManager = ref.read(keyManagerProvider); final settings = ref.read(settingsProvider); final List<String> orderIds = []; - final masterKey = keyManager.masterKeyPair!; + final masterKey = keyManager.masterKeyPair; + if (masterKey == null) { + _logger.e('Cannot process restore data: master key not available'); + progressNotifier.showError('Master key not available'); + return; + }
🧹 Nitpick comments (2)
lib/data/repositories/event_storage.dart (2)
6-6: Consider using a named logger for better log filtering.Creating a named logger helps filter logs by component in production environments.
Apply this diff to use a named logger:
- final _logger = Logger(); + final _logger = Logger(printer: PrettyPrinter(methodCount: 0));Or create a named logger:
final _logger = Logger( printer: SimplePrinter(), filter: ProductionFilter(), );
49-85: Excellent documentation and logging; consider adding error handling.The implementation correctly deletes all events and provides clear warnings about the implications of deleting admin events. The logging is informative and helpful for debugging.
Recommended: Add error handling for robustness.
Apply this diff to add error handling around the transaction:
@override Future<void> deleteAll() async { final allRecords = await store.find(db); int adminEventsDeleted = 0; int totalDeleted = 0; - await db.transaction((txn) async { - for (final record in allRecords) { - final eventType = record.value['type'] as String?; - - if (eventType == 'admin') { - adminEventsDeleted++; - } - - await store.record(record.key).delete(txn); - totalDeleted++; - } - }); + try { + await db.transaction((txn) async { + for (final record in allRecords) { + final eventType = record.value['type'] as String?; + + if (eventType == 'admin') { + adminEventsDeleted++; + } + + await store.record(record.key).delete(txn); + totalDeleted++; + } + }); + } catch (e, stackTrace) { + _logger.e('Failed to delete all events', error: e, stackTrace: stackTrace); + rethrow; + } if (adminEventsDeleted > 0) { _logger.w('WARNING: deleteAll deleted $adminEventsDeleted admin events! ' 'This may cause restore message reprocessing on next startup. ' 'Total events deleted: $totalDeleted'); } else { _logger.i('deleteAll: deleted $totalDeleted events (no admin events present)'); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/data/repositories/event_storage.dart(2 hunks)lib/features/key_manager/key_management_screen.dart(6 hunks)lib/features/subscriptions/subscription_manager.dart(9 hunks)lib/services/lifecycle_manager.dart(1 hunks)lib/services/mostro_service.dart(3 hunks)lib/services/restore_service.dart(1 hunks)test/mocks.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
test/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*.dart: Mirror feature layout for tests under test/
Prefer Mockito fakes generated via build_runner in tests
test/**/*.dart: Unit tests belong in test/; integration tests in integration_test/
Mocks are generated via Mockito; regenerate with dart run build_runner build -d after mock changes
Files:
test/mocks.dart
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
**/*.dart: Remove unused imports and unused dependencies
Do not add // ignore: must_be_immutable to classes within generated files; rely on regeneration and existing file-level ignores
Files:
test/mocks.dartlib/services/lifecycle_manager.dartlib/data/repositories/event_storage.dartlib/services/mostro_service.dartlib/features/key_manager/key_management_screen.dartlib/services/restore_service.dartlib/features/subscriptions/subscription_manager.dart
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Place service-layer code in lib/services/
Files:
lib/services/lifecycle_manager.dartlib/services/mostro_service.dartlib/services/restore_service.dart
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Name Riverpod providers as Provider or Notifier
lib/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Use clear, concise English for variable names, function names, and code comments; all comments must be in English
Target zero flutter analyze issues
Use latest Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
After async operations, check mounted before using BuildContext
Use const constructors where possible
Files:
lib/services/lifecycle_manager.dartlib/data/repositories/event_storage.dartlib/services/mostro_service.dartlib/features/key_manager/key_management_screen.dartlib/services/restore_service.dartlib/features/subscriptions/subscription_manager.dart
lib/data/**
📄 CodeRabbit inference engine (AGENTS.md)
Store persistence and API code under lib/data/
Files:
lib/data/repositories/event_storage.dart
lib/data/repositories/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Repository pattern for all data access (repositories under data/repositories)
Files:
lib/data/repositories/event_storage.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/key_manager/key_management_screen.dartlib/features/subscriptions/subscription_manager.dart
lib/features/subscriptions/subscription_manager.dart
📄 CodeRabbit inference engine (CLAUDE.md)
SubscriptionManager manages subscriptions, including relay list stream; ensure proper initialization and cleanup
Files:
lib/features/subscriptions/subscription_manager.dart
🧠 Learnings (3)
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to test/mocks.mocks.dart : Do not manually edit test/mocks.mocks.dart (Mockito-generated)
Applied to files:
test/mocks.dart
📚 Learning: 2025-05-06T15:49:26.443Z
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.
Applied to files:
lib/data/repositories/event_storage.dartlib/features/subscriptions/subscription_manager.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : SubscriptionManager manages subscriptions, including relay list stream; ensure proper initialization and cleanup
Applied to files:
lib/features/subscriptions/subscription_manager.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
lib/data/repositories/event_storage.dart (1)
25-47: Add error handling and clarify null type field behavior.The two concerns in the original review are substantiated:
Events without a
typefield are deleted silently. The code castseventTypeasString?and checksif (eventType != 'admin'). Null values match this condition and are deleted. While other repositories in the codebase (e.g.,notifications_history_repository.dart) defensively handle null type fields with defaults, this code lacks such protection. If events are guaranteed to have atypefield, this is acceptable but should be documented. If malformed events might exist, they'll be silently removed without logging.No error handling around the transaction. If the transaction fails, the method throws without context.
Recommended changes:
Add a try-catch block and document the null handling behavior:
Future<void> deleteAllExceptAdmin() async { final allRecords = await store.find(db); int deletedCount = 0; int preservedCount = 0; - await db.transaction((txn) async { - for (final record in allRecords) { - final eventType = record.value['type'] as String?; - - if (eventType != 'admin') { - await store.record(record.key).delete(txn); - deletedCount++; - } else { - preservedCount++; - } - } - }); + try { + await db.transaction((txn) async { + for (final record in allRecords) { + final eventType = record.value['type'] as String?; + + // Events without a type or with non-admin type are deleted + if (eventType != 'admin') { + await store.record(record.key).delete(txn); + deletedCount++; + } else { + preservedCount++; + } + } + }); + } catch (e, stackTrace) { + _logger.e('Failed to delete non-admin events', error: e, stackTrace: stackTrace); + rethrow; + } _logger.i('deleteAllExceptAdmin: deleted $deletedCount events, preserved $preservedCount admin events'); }lib/services/lifecycle_manager.dart (1)
106-106: LGTM! Admin subscriptions preserved during background transitions.The change from
unsubscribeAll()tounsubscribeAllExceptAdmin()correctly preserves admin subscriptions when the app moves to background, allowing administrative messages (restore-session, last-trade-index) to continue being received.test/mocks.dart (1)
149-151: LGTM! Mock updated to align with production API.The method rename from
unsubscribeAll()tounsubscribeAllExceptAdmin()keeps the mock aligned with the productionSubscriptionManagerinterface. The implementation (clearing all subscriptions) is appropriate for test mocks.lib/features/key_manager/key_management_screen.dart (4)
71-71: LGTM! Admin events preserved during cleanup.Calling
deleteAllExceptAdmin()instead ofdeleteAll()ensures that admin event history is preserved, preventing reprocessing of historical restore-session messages when generating a new master key.
76-78: LGTM! Admin subscription refreshed after key generation.Updating the admin subscription after generating a new master key ensures the subscription filter is correctly configured with the new master key's public key, enabling proper receipt of admin messages.
720-799: LGTM! Refresh flow includes proper confirmation and error handling.The refresh user dialog includes:
- Confirmation dialog to prevent accidental triggers
- Try-catch for error handling
- Mounted checks before showing snackbars
- User-facing success/failure feedback
- Logging for debugging
801-827: LGTM! Import flow includes proper error handling.The import mnemonic dialog flow includes:
- Validation via ImportMnemonicDialog
- Try-catch for error handling
- Mounted checks before showing snackbars
- Key reload after successful import
- User-facing success/failure feedback
- Logging for debugging
lib/services/mostro_service.dart (2)
25-25: LGTM! Admin subscription lifecycle properly managed.The admin subscription is correctly initialized in
init()alongside the orders subscription, with proper error handling andcancelOnError: falseto ensure resilience. Cleanup indispose()cancels both subscriptions.Also applies to: 32-49, 53-54
58-131: LGTM! Admin event processing includes proper validation and routing.The
_onAdminDatamethod includes:
- Duplicate event detection (lines 62-65)
- Master key null guard (lines 76-81)
- Empty/invalid payload checks (lines 89-92)
- Proper routing to restore and last-trade-index services
- Comprehensive error logging
lib/features/subscriptions/subscription_manager.dart (2)
28-33: LGTM! Admin subscription architecture is well-designed.The admin subscription implementation demonstrates good architectural decisions:
- Independent lifecycle: Admin subscriptions are managed separately from session-based subscriptions, initialized unconditionally during startup (lines 64-66)
- Master key integration: Admin filter correctly uses the master key's public key (lines 196-204)
- Graceful degradation: Returns early with logging when master key is unavailable (lines 84-87)
- Refresh mechanism:
updateAdminSubscription()allows re-initialization after key import/generation (lines 113-119)- Proper cleanup: Admin controller closed in dispose (line 400)
- Event routing: Admin events correctly routed to dedicated stream (lines 220-222)
Also applies to: 64-111, 195-204, 220-222, 396-400
316-327: LGTM! Method renamed to reflect admin subscription preservation.Renaming
unsubscribeAll()tounsubscribeAllExceptAdmin()with admin subscription skip (lines 323-324) correctly reflects the behavior and prevents accidental cancellation of the admin channel during routine subscription management.lib/services/restore_service.dart (2)
31-42: LGTM! Restore flow architecture and cleanup logic are well-structured.The restore workflow demonstrates good practices:
- Clear separation:
importMnemonicAndRestoredelegates torestoreafter import, enabling reuse- Admin subscription refresh: Line 39 updates admin subscription after key import
- Progress tracking: Restore progress notifier provides user feedback at each step (lines 50-51, 101)
- Validation: Master key and Mostro config validated before sending request (lines 55-68)
- Error handling: Comprehensive try-catch blocks with progress notifications on failure (lines 99-102)
- State cleanup:
_clearAllremoves sessions, storage, and notifications while preserving admin event history viadeleteAllExceptAdmin()(line 240), preventing reprocessing of historical admin messagesAlso applies to: 44-104, 229-252
206-227: LGTM! Role assignment logic correctly matches trade keys.
processOrderDetailsassigns session roles by comparing the session's trade key public key against the order's buyer/seller trade pubkeys from the backend response. This ensures restored sessions reflect the user's actual role in each order.
|
@Catrya saludos, tengo el siguiente problema: Estado actual del restablecimiento
Problema actualA pesar de que el flujo funciona correctamente en algunos casos, en otras ocasiones se procesan eventos duplicados después de enviar la petición de restore. DudasEstoy pegado con ese problema, necesito saber si estoy previniendo la duplicación de eventos o saber si el enfoque actual no esta bien o si las duplicaciones se manejan diferente. La verificación de duplicación en Sembast es lo correcto? Cualquier orientación o mejora sobre este enfoque lo agradezco. |
|
Hola @BraCR10
antes sí, pero era un error que se corrigió ahora mostrod envía la respuesta a la trade key que envíe el mensaje de solicitud |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
lib/features/restore/order_restorer.dart (1)
6-47: Deduplicate order IDs before requesting detailsWhen an order also has a dispute entry, this helper now returns duplicate IDs, so downstream detail fetches/logs trigger twice for the same order. Gathering into an ordered set fixes that without changing call sites.
- final orderIds = <String>[]; + final orderIds = <String>{}; ... - orderIds.add(orderId); + orderIds.add(orderId); ... - if (orderIds.isNotEmpty) { - _logger.d('Extracted ${orderIds.length} order IDs for details request'); - } - - return orderIds; + final ordered = orderIds.toList(); + if (ordered.isNotEmpty) { + _logger.d('Extracted ${ordered.length} order IDs for details request'); + } + + return ordered;lib/features/restore/session_restorer.dart (2)
31-185: Avoid double-processing order detailsBecause the dispute list can echo entries already covered in
restoreOrders, we currently emit duplicate sessions/messages and repeatedly walk the fullorderDetailslist. Tracking processed IDs and caching the detail map once keeps things linear and prevents duplicate storage writes.- if (restoreOrders != null) { + final processedIds = <String>{}; + final orderDetailsById = <String, Map<String, dynamic>>{}; + if (orderDetails != null) { + for (final detail in orderDetails) { + if (detail is Map<String, dynamic>) { + final id = detail['id'] as String?; + if (id != null) { + orderDetailsById[id] = detail; + } + } + } + } + + Future<Role?> resolveRole(String orderId, int tradeIndex) => + _determineRole(orderId, tradeIndex, orderDetailsById[orderId]); + + if (restoreOrders != null) { for (final order in restoreOrders) { - final orderId = order['order_id'] as String?; + final orderId = order['order_id'] as String?; ... - if (orderId != null && tradeIndex != null) { + if (orderId != null && tradeIndex != null && processedIds.add(orderId)) { final tradeKey = await keyManager.deriveTradeKeyFromIndex(tradeIndex); - final role = await _determineRole(orderId, tradeIndex, orderDetails); + final role = await resolveRole(orderId, tradeIndex); ... - await _saveOrderDetailsAsMessage(orderId, orderDetails, storage); + await _saveOrderDetailsAsMessage(orderId, orderDetailsById[orderId], storage); ... if (restoreDisputes != null) { for (final dispute in restoreDisputes) { - final orderId = dispute['order_id'] as String?; + final orderId = dispute['order_id'] as String?; ... - if (orderId != null && tradeIndex != null) { + if (orderId != null && tradeIndex != null && processedIds.add(orderId)) { final tradeKey = await keyManager.deriveTradeKeyFromIndex(tradeIndex); - final role = await _determineRole(orderId, tradeIndex, orderDetails); + final role = await resolveRole(orderId, tradeIndex); ... - await _saveOrderDetailsAsMessage(orderId, orderDetails, storage); + await _saveOrderDetailsAsMessage(orderId, orderDetailsById[orderId], storage);And adjust
_saveOrderDetailsAsMessageto accept a single detail map instead of the entire list.- Future<void> _saveOrderDetailsAsMessage( - String orderId, - List<dynamic>? orderDetails, - dynamic storage, - ) async { - if (orderDetails == null || orderDetails.isEmpty) { + Future<void> _saveOrderDetailsAsMessage( + String orderId, + Map<String, dynamic>? orderDetail, + dynamic storage, + ) async { + if (orderDetail == null) { ... - Map<String, dynamic>? orderDetail; - - for (final detail in orderDetails) { - if (detail is Map<String, dynamic> && detail['id'] == orderId) { - orderDetail = detail; - break; - } - } - - if (orderDetail == null) { - _logger.w('Order detail not found for order $orderId'); - return; - } - final order = Order.fromJson(orderDetail);
172-181: Use microsecond precision for storage keysTwo restore writes for the same order can happen in the same millisecond (order loop + dispute loop), which risks identical keys. Bumping to microseconds avoids that without extra state.
- final messageKey = '$orderId-restore-${DateTime.now().millisecondsSinceEpoch}'; + final messageKey = '$orderId-restore-${DateTime.now().microsecondsSinceEpoch}';lib/features/restore/restore_service.dart (1)
101-113: Safe force unwrap after validation, but consider defensive guard.The force unwrap of
keyManager.masterKeyPair!at line 107 is currently safe because_validatePrerequisitesvalidates it beforehand. However, this creates a hidden dependency on call order that could break during refactoring.For defensive coding, consider adding a runtime assertion or null-guard:
Future<void> _sendRestoreRequest() async { final keyManager = ref.read(keyManagerProvider); final settings = ref.read(settingsProvider); + + assert(keyManager.masterKeyPair != null, 'Master key must be validated before sending restore request'); final request = await _messageHandler.createRestoreRequest( tradeKey: _tempTradeKey!, masterKey: keyManager.masterKeyPair!, mostroPubkey: settings.mostroPublicKey, ); await ref.read(nostrServiceProvider).publishEvent(request); _logger.d('Restore: request sent'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
lib/data/models/nostr_event.dart(1 hunks)lib/data/repositories/event_storage.dart(1 hunks)lib/features/key_manager/key_management_screen.dart(5 hunks)lib/features/restore/order_restorer.dart(1 hunks)lib/features/restore/restore_message_handler.dart(1 hunks)lib/features/restore/restore_service.dart(1 hunks)lib/features/restore/session_restorer.dart(1 hunks)lib/features/settings/settings_notifier.dart(3 hunks)lib/features/subscriptions/subscription_manager.dart(1 hunks)lib/services/lifecycle_manager.dart(1 hunks)lib/services/mostro_service.dart(1 hunks)lib/services/restore_service.dart(1 hunks)test/mocks.dart(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- lib/features/subscriptions/subscription_manager.dart
- lib/services/lifecycle_manager.dart
- lib/services/mostro_service.dart
- lib/data/repositories/event_storage.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- test/mocks.dart
🧰 Additional context used
📓 Path-based instructions (5)
lib/data/**
📄 CodeRabbit inference engine (AGENTS.md)
Store persistence and API code under lib/data/
Files:
lib/data/models/nostr_event.dart
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
**/*.dart: Remove unused imports and unused dependencies
Do not add // ignore: must_be_immutable to classes within generated files; rely on regeneration and existing file-level ignores
Files:
lib/data/models/nostr_event.dartlib/features/key_manager/key_management_screen.dartlib/features/restore/order_restorer.dartlib/services/restore_service.dartlib/features/restore/restore_message_handler.dartlib/features/restore/session_restorer.dartlib/features/restore/restore_service.dartlib/features/settings/settings_notifier.dart
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Name Riverpod providers as Provider or Notifier
lib/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Use clear, concise English for variable names, function names, and code comments; all comments must be in English
Target zero flutter analyze issues
Use latest Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
After async operations, check mounted before using BuildContext
Use const constructors where possible
Files:
lib/data/models/nostr_event.dartlib/features/key_manager/key_management_screen.dartlib/features/restore/order_restorer.dartlib/services/restore_service.dartlib/features/restore/restore_message_handler.dartlib/features/restore/session_restorer.dartlib/features/restore/restore_service.dartlib/features/settings/settings_notifier.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/key_manager/key_management_screen.dartlib/features/restore/order_restorer.dartlib/features/restore/restore_message_handler.dartlib/features/restore/session_restorer.dartlib/features/restore/restore_service.dartlib/features/settings/settings_notifier.dart
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Place service-layer code in lib/services/
Files:
lib/services/restore_service.dart
🧠 Learnings (20)
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 215
File: lib/features/order/notfiers/order_notifier.dart:0-0
Timestamp: 2025-07-20T23:33:17.689Z
Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Applied to files:
lib/data/models/nostr_event.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/**/screens/**/*.dart : Pass BuildContext to methods that need localization (S.of(context)!.keyName usage)
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/features/key_manager/key_management_screen.dartlib/services/restore_service.dartlib/features/restore/session_restorer.dartlib/features/restore/restore_service.dartlib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/core/app_routes.dart : Use GoRouter for navigation configured in core/app_routes.dart
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/*/providers/**/*.dart : Organize Riverpod providers by feature under features/{feature}/providers/
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-07T17:31:12.117Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T17:31:12.117Z
Learning: Applies to lib/**/*.dart : Name Riverpod providers as <Feature>Provider or <Feature>Notifier
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/generated/l10n.dart : Generated localization entrypoint file is lib/generated/l10n.dart
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/main.dart : Configure timeago locales and app initialization in main.dart
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/data/models.dart : Export models through lib/data/models.dart
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/shared/providers/mostro_database_provider.dart : Initialize Sembast database in shared/providers/mostro_database_provider.dart
Applied to files:
lib/features/key_manager/key_management_screen.dartlib/services/restore_service.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/services/nostr_service.dart : All Nostr protocol interactions must go through NostrService (services/nostr_service.dart)
Applied to files:
lib/services/restore_service.dartlib/features/restore/restore_message_handler.dartlib/features/restore/restore_service.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : SubscriptionManager manages subscriptions, including relay list stream; ensure proper initialization and cleanup
Applied to files:
lib/features/restore/restore_service.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/relays/relays_notifier.dart : Implement relay operations: syncWithMostroInstance, removeRelayWithBlacklist, addRelayWithSmartValidation, _handleMostroRelayListUpdate
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.974Z
Learning: In the Mostro mobile app's RelaySelector widget (lib/features/relays/widgets/relay_selector.dart), watching relaysProvider.notifier correctly triggers rebuilds because the relaysProvider itself depends on settingsProvider (line 8 in relays_provider.dart). When blacklist changes via toggleMostroRelayBlacklist(), the settingsProvider updates, causing relaysProvider to rebuild, which then notifies widgets watching the notifier. The UI correctly reflects active/inactive states in real-time through this dependency chain.
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/settings/settings.dart : Settings model must include blacklistedRelays and maintain backward-compatible serialization
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/notifications/notification_service.dart : Notification service configuration and custom icons should be defined in notification_service.dart
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/core/mostro_fsm.dart : MostroFSM manages order state transitions (core/mostro_fsm.dart)
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/relays/relay.dart : Relay model must track source (user, mostro, defaultConfig) and expose canDelete/canBlacklist semantics
Applied to files:
lib/features/settings/settings_notifier.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 (4)
lib/features/key_manager/key_management_screen.dart (1)
739-844: Solid restore trigger with UI safeguardsThe confirmation dialog plus
mountedguards around the snackbars keep this refresh flow safe, and the added logging will help diagnose restore failures quickly.lib/data/models/nostr_event.dart (1)
268-352: Extension matches separated-key wrap flowThe stepwise build of rumor → seal → gift wrap with distinct key pairs is spot on and keeps the restore payload aligned with the daemon’s expectations.
lib/features/settings/settings_notifier.dart (1)
51-88: Mostro switch rehydrate flow looks goodNice to see the blacklist/user relay reset, prefs flush, and automatic restore chained together—the guard logs on null
refwill be handy if the notifier is reused elsewhere.lib/services/restore_service.dart (1)
1-377: Remove duplicate restore service file.This file at
lib/services/restore_service.dartappears to be superseded by the refactored version atlib/features/restore/restore_service.dart. Having both files creates confusion, maintenance burden, and violates the coding guideline that application feature code should be placed underlib/features/<feature>/.The
lib/features/restore/version:
- Uses
RestoreMessageHandlerfor message creation (eliminating duplication)- Has cleaner structure with
_RestoreDataclass- Uses helper classes
SessionRestorerandOrderRestorer- Better separation of concerns
Remove this file and ensure all references point to
lib/features/restore/restore_service.dart.#!/bin/bash # Verify no imports reference the services/ version rg -n "import.*services/restore_service" --type=dart⛔ Skipped due to learnings
Learnt from: CR Repo: MostroP2P/mobile PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-10-22T12:29:26.971Z Learning: Applies to lib/services/nostr_service.dart : All Nostr protocol interactions must go through NostrService (services/nostr_service.dart)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.Learnt from: CR Repo: MostroP2P/mobile PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-10-22T12:29:26.971Z Learning: Applies to lib/shared/providers/mostro_database_provider.dart : Initialize Sembast database in shared/providers/mostro_database_provider.dart
| void _handleCantDo(dynamic payload) { | ||
| // Check if it's a not_found error (user has no data to restore) | ||
| if (payload != null && payload.toString().toLowerCase().contains('not_found')) { | ||
| _logger.i('Restore: no data found for user (cant-do: not_found)'); | ||
| // Clean up and close the restore process | ||
| _cleanup(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fragile error detection using string matching.
Line 172 uses payload.toString().toLowerCase().contains('not_found') to detect if the user has no data to restore. This string matching is brittle and may break if the backend changes error message format or if the payload structure changes.
Consider checking for structured error codes or specific payload fields instead:
void _handleCantDo(dynamic payload) {
- // Check if it's a not_found error (user has no data to restore)
- if (payload != null && payload.toString().toLowerCase().contains('not_found')) {
+ // Check if it's a not_found error (user has no data to restore)
+ if (payload is Map && payload['error_code'] == 'not_found') {
_logger.i('Restore: no data found for user (cant-do: not_found)');
- // Clean up and close the restore process
_cleanup();
}
}If the backend doesn't provide structured error codes, document the expected error format and add a more robust check.
Committable suggestion skipped: line range outside the PR's diff.
| // Invalidate and rebuild SubscriptionManager and MostroService to ensure fresh state | ||
| ref.invalidate(subscriptionManagerProvider); | ||
| ref.invalidate(mostroServiceProvider); | ||
|
|
||
| // Force rebuild by reading them immediately | ||
| ref.read(subscriptionManagerProvider); | ||
| final mostroService = ref.read(mostroServiceProvider); | ||
| mostroService.init(); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify provider invalidation pattern aligns with Riverpod best practices.
The code invalidates subscriptionManagerProvider and mostroServiceProvider, then immediately reads them and calls init(). This pattern forces a rebuild of these providers.
Please verify this pattern is the intended way to reinitialize services after restore in this codebase. Check if there's a standard service reinitialization pattern documented elsewhere:
🏁 Script executed:
#!/bin/bash
# Search for similar provider invalidation patterns in the codebase
rg -n -A3 -B3 "invalidate.*Provider" --type=dart -g '!**/restore_service.dart'Length of output: 2996
I need to examine the provider definitions and understand the actual pattern better.
Now let me search for the provider definitions and init() method:
I need to examine the codebase directly to understand these providers. Let me search for the provider definitions and the init() method:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for subscriptionManagerProvider and mostroServiceProvider definitions
echo "=== Searching for provider definitions ==="
rg -n "subscriptionManagerProvider|mostroServiceProvider" --type=dart -g '!**/restore_service.dart' -A 2
echo ""
echo "=== Searching for init() method definitions on services ==="
rg -n "\.init\(\)" --type=dart lib/services lib/features -B 2 -A 2
Length of output: 7524
Let me search for the MostroService class definition and init() method to understand what this pattern does:
🏁 Script executed:
#!/bin/bash
# Find MostroService class definition and init() method
echo "=== MostroService class definition and init() method ==="
fd -e dart -x grep -l "class MostroService" {} \;
echo ""
echo "=== Search for init() method in MostroService ==="
rg -n "class MostroService" --type=dart -A 50 | head -100Length of output: 3515
🏁 Script executed:
#!/bin/bash
# Find MostroService init method more specifically
rg -n "void init\(\)|Future.*init\(\)" --type=dart lib/services/ -B 5 -A 10 | grep -A 15 "MostroService\|mostro_service"Length of output: 1181
Remove unnecessary provider invalidation pattern; call init() directly like in lifecycle_manager.dart.
The invalidate-then-read pattern differs from how service reinitialization is done elsewhere in the codebase. In lib/services/lifecycle_manager.dart (lines 61-66), services are reinitialized by directly reading the provider and calling init() without invalidation first:
ref.read(subscriptionManagerProvider).subscribeAll();
ref.read(mostroServiceProvider).init();Replace lines 272-278 in restore_service.dart to follow the same pattern:
ref.read(subscriptionManagerProvider).subscribeAll();
ref.read(mostroServiceProvider).init();Invalidating providers before immediately reading them is not a standard Riverpod pattern and can introduce timing issues. The init() method already handles subscription setup on the existing instance.
🤖 Prompt for AI Agents
In lib/features/restore/restore_service.dart around lines 271-278, remove the
invalidate-then-read pattern and instead directly read the existing providers
and call their reinit methods; replace the ref.invalidate(...) + ref.read(...)
sequence with a direct ref.read(subscriptionManagerProvider).subscribeAll() and
a direct ref.read(mostroServiceProvider).init() so the existing instances are
reused and initialized consistently with lifecycle_manager.dart.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
lib/data/models/nostr_event.dart(1 hunks)lib/data/repositories/event_storage.dart(1 hunks)lib/features/key_manager/key_management_screen.dart(5 hunks)lib/features/restore/order_restorer.dart(1 hunks)lib/features/restore/restore_message_handler.dart(1 hunks)lib/features/restore/restore_service.dart(1 hunks)lib/features/restore/session_restorer.dart(1 hunks)lib/features/settings/settings_notifier.dart(3 hunks)lib/features/subscriptions/subscription_manager.dart(1 hunks)lib/services/lifecycle_manager.dart(1 hunks)lib/services/mostro_service.dart(1 hunks)lib/services/restore_service.dart(1 hunks)test/mocks.dart(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/features/subscriptions/subscription_manager.dart
- lib/data/repositories/event_storage.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/services/mostro_service.dart
- test/mocks.dart
- lib/services/lifecycle_manager.dart
🧰 Additional context used
📓 Path-based instructions (5)
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/restore/restore_message_handler.dartlib/features/key_manager/key_management_screen.dartlib/features/restore/order_restorer.dartlib/features/restore/session_restorer.dartlib/features/restore/restore_service.dartlib/features/settings/settings_notifier.dart
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
**/*.dart: Remove unused imports and unused dependencies
Do not add // ignore: must_be_immutable to classes within generated files; rely on regeneration and existing file-level ignores
Files:
lib/features/restore/restore_message_handler.dartlib/features/key_manager/key_management_screen.dartlib/features/restore/order_restorer.dartlib/services/restore_service.dartlib/data/models/nostr_event.dartlib/features/restore/session_restorer.dartlib/features/restore/restore_service.dartlib/features/settings/settings_notifier.dart
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Name Riverpod providers as Provider or Notifier
lib/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Use clear, concise English for variable names, function names, and code comments; all comments must be in English
Target zero flutter analyze issues
Use latest Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
After async operations, check mounted before using BuildContext
Use const constructors where possible
Files:
lib/features/restore/restore_message_handler.dartlib/features/key_manager/key_management_screen.dartlib/features/restore/order_restorer.dartlib/services/restore_service.dartlib/data/models/nostr_event.dartlib/features/restore/session_restorer.dartlib/features/restore/restore_service.dartlib/features/settings/settings_notifier.dart
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Place service-layer code in lib/services/
Files:
lib/services/restore_service.dart
lib/data/**
📄 CodeRabbit inference engine (AGENTS.md)
Store persistence and API code under lib/data/
Files:
lib/data/models/nostr_event.dart
🧠 Learnings (22)
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/services/nostr_service.dart : All Nostr protocol interactions must go through NostrService (services/nostr_service.dart)
Applied to files:
lib/features/restore/restore_message_handler.dartlib/services/restore_service.dartlib/features/restore/restore_service.dart
📚 Learning: 2025-10-21T21:47:03.451Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:157-182
Timestamp: 2025-10-21T21:47:03.451Z
Learning: In MostroP2P/mobile, for Action.canceled handling in abstract_mostro_notifier.dart (Riverpod StateNotifier), do not add mounted checks after async sessionNotifier.deleteSession(orderId) as they break order state synchronization during app restart. The Action.canceled flow contains critical business logic that must complete fully; Riverpod handles provider disposal automatically. Mounted checks should only protect UI operations, not business logic in StateNotifiers.
Applied to files:
lib/features/key_manager/key_management_screen.dartlib/services/restore_service.dartlib/features/restore/session_restorer.dartlib/features/restore/restore_service.dartlib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/core/app_routes.dart : Use GoRouter for navigation configured in core/app_routes.dart
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used with `Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/*/providers/**/*.dart : Organize Riverpod providers by feature under features/{feature}/providers/
Applied to files:
lib/features/key_manager/key_management_screen.dartlib/features/restore/session_restorer.dart
📚 Learning: 2025-10-07T17:31:12.117Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T17:31:12.117Z
Learning: Applies to lib/**/*.dart : Name Riverpod providers as <Feature>Provider or <Feature>Notifier
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/generated/l10n.dart : Generated localization entrypoint file is lib/generated/l10n.dart
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/main.dart : Configure timeago locales and app initialization in main.dart
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/data/models.dart : Export models through lib/data/models.dart
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/**/screens/**/*.dart : Pass BuildContext to methods that need localization (S.of(context)!.keyName usage)
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/shared/providers/mostro_database_provider.dart : Initialize Sembast database in shared/providers/mostro_database_provider.dart
Applied to files:
lib/features/key_manager/key_management_screen.dartlib/services/restore_service.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/**/*.dart : Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Applied to files:
lib/features/key_manager/key_management_screen.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/core/models/relay_list_event.dart : Parse NIP-65 kind 10002 events, validate WS URLs, and handle timestamps/null-safety
Applied to files:
lib/data/models/nostr_event.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : SubscriptionManager manages subscriptions, including relay list stream; ensure proper initialization and cleanup
Applied to files:
lib/features/restore/restore_service.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.974Z
Learning: In the Mostro mobile app's RelaySelector widget (lib/features/relays/widgets/relay_selector.dart), watching relaysProvider.notifier correctly triggers rebuilds because the relaysProvider itself depends on settingsProvider (line 8 in relays_provider.dart). When blacklist changes via toggleMostroRelayBlacklist(), the settingsProvider updates, causing relaysProvider to rebuild, which then notifies widgets watching the notifier. The UI correctly reflects active/inactive states in real-time through this dependency chain.
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/settings/settings.dart : Settings model must include blacklistedRelays and maintain backward-compatible serialization
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/relays/relays_notifier.dart : Implement relay operations: syncWithMostroInstance, removeRelayWithBlacklist, addRelayWithSmartValidation, _handleMostroRelayListUpdate
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers (use Notifier pattern for complex state logic)
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/notifications/notification_service.dart : Notification service configuration and custom icons should be defined in notification_service.dart
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/core/mostro_fsm.dart : MostroFSM manages order state transitions (core/mostro_fsm.dart)
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-10-22T12:29:26.971Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-22T12:29:26.971Z
Learning: Applies to lib/features/relays/relay.dart : Relay model must track source (user, mostro, defaultConfig) and expose canDelete/canBlacklist semantics
Applied to files:
lib/features/settings/settings_notifier.dart
🔇 Additional comments (4)
lib/data/models/nostr_event.dart (1)
284-349: Dual-key wrap flow looks solidThe separation between rumor and seal key material is implemented cleanly here; the serialization and encryption steps line up with the new restore flow. Nicely done.
lib/features/settings/settings_notifier.dart (1)
85-88: Skip rewriting identical Mostro key stateWhen
oldPubkey == newValue, we still clone state and persist to prefs even though nothing changed. That means we notify listeners and hit disk for no benefit. Dropping the else branch keeps the hot path quiet while preserving the new restore workflow.[ suggest_optional_refactor ]
- } else { - state = state.copyWith(mostroPublicKey: newValue); - await _saveToPrefs(); - } + }lib/features/restore/order_restorer.dart (1)
6-46: Good extraction/logging helpersAppreciate the focused helpers here—collecting IDs and logging detail counts keeps the restore pipeline tidy.
lib/features/key_manager/key_management_screen.dart (1)
788-855: Thanks for capturing context before awaitsNice touch precomputing the scaffold messenger and localized strings before awaiting restore flows—prevents mounted checks from tripping over a stale
BuildContext.
| Future<void> restore() async { | ||
| _logger.i('Restore: initiated'); | ||
|
|
||
| await _clearAll(); | ||
| _reset(); | ||
|
|
||
| final progress = ref.read(restoreProgressProvider.notifier); | ||
| progress.startRestore(); | ||
|
|
||
| try { | ||
| await _validatePrerequisites(); | ||
| await _setupSubscription(); | ||
| await _sendRestoreRequest(); | ||
| } catch (e, stackTrace) { | ||
| _logger.e('Restore: failed', error: e, stackTrace: stackTrace); | ||
| progress.showError(e.toString()); | ||
| rethrow; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Don’t wipe local state before we know restore will succeed
restore() calls _clearAll() up front, so we delete every session, notification, and cached message before verifying prerequisites, subscribing, or receiving any restore data. Any exception (network failure, malformed response, timeout, etc.) leaves the app in a wall‑clock empty state with no way to operate until the user retries, effectively losing all locally restored context. Please postpone _clearAll() until finalization—after we have restore data and just before we persist the new state—so a failed attempt doesn’t brick the client.
One possible fix:
- await _clearAll();
- _reset();
+ _reset();and then invoke _clearAll() at the start of _finalize() (before calling _sessionRestorer.restoreSessions) so we only wipe once success is imminent.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/features/restore/restore_service.dart around lines 47 to 66, the code
calls _clearAll() immediately at the start of restore(), which wipes local
sessions/notifications/caches before prerequisites, subscription, or restore
data are confirmed; move the deletion so failed restores do not leave the app
empty: remove the early _clearAll() call from restore(), and instead invoke
_clearAll() at the start of _finalize() (immediately before persisting/restoring
new state and before calling _sessionRestorer.restoreSessions), keeping _reset()
and progress handling as appropriate; ensure error paths still rethrow and do
not perform any clearing when exceptions occur prior to _finalize().
| for (final order in restoreOrders) { | ||
| final orderId = order['order_id'] as String?; | ||
| final tradeIndex = order['trade_index'] as int?; | ||
|
|
||
| if (orderId != null && tradeIndex != null) { | ||
| final tradeKey = await keyManager.deriveTradeKeyFromIndex(tradeIndex); | ||
| final role = await _determineRole(orderId, tradeIndex, orderDetails); | ||
|
|
||
| final session = Session( | ||
| masterKey: masterKey, | ||
| tradeKey: tradeKey, | ||
| keyIndex: tradeIndex, | ||
| fullPrivacy: settings.fullPrivacyMode, | ||
| startTime: DateTime.now(), | ||
| orderId: orderId, | ||
| role: role, | ||
| ); | ||
|
|
||
| await sessionNotifier.saveSession(session); | ||
|
|
||
| // Save order details as MostroMessage | ||
| await _saveOrderDetailsAsMessage(orderId, orderDetails, storage); | ||
|
|
||
| restoredCount++; | ||
| progress.incrementProgress(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (restoreDisputes != null) { | ||
| for (final dispute in restoreDisputes) { | ||
| final orderId = dispute['order_id'] as String?; | ||
| final tradeIndex = dispute['trade_index'] as int?; | ||
|
|
||
| if (orderId != null && tradeIndex != null) { | ||
| final tradeKey = await keyManager.deriveTradeKeyFromIndex(tradeIndex); | ||
| final role = await _determineRole(orderId, tradeIndex, orderDetails); | ||
|
|
||
| final session = Session( | ||
| masterKey: masterKey, | ||
| tradeKey: tradeKey, | ||
| keyIndex: tradeIndex, | ||
| fullPrivacy: settings.fullPrivacyMode, | ||
| startTime: DateTime.now(), | ||
| orderId: orderId, | ||
| role: role, | ||
| ); | ||
|
|
||
| await sessionNotifier.saveSession(session); | ||
|
|
||
| // Save order details as MostroMessage | ||
| await _saveOrderDetailsAsMessage(orderId, orderDetails, storage); | ||
|
|
||
| restoredCount++; | ||
| progress.incrementProgress(); | ||
| } |
There was a problem hiding this comment.
Restored orders never persist because we read the wrong field
The restore payload for orders/disputes comes back as {id, trade_index, status} (see PR summary), so order['order_id'] is always null. As a result every iteration bails out and no sessions are recreated. The same mistake is repeated for disputes. Please read the correct id field (with an optional fallback if the backend ever sends order_id).
Apply this diff to stop skipping every restored entry:
- final orderId = order['order_id'] as String?;
+ final orderId = (order['id'] ?? order['order_id']) as String?;
...
- final orderId = dispute['order_id'] as String?;
+ final orderId = (dispute['id'] ?? dispute['order_id']) as String?;📝 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.
| for (final order in restoreOrders) { | |
| final orderId = order['order_id'] as String?; | |
| final tradeIndex = order['trade_index'] as int?; | |
| if (orderId != null && tradeIndex != null) { | |
| final tradeKey = await keyManager.deriveTradeKeyFromIndex(tradeIndex); | |
| final role = await _determineRole(orderId, tradeIndex, orderDetails); | |
| final session = Session( | |
| masterKey: masterKey, | |
| tradeKey: tradeKey, | |
| keyIndex: tradeIndex, | |
| fullPrivacy: settings.fullPrivacyMode, | |
| startTime: DateTime.now(), | |
| orderId: orderId, | |
| role: role, | |
| ); | |
| await sessionNotifier.saveSession(session); | |
| // Save order details as MostroMessage | |
| await _saveOrderDetailsAsMessage(orderId, orderDetails, storage); | |
| restoredCount++; | |
| progress.incrementProgress(); | |
| } | |
| } | |
| } | |
| if (restoreDisputes != null) { | |
| for (final dispute in restoreDisputes) { | |
| final orderId = dispute['order_id'] as String?; | |
| final tradeIndex = dispute['trade_index'] as int?; | |
| if (orderId != null && tradeIndex != null) { | |
| final tradeKey = await keyManager.deriveTradeKeyFromIndex(tradeIndex); | |
| final role = await _determineRole(orderId, tradeIndex, orderDetails); | |
| final session = Session( | |
| masterKey: masterKey, | |
| tradeKey: tradeKey, | |
| keyIndex: tradeIndex, | |
| fullPrivacy: settings.fullPrivacyMode, | |
| startTime: DateTime.now(), | |
| orderId: orderId, | |
| role: role, | |
| ); | |
| await sessionNotifier.saveSession(session); | |
| // Save order details as MostroMessage | |
| await _saveOrderDetailsAsMessage(orderId, orderDetails, storage); | |
| restoredCount++; | |
| progress.incrementProgress(); | |
| } | |
| for (final order in restoreOrders) { | |
| final orderId = (order['id'] ?? order['order_id']) as String?; | |
| final tradeIndex = order['trade_index'] as int?; | |
| if (orderId != null && tradeIndex != null) { | |
| final tradeKey = await keyManager.deriveTradeKeyFromIndex(tradeIndex); | |
| final role = await _determineRole(orderId, tradeIndex, orderDetails); | |
| final session = Session( | |
| masterKey: masterKey, | |
| tradeKey: tradeKey, | |
| keyIndex: tradeIndex, | |
| fullPrivacy: settings.fullPrivacyMode, | |
| startTime: DateTime.now(), | |
| orderId: orderId, | |
| role: role, | |
| ); | |
| await sessionNotifier.saveSession(session); | |
| // Save order details as MostroMessage | |
| await _saveOrderDetailsAsMessage(orderId, orderDetails, storage); | |
| restoredCount++; | |
| progress.incrementProgress(); | |
| } | |
| } | |
| } | |
| if (restoreDisputes != null) { | |
| for (final dispute in restoreDisputes) { | |
| final orderId = (dispute['id'] ?? dispute['order_id']) as String?; | |
| final tradeIndex = dispute['trade_index'] as int?; | |
| if (orderId != null && tradeIndex != null) { | |
| final tradeKey = await keyManager.deriveTradeKeyFromIndex(tradeIndex); | |
| final role = await _determineRole(orderId, tradeIndex, orderDetails); | |
| final session = Session( | |
| masterKey: masterKey, | |
| tradeKey: tradeKey, | |
| keyIndex: tradeIndex, | |
| fullPrivacy: settings.fullPrivacyMode, | |
| startTime: DateTime.now(), | |
| orderId: orderId, | |
| role: role, | |
| ); | |
| await sessionNotifier.saveSession(session); | |
| // Save order details as MostroMessage | |
| await _saveOrderDetailsAsMessage(orderId, orderDetails, storage); | |
| restoredCount++; | |
| progress.incrementProgress(); | |
| } |
🤖 Prompt for AI Agents
In lib/features/restore/session_restorer.dart around lines 32 to 87, the code
reads order['order_id'] which is null because the payload uses id; change the
extraction to prefer order['id'] and fall back to order['order_id'] (e.g. final
orderId = (order['id'] ?? order['order_id']) as String?); apply the same change
for disputes, keeping the rest of the logic (deriveTradeKeyFromIndex,
_determineRole, creating and saving Session, saving message, incrementing
counters) unchanged so restored entries are not skipped.
| } catch (e) { | ||
| _logger.e('Error processing all data: $e'); | ||
| _cleanupTempSession(); | ||
| } | ||
| } | ||
|
|
||
| void _cleanupTempSession() { | ||
| _tempSubscription?.cancel(); | ||
| _tempSubscription = null; | ||
| _tempTradeKey1 = null; | ||
| _processedEventIds.clear(); | ||
|
|
||
| _receivedRestoreData = false; | ||
| _receivedOrdersInfo = false; | ||
| _receivedLastTradeIndex = false; | ||
|
|
||
| _tempRestoreOrders = null; | ||
| _tempRestoreDisputes = null; | ||
| _tempOrdersDetails = null; | ||
| _tempLastTradeIndex = null; | ||
|
|
||
| final progressNotifier = ref.read(restoreProgressProvider.notifier); | ||
| progressNotifier.completeRestore(); | ||
|
|
||
| _logger.i('Restore completed'); |
There was a problem hiding this comment.
Do not report restore success after an exception
If _processAllData() throws, we catch it, call _cleanupTempSession(), and that method unconditionally fires progressNotifier.completeRestore(). The overlay therefore reports success even though the restore failed, and we never surface an error to the user.
Guard the cleanup routine so only the happy path calls completeRestore(), and route failures to showError(...) (or similar). For example:
- } catch (e) {
- _logger.e('Error processing all data: $e');
- _cleanupTempSession();
- }
+ } catch (e) {
+ _logger.e('Error processing all data: $e');
+ _cleanupTempSession(success: false, errorMessage: e.toString());
+ }
}
- void _cleanupTempSession() {
+ void _cleanupTempSession({bool success = true, String? errorMessage}) {
_tempSubscription?.cancel();
_tempSubscription = null;
_tempTradeKey1 = null;
_processedEventIds.clear();
@@
- final progressNotifier = ref.read(restoreProgressProvider.notifier);
- progressNotifier.completeRestore();
-
- _logger.i('Restore completed');
+ final progressNotifier = ref.read(restoreProgressProvider.notifier);
+ if (success) {
+ progressNotifier.completeRestore();
+ _logger.i('Restore completed');
+ } else {
+ progressNotifier.showError(errorMessage ?? 'Restore failed');
+ _logger.w('Restore aborted due to errors');
+ }
}🤖 Prompt for AI Agents
In lib/services/restore_service.dart around lines 329 to 353, the cleanup
routine unconditionally calls progressNotifier.completeRestore() which reports
success even when _processAllData() throws; change the flow so
_cleanupTempSession() only clears subscriptions/state and DOES NOT call
completeRestore(), and instead call completeRestore() from the successful/happy
path after _processAllData() finishes; in the catch block, after cleaning up,
call the error surface method (e.g. showError(...) or a
progressNotifier.fail/error method) to inform the user and log the exception,
ensuring no success signal is emitted on failure.
| /// - rumorKeys: The keys used to sign the rumor (trade key) | ||
| /// - sealKeys: The keys used to sign the seal (master/identity key) | ||
| /// - receiverPubkey: The receiver's public key (Mostro pubkey) | ||
| Future<NostrEvent> mostroWrapWithSeparateKeys({ |
There was a problem hiding this comment.
why create a new function to build gift wrap event there is mostroWrap() in this same file?
|
solved in #355 |
3 participants
This PR incorporates the process to restore orders and disputes using the user mnemonic keys
How Orders Are Marked as Owned
The restore session feature works by reconstructing user sessions from backend data, which automatically marks orders and disputes as owned through the existing session-based ownership detection system.
Closes #78
Current Flow:
- User imports mnemonic via import dialog
- App sends encrypted restore-session action to Mostro daemon
- Request is encrypted to master key derived from mnemonic
- Returns array of {id, trade_index, status} for each order
for (final orderInfo in restoreData.orders) {
// Derive the same trade key that was originally used
final tradeKey = await keyManager.deriveTradeKeyFromIndex(orderInfo.tradeIndex);
}
4. Automatic Ownership Detection (mobile/lib/features/home/widgets/order_list_item.dart:44-50)
final session = ref.watch(sessionProvider(order.orderId!));
if (session != null && session.role != null) {
orderLabel = session.role == Role.seller
? S.of(context)!.youAreSelling.toUpperCase()
: S.of(context)!.youAreBuying.toUpperCase();
}
Key Points:
Additional Features Implemented
Validation rules enforced:
Note
Currently, all sessions restored at line 362 in mostro_service.dart are hardcoded with Role.seller. This causes buyer orders to incorrectly display seller information,
affecting:
Proposed Solutions:
Option 1 (Recommended): Add a role field to the current restore-session payload response:
Option 2: Wait for the upcoming orders endpoint feature that returns complete order data, then make a second call after restore to fetch full details.
Documentation
Recommendation: If it is possible, Option 1 is more efficient as it avoids double network calls and provides immediate correct role assignment during the restore flow. The role field is minimal data (one string per order) and critical for proper client-side behavior.
Summary by CodeRabbit
Release Notes
New Features
Localization