Feat: Add in-app logs screen (Phase 1) #398
Conversation
Implements Phase 1 of the logging system with complete UI without backend. Phase 2 will add the actual logger service integration. Changes: - Add LogsScreen with filtering, search, and toggle - Translations for EN, ES, IT - Settings integration with isLoggingEnabled field - Dev Tools section in settings screen - Route configuration for /logs Files (8): - lib/features/logs/screens/logs_screen.dart - lib/l10n/intl_en.arb - lib/l10n/intl_es.arb - lib/l10n/intl_it.arb - lib/features/settings/settings.dart - lib/features/settings/settings_notifier.dart - lib/core/app_routes.dart - lib/features/settings/settings_screen.dart
- Remove share button (will be in Phase 5) - Move Dev Tools section after Relays in settings - Toggle behavior already correct (persists via SharedPreferences) Changes: - lib/features/logs/screens/logs_screen.dart - lib/features/settings/settings_screen.dart
WalkthroughAdds a new LogsScreen and LogLevel enum, registers route Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (UI)
participant Router as Router
participant Logs as LogsScreen
participant Settings as SettingsNotifier
Note over User,Router: Navigate to Logs from Settings Dev Tools
User->>Router: tap DevTools -> push("/logs")
Router->>Logs: instantiate LogsScreen (watch provider)
Logs->>Settings: read isLoggingEnabled (watch)
Settings--)Logs: current isLoggingEnabled
alt Enable logging
User->>Logs: toggle ON
Logs->>User: show Performance Warning
User->>Logs: confirm Enable
Logs->>Settings: updateLoggingEnabled(true)
Settings--)Logs: new state propagated
else Disable logging
User->>Logs: toggle OFF
Logs->>Settings: updateLoggingEnabled(false)
Settings--)Logs: new state propagated
end
Note over Logs,User: Search, filter, and clear are local UI flows
User->>Logs: tap Clear Logs
Logs->>User: show Clear Confirmation
User->>Logs: confirm
Logs->>Logs: clear in-memory buffer
Logs->>User: show Snackbar "Logs cleared"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)lib/l10n/**/*.arb📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (4)📚 Learning: 2025-11-27T12:10:12.082ZApplied to files:
📚 Learning: 2025-11-27T12:10:12.082ZApplied to files:
📚 Learning: 2025-11-27T12:10:12.082ZApplied to files:
📚 Learning: 2025-11-27T12:10:26.407ZApplied to files:
⏰ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/features/logs/screens/logs_screen.dart:
- Around line 100-134: The SnackBar side effect in _showClearConfirmation should
be scheduled in a post-frame callback to avoid running after the widget is
disposed; replace the direct ScaffoldMessenger.of(context).showSnackBar(...)
call inside the confirmed == true && mounted block with
WidgetsBinding.instance.addPostFrameCallback((_) { if (mounted)
ScaffoldMessenger.of(context).showSnackBar(SnackBar(content:
Text(S.of(context)!.logsCleared))); }); so the SnackBar is shown only after the
current frame completes and with a fresh mounted check.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/LOGGING_IMPLEMENTATION.mdlib/core/app_routes.dartlib/features/logs/screens/logs_screen.dartlib/features/settings/settings.dartlib/features/settings/settings_notifier.dartlib/features/settings/settings_screen.dartlib/l10n/intl_en.arblib/l10n/intl_es.arblib/l10n/intl_it.arb
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/settings/settings_screen.dartlib/features/settings/settings_notifier.dartlib/features/logs/screens/logs_screen.dartlib/core/app_routes.dartlib/features/settings/settings.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/settings/settings_screen.dartlib/features/settings/settings_notifier.dartlib/features/logs/screens/logs_screen.dartlib/core/app_routes.dartlib/features/settings/settings.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/core/app_routes.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Use GoRouter for all navigation configuration instead of Navigator API - maintain all routes in app_routes.dart
Files:
lib/core/app_routes.dart
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
lib/features/settings/settings.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Use null-aware operators (
??) in SettingscopyWith()method to preserve existing values for selectedLanguage and defaultLightningAddress when not explicitly overridden
Files:
lib/features/settings/settings.dart
🧠 Learnings (11)
📚 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/features/settings/settings_notifier.dartlib/core/app_routes.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/settings/settings.dart : Use null-aware operators (`??`) in Settings `copyWith()` method to preserve existing values for selectedLanguage and defaultLightningAddress when not explicitly overridden
Applied to files:
lib/features/settings/settings_notifier.dartlib/features/settings/settings.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.dartlib/core/app_routes.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/core/app_routes.dart : Use GoRouter for all navigation configuration instead of Navigator API - maintain all routes in app_routes.dart
Applied to files:
lib/core/app_routes.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/core/app_routes.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/core/app_routes.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/core/app_routes.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/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-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:
docs/LOGGING_IMPLEMENTATION.md
📚 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:
docs/LOGGING_IMPLEMENTATION.mdlib/features/settings/settings.dart
📚 Learning: 2025-11-27T12:10:12.082Z
Learnt from: CR
Repo: MostroP2P/mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-27T12:10:12.082Z
Learning: Applies to lib/features/relays/relays_notifier.dart : Use `removeRelayWithBlacklist()` for Mostro/default relays (adds to blacklist) and `removeRelay()` for permanent user relay deletion
Applied to files:
lib/features/settings/settings.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 (17)
docs/LOGGING_IMPLEMENTATION.md (2)
7-47: Excellent phase-driven documentation structure.The implementation phases are clearly articulated and well-organized, providing a solid roadmap for the team. Phase 1 accurately reflects the current PR scope, and the subsequent phases offer clear guidance for future work. The description of Phase 2 as "Current" with specific test files (RelaysNotifier, SubscriptionManager) is particularly helpful for the next iteration.
48-180: Design approach and examples are practical and well-motivated.The singleton logger pattern rationale is clearly articulated, and the provided migration examples for both regular services and background isolates give implementers concrete guidance. The buffer management strategy and isolate integration approach are appropriately detailed.
lib/core/app_routes.dart (1)
27-27: LGTM! Route configuration follows established patterns.The import and route definition for the LogsScreen are consistent with existing routes. The use of
buildPageWithDefaultTransitionand placement within theShellRouteensure proper navigation behavior and widget wrapping.Also applies to: 288-296
lib/features/settings/settings_notifier.dart (1)
152-154: LGTM! Session-based state update is correctly implemented.The
updateLoggingEnabledmethod intentionally omits the_saveToPrefs()call, ensuring the logging state doesn't persist across sessions. This aligns with the PR's safety requirement that users must explicitly enable logging each session.lib/features/settings/settings.dart (3)
10-10: LGTM! Field properly declared with safe default.The
isLoggingEnabledfield is correctly declared as a non-nullableboolwith a default value offalsein the constructor, ensuring logging is disabled by default.Also applies to: 21-21
34-34: LGTM!copyWithcorrectly handles the new field.The
isLoggingEnabledparameter is properly added tocopyWithand uses the null-aware operator pattern (isLoggingEnabled ?? this.isLoggingEnabled) to preserve the existing value when not explicitly overridden.Based on learnings: "Use null-aware operators (
??) in SettingscopyWith()method to preserve existing values."Also applies to: 47-47
51-60: LGTM! Session-based logging state correctly implemented.The intentional omission of
isLoggingEnabledfromtoJson()(Lines 51-60) and the hard-codedfalseinitialization infromJson()(Line 73) ensure that logging state never persists across app sessions. This design enforces the safety requirement that users must explicitly enable logging in each session.Based on PR objectives: "the toggle must be re-enabled each session for safety."
Also applies to: 73-73
lib/features/settings/settings_screen.dart (1)
119-216: No issues with.withValues()API usage. The project specifies Flutter >=3.27.0 in pubspec.yaml, which fully supports theColor.withValues()method used throughout this implementation.Likely an incorrect or invalid review comment.
lib/l10n/intl_it.arb (1)
1269-1324: Localization keys are consistent across all three ARB files.The logging UI localization keys are properly replicated across
lib/l10n/intl_en.arb,lib/l10n/intl_es.arb, andlib/l10n/intl_it.arbwith matching sets and correct placeholder definitions.lib/features/logs/screens/logs_screen.dart (6)
8-15: LGTM! Clean enum and widget declaration.The LogLevel enum and LogsScreen class follow Flutter best practices with proper const constructor usage.
17-53: LGTM! Proper resource management.Controllers are correctly initialized and disposed, preventing memory leaks. The scroll listener is appropriately attached in initState.
136-194: LGTM! Well-structured build method.The build method properly structures the UI with stats header, search, filters, and empty state. The hardcoded empty logs list (line 140) is intentional for Phase 1 per the PR objectives.
196-274: LGTM! Comprehensive stats header.The stats header provides clear feedback on logging state with the toggle, capture status, and entry counts. Proper use of localization and theme constants throughout.
276-352: LGTM! Excellent search and filter UX.The search bar with clear functionality and filter chips provide intuitive log filtering. The UI properly updates state on user interaction.
354-388: LGTM! Clear empty state messaging.The empty state properly informs users that logs will appear when capture is enabled, setting correct expectations for Phase 1.
lib/l10n/intl_es.arb (1)
1212-1270: LGTM! Complete Spanish translations for logging UI.All new logging UI keys are properly translated with correct placeholder metadata. The translations maintain consistency with existing Spanish strings in the file.
lib/l10n/intl_en.arb (1)
1236-1294: LGTM! Complete English translations for logging UI.All new logging UI keys are properly defined with correct placeholder metadata. The strings are clear, concise, and professional. Placeholder types (int for counts, String for path) are correctly specified.
Addresses an issue where the snackbar indicating successful log clearing might not display correctly due to the widget tree potentially being rebuilt before the snackbar is shown. Wraps the snackbar display logic in `WidgetsBinding.instance.addPostFrameCallback` to ensure the widget is properly mounted before displaying the message.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @lib/l10n/intl_en.arb:
- Around line 1240-1242: Add the missing trailing comma after the
"invalidKeyFormat" entry to fix the JSON syntax error in the ARB entries around
"@_comment_logging_ui" and "logCapture"—ensure the line containing
"invalidKeyFormat": "Invalid key format" ends with a comma so the subsequent
"@_comment_logging_ui" and "logCapture" keys parse correctly.
In @lib/l10n/intl_es.arb:
- Around line 1215-1217: The ARB file has a JSON syntax error: the entry
"invalidKeyFormat" is missing a trailing comma which breaks parsing; add a comma
after the value for the "invalidKeyFormat" key so the following metadata entry
"@_comment_logging_ui" is a valid separate property, then re-run the
linter/parser to confirm the ARB is valid.
In @lib/l10n/intl_it.arb:
- Around line 1270-1272: The ARB JSON is invalid because the entry
"invalidKeyFormat": "Formato chiave non valido" (key invalidKeyFormat) is
missing a trailing comma; fix by adding a comma after that value so the
following metadata entry ("@_comment_logging_ui") is properly separated,
ensuring the file parses correctly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/features/settings/settings_screen.dartlib/l10n/intl_en.arblib/l10n/intl_es.arblib/l10n/intl_it.arb
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/settings/settings_screen.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 (1)
📚 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
🪛 GitHub Actions: Flutter
lib/l10n/intl_en.arb
[error] 1241-1241: Flutter ARB formatting error: Unexpected character at line 1241, column 3 in intl_en.arb. The line shown is "@_comment_logging_ui": "Logging UI strings".
🪛 RuboCop (1.82.1)
lib/l10n/intl_it.arb
[fatal] 1272-1272: unexpected token tSTRING
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
lib/l10n/intl_es.arb
[fatal] 1217-1217: unexpected token tSTRING
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
lib/l10n/intl_en.arb
[fatal] 1241-1241: unexpected token tSTRING
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
🔇 Additional comments (3)
lib/l10n/intl_it.arb (1)
1272-1327: LGTM after fixing the syntax error!The logging UI translations are comprehensive and correctly structured. All required keys are present and properly localized to Italian, with appropriate placeholder metadata for dynamic values (count, path).
Based on coding guidelines, this correctly adds new localization keys to all three ARB files (en, es, it).
lib/l10n/intl_es.arb (1)
1217-1272: LGTM after fixing the syntax error!The Spanish logging UI translations are complete and properly structured. All required keys match the English and Italian versions, maintaining consistency across locales as per coding guidelines.
lib/l10n/intl_en.arb (1)
1242-1296: LGTM after fixing the syntax error!The English logging UI translations are comprehensive and well-structured. They cover all Phase 1 requirements mentioned in the PR objectives:
- Log capture toggle with performance warning
- Level filtering (All, Errors, Warnings, Info, Debug)
- Search functionality
- Empty state messaging
- Export actions (copy, save, share)
- Dev Tools section integration
The placeholder metadata for
totalLogs,maxEntries, andlogsSavedTois correctly defined with appropriate types.Based on coding guidelines, all three ARB files (en, es, it) correctly include the same localization keys.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
@BraCR10
Estaba viendo la UI y quizá se pudiera simplificar un poco: como dentro de "Herramientas de desarrollo" solo está Reporte de registros, tal vez se vea mejor dejar ese ítem directamente en Settings sin esa sección intermedia. Así queda más compacto visualmente y se evita un título en plural si hay 1 sola opción.
De todas formas, en general para mí está bien ya como está, espera a ver qué les parece a los demás antes de cambiar algo
Ok, esperemos a ver qué opinan los demás. @grunch @AndreaDiazCorreia |
Si se van a poner otras herramientas está bien, pero ahora no se me ocurre ninguna otra para el futuro. De todas formas esa UI no está mal, cualquiera de las 2 opciones de dejarlo o no, me parece bien |
|
@BraCR10 vamos a hacerle merge al pr y luego revisamos con calma lo de la ui de la seccion, par aque ahora puedas continuar con el siguiente pr |
2 participants
Phase 1: Core Logging UI
This PR implements the UI controls and multi-language support for the logging system. It does not record logs yet and only includes UI components.
What's Included
Key Behaviors
Testing
Navigate to: Settings → Relays → Dev Tools → Logs Report
Next Phase
Phase 2 will connect the actual logger service with memory buffer and integrate 2 test files.
See
docs/LOGGING_IMPLEMENTATION.mdfor complete architecture details.Summary by CodeRabbit
New Features
Localization
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.