feat(phase16): settings & preferences — settings API, screens ... - #67
Merged
Conversation
…ge/currency/relay/node/notification/log/about UI - rust/src/api/settings.rs: SettingsStore with RwLock<AppSettings>, broadcast stream, validated setters for theme/language/fiat/lightning/logging; 10 tests - lib/features/settings/screens/settings_screen.dart: 8-card settings screen wired to localeProvider + settingsProvider - lib/features/settings/widgets/: language_selector, currency_selector_dialog, relay_management_card, mostro_node_selector - lib/features/settings/screens/notification_settings_screen.dart: per-category push notification toggles - lib/features/settings/screens/log_report_screen.dart: scrollable log viewer with share export - lib/features/about/screens/about_screen.dart: app version, daemon pubkey/relay info - lib/core/app.dart: locale + themeMode driven by providers - lib/core/app_routes.dart: stub routes replaced with real screens - Fix: databaseFactoryWeb → databaseFactoryMemory (sembast_web not in pubspec); withOpacity → withValues
…ging fallback, accessibility, relay bug, error handling - rust/src/api/settings.rs: enforce ISO 4217 exactly 3 chars in validate_fiat_code; add settings_lock() to all_supported_locales_accepted test; set_logging_enabled falls back to sync write when no Tokio runtime - lib/features/settings/providers/settings_provider.dart: derive localeProvider from settingsProvider (Provider, not StateProvider) - lib/features/settings/widgets/language_selector.dart: remove manual localeProvider update (now auto-derived); fix doc comment native vs English - lib/features/settings/widgets/relay_management_card.dart: capture URL before removeAt to fix use-after-removal bug; split wss/short-URL error messages; assert AppColors - lib/features/settings/widgets/currency_selector_dialog.dart: log asset loading errors; assert AppColors - lib/features/settings/screens/notification_settings_screen.dart: assert AppColors - lib/features/settings/screens/log_report_screen.dart: await share() and catch errors - lib/features/about/screens/about_screen.dart: assert AppColors; wrap pubkey in Semantics+InkWell for accessibility
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a full settings subsystem: Flutter state/providers, multiple settings screens/widgets, app wiring to drive theme and locale from settings, shared defaults, and a Rust in-memory settings API with broadcasts and validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LanguageSelector
participant SettingsNotifier
participant localeProvider
participant MaterialApp
User->>LanguageSelector: select language code
LanguageSelector->>SettingsNotifier: setLanguage(code)
SettingsNotifier->>SettingsNotifier: update AppSettingsState.language
SettingsNotifier->>localeProvider: provider recomputes
localeProvider->>MaterialApp: emit new Locale
MaterialApp->>MaterialApp: rebuild UI with new locale/theme
MaterialApp->>User: UI updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
lib/features/settings/widgets/language_selector.dart (1)
29-30: Inconsistent null check pattern forAppColors.Other screens in this PR use an explicit
assertbefore the bang operator (e.g.,about_screen.dartlines 46-48). Consider aligning for consistency and better debug-mode error messages.♻️ Suggested fix
- final colors = Theme.of(context).extension<AppColors>()!; + final colorsRaw = Theme.of(context).extension<AppColors>(); + assert(colorsRaw != null, 'AppColors theme extension must be registered'); + final colors = colorsRaw!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/widgets/language_selector.dart` around lines 29 - 30, The null-check pattern for AppColors is inconsistent: instead of directly using the bang on Theme.of(context).extension<AppColors>() in language_selector.dart, add an explicit assert that Theme.of(context).extension<AppColors>() != null before assigning to colors so debug builds produce a clear failure; update the colors assignment (used alongside currentCode and settingsProvider) to rely on that assert and then safely cast/unwrap the extension.lib/features/about/screens/about_screen.dart (1)
6-12: Duplicated default pubkey constant.
_defaultPubkeyduplicates the value defined inmostro_node_selector.dart(_defaultMostroPubkey). Consider extracting these to a shared constant (e.g., inlib/core/config.dartor importing fromrust/src/config.rsvia the bridge) to avoid drift.♻️ Suggested approach
// lib/core/config.dart const defaultMostroPubkey = '82fa8cb978b43c79b2156585bac2c011176a21d2aead6d9f7c575c005be88390'; const defaultRelays = [ 'wss://relay.mostro.network', 'wss://nos.lol', ];Then import in both
about_screen.dartandmostro_node_selector.dart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/about/screens/about_screen.dart` around lines 6 - 12, The file defines duplicated defaults (_defaultPubkey and _defaultRelays) that duplicate _defaultMostroPubkey in mostro_node_selector.dart; extract these into a single shared constant set (e.g., create defaultMostroPubkey and defaultRelays in a new core config module) and replace the local _defaultPubkey/_defaultRelays and _defaultMostroPubkey usages to import and use the shared constants, removing the duplicate definitions and updating imports in both about_screen.dart and mostro_node_selector.dart.lib/features/settings/screens/log_report_screen.dart (2)
260-265: Consider including the date for logs spanning multiple days.The current format
HH:MM:SSworks well for same-day logs but could be confusing when viewing historical logs from different days. Consider adding date information when the log's date differs from today.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/screens/log_report_screen.dart` around lines 260 - 265, The _formatTimestamp function currently returns only time (HH:MM:SS) which is ambiguous across days; update _formatTimestamp(int unixSeconds) to compute the DateTime dt and compare its date portion to DateTime.now(), and when dt is not the same day include a date prefix (e.g. YYYY-MM-DD or localized date) before the time; keep the existing zero-padded time format for same-day logs to preserve current output.
42-64: Acknowledge the TODO for replacing mock data.The mock entries are clearly marked with a TODO comment for future bridge integration. This is acceptable for the current implementation phase.
Do you want me to open an issue to track the implementation of the real log stream from the Rust bridge?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/screens/log_report_screen.dart` around lines 42 - 64, The file currently uses static mock data in _mockEntries (instances of _LogEntry) marked by a TODO to be replaced by the Rust bridge log stream; keep the TODO but create and link a tracker issue to implement consuming the real log stream from the Rust bridge and update the code to replace _mockEntries with the live stream source (consume whatever method/event added to the bridge API), then update the TODO comment to reference that issue ID so future work is discoverable.lib/features/settings/widgets/relay_management_card.dart (1)
34-49: Relay state is managed locally instead of via a Riverpod provider.The relay list is stored in widget-local state and lost on navigation/rebuild. Consider creating a
relaysProviderto persist relay configuration across the app lifecycle, especially since the Rust bridge already hasadd_relay(),remove_relay(), andget_relays()methods available (perrust/src/api/nostr.rs).The hardcoded default relays match
rust/src/config.rswhich is good for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/widgets/relay_management_card.dart` around lines 34 - 49, The relay list is held in widget-local state (_relays in _RelayManagementCardState initialized in initState) so it gets lost on navigation; refactor by creating a Riverpod provider (e.g., relaysProvider) that stores List<_RelayEntry> (or a serializable model) and exposes methods to load/get relays from the Rust bridge (call get_relays()), add (add_relay()), and remove (remove_relay()) relays; update _RelayManagementCardState to read/watch relaysProvider instead of using its local _relays and dispatch add/remove actions through the provider so relay configuration persists across the app lifecycle and stays in sync with the Rust backend.rust/src/api/settings.rs (1)
69-81: Document or centralize the locale list coupling between Rust and Flutter.The
SUPPORTED_LOCALESconstant in Rust duplicates the locale list from Flutter'sAppLocalizations.supportedLocales. Both currently list the same locales (de, en, es, fr, it), but there is no mechanism to detect or prevent drift if new locales are added to one codebase without updating the other. The architecture document confirms the Rust Core + Flutter Shell separation, but provides no guidance for maintaining this particular synchronization.Consider adding a comment documenting the coupling, linking to Flutter's locale definitions, or introducing a shared configuration source if feasible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/settings.rs` around lines 69 - 81, The SUPPORTED_LOCALES constant and validate_locale function duplicate the locale list maintained in Flutter (AppLocalizations.supportedLocales) which risks drift; either add a clear doc comment above SUPPORTED_LOCALES referencing the Flutter source/location and the architectural coupling, or refactor to read the locale list from a shared configuration (e.g., env var, JSON file, or generated Rust module) so both runtimes use the same source of truth; update the comment on SUPPORTED_LOCALES and the validate_locale function to mention the shared source or link to AppLocalizations.supportedLocales to prevent silent divergence.lib/features/notifications/providers/notifications_provider.dart (1)
32-34: Consider tracking the web persistence gap as an issue.The in-memory fallback means
notificationsProviderWithDbloses all data on page reload for web users—effectively degrading to the same behavior as the legacynotificationsProvider. The TODO is helpful, but this user-visible regression should be tracked to avoid being forgotten.Would you like me to open an issue to track adding
sembast_webtopubspec.yamland restoring IndexedDB-backed persistence on web?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/notifications/providers/notifications_provider.dart` around lines 32 - 34, The new web fallback uses databaseFactoryMemory (openDatabase(_dbName)) which loses data on reload and regresses notificationsProviderWithDb to legacy behavior; create a tracked issue to add sembast_web to pubspec.yaml and restore IndexedDB persistence, link the issue to the TODO(comment) in notifications_provider.dart, include reproduction steps (web reload losing notifications), the desired fix (replace databaseFactoryMemory with databaseFactoryWeb and ensure sembast_web is added), and tag the relevant symbols: notificationsProviderWithDb, _dbName, and databaseFactoryMemory so the work can be scheduled and assigned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/notifications/widgets/notification_card.dart`:
- Line 267: The code uses Color.withValues (bg.withValues(alpha: 0.2)) which
requires Flutter 3.27+; either update your SDK constraint in pubspec.yaml to
">=3.27.0 <4.0.0" or change the call to bg.withOpacity(0.2) for backward
compatibility; locate the usage in the NotificationCard widget (the color:
property where bg.withValues is used) and apply one of these two fixes so the
project compiles on the currently targeted SDK range.
In `@lib/features/settings/widgets/mostro_node_selector.dart`:
- Around line 6-15: mostroPubkeyProvider currently only affects UI and is never
sent to the backend; fix by plumbing its value into the Rust bridge calls that
send Nostr/Mostro orders (or explicitly document it as a placeholder). Locate
where orders/messages are constructed/sent (the module that calls the Rust
bridge / FFI functions) and read the provider value (mostroPubkeyProvider)
there—e.g., ref.read(mostroPubkeyProvider) when building the outgoing
payload—and pass that pubkey into the bridge function so routing uses the
selected node; alternatively add a clear comment in
settings_screen.dart/mostro_node_selector.dart stating it is a UI-only
placeholder until the bridge integration is implemented.
In `@rust/src/api/settings.rs`:
- Around line 165-181: The no-runtime branch of set_logging_enabled currently
swallows failures from store().settings.try_write(), silently dropping the
logging_enabled update; change this to surface the failure by either returning a
Result from set_logging_enabled or at minimum logging a warning when try_write()
returns Err so callers/operators can detect contention—locate the
set_logging_enabled function and update the Err path to call your logger (e.g.,
process_logger or a crate logger) with context including that write failed and
the desired enabled value, or change the function signature to return Result<(),
LockError> and map the try_write() outcome into that Result.
---
Nitpick comments:
In `@lib/features/about/screens/about_screen.dart`:
- Around line 6-12: The file defines duplicated defaults (_defaultPubkey and
_defaultRelays) that duplicate _defaultMostroPubkey in
mostro_node_selector.dart; extract these into a single shared constant set
(e.g., create defaultMostroPubkey and defaultRelays in a new core config module)
and replace the local _defaultPubkey/_defaultRelays and _defaultMostroPubkey
usages to import and use the shared constants, removing the duplicate
definitions and updating imports in both about_screen.dart and
mostro_node_selector.dart.
In `@lib/features/notifications/providers/notifications_provider.dart`:
- Around line 32-34: The new web fallback uses databaseFactoryMemory
(openDatabase(_dbName)) which loses data on reload and regresses
notificationsProviderWithDb to legacy behavior; create a tracked issue to add
sembast_web to pubspec.yaml and restore IndexedDB persistence, link the issue to
the TODO(comment) in notifications_provider.dart, include reproduction steps
(web reload losing notifications), the desired fix (replace
databaseFactoryMemory with databaseFactoryWeb and ensure sembast_web is added),
and tag the relevant symbols: notificationsProviderWithDb, _dbName, and
databaseFactoryMemory so the work can be scheduled and assigned.
In `@lib/features/settings/screens/log_report_screen.dart`:
- Around line 260-265: The _formatTimestamp function currently returns only time
(HH:MM:SS) which is ambiguous across days; update _formatTimestamp(int
unixSeconds) to compute the DateTime dt and compare its date portion to
DateTime.now(), and when dt is not the same day include a date prefix (e.g.
YYYY-MM-DD or localized date) before the time; keep the existing zero-padded
time format for same-day logs to preserve current output.
- Around line 42-64: The file currently uses static mock data in _mockEntries
(instances of _LogEntry) marked by a TODO to be replaced by the Rust bridge log
stream; keep the TODO but create and link a tracker issue to implement consuming
the real log stream from the Rust bridge and update the code to replace
_mockEntries with the live stream source (consume whatever method/event added to
the bridge API), then update the TODO comment to reference that issue ID so
future work is discoverable.
In `@lib/features/settings/widgets/language_selector.dart`:
- Around line 29-30: The null-check pattern for AppColors is inconsistent:
instead of directly using the bang on Theme.of(context).extension<AppColors>()
in language_selector.dart, add an explicit assert that
Theme.of(context).extension<AppColors>() != null before assigning to colors so
debug builds produce a clear failure; update the colors assignment (used
alongside currentCode and settingsProvider) to rely on that assert and then
safely cast/unwrap the extension.
In `@lib/features/settings/widgets/relay_management_card.dart`:
- Around line 34-49: The relay list is held in widget-local state (_relays in
_RelayManagementCardState initialized in initState) so it gets lost on
navigation; refactor by creating a Riverpod provider (e.g., relaysProvider) that
stores List<_RelayEntry> (or a serializable model) and exposes methods to
load/get relays from the Rust bridge (call get_relays()), add (add_relay()), and
remove (remove_relay()) relays; update _RelayManagementCardState to read/watch
relaysProvider instead of using its local _relays and dispatch add/remove
actions through the provider so relay configuration persists across the app
lifecycle and stays in sync with the Rust backend.
In `@rust/src/api/settings.rs`:
- Around line 69-81: The SUPPORTED_LOCALES constant and validate_locale function
duplicate the locale list maintained in Flutter
(AppLocalizations.supportedLocales) which risks drift; either add a clear doc
comment above SUPPORTED_LOCALES referencing the Flutter source/location and the
architectural coupling, or refactor to read the locale list from a shared
configuration (e.g., env var, JSON file, or generated Rust module) so both
runtimes use the same source of truth; update the comment on SUPPORTED_LOCALES
and the validate_locale function to mention the shared source or link to
AppLocalizations.supportedLocales to prevent silent divergence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ddfdea0-889c-49db-b03a-d3cc35cf1a26
📒 Files selected for processing (17)
lib/core/app.dartlib/core/app_routes.dartlib/features/about/screens/about_screen.dartlib/features/notifications/providers/notifications_provider.dartlib/features/notifications/widgets/notification_card.dartlib/features/settings/providers/settings_provider.dartlib/features/settings/screens/log_report_screen.dartlib/features/settings/screens/notification_settings_screen.dartlib/features/settings/screens/settings_screen.dartlib/features/settings/widgets/currency_selector_dialog.dartlib/features/settings/widgets/language_selector.dartlib/features/settings/widgets/mostro_node_selector.dartlib/features/settings/widgets/relay_management_card.dartrust/src/api/mod.rsrust/src/api/settings.rsrust/src/api/types.rsspecs/004-mostro-p2p-client/tasks.md
… coupling, logging fallback warning, timestamp date prefix - pubspec.yaml: raise SDK floor to >=3.7.0 so Color.withValues() compiles cleanly - lib/core/mostro_defaults.dart: extract defaultMostroPubkey + defaultMostroRelays shared constants - about_screen.dart, mostro_node_selector.dart, relay_management_card.dart: import and use shared defaults instead of duplicated literals - mostro_node_selector.dart: document mostroPubkeyProvider as UI-only placeholder until bridge integration - relay_management_card.dart: document local state limitation, link to future bridge refactor - language_selector.dart: assert AppColors before unwrap for consistency - log_report_screen.dart: _formatTimestamp includes YYYY-MM-DD prefix for non-today entries; improve bridge TODO comments - notifications_provider.dart: expand TODO explaining web persistence regression - rust/src/api/settings.rs: SUPPORTED_LOCALES doc references Flutter coupling; set_logging_enabled logs warning on lock contention
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rust/src/api/settings.rs (1)
166-170:⚠️ Potential issue | 🟠 Major
set_logging_enabledguarantee is currently false under contention.Line 169 says the flag is “always set”, but Line 184 explicitly drops the update when
try_write()fails. This can leave logging state stale in sync/no-runtime contexts.🔧 Suggested fix
- Err(_) => { - // No async runtime — update the flag synchronously. - // Notification is intentionally skipped here (best-effort). - match store().settings.try_write() { - Ok(mut guard) => guard.logging_enabled = enabled, - Err(_) => eprintln!( - "[settings] set_logging_enabled({enabled}): lock contention, update dropped" - ), - } - } + Err(_) => { + // No async runtime — update the flag synchronously. + // Use blocking_write so this path preserves the "always set" contract. + let mut guard = store().settings.blocking_write(); + guard.logging_enabled = enabled; + // Notification intentionally skipped in no-runtime path. + }#!/bin/bash set -euo pipefail # Verify current behavior and tokio configuration needed for blocking_write. fd -i Cargo.toml --exec rg -n --type toml 'tokio|features' rg -n -A20 -B8 'pub fn set_logging_enabled|try_write|blocking_write' rust/src/api/settings.rsExpected result: confirm
try_write()is currently used in the no-runtime branch and verify Tokio features/version support forRwLock::blocking_write().Also applies to: 181-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/settings.rs` around lines 166 - 170, set_logging_enabled currently can drop the update when try_write() fails in the "no-runtime" path; change the synchronous branch to perform a guaranteed blocking write so the logging flag is always updated: replace the try_write() usage with RwLock::blocking_write() (or, if your tokio version lacks blocking_write, implement a short blocking loop that obtains the write lock (e.g., std::thread::yield_now/sleep between attempts) until successful). Update the call sites in set_logging_enabled to use blocking_write() on the same RwLock used now and ensure the crate's tokio features/version support blocking_write (or add the minimal polling fallback) so the flag cannot be dropped under contention.
🧹 Nitpick comments (6)
lib/features/settings/widgets/mostro_node_selector.dart (3)
109-112: Add semantic label for accessibility.The close button should have a
tooltiporsemanticLabelso screen readers can announce its purpose.♿ Suggested fix
IconButton( - icon: const Icon(Icons.close), + icon: const Icon(Icons.close), + tooltip: 'Close', onPressed: () => Navigator.of(context).pop(), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/widgets/mostro_node_selector.dart` around lines 109 - 112, The IconButton used to close the dialog (the IconButton with icon: const Icon(Icons.close) and onPressed: () => Navigator.of(context).pop()) lacks accessibility metadata; update this widget (in MostroNodeSelector) to provide a semantic label or tooltip (for example add a tooltip: 'Close' or wrap with a Semantics/Tooltip widget and set semanticLabel: 'Close dialog') so screen readers and hover tooltips announce its purpose while preserving the existing onPressed behavior.
71-83: Consider normalizing pubkey to lowercase.Nostr pubkeys are conventionally lowercase hex. The regex accepts both cases, but storing user input as-is could cause comparison mismatches if other parts of the codebase or the Rust backend expect lowercase.
🔧 Suggested fix
void _confirm() { final input = _controller.text.trim(); if (input.isEmpty) { _useDefault(); return; } if (!_hexRegex.hasMatch(input)) { setState(() => _errorText = 'Must be a 64-character hex string'); return; } - ref.read(mostroPubkeyProvider.notifier).state = input; + ref.read(mostroPubkeyProvider.notifier).state = input.toLowerCase(); Navigator.of(context).pop(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/widgets/mostro_node_selector.dart` around lines 71 - 83, Summary: Normalize the entered pubkey to lowercase before storing to avoid case-mismatch downstream. In _confirm(), after trimming and validating the input with _hexRegex, convert the input to lowercase (e.g., final normalized = input.toLowerCase()) and assign that to ref.read(mostroPubkeyProvider.notifier).state instead of the raw input; keep validation against _hexRegex as-is and then call Navigator.of(context).pop() as before.
175-186: Consider disabling autocorrect for hex input.Since this field expects a hex string, disabling autocorrect and suggestions would improve UX.
⌨️ Suggested fix
TextField( controller: _controller, maxLength: 64, + autocorrect: false, + enableSuggestions: false, decoration: InputDecoration( hintText: 'Enter 64-char hex pubkey', errorText: _errorText, counterText: '', ), onChanged: (_) { if (_errorText != null) setState(() => _errorText = null); }, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/widgets/mostro_node_selector.dart` around lines 175 - 186, The TextField for hex pubkey (the widget using _controller and showing _errorText) should disable autocorrect and keyboard suggestions to avoid unwanted substitutions; update the TextField properties by setting autocorrect: false and enableSuggestions: false (optionally also ensure keyboardType is a non-autocorrect type such as TextInputType.text or TextInputType.visiblePassword) so the hex input UX is improved without changing validation logic.lib/features/settings/screens/log_report_screen.dart (2)
73-73: Consider defensive null handling for theme extension.The
!operator assumesAppColorsextension is always registered. If the theme configuration changes or this screen is accessed in an unusual context, this could throw.♻️ Suggested defensive approach
- final colors = Theme.of(context).extension<AppColors>()!; + final colors = Theme.of(context).extension<AppColors>() ?? AppColors.dark;This requires
AppColors.darkto be a static const or factory, which may already exist in your theme setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/screens/log_report_screen.dart` at line 73, The code uses Theme.of(context).extension<AppColors>()! which can throw if the AppColors extension is not registered; update the LogReportScreen to defensively handle a missing extension by replacing the forced unwrap with a null-aware fallback (e.g., var colors = Theme.of(context).extension<AppColors>() ?? AppColors.dark) so the screen uses a safe default; ensure AppColors.dark (or another static default on AppColors) exists and reference the exact symbol Theme.of(context).extension<AppColors>() and AppColors.dark when making the change.
173-177: Consider user feedback on share failure.The
debugPrintlogs the error but gives no user indication that sharing failed. A brief SnackBar could improve UX.♻️ Optional: Show SnackBar on failure
try { await SharePlus.instance.share(ShareParams(text: content)); } catch (e) { debugPrint('Failed to share logs: $e'); + if (mounted) { + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar(content: Text('Failed to share logs')), + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/screens/log_report_screen.dart` around lines 173 - 177, The catch on SharePlus.instance.share(ShareParams(...)) only debugPrints the error; update the catch block in log_report_screen.dart (around the share call) to also show a user-facing SnackBar via ScaffoldMessenger.of(context).showSnackBar(...) with a short failure message (e.g., "Failed to share logs"), optionally including minimal error info, and keep the debugPrint for diagnostics; ensure you use the current BuildContext (or check mounted if inside a stateful widget) so the SnackBar call is safe.lib/features/settings/widgets/relay_management_card.dart (1)
174-178: Consider adding accessibility support for the Switch.The
Switchwidget lacks a semantic label, making it unclear to screen reader users what the toggle controls. Consider wrapping withSemanticsor usingSwitch.adaptivewith a label.Proposed improvement
- Switch( - value: relay.isActive, - onChanged: (v) => _toggleRelay(index, v), - activeThumbColor: c.mostroGreen, - ), + Semantics( + label: 'Toggle ${relay.url} relay', + child: Switch( + value: relay.isActive, + onChanged: (v) => _toggleRelay(index, v), + activeThumbColor: c.mostroGreen, + ), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/widgets/relay_management_card.dart` around lines 174 - 178, The Switch control (value: relay.isActive, onChanged: _toggleRelay) needs an accessibility label for screen readers; wrap the Switch in a Semantics widget (or replace with a labeled alternative) and provide a clear label such as "Enable relay {relay.name or relay.url}" via the Semantics(label: ...) or by using a labeled Switch.adaptive, ensuring the same onChanged callback (_toggleRelay) and activeThumbColor are preserved so behavior doesn't change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/settings/widgets/relay_management_card.dart`:
- Around line 111-119: The async dialog callback calls setState to add a new
_RelayEntry to _relays but can run after the widget is disposed; update the
callback to check the State's mounted property before calling setState (e.g.,
return early if !mounted) so that _relays.add and setState are only invoked
while the widget is still mounted; ensure this check is added in the closure
where _RelayEntry(url: ..., isActive: true, isDefault: false) is appended.
In `@rust/src/api/settings.rs`:
- Around line 102-105: The validate_lightning_address function currently uses
splitn(2, '@') which allows extra '@' characters in the second part (e.g.,
user@domain@extra); update the validation to require exactly one '@' by either
counting occurrences of '@' (e.g., address.matches('@').count() == 1) or by
using split and ensuring the resulting Vec has length == 2 and that neither part
contains an additional '@' (e.g., parts.len() == 2 && !parts[0].is_empty() &&
!parts[1].is_empty() && !parts[1].contains('@')). Apply this change inside
validate_lightning_address so only addresses with a single '@' and non-empty
user/domain pass.
---
Duplicate comments:
In `@rust/src/api/settings.rs`:
- Around line 166-170: set_logging_enabled currently can drop the update when
try_write() fails in the "no-runtime" path; change the synchronous branch to
perform a guaranteed blocking write so the logging flag is always updated:
replace the try_write() usage with RwLock::blocking_write() (or, if your tokio
version lacks blocking_write, implement a short blocking loop that obtains the
write lock (e.g., std::thread::yield_now/sleep between attempts) until
successful). Update the call sites in set_logging_enabled to use
blocking_write() on the same RwLock used now and ensure the crate's tokio
features/version support blocking_write (or add the minimal polling fallback) so
the flag cannot be dropped under contention.
---
Nitpick comments:
In `@lib/features/settings/screens/log_report_screen.dart`:
- Line 73: The code uses Theme.of(context).extension<AppColors>()! which can
throw if the AppColors extension is not registered; update the LogReportScreen
to defensively handle a missing extension by replacing the forced unwrap with a
null-aware fallback (e.g., var colors = Theme.of(context).extension<AppColors>()
?? AppColors.dark) so the screen uses a safe default; ensure AppColors.dark (or
another static default on AppColors) exists and reference the exact symbol
Theme.of(context).extension<AppColors>() and AppColors.dark when making the
change.
- Around line 173-177: The catch on SharePlus.instance.share(ShareParams(...))
only debugPrints the error; update the catch block in log_report_screen.dart
(around the share call) to also show a user-facing SnackBar via
ScaffoldMessenger.of(context).showSnackBar(...) with a short failure message
(e.g., "Failed to share logs"), optionally including minimal error info, and
keep the debugPrint for diagnostics; ensure you use the current BuildContext (or
check mounted if inside a stateful widget) so the SnackBar call is safe.
In `@lib/features/settings/widgets/mostro_node_selector.dart`:
- Around line 109-112: The IconButton used to close the dialog (the IconButton
with icon: const Icon(Icons.close) and onPressed: () =>
Navigator.of(context).pop()) lacks accessibility metadata; update this widget
(in MostroNodeSelector) to provide a semantic label or tooltip (for example add
a tooltip: 'Close' or wrap with a Semantics/Tooltip widget and set
semanticLabel: 'Close dialog') so screen readers and hover tooltips announce its
purpose while preserving the existing onPressed behavior.
- Around line 71-83: Summary: Normalize the entered pubkey to lowercase before
storing to avoid case-mismatch downstream. In _confirm(), after trimming and
validating the input with _hexRegex, convert the input to lowercase (e.g., final
normalized = input.toLowerCase()) and assign that to
ref.read(mostroPubkeyProvider.notifier).state instead of the raw input; keep
validation against _hexRegex as-is and then call Navigator.of(context).pop() as
before.
- Around line 175-186: The TextField for hex pubkey (the widget using
_controller and showing _errorText) should disable autocorrect and keyboard
suggestions to avoid unwanted substitutions; update the TextField properties by
setting autocorrect: false and enableSuggestions: false (optionally also ensure
keyboardType is a non-autocorrect type such as TextInputType.text or
TextInputType.visiblePassword) so the hex input UX is improved without changing
validation logic.
In `@lib/features/settings/widgets/relay_management_card.dart`:
- Around line 174-178: The Switch control (value: relay.isActive, onChanged:
_toggleRelay) needs an accessibility label for screen readers; wrap the Switch
in a Semantics widget (or replace with a labeled alternative) and provide a
clear label such as "Enable relay {relay.name or relay.url}" via the
Semantics(label: ...) or by using a labeled Switch.adaptive, ensuring the same
onChanged callback (_toggleRelay) and activeThumbColor are preserved so behavior
doesn't change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcb10ed1-32de-43a7-9a1d-d9889904551b
📒 Files selected for processing (9)
lib/core/mostro_defaults.dartlib/features/about/screens/about_screen.dartlib/features/notifications/providers/notifications_provider.dartlib/features/settings/screens/log_report_screen.dartlib/features/settings/widgets/language_selector.dartlib/features/settings/widgets/mostro_node_selector.dartlib/features/settings/widgets/relay_management_card.dartpubspec.yamlrust/src/api/settings.rs
✅ Files skipped from review due to trivial changes (2)
- pubspec.yaml
- lib/core/mostro_defaults.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/features/notifications/providers/notifications_provider.dart
- lib/features/settings/widgets/language_selector.dart
- lib/features/about/screens/about_screen.dart
… lowercase, a11y labels, autocorrect off, share SnackBar
- relay_management_card.dart: check mounted before setState in async dialog callback; wrap Switch in Semantics with relay URL label
- mostro_node_selector.dart: normalize stored pubkey to lowercase; add tooltip to close button; disable autocorrect/suggestions on hex TextField
- log_report_screen.dart: assert AppColors; show SnackBar on share failure alongside debugPrint
- rust/src/api/settings.rs: validate_lightning_address uses split('@') instead of splitn(2,'@') to reject multiple '@' characters
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/features/settings/widgets/mostro_node_selector.dart (1)
130-142: Consider adding semantic label for screen reader accessibility.The truncated pubkey display may be unclear to screen reader users. Wrapping with
Semanticswould provide context.♿ Proposed accessibility improvement
const SizedBox(height: AppSpacing.xs), - Text( - truncatePubkey(currentPubkey), - style: Theme.of(context).textTheme.bodyMedium?.copyWith( - fontFamily: 'monospace', - ), + Semantics( + label: 'Current Mostro node pubkey: $currentPubkey', + child: Text( + truncatePubkey(currentPubkey), + style: Theme.of(context).textTheme.bodyMedium?.copyWith( + fontFamily: 'monospace', + ), + ), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/widgets/mostro_node_selector.dart` around lines 130 - 142, Wrap the Text that renders truncatePubkey(currentPubkey) in a Semantics widget to provide a meaningful label and/or value for screen readers (e.g., semanticsLabel: 'Current node public key, truncated' and/or value: currentPubkey) while preserving the existing style; locate the Text displaying truncatePubkey in mostro_node_selector.dart and update that widget tree so screen readers get explicit context for the truncated pubkey without changing the visual appearance.lib/features/settings/widgets/relay_management_card.dart (1)
145-195: Consider using Dart 3's.indexedfor cleaner iteration.The
.asMap().entries.map()pattern works correctly, but Dart 3 provides the.indexedextension for cleaner index-value iteration.♻️ Optional: Use .indexed for cleaner syntax
- ..._relays.asMap().entries.map((entry) { - final index = entry.key; - final relay = entry.value; + ..._relays.indexed.map((entry) { + final (index, relay) = entry; final dotColor = relay.isActive ? c.mostroGreen : c.textDisabled;Or with destructuring directly in the parameter:
- ..._relays.asMap().entries.map((entry) { - final index = entry.key; - final relay = entry.value; + ..._relays.indexed.map(((index, relay)) { final dotColor = relay.isActive ? c.mostroGreen : c.textDisabled;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/widgets/relay_management_card.dart` around lines 145 - 195, Replace the .asMap().entries.map(...) iteration over _relays with Dart 3's .indexed to simplify index/value access; use _relays.indexed.map(...) (or destructured parameters like (index, relay) if you prefer) and refer to indexed.index / indexed.value (or the destructured names) where the code currently uses entry.key and entry.value so calls to _toggleRelay(index, v) and _removeRelay(index) continue to work unchanged and all references to relay (e.g., relay.url, relay.isActive, relay.isDefault) remain the same.lib/features/settings/screens/log_report_screen.dart (1)
46-68: Make_mockEntriesimmutable.The list is
static finalbut still mutable. Wrapping it as unmodifiable avoids accidental in-place edits and state drift across instances.Proposed fix
- static final List<_LogEntry> _mockEntries = [ + static final List<_LogEntry> _mockEntries = List.unmodifiable([ _LogEntry( id: 1, level: _LogLevel.info, tag: 'App', message: 'Application started successfully', timestamp: DateTime.now().millisecondsSinceEpoch ~/ 1000 - 120, ), @@ - ]; + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/settings/screens/log_report_screen.dart` around lines 46 - 68, The _mockEntries list is declared static final but remains mutable; wrap it as an unmodifiable list to prevent in-place edits. Replace the current literal with an immutable wrapper (e.g., use List.unmodifiable([...]) or UnmodifiableListView([...]) from dart:collection) when initializing _mockEntries so the List<_LogEntry> cannot be modified at runtime; keep the same _LogEntry instances and timestamps but ensure you add the necessary import if using UnmodifiableListView.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/settings/screens/log_report_screen.dart`:
- Around line 82-86: The Share action should be disabled when there are no log
entries; update the IconButton(s) that currently use onPressed: () =>
_shareLogs() so they compute an enabled flag from the log list (e.g., final
hasLogs = logEntries.isNotEmpty) and set onPressed: hasLogs ? _shareLogs : null
(and optionally adjust the tooltip to reflect disabled state). Apply this change
for the IconButton instance referencing _shareLogs shown in the diff and the
other IconButton occurrence around the second block (lines 141-148) so sharing
is only possible when logEntries.isNotEmpty.
- Around line 164-184: The _shareLogs() function currently shares raw
tag/message text; add a sanitizer function (e.g., _sanitizeForShare) and call it
on e.tag and e.message (or the final line) before joining so PII/secrets are
redacted; implement regex replacements for common auth headers and keys (e.g.,
patterns for Authorization/Bearer, token|apikey|api_key|secret|password with
separators, and long hex/npub/nsec-like keys) and replace matches with stable
tokens like [REDACTED_AUTH]/[REDACTED_SECRET]/[REDACTED_KEY], then use the
sanitized strings in the lines construction inside _shareLogs() and keep
existing error handling intact.
---
Nitpick comments:
In `@lib/features/settings/screens/log_report_screen.dart`:
- Around line 46-68: The _mockEntries list is declared static final but remains
mutable; wrap it as an unmodifiable list to prevent in-place edits. Replace the
current literal with an immutable wrapper (e.g., use List.unmodifiable([...]) or
UnmodifiableListView([...]) from dart:collection) when initializing _mockEntries
so the List<_LogEntry> cannot be modified at runtime; keep the same _LogEntry
instances and timestamps but ensure you add the necessary import if using
UnmodifiableListView.
In `@lib/features/settings/widgets/mostro_node_selector.dart`:
- Around line 130-142: Wrap the Text that renders truncatePubkey(currentPubkey)
in a Semantics widget to provide a meaningful label and/or value for screen
readers (e.g., semanticsLabel: 'Current node public key, truncated' and/or
value: currentPubkey) while preserving the existing style; locate the Text
displaying truncatePubkey in mostro_node_selector.dart and update that widget
tree so screen readers get explicit context for the truncated pubkey without
changing the visual appearance.
In `@lib/features/settings/widgets/relay_management_card.dart`:
- Around line 145-195: Replace the .asMap().entries.map(...) iteration over
_relays with Dart 3's .indexed to simplify index/value access; use
_relays.indexed.map(...) (or destructured parameters like (index, relay) if you
prefer) and refer to indexed.index / indexed.value (or the destructured names)
where the code currently uses entry.key and entry.value so calls to
_toggleRelay(index, v) and _removeRelay(index) continue to work unchanged and
all references to relay (e.g., relay.url, relay.isActive, relay.isDefault)
remain the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6482c38-f3f4-4cf6-a9c7-257d55a44990
📒 Files selected for processing (4)
lib/features/settings/screens/log_report_screen.dartlib/features/settings/widgets/mostro_node_selector.dartlib/features/settings/widgets/relay_management_card.dartrust/src/api/settings.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust/src/api/settings.rs
…difiable list, pubkey semantics, indexed iteration - log_report_screen.dart: share button disabled when no entries; _sanitizeForShare redacts auth tokens, key-value secrets, long hex/npub/nsec strings before export; _mockEntries wrapped with List.unmodifiable - mostro_node_selector.dart: truncated pubkey Text wrapped in Semantics with full pubkey as value for screen readers - relay_management_card.dart: replace .asMap().entries.map() with .indexed and destructured record for cleaner Dart 3 style
This was referenced Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit