Feat: Add in-app logs screen and export feature #370
Conversation
Introduces an in-app screen to view and export application logs for debugging and support purposes. Replaces the `logger` package with a custom `logger_service` to: - Capture all logs in a memory buffer for display in the app. - Enhance data privacy: ensure sensitive data like keys and mnemonics are scrubbed from the logs before they are stored or exported. Adds a new route `/logs` with a UI to filter, search, and export logs. The screen includes options to share or save logs to the device. Updates background services, repositories, notifiers and other components to leverage the new logger service.
WalkthroughAdds a centralized, isolate-aware logging system with in-memory storage and cross-isolate forwarding, a Logs UI and Settings integration, wires background isolates to forward logs to the main isolate, and refactors many files to use the new logger service. Changes
Sequence Diagram(s)sequenceDiagram
participant BG as Background Isolate
participant IsoOut as IsolateLogOutput
participant SP as SendPort
participant Main as Main Isolate
participant RP as ReceivePort
participant Mem as MemoryLogOutput
participant UI as LogsScreen
BG->>IsoOut: logger.log(event)
IsoOut->>SP: send(serialized log map)
SP->>RP: deliver(log map)
RP->>Main: onMessage -> addLogFromIsolate(logMap)
Main->>Mem: append LogEntry (enforce Config limits)
UI->>Mem: getAllLogs()/subscribe -> render list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
BraCR10
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
lib/services/nostr_service.dart (1)
22-26: Consider if fallback is necessary.The new
settingsgetter provides a fallback when_settingsis null. However, this raises a question: when would_settingsbe null after proper initialization?If
_settingscan legitimately be null (e.g., beforeinit()is called), then callers would get a Settings object with empty relays, which would cause failures elsewhere (like inpublishEventat line 124).Consider whether it would be better to:
- Make
_settingsnon-nullable and require initialization before construction, or- Throw an exception when accessed before initialization
This would make bugs more visible rather than silently providing invalid default settings.
lib/features/logs/screens/logs_screen.dart (1)
483-491: Consider logging export errors for debugging.The caught exception is discarded, making it difficult to diagnose export failures.
} catch (e) { + logger.e('Failed to share logs: $e'); if (mounted) { _showErrorSnackBar(exportFailedMsg); }lib/features/settings/settings.dart (1)
34-47: Consider adding a clear flag for consistency.Unlike
defaultLightningAddresswhich hasclearDefaultLightningAddress, there's no way to explicitly clearcustomLogStorageDirectoryback to null viacopyWith. This may be intentional if resetting to default is not a use case.If users need to reset to the default storage location, consider adding a
clearCustomLogStorageDirectoryparameter similar to line 31.lib/services/logger_service.dart (1)
85-89: Hex redaction may be over-aggressive.The regex
[0-9a-f]{64}will match any 64-character lowercase hex string, including legitimate data like event IDs, order hashes, or other non-sensitive identifiers. This could make debugging harder.Consider being more targeted, such as only redacting hex strings that appear in sensitive contexts (e.g., after
"key":patterns), or accept that some non-sensitive data may be redacted for safety.- .replaceAll(RegExp(r'[0-9a-f]{64}'), '[HEX]') + // Only redact hex that looks like keys (near key-related terms) + .replaceAll(RegExp(r'(?:key|secret|private)["\s:]*[0-9a-f]{64}', caseSensitive: false), '[REDACTED_KEY]')Alternatively, document this behavior so developers understand why certain IDs appear redacted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
lib/background/background.dart(2 hunks)lib/background/desktop_background_service.dart(2 hunks)lib/background/mobile_background_service.dart(7 hunks)lib/core/app_routes.dart(3 hunks)lib/core/config.dart(1 hunks)lib/core/deep_link_handler.dart(5 hunks)lib/core/deep_link_interceptor.dart(5 hunks)lib/data/repositories/dispute_repository.dart(5 hunks)lib/data/repositories/mostro_storage.dart(4 hunks)lib/data/repositories/open_orders_repository.dart(4 hunks)lib/features/chat/chat_room_provider.dart(3 hunks)lib/features/chat/notifiers/chat_room_notifier.dart(11 hunks)lib/features/chat/notifiers/chat_rooms_notifier.dart(5 hunks)lib/features/chat/providers/chat_room_providers.dart(2 hunks)lib/features/disputes/notifiers/dispute_chat_notifier.dart(21 hunks)lib/features/logs/providers/logs_provider.dart(1 hunks)lib/features/logs/screens/logs_screen.dart(1 hunks)lib/features/notifications/services/background_notification_service.dart(6 hunks)lib/features/notifications/utils/notification_data_extractor.dart(2 hunks)lib/features/order/models/order_state.dart(8 hunks)lib/features/order/notfiers/abstract_mostro_notifier.dart(6 hunks)lib/features/order/notfiers/add_order_notifier.dart(1 hunks)lib/features/order/notfiers/order_notifier.dart(6 hunks)lib/features/rate/rate_counterpart_screen.dart(2 hunks)lib/features/relays/relays_notifier.dart(23 hunks)lib/features/restore/restore_manager.dart(22 hunks)lib/features/restore/restore_progress_notifier.dart(7 hunks)lib/features/settings/settings.dart(5 hunks)lib/features/settings/settings_notifier.dart(7 hunks)lib/features/settings/settings_screen.dart(2 hunks)lib/features/subscriptions/subscription_manager.dart(11 hunks)lib/features/trades/providers/trades_provider.dart(4 hunks)lib/features/trades/widgets/status_filter_widget.dart(2 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_es.arb(1 hunks)lib/l10n/intl_it.arb(1 hunks)lib/services/deep_link_service.dart(9 hunks)lib/services/lifecycle_manager.dart(4 hunks)lib/services/logger_service.dart(1 hunks)lib/services/mostro_service.dart(10 hunks)lib/services/nostr_service.dart(16 hunks)lib/shared/notifiers/session_notifier.dart(7 hunks)lib/shared/widgets/notification_listener_widget.dart(2 hunks)lib/shared/widgets/pay_lightning_invoice_widget.dart(3 hunks)test/mocks.mocks.dart(3 hunks)
🧰 Additional context used
🧠 Learnings (17)
📚 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/order/notfiers/add_order_notifier.dartlib/features/settings/settings_notifier.dartlib/features/logs/providers/logs_provider.dartlib/features/relays/relays_notifier.dartlib/features/order/models/order_state.dartlib/shared/widgets/notification_listener_widget.dartlib/features/notifications/services/background_notification_service.dartlib/features/chat/chat_room_provider.dartlib/services/deep_link_service.dartlib/features/notifications/utils/notification_data_extractor.dartlib/data/repositories/open_orders_repository.dartlib/data/repositories/mostro_storage.dartlib/features/chat/providers/chat_room_providers.dartlib/features/order/notfiers/order_notifier.dartlib/services/lifecycle_manager.dartlib/features/chat/notifiers/chat_room_notifier.dartlib/background/background.dartlib/features/restore/restore_manager.dartlib/shared/notifiers/session_notifier.dartlib/data/repositories/dispute_repository.dartlib/features/trades/providers/trades_provider.dartlib/features/rate/rate_counterpart_screen.dartlib/services/mostro_service.dartlib/features/subscriptions/subscription_manager.dartlib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/trades/widgets/status_filter_widget.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/order/notfiers/add_order_notifier.dartlib/features/relays/relays_notifier.dartlib/shared/widgets/notification_listener_widget.dartlib/features/chat/chat_room_provider.dartlib/services/deep_link_service.dartlib/features/chat/notifiers/chat_rooms_notifier.dartlib/features/notifications/utils/notification_data_extractor.dartlib/features/chat/providers/chat_room_providers.dartlib/features/restore/restore_progress_notifier.dartlib/features/order/notfiers/order_notifier.dartlib/services/lifecycle_manager.dartlib/features/chat/notifiers/chat_room_notifier.dartlib/features/restore/restore_manager.dartlib/shared/notifiers/session_notifier.dartlib/features/order/notfiers/abstract_mostro_notifier.dartlib/data/repositories/dispute_repository.dartlib/features/trades/providers/trades_provider.dartlib/features/rate/rate_counterpart_screen.dartlib/services/mostro_service.dartlib/features/subscriptions/subscription_manager.dartlib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/trades/widgets/status_filter_widget.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/order/notfiers/add_order_notifier.dartlib/features/settings/settings_notifier.dartlib/features/logs/providers/logs_provider.dartlib/features/relays/relays_notifier.dartlib/features/order/models/order_state.dartlib/features/notifications/services/background_notification_service.dartlib/features/chat/chat_room_provider.dartlib/services/deep_link_service.dartlib/features/notifications/utils/notification_data_extractor.dartlib/data/repositories/open_orders_repository.dartlib/data/repositories/mostro_storage.dartlib/features/chat/providers/chat_room_providers.dartlib/features/order/notfiers/order_notifier.dartlib/services/lifecycle_manager.dartlib/features/chat/notifiers/chat_room_notifier.dartlib/background/background.dartlib/features/restore/restore_manager.dartlib/shared/notifiers/session_notifier.dartlib/background/desktop_background_service.dartlib/features/order/notfiers/abstract_mostro_notifier.dartlib/data/repositories/dispute_repository.dartlib/features/trades/providers/trades_provider.dartlib/background/mobile_background_service.dartlib/features/rate/rate_counterpart_screen.dartlib/services/mostro_service.dartlib/features/subscriptions/subscription_manager.dartlib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/trades/widgets/status_filter_widget.dartlib/shared/widgets/pay_lightning_invoice_widget.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/features/order/notfiers/add_order_notifier.dartlib/core/config.dartlib/shared/widgets/notification_listener_widget.dartlib/features/notifications/services/background_notification_service.dartlib/features/restore/restore_progress_notifier.dartlib/features/order/notfiers/order_notifier.dartlib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/trades/providers/trades_provider.dartlib/features/rate/rate_counterpart_screen.dartlib/features/disputes/notifiers/dispute_chat_notifier.dart
📚 Learning: 2025-08-15T01:37:12.243Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.
Applied to files:
lib/features/settings/settings_notifier.dartlib/features/chat/chat_room_provider.dartlib/features/notifications/utils/notification_data_extractor.dartlib/features/chat/providers/chat_room_providers.dartlib/services/lifecycle_manager.dartlib/background/background.dartlib/data/repositories/dispute_repository.dartlib/services/mostro_service.dartlib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/trades/widgets/status_filter_widget.dartlib/shared/widgets/pay_lightning_invoice_widget.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.dartlib/features/relays/relays_notifier.dartlib/features/chat/notifiers/chat_rooms_notifier.dartlib/background/background.dartlib/services/nostr_service.dartlib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/trades/providers/trades_provider.dartlib/services/mostro_service.dartlib/features/subscriptions/subscription_manager.dart
📚 Learning: 2025-05-06T15:46:08.942Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/nostr_service.dart:3-4
Timestamp: 2025-05-06T15:46:08.942Z
Learning: The file in dart_nostr library is named "ease.dart" (not "eose.dart" as might be expected), as confirmed by the project maintainer.
Applied to files:
lib/features/relays/relays_notifier.dart
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 272
File: lib/features/relays/relays_notifier.dart:41-49
Timestamp: 2025-08-19T17:54:15.016Z
Learning: In the Mostro mobile app relay synchronization system, the hardcoded 'wss://relay.mostro.network' relay in RelaysNotifier._loadRelays() is intentional for bootstrapping. Config.nostrRelays only contains this single active relay anyway (other entries are commented-out dev relays), so hardcoding is functionally equivalent and more explicit about the bootstrap requirement for fetching kind 10002 relay list events.
Applied to files:
lib/features/relays/relays_notifier.dartlib/services/nostr_service.dartlib/features/subscriptions/subscription_manager.dart
📚 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/features/relays/relays_notifier.dartlib/features/notifications/services/background_notification_service.dartlib/data/repositories/open_orders_repository.dartlib/features/order/notfiers/order_notifier.dartlib/features/chat/notifiers/chat_room_notifier.dartlib/background/background.dartlib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-06-26T15:03:23.529Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
Applied to files:
lib/features/order/models/order_state.dartlib/shared/widgets/notification_listener_widget.dartlib/features/order/notfiers/order_notifier.dartlib/features/chat/notifiers/chat_room_notifier.dartlib/features/restore/restore_manager.dartlib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/trades/providers/trades_provider.dart
📚 Learning: 2025-06-08T23:54:09.987Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 110
File: lib/features/trades/widgets/mostro_message_detail_widget.dart:134-141
Timestamp: 2025-06-08T23:54:09.987Z
Learning: In the OrderState refactor, public keys should be accessed via `tradeState.peer?.publicKey` from the OrderState instance rather than through `session?.peer?.publicKey`, as the OrderState encapsulates peer information directly.
Applied to files:
lib/features/order/models/order_state.dartlib/features/order/notfiers/order_notifier.dartlib/features/restore/restore_manager.dartlib/shared/notifiers/session_notifier.dartlib/services/mostro_service.dart
📚 Learning: 2025-07-08T05:40:47.527Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 127
File: lib/data/models/cant_do.dart:10-27
Timestamp: 2025-07-08T05:40:47.527Z
Learning: In the CantDo model parsing in lib/data/models/cant_do.dart, the inconsistent key naming between 'cant_do' (top-level) and 'cant-do' (nested) is required by the protocol specification and should not be changed as it would break message parsing compatibility.
Applied to files:
lib/shared/widgets/notification_listener_widget.dart
📚 Learning: 2025-05-08T16:31:29.582Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
Applied to files:
lib/services/deep_link_service.dartlib/data/repositories/open_orders_repository.dartlib/features/chat/notifiers/chat_room_notifier.dartlib/services/nostr_service.dart
📚 Learning: 2025-05-06T22:23:06.684Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/features/chat/notifiers/chat_rooms_notifier.dart:0-0
Timestamp: 2025-05-06T22:23:06.684Z
Learning: The `reload()` method in the chat_room_notifier is synchronous, so await is not required or possible.
Applied to files:
lib/features/chat/notifiers/chat_rooms_notifier.dart
📚 Learning: 2025-05-08T16:29:52.154Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/background/background.dart:74-77
Timestamp: 2025-05-08T16:29:52.154Z
Learning: In the Mostro Mobile background service architecture, events aren't stored by the background process. Instead, the background service only checks if events exist in the eventStore and sends notifications for new ones, while the foreground process is responsible for storing and processing events. This is an intentional design decision to separate concerns.
Applied to files:
lib/data/repositories/open_orders_repository.dartlib/background/background.dart
📚 Learning: 2025-06-04T19:35:20.209Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 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:
lib/services/lifecycle_manager.dartlib/features/trades/providers/trades_provider.dartlib/services/mostro_service.dart
📚 Learning: 2025-05-06T15:49:55.079Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:97-103
Timestamp: 2025-05-06T15:49:55.079Z
Learning: In the Mostro protocol, an order cannot be canceled unless it has a valid orderId, so null checks on orderId during cancel operations are unnecessary.
Applied to files:
lib/services/nostr_service.dartlib/features/order/notfiers/abstract_mostro_notifier.dart
🪛 RuboCop (1.81.7)
lib/l10n/intl_es.arb
[warning] 1211-1211: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
[warning] 1212-1212: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
lib/l10n/intl_it.arb
[warning] 1266-1266: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
[warning] 1267-1267: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
lib/l10n/intl_en.arb
[warning] 1233-1233: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
[warning] 1234-1234: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
⏰ 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 (60)
lib/shared/widgets/notification_listener_widget.dart (1)
3-3: LGTM! Clean migration to centralized logger.The import and logging call have been correctly updated to use the global logger service, maintaining the same logging behavior while aligning with the PR's centralized logging architecture.
Also applies to: 37-37
lib/l10n/intl_en.arb (1)
1196-1232: LGTM! Comprehensive localization for logs feature.The new translation keys for the logs screen, developer tools, and log filtering UI are well-structured with proper placeholder definitions and metadata. This provides complete i18n support for the new logging feature.
Also applies to: 1235-1257
lib/features/relays/relays_notifier.dart (1)
9-9: LGTM! Comprehensive logging migration.All logging calls have been correctly migrated from the local
_loggerinstance to the globalloggerservice. The log levels and messages remain unchanged, maintaining the same observability while aligning with the centralized logging architecture.Also applies to: 53-54, 77-77, 93-93, 102-102, 382-382, 426-427, 432-435, 444-444, 448-448, 462-462, 468-468, 473-474, 486-487, 490-490, 507-507, 511-511, 519-519, 530-531, 538-539, 546-546, 550-550, 573-574, 579-579, 594-594, 600-600, 607-607, 614-614, 627-627, 634-635, 646-646, 652-652, 656-656, 676-676, 683-683, 693-693, 700-700, 710-711, 786-786, 799-799, 809-809, 816-816, 841-841
lib/shared/widgets/pay_lightning_invoice_widget.dart (1)
3-3: LGTM! Clean widget logging migration.The widget has been successfully migrated to use the global logger service. All logging calls maintain their original behavior while removing the dependency on a widget-scoped logger field.
Also applies to: 77-78, 102-103, 108-109, 112-112
lib/services/lifecycle_manager.dart (1)
6-6: LGTM! Enhanced lifecycle logging with context.The logging migration includes helpful component-specific prefixes (Lifecycle, BackgroundService, MostroService, Repository, Chat, UI) that improve log readability and debugging. All calls correctly use the global logger service.
Also applies to: 50-50, 55-55, 64-64, 68-68, 73-73, 78-78, 81-81, 83-83, 97-97, 104-104, 109-109, 112-112, 115-115, 117-117, 123-123
lib/features/restore/restore_progress_notifier.dart (3)
3-3: LGTM! Logging migration complete.All logging calls have been correctly updated to use the global logger service.
Also applies to: 14-14, 24-24, 35-35, 54-54, 70-70, 87-87, 96-96
34-43: LGTM! Proper progress initialization.The method correctly initializes
currentProgressto 0 when orders are received, preparing the state for incremental progress updates via the newincrementProgress()method.
45-51: LGTM! Useful progress tracking API.The new
incrementProgress()method provides a clean way to update restore progress incrementally. It correctly resets the timeout timer to ensure the overlay remains visible during active progress.lib/l10n/intl_it.arb (1)
1229-1265: LGTM! Complete Italian localization for logs feature.The Italian translations for the logs screen, developer tools, and filtering UI are comprehensive and well-structured, maintaining consistency with the English localization.
Also applies to: 1268-1290
lib/features/order/notfiers/add_order_notifier.dart (1)
5-5: LGTM! Import addition for centralized logger.The import has been correctly added to support the existing logger usage in this notifier, aligning with the project-wide logging migration.
lib/features/settings/settings_notifier.dart (1)
3-3: LGTM! Logging migration complete.The transition from local Logger instance to the centralized logger service is clean and consistent throughout the file. All logging calls have been properly updated.
Also applies to: 12-12, 53-53, 62-62, 100-100, 111-111, 136-136, 144-144
lib/features/rate/rate_counterpart_screen.dart (1)
4-4: LGTM! Clean migration to centralized logger.The logging change is minimal and appropriate for this UI component.
Also applies to: 26-26
lib/features/chat/providers/chat_room_providers.dart (1)
2-2: LGTM! Logging refactor properly applied.The migration to the centralized logger service is consistent across all logging calls in the provider and helper function.
Also applies to: 55-55, 58-58, 62-72
lib/features/disputes/notifiers/dispute_chat_notifier.dart (1)
5-5: LGTM! Comprehensive logging migration.The dispute chat notifier has been successfully migrated to use the centralized logger service. All logging calls throughout the complex subscription, message handling, and error paths have been consistently updated.
Also applies to: 48-48, 59-59, 69-69, 76-76, 93-93, 102-115, 191-196, 206-206, 215-216, 256-259, 265-266, 279-279, 299-306, 323-329, 333-334, 343-365, 371-372, 417-418, 422-425, 456-457, 485-486, 522-523, 531-532, 541-542, 551-555
lib/features/order/models/order_state.dart (1)
1-1: LGTM! OrderState logging migration complete.The order state model has been consistently updated to use the centralized logger service across all state transitions, dispute handling, and admin action processing.
Also applies to: 91-91, 103-103, 109-109, 118-118, 127-127, 147-147, 154-211
lib/features/notifications/services/background_notification_service.dart (1)
8-8: Based on my investigation, I found that the concern raised in the review comment is theoretically valid. The web search confirms that:
- Background isolates in Dart/Flutter have separate heaps and cannot directly access main isolate singletons/services like a global logger
- Standard logger services are main-isolate singletons and would not be available in isolate context
- Proper patterns require either SendPort/ReceivePort messaging or BackgroundIsolateBinaryMessenger with RootIsolateToken initialization
However, I cannot fully verify this concern because:
- The repository failed to clone, preventing code inspection
- Without seeing the actual implementation, I cannot determine:
- Whether the notification functions actually use the logger
- Whether they run in the background isolate or call out via platform channels
- Whether the code already handles isolate context properly (e.g., using platform channels)
- Whether this is a real issue or already implemented correctly
lib/data/repositories/open_orders_repository.dart (1)
8-8: LGTM! Clean logging migration.The import and logging calls have been cleanly migrated to the centralized logger service with consistent "Repository: " prefixes. No logic changes detected.
Also applies to: 56-56, 60-60, 125-125, 135-135, 142-142
lib/features/chat/notifiers/chat_room_notifier.dart (1)
5-5: LGTM! Comprehensive logging migration.All logging calls have been successfully migrated to the centralized logger service. The verbose logging throughout chat room operations will be helpful for debugging.
Also applies to: 72-72, 77-78, 85-86, 99-99, 127-127, 140-140, 153-153, 155-155, 162-162, 165-165, 172-173, 176-178, 199-199, 208-211, 214-215, 222-223, 225-226, 232-232, 236-243, 246-247, 254-255, 265-277, 280-281, 288-293, 302-320, 323-333, 335-342, 356-373, 379-379
lib/services/mostro_service.dart (1)
15-15: LGTM! Well-organized logging with clear prefixes.The logging migration uses clear categorical prefixes (Subscription:, Event:, Message:, Order:, Session:) that will make log filtering easier. No logic changes detected.
Also applies to: 31-32, 40-40, 114-114, 128-129, 134-135, 151-151, 155-155, 179-179, 289-289, 306-306, 308-308, 345-346
lib/data/repositories/dispute_repository.dart (1)
3-3: LGTM! Consistent repository logging.The migration follows the same "Repository: " prefix pattern as other repository files, maintaining consistency across the data layer.
Also applies to: 22-22, 31-32, 38-38, 57-57, 60-60, 67-67, 83-83, 88-88, 91-91, 98-115
lib/features/notifications/utils/notification_data_extractor.dart (1)
2-2: LGTM! Straightforward migration.The two logging calls have been cleanly migrated from per-use
Logger()instances to the globalloggerinstance.Also applies to: 104-104, 106-106
lib/features/chat/notifiers/chat_rooms_notifier.dart (1)
4-4: LGTM! Clean migration.All logging calls successfully migrated to the centralized logger service with appropriate log levels (error, info, debug).
Also applies to: 26-26, 36-36, 55-55, 57-57, 88-88, 90-90, 97-98
lib/services/deep_link_service.dart (1)
8-8: LGTM! Well-categorized deep link logging.The migration uses clear prefixes (DeepLink:, Navigation:, DeepLinkService:) that will make troubleshooting deep link issues much easier. No logic changes detected.
Also applies to: 43-43, 47-47, 55-55, 57-57, 74-74, 100-100, 119-119, 150-150, 179-179, 198-199, 209-209, 212-212, 217-217
lib/shared/notifiers/session_notifier.dart (1)
10-10: LGTM! Session management logging migrated successfully.All logging calls have been cleanly migrated to the centralized logger service. The session lifecycle events are now consistently logged.
Also applies to: 206-206, 233-235, 248-251, 259-261, 270-275, 295-295
lib/features/order/notfiers/abstract_mostro_notifier.dart (2)
14-14: LGTM - Logger migration to centralized service.The import correctly switches to the centralized logger service, and all subsequent
logger.x()calls throughout the file are consistent with this change.
506-516: Correct usage of global logger in static methods.Using the global
loggerin static methods is appropriate here since static methods cannot access instance fields. The centralized logger service pattern handles this well.lib/core/deep_link_interceptor.dart (1)
3-3: LGTM - Clean logger migration.The centralized logger is correctly imported and used throughout the interceptor. Log levels are appropriate:
logger.ifor informational messages andlogger.wfor warnings.Also applies to: 17-17
lib/features/chat/chat_room_provider.dart (1)
2-2: LGTM - Proper logger migration with structured error logging.The migration correctly uses the centralized logger and properly passes
errorandstackTraceas named parameters for structured error logging.Also applies to: 43-53
lib/features/trades/widgets/status_filter_widget.dart (1)
3-3: LGTM - Simple logger migration.The import and single
logger.ecall are correctly migrated to use the centralized logger service.Also applies to: 173-173
lib/core/app_routes.dart (2)
27-27: LGTM - New imports for logs feature.The imports for
LogsScreenandlogger_serviceare correctly added to support the new logs functionality and centralized logging.Also applies to: 32-32
288-296: LGTM - New /logs route properly integrated.The
/logsroute follows the same pattern as other routes in the shell, usingbuildPageWithDefaultTransitionfor consistent navigation behavior. TheLogsScreenis correctly instantiated asconst.lib/core/config.dart (1)
45-49: LGTM - Logger configuration fields added.The configuration values are reasonable:
logMaxEntries = 1000: Sensible limit for in-memory storagelogBatchDeleteSize = 100: Efficient batch deletion to avoid frequent trimming operationsfullLogsInfo: Mutable to allow runtime toggling of log verbosityThe inline comment clearly documents the purpose of
fullLogsInfo.lib/data/repositories/mostro_storage.dart (1)
1-1: LGTM - Logger migration with improved contextual prefixes.The migration to the centralized logger is correct. The addition of "Storage: " prefixes to log messages improves log filtering and debugging clarity. Error logging properly uses named
errorandstackTraceparameters.Also applies to: 23-31
lib/features/logs/providers/logs_provider.dart (1)
5-12: I attempted to verify the review comment by examining howlogsProvideris consumed in the codebase, but the repository clone failed. Without access to the actual codebase, I cannot definitively verify:
- How LogsScreen consumes the provider – whether it implements manual refresh or relies on auto-updates
- MemoryLogOutput implementation – whether it supports streaming or reactive patterns
- Existing refresh mechanisms – whether manual invalidation is already in place elsewhere
- Intentional design – whether the snapshot behavior is deliberate or an oversight
The review's architectural concerns about provider reactivity are reasonable and worth considering, but the specific recommendations cannot be fully validated without examining the actual code usage patterns.
lib/features/restore/restore_manager.dart (1)
8-8: LGTM! Centralized logging migration.The migration from local logger to the centralized logger service is clean and consistent. All log messages include the "Restore: " prefix for easy filtering, and error handling properly includes stack traces.
Also applies to: 57-57, 62-62, 77-77, 85-85, 100-100, 103-103, 133-133, 138-138, 143-143, 165-165, 189-189, 197-197, 204-204, 225-225, 231-231, 253-253, 259-259, 282-282, 286-286, 292-292, 314-314, 319-319, 336-336, 344-344, 350-350, 354-354, 384-388, 467-467, 510-510, 516-516, 554-554, 580-580, 607-607, 610-610, 614-614, 618-618, 623-623, 647-647, 651-651, 656-656, 662-662, 677-677, 690-690, 718-718, 722-722
lib/features/settings/settings_screen.dart (1)
499-637: LGTM! Dev Tools card implementation.The new Dev Tools card follows the existing UI patterns and provides clear navigation to the logs screen. The warning styling with the amber border appropriately signals that these are advanced features. The log storage location display is helpful for users who need to locate exported logs.
lib/features/order/notfiers/order_notifier.dart (2)
7-7: LGTM! Centralized logging and improved sync state management.The migration to the centralized logger service is clean with consistent "Order: " prefixes. The addition of the
finallyblock at line 70 to reset_isSyncingis a good defensive coding practice that ensures the flag is always cleared even if an exception occurs.Also applies to: 26-26, 41-41, 61-66
167-182: LGTM! Automatic expiration handling.The automatic cancellation logic correctly detects expired orders and removes them from My Trades by deleting the session. The notification to the user is appropriate.
lib/background/mobile_background_service.dart (1)
5-5: LGTM! Centralized logging migration.The migration from the local logger instance to the centralized logger service is complete and correct. All logging calls have been properly updated.
Also applies to: 48-56, 71-71, 130-130, 140-155
lib/features/subscriptions/subscription_manager.dart (2)
59-72: LGTM! Critical bug fix for existing sessions.The
_initializeExistingSessionsmethod is an important addition that ensures subscriptions are created for sessions that exist when the SubscriptionManager is initialized. The comment correctly identifies this as fixing the "stuck orders bug" that would occur on app restart. This is essential sincefireImmediately: falseprevents automatic initialization.
5-5: LGTM! Centralized logging migration.The migration to the centralized logger service is clean and consistent throughout the subscription management logic.
Also applies to: 47-49, 63-70, 76-76, 94-94, 111-115, 167-167, 185-186, 273-276, 298-299, 323-323
lib/core/deep_link_handler.dart (1)
5-5: LGTM! Centralized logging migration.The migration to the centralized logger service is clean and complete. All logging calls have been properly updated.
Also applies to: 18-18, 29-29, 44-57, 85-85, 105-115
lib/l10n/intl_es.arb (1)
1174-1234: LGTM! Spanish localization for logs UI.The new Spanish translations for the logs screen and dev tools are comprehensive and well-structured. The translations cover all necessary UI elements including filters, export functionality, and developer tools warnings.
lib/services/nostr_service.dart (2)
118-146: LGTM! Good defensive validation.The addition of runtime validation to ensure relays are configured before publishing events is excellent defensive programming. The error messages are clear and provide actionable information. This prevents the obscure assertion error from dart_nostr and makes debugging easier.
11-11: LGTM! Comprehensive logging enhancement.The migration to the centralized logger service is clean, and the logging has been significantly improved with:
- Categorized prefixes ("Relay: ", "Event: ", "Order: ") for easy filtering
- Detailed context in each log message
- Consistent formatting across all operations
- Proper error logging with context
This makes debugging and monitoring much easier.
Also applies to: 34-71, 77-109, 129-139, 183-183, 272-318, 326-405, 421-451
lib/features/logs/screens/logs_screen.dart (3)
29-57: LGTM!Proper lifecycle management with
initState/disposefor controllers, and the scroll listener correctly checkshasClientsbefore accessing offset.
387-398: LGTM!Good use of
mountedcheck after async operation before accessingcontextfor the SnackBar.
516-535: LGTM!Log file creation is well-structured with proper formatting and temp directory usage.
lib/features/trades/providers/trades_provider.dart (3)
4-4: LGTM!Import correctly updated to use the centralized logger service.
22-23: LGTM!The
math.minguard prevents substring overflow. Logging at debug level is appropriate for filtering state.
40-51: LGTM!Watching multiple
OrderNotifierproviders in a loop is a valid Riverpod pattern for reactive updates. The try-catch properly handles cases where anOrderStatecannot be obtained.test/mocks.mocks.dart (1)
1959-1993: Auto-generated mock file correctly updated.This file is generated by Mockito and correctly reflects the new
customLogStorageDirectoryparameter added toSettings.copyWith. No manual changes needed.lib/features/settings/settings.dart (3)
10-10: LGTM!Clear documentation comment explaining the field's purpose.
60-60: LGTM!Serialization correctly includes the new field.
74-74: LGTM!Deserialization correctly handles the nullable field without explicit null checks (Dart's dynamic access returns null for missing keys).
lib/services/logger_service.dart (5)
4-24: LGTM!Clean
LogEntrydata class with a well-formattedformat()method that produces consistent log output.
27-61: LGTM!Singleton pattern is appropriate for a global log buffer. The batch deletion strategy (removing
logBatchDeleteSizeentries when exceedinglogMaxEntries) prevents frequent list mutations.
156-190: LGTM!Robust stack trace parsing that correctly skips logger-internal frames and handles multiple frame formats. The
lastValidtracking ensures a fallback is available.
198-225: Logger configuration is cached; changes toConfig.fullLogsInforequire app restart.The logger is lazily cached based on
Config.fullLogsInfoat first access. If this config value can change at runtime (e.g., via settings), the cached logger won't reflect the change.Verify whether
Config.fullLogsInfois intended to be a compile-time constant or if it can change at runtime. If runtime changes are expected, consider invalidating the cache:+void resetLoggerCache() { + _cachedSimpleLogger = null; + _cachedFullLogger = null; +} Logger get logger { if (Config.fullLogsInfo) {
100-121: LGTM!Clean separation of concerns with
_ConsoleOnlyOutputfor simple printing and_MultiOutputfor routing to both memory and console outputs.
lib/l10n/intl_es.arb
Outdated
| "clear": "Limpiar", | ||
| "cancel": "Cancelar", |
There was a problem hiding this comment.
Fix duplicate key in ARB file.
Static analysis correctly identifies duplicate keys in this ARB file. The key "cancel" is defined twice:
- First at line 188:
"cancel": "Cancelar" - Again at line 1212:
"cancel": "Cancelar"
In JSON/ARB files, duplicate keys are invalid, and the second definition will override the first. Since the translation is identical and the key already exists at line 188, the duplicate at line 1212 should be removed.
Apply this diff to remove the duplicate:
"clearLogsConfirmMessage": "Esta acción no se puede deshacer. Todos los registros se eliminarán permanentemente.",
"clear": "Limpiar",
- "cancel": "Cancelar",📝 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.
| "clear": "Limpiar", | |
| "cancel": "Cancelar", | |
| "clear": "Limpiar", |
🧰 Tools
🪛 RuboCop (1.81.7)
[warning] 1211-1211: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
[warning] 1212-1212: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
🤖 Prompt for AI Agents
In lib/l10n/intl_es.arb around lines 1211-1212, there's a duplicate key "cancel"
that repeats the entry already defined at line 188; remove the redundant
`"cancel": "Cancelar",` at lines 1211-1212 and ensure the surrounding JSON/ARB
syntax remains valid (no dangling commas) so the file continues to parse
correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/shared/widgets/pay_lightning_invoice_widget.dart (2)
75-85: Consider not logging the full LN invoice on copy
logger.i('Copied LN Invoice to clipboard: ${widget.lnInvoice}');will store the full invoice string in the in‑app logs and any exported log files. For a user‑facing exportable log, that may be more detail than necessary.You could log only a short prefix or just the fact that an invoice was copied, e.g.:
-logger.i('Copied LN Invoice to clipboard: ${widget.lnInvoice}'); +final invoicePreview = widget.lnInvoice.length > 16 + ? '${widget.lnInvoice.substring(0, 16)}...' + : widget.lnInvoice; +logger.i('Copied LN invoice to clipboard (preview: $invoicePreview)');
96-123: Tight logging integration is good; consider truncation / more contextual dataThe three logger calls here (
Launched Lightning wallet...,Shared LN Invoice..., and theFailed to share LN Invoice: $eerror) are correctly using the global logger and give useful diagnostics.Two optional tweaks to consider:
Avoid full invoice in logs (same rationale as the copy handler):
- Either omit the invoice value or log only a short preview to reduce noise and potential privacy surface in exported logs.
Add order context instead of full invoice:
- Since you already have
widget.orderId, logging it instead (or in addition) would make log scanning easier without relying on invoice strings.Example:
-logger.i('Launched Lightning wallet with invoice: ${widget.lnInvoice}'); +logger.i('Launched Lightning wallet for order ${widget.orderId}'); -logger.i('Shared LN Invoice via share sheet: ${widget.lnInvoice}'); +logger.i('Shared LN invoice via share sheet for order ${widget.orderId}');The error log
logger.e('Failed to share LN Invoice: $e');is fine as‑is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/shared/widgets/pay_lightning_invoice_widget.dart(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
lib/shared/widgets/pay_lightning_invoice_widget.dart
📚 Learning: 2025-08-15T01:37:12.243Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.
Applied to files:
lib/shared/widgets/pay_lightning_invoice_widget.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 (2)
lib/shared/widgets/pay_lightning_invoice_widget.dart (2)
1-9: Switch to centralized logger import looks correctUsing
package:mostro_mobile/services/logger_service.darthere aligns with the new global logging architecture and removes the per‑widget logger dependency cleanly.
19-28:constconstructor is appropriate and non‑breakingAll fields are
final, so marking the constructorconstis valid and gives minor rebuild/perf benefits without impacting callers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/features/logs/screens/logs_screen.dart (1)
537-570: Add null check before using directory to prevent potential crash.The fallback chain could theoretically still result in a null
directoryif all methods fail (e.g., on an unsupported platform or edge case). This was flagged in a previous review but only partially addressed.Apply this diff to add a null guard:
} else { // For other platforms (Windows, Linux, macOS) directory = await getApplicationDocumentsDirectory(); } + if (directory == null) { + throw Exception('Could not determine storage directory'); + } - final logsDir = Directory('${directory.path}/MostroLogs'); + final logsDir = Directory('${directory!.path}/MostroLogs'); await logsDir.create(recursive: true);Alternatively, wrap the operation in try-catch to provide a user-friendly error message.
🧹 Nitpick comments (1)
lib/features/logs/screens/logs_screen.dart (1)
75-78: Consider using Riverpod provider for reactive log updates.The screen fetches logs directly from
MemoryLogOutput.instance.getAllLogs()rather than watching a provider. This means the UI won't automatically update when new logs are added while the screen is open. Users would need to navigate away and back, or trigger a filter change to see new entries.If real-time log updates are desired, consider watching the
logsProvidermentioned in the PR architecture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/background/background.dart(2 hunks)lib/background/desktop_background_service.dart(2 hunks)lib/features/logs/screens/logs_screen.dart(1 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_es.arb(1 hunks)lib/l10n/intl_it.arb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/background/desktop_background_service.dart
- lib/l10n/intl_it.arb
- lib/background/background.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 (5)
lib/l10n/intl_en.arb (1)
1196-1254: LGTM! Localization entries for logs feature are well-structured.The new translation keys for the logs screen, dev tools, and log filtering are properly organized with correct placeholder definitions. The duplicate key issue from the previous review has been addressed.
lib/features/logs/screens/logs_screen.dart (3)
288-297: LGTM! Reverse ordering correctly shows newest logs first.The implementation correctly reverses the display order so the most recent logs appear at the top, matching the PR requirements.
607-612: LGTM!The clear confirmation handler correctly clears logs, dismisses the dialog, and triggers a UI refresh.
472-535: LGTM! Export functions are well-implemented.Good practices observed:
- Proper
mountedchecks before UI updates after async operations- Meaningful file naming with timestamps
- Log file includes useful header metadata (generation time, entry count)
- Error handling with user-friendly messages
lib/l10n/intl_es.arb (1)
1174-1232: LGTM! Spanish translations are accurate and well-structured.The new localization entries for the logs feature, dev tools, and filtering are properly translated. The duplicate key issue from the previous review has been resolved. Placeholder definitions match the English source file.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
@BraCR10 me funciona bastante bien, creo que sería bueno que sí mostrara los valores hexadecimales, como el id de los eventos o los datos de la instancia de mostro, porque pudieran ser útiles en caso de que hubiera que buscar algún evento, o saber a cual instancia estaba conectado
En cuanto a UI, no me gusta mucho que esté resaltado en amarillo, revisa este comentario de @grunch a otro dev que estuvo trabajando antes en esta feature #342 (comment)
Quizás ¨Predeterminado (Descargas)¨ se pudiera poner dentro de la pantalla que muestra los logs para no ocupar tanto espacio afuera, pero veo que no se puede modificar dónde se descargan los logs, entonces no debería estar con la UI que usan los botones si no lo es, si se va a dejar por ahora así mejor que se note que no puede cambiar, la otra opción es que se pueda modificar y entonces como botón sí se vería bien
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/features/logs/screens/logs_screen.dart (1)
572-605: Add null safety check before using directory.While the code addresses the previous hardcoded path issue, there's still a potential null pointer exception at line 597 if all directory resolution methods return null (though unlikely). Add a safety check to handle this edge case gracefully.
Apply this diff to add defensive null checking:
} else { // For other platforms (Windows, Linux, macOS) directory = await getApplicationDocumentsDirectory(); } + if (directory == null) { + throw Exception(S.of(context)!.couldNotDetermineStorageDirectory); + } final logsDir = Directory('${directory.path}/MostroLogs');Note: You'll need to add the
couldNotDetermineStorageDirectorylocalization key to your ARB files.
🧹 Nitpick comments (2)
lib/services/logger_service.dart (2)
74-91: Good sanitization, but consider additional hex value redaction.The message cleaning comprehensively removes formatting artifacts and redacts sensitive data like private keys and mnemonics.
Based on the PR objectives mentioning "sanitizes sensitive data (private keys, mnemonics, hex values)", consider adding explicit hex value redaction if long hex strings commonly appear in logs:
.replaceAll(RegExp(r'"privateKey"\s*:\s*"[^"]*"'), '"privateKey":"[REDACTED]"') .replaceAll(RegExp(r'"mnemonic"\s*:\s*"[^"]*"'), '"mnemonic":"[REDACTED]"') + .replaceAll(RegExp(r'\b[0-9a-fA-F]{32,}\b'), '[HEX_VALUE]') .replaceAll(RegExp(r'[^A-Za-z0-9\s.:,!?\-_/\[\]]'), ' ')This would redact hex strings of 32+ characters that might contain sensitive data.
154-189: Consider returning first valid frame instead of last.The current implementation returns the
lastValidframe found, which walks through the entire stack trace. For typical log calls, you probably want the first application frame (the actual call site) rather than the last one before returning to the caller.Consider changing to return early on first valid match:
for (final line in lines) { if (line.contains('logger_service.dart') || line.contains('logger.dart') || line.contains(' (dart:') || line.contains('<asynchronous suspension>') || line.trim().isEmpty) { continue; } var match = RegExp(r'#\d+\s+\S+\s+\((?:package:[\w_]+/)?(?:.*/)(\w+)\.dart:(\d+)').firstMatch(line); if (match != null) { - lastValid = { + return { 'service': match.group(1) ?? 'App', 'line': match.group(2) ?? '0' }; - continue; } match = RegExp(r'package:[\w_]+/(?:.*/)(\w+)\.dart:(\d+)').firstMatch(line); if (match != null) { - lastValid = { + return { 'service': match.group(1) ?? 'App', 'line': match.group(2) ?? '0' }; } } - return lastValid ?? {'service': 'App', 'line': '0'}; + return {'service': 'App', 'line': '0'};This would report the actual call site rather than a frame deeper in the stack.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/logs/screens/logs_screen.dart(1 hunks)lib/features/settings/settings_screen.dart(2 hunks)lib/services/logger_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/features/logs/screens/logs_screen.dartlib/services/logger_service.dartlib/features/settings/settings_screen.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/features/logs/screens/logs_screen.dartlib/services/logger_service.dartlib/features/settings/settings_screen.dart
lib/features/**/screens/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/features/**/screens/**/*.dart: Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
UseS.of(context)!.yourKeyfor all user-facing strings instead of hardcoded text
Files:
lib/features/logs/screens/logs_screen.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/logger_service.dart
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: PR descriptions should capture intent, list key changes, link tracking issues, flag risk areas, include command output for manual tests, and provide screenshots for UI updates
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/features/logs/screens/logs_screen.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Use `S.of(context)!.yourKey` for all user-facing strings instead of hardcoded text
Applied to files:
lib/features/logs/screens/logs_screen.dart
🔇 Additional comments (15)
lib/features/settings/settings_screen.dart (2)
103-105: LGTM! Dev Tools card integration follows existing pattern.The card is properly positioned in the layout and follows the same spacing pattern as other setting cards.
499-595: LGTM! Well-structured Dev Tools card.The implementation follows the established pattern of other cards in the settings screen, uses proper localization, and provides clear navigation to the logs feature. The styling is consistent with the app theme.
lib/features/logs/screens/logs_screen.dart (9)
22-58: LGTM! Proper state management and resource cleanup.The state initialization, scroll handling, and disposal follow Flutter best practices. The scroll-to-top button appears after 200px of scrolling, providing good UX.
422-433: LGTM! Proper mounted check and good UX.The clipboard copy functionality correctly checks
mountedbefore showing the SnackBar, following Flutter best practices for async operations in stateful widgets.
507-527: LGTM! Robust sharing implementation.The share functionality properly manages state, checks
mountedbefore UI updates, and handles errors gracefully with user feedback.
529-549: LGTM! Proper state and error handling.The save-to-device functionality correctly manages state and uses
mountedchecks before showing success/error messages.
551-570: LGTM! Clean log file generation.The log file creation is well-structured with a clear header, proper formatting, and efficient use of StringBuffer for building the content.
607-660: LGTM! Proper confirmation dialog implementation.The clear logs confirmation follows Flutter dialog patterns correctly, uses proper localization, and appropriately updates state after clearing.
82-141: LGTM! Well-structured main build method.The build method properly retrieves and filters logs, provides conditional UI based on state, and includes a floating action button for scrolling to top. The delete action is appropriately disabled when there are no logs.
143-505: LGTM! Comprehensive and well-organized UI implementation.The UI builder methods are well-structured with proper separation of concerns. Notable good practices:
- Reversed log ordering (newest first) for better UX
- Consistent use of localization throughout
- Proper styling aligned with AppTheme
- Clean helper methods for colors, icons, and formatting
76-79: Verify that the Settings model includes thecustomLogStorageDirectoryfield.The code references
settings.customLogStorageDirectory, but I cannot access the repository to verify this field exists in the Settings model definition. Confirm the Settings model includes this field to prevent runtime errors when the field is accessed.lib/services/logger_service.dart (4)
1-24: LGTM! Clean LogEntry data structure.The LogEntry class provides a clear structure for log data with a helpful format() method for exporting logs.
26-72: LGTM! Solid buffer management implementation.The singleton pattern is appropriate for the global log buffer, and the circular buffer implementation with batch deletion provides good performance. The regex extraction properly parses the formatted log lines.
93-119: LGTM! Clean API and output routing.The public API methods are well-designed, returning unmodifiable lists for safety. The multi-output pattern effectively routes logs to both memory and console.
191-223: LGTM! Well-designed logger configuration.The logger getter appropriately switches between detailed and simple logging modes based on configuration, with caching to avoid repeated initialization. Both modes properly route logs to memory and console via the multi-output pattern.
|
Hi @Catrya, thank you for your feedback. The directory info was moved inside the logs screen and its style was updated. The card style in Settings was modified and also moved according to 
Note: In the comment #342 (comment), another approach is described. It suggests using a switch that the user can toggle to start recording logs.However, I had some doubts about this. Once the user turns it on, logs would have to be written directly to the file in real time, which could potentially introduce performance issues, couldn't it? In the current approach, I’m doing something different:
Please let me know if I should change this approach. |
There was a problem hiding this comment.
Me funciona bien. Me preocupa un poco que hay 45 archivos modificados.
La sugerencia del toggle era más simple: solo para activar/desactivar el buffer en memoria (no escribir a archivo en tiempo real). Toggle ON = funciona como ahora con buffer de 1000 entradas, Toggle OFF = logs solo van a console para ahorrar RAM. El export seguiría funcionando igual cuando esté activado.
Tengo una duda: el logger del background isolate (background.dart:19) no especifica output, mientras que el global (logger_service.dart:210) usa MemoryLogOutput.instance. ¿Los logs del background aparecen en la UI de logs o solo en consola? no he podido testear con errores de background porque son bastante random, pero no se si un error como el de Exception: Nostr is not initialized. Call init() first. (#328 ) este salga en la ui y es importante
|
Hola @Catrya
Al principio se usó el mismo patrón para todos los background que consiste en utilizar mostro_mobile/services/logger_service.dart en vez de logger/logger.dart pero coderabbit #370 (comment) #370 (comment) señaló algunos puntos a tomar en en cuenta ya que algunos procesos background corren de manera aislada por lo que no comparten memoria con la app, por lo que me temo que debido a eso no se puede mostrar los logs desde esos backgrounds en la UI, quedo atento a cualquier sugerencia.
es debido al approach actual en el que se creó la clase mobile/lib/services/logger_service.dart, la cual proporciona un logger centralizado con persistencia en memoria a través de MemoryLogOutput. Cada archivo que anteriormente creaba su propia instancia de Logger() fue modificado para usar el global logger, lo que permite capturar automáticamente todos los logs en un buffer de memoria. Crees que hay un approach mas correcto para evitar modificar cada archivo?
|
La idea de ver los logs en la app es para poder debugguear cuando haya errores, pero si no vemos logs que son importantes entonces vamos estar igual que ahora sin saber qué pasó. |
|
Quizás en este PR se pudiera completar la funcionalidad que necesitamos de que todos los logs aparezcan en la UI (incluyendo los de background), y luego en otro PR implementar las mejoras de UX como el toggle para activar/desactivar la captura de logs y la selección de ruta para guardar el archivo, para que no quede tan grande este PR. |
|
Saludos @Catrya estoy de acuerdo con #370 (comment) Voy a investigar para solucionar el problema de los missing logs #370 (comment) |
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 (8)
lib/background/background.dart(2 hunks)lib/features/chat/notifiers/chat_room_notifier.dart(12 hunks)lib/features/notifications/services/background_notification_service.dart(6 hunks)lib/features/notifications/utils/notification_data_extractor.dart(2 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_es.arb(1 hunks)lib/l10n/intl_it.arb(1 hunks)test/mocks.mocks.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/background/background.dart
- lib/features/notifications/utils/notification_data_extractor.dart
- test/mocks.mocks.dart
- lib/features/notifications/services/background_notification_service.dart
- lib/features/chat/notifiers/chat_room_notifier.dart
🧰 Additional context used
📓 Path-based instructions (1)
lib/l10n/**/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
Add new localization keys to all three ARB files (en, es, it) - use
S.of(context)!.keyNamefor all user-facing strings
Files:
lib/l10n/intl_it.arblib/l10n/intl_es.arblib/l10n/intl_en.arb
🧠 Learnings (3)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/l10n/**/*.arb : Add new localization keys to all three ARB files (en, es, it) - use `S.of(context)!.keyName` for all user-facing strings
Applied to files:
lib/l10n/intl_it.arblib/l10n/intl_es.arblib/l10n/intl_en.arb
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/generated/l10n*.dart : Never manually edit generated localization files - regenerate with `dart run build_runner build -d` after modifying ARB files
Applied to files:
lib/l10n/intl_it.arblib/l10n/intl_en.arb
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Localize all user-facing strings via ARB files and access them with `S.of(context)` rather than hard-coded literals
Applied to files:
lib/l10n/intl_en.arb
🪛 GitHub Actions: Flutter
lib/l10n/intl_en.arb
[error] 1258-1258: FormatException: Unexpected character (at line 1258, character 3) in ARB file. The arb file has a formatting issue at this line.
🔇 Additional comments (1)
lib/l10n/intl_it.arb (1)
1232-1290: Verify new log-related keys across locales and placeholder types.The new keys for logs/dev tools/log filters (
logsReport,viewAndExportLogs,totalLogs,maxEntries,logsSavedTo, etc.) look consistent here, but to avoid runtime or generation issues:
- Ensure the same key set exists in the English and Spanish ARB files.
- Keep placeholder names and types (
count/maxasint,pathasString) identical across all three locales.Based on learnings, this keeps
S.of(context)!.keyNameusage consistent and prevents missing or mismatched translations.
|
@Catrya consulta, los logs de background deberia aparecer incluso cuando la app esta terminated o solo en segundo plano? |
Me parece que solo en segundo plano, no cuando está terminada |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/shared/utils/logger_utils.dart (1)
4-20: Minor robustness tweaks for sanitizing and level parsingThe current implementation works, but a couple of small adjustments would make it more robust:
extractFromFormattedLineonly matches fixed levels (ERROR|WARN|INFO|DEBUG|TRACE). IfSimplePrinter._formatLevelever introduces additional labels (e.g.LOG), service/line will silently fall back toApp/0. Consider loosening the pattern to accept any uppercase word in that position.- The
nsecredaction is case-sensitive and only matchesnsec[0-9a-z]+. If there is any chance of mixed/capitalized forms, switching to a case-insensitive regex would make redaction harder to bypass.These are minor, but cheap hardening wins for log safety.
Also applies to: 24-32
lib/main.dart (1)
30-36: Consider initializing persistent logger earlier in main()Right now
logsDatabase/logStorage/initializeLogger(logStorage)run afterrequestNotificationPermissionIfNeeded()and other setup. Any logs emitted before this point will only hit the in-memory fallback and won’t appear in the persistent log store or Logs screen.If early startup diagnostics are important, consider moving the
logs.dbopen +LogStoragecreation +initializeLoggercloser toWidgetsFlutterBinding.ensureInitialized()so all subsequent initialization stages are persisted.lib/services/logger_service.dart (1)
49-62: Avoid relying on microsecond timestamp alone as unique log ID
PersistentLogOutputusesevent.origin.time.microsecondsSinceEpoch.toString()as the record ID:final id = event.origin.time.microsecondsSinceEpoch.toString(); _logStorage.putItem(id, logEntry);If multiple log events occur within the same microsecond (very possible in tight loops or concurrent code), later writes will overwrite earlier entries with the same ID, silently dropping logs.
Consider making the key more collision-resistant, for example:
- Append some extra entropy (e.g. hashCode of the message/stack, or an incrementing counter), or
- Let the storage layer assign keys (e.g. via an
addItemAPI ifBaseStoragesupports it), and store the timestamp only as a field.This keeps log persistence reliable even under bursty logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/background/background.dart(4 hunks)lib/background/desktop_background_service.dart(3 hunks)lib/data/repositories/log_storage.dart(1 hunks)lib/features/logs/screens/logs_screen.dart(1 hunks)lib/main.dart(3 hunks)lib/services/logger_service.dart(1 hunks)lib/shared/providers/log_storage_provider.dart(1 hunks)lib/shared/providers/mostro_database_provider.dart(1 hunks)lib/shared/utils/logger_utils.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/background/background.dart
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/shared/providers/mostro_database_provider.dartlib/shared/providers/log_storage_provider.dartlib/shared/utils/logger_utils.dartlib/features/logs/screens/logs_screen.dartlib/data/repositories/log_storage.dartlib/background/desktop_background_service.dartlib/main.dartlib/services/logger_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/shared/providers/mostro_database_provider.dartlib/shared/providers/log_storage_provider.dartlib/shared/utils/logger_utils.dartlib/features/logs/screens/logs_screen.dartlib/data/repositories/log_storage.dartlib/background/desktop_background_service.dartlib/main.dartlib/services/logger_service.dart
lib/shared/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Files:
lib/shared/providers/mostro_database_provider.dartlib/shared/providers/log_storage_provider.dartlib/shared/utils/logger_utils.dart
lib/features/**/screens/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/features/**/screens/**/*.dart: Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
UseS.of(context)!.yourKeyfor all user-facing strings instead of hardcoded text
Files:
lib/features/logs/screens/logs_screen.dart
lib/data/repositories/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Implement Repository pattern for all data access - all data operations must go through repository classes
Files:
lib/data/repositories/log_storage.dart
lib/main.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Configure timeago package locales in app initialization for proper relative time formatting (e.g., 'hace X horas' vs 'hours ago')
Files:
lib/main.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/logger_service.dart
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: PR descriptions should capture intent, list key changes, link tracking issues, flag risk areas, include command output for manual tests, and provide screenshots for UI updates
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
lib/shared/providers/mostro_database_provider.dartlib/shared/providers/log_storage_provider.dartlib/data/repositories/log_storage.dartlib/main.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, 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/mostro_database_provider.dartlib/shared/providers/log_storage_provider.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
lib/shared/providers/log_storage_provider.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Name Riverpod providers using the `<Feature>Provider` or `<Feature>Notifier` convention
Applied to files:
lib/shared/providers/log_storage_provider.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/shared/providers/log_storage_provider.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
lib/shared/utils/logger_utils.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/features/logs/screens/logs_screen.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Use `S.of(context)!.yourKey` for all user-facing strings instead of hardcoded text
Applied to files:
lib/features/logs/screens/logs_screen.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/data/repositories/**/*.dart : Implement Repository pattern for all data access - all data operations must go through repository classes
Applied to files:
lib/data/repositories/log_storage.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Use dual storage strategy: store Mostro/default relays in `settings.relays` and user relays in `settings.userRelays` with full JSON metadata via `toJson()`/`fromJson()`
Applied to files:
lib/data/repositories/log_storage.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Persistence, APIs, and background jobs should live in `lib/data/` and `lib/background/`; generated localization output must be in `lib/generated/` and must stay untouched
Applied to files:
lib/background/desktop_background_service.dartlib/main.dartlib/services/logger_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/**/*.dart : Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Applied to files:
lib/background/desktop_background_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
lib/background/desktop_background_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/main.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/shared/providers/mostro_database_provider.dart (1)
25-27: logsDatabaseProvider mirrors existing DB providers correctlyThe new
logsDatabaseProviderfollows the same pattern asmostroDatabaseProviderandeventDatabaseProvider, and is correctly overridden with a realDatabaseinstance inmain.dart. As long as every runtime entry point provides that override, this is safe.lib/shared/providers/log_storage_provider.dart (1)
5-8: logStorageProvider composition is soundComposing
LogStoragevialogsDatabaseProviderkeeps storage wiring centralized and respects the repository pattern. This looks good as a DI entry point.lib/features/logs/screens/logs_screen.dart (1)
81-145: LogsScreen behavior and export flow look solidThe Logs screen:
- Correctly consumes
logger_service.watchLogs()and applies level + text filters.- Uses localized strings via
S.of(context)for all user-facing UI.- Handles async UI side effects (SnackBars, dialogs) with
mountedchecks.- Implements export/save via a temp file and platform-appropriate directories without hardcoded Android paths, and safely handles null directory fallbacks.
Overall this aligns well with the stated UX and logging requirements.
Also applies to: 440-610
lib/data/repositories/log_storage.dart (1)
7-51: LogStorage repository implementation is consistent and rotation-awareThe Sembast mapping and repository API look correct:
toDbMap/fromDbMapcorrectly persistLogEntryincluding ISO8601 timestamps and level strings.getAllLogsandwatchLogsreturn newest-first order, matching the Logs UI expectation.rotateIfNeededdeletes batches of oldest records when exceedingmaxEntries, which aligns with the configured retention strategy.clearAllcleanly wipes the store.No changes needed here beyond the broader DB concurrency concern already noted for
logs.db.Also applies to: 53-98
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
lib/services/logger_service.dart (5)
21-25: Consider usingDateFormatfor robust timestamp formatting.The
substring(0, 19)approach assumes a specific DateTime string format. While this works for typical DateTime instances, usingDateFormatfromintlpackage provides more explicit control and clarity.+import 'package:intl/intl.dart'; + +// Inside LogEntry class +static final _dateFormat = DateFormat('yyyy-MM-dd HH:mm:ss'); + String format() { - final time = timestamp.toString().substring(0, 19); + final time = _dateFormat.format(timestamp); final levelStr = level.toString().split('.').last.toUpperCase(); return '[$levelStr]($service:$line) $time - $message'; }
60-66: Silent error handling may mask persistent storage failures.While avoiding recursive logging is valid, completely swallowing errors makes debugging storage issues difficult. Consider using
debugPrintfor critical write failures—it won't trigger the logger recursively.+import 'package:flutter/foundation.dart'; + _logStorage.putItem(id, logEntry).then((_) { _logStorage.rotateIfNeeded(Config.logMaxEntries, Config.logBatchDeleteSize); }).catchError((error) { - // Silent fail to avoid recursive logging errors + // Use debugPrint to avoid recursive logging while preserving error visibility + debugPrint('LogStorage write failed: $error'); });
102-104: Add defensive bounds check for batch deletion.If
Config.logBatchDeleteSizeexceeds_buffer.length(e.g., due to misconfiguration),removeRangewill throw. A defensive check ensures robustness.if (_buffer.length > Config.logMaxEntries) { - _buffer.removeRange(0, Config.logBatchDeleteSize); + final deleteCount = Config.logBatchDeleteSize.clamp(0, _buffer.length); + _buffer.removeRange(0, deleteCount); }
252-256: Inefficient:PersistentLogOutputcreated on everyloggeraccess.A new
PersistentLogOutputis instantiated each timeloggeris accessed, but it's only used when the cached logger is null. This creates unnecessary object allocations on every log call.Consider caching the output alongside the logger, or computing it only when needed:
Logger? _cachedSimpleLogger; Logger? _cachedFullLogger; +LogOutput? _cachedPrimaryOutput; Logger get logger { - final LogOutput primaryOutput = _currentContext != null && _logStorages.containsKey(_currentContext) - ? PersistentLogOutput(_logStorages[_currentContext]!) - : MemoryLogOutput.instance; + LogOutput primaryOutput() { + _cachedPrimaryOutput ??= _currentContext != null && _logStorages.containsKey(_currentContext) + ? PersistentLogOutput(_logStorages[_currentContext]!) + : MemoryLogOutput.instance; + return _cachedPrimaryOutput!; + } if (Config.fullLogsInfo) { _cachedFullLogger ??= Logger( ... - output: _MultiOutput(primaryOutput, ConsoleOutput()), + output: _MultiOutput(primaryOutput(), ConsoleOutput()), );Also reset
_cachedPrimaryOutput = nullininitializeLogger.
312-316: Polling every 500ms is inefficient for real-time log watching.This fetches and yields all logs every 500ms regardless of changes. For a debugging UI this may be acceptable, but consider:
- Adding a debounce or change detection to avoid redundant yields
- Using Sembast's built-in watch capabilities if available
If Sembast supports watching, leverage
LogStorage.watch()streams. Otherwise, at minimum, compare log counts before yielding:Stream<List<LogEntry>> watchLogs() async* { ... + int lastCount = 0; await for (final _ in Stream.periodic(const Duration(milliseconds: 500))) { + final currentCount = await getLogCount(); + if (currentCount == lastCount) continue; + lastCount = currentCount; final allLogs = await getAllLogs(); yield allLogs; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/background/background.dart(4 hunks)lib/background/desktop_background_service.dart(3 hunks)lib/main.dart(3 hunks)lib/services/logger_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/background/background.dart
- lib/main.dart
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/background/desktop_background_service.dartlib/services/logger_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/background/desktop_background_service.dartlib/services/logger_service.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/logger_service.dart
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Persistence, APIs, and background jobs should live in `lib/data/` and `lib/background/`; generated localization output must be in `lib/generated/` and must stay untouched
Applied to files:
lib/background/desktop_background_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/**/*.dart : Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Applied to files:
lib/background/desktop_background_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
lib/background/desktop_background_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 (5)
lib/services/logger_service.dart (2)
136-204: LGTM!The
SimplePrinterimplementation correctly extracts service name and line number from stack traces, properly filtering out logger-internal frames and Dart internals.
283-343: LGTM!The cross-storage log operations are well-structured with proper fallback to memory buffer when persistent storage isn't initialized. The sequential iteration across contexts is appropriate given the small fixed number of contexts.
lib/background/desktop_background_service.dart (3)
32-36: Concurrent database access issue resolved.Using
getLogsDatabaseName(LogContext.desktop)returns'logs_desktop.db', ensuring this isolate uses a separate database file from the main app ('logs_main.db') and mobile background ('logs_mobile.db'). This addresses the previous review concern about Sembast not supporting concurrent access from multiple isolates to the same file.
84-86: LGTM!The global
loggerusage is now correct sinceinitializeLoggeris called earlier in the isolate (lines 32-35), ensuring the logger is properly configured with the desktop context storage before use.
81-83: Close nostrService relay connections on isolate stop.The stop command only closes the database but leaves active
nostrServicesubscriptions and relay connections open. The subscriptions created at line 71 (nostrService.subscribeToEvents(request)) are never canceled, andnostrServicehas no cleanup method to properly close relay connections. Before the isolate terminates, explicitly close all active subscriptions and call a cleanup method onnostrService(or directly onNostr.instance.services.relays) to ensure clean disconnection from relays and prevent hanging connections.
BraCR10
force-pushed
the
feat/br/logs
branch
from
37b536b to
4ffc008
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/main.dart (1)
30-60: Guard logs database initialization with error handling to prevent startup crash.If
openLogsDatabase()fails (disk error, permissions, etc.), the app will crash. Wrap the initialization in try/catch and skip persistent log storage if the database fails to initialize—the app will continue with memory-only logging instead.Also ensure to run
flutter analyzeandflutter testafter these changes per project standards.- final logsDatabase = await openLogsDatabase(); + Database? logsDatabase; + try { + logsDatabase = await openLogsDatabase(); + } catch (e) { + debugPrint('Failed to open logs database: $e'); + } @@ - logsDatabaseProvider.overrideWithValue(logsDatabase), + if (logsDatabase != null) logsDatabaseProvider.overrideWithValue(logsDatabase), @@ - final logStorage = container.read(logStorageProvider); - MemoryLogOutput.instance.setStorage(logStorage); + if (logsDatabase != null) { + final logStorage = container.read(logStorageProvider); + MemoryLogOutput.instance.setStorage(logStorage); + }
🧹 Nitpick comments (4)
lib/shared/providers/logs_database_provider.dart (1)
4-6: Make the “must override” error actionable.
UnimplementedError()is a bit opaque when it fires in prod; consider aStateErrorwith a clear hint about overriding inmain()/ background entrypoints.lib/features/logs/screens/logs_screen.dart (2)
604-623: Exported report header strings are user-facing; consider localizing them too.
The file content includes hard-coded English lines (“Mostro Mobile - Logs Report”, “Generated: …”).
32-35: Consider moving_logsStreamProviderto a feature-level provider file.
Declaring providers inside widgets is harder to reuse/test and diverges from your Riverpod conventions. Based on learnings.lib/data/repositories/log_storage.dart (1)
52-55:getCount()should use a DB count instead of loading all rows.
Even with 1000 rows this is avoidable churn; sembast supportsstore.count().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/data/repositories/log_storage.dart(1 hunks)lib/features/logs/screens/logs_screen.dart(1 hunks)lib/main.dart(3 hunks)lib/services/logger_service.dart(1 hunks)lib/shared/providers/log_storage_provider.dart(1 hunks)lib/shared/providers/logs_database_provider.dart(1 hunks)lib/shared/providers/mostro_database_provider.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/shared/providers/log_storage_provider.dart
- lib/shared/providers/mostro_database_provider.dart
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/shared/providers/logs_database_provider.dartlib/main.dartlib/data/repositories/log_storage.dartlib/features/logs/screens/logs_screen.dartlib/services/logger_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/shared/providers/logs_database_provider.dartlib/main.dartlib/data/repositories/log_storage.dartlib/features/logs/screens/logs_screen.dartlib/services/logger_service.dart
lib/shared/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Files:
lib/shared/providers/logs_database_provider.dart
lib/main.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Configure timeago package locales in app initialization for proper relative time formatting (e.g., 'hace X horas' vs 'hours ago')
Files:
lib/main.dart
lib/data/repositories/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Implement Repository pattern for all data access - all data operations must go through repository classes
Files:
lib/data/repositories/log_storage.dart
lib/features/**/screens/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/features/**/screens/**/*.dart: Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
UseS.of(context)!.yourKeyfor all user-facing strings instead of hardcoded text
Files:
lib/features/logs/screens/logs_screen.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/logger_service.dart
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: PR descriptions should capture intent, list key changes, link tracking issues, flag risk areas, include command output for manual tests, and provide screenshots for UI updates
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Name Riverpod providers using the `<Feature>Provider` or `<Feature>Notifier` convention
Applied to files:
lib/shared/providers/logs_database_provider.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Organize Riverpod providers by feature in `features/{feature}/providers/` using Notifier pattern for complex state logic
Applied to files:
lib/shared/providers/logs_database_provider.dartlib/main.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, 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/logs_database_provider.dartlib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to **/*.dart : Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
Applied to files:
lib/shared/providers/logs_database_provider.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
Repo: MostroP2P/mobile PR: 74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
lib/shared/providers/logs_database_provider.dartlib/main.dartlib/data/repositories/log_storage.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Use dual storage strategy: store Mostro/default relays in `settings.relays` and user relays in `settings.userRelays` with full JSON metadata via `toJson()`/`fromJson()`
Applied to files:
lib/main.dartlib/data/repositories/log_storage.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Track relay sources (user, mostro, default) using RelaySource enum for appropriate handling and storage strategy
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Persistence, APIs, and background jobs should live in `lib/data/` and `lib/background/`; generated localization output must be in `lib/generated/` and must stay untouched
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/shared/**/*.dart : Follow existing feature patterns when adding new shared utilities - refer to order, chat, and auth features as implementation examples
Applied to files:
lib/main.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/main.dart
📚 Learning: 2025-10-14T21:12:06.887Z
Learnt from: Catrya
Repo: MostroP2P/mobile PR: 327
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:141-154
Timestamp: 2025-10-14T21:12:06.887Z
Learning: In the MostroP2P mobile codebase, the notification system uses a two-layer localization pattern: providers/notifiers (without BuildContext access) call `showCustomMessage()` with string keys (e.g., 'orderTimeoutMaker', 'orderCanceled'), and the UI layer's `NotificationListenerWidget` has a switch statement that maps these keys to localized strings using `S.of(context)`. This architectural pattern properly separates concerns while maintaining full localization support for all user-facing messages.
Applied to files:
lib/main.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/data/repositories/**/*.dart : Implement Repository pattern for all data access - all data operations must go through repository classes
Applied to files:
lib/data/repositories/log_storage.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/features/logs/screens/logs_screen.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Use `S.of(context)!.yourKey` for all user-facing strings instead of hardcoded text
Applied to files:
lib/features/logs/screens/logs_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
🔇 Additional comments (2)
lib/services/logger_service.dart (1)
253-277: Logger construction looks reasonable; verifyfilter: _AlwaysStackTraceFilter()behavior withloggerv2.5.0.
You’re depending on stack traces to extract(service:line); please confirm the package actually supplies a usable stack for non-error logs in this configuration (otherwise you’ll mostly seeApp:0).lib/main.dart (1)
1-93: Don’t forgetflutter analyze+flutter testfor these Dart changes.
As per coding guidelines.
| String _getLogStorageLocation(BuildContext context) { | ||
| final settings = ref.watch(settingsProvider); | ||
| return settings.customLogStorageDirectory ?? S.of(context)!.defaultDownloads; | ||
| } |
There was a problem hiding this comment.
UI shows a storage path that _saveToDocuments() doesn’t use.
_getLogStorageLocation() reports settings.customLogStorageDirectory, but saving always writes under the resolved OS directory. This is misleading.
Future<String> _saveToDocuments(File tempFile) async {
+ final settings = ref.read(settingsProvider);
+ final customDir = settings.customLogStorageDirectory;
Directory? directory;
+ if (customDir != null && customDir.trim().isNotEmpty) {
+ directory = Directory(customDir);
+ }
+
+ if (directory != null) {
+ await directory.create(recursive: true);
+ }
+
- if (Platform.isAndroid) {
+ if (directory == null && Platform.isAndroid) {
@@
- } else if (Platform.isIOS) {
+ } else if (directory == null && Platform.isIOS) {
@@
- } else {
+ } else if (directory == null) {
@@
- final logsDir = Directory('${directory.path}/MostroLogs');
+ final logsDir = Directory('${directory!.path}/MostroLogs');Also applies to: 196-257, 626-659
🤖 Prompt for AI Agents
In lib/features/logs/screens/logs_screen.dart around lines 83-86 (and also
update the similar logic at 196-257 and 626-659), the displayed storage path is
taken from settings.customLogStorageDirectory but the actual save routine always
writes to the resolved OS directory, so the UI is misleading; change
_getLogStorageLocation to return the exact path that _saveToDocuments() will
write to (i.e., if settings.customLogStorageDirectory is set return that
absolute path, otherwise compute and return the platform-resolved
documents/downloads path used by the save routine), or alternatively change the
save routine to honor settings.customLogStorageDirectory — pick one consistent
approach and apply it to all three code locations so displayed and actual
storage locations match.
lib/services/logger_service.dart
Outdated
| final formattedLines = _printer.log(LogEvent( | ||
| event.level, | ||
| event.origin.message, | ||
| time: event.origin.time, | ||
| error: event.origin.error, | ||
| stackTrace: event.origin.stackTrace, | ||
| )); | ||
|
|
||
| if (formattedLines.isNotEmpty) { | ||
| final formattedLine = formattedLines[0]; | ||
| final serviceAndLine = _extractFromFormattedLine(formattedLine); | ||
|
|
||
| final entry = LogEntry( | ||
| timestamp: event.origin.time, | ||
| level: event.level, | ||
| message: _cleanMessage(event.origin.message.toString()), | ||
| service: serviceAndLine['service'] ?? 'App', | ||
| line: serviceAndLine['line'] ?? '0', | ||
| ); | ||
|
|
||
| _pendingBatch.add(entry); | ||
|
|
||
| if (_pendingBatch.length >= _batchFlushSize) { | ||
| _flushBatch(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Future<void> _flushBatch() async { | ||
| if (_storage == null || _pendingBatch.isEmpty) return; | ||
|
|
||
| final batch = List<LogEntry>.from(_pendingBatch); | ||
| _pendingBatch.clear(); | ||
|
|
||
| try { | ||
| for (final entry in batch) { | ||
| final id = entry.timestamp.millisecondsSinceEpoch.toString(); | ||
| await _storage!.putItem(id, entry); | ||
| } | ||
|
|
||
| final count = await _storage!.getCount(); | ||
| if (count > Config.logMaxEntries) { | ||
| await _storage!.deleteOldest(Config.logBatchDeleteSize); | ||
| } | ||
| } catch (e) { | ||
| // ignore: avoid_print | ||
| print('Failed to flush log batch: $e'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the LoggerService and related files
fd -e dart | grep -E "(logger|storage)" | head -20Repository: MostroP2P/mobile
Length of output: 571
🏁 Script executed:
# Get full context of logger_service.dart
wc -l lib/services/logger_service.dartRepository: MostroP2P/mobile
Length of output: 96
🏁 Script executed:
# Read the full logger_service.dart to understand the class structure
cat -n lib/services/logger_service.dartRepository: MostroP2P/mobile
Length of output: 9636
🏁 Script executed:
# Search for BaseStorage definition
rg -t dart "class BaseStorage" -A 10Repository: MostroP2P/mobile
Length of output: 843
🏁 Script executed:
# Search for putItem method signature
rg -t dart "putItem" -B 2 -A 5Repository: MostroP2P/mobile
Length of output: 8541
🏁 Script executed:
# Check imports in logger_service.dart to understand dependencies
rg -t dart "^import|^export" lib/services/logger_service.dartRepository: MostroP2P/mobile
Length of output: 233
🏁 Script executed:
# Get full BaseStorage to verify putItem semantics
cat -n lib/data/repositories/base_storage.dartRepository: MostroP2P/mobile
Length of output: 3607
🏁 Script executed:
# Check LogStorage implementation
cat -n lib/data/repositories/log_storage.dartRepository: MostroP2P/mobile
Length of output: 2155
🏁 Script executed:
# Verify LogEntry has proper serialization
rg -t dart "class LogEntry" -A 30 lib/services/logger_service.dartRepository: MostroP2P/mobile
Length of output: 866
🏁 Script executed:
# Check if LogStorage is using toDbMap/fromDbMap for LogEntry
rg -t dart "LogEntry" lib/data/repositories/log_storage.dart -B 3 -A 3Repository: MostroP2P/mobile
Length of output: 874
🏁 Script executed:
# Check where dispose() is called from
rg -t dart "\.dispose\(\)" --context 3 lib/services/logger_service.dartRepository: MostroP2P/mobile
Length of output: 42
🏁 Script executed:
# Check if dispose is async anywhere or if there's a pattern for handling it
rg -t dart "dispose()" lib/ -B 3 -A 3 | head -50Repository: MostroP2P/mobile
Length of output: 2536
🏁 Script executed:
# Check if there are any tests for logger_service
fd -e dart test | grep loggerRepository: MostroP2P/mobile
Length of output: 42
🏁 Script executed:
# Verify how LogStorage handles the timestamp key - check if there's ID generation consistency
rg -t dart "millisecondsSinceEpoch" lib/services/logger_service.dart lib/data/repositories/log_storage.dart -B 2 -A 2Repository: MostroP2P/mobile
Length of output: 728
🏁 Script executed:
# Find where setStorage is called and initialization happens
rg -t dart "setStorage\|MemoryLogOutput" lib/ -B 2 -A 2Repository: MostroP2P/mobile
Length of output: 42
🏁 Script executed:
# Check if there's any cleanup/shutdown logic in the app
fd -e dart | xargs rg -l "main\|runApp" | head -5Repository: MostroP2P/mobile
Length of output: 42
🏁 Script executed:
# Check main.dart for shutdown handling
cat -n lib/main.dart 2>/dev/null || cat -n lib/main.dart 2>/dev/null || find . -name "main.dart" -exec cat -n {} \;Repository: MostroP2P/mobile
Length of output: 4476
Prevent log loss: ID collisions, clear-before-write race, and missing async dispose.
Using millisecondsSinceEpoch as the record key causes overwrites when multiple logs are emitted in the same millisecond. The batch is cleared (line 86) before the storage write completes (line 91), so any write failure silently drops logs. Additionally, _flushBatch() is called without awaiting from three locations: the periodic timer (line 48), the size trigger (line 77), and dispose() (line 118), allowing concurrent flushes to corrupt state; dispose() in particular must complete before app shutdown, but currently returns synchronously without awaiting the pending write.
class MemoryLogOutput extends LogOutput {
static final MemoryLogOutput instance = MemoryLogOutput._();
+ int _seq = 0;
+ bool _isFlushing = false;
Future<void> _flushBatch() async {
if (_storage == null || _pendingBatch.isEmpty) return;
+ if (_isFlushing) return;
+ _isFlushing = true;
final batch = List<LogEntry>.from(_pendingBatch);
_pendingBatch.clear();
try {
for (final entry in batch) {
- final id = entry.timestamp.millisecondsSinceEpoch.toString();
+ // microseconds reduces collision risk; seq guards against same-microsecond bursts
+ final id = '${entry.timestamp.microsecondsSinceEpoch}_${_seq++}';
await _storage!.putItem(id, entry);
}
@@
} catch (e) {
+ // Re-queue to avoid silent loss
+ _pendingBatch.insertAll(0, batch);
// ignore: avoid_print
print('Failed to flush log batch: $e');
+ } finally {
+ _isFlushing = false;
}
}
- void dispose() {
+ Future<void> dispose() async {
_flushTimer?.cancel();
- _flushBatch();
+ await _flushBatch();
}Update main.dart line 59 to await the disposal of pending logs before app exit. Also ensure callers of dispose() (particularly app shutdown code) await this Future.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
lib/services/logger_service.dart lines 54-102: the flush logic causes lost or
overwritten logs and concurrent flush races — replace millisecond-based IDs with
a unique stable ID per entry (e.g., UUID or combination of timestamp+monotonic
counter) to avoid collisions; do not clear _pendingBatch before writes complete
— copy the batch to a local variable and only remove/trim the shared list after
successful writes or remove items as you confirm persistence, and protect flush
with a reentrant-safe guard (e.g., an _isFlushing bool or a mutex/Future that
serializes concurrent _flushBatch calls) so multiple concurrent calls don't
corrupt state; make _flushBatch return a Future that completes only after
storage writes and deletion are done and propagate errors (log but do not clear
on failure), and change dispose() to be async and await a final _flushBatch();
also update callers (main.dart line 59 and any shutdown code) to await dispose()
so pending logs are flushed before exit.
|
@Catrya hola, he intentado varias cosas pero aun no lo he logrado. |
BraCR10
force-pushed
the
feat/br/logs
branch
from
4ffc008 to
49594e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/background/mobile_background_service.dart (2)
43-51: IncludeloggerSendPorton allstartinvocations + avoid logging full settings payload.
Right nowservice.invoke('start', ...)in theon-starthandler omitsloggerSendPort, and the debug log prints the full settings map (risking secret/PII leakage to console logs).Proposed diff:
service.on('on-start').listen((data) { _isRunning = true; service.invoke('start', { 'settings': _settings.toJson(), + 'loggerSendPort': logger_service.isolateLogSenderPort, }); - logger_service.logger.d( - 'Service started with settings: ${_settings.toJson()}', - ); + logger_service.logger.d('Service started'); });
127-135: Preserve stack traces on retryable start failures.
Catching onlyedrops the stack trace, making “start failed” reports hard to debug.Suggested diff:
- } catch (e) { - logger_service.logger.e('Error starting service: $e'); + } catch (e, st) { + logger_service.logger.e('Error starting service', error: e, stackTrace: st); // Retry with a delay if needed await Future.delayed(Duration(seconds: 1)); await _startService(); }
🧹 Nitpick comments (3)
lib/services/logger_service.dart (3)
70-105: Avoid re-formatting just to extract service/line (perf + correctness).
MemoryLogOutput.output()calls_printer.log(...)to generate a formatted line, then parses it back. Consider extracting service/line directly fromevent.origin.stackTrace(you already have_extractFromStackTracelogic inSimplePrinter), or haveSimplePrinterreturn structured fields.
107-116: Regex(\w+)is too restrictive for Dart filenames.
If a filename contains characters outside\w(e.g., hyphens), service extraction will silently fall back toApp:0.Suggested tweak:
- final match = RegExp(r'\[(?:ERROR|WARN|INFO|DEBUG|TRACE)\]\((\w+):(\d+)\)').firstMatch(line); + final match = RegExp(r'\[(?:ERROR|WARN|INFO|DEBUG|TRACE)\]\(([^:)]+):(\d+)\)').firstMatch(line);
243-267: Cached loggers won’t reflect runtime changes toConfig.fullLogsInfo.
IffullLogsInfois user-toggleable, you’ll need a way to rebuild the logger or clear_cachedSimpleLogger/_cachedFullLogger.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/background/background.dart(4 hunks)lib/background/desktop_background_service.dart(2 hunks)lib/background/mobile_background_service.dart(7 hunks)lib/main.dart(1 hunks)lib/services/logger_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/main.dart
- lib/background/background.dart
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/background/mobile_background_service.dartlib/background/desktop_background_service.dartlib/services/logger_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/background/mobile_background_service.dartlib/background/desktop_background_service.dartlib/services/logger_service.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/logger_service.dart
🧠 Learnings (8)
📚 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/background/mobile_background_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/**/*.dart : Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Applied to files:
lib/background/mobile_background_service.dartlib/background/desktop_background_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/services/nostr_service.dart : Manage all relay connections and Nostr messaging through NostrService - automatically reconnect when relay list updates
Applied to files:
lib/background/mobile_background_service.dartlib/background/desktop_background_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/notifiers/**/*.dart : Encapsulate business logic in Notifiers - Notifiers should expose state via providers and handle all complex state transitions
Applied to files:
lib/background/mobile_background_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/providers/**/*.dart : Use Notifier pattern instead of simple StateNotifier for complex state logic requiring business rule encapsulation
Applied to files:
lib/background/mobile_background_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/subscriptions/subscription_manager.dart : Use SubscriptionManager with `fireImmediately: false` during SessionNotifier initialization to prevent premature execution
Applied to files:
lib/background/mobile_background_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/services/logger_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/services/logger_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 (4)
lib/background/mobile_background_service.dart (1)
139-160: VerifyisolateLogSenderPortis initialized before_startService()runs.
IfinitIsolateLogReceiver()wasn’t called yet,loggerSendPortwill be null and isolate logs won’t appear in the in-app buffer.Run/confirm in your app startup sequence that isolate log receiver init happens before any background service start (including auto-start paths).
lib/background/desktop_background_service.dart (2)
22-37: Isolate-local logger wiring looks correct (SendPort is optional).
This avoids relying on main-isolate globals and cleanly enables log forwarding when a port is provided.
26-36: Confirmloggerpackage v2.5.0 API used here (Level,.name) matches your constraints.logger Dart package v2.5.0 Level enum values (does it include trace?) and does Level have a `.name` getter returning strings like "debug"/"info"/"warning"/"error"?lib/services/logger_service.dart (1)
1-310: Run analyzer/tests for Dart changes (per repo guidelines).flutter analyze flutter test
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/services/logger_service.dart (1)
26-44: Enhance sanitization to catch raw hex keys and refine the catch-all pattern.The current implementation sanitizes
nsec,privateKey, andmnemonicfields, but may miss:
- Raw 64-character hex strings (common for Nostr private keys)
- Raw 128-character hex strings (seed phrases in hex)
- The catch-all regex on line 41 may be overly aggressive and corrupt legitimate log content
Consider adding these patterns before line 41:
.replaceAll(RegExp(r'"mnemonic"\s*:\s*"[^"]*"'), '"mnemonic":"[REDACTED]"') + .replaceAll(RegExp(r'\b[0-9a-fA-F]{64}\b'), '[HEX_KEY]') + .replaceAll(RegExp(r'\b[0-9a-fA-F]{128}\b'), '[HEX_SEED]') .replaceAll(RegExp(r'[^A-Za-z0-9\s.:,!?\-_/\[\]]'), ' ')And consider making line 41's character filter more permissive (e.g., allow parentheses, quotes, and common punctuation) to preserve log readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/services/logger_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/services/logger_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/services/logger_service.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/logger_service.dart
🧠 Learnings (2)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/services/logger_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/services/logger_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 (4)
lib/services/logger_service.dart (4)
10-22: LGTM! Idempotency issue resolved.The early return guard at lines 11-12 prevents port leaks and duplicate listeners, addressing the previous review concern.
46-85: LGTM! Previous validation and trimming concerns resolved.The function now properly:
- Validates and sanitizes the timestamp with fallback to
DateTime.now()(lines 47-58)- Validates all incoming fields with safe defaults (lines 60-66)
- Sanitizes messages using
cleanMessage()(line 64)- Performs bounds-safe buffer trimming (lines 76-84)
273-300: LGTM! Clean logger caching and configuration.The cached logger pattern prevents unnecessary recreation, and the configuration-based selection between
PrettyPrinterandSimplePrinterprovides appropriate flexibility. The_AlwaysStackTraceFilterensures stack traces are captured for service/line extraction.
302-333: LGTM! Previous isolate forwarding concerns resolved.The implementation now:
- Preserves original caller metadata by extracting service/line from
event.origin.stackTrace(lines 317-318)- Sanitizes messages using
cleanMessage()before forwarding (lines 321-322)This addresses the previous review concerns about losing caller context and forwarding unsanitized data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/services/logger_service.dart (4)
16-20: Add error handling in the isolate listener.The listener callback invokes
addLogFromIsolatewithout error handling. If that function throws (e.g., due to unexpected data format), the listener may terminate or propagate the exception unhandled.Apply this diff to guard against unexpected errors:
_isolateLogReceiver!.listen((message) { - if (message is Map<String, dynamic>) { - addLogFromIsolate(message); - } + try { + if (message is Map<String, dynamic>) { + addLogFromIsolate(message); + } + } catch (e) { + // Silently drop malformed isolate logs to avoid crashing the listener + } });
39-39: Verify that the aggressive character filter doesn't remove useful log content.The pattern
[^A-Za-z0-9\s.:,!?\-_/\[\]]removes many common punctuation marks (parentheses, quotes, braces, semicolons, equals signs, etc.) that are often useful in debugging logs, especially for JSON structures or stack traces.Consider whether this whitelist is too restrictive for your logging needs. If legitimate log content is being mangled, you might instead target specific known-problematic patterns or expand the allowed character set.
125-155: Consider extracting service/line directly from stackTrace.The current approach formats the log with
SimplePrinter, then parses the formatted string with a regex to extract service and line information. This is a round-trip that could be avoided by extracting directly fromevent.origin.stackTrace.You could refactor like this:
@override void output(OutputEvent event) { - final formattedLines = _printer.log(LogEvent( - event.level, - event.origin.message, - time: event.origin.time, - error: event.origin.error, - stackTrace: event.origin.stackTrace, - )); - - if (formattedLines.isNotEmpty) { - final formattedLine = formattedLines[0]; - final serviceAndLine = _extractFromFormattedLine(formattedLine); + final serviceAndLine = _printer.extractFromStackTrace(event.origin.stackTrace); - _buffer.add(LogEntry( - timestamp: event.origin.time, - level: event.level, - message: cleanMessage(event.origin.message.toString()), - service: serviceAndLine['service'] ?? 'App', - line: serviceAndLine['line'] ?? '0', - )); + _buffer.add(LogEntry( + timestamp: event.origin.time, + level: event.level, + message: cleanMessage(event.origin.message.toString()), + service: serviceAndLine['service'] ?? 'App', + line: serviceAndLine['line'] ?? '0', + )); - if (_buffer.length > Config.logMaxEntries) { - final deleteCount = _buffer.length < Config.logBatchDeleteSize - ? _buffer.length - Config.logMaxEntries - : Config.logBatchDeleteSize; - if (deleteCount > 0) { - _buffer.removeRange(0, deleteCount); - } + if (_buffer.length > Config.logMaxEntries) { + final deleteCount = _buffer.length < Config.logBatchDeleteSize + ? _buffer.length - Config.logMaxEntries + : Config.logBatchDeleteSize; + if (deleteCount > 0) { + _buffer.removeRange(0, deleteCount); } } }This would eliminate the format-then-parse round-trip and the now-unused
_extractFromFormattedLinehelper.
312-326: Cache SimplePrinter instance to avoid repeated allocation.A new
SimplePrinteris created on every log event (line 313). Consider making it a class field to avoid the overhead.Apply this diff:
class IsolateLogOutput extends LogOutput { final SendPort? sendPort; + final SimplePrinter _printer = SimplePrinter(); IsolateLogOutput(this.sendPort); @override void output(OutputEvent event) { for (final line in event.lines) { // ignore: avoid_print print(line); } if (sendPort != null) { - final printer = SimplePrinter(); - final serviceAndLine = printer.extractFromStackTrace(event.origin.stackTrace); + final serviceAndLine = _printer.extractFromStackTrace(event.origin.stackTrace); final rawMessage = event.origin.message.toString(); final sanitizedMessage = cleanMessage(rawMessage); sendPort!.send({ 'timestamp': event.origin.time.toIso8601String(), 'level': event.level.name, 'message': sanitizedMessage, 'service': serviceAndLine['service'] ?? 'Background', 'line': serviceAndLine['line'] ?? '0', }); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/services/logger_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/services/logger_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/services/logger_service.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/logger_service.dart
🧠 Learnings (2)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/services/logger_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/services/logger_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 (5)
lib/services/logger_service.dart (5)
44-80: LGTM!The isolate log processing is well-structured with robust error handling, sensible defaults, proper sanitization, and safe buffer management.
82-113: LGTM!The helper functions and LogEntry data structure are clean and straightforward.
173-194: LGTM!The output routing classes are simple and correct.
196-263: LGTM!The SimplePrinter implementation is well-designed with robust stack trace parsing that correctly skips internal frames and extracts caller information.
265-297: LGTM!The logger factory correctly caches instances and provides two configurations based on
Config.fullLogsInfo. The use of_AlwaysStackTraceFilteronly with the simple logger ensures stack traces are captured for service/line extraction.
|
Hello @Catrya this new approach solves the problem related to background isolates not being able to write to the main thread's log buffer by forwarding logs via SendPort with validation. 1. Main Thread -
|
|
@BraCR10 |
|
@Catrya |
Tienes razón. Las instancias de NostrService en background ya existían antes de este PR, disculpa. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/services/logger_service.dart (2)
126-128: Remove misleadingStackTrace.currentfallback.Using
StackTrace.currentwhenevent.origin.stackTraceis null will point tologger_service.dartrather than the actual caller, making logs less useful. TheextractFromStackTracemethod already handlesnullgracefully (line 213).🔎 Proposed fix
@override void output(OutputEvent event) { - // Use StackTrace.current as fallback to get accurate caller info - final stackTrace = event.origin.stackTrace ?? StackTrace.current; + final stackTrace = event.origin.stackTrace; final serviceAndLine = _printer.extractFromStackTrace(stackTrace);Based on past review feedback, this issue was flagged but appears unresolved in commit 6698ea8.
184-186: Remove misleadingStackTrace.currentfallback.Same issue as line 127: using
StackTrace.currentwhenevent.stackTraceis null will point tologger_service.dartrather than the actual caller. TheextractFromStackTracemethod already handlesnullgracefully (line 213).🔎 Proposed fix
final timestamp = event.time.toString().substring(0, 19); - // Use StackTrace.current as fallback to get accurate caller info - final stackTrace = event.stackTrace ?? StackTrace.current; + final stackTrace = event.stackTrace; final serviceAndLine = extractFromStackTrace(stackTrace);Based on past review feedback, this issue was flagged but appears unresolved in commit 6698ea8.
🧹 Nitpick comments (5)
lib/services/logger_service.dart (5)
25-42: Consider redacting additional sensitive formats.The sanitization covers
nseckeys and JSON fields containingprivateKey/mnemonic, but may miss other common sensitive formats in Nostr applications:
- Raw hex keys (64 or 128 character hex strings)
- Public keys in
npubformat (if they should be considered sensitive in your context)- Authorization tokens or session identifiers
🔎 Optional enhancement
.replaceAll(RegExp(r'nsec[0-9a-z]+'), '[PRIVATE_KEY]') + .replaceAll(RegExp(r'\b[0-9a-fA-F]{64,128}\b'), '[HEX_KEY]') + .replaceAll(RegExp(r'npub[0-9a-z]+'), '[PUBLIC_KEY]') .replaceAll(RegExp(r'"privateKey"\s*:\s*"[^"]*"'), '"privateKey":"[REDACTED]"') .replaceAll(RegExp(r'"mnemonic"\s*:\s*"[^"]*"'), '"mnemonic":"[REDACTED]"') + .replaceAll(RegExp(r'"token"\s*:\s*"[^"]*"'), '"token":"[REDACTED]"') .replaceAll(RegExp(r'[^A-Za-z0-9\s.:,!?\-_/\[\]]'), ' ')
64-70: Encapsulation: add logs via a public method instead of direct buffer access.Directly accessing
MemoryLogOutput.instance._bufferfrom outside the class breaks encapsulation and makes the code harder to maintain. Consider adding a public method likeaddEntry()toMemoryLogOutput.🔎 Proposed refactor
In
MemoryLogOutputclass, add:void addEntry(LogEntry entry) { _buffer.add(entry); if (_buffer.length > Config.logMaxEntries) { final deleteCount = _buffer.length < Config.logBatchDeleteSize ? _buffer.length - Config.logMaxEntries : Config.logBatchDeleteSize; if (deleteCount > 0) { _buffer.removeRange(0, deleteCount); } } }Then update line 64:
- MemoryLogOutput.instance._buffer.add(LogEntry( + MemoryLogOutput.instance.addEntry(LogEntry( timestamp: timestamp, level: level, message: message, service: service, line: line, )); - - if (MemoryLogOutput.instance._buffer.length > Config.logMaxEntries) { - final deleteCount = MemoryLogOutput.instance._buffer.length < Config.logBatchDeleteSize - ? MemoryLogOutput.instance._buffer.length - Config.logMaxEntries - : Config.logBatchDeleteSize; - if (deleteCount > 0) { - MemoryLogOutput.instance._buffer.removeRange(0, deleteCount); - } - }
108-112: Consider safer timestamp formatting.Using
substring(0, 19)assumes the timestamp string is always at least 19 characters. While DateTime.toString() typically produces a longer string, consider using a more explicit format to avoid potential edge cases.🔎 Optional enhancement
String format() { - final time = timestamp.toString().substring(0, 19); + final time = '${timestamp.year}-${timestamp.month.toString().padLeft(2, '0')}-' + '${timestamp.day.toString().padLeft(2, '0')} ' + '${timestamp.hour.toString().padLeft(2, '0')}:' + '${timestamp.minute.toString().padLeft(2, '0')}:' + '${timestamp.second.toString().padLeft(2, '0')}'; final levelStr = level.toString().split('.').last.toUpperCase(); return '[$levelStr]($service:$line) $time - $message'; }Or use the
intlpackage for more robust date formatting.
155-163: Consider using built-inConsoleOutputfrom logger package.The
_ConsoleOnlyOutputclass is functionally identical to theConsoleOutputclass provided by the logger package. Using the built-in class would reduce code duplication.🔎 Proposed change
Remove the
_ConsoleOnlyOutputclass and useConsoleOutput()directly at line 274:_cachedSimpleLogger ??= Logger( printer: SimplePrinter(), - output: _MultiOutput(MemoryLogOutput.instance, _ConsoleOnlyOutput()), + output: _MultiOutput(MemoryLogOutput.instance, ConsoleOutput()), level: Level.debug, filter: _AlwaysStackTraceFilter(), );
10-21: Consider adding a disposal mechanism forReceivePort.While the
ReceivePortis intended to live for the app's lifetime, providing a way to close it (e.g.,disposeIsolateLogReceiver()) would enable cleaner shutdown and facilitate testing.🔎 Optional enhancement
void initIsolateLogReceiver() { if (_isolateLogReceiver != null) return; _isolateLogReceiver = ReceivePort(); _isolateLogSender = _isolateLogReceiver!.sendPort; _isolateLogReceiver!.listen((message) { if (message is Map<String, dynamic>) { addLogFromIsolate(message); } }); } + +void disposeIsolateLogReceiver() { + _isolateLogReceiver?.close(); + _isolateLogReceiver = null; + _isolateLogSender = null; +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/services/logger_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/services/logger_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/services/logger_service.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/logger_service.dart
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: PR descriptions should capture intent, list key changes, link tracking issues, flag risk areas, include command output for manual tests, and provide screenshots for UI updates
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/services/logger_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/services/logger_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 (3)
lib/services/logger_service.dart (3)
82-91: LGTM!Level parsing is correct and handles unknown values gracefully with a sensible default.
252-280: LGTM!Logger configuration and caching logic is well-structured. The conditional logic properly selects between simple and full logging modes, and both configurations correctly route logs to the memory buffer and console.
282-311: LGTM!
IsolateLogOutputcorrectly forwards logs from background isolates to the main thread. The implementation properly:
- Uses the original stack trace without misleading fallbacks
- Sanitizes messages before transmission
- Sends well-structured data compatible with
addLogFromIsolate
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/services/logger_service.dart (2)
132-133: Remove misleadingStackTrace.currentfallback (still present despite past review).This was flagged in a previous review (commit 6698ea8) but remains in the code. Using
StackTrace.currentwhenevent.origin.stackTraceis null will point tologger_service.dartrather than the actual caller, making logs less useful. TheextractFromStackTracemethod already handlesnullgracefully (line 218).🔎 Proposed fix
- // Use StackTrace.current as fallback to get accurate caller info - final stackTrace = event.origin.stackTrace ?? StackTrace.current; + final stackTrace = event.origin.stackTrace; final serviceAndLine = _printer.extractFromStackTrace(stackTrace);
190-191: Remove misleadingStackTrace.currentfallback (still present despite commit 6698ea8).A previous review explicitly flagged this line and marked it as addressed in commit 6698ea8, but the fallback remains. Using
StackTrace.currentwhenevent.stackTraceis null points tologger_service.dartinstead of the actual caller. TheextractFromStackTracemethod already handlesnullsafely (line 218).🔎 Proposed fix
- // Use StackTrace.current as fallback to get accurate caller info - final stackTrace = event.stackTrace ?? StackTrace.current; + final stackTrace = event.stackTrace; final serviceAndLine = extractFromStackTrace(stackTrace);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/services/logger_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/services/logger_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/services/logger_service.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/logger_service.dart
🧠 Learnings (2)
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/services/logger_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/services/logger_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 (3)
lib/services/logger_service.dart (3)
10-26: LGTM: Isolate receiver initialization is properly guarded.The idempotency check (line 11) and error handling (lines 18-23) correctly address the concerns from previous reviews. Printing errors directly to console avoids infinite recursion and is appropriate here.
49-85: LGTM: Input validation and buffer management are robust.The function properly validates all incoming fields, applies message sanitization, and uses safe batch deletion logic to prevent
RangeError(lines 78-82). This addresses the critical issues from previous reviews.
287-316: LGTM: Isolate log forwarding correctly preserves caller context and sanitizes data.The implementation properly extracts service/line from the original
event.origin.stackTrace(line 302) without using a misleading fallback, sanitizes the message (line 305), and safely handles nullsendPort. This correctly addresses the critical issue from previous reviews.
| .replaceAll(RegExp(r'nsec[0-9a-z]+'), '[PRIVATE_KEY]') | ||
| .replaceAll(RegExp(r'"privateKey"\s*:\s*"[^"]*"'), '"privateKey":"[REDACTED]"') | ||
| .replaceAll(RegExp(r'"mnemonic"\s*:\s*"[^"]*"'), '"mnemonic":"[REDACTED]"') | ||
| .replaceAll(RegExp(r'[^A-Za-z0-9\s.:,!?\-_/\[\]]'), ' ') |
There was a problem hiding this comment.
Consider relaxing the character whitelist to preserve log readability.
Line 44 removes parentheses, braces, quotes, equals signs, and other punctuation that helps preserve structure in logs (e.g., JSON, function calls, key-value pairs). This can make debugging significantly harder.
Consider allowing at least: (){}="' in addition to the current set.
🔎 Suggested adjustment
- .replaceAll(RegExp(r'[^A-Za-z0-9\s.:,!?\-_/\[\]]'), ' ')
+ .replaceAll(RegExp(r'[^A-Za-z0-9\s.:,!?\-_/\[\](){}="\'@#]'), ' ')🤖 Prompt for AI Agents
In lib/services/logger_service.dart around line 44, the current regex in
replaceAll strips useful punctuation making logs hard to read; update the
character whitelist to also permit parentheses, braces, double and single
quotes, and equals (e.g. allow (){}="' ) by modifying the RegExp so those
characters are included in the character class (keep existing allowed characters
and add the new ones) so structured content (JSON, key=value, function calls)
remains intact in logs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/services/logger_service.dart (2)
46-46: Character whitelist may still be too restrictive for debugging.As previously noted, line 46 removes parentheses, braces, quotes, and equals signs that help preserve structure in logs (JSON, function calls, key-value pairs).
259-287: Logger caching still doesn't respond to runtime config changes.As previously noted, the cached loggers (
_cachedSimpleLoggerand_cachedFullLogger) are selected based onConfig.fullLogsInfobut never invalidated. Additionally,_AlwaysStackTraceFilter(lines 254-257) always returnstrue, making it redundant.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/services/logger_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/services/logger_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/services/logger_service.dart
lib/services/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Access NostrService for all Nostr protocol interactions - NostrService manages relay connections and messaging
Files:
lib/services/logger_service.dart
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: PR descriptions should capture intent, list key changes, link tracking issues, flag risk areas, include command output for manual tests, and provide screenshots for UI updates
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/auth/notifiers/abstract_mostro_notifier.dart : Start 10-second cleanup timer automatically when taking orders via `startSessionTimeoutCleanup()` to prevent orphan sessions
Applied to files:
lib/services/logger_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/services/logger_service.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Implement relay URL normalization by removing trailing slashes in all blacklist operations to ensure consistent matching
Applied to files:
lib/services/logger_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 (2)
lib/services/logger_service.dart (2)
10-28: LGTM: Isolate receiver initialization is now safe.The idempotency guard (line 11) prevents port leaks, and the error handling (lines 18-25) prevents crashes from malformed messages. Both previously flagged issues have been properly addressed.
51-87: LGTM: Robust validation and safe buffer management.Timestamp parsing includes proper error handling (lines 52-62), fields are safely extracted with defaults (lines 64-69), message sanitization is applied (line 67), and buffer trimming uses safe bounds checking (lines 79-86). All previously raised concerns have been addressed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/app/src/main/AndroidManifest.xml(1 hunks)lib/features/logs/screens/logs_screen.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/logs/screens/logs_screen.dart
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: PR descriptions should capture intent, list key changes, link tracking issues, flag risk areas, include command output for manual tests, and provide screenshots for UI updates
⏰ 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
| <uses-permission android:name="android.permission.FOREGROUND_SERVICE_DATA_SYNC" /> | ||
| <uses-permission android:name="android.permission.POST_NOTIFICATIONS" /> | ||
| <uses-permission android:name="android.permission.INTERNET" /> | ||
| <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" android:maxSdkVersion="32" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the log export implementation
find . -type f -name "*.dart" -o -name "*.java" -o -name "*.kt" | head -20Repository: MostroP2P/mobile
Length of output: 814
🏁 Script executed:
#!/bin/bash
# Search for log export functionality
rg -l "log.*export|export.*log|Downloads" --type dart lib/ | head -20Repository: MostroP2P/mobile
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Read the logs_screen.dart file
cat -n lib/features/logs/screens/logs_screen.dart | head -100Repository: MostroP2P/mobile
Length of output: 3686
🏁 Script executed:
#!/bin/bash
# Continue reading the logs_screen.dart file - look for export function
cat -n lib/features/logs/screens/logs_screen.dart | grep -A 50 "export\|_export\|share"Repository: MostroP2P/mobile
Length of output: 5491
🏁 Script executed:
#!/bin/bash
# Find the _saveToDocuments and _createLogFile functions
cat -n lib/features/logs/screens/logs_screen.dart | grep -A 20 "_saveToDocuments\|_createLogFile"Repository: MostroP2P/mobile
Length of output: 3260
🏁 Script executed:
#!/bin/bash
# Get the rest of _saveToDocuments function
cat -n lib/features/logs/screens/logs_screen.dart | sed -n '597,650p'Repository: MostroP2P/mobile
Length of output: 2342
The manifest permission is correct but the implementation has critical Android 13+ compatibility issues.
The WRITE_EXTERNAL_STORAGE permission with maxSdkVersion="32" correctly limits the legacy permission to Android 12L and below. However, the log export implementation has a critical flaw for Android 13+ devices.
The _shareLogs() function works correctly—it uses FileProvider to securely provide access to files across app boundaries on Android, which is compatible with all Android versions.
The _saveToDevice() function will fail on Android 13+ (API 33+). The code:
- Uses
getExternalStorageDirectory()with manual path string manipulation to access the Downloads folder - Performs direct file writes to public storage directories
- Does not use scoped storage APIs, which apps targeting API 30+ must use
Direct access to shared/external storage is deprecated; when an app targets API level 29+, paths returned by getExternalStorageDirectory() are no longer directly accessible. On Android 11+, MediaProvider becomes the file system handler for external storage via FUSE and can intercept kernel calls to ensure file operations are privacy-safe. This blocks the direct file operations the current implementation attempts.
Migrate the _saveToDevice() logic to use MediaStore.Downloads collection (available on Android 10+), which requires no storage permissions for files the app creates, or implement version-specific logic that uses the Storage Access Framework for Android 13+.
🤖 Prompt for AI Agents
In android/app/src/main/AndroidManifest.xml around line 66, the manifest
correctly limits WRITE_EXTERNAL_STORAGE with maxSdkVersion="32", but the app's
_saveToDevice() implementation uses getExternalStorageDirectory() and direct
file writes which will fail on Android 11+ (and Android 13/API33+) due to scoped
storage; update _saveToDevice() to use MediaStore.Downloads (insert via
ContentResolver with proper ContentValues) for Android 10+ so you can save files
to Downloads without storage permission, and implement a versioned fallback that
uses the Storage Access Framework (ACTION_CREATE_DOCUMENT / SAF) for devices
where MediaStore is not appropriate or when targeting stricter scopes; keep
_shareLogs() using FileProvider unchanged and remove any reliance on legacy
external storage paths or WRITE_EXTERNAL_STORAGE for API 30+.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/features/logs/screens/logs_screen.dart (1)
77-80: Storage path display remains inconsistent with actual save location.The UI displays
settings.customLogStorageDirectory, but_saveToDocuments()(lines 597-627) ignores this setting and always writes to the platform-resolved directory. Users see one path but files are saved elsewhere.Either honor
customLogStorageDirectoryin the save routine, or display the actual resolved path that will be used.As per past review comments, this was marked as addressed but the issue persists in the current code.
🧹 Nitpick comments (2)
lib/features/logs/screens/logs_screen.dart (2)
684-702: SnackBar helper methods should checkmounteddefensively.While current call sites properly check
mountedbefore invoking these methods, the methods themselves don't verify it. This is fragile—future callers might forget the check.🔎 Proposed refactor
void _showSuccessSnackBar(String message) { + if (!mounted) return; ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text(message), backgroundColor: Colors.green, behavior: SnackBarBehavior.floating, ), ); } void _showErrorSnackBar(String message) { + if (!mounted) return; ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text(message), backgroundColor: Theme.of(context).colorScheme.error, behavior: SnackBarBehavior.floating, ), ); }Based on coding guidelines requiring mounted checks after async operations.
133-137: Consider usingconstfor Icon constructors where possible.Several Icon widgets have compile-time constant parameters and can be marked
constfor better performance and immutability.Examples
Line 134-136:
- child: const Icon( + child: Icon( Icons.arrow_upward, color: Colors.white, size: 20, ),Line 294-296:
- const Icon( + Icon( Icons.info_outline, size: 64, color: AppTheme.textInactive, ),Similar changes at lines 410, 453, and 461.
Actually, upon closer inspection these are already const or cannot be const due to dynamic context. Please verify if const is applicable in each case.
Based on coding guidelines encouraging const constructors for performance.
Also applies to: 293-297, 410-410, 453-453, 461-461
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/features/logs/screens/logs_screen.dart(1 hunks)lib/l10n/intl_en.arb(1 hunks)lib/l10n/intl_es.arb(1 hunks)lib/l10n/intl_it.arb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/l10n/intl_en.arb
- lib/l10n/intl_it.arb
🧰 Additional context used
📓 Path-based instructions (4)
lib/l10n/**/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
Add new localization keys to all three ARB files (en, es, it) - use
S.of(context)!.keyNamefor all user-facing strings
Files:
lib/l10n/intl_es.arb
**/*.{dart,flutter}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{dart,flutter}: Runflutter analyzeafter any code change - Mandatory before commits to ensure zero linting issues
Runflutter testafter any code change - Mandatory before commits to ensure all unit tests pass
Files:
lib/features/logs/screens/logs_screen.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: Use Riverpod for all state management - encapsulate business logic in Notifiers and access data only through repository classes
All code comments must be in English - use clear, concise English for variable names, function names, and comments
Always checkmountedbefore using BuildContext after async operations to prevent errors on disposed widgets
Useconstconstructors where possible for better performance and immutability
Remove unused imports and dependencies to maintain code cleanliness and reduce build size
**/*.dart: Application code should be organized underlib/, grouped by domain withlib/features/<feature>/structure, shared utilities inlib/shared/, dependency wiring inlib/core/, and services inlib/services/
Persistence, APIs, and background jobs should live inlib/data/andlib/background/; generated localization output must be inlib/generated/and must stay untouched
Applyflutter format .to enforce canonical Dart formatting (two-space indentation, trailing commas) before committing
Resolve every analyzer warning in Dart code
Name Riverpod providers using the<Feature>Provideror<Feature>Notifierconvention
Localize all user-facing strings via ARB files and access them withS.of(context)rather than hard-coded literals
Files:
lib/features/logs/screens/logs_screen.dart
lib/features/**/screens/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/features/**/screens/**/*.dart: Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
UseS.of(context)!.yourKeyfor all user-facing strings instead of hardcoded text
Files:
lib/features/logs/screens/logs_screen.dart
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: PR descriptions should capture intent, list key changes, link tracking issues, flag risk areas, include command output for manual tests, and provide screenshots for UI updates
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/l10n/**/*.arb : Add new localization keys to all three ARB files (en, es, it) - use `S.of(context)!.keyName` for all user-facing strings
Applied to files:
lib/l10n/intl_es.arblib/features/logs/screens/logs_screen.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/generated/l10n*.dart : Never manually edit generated localization files - regenerate with `dart run build_runner build -d` after modifying ARB files
Applied to files:
lib/l10n/intl_es.arblib/features/logs/screens/logs_screen.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Keep UI code declarative and side-effect free - use post-frame callbacks for side effects like SnackBars/dialogs
Applied to files:
lib/features/logs/screens/logs_screen.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/**/screens/**/*.dart : Use `S.of(context)!.yourKey` for all user-facing strings instead of hardcoded text
Applied to files:
lib/features/logs/screens/logs_screen.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/**/*.dart : Use dual storage strategy: store Mostro/default relays in `settings.relays` and user relays in `settings.userRelays` with full JSON metadata via `toJson()`/`fromJson()`
Applied to files:
lib/features/logs/screens/logs_screen.dart
📚 Learning: 2025-11-27T12:10:26.407Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:10:26.407Z
Learning: Applies to **/*.dart : Localize all user-facing strings via ARB files and access them with `S.of(context)` rather than hard-coded literals
Applied to files:
lib/features/logs/screens/logs_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
🔇 Additional comments (1)
lib/l10n/intl_es.arb (1)
1177-1236: Spanish localization for logs feature is well-structured and complete.All log-related localization keys are properly formatted with correct JSON syntax, appropriate placeholder definitions, and contextually accurate Spanish translations. All 38 new keys are consistently present across the English, Spanish, and Italian ARB files, satisfying the requirement to add new localization keys to all three files.
|
@Catrya El logger no funcionaba en release mode porque el paquete logger por defecto bloquea todos los logs en release mode para optimización. La solución fue agregar filter: _AlwaysStackTraceFilter() al logger completo. Sería importante abrir otro issue para permitir que el usuario pueda guardar los logs en la ubicación que desee. Esto implicaría solicitar permisos de acceso al almacenamiento, los cuales creo que la app actualmente no solicita. Por ahora, los logs se pueden compartir sin problemas y guardar en un path hardcoded |
2 participants
This PR includes a new functionality to allow users to check application logs.
Closes : #316
Please let me now if any change or approach implementation is required
Key details:
Configuration Settings (mobile\lib\core\config.dart)
Three new logger configuration settings were added:
logMaxEntries (1000): Maximum number of log entries stored in memory buffer before cleanup is triggered. This prevents unlimited memory growth while maintaining a reasonable history for debugging.
logBatchDeleteSize (100): Number of oldest log entries removed when buffer reaches maximum capacity. Batch deletion is more efficient than removing entries one by one.
fullLogsInfo (true): Controls console output format. When true, uses PrettyPrinter with detailed formatting, stack traces, and visual boxes for development (the current logs style). When false, uses SimplePrinter with compact single-line format.
Memory buffer always stores compact format regardless of this setting.
Logger Service (mobile\lib\services\logger_service.dart)
Core logging infrastructure with three main components:
MemoryLogOutput: Captures all logs to in-memory circular buffer. Extracts service name and line number from stack trace, sanitizes sensitive data (private keys, mnemonics, hex values), and stores in structured LogEntry
format. When buffer exceeds logMaxEntries, oldest logBatchDeleteSize entries are removed.
SimplePrinter: Produces compact single-line format LEVEL Timestamp - Message. Automatically extracts service name from stack trace by parsing dart file names, skipping logger internals.
Global logger getter: Returns singleton Logger instance. Uses PrettyPrinter with full details when fullLogsInfo is true, SimplePrinter for compact output when false. Both configurations use MultiOutput to simultaneously
write to console and memory buffer.
Logs Screen (mobile\lib\features\logs)
User interface for viewing, filtering, and exporting captured logs. Features include:
Memory Buffer Architecture
Logs are stored in memory rather than database for performance reasons. Database writes would create significant overhead for high-frequency logging operations. Memory buffer provides:
Users can export logs to files when longer-term storage is required. The in-memory approach balances performance with utility.
Migration to Logger Service
All files previously using direct Logger instances have been migrated to use the global logger from logger_service.dart:
This migration ensures all application logs are captured in the memory buffer and accessible through the logs screen. The centralized logger service provides consistent formatting and automatic service name extraction across the entire codebase.
Note: background processes use an isolated logging system
Summary by CodeRabbit
New Features
Settings / UI
Chores / Infra
Localization
✏️ Tip: You can customize this high-level summary in your review settings.