feat: implement automatic relay synchronization with Mostro instances #272
Conversation
Add relay sync system that auto-discovers relays from Mostro via kind 10002 events with user blacklist for permanent blocking. Users can remove relays from blacklist and manually re-add them anytime. - Real-time sync via Nostr subscriptions - User blacklist prevents unwanted relay re-addition - Smart re-enablement: manually adding removes from blacklist - Source tracking (user/mostro/default) - Backward compatible with existing configurations
WalkthroughImplements NIP‑65 relay‑list support: new RelayListEvent model, subscription APIs for Mostro relay lists, RelaysNotifier Mostro sync/merge with blacklist and user‑relay metadata, Settings blacklist persistence and APIs, provider/ref wiring, UI/localization updates, and test/mocks adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as main()
participant Provider as relaysProvider
participant Notifier as RelaysNotifier
participant Subs as SubscriptionManager
participant Mostro as Mostro (pubkey)
App->>Provider: read(relaysProvider)
Provider->>Notifier: instantiate RelaysNotifier(settings, ref)
Notifier->>Subs: subscribeToMostroRelayList(mostroPubkey)
Subs->>Mostro: request events (kind=10002, authors=[pubkey])
Mostro-->>Subs: RelayListEvent(s)
Subs-->>Notifier: relayList stream emits RelayListEvent
Notifier->>Notifier: merge relays (preserve defaults/user, skip blacklisted)
Notifier-->>App: state updated (mostroRelaysWithStatus)
sequenceDiagram
autonumber
participant Settings as SettingsNotifier
participant Notifier as RelaysNotifier
participant Subs as SubscriptionManager
Settings->>Notifier: mostroPublicKey changed
Notifier->>Subs: unsubscribeFromMostroRelayList()
Notifier->>Subs: subscribeToMostroRelayList(newPubkey)
Subs-->>Notifier: relayList events
Notifier->>Notifier: reconcile relays (skip blacklisted, promote/demote per rules)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
lib/features/relays/relays_notifier.dart (1)
211-219: Do not disconnect the global Nostr singleton on test failureIn _testNostrProtocol() catch block you call _cleanupTestConnection(Nostr.instance), which can tear down the app’s main connection. You should only clean up the isolated test instance you created.
Apply this diff:
- } catch (e) { - // Protocol test failed with error - try { - await _cleanupTestConnection(Nostr.instance); - } catch (_) { - // Ignore cleanup errors - } - return false; - } + } catch (e) { + // Protocol test failed with error + try { + await _cleanupTestConnection(testNostr); + } catch (_) { + // Ignore cleanup errors + } + return false; + }
🧹 Nitpick comments (11)
lib/main.dart (2)
5-6: Remove unused import.
auth_notifier_provider.dartappears unused in this file. If unused, drop it to keep imports clean.-import 'package:mostro_mobile/features/auth/providers/auth_notifier_provider.dart'; import 'package:mostro_mobile/features/relays/relays_provider.dart';
52-54: Triggering sync by reading provider is fine; suggest reading the notifier for intent clarity and capturing async errors.
- Reading the state lazily initializes the notifier; reading the notifier makes the side-effect intent clearer.
- Consider using container.listen(relaysProvider, ...) to surface async errors, or moving the side-effect into the notifier’s initialization (e.g., in constructor/build + ref.onDispose) to centralize lifecycle.
- _initializeRelaySynchronization(container); + _initializeRelaySynchronization(container); @@ -void _initializeRelaySynchronization(ProviderContainer container) { +void _initializeRelaySynchronization(ProviderContainer container) { try { - // Read the relays provider to trigger initialization of RelaysNotifier - // This will automatically start sync with the configured Mostro instance - container.read(relaysProvider); + // Read the notifier to trigger initialization and side-effects explicitly + container.read(relaysProvider.notifier); } catch (e) { // Log error but don't crash app if relay sync initialization fails debugPrint('Failed to initialize relay synchronization: $e'); } }If you want, I can provide a variant using container.listen to bubble up errors during sync initialization.
Also applies to: 63-73
lib/features/relays/relays_provider.dart (1)
8-11: Avoid watching the notifier; use read and drop the leftover comment.Watching the notifier rarely re-triggers provider creation. Since you just need the instance to build
RelaysNotifier,ref.readis more accurate. Also remove the “Assume you have this provider defined.” comment.- final settings = ref.watch( - settingsProvider.notifier); // Assume you have this provider defined. + final settings = ref.read(settingsProvider.notifier); return RelaysNotifier(settings, ref);test/features/relays/relays_notifier_test.dart (1)
5-15: Placeholder test drops coverage; propose a minimal harness to test blacklist and sync behaviors.Replacing the suite with a placeholder loses signal on regressions. Short-term, keep a skipped test documenting TODOs; mid-term, stand up a ProviderContainer with overrides and mock the subscription layer.
Suggested immediate change: mark placeholder as skipped and add explicit TODO to track.
- group('RelaysNotifier', () { + group('RelaysNotifier', () { // TODO: Re-enable these tests after implementing proper Ref mocking // The RelaysNotifier now requires a Ref parameter for Mostro relay synchronization // These tests need to be updated to provide proper mocks for the new sync functionality - test('placeholder for future test implementation', () { + test('placeholder for future test implementation', () { // This test serves as a placeholder while the relay sync functionality // is being implemented. The original tests tested URL validation and // relay connectivity, which will need to be adapted to work with // the new Mostro relay synchronization features. - expect(true, true); + expect(true, true); }); });Targets for the new tests (I can help scaffold these):
- When a relay is manually added, it’s removed from blacklist (smart re-enablement).
- Sync merges Mostro relays (kind 10002) with user/default relays respecting blacklist.
- Blacklisted relay is not re-added by sync; removal from blacklist allows re-add.
- Source attribution (user/mostro/default) is preserved across sync operations.
Do you want me to open a follow-up issue and push a test harness using ProviderContainer with overrides/mocks for the subscription manager?
lib/features/settings/settings.dart (1)
20-38: copyWith inconsistency: nullable fields may be reset unintentionally.
selectedLanguageanddefaultLightningAddressare assigned directly, so callingcopyWith()without those params will null them out. Align with the other fields using??to avoid accidental clears.return Settings( relays: relays ?? this.relays, fullPrivacyMode: privacyModeSetting ?? fullPrivacyMode, mostroPublicKey: mostroInstance ?? mostroPublicKey, defaultFiatCode: defaultFiatCode ?? this.defaultFiatCode, - selectedLanguage: selectedLanguage, - defaultLightningAddress: defaultLightningAddress, + selectedLanguage: selectedLanguage ?? this.selectedLanguage, + defaultLightningAddress: defaultLightningAddress ?? this.defaultLightningAddress, blacklistedRelays: blacklistedRelays ?? this.blacklistedRelays, );lib/core/models/relay_list_event.dart (2)
21-27: Deduplicate and sanitize relay URLs extracted from r-tagsIf the event repeats r-tags or includes incidental whitespace, you'll carry duplicates into state.
Apply this diff:
- final relays = event.tags - ?.where((tag) => tag.isNotEmpty && tag[0] == 'r') - .where((tag) => tag.length >= 2) - .map((tag) => tag[1]) - .where((url) => url.isNotEmpty) - .toList() ?? <String>[]; + final relays = event.tags + ?.where((tag) => tag.isNotEmpty && tag[0] == 'r') + .where((tag) => tag.length >= 2) + .map((tag) => tag[1].trim()) + .where((url) => url.isNotEmpty) + .toSet() // dedupe + .toList() ?? + <String>[];
47-51: Optionally restrict to secure relays (wss://) onlyFor user safety, you may want to drop ws:// endpoints from validRelays, or at least expose a flag to opt-in. Right now validRelays allows ws://.
- List<String> get validRelays { - return relays - .where((url) => url.startsWith('wss://') || url.startsWith('ws://')) - .toList(); - } + List<String> get validRelays { + return relays.where((url) => url.startsWith('wss://')).toList(); + }lib/features/subscriptions/subscription_manager.dart (1)
236-253: Minor DRY opportunity: unify relay-list event routingYou duplicate event routing logic for relayList in both _handleEvent and _subscribeToRelayList. Consider funneling both through a single handler or reusing _handleEvent by allowing subscribe() with relayList and internally building the filter.
No functional change required; this is a maintainability suggestion to reduce divergent code paths and future drift.
Also applies to: 255-293
lib/features/settings/settings_notifier.dart (1)
11-16: ref is unusedThe optional Ref is currently stored but not used. Consider removing until needed to avoid confusion.
- final Ref? ref; - final _logger = Logger(); + final Ref? ref; // consider removing until used + final _logger = Logger();Alternatively, add a TODO explaining forthcoming usage.
lib/features/relays/relay.dart (1)
47-59: Default isHealthy from JSON should align with constructor defaultConstructor defaults isHealthy = true, but fromJson falls back to false when the field is absent. This regresses legacy data to “unhealthy” on load.
Apply this diff:
- isHealthy: json['isHealthy'] as bool? ?? false, + isHealthy: json['isHealthy'] as bool? ?? true,test/mocks.mocks.dart (1)
1-4: Do not manually edit Mockito-generated file; regenerate with build_runner.Per repo guidelines and the file header, this file must be generated. If these diffs weren’t produced by running build_runner, please discard manual edits and regenerate. Also, avoid adding ad‑hoc // ignore comments in this file; rely on the existing file-level ignores produced by the generator.
You can (re)generate mocks locally with:
- flutter: flutter pub run build_runner build --delete-conflicting-outputs
- dart: dart run build_runner build --delete-conflicting-outputs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
lib/core/models/relay_list_event.dart(1 hunks)lib/features/relays/relay.dart(1 hunks)lib/features/relays/relays_notifier.dart(5 hunks)lib/features/relays/relays_provider.dart(1 hunks)lib/features/settings/settings.dart(6 hunks)lib/features/settings/settings_notifier.dart(3 hunks)lib/features/subscriptions/subscription_manager.dart(6 hunks)lib/features/subscriptions/subscription_type.dart(1 hunks)lib/main.dart(2 hunks)test/features/relays/relays_notifier_test.dart(1 hunks)test/mocks.mocks.dart(6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
lib/!(generated)/**/*.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/subscriptions/subscription_type.dartlib/core/models/relay_list_event.dartlib/features/relays/relays_provider.dartlib/features/subscriptions/subscription_manager.dartlib/features/settings/settings.dartlib/features/settings/settings_notifier.dartlib/features/relays/relay.dartlib/features/relays/relays_notifier.dart
lib/main.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
Configure timeago locale messages in main.dart using timeago.setLocaleMessages()
Files:
lib/main.dart
**/*.mocks.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.mocks.dart: Never manually edit *.mocks.dart Mockito-generated files
Do not add ignore comments to generated mock files; regenerate instead
Files:
test/mocks.mocks.dart
test/mocks.mocks.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
test/mocks.mocks.dart: Do not add individual // ignore: must_be_immutable comments; rely on the existing file-level ignore
Never manually edit test/mocks.mocks.dart; regenerate with build_runner when needed
Files:
test/mocks.mocks.dart
🧠 Learnings (8)
📚 Learning: 2025-05-08T16:31:29.582Z
Learnt from: chebizarro
PR: MostroP2P/mobile#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/core/models/relay_list_event.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to test/mocks.mocks.dart : Never manually edit test/mocks.mocks.dart; regenerate with build_runner when needed
Applied to files:
test/features/relays/relays_notifier_test.darttest/mocks.mocks.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to test/mocks.mocks.dart : Do not add individual // ignore: must_be_immutable comments; rely on the existing file-level ignore
Applied to files:
test/features/relays/relays_notifier_test.darttest/mocks.mocks.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
lib/main.dartlib/features/subscriptions/subscription_manager.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, 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/main.dartlib/features/subscriptions/subscription_manager.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/shared/providers/mostro_database_provider.dart : Initialize the Sembast database in mostro_database_provider.dart
Applied to files:
lib/main.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/features/*/providers/**/*.dart : Organize Riverpod providers by feature under lib/features/{feature}/providers/
Applied to files:
lib/main.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/data/models.dart : Export models via the barrel file lib/data/models.dart
Applied to files:
lib/features/subscriptions/subscription_manager.dart
🔇 Additional comments (13)
lib/main.dart (2)
40-50: Good DI: preconfigured ProviderContainer + UncontrolledProviderScope.The overrides are clear and explicit, and this pattern is appropriate since you need to read providers before runApp. No action required.
75-84: timeago locales initialized in main.dart — matches guideline.Spanish and Italian locales are registered early; English is default. This satisfies the guideline to configure timeago in main.dart.
lib/features/settings/settings.dart (3)
8-18: Settings model: blacklist field addition/persistence looks solid.New
blacklistedRelayswith default empty list and JSON wiring will keep backward compatibility with existing stored settings.
40-49: JSON persistence for blacklist is correct.
toJsonincludingblacklistedRelaysis correct and aligns withfromJson. No action needed.
50-59: fromJson defaults are safe and backward compatible.Graceful defaulting to
[]for missingblacklistedRelaysavoids migrations for existing users.lib/features/subscriptions/subscription_manager.dart (1)
185-188: Explicitly gate relayList via dedicated API: good guardrailThrowing on subscribe(type: relayList, ...) prevents accidental misuse and funnels callers through subscribeToMostroRelayList(), which wires the Mostro-specific filter and lifecycle.
lib/features/settings/settings_notifier.dart (1)
76-85: Blacklist API looks solidCopy-on-write updates, persistence via _saveToPrefs, and idempotent add/remove semantics are correct. Good logging and simple getters.
Also applies to: 87-95, 97-104, 106-112
lib/features/relays/relay.dart (1)
61-79: New source/addedAt fields and helpers are well designedClear source semantics, convenience factories, and derived getters (isAutoDiscovered, canDelete, canBlacklist) improve UX and future-proof the model.
Also applies to: 81-103
lib/features/relays/relays_notifier.dart (2)
372-381: Mostro relay sync wiring looks goodCreating a dedicated SubscriptionManager, listening to relayList, and delegating to _handleMostroRelayListUpdate is a clean separation. Logging is helpful.
Also applies to: 383-405
4-4: Typo in import path:ease.dartshould beeose.dart(or remove import)The file name is almost certainly eose.dart (EOSE = End Of Stored Events). Importing ease.dart will fail at compile time. Also, many packages re-export EOSE types from package:dart_nostr/dart_nostr.dart, making the extra import unnecessary.
Apply one of these:
Option A (preferred if re-exported):
-import 'package:dart_nostr/nostr/model/ease.dart';Option B (if direct import is needed):
-import 'package:dart_nostr/nostr/model/ease.dart'; +import 'package:dart_nostr/nostr/model/eose.dart';⛔ Skipped due to learnings
Learnt from: chebizarro PR: MostroP2P/mobile#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.test/mocks.mocks.dart (3)
2400-2456: Production RelaysNotifier API Implemented
Confirmed that all methods—syncWithMostroInstance,removeRelayWithBlacklist,removeRelayWithSource,isRelayBlacklisted,clearBlacklistAndResync, anddispose—are defined in lib/features/relays/relays_notifier.dart with matching signatures. The mocks in test/mocks.mocks.dart align correctly and can now be stubbed/verified in your tests.
2227-2235: Mocks align with RelaysNotifier APIVerified that in
lib/features/relays/relays_notifier.darttheRelaysNotifierclass declares:
final Ref ref;(line 28)List<String> get blacklistedRelays => settings.blacklistedRelays;(line 519)The mocks correctly override these members—no further changes needed.
1882-1887: Verified Settings API matches mocks forblacklistedRelays
TheSettingsclass declaresblacklistedRelays, defaults it in the constructor, accepts it incopyWith, and includes it in bothtoJsonandfromJson. Mocks and real API are in sync — no further changes needed.
| enum SubscriptionType { | ||
| chat, | ||
| orders, | ||
| relayList, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enum addition looks good; verify exhaustive switches handle relayList.
Appending relayList keeps existing indices stable. Please scan for switch/when statements or maps over SubscriptionType to ensure relayList is handled to avoid runtime fall-throughs.
Run this to locate potential switch-sites that may need a new case:
🏁 Script executed:
#!/bin/bash
# Show switch/when statements involving SubscriptionType and nearby context
rg -nP --type=dart -C3 '(switch|when)\s*\([^)]*\bSubscriptionType\b'
# Double-check for places that list enum cases explicitly (maps/arrays)
rg -nP --type=dart -C2 '\bSubscriptionType\.(chat|orders)\b'Length of output: 3207
Handle new relayList case in all switch/if blocks
Please update every place that branches on SubscriptionType to explicitly handle the new relayList value:
• lib/features/subscriptions/subscription_manager.dart
– Lines 96–103 (subscription creation switch): add
dart case SubscriptionType.relayList: // TODO: build a relayList subscription break;
• lib/features/subscriptions/subscription_manager.dart
– Lines 126–133 (event dispatch switch): add
dart case SubscriptionType.relayList: _relayListController.add(event); break;
• test/mocks.dart
– Lines 126–128 and 164–168 (if/else on orders/chat): extend to cover relayList (or switch to a switch for exhaustiveness)
Ensuring every branch covers relayList will prevent runtime fall-throughs when the enum grows.
🤖 Prompt for AI Agents
In lib/features/subscriptions/subscription_type.dart (enum updated to include
relayList), update all switch/if branches that discriminate on SubscriptionType
to explicitly handle the new relayList case: in
lib/features/subscriptions/subscription_manager.dart around lines 96–103 add a
case SubscriptionType.relayList that builds or returns the appropriate relayList
subscription (placeholder TODO ok for now) and in the same file around lines
126–133 add a case SubscriptionType.relayList that forwards events to the relay
list stream (e.g., _relayListController.add(event)); also update test/mocks.dart
at the if/else blocks around lines 126–128 and 164–168 to handle relayList (or
convert those conditionals to a switch on SubscriptionType so all enum values
are handled exhaustively) to avoid fall-throughs or missing-case runtime
behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
CLAUDE.md (7)
20-25: Fix minor grammar in “Essential Commands” (build_runner item).Add “is” to improve readability.
- - **`dart run build_runner build -d`** - 🟡 **Only when code generation needed** (models, providers, mocks, localization) + - **`dart run build_runner build -d`** - 🟡 **Only when code generation is needed** (models, providers, mocks, localization)
57-63: Polish wording and punctuation in Relay Management bullets.Add terminal periods and tighten phrasing.
- - **Automatic Sync**: Real-time synchronization with Mostro instance relay lists via kind 10002 events - - **User Control**: Sophisticated blacklist system allowing permanent blocking of auto-discovered relays - - **Smart Re-enablement**: Manual relay addition automatically removes from blacklist - - **Source Tracking**: Relays tagged by source (user, mostro, default) for appropriate handling - - **Implementation**: Located in `features/relays/` with core logic in `RelaysNotifier` + - **Automatic Sync**: Real-time synchronization with the Mostro instance’s relay list via kind‑10002 events. + - **User Control**: Sophisticated blacklist system allowing permanent blocking of auto-discovered relays. + - **Smart Re-enablement**: Manually adding a relay automatically removes it from the blacklist. + - **Source Tracking**: Relays tagged by source (user, mostro, default) for appropriate handling. + - **Implementation**: Located in `features/relays/` with core logic in `RelaysNotifier`.
132-137: Unify localization API usage (nullable S).Elsewhere you use
S.of(context)!.keyName. Mirror that here for consistency.- - User-facing strings use localization system (`S.of(context).keyName`) + - User-facing strings use the localization system (`S.of(context)!.keyName`)
216-221: Add a language to fenced code block to satisfy markdownlint (MD040).Use “text” for non-source code blocks.
-``` +```text User removes Mostro relay → Added to blacklist → Never re-added during sync User manually adds relay → Removed from blacklist → Works as user relay--- `248-254`: **Clarify testing statement; avoid “comprehensive” while noting temporary disablement.** As written, “comprehensive” + “currently disabled” can confuse readers. Suggest clearer, action-oriented phrasing with a tracking note. ```diff - - **Testing**: Comprehensive unit tests in `test/features/relays/` (currently disabled due to complex mocking requirements) + - **Testing**: Unit tests exist; relay sync tests are temporarily disabled pending mock refactor (tracked in the issue tracker). Please run `flutter test` before commits.
388-396: Optionally auto-verify referenced file list stays valid.Consider adding a quick CI/lint step (or a local pre-commit) that checks these paths exist; script provided above covers the same. Keeps CLAUDE.md from drifting.
433-437: Optional: Reduce staleness risk in version lines.“Latest stable” can go stale quickly. Either pin versions or specify “stable channel” to avoid needing frequent doc edits.
Example alternative:
- Flutter: stable channel
- Dart: stable channel
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
CLAUDE.md(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Run dart run build_runner build -d after installing dependencies or when code generation is needed
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/l10n/*.arb : After modifying ARB files, regenerate localization outputs (e.g., dart run build_runner build -d or flutter gen-l10n)
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Run flutter analyze and keep zero issues before committing
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/!(generated)/**/*.dart : Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Applied to files:
CLAUDE.md
🪛 LanguageTool
CLAUDE.md
[grammar] ~23-~23: There might be a mistake here.
Context: ...nner build -d`** - 🟡 Only when code generation needed (models, providers, mocks, loca...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ... management ### Relay Management System - Automatic Sync: Real-time synchronizat...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ...stance relay lists via kind 10002 events - User Control: Sophisticated blacklist ...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...anent blocking of auto-discovered relays - Smart Re-enablement: Manual relay addi...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ...ion automatically removes from blacklist - Source Tracking: Relays tagged by sour...
(QB_NEW_EN)
[grammar] ~61-~61: There might be a mistake here.
Context: ...ostro, default) for appropriate handling - Implementation: Located in `features/r...
(QB_NEW_EN)
[grammar] ~132-~132: There might be a mistake here.
Context: ...rns ### Code Comments and Documentation - All code comments must be in English -...
(QB_NEW_EN)
[grammar] ~136-~136: There might be a mistake here.
Context: ...uld be in English - User-facing strings use localization system (`S.of(context).key...
(QB_NEW_EN)
[grammar] ~171-~171: There might be a mistake here.
Context: ...lay Synchronization System ### Overview Comprehensive system that automatically ...
(QB_NEW_EN)
[grammar] ~176-~176: There might be a mistake here.
Context: ...lib/core/models/relay_list_event.dart) - Parses NIP-65 (kind 10002) events from M...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...kind 10002) events from Mostro instances - Validates relay URLs (WebSocket only) - ...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ... - Validates relay URLs (WebSocket only) - Robust handling of different timestamp f...
(QB_NEW_EN)
[grammar] ~179-~179: There might be a mistake here.
Context: ... handling of different timestamp formats - Null-safe parsing for malformed events ...
(QB_NEW_EN)
[grammar] ~197-~197: There might be a mistake here.
Context: ... (lib/features/settings/settings.dart) - New blacklistedRelays: List<String> fi...
(QB_NEW_EN)
[grammar] ~198-~198: There might be a mistake here.
Context: ... blacklistedRelays: List<String> field - Backward-compatible serialization - Auto...
(QB_NEW_EN)
[grammar] ~199-~199: There might be a mistake here.
Context: ...ield - Backward-compatible serialization - Automatic migration for existing users ...
(QB_NEW_EN)
[grammar] ~202-~202: There might be a mistake here.
Context: ...b/features/relays/relays_notifier.dart) - **syncWithMostroInstance()**: Manual sync trigger - **removeRelayWi...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...MostroInstance()**: Manual sync trigger - **removeRelayWithBlacklist(String url)**: Smart removal with blacklisting - **a...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...url)**: Smart removal with blacklisting - **addRelayWithSmartValidation(...)`**: Auto-removes from blacklist when user ...
(QB_NEW_EN)
[grammar] ~205-~205: There might be a mistake here.
Context: ...s from blacklist when user manually adds - _handleMostroRelayListUpdate(): Filters blacklisted relays during sync...
(QB_NEW_EN)
[grammar] ~210-~210: There might be a mistake here.
Context: ...chronization Flow #### Real-time Sync 1. App Launch: Automatic subscription to ...
(QB_NEW_EN)
[grammar] ~211-~211: There might be a mistake here.
Context: ...kind 10002 events from configured Mostro 2. Event Reception: Parse relay list and ...
(QB_NEW_EN)
[grammar] ~212-~212: There might be a mistake here.
Context: ... relay list and filter against blacklist 3. State Update: Merge new relays while p...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ... new relays while preserving user relays 4. NostrService: Automatic reconnection t...
(QB_NEW_EN)
[grammar] ~224-~224: There might be a mistake here.
Context: ...### Key Features #### User Experience - Transparent Operation: Sync happens au...
(QB_NEW_EN)
[grammar] ~225-~225: There might be a mistake here.
Context: ...Operation**: Sync happens automatically in background - Full User Control: Can...
(QB_NEW_EN)
[grammar] ~230-~230: There might be a mistake here.
Context: ...cross syncs #### Technical Robustness - Real-time Updates: WebSocket subscript...
(QB_NEW_EN)
[grammar] ~231-~231: There might be a mistake here.
Context: ...WebSocket subscriptions for instant sync - Error Resilience: Graceful fallbacks a...
(QB_NEW_EN)
[grammar] ~232-~232: There might be a mistake here.
Context: ...llbacks and comprehensive error handling - Race Protection: Prevents concurrent s...
(QB_NEW_EN)
[grammar] ~233-~233: There might be a mistake here.
Context: ...n**: Prevents concurrent sync operations - Logging: Detailed logging for debuggin...
(QB_NEW_EN)
[grammar] ~248-~248: There might be a mistake here.
Context: ...dResync(); ``` ### Implementation Notes - Subscription Management: Uses `Subscri...
(QB_NEW_EN)
[grammar] ~433-~433: There might be a mistake here.
Context: ...ments --- Last Updated: 2025-08-18 Flutter Version: Latest stable **Dar...
(QB_NEW_EN)
[grammar] ~434-~434: There might be a mistake here.
Context: ...-18 Flutter Version: Latest stable Dart Version: Latest stable **Key De...
(QB_NEW_EN)
[grammar] ~435-~435: There might be a mistake here.
Context: ...stable Dart Version: Latest stable Key Dependencies: Riverpod, GoRouter, ...
(QB_NEW_EN)
[grammar] ~455-~455: There might be a mistake here.
Context: ... Live updates via Nostr protocol - 🔗 Smart Relay Management: Automatic sync ...
(QB_NEW_EN)
[grammar] ~457-~457: There might be a mistake here.
Context: ...ic sync with blacklist control ### Recent Achievements - UI Modernization: Co...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
CLAUDE.md
217-217: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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)
CLAUDE.md (1)
169-247: ✅ Doc-to-code alignment verified for Relay Sync APIs and types
All referenced files and symbols (models, enums, methods, and subscription hooks) exist exactly as documented. Documentation is up-to-date with the implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
lib/core/models/relay_list_event.dart (5)
21-28: Normalize and de-duplicate relay URLs when extracting r-tagsPrevents duplicate entries and whitespace artifacts while preserving source order. Also reduces iterator churn.
Apply this diff:
- // Extract relay URLs from 'r' tags - final relays = event.tags - ?.where((tag) => tag.isNotEmpty && tag[0] == 'r') - .where((tag) => tag.length >= 2) - .map((tag) => tag[1]) - .where((url) => url.isNotEmpty) - .toList() ?? <String>[]; + // Extract relay URLs from 'r' tags (trimmed, de-duplicated, order-preserving) + final relays = <String>[]; + final seen = <String>{}; + for (final tag in event.tags ?? const <List<String>>[]) { + if (tag.isEmpty || tag[0] != 'r' || tag.length < 2) continue; + final url = tag[1].trim(); + if (url.isEmpty) continue; + if (seen.add(url)) relays.add(url); + }
34-34: Parse Nostr created_at as UTCNostr timestamps are epoch seconds in UTC. Parsing without isUtc can introduce subtle timezone skew in comparisons and ordering.
- publishedAt = DateTime.fromMillisecondsSinceEpoch((event.createdAt as int) * 1000); + publishedAt = DateTime.fromMillisecondsSinceEpoch( + (event.createdAt as int) * 1000, + isUtc: true, + );
35-37: Avoid inventing timestamps; drop invalid events insteadFalling back to DateTime.now() can corrupt ordering and cache behavior. If createdAt is of an unknown type, better to ignore the event.
- } else { - publishedAt = DateTime.now(); // Fallback to current time - } + } else { + // Unknown createdAt type; ignore this malformed event + return null; + }
47-51: Harden relay URL validation and remove duplicates in getterUse Uri.tryParse for robust parsing, accept scheme case-insensitively, trim values, and return unique URLs.
List<String> get validRelays { - return relays - .where((url) => url.startsWith('wss://') || url.startsWith('ws://')) - .toList(); + final seen = <String>{}; + final out = <String>[]; + for (final raw in relays) { + final url = raw.trim(); + final uri = Uri.tryParse(url); + final scheme = uri?.scheme.toLowerCase(); + final isWs = scheme == 'ws' || scheme == 'wss'; + if (uri != null && isWs && seen.add(url)) { + out.add(url); + } + } + return out; }
10-14: Make the relays list immutable to prevent external mutationThe field is final but the List remains mutable; external mutation can break equality/hashCode invariants. Store an unmodifiable copy.
- const RelayListEvent({ - required this.relays, - required this.publishedAt, - required this.authorPubkey, - }); + RelayListEvent({ + required List<String> relays, + required this.publishedAt, + required this.authorPubkey, + }) : relays = List.unmodifiable(relays);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/core/models/relay_list_event.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
lib/!(generated)/**/*.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/core/models/relay_list_event.dart
🧠 Learnings (1)
📚 Learning: 2025-05-08T16:31:29.582Z
Learnt from: chebizarro
PR: MostroP2P/mobile#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/core/models/relay_list_event.dart
🔇 Additional comments (1)
lib/core/models/relay_list_event.dart (1)
58-67: Set-based equality and unordered hash are correctGood job making equality order-insensitive and consistent with hashCode. This aligns with the semantics of relay sets.
Also applies to: 69-73
- Cancel previous subscriptions before creating new ones when changing Mostro instances - Add authorPubkey validation to filter events from wrong Mostro instances - Clean Mostro relay state when switching instances to prevent contamination - Add URL normalization to prevent duplicate relays with/without trailing slash
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/relays/relays_notifier.dart (1)
126-221: Critical: Catch block cleans up the global Nostr.instance (disconnects app), not the local test instanceIf _testNostrProtocol throws, the catch path disconnects Nostr.instance, potentially tearing down the app’s live connections. Use the locally created test instance and guarantee cleanup in finally.
Apply:
Future<bool> _testNostrProtocol(String url) async { - // Generate unique subscription ID for this test - final testSubId = 'relay_test_${DateTime.now().millisecondsSinceEpoch}'; - bool receivedEvent = false; - bool receivedEose = false; - bool isConnected = false; - - try { - // Create isolated instance for testing - final testNostr = Nostr(); + final testSubId = 'relay_test_${DateTime.now().millisecondsSinceEpoch}'; + bool receivedEvent = false; + bool receivedEose = false; + bool isConnected = false; + Nostr? testNostr; + try { + // Create isolated instance for testing + testNostr = Nostr(); @@ - if (!isConnected) { - // Failed to connect within timeout - await _cleanupTestConnection(testNostr); - return false; - } + if (!isConnected) { + // Failed to connect within timeout + return false; + } @@ - // Clean up connection - await _cleanupTestConnection(testNostr); - // Relay is healthy if we received either EVENT or EOSE (or both) return receivedEvent || receivedEose; - } catch (e) { - // Protocol test failed with error - try { - await _cleanupTestConnection(Nostr.instance); - } catch (_) { - // Ignore cleanup errors - } - return false; - } + } catch (e) { + // Protocol test failed with error + return false; + } finally { + if (testNostr != null) { + try { + await _cleanupTestConnection(testNostr); + } catch (_) {} + } + } }
♻️ Duplicate comments (2)
lib/features/settings/settings_notifier.dart (1)
49-58: Fix confirmed: now updating the correct field (mostroPublicKey)This addresses the earlier review about updating the wrong field in updateMostroInstance. The method now correctly updates mostroPublicKey and logs the change.
lib/features/relays/relays_notifier.dart (1)
40-51: Intentional bootstrap relay hardcode is acceptable hereGiven prior discussion and current config (only one active default relay in Config.nostrRelays), hardcoding 'wss://relay.mostro.network' for default classification is an explicit bootstrap choice to fetch NIP-65 relay lists. No change requested.
🧹 Nitpick comments (2)
lib/features/relays/relays_notifier.dart (2)
552-560: Fix logging of removed Mostro relays countYou're computing the delta after assigning state = cleanedRelays, so it always logs 0. Compute before reassignment.
void _cleanMostroRelaysFromState() { final cleanedRelays = state.where((relay) => relay.source != RelaySource.mostro).toList(); if (cleanedRelays.length != state.length) { - state = cleanedRelays; - _saveRelays(); - _logger.i('Cleaned ${state.length - cleanedRelays.length} Mostro relays from state'); + final removed = state.length - cleanedRelays.length; + state = cleanedRelays; + _saveRelays(); + _logger.i('Cleaned $removed Mostro relays from state'); } }
355-365: Optional: parallelize health checks to reduce overall latencySequential connectivity tests can take a long time (especially with timeouts). Parallelizing with a concurrency cap improves UX without overloading the network.
Example:
Future<void> refreshRelayHealth() async { // Cap concurrency to e.g. 4 at a time final results = <Relay>[]; final queue = List<Relay>.from(state); const maxConcurrent = 4; final inFlight = <Future<void>>[]; Future<void> runNext() async { if (queue.isEmpty) return; final relay = queue.removeAt(0); final isHealthy = await testRelayConnectivity(relay.url); results.add(relay.copyWith(isHealthy: isHealthy)); await runNext(); } for (int i = 0; i < maxConcurrent && i < queue.length; i++) { inFlight.add(runNext()); } await Future.wait(inFlight); state = results; await _saveRelays(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
lib/core/models/relay_list_event.dart(1 hunks)lib/features/relays/relays_notifier.dart(5 hunks)lib/features/settings/settings.dart(4 hunks)lib/features/settings/settings_notifier.dart(3 hunks)test/mocks.mocks.dart(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/core/models/relay_list_event.dart
- test/mocks.mocks.dart
- lib/features/settings/settings.dart
🧰 Additional context used
📓 Path-based instructions (1)
lib/!(generated)/**/*.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/relays/relays_notifier.dartlib/features/settings/settings_notifier.dart
🧠 Learnings (2)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/relays_notifier.dart:41-49
Timestamp: 2025-08-19T17:54:14.966Z
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.
📚 Learning: 2025-08-19T17:54:14.966Z
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/relays_notifier.dart:41-49
Timestamp: 2025-08-19T17:54:14.966Z
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.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/features/settings/settings_notifier.dart (1)
44-47: No changes needed:privacyModeSettingcorrectly maps tofullPrivacyMode
After verifying theSettings.copyWithsignature, it accepts abool? privacyModeSettingparameter which is used to set the internalfullPrivacyModefield. Your existing callstate = state.copyWith(privacyModeSetting: newValue);is correct and does update
fullPrivacyMode. Please ignore the suggested diff.Likely an incorrect or invalid review comment.
- Modern switch-based UI with vertical alignment and consistent styling - Fix user relays disappearing on app restart via proper state preservation - Prevent blacklist contamination between different Mostro instances - Include user relays in NostrService operations alongside Mostro/default relays
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/l10n/intl_en.arb (1)
1-865: Add missingmyActiveTradeskey to Spanish ARB and regenerate l10nThe verification script shows that the key
myActiveTradesexists inintl_en.arbandintl_it.arbbut is missing fromintl_es.arb. Please add it, include its metadata, and then regenerate the localization outputs.• File needing update:
lib/l10n/intl_es.arb— add themyActiveTradeskey and its metadata.Suggested diff for
intl_es.arb:--- a/lib/l10n/intl_es.arb +++ b/lib/l10n/intl_es.arb @@ "mostroPubkey": "Clave pública de Mostro", + "myActiveTrades": "Mis operaciones activas", + "@myActiveTrades": { + "description": "Etiqueta para la sección de operaciones activas del usuario" + }, "@mostroPubkey": { "description": "La clave pública del nodo Mostro en la red Nostr" },After merging this change, regenerate the Dart localization classes by running one of the following from the project root:
•
flutter gen-l10n
•dart run build_runner build -dThis will ensure all three locales remain in sync.
♻️ Duplicate comments (3)
lib/features/settings/settings_notifier.dart (3)
107-114: Normalize on reads/checks to keep behavior consistentUse the same normalization for isRelayBlacklisted. Optionally expose a normalized/deduped getter if needed.
/// Check if a relay URL is blacklisted bool isRelayBlacklisted(String relayUrl) { - return state.blacklistedRelays.contains(relayUrl); + return state.blacklistedRelays.contains(_normalizeUrl(relayUrl)); } @@ - /// Get all blacklisted relay URLs - List<String> get blacklistedRelays => List<String>.from(state.blacklistedRelays); + /// Get all blacklisted relay URLs (as stored) + List<String> get blacklistedRelays => List<String>.from(state.blacklistedRelays); + // Optionally also provide: + // List<String> get normalizedBlacklistedRelays => + // state.blacklistedRelays.map(_normalizeUrl).toSet().toList();
86-95: Normalize URLs when adding to blacklist to ensure reliable matchingPersisting raw inputs leads to duplicates and mismatches (case, trailing slash). Normalize once on write.
/// Add a relay URL to the blacklist to prevent it from being auto-synced from Mostro Future<void> addToBlacklist(String relayUrl) async { - final currentBlacklist = List<String>.from(state.blacklistedRelays); - if (!currentBlacklist.contains(relayUrl)) { - currentBlacklist.add(relayUrl); + final normalized = _normalizeUrl(relayUrl); + final currentBlacklist = List<String>.from(state.blacklistedRelays); + if (!currentBlacklist.contains(normalized)) { + currentBlacklist.add(normalized); state = state.copyWith(blacklistedRelays: currentBlacklist); await _saveToPrefs(); - _logger.i('Added relay to blacklist: $relayUrl'); + _logger.i('Added relay to blacklist: $normalized'); } } + + String _normalizeUrl(String url) { + var u = url.trim().toLowerCase(); + if (u.endsWith('/')) u = u.substring(0, u.length - 1); + return u; + }
97-105: Normalize on removal to prevent orphaned entriesSymmetric with add: remove normalized variant.
/// Remove a relay URL from the blacklist, allowing it to be auto-synced again Future<void> removeFromBlacklist(String relayUrl) async { - final currentBlacklist = List<String>.from(state.blacklistedRelays); - if (currentBlacklist.remove(relayUrl)) { + final normalized = _normalizeUrl(relayUrl); + final currentBlacklist = List<String>.from(state.blacklistedRelays); + if (currentBlacklist.remove(normalized)) { state = state.copyWith(blacklistedRelays: currentBlacklist); await _saveToPrefs(); - _logger.i('Removed relay from blacklist: $relayUrl'); + _logger.i('Removed relay from blacklist: $normalized'); } }
🧹 Nitpick comments (16)
lib/features/settings/settings.dart (3)
55-66: Harden fromJson typing and nested map casting
- mostroPublicKey is required; cast it explicitly.
- Optional fields should be cast as nullable.
- For userRelays, ensure inner maps are Map<String, dynamic> to avoid runtime type errors after JSON decoding.
return Settings( relays: (json['relays'] as List<dynamic>?)?.cast<String>() ?? [], fullPrivacyMode: json['fullPrivacyMode'] as bool, - mostroPublicKey: json['mostroPublicKey'], - defaultFiatCode: json['defaultFiatCode'], - selectedLanguage: json['selectedLanguage'], - defaultLightningAddress: json['defaultLightningAddress'], + mostroPublicKey: json['mostroPublicKey'] as String, + defaultFiatCode: json['defaultFiatCode'] as String?, + selectedLanguage: json['selectedLanguage'] as String?, + defaultLightningAddress: json['defaultLightningAddress'] as String?, blacklistedRelays: (json['blacklistedRelays'] as List<dynamic>?)?.cast<String>() ?? [], - userRelays: (json['userRelays'] as List<dynamic>?) - ?.cast<Map<String, dynamic>>() ?? [], + userRelays: (json['userRelays'] as List<dynamic>?) + ?.map((e) => Map<String, dynamic>.from(e as Map)) + .toList() ?? + const [], );
22-31: Align parameter naming with field for clarityprivacyModeSetting vs fullPrivacyMode is inconsistent. Prefer using fullPrivacyMode in the copyWith signature to match the model’s field.
- Settings copyWith({ - List<String>? relays, - bool? privacyModeSetting, + Settings copyWith({ + List<String>? relays, + bool? fullPrivacyMode, String? mostroPublicKey, String? defaultFiatCode, String? selectedLanguage, String? defaultLightningAddress, List<String>? blacklistedRelays, List<Map<String, dynamic>>? userRelays, }) { return Settings( relays: relays ?? this.relays, - fullPrivacyMode: privacyModeSetting ?? fullPrivacyMode, + fullPrivacyMode: fullPrivacyMode ?? this.fullPrivacyMode,Note: requires updating call sites.
8-20: Consider making list fields defensively immutablerelays, blacklistedRelays, and userRelays are final references but mutable lists. External mutation can bypass state-change notifications and break equality assumptions. Wrap with List.unmodifiable at construction (and in copyWith) or clone on read.
lib/features/relays/relay.dart (1)
61-79: Normalize relay URLs to prevent duplicates by formattingEquality is based on url only. Without canonicalization, “wss://relay.example.com”, “wss://relay.example.com/”, and case variations may be treated as different in persistence/UI flows.
- factory Relay.fromMostro(String url) { + factory Relay.fromMostro(String url) { return Relay( - url: url, + url: _normalizeRelayUrl(url), isHealthy: true, source: RelaySource.mostro, addedAt: DateTime.now(), ); } @@ - factory Relay.fromDefault(String url) { + factory Relay.fromDefault(String url) { return Relay( - url: url, + url: _normalizeRelayUrl(url), isHealthy: true, source: RelaySource.defaultConfig, addedAt: DateTime.now(), ); }Add this helper in the file (outside the class):
String _normalizeRelayUrl(String url) { var u = url.trim(); if (!u.startsWith('ws://') && !u.startsWith('wss://')) { u = 'wss://$u'; } // Lowercase scheme/host, drop trailing slash final uri = Uri.parse(u); final hostLower = uri.host.toLowerCase(); final schemeLower = uri.scheme.toLowerCase(); final path = uri.path == '/' ? '' : uri.path; final norm = Uri( scheme: schemeLower, userInfo: uri.userInfo, host: hostLower, port: uri.hasPort ? uri.port : null, path: path, query: uri.query, fragment: uri.fragment, ).toString(); return norm.endsWith('/') ? norm.substring(0, norm.length - 1) : norm; }lib/l10n/intl_en.arb (1)
402-404: Avoid hardcoding relay.mostro.network in localized strings; use a placeholder or keep language-agnosticPer team learning, the bootstrap default is intentional, but we should not bake the host into translations. Either:
- Keep the generic text already present in es/it, or
- Add a {relayHost} placeholder, surfaced from config so white-labeling/instance overrides remain easy.
Example generic replacement:
- "blacklistDefaultRelayMessage": "You're about to blacklist relay.mostro.network, which is the default relay for basic connectivity and automatic relay synchronization.\n\nThis may affect:\n• Automatic relay updates\n• Connection reliability\n• Event synchronization\n\nAre you sure you want to continue?", + "blacklistDefaultRelayMessage": "Are you sure you want to blacklist the default Mostro relay? This may affect connectivity, automatic relay updates, and event synchronization.",lib/features/relays/relays_screen.dart (1)
21-26: Localize AppBar title instead of hardcoding 'Configuración'User-facing text must come from S.of(context)!. Consider adding a key like relaysTitle/settingsTitle and using it here.
- title: Text( - 'Configuración', + title: Text( + S.of(context)!.relaysTitle, // or S.of(context)!.settingsTitle style: TextStyle( color: AppTheme.cream1, ), ),test/features/relays/widgets/relay_selector_test.dart (2)
1-11: Avoid disabling the entire test file; prefer skipped groups/imports intactCommenting out imports makes it easy to forget reinstatement and breaks IDE tooling. Keep imports and wrap outdated tests in group(..., skip: 'reason') until rewritten.
-// import 'dart:async'; -// import 'package:flutter/material.dart'; -// import 'package:flutter_riverpod/flutter_riverpod.dart'; -// import 'package:flutter_test/flutter_test.dart'; -// import 'package:mockito/mockito.dart'; -// import 'package:mostro_mobile/features/relays/relays_notifier.dart'; -// import 'package:mostro_mobile/features/relays/relays_provider.dart'; -// import 'package:mostro_mobile/features/relays/widgets/relay_selector.dart'; -// import 'package:mostro_mobile/generated/l10n.dart'; -// import '../../../mocks.mocks.dart'; +import 'dart:async'; +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:mockito/mockito.dart'; +import 'package:mostro_mobile/features/relays/relays_notifier.dart'; +import 'package:mostro_mobile/features/relays/relays_provider.dart'; +import 'package:mostro_mobile/features/relays/widgets/relay_selector.dart'; +import 'package:mostro_mobile/generated/l10n.dart'; +import '../../../mocks.mocks.dart';
14-17: Add at least a minimal smoke test for the new UX (toggle and add button presence)While full dialog tests are being rewritten, add a simple test ensuring the widget builds and primary actions exist.
void main() { - // TODO: Update tests for new RelaySelector UX - old showAddDialog tests no longer relevant - - // Tests commented out since we changed from showAddDialog to new blacklist toggle UX - /* + testWidgets('RelaySelector renders and shows Add Relay action', (tester) async { + await tester.pumpWidget( + const ProviderScope( + child: MaterialApp(home: Scaffold(body: RelaySelector())), + ), + ); + // Ensure widget builds + expect(find.byType(RelaySelector), findsOneWidget); + // Button text is localized; fallback to key check if ARB not loaded in test env + expect(find.textContaining('Add'), findsWidgets); + }); + + // TODO: Rewrite integration tests for blacklist toggle & last-relay guard. + // Below legacy tests can be kept but marked skipped until replaced. + /* group('RelaySelector Dialog Integration Tests - DISABLED', () { ... }); */ }Also applies to: 340-341
lib/features/settings/settings_notifier.dart (1)
49-69: Avoid redundant state write when pubkey unchanged; simplify control flowEarly-return if unchanged. This reduces unnecessary writes and logs. Also update the comment in the else-branch (it currently says “same” but code runs when different).
Future<void> updateMostroInstance(String newValue) async { - final oldPubkey = state.mostroPublicKey; - - if (oldPubkey != newValue) { + final oldPubkey = state.mostroPublicKey; + if (oldPubkey == newValue) { + // No-op when unchanged; still persist to be safe is unnecessary here. + return; + } + + try { _logger.i('Mostro change detected: $oldPubkey → $newValue'); // 🔥 RESET COMPLETO: Limpiar blacklist y user relays al cambiar Mostro state = state.copyWith( mostroPublicKey: newValue, blacklistedRelays: const [], // Blacklist vacío userRelays: const [], // User relays vacíos ); _logger.i('Reset blacklist and user relays for new Mostro instance'); - } else { - // Solo actualizar pubkey si es el mismo (sin reset) - state = state.copyWith(mostroPublicKey: newValue); - } - - await _saveToPrefs(); + await _saveToPrefs(); + } finally { + // Optionally notify listeners or trigger resync elsewhere + } }lib/features/relays/widgets/relay_selector.dart (4)
311-315: Localize the hint text instead of hardcoding 'relay.example.com'Follow the localization guideline for all user-facing text.
- hintText: 'relay.example.com', + hintText: S.of(context)!.addRelayDialogHint, // add key in ARB
160-189: Prefer using Switch/CupertinoSwitch for accessibility and semanticsA custom GestureDetector slider lacks semantics, focus, and a11y affordances. Consider Switch/CheckboxListTile with proper semantics and hit targets.
135-151: Avoid fixed width for status area to prevent i18n truncationThe 140px width may clip longer localized strings. Let the row flex or measure text instead.
- Container( - width: 140, // Increased width to show full text - padding: const EdgeInsets.only(right: 16), // Prevent overflow - child: Row( + Flexible( + child: Padding( + padding: const EdgeInsets.only(right: 16), + child: Row( mainAxisAlignment: MainAxisAlignment.start, children: [ _buildRelaySwitch(context, ref, relayInfo), const SizedBox(width: 8), - Expanded( - child: Text( + Expanded( + child: Text( relayInfo.isActive ? S.of(context)!.activated : S.of(context)!.deactivated, style: const TextStyle( color: AppTheme.textSecondary, // Use same grey as description fontSize: 12, fontWeight: FontWeight.w600, ), ), ), ], - ), - ), + ), + ), + ),
194-194: Default relay check should use a single source of truth (Config or Notifier)Hardcoding the Mostro default relay URL duplicates knowledge. Expose/configure a constant (e.g., Config.bootstrapRelay) and reuse it here.
lib/features/relays/relays_notifier.dart (3)
415-423: Ensure null-safety before accessing relayList streamThe code doesn't check if
_subscriptionManageris successfully initialized before accessingrelayList. Consider adding defensive checks to prevent potential runtime errors._relayListSubscription = _subscriptionManager!.relayList.listen( +_relayListSubscription = _subscriptionManager?.relayList.listen( (relayListEvent) { _handleMostroRelayListUpdate(relayListEvent); }, onError: (error, stackTrace) { _logger.e('Error handling relay list event', error: error, stackTrace: stackTrace); }, );
554-557: Empty Relay creation as fallback is potentially misleadingCreating an empty
Relay(url: '')as a fallback infirstWherecould lead to subtle bugs if the empty URL is not properly handled downstream.Consider using a nullable approach for clarity:
-// Check if this relay was previously a user relay (PROMOTION case) -final existingUserRelay = userRelays.firstWhere( - (r) => r.url == relayUrl, - orElse: () => Relay(url: ''), // Empty relay if not found -); - -if (existingUserRelay.url.isNotEmpty) { +// Check if this relay was previously a user relay (PROMOTION case) +final existingUserRelay = userRelays.cast<Relay?>().firstWhere( + (r) => r!.url == relayUrl, + orElse: () => null, +); + +if (existingUserRelay != null) { // PROMOTION: User relay → Mostro relay (move to beginning) userRelays.removeWhere((r) => r.url == relayUrl); final promotedRelay = Relay.fromMostro(relayUrl); updatedRelays.insert(0, promotedRelay); // Insert at beginning _logger.i('Promoted user relay to Mostro relay: $relayUrl'); continue; }
791-798: URL normalization doesn't handle all edge casesThe current implementation only removes trailing slashes but doesn't handle other normalization needs like case sensitivity or query parameters.
Consider a more comprehensive normalization:
String _normalizeRelayUrl(String url) { url = url.trim(); + // Convert to lowercase for consistent comparison + url = url.toLowerCase(); // Remove trailing slash if present if (url.endsWith('/')) { url = url.substring(0, url.length - 1); } + // Remove query parameters if present + final queryIndex = url.indexOf('?'); + if (queryIndex != -1) { + url = url.substring(0, queryIndex); + } return url; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
lib/features/relays/relay.dart(1 hunks)lib/features/relays/relays_notifier.dart(5 hunks)lib/features/relays/relays_screen.dart(1 hunks)lib/features/relays/widgets/relay_selector.dart(1 hunks)lib/features/settings/settings.dart(4 hunks)lib/features/settings/settings_notifier.dart(3 hunks)lib/features/settings/settings_screen.dart(0 hunks)lib/l10n/intl_en.arb(3 hunks)lib/l10n/intl_es.arb(2 hunks)lib/l10n/intl_it.arb(2 hunks)test/features/relays/widgets/relay_selector_test.dart(2 hunks)test/mocks.mocks.dart(5 hunks)
💤 Files with no reviewable changes (1)
- lib/features/settings/settings_screen.dart
🧰 Additional context used
📓 Path-based instructions (7)
lib/!(generated)/**/*.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/relays/relays_screen.dartlib/features/settings/settings_notifier.dartlib/features/relays/relay.dartlib/features/relays/relays_notifier.dartlib/features/settings/settings.dartlib/features/relays/widgets/relay_selector.dart
lib/l10n/intl_{en,es,it}.arb
📄 CodeRabbit Inference Engine (CLAUDE.md)
When adding a localization key, add it to all three ARB files: intl_en.arb, intl_es.arb, and intl_it.arb
Files:
lib/l10n/intl_it.arblib/l10n/intl_en.arblib/l10n/intl_es.arb
lib/l10n/*.arb
📄 CodeRabbit Inference Engine (CLAUDE.md)
After modifying ARB files, regenerate localization outputs (e.g., dart run build_runner build -d or flutter gen-l10n)
Files:
lib/l10n/intl_it.arblib/l10n/intl_en.arblib/l10n/intl_es.arb
**/*.mocks.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.mocks.dart: Never manually edit *.mocks.dart Mockito-generated files
Do not add ignore comments to generated mock files; regenerate instead
Files:
test/mocks.mocks.dart
test/mocks.mocks.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
test/mocks.mocks.dart: Do not add individual // ignore: must_be_immutable comments; rely on the existing file-level ignore
Never manually edit test/mocks.mocks.dart; regenerate with build_runner when needed
Files:
test/mocks.mocks.dart
lib/**/@(screens|widgets)/**/*.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs) instead of inline side effects in build methods
Files:
lib/features/relays/widgets/relay_selector.dart
lib/features/*/{screens,providers,notifiers,widgets}/**/*
📄 CodeRabbit Inference Engine (CLAUDE.md)
Follow the feature-based structure: features/{feature}/{screens|providers|notifiers|widgets}/
Files:
lib/features/relays/widgets/relay_selector.dart
🧠 Learnings (5)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mobile#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.
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to test/mocks.mocks.dart : Do not add individual // ignore: must_be_immutable comments; rely on the existing file-level ignore
Applied to files:
test/features/relays/widgets/relay_selector_test.darttest/mocks.mocks.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to test/mocks.mocks.dart : Never manually edit test/mocks.mocks.dart; regenerate with build_runner when needed
Applied to files:
test/features/relays/widgets/relay_selector_test.darttest/mocks.mocks.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to **/*.mocks.dart : Never manually edit *.mocks.dart Mockito-generated files
Applied to files:
test/features/relays/widgets/relay_selector_test.dart
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
PR: MostroP2P/mobile#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.dart
🪛 RuboCop (1.76.1)
lib/l10n/intl_en.arb
[warning] 856-856: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
[warning] 857-857: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
[warning] 858-858: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
[warning] 859-859: Duplicated key in hash literal.
(Lint/DuplicateHashKey)
🔇 Additional comments (5)
lib/l10n/intl_it.arb (1)
821-893: Italian additions look consistent and preciseThe Add Relay, status, blacklist, and last-relay strings read well and align with the new flows. Placeholders are correctly declared where needed.
lib/features/relays/relay.dart (1)
81-103: Derived getters align with UX constraintsisAutoDiscovered, canDelete, and canBlacklist match the intended behavior for Mostro/default vs. user relays. Equality/hash by URL is also appropriate once URLs are normalized.
lib/l10n/intl_es.arb (1)
814-885: Spanish additions look good and match en/it semanticsAdd Relay, Mostro reset, status, blacklist, and last-relay protection strings are consistent. Placeholders for {url} are present where needed.
lib/features/relays/relays_screen.dart (1)
29-32: Wrapping content in a scroll view is a good UX improvementSingleChildScrollView with padding works well for variable-length relay lists.
test/mocks.mocks.dart (1)
1-6: Confirm this file is auto-generated; do not edit manuallyLooks consistent with Mockito 5.4.6 output. If any edits were manual (including ignore annotations), please regenerate via build_runner.
Also applies to: 817-821
| if (newPubkey != currentPubkey && | ||
| currentPubkey != null && | ||
| newPubkey.isNotEmpty && | ||
| currentPubkey!.isNotEmpty) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pubkey comparison uses potentially unstable null checks
The null check logic at Line 639 could fail if currentPubkey is initially null but later becomes an empty string.
Simplify the logic to handle all edge cases:
-// Only reset if there's a REAL change (both values are non-empty and different)
-if (newPubkey != currentPubkey &&
- currentPubkey != null &&
- newPubkey.isNotEmpty &&
- currentPubkey!.isNotEmpty) {
+// Only reset if there's a REAL change (both values are non-empty and different)
+if (newPubkey != currentPubkey &&
+ currentPubkey?.isNotEmpty == true &&
+ newPubkey.isNotEmpty) {📝 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.
| if (newPubkey != currentPubkey && | |
| currentPubkey != null && | |
| newPubkey.isNotEmpty && | |
| currentPubkey!.isNotEmpty) { | |
| // Only reset if there's a REAL change (both values are non-empty and different) | |
| if (newPubkey != currentPubkey && | |
| currentPubkey?.isNotEmpty == true && | |
| newPubkey.isNotEmpty) { | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In lib/features/relays/relays_notifier.dart around lines 638 to 641, the pubkey
comparison uses fragile null checks (currentPubkey != null and
currentPubkey!.isNotEmpty) which can misbehave if currentPubkey transitions
between null and empty; change the logic to treat null as an empty string for
comparison (e.g. compare newPubkey to (currentPubkey ?? '') and require
newPubkey.isNotEmpty and (currentPubkey ?? '').isNotEmpty) and remove any forced
unwrapping so the check is stable for all null/empty states.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/features/relays/widgets/relay_selector.dart (2)
13-14: Watch the provider’s state (not the notifier) so the widget rebuilds on changesWatching
relaysProvider.notifierwon’t subscribe this widget to state updates; the list and switches won’t refresh. Watch the provider state (or aselect) and read the notifier separately.Apply one of these diffs (Option A is the safest drop-in):
- final relaysNotifier = ref.watch(relaysProvider.notifier); - final mostroRelays = relaysNotifier.mostroRelaysWithStatus; + // Rebuild on state changes + final _ = ref.watch(relaysProvider); + // Mutations / derived data via notifier + final relaysNotifier = ref.read(relaysProvider.notifier); + final mostroRelays = relaysNotifier.mostroRelaysWithStatus;Or, if
mostroRelaysWithStatusis exposed on state:- final relaysNotifier = ref.watch(relaysProvider.notifier); - final mostroRelays = relaysNotifier.mostroRelaysWithStatus; + final mostroRelays = ref.watch( + relaysProvider.select((s) => s.mostroRelaysWithStatus), + ); + final relaysNotifier = ref.read(relaysProvider.notifier);
295-305: Success SnackBar likely never shows: using dialog’s inner context after popYou capture
ScaffoldMessengerfrom the dialog’s inner context, thenpop(). After pop, that inner context is unmounted, making the mounted check fail and skipping the SnackBar.Apply this diff to capture the outer context and use it for SnackBar after closing the dialog. Also, avoid reusing the name
contextin the inner builder to reduce confusion.Future<void> showAddDialog(BuildContext context, WidgetRef ref) async { - final textController = TextEditingController(); + final outerContext = context; // preserve a safe context outside the dialog + final textController = TextEditingController(); @@ - builder: (BuildContext dialogContext) { + builder: (BuildContext dialogContext) { return StatefulBuilder( - builder: (context, setState) { + builder: (innerContext, setState) { return AlertDialog( @@ - final localizations = S.of(context)!; - final scaffoldMessenger = ScaffoldMessenger.of(context); + final localizations = S.of(innerContext)!; // for strings while dialog is up + final scaffoldMessenger = ScaffoldMessenger.of(outerContext); // for SnackBar after pop final navigator = Navigator.of(dialogContext); @@ - if (result.success) { - navigator.pop(); - if (context.mounted) { + if (result.success) { + navigator.pop(); + if (!outerContext.mounted) return; scaffoldMessenger.showSnackBar( SnackBar( content: Text( localizations.addRelaySuccessMessage(result.normalizedUrl!), style: const TextStyle(color: Colors.white), ), backgroundColor: Colors.green, ), ); - } } else {Also applies to: 306-307, 381-385, 401-413
🧹 Nitpick comments (7)
lib/features/relays/widgets/relay_selector.dart (7)
135-143: Avoid fixed width for status area; make it responsive and localization-proofA hardcoded width (140) risks overflow with longer localized strings. Prefer Flexible + ellipsis.
- Container( - width: 140, // Increased width to show full text - padding: const EdgeInsets.only(right: 16), // Prevent overflow - child: Row( + Flexible( + child: Row( mainAxisAlignment: MainAxisAlignment.start, children: [ _buildRelaySwitch(context, ref, relayInfo), const SizedBox(width: 8), - Expanded( + Flexible( child: Text( relayInfo.isActive ? S.of(context)!.activated : S.of(context)!.deactivated, style: const TextStyle( color: AppTheme.textSecondary, // Use same grey as description fontSize: 12, fontWeight: FontWeight.w600, ), + overflow: TextOverflow.ellipsis, ), ), ], ), - ), + ),
160-193: Custom toggle lacks accessibility; preferSwitch.adaptiveor add proper semanticsThe bespoke switch isn’t focusable and doesn’t announce as a switch to screen readers. Use the platform switch or wrap with Semantics/Focus to meet a11y.
Option 1 (preferred – replaces custom control):
- return GestureDetector( - onTap: () async { - await _handleRelayToggle(context, ref, relayInfo); - }, - child: Container( - width: 50, - height: 26, - padding: const EdgeInsets.all(2), - decoration: BoxDecoration( - color: isActive ? AppTheme.activeColor : AppTheme.red1, - borderRadius: BorderRadius.circular(13), - ), - child: AnimatedAlign( - duration: const Duration(milliseconds: 200), - alignment: isActive ? Alignment.centerRight : Alignment.centerLeft, - child: Container( - width: 22, - height: 22, - decoration: BoxDecoration( - color: Colors.white, - borderRadius: BorderRadius.circular(11), - border: Border.all( - color: Colors.black, - width: 2, - ), - ), - ), - ), - ), - ); + return Switch.adaptive( + value: isActive, + activeColor: AppTheme.activeColor, + inactiveTrackColor: AppTheme.red1, + onChanged: (_) async => _handleRelayToggle(context, ref, relayInfo), + );Option 2 (if you must keep the custom visuals), minimally add semantics/focus:
- return GestureDetector( + return FocusableActionDetector( + autofocus: false, + descendantsAreFocusable: false, + onShowFocusHighlight: (_) {}, + child: Semantics( + role: Role.switch, + onTap: () async => _handleRelayToggle(context, ref, relayInfo), + toggled: isActive, + label: relayInfo.url, + child: GestureDetector( onTap: () async { await _handleRelayToggle(context, ref, relayInfo); }, child: Container( width: 50, height: 26, padding: const EdgeInsets.all(2), decoration: BoxDecoration( color: isActive ? AppTheme.activeColor : AppTheme.red1, borderRadius: BorderRadius.circular(13), ), child: AnimatedAlign( duration: const Duration(milliseconds: 200), alignment: isActive ? Alignment.centerRight : Alignment.centerLeft, child: Container( width: 22, height: 22, decoration: BoxDecoration( color: Colors.white, borderRadius: BorderRadius.circular(11), border: Border.all( color: Colors.black, width: 2, ), ), ), ), ), - ); + ), + ), + );
198-199: Make “default Mostro relay” detection robust using URI parsingString prefix checks can misclassify URLs (e.g., case, trailing slashes, paths). Parse and compare host.
- final isDefaultMostroRelay = relayInfo.url.startsWith('wss://relay.mostro.network'); + final host = Uri.tryParse(relayInfo.url)?.host ?? ''; + final isDefaultMostroRelay = host.toLowerCase() == 'relay.mostro.network';Note: Per the retrieved learning, keeping a hardcoded bootstrap default is intentional; this change only hardens detection without altering that behavior.
296-303: Dispose TextEditingController to avoid leaksThe controller is created outside the dialog widget tree and never disposed. Wrap the
showDialogcall in try/finally and dispose afterward.Future<void> showAddDialog(BuildContext context, WidgetRef ref) async { - final textController = TextEditingController(); + final textController = TextEditingController(); @@ - await showDialog<void>( - context: context, - barrierDismissible: false, - builder: (BuildContext dialogContext) { - return StatefulBuilder( - builder: (context, setState) { - return AlertDialog( + try { + await showDialog<void>( + context: context, + barrierDismissible: false, + builder: (BuildContext dialogContext) { + return StatefulBuilder( + builder: (context, setState) { + return AlertDialog( // ... - ); - }, - ); - }, - ); + ); + }, + ); + }, + ); + } finally { + textController.dispose(); + } }Also applies to: 439-441
369-371: Nit: simplify redundant ternary in Text styleBoth branches return the same color.
- style: TextStyle( - color: isLoading ? AppTheme.textSecondary : AppTheme.textSecondary, - ), + style: const TextStyle( + color: AppTheme.textSecondary, + ),
322-341: Nit: improve TextField ergonomics for URL inputSmall UX wins for URL entry.
TextField( controller: textController, enabled: !isLoading, style: const TextStyle(color: AppTheme.cream1), + keyboardType: TextInputType.url, + textInputAction: TextInputAction.done, + autocorrect: false, + enableSuggestions: false, decoration: InputDecoration(
202-208: Avoid sentinelRelay(url: ''); prefer nullable lookupCreating a fake
Relayinstance to satisfyorElseobscures intent. Use a nullable lookup (e.g.,firstWhereOrNull) and test against null.- final relay = currentRelays.firstWhere( - (r) => r.url == relayInfo.url, - orElse: () => Relay(url: ''), // Empty relay if not found - ); - final isUserRelay = relay.url.isNotEmpty && relay.source == RelaySource.user; + final relay = currentRelays.firstWhereOrNull((r) => r.url == relayInfo.url); + final isUserRelay = relay?.source == RelaySource.user;Add this import at the top of the file (outside the selected range):
import 'package:collection/collection.dart'; // for firstWhereOrNullIf you’d rather avoid the dependency, manually loop to find the match and use a nullable local instead of constructing a dummy instance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
lib/features/relays/widgets/relay_selector.dart(1 hunks)lib/features/settings/settings.dart(4 hunks)lib/l10n/intl_en.arb(2 hunks)lib/l10n/intl_es.arb(2 hunks)lib/l10n/intl_it.arb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/l10n/intl_en.arb
- lib/l10n/intl_it.arb
- lib/features/settings/settings.dart
- lib/l10n/intl_es.arb
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/@(screens|widgets)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs) instead of inline side effects in build methods
Files:
lib/features/relays/widgets/relay_selector.dart
lib/!(generated)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/relays/widgets/relay_selector.dart
lib/features/*/{screens,providers,notifiers,widgets}/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the feature-based structure: features/{feature}/{screens|providers|notifiers|widgets}/
Files:
lib/features/relays/widgets/relay_selector.dart
🧠 Learnings (3)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mobile#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.
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/**/@(screens|widgets)/**/*.dart : Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs) instead of inline side effects in build methods
Applied to files:
lib/features/relays/widgets/relay_selector.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/!(generated)/**/*.dart : Check mounted before using BuildContext after async gaps
Applied to files:
lib/features/relays/widgets/relay_selector.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
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/features/relays/relays_notifier.dart (2)
167-262: Critical: test cleanup may disconnect the global Nostr instanceIn the catch block you call _cleanupTestConnection(Nostr.instance). If dart_nostr uses a shared singleton, this could disconnect the app’s primary Nostr connection when a test fails. Keep a handle to the test instance and always clean it up in finally. Also remove the singleton cleanup.
- Future<bool> _testNostrProtocol(String url) async { - // Generate unique subscription ID for this test + Future<bool> _testNostrProtocol(String url) async { + // Generate unique subscription ID for this test final testSubId = 'relay_test_${DateTime.now().millisecondsSinceEpoch}'; bool receivedEvent = false; bool receivedEose = false; bool isConnected = false; - - try { - // Create isolated instance for testing - final testNostr = Nostr(); + Nostr? testNostr; + try { + // Create isolated instance for testing + testNostr = Nostr(); @@ - await _cleanupTestConnection(testNostr); + await _cleanupTestConnection(testNostr); @@ - } catch (e) { - // Protocol test failed with error - try { - await _cleanupTestConnection(Nostr.instance); - } catch (_) { - // Ignore cleanup errors - } - return false; - } + } catch (e) { + // Protocol test failed with error + return false; + } finally { + if (testNostr != null) { + try { + await _cleanupTestConnection(testNostr); + } catch (_) {} + } + }
355-360: Duplicate detection and blacklist removal should use canonical URLsComparing raw URLs can miss duplicates and blacklist hits. Canonicalize first to ensure consistent behavior.
- // Step 2: Check for duplicates - if (state.any((relay) => relay.url == normalizedUrl)) { + // Step 2: Check for duplicates (canonical) + final normalizedKey = _normalizeRelayUrl(normalizedUrl); + if (state.any((relay) => _normalizeRelayUrl(relay.url) == normalizedKey)) { return RelayValidationResult( success: false, error: errorAlreadyExists, ); } @@ - if (settings.state.blacklistedRelays.contains(normalizedUrl)) { - await settings.removeFromBlacklist(normalizedUrl); + final bl = settings.state.blacklistedRelays.map(_normalizeRelayUrl).toSet(); + if (bl.contains(normalizedKey)) { + await settings.removeFromBlacklist(normalizedKey); _logger.i('Removed $normalizedUrl from blacklist - user manually added it'); }Also applies to: 374-377
♻️ Duplicate comments (3)
lib/features/relays/relays_notifier.dart (3)
52-61: Bootstrap default relay: acknowledged and OKKeeping the hardcoded Mostro relay for bootstrap is intentional per prior discussion and learning, and it’s appropriate here.
472-484: Retry sync timer can accumulate; store and cancel itRepeated calls to _scheduleRetrySync will create multiple timers. Track the timer and cancel before rescheduling. Also cancel in dispose().
- void _scheduleRetrySync(String mostroPubkey) { - Timer(const Duration(seconds: 10), () async { + void _scheduleRetrySync(String mostroPubkey) { + _retrySyncTimer?.cancel(); + _retrySyncTimer = Timer(const Duration(seconds: 10), () async { try { if (settings.state.mostroPublicKey == mostroPubkey) { _logger.i('Retrying relay sync for Mostro: $mostroPubkey'); _subscriptionManager?.subscribeToMostroRelayList(mostroPubkey); } } catch (e) { _logger.w('Retry sync failed: $e'); } }); }And in dispose():
_subscriptionManager?.dispose(); _settingsWatchTimer?.cancel(); + _retrySyncTimer?.cancel();
631-658: Stabilize pubkey change detection for null/empty transitionsThe current checks can be fragile when transitioning between null/empty. Simplify and avoid forced unwrap.
- if (newPubkey != currentPubkey && - currentPubkey != null && - newPubkey.isNotEmpty && - currentPubkey!.isNotEmpty) { + if (newPubkey != currentPubkey && + (currentPubkey?.isNotEmpty == true) && + newPubkey.isNotEmpty) {
🧹 Nitpick comments (6)
lib/features/relays/relays_notifier.dart (6)
29-35: Track retry timer to prevent duplicates and leaksFields look good. Please also keep a handle for the retry timer you create later so you can cancel/replace it.
final _logger = Logger(); SubscriptionManager? _subscriptionManager; StreamSubscription<RelayListEvent>? _relayListSubscription; Timer? _settingsWatchTimer; + Timer? _retrySyncTimer;
396-407: Health flag is hardcoded to trueIf this is temporary, add a TODO and consider deriving from NostrService connection state to avoid misleading UIs.
649-656: Use English-only comments for consistencyReplace Spanish comments with English to keep the codebase consistent.
- // 🔥 RESET COMPLETO: Limpiar todos los relays y hacer sync fresco + // Full reset: clear all relays and perform a fresh sync @@ - // 🔥 LIMPIAR TODOS los relays (solo mantener default) + // Clear all relays (keep only the default) @@ - // Iniciar sync completamente fresco con nuevo Mostro + // Start a completely fresh sync with the new MostroAlso applies to: 665-674
794-802: Make _normalizeRelayUrl a canonicalizer (lowercase scheme/host, strip trailing slash)This ensures consistent comparisons across the file and avoids case/trailing-slash issues. It does not modify scheme/host presence; validation remains in normalizeRelayUrl for user input.
String _normalizeRelayUrl(String url) { - url = url.trim(); - // Remove trailing slash if present - if (url.endsWith('/')) { - url = url.substring(0, url.length - 1); - } - return url; + final trimmed = url.trim(); + Uri? uri; + try { + uri = Uri.parse(trimmed); + } catch (_) { + return trimmed.endsWith('/') ? trimmed.substring(0, trimmed.length - 1) : trimmed; + } + final scheme = uri.scheme.isNotEmpty ? uri.scheme.toLowerCase() : uri.scheme; + final host = uri.host.isNotEmpty ? uri.host.toLowerCase() : uri.host; + final normalized = Uri( + scheme: scheme, + userInfo: uri.userInfo, + host: host, + port: uri.hasPort ? uri.port : null, + path: (uri.path == '/' ? '' : uri.path), + query: uri.hasQuery ? uri.query : null, + ).toString(); + return normalized.endsWith('/') ? normalized.substring(0, normalized.length - 1) : normalized; }
804-810: Dispose: also cancel retry timerAdd cancellation for the retry timer introduced above.
_subscriptionManager?.dispose(); _settingsWatchTimer?.cancel(); + _retrySyncTimer?.cancel(); super.dispose();
114-127: Unify URL canonicalization and validation responsibilitiesYou currently maintain two distinct normalization paths—
normalizeRelayUrl(trims, lowercases, enforceswss://, and validates) and_normalizeRelayUrl(trims and strips trailing slashes). To avoid subtle mismatches:
- Refactor
_normalizeRelayUrlinto a pure canonicalizer:
- Lowercase the scheme and host
- Strip any trailing slash
- Leave validation (protocol checks, domain/IP format) to a separate step
- Reserve
normalizeRelayUrlfor input validation and secure‐upgrade logic:
- Trim and lowercase purely for comparison
- Reject non‐secure protocols (
ws://,http://)- Auto‐prepend
wss://when appropriatePer the NIP-65 spec, IP‐based hosts (e.g.
wss://192.0.2.10) are technically allowed, though domain names remain the recommended best practice for public relays. Your currentisValidDomainFormat()rejects IP literals—if you plan to support private or experimental IP‐based relays, consider extending it to accept both IPv4 dotted quads and IPv6 address literals.Locations to address:
- lib/features/relays/relays_notifier.dart: lines 114–127 (refactor
normalizeRelayUrl)- lib/features/relays/relays_notifier.dart: lines 129–151 (ensure
_normalizeRelayUrland validation stay in sync)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/features/relays/relays_notifier.dart(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
lib/!(generated)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/relays/relays_notifier.dart
🧠 Learnings (2)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mobile#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.
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
PR: MostroP2P/mobile#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.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/features/relays/relays_notifier.dart (1)
35-42: Constructor flow LGTMMicrotask deferral to avoid provider init cycles and the early init calls are sensible.
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 (3)
lib/features/relays/relays_notifier.dart (3)
114-151: Unify normalization: makenormalizeRelayUrlcanonical and use it everywhere.You have both
normalizeRelayUrl(secure, lowercase) and_normalizeRelayUrl(trim/trailing slash). Consolidate into a single canonical function; also strip trailing slash here to avoid a second helper.String? normalizeRelayUrl(String input) { - input = input.trim().toLowerCase(); + input = input.trim().toLowerCase(); + // Remove trailing slash early + if (input.endsWith('/')) { + input = input.substring(0, input.length - 1); + } @@ - } else { - return 'wss://$input'; // Auto-add wss:// prefix - } + } else { + var out = 'wss://$input'; // Auto-add wss:// prefix + // Ensure no trailing slash after adding scheme + if (out.endsWith('/')) out = out.substring(0, out.length - 1); + return out; + } }Then make
_normalizeRelayUrla thin delegator (see comment at lines 797-806).
354-361: Duplicate detection should use canonical URLs.Comparing raw
relay.urltonormalizedUrlcan miss duplicates. Canonicalize both sides.- if (state.any((relay) => relay.url == normalizedUrl)) { + if (state.any((relay) => normalizeRelayUrl(relay.url) == normalizedUrl)) { return RelayValidationResult( success: false, error: errorAlreadyExists, ); }
166-262: Ensure proper cleanup of the localtestNostrinstance in_testNostrProtocolThe current implementation accidentally calls
_cleanupTestConnection(Nostr.instance)in thecatchblock—leaving the locally createdtestNostralive on errors and potentially leaking resources. We should:• Declare
testNostrbefore thetryso it’s visible incatch/finally.
• Remove the nested cleanup insidecatch.
• Add afinallyblock that always invokes_cleanupTestConnection(testNostr)if it was initialized.The handling of EOSE messages already uses
NostrRequestEoseCommand, which matches the class defined inlib/services/nostr_service.dart.Suggested diff:
Future<bool> _testNostrProtocol(String url) async { final testSubId = 'relay_test_${DateTime.now().millisecondsSinceEpoch}'; bool receivedEvent = false; bool receivedEose = false; bool isConnected = false; - try { - // Create isolated instance for testing - final testNostr = Nostr(); + Nostr? testNostr; + try { + // Create isolated instance for testing + testNostr = Nostr(); await testNostr.services.relays.init( relaysUrl: [url], … ); @@ - } catch (e) { - // Protocol test failed with error - try { - await _cleanupTestConnection(Nostr.instance); - } catch (_) { - // Ignore cleanup errors - } - return false; - } + } catch (e) { + // Protocol test failed + return false; + } finally { + if (testNostr != null) { + try { + await _cleanupTestConnection(testNostr); + } catch (_) { + // Ignore cleanup errors + } + } + } }
♻️ Duplicate comments (13)
lib/features/relays/widgets/relay_selector.dart (1)
345-351: SnackBar may never show: using dialog’s inner context after pop.You capture
ScaffoldMessengerfrom the dialog’s inner context, thennavigator.pop()unmounts it, socontext.mountedis false and the SnackBar doesn’t show. Capture the outer widget context before opening the dialog and use that to display the SnackBar after closing the dialog.Future<void> showAddDialog(BuildContext context, WidgetRef ref) async { - final textController = TextEditingController(); + final outerContext = context; // preserve a safe context outside the dialog + final textController = TextEditingController(); @@ - builder: (context, setState) { + builder: (innerContext, setState) { @@ - final localizations = S.of(context)!; - final scaffoldMessenger = ScaffoldMessenger.of(context); + final localizations = S.of(innerContext)!; + // Use outer context so it's still mounted after pop + final scaffoldMessenger = ScaffoldMessenger.of(outerContext); final navigator = Navigator.of(dialogContext); @@ - if (result.success) { - navigator.pop(); - if (context.mounted) { + if (result.success) { + navigator.pop(); + if (!outerContext.mounted) return; scaffoldMessenger.showSnackBar( SnackBar( content: Text( localizations.addRelaySuccessMessage(result.normalizedUrl!), style: const TextStyle(color: Colors.white), ), backgroundColor: Colors.green, ), ); - } } else {Also applies to: 447-479
lib/features/relays/relay.dart (1)
47-59: Defaulting isHealthy to false in fromJson conflicts with constructor default.This will mark relays unhealthy when the key is absent (e.g., after migrations). Also,
addedAtparsing only accepts ints; older persisted values may be ISO strings. Align defaults and broaden parsing.factory Relay.fromJson(Map<String, dynamic> json) { return Relay( url: json['url'] as String, - isHealthy: json['isHealthy'] as bool? ?? false, + isHealthy: json['isHealthy'] as bool? ?? true, source: RelaySource.values.firstWhere( (e) => e.name == json['source'], orElse: () => RelaySource.user, ), - addedAt: json['addedAt'] != null - ? DateTime.fromMillisecondsSinceEpoch(json['addedAt'] as int) - : null, + addedAt: (() { + final v = json['addedAt']; + if (v == null) return null; + if (v is int) return DateTime.fromMillisecondsSinceEpoch(v); + if (v is String) return DateTime.tryParse(v); + return null; + })(), ); }lib/features/relays/relays_notifier.dart (11)
52-55: Bootstrap default relay is acceptable (intentional design).Acknowledging the hardcoded default 'wss://relay.mostro.network' for bootstrap. This matches the documented intent and current Config contents. No change requested.
631-658: Pubkey change detection: simplify null/empty logic; translate non-English comments.
- Simplify null/empty checks to avoid forced unwraps.
- Comments contain Spanish; guidelines require English-only code comments.
- // Only reset if there's a REAL change (both values are non-empty and different) - if (newPubkey != currentPubkey && - currentPubkey != null && - newPubkey.isNotEmpty && - currentPubkey!.isNotEmpty) { + // Only reset if there's a real change (both values are non-empty and different) + if (newPubkey != currentPubkey && + (currentPubkey?.isNotEmpty == true) && + newPubkey.isNotEmpty) { _logger.i('Detected REAL Mostro pubkey change: $currentPubkey -> $newPubkey'); currentPubkey = newPubkey; - - // 🔥 RESET COMPLETO: Limpiar todos los relays y hacer sync fresco + // Full reset: clear relays and perform a fresh sync _cleanAllRelaysAndResync(); } else if (newPubkey != currentPubkey) { - // Just update the tracking variable without reset (initial load) + // Initial load: update the tracking variable and sync#!/bin/bash # Verify no remaining non-English comment fragments rg -nP "Limpiar|RESET COMPLETO|Iniciar sync|LIMPIAR" --type=dart -C1
743-754: Simulated “no active relays” check should use canonicalization to be accurate.Avoid raw string comparisons.
bool wouldLeaveNoActiveRelays(String urlToBlacklist) { - final currentActiveRelays = state.map((r) => r.url).toList(); - final currentBlacklist = settings.state.blacklistedRelays; + final currentActiveRelays = state.map((r) => normalizeRelayUrl(r.url)!).toList(); + final currentBlacklist = settings.state.blacklistedRelays + .map(normalizeRelayUrl) + .whereType<String>() + .toList(); @@ - final wouldBeBlacklisted = [...currentBlacklist, urlToBlacklist]; + final key = normalizeRelayUrl(urlToBlacklist) ?? urlToBlacklist; + final wouldBeBlacklisted = [...currentBlacklist, key]; final wouldRemainActive = currentActiveRelays.where((url) => !wouldBeBlacklisted.contains(url)).toList();
807-813: Remember to cancel the new retry timer in dispose().If you adopt the retry timer tracking earlier, include its cancellation here (see diff in the retry comment).
74-97: Persisting relays/blacklist without canonicalization risks mismatches and duplicates.Currently compares raw URLs against the blacklist and saves raw URLs. Canonicalize both sides to avoid misses (trailing slash, case) and persist deduped canonical values.
Apply this diff:
Future<void> _saveRelays() async { - // Get blacklisted relays - final blacklistedUrls = settings.state.blacklistedRelays; + // Canonical blacklist + final blacklistedUrls = settings.state.blacklistedRelays + .map(normalizeRelayUrl) + .whereType<String>() + .toSet(); @@ - // Include ALL active relays (Mostro/default + user) that are NOT blacklisted - final allActiveRelayUrls = state - .where((r) => !blacklistedUrls.contains(r.url)) - .map((r) => r.url) - .toList(); + // Include ALL active relays (Mostro/default + user) that are NOT blacklisted (canonical) + final allActiveRelayUrls = state + .where((r) => !blacklistedUrls.contains(normalizeRelayUrl(r.url))) + .map((r) => normalizeRelayUrl(r.url)!) + .toSet() // dedupe + .toList();
472-484: Retry Timer can multiply; track and cancel to avoid leaks/duplication.Repeated calls to
_scheduleRetrySyncwill create overlapping timers. Store the timer and cancel before scheduling anew; cancel indispose().- void _scheduleRetrySync(String mostroPubkey) { - Timer(const Duration(seconds: 10), () async { + Timer? _retrySyncTimer; // add as a field (top of class) + + void _scheduleRetrySync(String mostroPubkey) { + _retrySyncTimer?.cancel(); + _retrySyncTimer = Timer(const Duration(seconds: 10), () async { try { if (settings.state.mostroPublicKey == mostroPubkey) { _logger.i('Retrying relay sync for Mostro: $mostroPubkey'); _subscriptionManager?.subscribeToMostroRelayList(mostroPubkey); } } catch (e) { _logger.w('Retry sync failed: $e'); } }); }Add to dispose:
void dispose() { _relayListSubscription?.cancel(); _subscriptionManager?.dispose(); _settingsWatchTimer?.cancel(); + _retrySyncTimer?.cancel(); super.dispose(); }
596-602: Change detection should compare sets of canonical URLs, not object equality.List equality and
state.contains(relay)rely onRelayequality and order. Use sets of canonical URLs to detect add/remove accurately.- // Update state if there are changes - if (finalRelays.length != state.length || - !finalRelays.every((relay) => state.contains(relay))) { - state = finalRelays; - await _saveRelays(); - _logger.i('Updated relay list with ${finalRelays.length} relays (${blacklistedUrls.length} blacklisted)'); - } + // Update state if URL sets differ (canonical) + final newUrls = finalRelays.map((r) => normalizeRelayUrl(r.url)!).toSet(); + final oldUrls = state.map((r) => normalizeRelayUrl(r.url)!).toSet(); + final hasChanges = newUrls.length != oldUrls.length || + !newUrls.containsAll(oldUrls) || + !oldUrls.containsAll(newUrls); + if (hasChanges) { + state = finalRelays; + await _saveRelays(); + _logger.i('Updated relay list with ${finalRelays.length} relays (${blacklistedUrls.length} blacklisted)'); + }
610-627: Blacklist/remove operations should use canonical URLs.Using raw strings can miss the target relay (format differences). Canonicalize URL and look up by canonical form.
Future<void> removeRelayWithBlacklist(String url) async { - final relay = state.firstWhere((r) => r.url == url, orElse: () => Relay(url: '')); + final key = normalizeRelayUrl(url) ?? url; + final relay = state.firstWhere( + (r) => normalizeRelayUrl(r.url) == key, + orElse: () => Relay(url: ''), + ); @@ - await settings.addToBlacklist(url); + await settings.addToBlacklist(key); @@ - await removeRelay(url); + await removeRelay(key); }
681-684: Blacklist check should canonicalize.Direct
contains(url)may miss due to formatting. Canonicalize before lookup.bool isRelayBlacklisted(String url) { - return settings.state.blacklistedRelays.contains(url); + final key = normalizeRelayUrl(url) ?? url; + return settings.state.blacklistedRelays + .map(normalizeRelayUrl) + .whereType<String>() + .toSet() + .contains(key); }
692-741: UI view composition mixes user/Mostro in blacklist-only items; also needs canonicalization throughout.
- Canonicalize blacklisted and active sets.
- Track the last Mostro relay set to categorize blacklisted-only entries correctly (avoid mislabeling user-blacklisted URLs as “Mostro/default”).
List<MostroRelayInfo> get mostroRelaysWithStatus { - final blacklistedUrls = settings.state.blacklistedRelays; - final activeRelays = state.map((r) => r.url).toSet(); + final blacklistedUrls = settings.state.blacklistedRelays + .map(normalizeRelayUrl) + .whereType<String>() + .toSet(); + final activeRelays = state.map((r) => normalizeRelayUrl(r.url)!).toSet(); @@ - final mostroAndDefaultActiveRelays = state + final mostroAndDefaultActiveRelays = state .where((r) => r.source == RelaySource.mostro || r.source == RelaySource.defaultConfig) .map((r) => MostroRelayInfo( url: r.url, - // Check if this relay is blacklisted (even if it's still in state) - isActive: !blacklistedUrls.contains(r.url), + // Check if this relay is blacklisted (even if it's still in state) + isActive: !blacklistedUrls.contains(normalizeRelayUrl(r.url)), isHealthy: r.isHealthy, source: r.source, )) .toList(); @@ - final mostroBlacklistedRelays = blacklistedUrls - .where((url) => !activeRelays.contains(url)) + final mostroBlacklistedRelays = blacklistedUrls + .where((url) => !activeRelays.contains(url) && _lastMostroRelays.contains(url)) .map((url) => MostroRelayInfo( url: url, isActive: false, isHealthy: false, source: null, // Unknown source for blacklisted-only relays )) .toList(); @@ final userRelays = state .where((r) => r.source == RelaySource.user) .map((r) => MostroRelayInfo( url: r.url, - isActive: !blacklistedUrls.contains(r.url), // User relays can also be blacklisted + isActive: !blacklistedUrls.contains(normalizeRelayUrl(r.url)), // User relays can also be blacklisted isHealthy: r.isHealthy, source: r.source, )) .toList();Add this field and set it in the event handler (outside this range):
// At class level: Set<String> _lastMostroRelays = {}; // In _handleMostroRelayListUpdate, after building normalizedRelays: _lastMostroRelays = normalizedRelays.map((u) => normalizeRelayUrl(u)!).toSet();
759-773: Toggle blacklist should canonicalize URL on both add/remove and state removal.Prevents mismatches when formats differ.
Future<void> toggleMostroRelayBlacklist(String url) async { - final isCurrentlyBlacklisted = settings.state.blacklistedRelays.contains(url); + final key = normalizeRelayUrl(url) ?? url; + final isCurrentlyBlacklisted = settings.state.blacklistedRelays + .map(normalizeRelayUrl) + .whereType<String>() + .toSet() + .contains(key); @@ - await settings.removeFromBlacklist(url); + await settings.removeFromBlacklist(key); _logger.i('Removed $url from blacklist, triggering re-sync'); await syncWithMostroInstance(); } else { // Add to blacklist and remove from current state - await settings.addToBlacklist(url); - await removeRelay(url); + await settings.addToBlacklist(key); + await removeRelay(key); _logger.i('Blacklisted and removed Mostro relay: $url'); } }
🧹 Nitpick comments (11)
lib/features/relays/widgets/relay_selector.dart (3)
3-3: Remove unused import.
lucide_iconsis not used. Drop the import to keep the file clean and satisfy lints.-import 'package:lucide_icons/lucide_icons.dart';
248-249: Avoid hardcoding the default bootstrap relay host.
relayInfo.url.startsWith('wss://relay.mostro.network')couples UI to a specific host. If the bootstrap relay changes, users won’t get the protection dialog. Prefer deriving this from configuration/notifier (e.g.,relaysNotifier.isDefaultBootstrap(relayInfo.url)or expose the default URL from a config/provider).I can wire this via
RelaysNotifieror a smallConfig.defaultBootstrapRelayprovider if you’d like.
168-201: Consider using a real Switch for accessibility and semantics.The custom toggle looks good but lacks semantics, focus behavior, and platform accessibility.
SwitchorSwitchListTile.adaptivewill give correct semantics and better a11y out of the box while still allowing custom theming.lib/features/relays/relay.dart (1)
90-98: Relay instantiation check completeI searched for direct
Relay(url: …)calls and found only the fallback cases that construct an “empty” relay (withurl: ''), which won’t introduce unnormalized URLs. All other relays go through your smart‐validation path.• Fallback constructors bypass normalization (but use
''):
– lib/features/relays/relays_notifier.dart:560 (orElse: () => Relay(url: ''))
– lib/features/relays/relays_notifier.dart:613 (orElse: () => Relay(url: ''))
– lib/features/relays/widgets/relay_selector.dart:255 (orElse: () => Relay(url: ''))Recommendation (optional refactor):
To guarantee URL normalization in all cases, centralize the sanitization inRelay’s constructor or a private factory. That way callers can’t bypass it—even in fallback or test code—and you won’t need to audit each instantiation site.lib/features/relays/relays_notifier.dart (3)
109-112: Remove by canonical URL to avoid stale entries.Comparing raw URLs can fail with formatting differences. Canonicalize both sides before filtering.
- Future<void> removeRelay(String url) async { - state = state.where((r) => r.url != url).toList(); + Future<void> removeRelay(String url) async { + final key = normalizeRelayUrl(url) ?? url; + state = state.where((r) => normalizeRelayUrl(r.url) != key).toList(); await _saveRelays(); }
396-407: Health refresh sets all relays healthy; consider querying actual status.If feasible, derive health from NostrService connections or a ping, otherwise mark as “unknown” and let UI reflect real-time connectivity. Not blocking this PR.
660-679: Translate comments to English for consistency with project guidelines.These inline comments are Spanish-only. Please translate to English.
CLAUDE.md (4)
20-25: Fix minor grammar and clarity in Essential Commands.Add “is” for readability and align bullets.
-### Essential Commands for Code Changes -- **`flutter analyze`** - ✅ **ALWAYS run after any code change** - Mandatory before commits -- **`flutter test`** - ✅ **ALWAYS run after any code change** - Mandatory before commits -- **`dart run build_runner build -d`** - 🟡 **Only when code generation needed** (models, providers, mocks, localization) -- **`flutter test integration_test/`** - 🟡 **Only for significant changes** (core services, main flows) +### Essential Commands for Code Changes +- **`flutter analyze`** — ✅ ALWAYS run after any code change; mandatory before commits +- **`flutter test`** — ✅ ALWAYS run after any code change; mandatory before commits +- **`dart run build_runner build -d`** — 🟡 Only when code generation is needed (models, providers, mocks, localization) +- **`flutter test integration_test/`** — 🟡 Only for significant changes (core services, main flows)
136-141: Docs/code consistency: enforce English-only comments.This guideline is good; there are Spanish comments in relays_notifier.dart. Translate them to English (see code review comment at lines 631-658 and 660-679 in that file).
173-259: Relay Sync docs: reflect canonicalization strategy and annotate code block language.
- You mention “trailing slash removal”; also document lowercasing and
wss://enforcement to match the proposed code.- The fenced block under “Blacklist System” lacks a language tag; add “text”.
-#### **URL Normalization**: All relay URLs are normalized by removing trailing slashes before blacklist comparison to ensure consistent matching regardless of input format +#### **URL Canonicalization**: Relay URLs are lowercased, forced to `wss://`, and trailing slashes removed before comparison and persistence to ensure consistent matching @@ -``` +```text User removes Mostro relay → Added to blacklist → Never re-added during sync User manually adds relay → Removed from blacklist → Works as user relay--- `401-409`: **File list is helpful; add shared_preferences if used by Settings.** Minor completeness tweak if Settings persistence relies on SharedPreferences. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** **💡 Knowledge Base configuration:** - MCP integration is disabled by default for public repositories - Jira integration is disabled by default for public repositories - Linear integration is disabled by default for public repositories You can enable these sources in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 2de36b6a79b21b77e153c382d7a4df81f812ae6c and 132a83b91a7b38ac133a1b9db216526a29c7311c. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `CLAUDE.md` (10 hunks) * `lib/features/relays/relay.dart` (1 hunks) * `lib/features/relays/relays_notifier.dart` (5 hunks) * `lib/features/relays/widgets/relay_selector.dart` (1 hunks) * `lib/l10n/intl_en.arb` (2 hunks) * `lib/l10n/intl_es.arb` (2 hunks) * `lib/l10n/intl_it.arb` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (5)</summary> <details> <summary>lib/l10n/intl_{en,es,it}.arb</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > When adding a localization key, add it to all three ARB files: intl_en.arb, intl_es.arb, and intl_it.arb Files: - `lib/l10n/intl_it.arb` - `lib/l10n/intl_es.arb` - `lib/l10n/intl_en.arb` </details> <details> <summary>lib/l10n/*.arb</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > After modifying ARB files, regenerate localization outputs (e.g., dart run build_runner build -d or flutter gen-l10n) Files: - `lib/l10n/intl_it.arb` - `lib/l10n/intl_es.arb` - `lib/l10n/intl_en.arb` </details> <details> <summary>lib/!(generated)/**/*.dart</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > `lib/!(generated)/**/*.dart`: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text > Pass BuildContext to methods that need localization rather than accessing localization without context > Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity()) > Check mounted before using BuildContext after async gaps > Remove unused imports and dependencies > Use const constructors where possible Files: - `lib/features/relays/relays_notifier.dart` - `lib/features/relays/relay.dart` - `lib/features/relays/widgets/relay_selector.dart` </details> <details> <summary>lib/**/@(screens|widgets)/**/*.dart</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs) instead of inline side effects in build methods Files: - `lib/features/relays/widgets/relay_selector.dart` </details> <details> <summary>lib/features/*/{screens,providers,notifiers,widgets}/**/*</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Follow the feature-based structure: features/{feature}/{screens|providers|notifiers|widgets}/ Files: - `lib/features/relays/widgets/relay_selector.dart` </details> </details><details> <summary>🧠 Learnings (9)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: Catrya
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.</details> <details> <summary>📚 Learning: 2025-08-21T14:45:43.964Z</summary>Learnt from: Catrya
PR: #272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.964Z
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/relays/relays_notifier.dart` - `lib/features/relays/relay.dart` - `lib/features/relays/widgets/relay_selector.dart` - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-08-19T17:54:15.016Z</summary>Learnt from: Catrya
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.dart` - `lib/features/relays/widgets/relay_selector.dart` </details> <details> <summary>📚 Learning: 2025-08-15T13:19:31.999Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib//@(screens|widgets)//*.dart : Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs) instead of inline side effects in build methods**Applied to files:** - `lib/features/relays/widgets/relay_selector.dart` - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-08-15T13:19:31.999Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/!(generated)/**/*.dart : Check mounted before using BuildContext after async gaps**Applied to files:** - `lib/features/relays/widgets/relay_selector.dart` </details> <details> <summary>📚 Learning: 2025-08-15T13:19:31.999Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/!(generated)/**/*.dart : Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text**Applied to files:** - `lib/features/relays/widgets/relay_selector.dart` - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-08-15T13:19:31.999Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Run dart run build_runner build -d after installing dependencies or when code generation is needed**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-08-15T13:19:31.999Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/l10n/*.arb : After modifying ARB files, regenerate localization outputs (e.g., dart run build_runner build -d or flutter gen-l10n)**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-08-15T13:19:31.999Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Run flutter analyze and keep zero issues before committing**Applied to files:** - `CLAUDE.md` </details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>CLAUDE.md</summary> [grammar] ~23-~23: There might be a mistake here. Context: ...nner build -d`** - 🟡 **Only when code generation needed** (models, providers, mocks, loca... (QB_NEW_EN) --- [grammar] ~136-~136: There might be a mistake here. Context: ...rns ### Code Comments and Documentation - **All code comments must be in English** -... (QB_NEW_EN) --- [grammar] ~140-~140: There might be a mistake here. Context: ...uld be in English - User-facing strings use localization system (`S.of(context).key... (QB_NEW_EN) --- [grammar] ~175-~175: There might be a mistake here. Context: ...lay Synchronization System ### Overview Comprehensive system that automatically ... (QB_NEW_EN) --- [grammar] ~180-~180: There might be a mistake here. Context: ...`lib/core/models/relay_list_event.dart`) - Parses NIP-65 (kind 10002) events from M... (QB_NEW_EN) --- [grammar] ~181-~181: There might be a mistake here. Context: ...kind 10002) events from Mostro instances - Validates relay URLs (WebSocket only) - ... (QB_NEW_EN) --- [grammar] ~182-~182: There might be a mistake here. Context: ... - Validates relay URLs (WebSocket only) - Robust handling of different timestamp f... (QB_NEW_EN) --- [grammar] ~183-~183: There might be a mistake here. Context: ... handling of different timestamp formats - Null-safe parsing for malformed events ... (QB_NEW_EN) --- [grammar] ~201-~201: There might be a mistake here. Context: ... (`lib/features/settings/settings.dart`) - New `blacklistedRelays: List<String>` fi... (QB_NEW_EN) --- [grammar] ~202-~202: There might be a mistake here. Context: ... `blacklistedRelays: List<String>` field - Backward-compatible serialization - Auto... (QB_NEW_EN) --- [grammar] ~203-~203: There might be a mistake here. Context: ...ield - Backward-compatible serialization - Automatic migration for existing users ... (QB_NEW_EN) --- [grammar] ~206-~206: There might be a mistake here. Context: ...b/features/relays/relays_notifier.dart`) - **`syncWithMostroInstance()`**: Manual sync trigger - **`removeRelayWi... (QB_NEW_EN) --- [grammar] ~207-~207: There might be a mistake here. Context: ...MostroInstance()`**: Manual sync trigger - **`removeRelayWithBlacklist(String url)`**: Smart removal with blacklisting - **`a... (QB_NEW_EN) --- [grammar] ~208-~208: There might be a mistake here. Context: ...url)`**: Smart removal with blacklisting - **`addRelayWithSmartValidation(...)`**: Auto-removes from blacklist when user ... (QB_NEW_EN) --- [grammar] ~209-~209: There might be a mistake here. Context: ...s from blacklist when user manually adds - **`_handleMostroRelayListUpdate()`**: Filters blacklisted relays during sync... (QB_NEW_EN) --- [grammar] ~214-~214: There might be a mistake here. Context: ...chronization Flow #### **Real-time Sync** 1. **App Launch**: Automatic subscription to ... (QB_NEW_EN) --- [grammar] ~215-~215: There might be a mistake here. Context: ...kind 10002 events from configured Mostro 2. **Event Reception**: Parse relay list and ... (QB_NEW_EN) --- [grammar] ~216-~216: There might be a mistake here. Context: ... relay list and filter against blacklist 3. **State Update**: Merge new relays while p... (QB_NEW_EN) --- [grammar] ~217-~217: There might be a mistake here. Context: ... new relays while preserving user relays 4. **NostrService**: Automatic reconnection t... (QB_NEW_EN) --- [grammar] ~228-~228: There might be a mistake here. Context: ...### Key Features #### **User Experience** - **Transparent Operation**: Sync happens au... (QB_NEW_EN) --- [grammar] ~229-~229: There might be a mistake here. Context: ...Operation**: Sync happens automatically in background - **Full User Control**: Can... (QB_NEW_EN) --- [grammar] ~234-~234: There might be a mistake here. Context: ...cross syncs #### **Technical Robustness** - **Real-time Updates**: WebSocket subscript... (QB_NEW_EN) --- [grammar] ~235-~235: There might be a mistake here. Context: ...WebSocket subscriptions for instant sync - **Error Resilience**: Graceful fallbacks a... (QB_NEW_EN) --- [grammar] ~236-~236: There might be a mistake here. Context: ...llbacks and comprehensive error handling - **Race Protection**: Prevents concurrent s... (QB_NEW_EN) --- [grammar] ~237-~237: There might be a mistake here. Context: ...n**: Prevents concurrent sync operations - **Logging**: Detailed logging for debuggin... (QB_NEW_EN) --- [grammar] ~252-~252: There might be a mistake here. Context: ...dResync(); ``` ### Implementation Notes - **Subscription Management**: Uses `Subscri... (QB_NEW_EN) --- [grammar] ~339-~339: There might be a mistake here. Context: ...dation protocol and localization support - **Settings screens**: Card-based layout wi... (QB_NEW_EN) --- [grammar] ~446-~446: There might be a mistake here. Context: ...ments --- **Last Updated**: 2025-08-21 **Flutter Version**: Latest stable **Dar... (QB_NEW_EN) --- [grammar] ~447-~447: There might be a mistake here. Context: ...-21 **Flutter Version**: Latest stable **Dart Version**: Latest stable **Key De... (QB_NEW_EN) --- [grammar] ~448-~448: There might be a mistake here. Context: ...stable **Dart Version**: Latest stable **Key Dependencies**: Riverpod, GoRouter, ... (QB_NEW_EN) --- [grammar] ~468-~468: There might be a mistake here. Context: ... Live updates via Nostr protocol - 🔗 **Smart Relay Management**: Automatic sync ... (QB_NEW_EN) --- [grammar] ~470-~470: There might be a mistake here. Context: ...ic sync with blacklist control ### Recent Achievements - **UI Modernization**: Co... (QB_NEW_EN) --- [grammar] ~470-~470: There might be a mistake here. Context: ...acklist control ### Recent Achievements - **UI Modernization**: Complete settings an... (QB_NEW_EN) --- [grammar] ~471-~471: There might be a mistake here. Context: ...ete settings and account screen redesign - **Icon Enhancement**: Improved app launche... (QB_NEW_EN) --- [grammar] ~472-~472: There might be a mistake here. Context: ...app launcher and notification visibility - **Localization Excellence**: 73+ new trans... (QB_NEW_EN) --- [grammar] ~473-~473: There might be a mistake here. Context: ... new translation keys across 3 languages - **Code Quality**: Zero analyzer issues wit... (QB_NEW_EN) --- [grammar] ~474-~474: There might be a mistake here. Context: ...zer issues with modern Flutter standards - **Documentation**: Comprehensive NOSTR.md ... (QB_NEW_EN) --- [grammar] ~475-~475: There might be a mistake here. Context: ...rehensive NOSTR.md and updated README.md - **Relay System Architecture**: URL normali... (QB_NEW_EN) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>CLAUDE.md</summary> 221-221: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: build </details> </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>lib/l10n/intl_en.arb (1)</summary><blockquote> `785-806`: **Add Relay strings verified – regenerate l10n** - Verified that the new add-relay and Mostro-sync keys in lib/l10n/intl_en.arb (lines 785–806 and 836–866) are consistent and have parity with the es/it files. - Confirmed the existing `relayUrlHint` key at lib/l10n/intl_en.arb:816; please reuse this in your dialog implementation to avoid duplicating hint text. - After modifying ARB files, run one of the following to regenerate localization outputs and include your new strings in the generated classes: ```bash flutter gen-l10n # or, if you use build_runner: dart run build_runner build -dNo further changes needed here—once l10n is regenerated, this can be marked complete.
lib/l10n/intl_es.arb (1)
814-835: Spanish Add Relay + relay status/blacklist strings are complete and consistent.Parity with en/it is good. Remember to regenerate localization artifacts so the keys are available to the UI.
Use the same l10n generation command as noted in the EN file comment.
Also applies to: 867-891
lib/l10n/intl_it.arb (1)
822-843: Italian Add Relay + relay status/blacklist strings look good.Consistent with en/es, including placeholders. Regenerate l10n artifacts to surface these keys.
Run the l10n generation as noted in the EN file comment.
Also applies to: 875-899
lib/features/relays/relays_notifier.dart (1)
782-795: Cleanup-on-instance-switch logic looks good.Retains defaults and user relays while dropping Mostro-sourced ones. Matches the intended semantics.
CLAUDE.md (1)
446-450: No changes required: CLAUDE.md dependencies are up-to-dateThe
Key Dependencieslist in CLAUDE.md includes bothloggerandshared_preferences, and thepubspec.yamlpins them as follows:
shared_preferences: ^2.3.3logger: ^2.5.0No drift detected between documentation and code.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lib/features/relays/widgets/relay_selector.dart (3)
388-396: Localize the hint text (avoid hardcoded user-facing strings)Use a localized string instead of the hardcoded example, per the project guideline.
- hintText: 'relay.example.com or wss://relay.example.com', + hintText: S.of(context)!.relayUrlHint,If relayUrlHint doesn’t exist for this exact phrasing, I can add an ARB key (e.g., addRelayDialogHintExample) in a follow-up.
202-242: Prevent deleting the last active relay in the delete flow (same guard as toggle path)This path can still remove the final active relay and strand the app offline. Reuse the guard used in _handleRelayToggle.
// If user confirmed deletion, remove the relay if (shouldDelete == true) { final relaysNotifier = ref.read(relaysProvider.notifier); - await relaysNotifier.removeRelay(relayInfo.url); + // Block if this would leave no active relays + if (relaysNotifier.wouldLeaveNoActiveRelays(relayInfo.url)) { + await showDialog( + context: context, + builder: (ctx) => AlertDialog( + backgroundColor: AppTheme.dark2, + title: Text( + S.of(ctx)!.cannotBlacklistLastRelayTitle, + style: const TextStyle(color: AppTheme.cream1), + ), + content: Text( + S.of(ctx)!.cannotBlacklistLastRelayMessage, + style: const TextStyle(color: AppTheme.textSecondary), + ), + actions: [ + TextButton( + onPressed: () => Navigator.of(ctx).pop(), + child: Text( + S.of(ctx)!.cannotBlacklistLastRelayOk, + style: const TextStyle(color: AppTheme.cream1), + ), + ), + ], + ), + ); + return; + } + await relaysNotifier.removeRelay(relayInfo.url); }
343-343: Success SnackBar may never show: using inner dialog context after pop; also dispose the controllerAfter navigator.pop(), the dialog’s build context is unmounted; your mounted check prevents showing the SnackBar. Use the outer context’s ScaffoldMessenger and mounted, and dispose the TextEditingController after the dialog closes.
Future<void> showAddDialog(BuildContext context, WidgetRef ref) async { - final textController = TextEditingController(); + final textController = TextEditingController(); + final outerContext = context; // preserve a safe context for post-pop UI @@ - final localizations = S.of(context)!; - final scaffoldMessenger = ScaffoldMessenger.of(context); + final localizations = S.of(context)!; + final scaffoldMessenger = ScaffoldMessenger.of(outerContext); final navigator = Navigator.of(dialogContext); @@ - if (context.mounted) { + if (outerContext.mounted) { scaffoldMessenger.showSnackBar( SnackBar( content: Text( localizations.addRelaySuccessMessage(result.normalizedUrl!), - style: const TextStyle(color: Colors.white), + style: const TextStyle(color: Colors.white), ), - backgroundColor: Colors.green, + backgroundColor: AppTheme.activeColor, ), ); } @@ - ); + ); + textController.dispose();Also applies to: 446-450, 468-468, 476-476, 515-515
🧹 Nitpick comments (6)
lib/features/relays/widgets/relay_selector.dart (6)
13-14: Acknowledged: watching the notifier is acceptable here given the provider dependency chainGiven relaysProvider internally depends on settingsProvider, watching relaysProvider.notifier still triggers rebuilds in this widget when the blacklist changes. Leaving this as-is is fine. Consider adding a short comment to document the nuance for future readers.
- final relaysNotifier = ref.watch(relaysProvider.notifier); + // relaysProvider depends on settingsProvider; watching the notifier + // still rebuilds this widget when settings change (blacklist toggles, etc.). + final relaysNotifier = ref.watch(relaysProvider.notifier);
120-129: Guard against long URLs overflowing the rowRelay URLs can be very long. Constrain to a single line with ellipsis for better layout stability.
Expanded( child: Text( relayInfo.url, style: const TextStyle( color: Colors.white, fontSize: 14, fontWeight: FontWeight.w500, ), + maxLines: 1, + overflow: TextOverflow.ellipsis, ), ),
145-165: Prefer IconButton over GestureDetector for delete: better semantics, focus, and a11yIconButton provides built-in semantics, focus, and splash/hover behavior. Keep the icon the same.
- GestureDetector( - onTap: () async { - await _showDeleteUserRelayDialog(context, ref, relayInfo); - }, - child: const Icon( - Icons.delete, - color: Colors.white, - size: 24, - ), - ), + IconButton( + onPressed: () async { + await _showDeleteUserRelayDialog(context, ref, relayInfo); + }, + icon: const Icon( + Icons.delete, + color: Colors.white, + size: 24, + ), + // Optional: add a localized tooltip if desired + // tooltip: S.of(context)!.deleteUserRelayTooltip, + ),
167-200: Improve accessibility of the custom switchThe custom GestureDetector-based switch lacks native semantics. Two options:
- Minimal change: wrap with Semantics to expose toggled state.
- Preferable: replace with a real Switch for correct a11y, focus, and platform behavior.
Minimal Semantics wrapper while keeping current visuals:
- return GestureDetector( + return Semantics( + button: true, + toggled: isActive, + onTap: () async { + await _handleRelayToggle(context, ref, relayInfo); + }, + child: GestureDetector( onTap: () async { await _handleRelayToggle(context, ref, relayInfo); }, child: Container( width: 50, height: 26, padding: const EdgeInsets.all(2), decoration: BoxDecoration( color: isActive ? AppTheme.activeColor : AppTheme.red1, borderRadius: BorderRadius.circular(13), ), child: AnimatedAlign( duration: const Duration(milliseconds: 200), alignment: isActive ? Alignment.centerRight : Alignment.centerLeft, child: Container( width: 22, height: 22, decoration: BoxDecoration( color: Colors.white, borderRadius: BorderRadius.circular(11), border: Border.all( color: Colors.black, width: 2, ), ), ), ), ), - ), - ); + ), + );Or replace with a Switch (updates visual style but improves UX):
return Switch( value: isActive, onChanged: (_) async => _handleRelayToggle(context, ref, relayInfo), // Optionally theme track/thumb via SwitchTheme if you need the exact look. );
247-257: Avoid URL string heuristics and redundant lookups; rely on model/notifier for relay type
- Hardcoding 'wss://relay.mostro.network' in the widget couples UI to a specific instance and risks drift. Prefer a source-of-truth (e.g., Config or notifier).
- You already have relayInfo.source earlier in this widget; reuse it instead of re-fetching relays to infer isUserRelay.
- final isDefaultMostroRelay = relayInfo.url.startsWith('wss://relay.mostro.network'); - final relaysNotifier = ref.read(relaysProvider.notifier); - - // Detect relay type (user vs mostro/default) - final currentRelays = ref.read(relaysProvider); - final relay = currentRelays.firstWhere( - (r) => r.url == relayInfo.url, - orElse: () => Relay(url: ''), // Empty relay if not found - ); - final isUserRelay = relay.url.isNotEmpty && relay.source == RelaySource.user; + final relaysNotifier = ref.read(relaysProvider.notifier); + // Prefer using metadata provided to the UI + final isUserRelay = relayInfo.source == RelaySource.user; + // Delegate default-mostro identification to a single place (notifier/config) + final isDefaultMostroRelay = relaysNotifier.isDefaultMostroRelay(relayInfo.url);Outside this file (not required for this PR but recommended), add to the notifier:
bool isDefaultMostroRelay(String url) { // Compare against configured bootstrap/default(s), normalize ws/wss if needed. return url == defaultMostroRelayUrl; // or set membership in a known set. }I can open a follow-up to centralize this logic in RelaysNotifier/Config to prevent future regressions. Want me to draft it?
456-477: Minor: be resilient if normalizedUrl is unexpectedly nullNot expected, but safe to fall back to the user’s input to avoid losing context in the success message.
- localizations.addRelaySuccessMessage(result.normalizedUrl!), + localizations.addRelaySuccessMessage( + result.normalizedUrl ?? input, + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/features/relays/widgets/relay_selector.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/@(screens|widgets)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs) instead of inline side effects in build methods
Files:
lib/features/relays/widgets/relay_selector.dart
lib/!(generated)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/relays/widgets/relay_selector.dart
lib/features/*/{screens,providers,notifiers,widgets}/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the feature-based structure: features/{feature}/{screens|providers|notifiers|widgets}/
Files:
lib/features/relays/widgets/relay_selector.dart
🧠 Learnings (6)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mobile#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.
📚 Learning: 2025-08-21T14:45:43.964Z
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.964Z
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/relays/widgets/relay_selector.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/**/@(screens|widgets)/**/*.dart : Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs) instead of inline side effects in build methods
Applied to files:
lib/features/relays/widgets/relay_selector.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/!(generated)/**/*.dart : Check mounted before using BuildContext after async gaps
Applied to files:
lib/features/relays/widgets/relay_selector.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/!(generated)/**/*.dart : Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Applied to files:
lib/features/relays/widgets/relay_selector.dart
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
PR: MostroP2P/mobile#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/widgets/relay_selector.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/features/relays/widgets/relay_selector.dart (2)
16-57: Layout and empty-state handling look solidGood structure, proper localization usage, and the new Color.withValues API. No issues.
Also applies to: 58-61
264-291: Dialogs and safety checks look goodGreat job blocking “last relay” blacklisting and confirming before blacklisting the default relay; localization and theming are consistent.
Also applies to: 300-336
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lib/features/relays/widgets/relay_selector.dart (3)
13-14: Prefer watching provider state; read notifier separately for actions (more robust rebuilds).Watching
relaysProvider.notifierwon’t rebuild on state changes unless the notifier instance itself is recreated. It works today due to upstream dependencies, but this coupling is brittle. Watch the provider’s state (or aselect) for rebuilds, and read the notifier for mutations/derived data.- final relaysNotifier = ref.watch(relaysProvider.notifier); - final mostroRelays = relaysNotifier.mostroRelaysWithStatus; + // Rebuild when relay state changes; perform actions via notifier + final _ = ref.watch(relaysProvider); + final relaysNotifier = ref.read(relaysProvider.notifier); + final mostroRelays = relaysNotifier.mostroRelaysWithStatus;
422-423: Localize the hint text (avoid hardcoded user-facing strings).Replace the hardcoded hint with an ARB-backed string (you already have
relayUrlHintper past notes).- hintText: 'relay.example.com or wss://relay.example.com', + hintText: S.of(context)!.relayUrlHint,
373-545: Success SnackBar likely never shows: using inner dialog context after pop.You capture
ScaffoldMessenger.of(context)from the dialog’s inner context. Afternavigator.pop(), thatcontextis unmounted, socontext.mountedis false and the SnackBar won’t show. Capture the outer context (the one passed toshowAddDialog) and use that after pop. Also disposeTextEditingController.Future<void> showAddDialog(BuildContext context, WidgetRef ref) async { - final textController = TextEditingController(); + final outerContext = context; // keep a safe context outside the dialog + final textController = TextEditingController(); final relaysNotifier = ref.read(relaysProvider.notifier); bool isLoading = false; String? errorMessage; - await showDialog<void>( + try { + await showDialog<void>( context: context, barrierDismissible: false, builder: (BuildContext dialogContext) { return StatefulBuilder( - builder: (context, setState) { + builder: (innerContext, setState) { return AlertDialog( backgroundColor: AppTheme.backgroundCard, shape: RoundedRectangleBorder( borderRadius: BorderRadius.circular(16), side: BorderSide(color: Colors.white.withValues(alpha: 0.1)), ), title: Text( - S.of(context)!.addRelayDialogTitle, + S.of(innerContext)!.addRelayDialogTitle, style: const TextStyle( color: AppTheme.textPrimary, fontSize: 18, fontWeight: FontWeight.w600, ), ), content: SizedBox( width: double.maxFinite, child: Column( mainAxisSize: MainAxisSize.min, crossAxisAlignment: CrossAxisAlignment.start, children: [ Container( padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 8), decoration: BoxDecoration( color: AppTheme.backgroundInput, borderRadius: BorderRadius.circular(8), border: Border.all(color: Colors.white.withValues(alpha: 0.1)), ), child: TextField( controller: textController, enabled: !isLoading, style: const TextStyle(color: AppTheme.textPrimary), decoration: InputDecoration( - labelText: S.of(context)!.addRelayDialogPlaceholder, + labelText: S.of(innerContext)!.addRelayDialogPlaceholder, labelStyle: const TextStyle(color: AppTheme.textSecondary), border: InputBorder.none, contentPadding: const EdgeInsets.symmetric(horizontal: 12, vertical: 8), - hintText: 'relay.example.com or wss://relay.example.com', + hintText: S.of(innerContext)!.relayUrlHint, hintStyle: const TextStyle(color: AppTheme.textSecondary), errorText: errorMessage, errorStyle: const TextStyle(color: Colors.red), ), autofocus: true, ), ), if (isLoading) ...[ const SizedBox(height: 16), Row( children: [ const SizedBox( width: 20, height: 20, child: CircularProgressIndicator( strokeWidth: 2, valueColor: AlwaysStoppedAnimation<Color>(AppTheme.cream1), ), ), const SizedBox(width: 12), Text( - S.of(context)!.addRelayDialogTesting, + S.of(innerContext)!.addRelayDialogTesting, style: const TextStyle(color: AppTheme.textSecondary), ), ], ), ], ], ), ), actions: [ if (!isLoading) ...[ TextButton( onPressed: () => Navigator.of(dialogContext).pop(), child: Text( - S.of(context)!.addRelayDialogCancel, + S.of(innerContext)!.addRelayDialogCancel, style: const TextStyle( color: AppTheme.textSecondary, fontSize: 16, fontWeight: FontWeight.w500, ), textAlign: TextAlign.center, ), ), const SizedBox(width: 12), ], ElevatedButton( onPressed: isLoading ? null : () async { final input = textController.text.trim(); if (input.isEmpty) return; // Capture context values before async operations - final localizations = S.of(context)!; - final scaffoldMessenger = ScaffoldMessenger.of(context); + final localizations = S.of(innerContext)!; + final scaffoldMessenger = ScaffoldMessenger.of(outerContext); final navigator = Navigator.of(dialogContext); setState(() { isLoading = true; errorMessage = null; }); try { final result = await relaysNotifier.addRelayWithSmartValidation( input, errorOnlySecure: localizations.addRelayErrorOnlySecure, errorNoHttp: localizations.addRelayNoHttp, errorInvalidDomain: localizations.addRelayErrorInvalidDomain, errorAlreadyExists: localizations.addRelayErrorAlreadyExists, errorNotValid: localizations.addRelayErrorNotValid, ); if (result.success) { navigator.pop(); - if (context.mounted) { + if (!outerContext.mounted) return; scaffoldMessenger.showSnackBar( SnackBar( content: Text( localizations.addRelaySuccessMessage(result.normalizedUrl!), style: const TextStyle(color: Colors.white), ), backgroundColor: Colors.green, ), ); - } } else { setState(() { errorMessage = result.error; isLoading = false; }); } } catch (e) { setState(() { errorMessage = localizations.addRelayErrorGeneric; isLoading = false; }); } }, style: ElevatedButton.styleFrom( backgroundColor: AppTheme.activeColor, foregroundColor: Colors.black, shape: RoundedRectangleBorder( borderRadius: BorderRadius.circular(8), ), padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 12), ), child: Text( - S.of(context)!.addRelayDialogAdd, + S.of(innerContext)!.addRelayDialogAdd, style: const TextStyle( fontSize: 16, fontWeight: FontWeight.w500, ), textAlign: TextAlign.center, ), ), ], ); }, ); }, - ); + ); + } finally { + textController.dispose(); + }
🧹 Nitpick comments (7)
lib/features/relays/widgets/relay_selector.dart (7)
30-56: Add Semantics for the empty-state block (accessibility).Wrap the placeholder with a Semantics widget so screen readers announce the message and purpose.
- if (mostroRelays.isEmpty) - Container( + if (mostroRelays.isEmpty) + Semantics( + label: S.of(context)!.noMostroRelaysAvailable, + // Consider liveRegion if this frequently toggles + // liveRegion: true, + child: Container( padding: const EdgeInsets.all(20), decoration: BoxDecoration( color: AppTheme.dark1.withValues(alpha: 0.5), borderRadius: BorderRadius.circular(8), border: Border.all(color: Colors.white.withValues(alpha: 0.1)), ), child: Column( children: [ Icon( Icons.info_outline, color: AppTheme.textSecondary, size: 24, ), const SizedBox(height: 8), Text( S.of(context)!.noMostroRelaysAvailable, style: const TextStyle( color: AppTheme.textSecondary, fontSize: 14, ), textAlign: TextAlign.center, ), ], ), - ), + ), )
58-60: Guard against overflow when rendering many relays.Rendering a potentially long list via spread into a Column may overflow if the parent isn’t scrollable. If this widget isn’t always inside a scroll view, prefer a
ListView.separatedor make this section scrollable.Options:
- If parent is not a scroll view:
- ...mostroRelays.map((relayInfo) { - return _buildRelayItem(context, ref, relayInfo); - }), + Flexible( + child: ListView.separated( + itemCount: mostroRelays.length, + separatorBuilder: (_, __) => const SizedBox(height: 12), + itemBuilder: (_, i) => _buildRelayItem(context, ref, mostroRelays[i]), + ), + ),
- If parent is a
SingleChildScrollView, ensure constraints (e.g., wrap the list in aColumnbut cap item count or lazy render viaListViewwithshrinkWrap: trueandphysics: NeverScrollableScrollPhysics()).
145-160: Use IconButton instead of GestureDetector for better a11y, focus, and tooltips.
IconButtonprovides semantics, focus, and larger hit targets by default. Add a tooltip for discoverability.- GestureDetector( - onTap: () async { - await _showDeleteUserRelayDialog(context, ref, relayInfo); - }, - child: const Icon( - Icons.delete, - color: Colors.white, - size: 24, - ), - ), + IconButton( + icon: const Icon(Icons.delete, color: Colors.white, size: 24), + tooltip: S.of(context)!.deleteUserRelayTitle, + onPressed: () async { + await _showDeleteUserRelayDialog(context, ref, relayInfo); + }, + ),
167-200: Prefer Switch.adaptive (built-in semantics, gestures, and accessibility).The custom toggle looks nice but lacks built-in a11y and focus features. Use
Switch.adaptive. If you must keep custom UI, at least wrap withSemantics(toggleable: true, checked: isActive).- return GestureDetector( - onTap: () async { - await _handleRelayToggle(context, ref, relayInfo); - }, - child: Container( - width: 50, - height: 26, - padding: const EdgeInsets.all(2), - decoration: BoxDecoration( - color: isActive ? AppTheme.activeColor : AppTheme.red1, - borderRadius: BorderRadius.circular(13), - ), - child: AnimatedAlign( - duration: const Duration(milliseconds: 200), - alignment: isActive ? Alignment.centerRight : Alignment.centerLeft, - child: Container( - width: 22, - height: 22, - decoration: BoxDecoration( - color: Colors.white, - borderRadius: BorderRadius.circular(11), - border: Border.all( - color: Colors.black, - width: 2, - ), - ), - ), - ), - ), - ); + return Switch.adaptive( + value: isActive, + onChanged: (_) async => _handleRelayToggle(context, ref, relayInfo), + activeColor: AppTheme.activeColor, + inactiveThumbColor: Colors.white, + inactiveTrackColor: AppTheme.red1, + );
281-287: Use relayInfo.source directly; avoid provider lookup for type detection.You already receive
MostroRelayInfowithsource. Re-reading provider state adds complexity and potential drift.- // Detect relay type (user vs mostro/default) - final currentRelays = ref.read(relaysProvider); - final relay = currentRelays.firstWhere( - (r) => r.url == relayInfo.url, - orElse: () => Relay(url: ''), // Empty relay if not found - ); - final isUserRelay = relay.url.isNotEmpty && relay.source == RelaySource.user; + // Detect relay type (user vs mostro/default) using passed-in info + final isUserRelay = relayInfo.source == RelaySource.user;
328-356: Theme consistency: avoid raw Colors.red in confirm button.Use
AppTheme.red1(or themed error color) for consistency with the rest of the UI.- child: Text( - S.of(context)!.blacklistDefaultRelayConfirm, - style: const TextStyle(color: Colors.red), - ), + child: Text( + S.of(context)!.blacklistDefaultRelayConfirm, + style: const TextStyle(color: AppTheme.red1), + ),
277-279: Centralize the bootstrap relay constant.
'wss://relay.mostro.network'appears here and elsewhere. Consider a single source of truth (e.g.,Config.bootstrapRelay) to avoid drift and simplify future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/features/relays/widgets/relay_selector.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/@(screens|widgets)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs) instead of inline side effects in build methods
Files:
lib/features/relays/widgets/relay_selector.dart
lib/!(generated)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/relays/widgets/relay_selector.dart
lib/features/*/{screens,providers,notifiers,widgets}/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the feature-based structure: features/{feature}/{screens|providers|notifiers|widgets}/
Files:
lib/features/relays/widgets/relay_selector.dart
🧠 Learnings (6)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mobile#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.
📚 Learning: 2025-08-21T14:45:43.964Z
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.964Z
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/relays/widgets/relay_selector.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/**/@(screens|widgets)/**/*.dart : Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs) instead of inline side effects in build methods
Applied to files:
lib/features/relays/widgets/relay_selector.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/!(generated)/**/*.dart : Check mounted before using BuildContext after async gaps
Applied to files:
lib/features/relays/widgets/relay_selector.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/!(generated)/**/*.dart : Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Applied to files:
lib/features/relays/widgets/relay_selector.dart
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
PR: MostroP2P/mobile#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/widgets/relay_selector.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/features/relays/widgets/relay_selector.dart (1)
202-233: Good guard: cannot delete the last active relay.The early-exit dialog prevents leaving the app without any active relay. Nice defensive UX.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/relays/relays_notifier.dart (1)
166-174: Bug: test connection cleanup can disconnect the app-wide Nostr instanceOn exception,
_testNostrProtocolcalls_cleanupTestConnection(Nostr.instance), potentially tearing down the production connection. Declare the test instance outside try and always clean that instance only.- Future<bool> _testNostrProtocol(String url) async { + Future<bool> _testNostrProtocol(String url) async { // Generate unique subscription ID for this test final testSubId = 'relay_test_${DateTime.now().millisecondsSinceEpoch}'; bool receivedEvent = false; bool receivedEose = false; bool isConnected = false; - - try { - // Create isolated instance for testing - final testNostr = Nostr(); + Nostr? testNostr; + try { + // Create isolated instance for testing + testNostr = Nostr(); @@ - await testNostr.services.relays.init( + await testNostr.services.relays.init( @@ - await _cleanupTestConnection(testNostr); + await _cleanupTestConnection(testNostr); @@ - } catch (e) { + } catch (e) { // Protocol test failed with error - try { - await _cleanupTestConnection(Nostr.instance); - } catch (_) { - // Ignore cleanup errors - } + try { + if (testNostr != null) { + await _cleanupTestConnection(testNostr); + } + } catch (_) { + // Ignore cleanup errors + } return false; } }Also applies to: 176-207, 254-261
♻️ Duplicate comments (12)
lib/features/relays/relays_notifier.dart (12)
557-566: Avoid sentinel “empty Relay” when checking for existing user relayUse where/any to check presence; this is safer and clearer.
- final existingUserRelay = userRelays.firstWhere( - (r) => _normalizeRelayUrl(r.url) == relayUrl, - orElse: () => Relay(url: ''), // Empty relay if not found - ); - - if (existingUserRelay.url.isNotEmpty) { + final existingUserRelay = userRelays.where( + (r) => _normalizeRelayUrl(r.url) == relayUrl, + ); + if (existingUserRelay.isNotEmpty) { // PROMOTION: User relay → Mostro relay (move to beginning) userRelays.removeWhere((r) => _normalizeRelayUrl(r.url) == relayUrl); final promotedRelay = Relay.fromMostro(relayUrl); updatedRelays.insert(0, promotedRelay); // Insert at beginning _logger.i('Promoted user relay to Mostro relay: $relayUrl'); continue; }
610-627: Comment is outdated; also canonicalize blacklist/remove opsThe comment claims “no user relays exist,” but the file supports user relays. Update the comment and use canonical URL for matching/blacklisting/removal.
- /// Remove relay with blacklist support - /// All relays are now blacklisted when removed (since no user relays exist) + /// Remove relay with blacklist support + /// Blacklist any removed relay to prevent re-add during sync; user can un-blacklist later Future<void> removeRelayWithBlacklist(String url) async { - final relay = state.firstWhere((r) => r.url == url, orElse: () => Relay(url: '')); + final key = _normalizeRelayUrl(url); + final relay = state.firstWhere((r) => _normalizeRelayUrl(r.url) == key, orElse: () => Relay(url: '')); @@ - await settings.addToBlacklist(url); - _logger.i('Blacklisted ${relay.source} relay: $url'); + await settings.addToBlacklist(key); + _logger.i('Blacklisted ${relay.source} relay: $key'); @@ - await removeRelay(url); - _logger.i('Removed relay: $url (source: ${relay.source})'); + await removeRelay(key); + _logger.i('Removed relay: $key (source: ${relay.source})'); }
641-646: Simplify pubkey change detection null/empty checksAvoid forced null check; this handles all states cleanly.
- if (newPubkey != currentPubkey && - currentPubkey != null && - newPubkey.isNotEmpty && - currentPubkey!.isNotEmpty) { + if (newPubkey != currentPubkey && + currentPubkey?.isNotEmpty == true && + newPubkey.isNotEmpty) {
743-754: “Would leave no active relays” should also use canonical formsPrevents false negatives if formats differ.
- final currentActiveRelays = state.map((r) => r.url).toList(); - final currentBlacklist = settings.state.blacklistedRelays; + final currentActiveRelays = state.map((r) => _normalizeRelayUrl(r.url)).toList(); + final currentBlacklist = settings.state.blacklistedRelays.map(_normalizeRelayUrl).toList(); @@ - final wouldBeBlacklisted = [...currentBlacklist, urlToBlacklist]; + final wouldBeBlacklisted = [...currentBlacklist, _normalizeRelayUrl(urlToBlacklist)]; final wouldRemainActive = currentActiveRelays.where((url) => !wouldBeBlacklisted.contains(url)).toList();
74-97: Persist using canonical URLs; blacklist compare currently mismatches formatsblacklist vs state comparisons use raw strings here; trailing slashes/case can slip through and persist blacklisted relays. Canonicalize both sides and dedupe before saving.
- // Get blacklisted relays - final blacklistedUrls = settings.state.blacklistedRelays; + // Get blacklisted relays (canonical) + final blacklistedUrls = settings.state.blacklistedRelays + .map(_normalizeRelayUrl) + .toSet(); @@ - // Include ALL active relays (Mostro/default + user) that are NOT blacklisted - final allActiveRelayUrls = state - .where((r) => !blacklistedUrls.contains(r.url)) - .map((r) => r.url) - .toList(); + // Include ALL active relays (Mostro/default + user) that are NOT blacklisted (canonical compare) + final allActiveRelayUrls = state + .where((r) => !blacklistedUrls.contains(_normalizeRelayUrl(r.url))) + .map((r) => _normalizeRelayUrl(r.url)) + .toSet() // dedupe + .toList();
372-386: Manual add should canonicalize and blacklist-remove using the same keyUse the canonical form for blacklist operations; otherwise, duplicates slip through if formats differ.
- if (settings.state.blacklistedRelays.contains(normalizedUrl)) { - await settings.removeFromBlacklist(normalizedUrl); + final key = _normalizeRelayUrl(normalizedUrl); + final isBlacklisted = settings.state.blacklistedRelays + .map(_normalizeRelayUrl) + .toSet() + .contains(key); + if (isBlacklisted) { + await settings.removeFromBlacklist(key); _logger.i('Removed $normalizedUrl from blacklist - user manually added it'); } @@ - final newRelay = Relay( - url: normalizedUrl, + final newRelay = Relay( + url: key,
472-484: Retry timer can leak and duplicate; retain and cancel itMultiple calls to
_scheduleRetrySynccan spawn concurrent timers. Track and cancel the previous one; also cancel in dispose.- void _scheduleRetrySync(String mostroPubkey) { - Timer(const Duration(seconds: 10), () async { + Timer? _retrySyncTimer; // add near other fields + void _scheduleRetrySync(String mostroPubkey) { + _retrySyncTimer?.cancel(); + _retrySyncTimer = Timer(const Duration(seconds: 10), () async { @@ }); }@override void dispose() { _relayListSubscription?.cancel(); _subscriptionManager?.dispose(); _settingsWatchTimer?.cancel(); + _retrySyncTimer?.cancel(); super.dispose(); }Also applies to: 813-819
526-538: Canonicalize URL sets and use set-based change detectionUse one canonical form for all comparisons; avoid object equality and redundant whereType on non-null map.
- final normalizedRelays = event.validRelays - .map((url) => _normalizeRelayUrl(url)) - .whereType<String>() // Filter out any null results - .toSet() // Remove duplicates - .toList(); + final normalizedRelays = event.validRelays + .map(_normalizeRelayUrl) + .toSet() + .toList(); @@ - final blacklistedUrls = settings.state.blacklistedRelays - .map((url) => _normalizeRelayUrl(url)) - .whereType<String>() // Filter out any null results - .toSet(); + final blacklistedUrls = settings.state.blacklistedRelays + .map(_normalizeRelayUrl) + .toSet(); @@ - if (finalRelays.length != state.length || - !finalRelays.every((relay) => state.contains(relay))) { + final newUrls = finalRelays.map((r) => _normalizeRelayUrl(r.url)).toSet(); + final oldUrls = state.map((r) => _normalizeRelayUrl(r.url)).toSet(); + final hasChanges = newUrls.length != oldUrls.length || + !newUrls.containsAll(oldUrls) || + !oldUrls.containsAll(newUrls); + if (hasChanges) { state = finalRelays; await _saveRelays(); _logger.i('Updated relay list with ${finalRelays.length} relays (${blacklistedUrls.length} blacklisted)'); }Also applies to: 596-602
681-685: Blacklist lookup should use canonical formDirect contains() on raw strings will miss variants (case, trailing slash).
- bool isRelayBlacklisted(String url) { - return settings.state.blacklistedRelays.contains(url); - } + bool isRelayBlacklisted(String url) { + final key = _normalizeRelayUrl(url); + return settings.state.blacklistedRelays + .map(_normalizeRelayUrl) + .toSet() + .contains(key); + }
693-741: UI model should use canonical URLs for active/blacklist status; optionally distinguish Mostro vs user blacklistedUse canonical sets for comparisons. If desired, track the last Mostro relay set to categorize blacklisted URLs accurately in the UI list.
- final blacklistedUrls = settings.state.blacklistedRelays; - final activeRelays = state.map((r) => r.url).toSet(); + final blacklistedUrls = settings.state.blacklistedRelays.map(_normalizeRelayUrl).toSet(); + final activeRelays = state.map((r) => _normalizeRelayUrl(r.url)).toSet(); @@ - .map((r) => MostroRelayInfo( - url: r.url, - // Check if this relay is blacklisted (even if it's still in state) - isActive: !blacklistedUrls.contains(r.url), + .map((r) => MostroRelayInfo( + url: r.url, + // Check if this relay is blacklisted (even if it's still in state) + isActive: !blacklistedUrls.contains(_normalizeRelayUrl(r.url)), isHealthy: r.isHealthy, source: r.source, )) @@ - final mostroBlacklistedRelays = blacklistedUrls - .where((url) => !activeRelays.contains(url)) + final mostroBlacklistedRelays = blacklistedUrls + .where((url) => !activeRelays.contains(url) /* && _lastMostroRelays.contains(url) */) .map((url) => MostroRelayInfo( url: url, isActive: false, isHealthy: false, - source: null, // Unknown source for blacklisted-only relays + source: null, // Set to RelaySource.mostro if you track _lastMostroRelays )) .toList(); @@ - .map((r) => MostroRelayInfo( - url: r.url, - isActive: !blacklistedUrls.contains(r.url), // User relays can also be blacklisted + .map((r) => MostroRelayInfo( + url: r.url, + isActive: !blacklistedUrls.contains(_normalizeRelayUrl(r.url)), // User relays can also be blacklisted isHealthy: r.isHealthy, source: r.source, ))Optional (outside selected range):
- Add
Set<String> _lastMostroRelays = {};- In
_handleMostroRelayListUpdate, set_lastMostroRelays = normalizedRelays.map(_normalizeRelayUrl).toSet();
759-773: Toggle blacklist must canonicalize; consider guard against blacklisting last activeBring this in line with the rest of the canonical handling. Optionally, avoid leaving zero active relays by checking
wouldLeaveNoActiveRelays.- Future<void> toggleMostroRelayBlacklist(String url) async { - final isCurrentlyBlacklisted = settings.state.blacklistedRelays.contains(url); + Future<void> toggleMostroRelayBlacklist(String url) async { + final key = _normalizeRelayUrl(url); + final isCurrentlyBlacklisted = settings.state.blacklistedRelays + .map(_normalizeRelayUrl) + .toSet() + .contains(key); @@ if (isCurrentlyBlacklisted) { // Remove from blacklist and trigger sync to add back - await settings.removeFromBlacklist(url); - _logger.i('Removed $url from blacklist, triggering re-sync'); + await settings.removeFromBlacklist(key); + _logger.i('Removed $key from blacklist, triggering re-sync'); await syncWithMostroInstance(); } else { + // Optional: guard + // if (wouldLeaveNoActiveRelays(key)) { _logger.w('Refusing to blacklist the last active relay'); return; } // Add to blacklist and remove from current state - await settings.addToBlacklist(url); - await removeRelay(url); - _logger.i('Blacklisted and removed Mostro relay: $url'); + await settings.addToBlacklist(key); + await removeRelay(key); + _logger.i('Blacklisted and removed Mostro relay: $key'); } }
803-811: Unify to a single canonicalization pathMake
_normalizeRelayUrldelegate tonormalizeRelayUrland always trim/remove a trailing slash for safety. This ensures all call sites compare the same keys.- /// Normalize relay URL to prevent duplicates (removes trailing slash) - String _normalizeRelayUrl(String url) { - url = url.trim(); - // Remove trailing slash if present - if (url.endsWith('/')) { - url = url.substring(0, url.length - 1); - } - return url; - } + /// Normalize relay URL: canonical form or fallback, always strip trailing slash + String _normalizeRelayUrl(String url) { + final canonical = normalizeRelayUrl(url); + return (canonical ?? url) + .trim() + .replaceAll(RegExp(r'/$'), ''); + }
🧹 Nitpick comments (5)
lib/features/relays/relays_notifier.dart (5)
53-61: Avoid magic string duplication for the bootstrap relayThe hardcoded bootstrap relay is intentional (per prior discussion/learning). Still, repeat occurrences increase drift risk. Extract a private const and reuse.
Apply within the selected ranges:
- final defaultRelay = Relay.fromDefault('wss://relay.mostro.network'); + final defaultRelay = Relay.fromDefault(_bootstrapRelay); @@ - .where((url) => url != 'wss://relay.mostro.network') // Avoid duplicates + .where((url) => url != _bootstrapRelay) // Avoid duplicates @@ - final defaultRelay = Relay.fromDefault('wss://relay.mostro.network'); + final defaultRelay = Relay.fromDefault(_bootstrapRelay);Additional change (outside selected ranges) to add the constant near other fields:
static const String _bootstrapRelay = 'wss://relay.mostro.network';Also applies to: 666-668
115-151: Normalization/validation: handle trailing slashes and optional ports; keep domain-only UX flexiblenormalizeRelayUrl rejects inputs like “wss://relay.example.com/” due to the path slash, and isValidDomainFormat lowercases the entire string. Trim a trailing slash before validation and isolate the host (strip path/query/fragments) so common inputs pass. Keep lowercasing to host/scheme implicitly via canonicalizer.
- String? normalizeRelayUrl(String input) { - input = input.trim().toLowerCase(); + String? normalizeRelayUrl(String input) { + input = input.trim(); + // Allow common copy-paste forms with a trailing slash + input = input.replaceAll(RegExp(r'/$'), ''); + final lowered = input.toLowerCase(); @@ - if (!isValidDomainFormat(input)) return null; + if (!isValidDomainFormat(lowered)) return null; @@ - if (input.startsWith('wss://')) { + if (lowered.startsWith('wss://')) { return input; // Already properly formatted - } else if (input.startsWith('ws://') || input.startsWith('http')) { + } else if (lowered.startsWith('ws://') || lowered.startsWith('http')) { return null; // Reject non-secure protocols } else { return 'wss://$input'; // Auto-add wss:// prefix } } @@ - bool isValidDomainFormat(String input) { + bool isValidDomainFormat(String input) { // Remove protocol prefix if present if (input.startsWith('wss://')) { input = input.substring(6); } else if (input.startsWith('ws://')) { input = input.substring(5); } else if (input.startsWith('http://')) { input = input.substring(7); } else if (input.startsWith('https://')) { input = input.substring(8); } + // Strip path/query/fragment if present + input = input.split('/').first.split('?').first.split('#').first; + // Strip port if present (IPv6 not supported in this simple validator) + input = input.split(':').first;If you intend to allow IP-based relays for dev, we can extend the validator to accept IPv4 (optional).
264-311: Ensure socket/listener are always closed on all pathsWrap WebSocket steps in try/finally to guarantee cleanup even if early returns or errors occur after connect.
- Future<bool> _testBasicWebSocketConnectivity(String url) async { - try { + Future<bool> _testBasicWebSocketConnectivity(String url) async { + WebSocket? socket; + StreamSubscription? subscription; + try { @@ - final socket = await WebSocket.connect( + socket = await WebSocket.connect( @@ - final subscription = socket.listen( + subscription = socket.listen( @@ - await subscription.cancel(); - await socket.close(); + await subscription?.cancel(); + await socket?.close(); return receivedResponse; } catch (e) { // WebSocket test failed return false; + } finally { + await subscription?.cancel(); + await socket?.close(); } }
782-801: Switching instances: consider dropping all Mostro relays for a cleaner slateCurrent filter keeps non-blacklisted Mostro relays, but the method name/comments imply a full clean. Removing all Mostro entries clarifies semantics; new instance will repopulate quickly.
- // Keep default config relays, user relays, AND non-blacklisted Mostro relays - final cleanedRelays = state.where((relay) => - relay.source == RelaySource.defaultConfig || - relay.source == RelaySource.user || - (relay.source == RelaySource.mostro && !blacklistedUrls.contains(_normalizeRelayUrl(relay.url))) - ).toList(); + // Keep only default config and user relays; drop all Mostro relays + final cleanedRelays = state.where((relay) => + relay.source == RelaySource.defaultConfig || + relay.source == RelaySource.user).toList();
47-71: Log volume/PII: consider reducing URL lists in info logsRelay URLs can be user-provided data. In production builds, prefer counts or debug-level logging to avoid leaking PII in logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/features/relays/relays_notifier.dart(5 hunks)test/features/relays/relays_notifier_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/features/relays/relays_notifier_test.dart
🧰 Additional context used
📓 Path-based instructions (1)
lib/!(generated)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/relays/relays_notifier.dart
🧠 Learnings (3)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mobile#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.
📚 Learning: 2025-08-21T14:45:43.964Z
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.964Z
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/relays/relays_notifier.dart
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
PR: MostroP2P/mobile#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.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/features/relays/relays_notifier.dart (2)
35-42: Initialization order and microtask defer look soundConstructor wiring, Mostro sync init, and deferred sync via microtask avoid provider init races. Good separation of concerns.
409-427: SubscriptionManager wiring and error handling look goodRelays stream subscription with typed event, guarded error logging, and avoiding immediate sync in init is solid.
…andling - Add timestamp validation to prevent processing older relay events - Add hash-based deduplication to avoid redundant relay list updates - Reset hash tracking on blacklist changes to ensure immediate UI updates - Fix relay disappearing temporarily when toggling blacklist status
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/relays/relays_notifier.dart (2)
173-268: Don’t disconnect the global Nostr instance in test cleanupIn the catch block you call _cleanupTestConnection(Nostr.instance), which risks tearing down the app’s live connection. Clean up only the dedicated test instance.
- Future<bool> _testNostrProtocol(String url) async { - // Generate unique subscription ID for this test + Future<bool> _testNostrProtocol(String url) async { + // Generate unique subscription ID for this test final testSubId = 'relay_test_${DateTime.now().millisecondsSinceEpoch}'; @@ - try { - // Create isolated instance for testing - final testNostr = Nostr(); + Nostr? testNostr; + try { + // Create isolated instance for testing + testNostr = Nostr(); @@ - await _cleanupTestConnection(testNostr); + await _cleanupTestConnection(testNostr); return false; } @@ - await _cleanupTestConnection(testNostr); + await _cleanupTestConnection(testNostr); @@ - } catch (e) { + } catch (e) { // Protocol test failed with error - try { - await _cleanupTestConnection(Nostr.instance); - } catch (_) { - // Ignore cleanup errors - } + if (testNostr != null) { + try { + await _cleanupTestConnection(testNostr); + } catch (_) {} + } return false; } }
360-367: Duplicate detection and blacklist removal should use canonical URLs
- Duplicate check compares raw URL against normalizedUrl. If state contains a URL with a trailing slash/case variant, duplicates will slip in.
- Blacklist removal also uses raw contains, so a canonically equivalent entry won’t be removed.
- if (state.any((relay) => relay.url == normalizedUrl)) { + final stateKeys = state.map((r) => _normalizeRelayUrl(r.url)).toSet(); + if (stateKeys.contains(_normalizeRelayUrl(normalizedUrl))) { return RelayValidationResult( success: false, error: errorAlreadyExists, ); } @@ - if (settings.state.blacklistedRelays.contains(normalizedUrl)) { - await settings.removeFromBlacklist(normalizedUrl); + final blKeys = settings.state.blacklistedRelays.map(_normalizeRelayUrl).toSet(); + if (blKeys.contains(_normalizeRelayUrl(normalizedUrl))) { + await settings.removeFromBlacklist(_normalizeRelayUrl(normalizedUrl)); _logger.i('Removed $normalizedUrl from blacklist - user manually added it'); }Also applies to: 379-383
♻️ Duplicate comments (10)
lib/features/relays/relays_notifier.dart (10)
58-68: Bootstrap default relay is acceptable (intent acknowledged)Keeping a hardcoded default relay for initial bootstrap is consistent with prior discussion and current Config contents. No change needed here.
Also applies to: 691-693
666-671: Null/empty pubkey comparison can be simplified and made safercurrentPubkey != null and currentPubkey!.isNotEmpty is brittle. Use nullable-aware checks.
- if (newPubkey != currentPubkey && - currentPubkey != null && - newPubkey.isNotEmpty && - currentPubkey!.isNotEmpty) { + if (newPubkey != currentPubkey && + newPubkey.isNotEmpty && + (currentPubkey?.isNotEmpty == true)) {
772-783: “Would leave no active relays” check should be canonicalRaw string comparisons will produce false negatives in the presence of formatting differences.
bool wouldLeaveNoActiveRelays(String urlToBlacklist) { - final currentActiveRelays = state.map((r) => r.url).toList(); - final currentBlacklist = settings.state.blacklistedRelays; + final currentActiveRelays = state.map((r) => _normalizeRelayUrl(r.url)).toList(); + final currentBlacklist = settings.state.blacklistedRelays.map(_normalizeRelayUrl).toList(); @@ - final wouldBeBlacklisted = [...currentBlacklist, urlToBlacklist]; + final wouldBeBlacklisted = [...currentBlacklist, _normalizeRelayUrl(urlToBlacklist)]; final wouldRemainActive = currentActiveRelays.where((url) => !wouldBeBlacklisted.contains(url)).toList();
840-848: Unify normalization helpers to avoid divergencetwo normalizers exist; most logic uses _normalizeRelayUrl while add/validation uses normalizeRelayUrl. Make the private helper delegate to the canonical normalizer and always trim+strip trailing slash to keep keys stable.
- /// Normalize relay URL to prevent duplicates (removes trailing slash) - String _normalizeRelayUrl(String url) { - url = url.trim(); - // Remove trailing slash if present - if (url.endsWith('/')) { - url = url.substring(0, url.length - 1); - } - return url; - } + /// Canonical key normalizer: delegate, then trim and strip trailing slash + String _normalizeRelayUrl(String url) { + final canonical = normalizeRelayUrl(url); + final base = (canonical ?? url).trim(); + return base.endsWith('/') ? base.substring(0, base.length - 1) : base; + }
478-490: Retry timer can leak and duplicate; track and cancel it_scheduleRetrySync creates unmanaged timers. Repeated calls accumulate background timers firing after disposal.
[suggested fix below]
StreamSubscription<RelayListEvent>? _relayListSubscription; Timer? _settingsWatchTimer; + Timer? _retrySyncTimer; @@ void _scheduleRetrySync(String mostroPubkey) { - Timer(const Duration(seconds: 10), () async { + _retrySyncTimer?.cancel(); + _retrySyncTimer = Timer(const Duration(seconds: 10), () async { try { if (settings.state.mostroPublicKey == mostroPubkey) { _logger.i('Retrying relay sync for Mostro: $mostroPubkey'); _subscriptionManager?.subscribeToMostroRelayList(mostroPubkey); } } catch (e) { _logger.w('Retry sync failed: $e'); } }); } @@ void dispose() { _relayListSubscription?.cancel(); _subscriptionManager?.dispose(); _settingsWatchTimer?.cancel(); + _retrySyncTimer?.cancel(); super.dispose(); }Also applies to: 850-856, 33-37
518-546: Canonicalize inputs and use set-based change detection; stabilize dedup hash
- event.validRelays hash uses the raw, order-sensitive join; a reorder triggers churn.
- Change detection relies on object equality; compare canonical URL sets instead.
- // Hash-based deduplication: ignore identical relay lists - final relayListHash = event.validRelays.join(','); + // Canonical, order-insensitive deduplication + final canonicalFromEvent = event.validRelays + .map(_normalizeRelayUrl) + .toSet() + .toList() + ..sort(); + final relayListHash = canonicalFromEvent.join(','); @@ - // Normalize relay URLs to prevent duplicates - final normalizedRelays = event.validRelays - .map((url) => _normalizeRelayUrl(url)) - .whereType<String>() // Filter out any null results - .toSet() // Remove duplicates - .toList(); + // Canonicalized relays from event + final normalizedRelays = canonicalFromEvent; @@ - // Update state if there are changes - if (finalRelays.length != state.length || - !finalRelays.every((relay) => state.contains(relay))) { + // Update state if canonical URL sets differ + final newUrls = finalRelays.map((r) => _normalizeRelayUrl(r.url)).toSet(); + final oldUrls = state.map((r) => _normalizeRelayUrl(r.url)).toSet(); + final hasChanges = newUrls.length != oldUrls.length || + !newUrls.containsAll(oldUrls) || !oldUrls.containsAll(newUrls); + if (hasChanges) { state = finalRelays; await _saveRelays(); _logger.i('Updated relay list with ${finalRelays.length} relays (${blacklistedUrls.length} blacklisted)'); }Also applies to: 547-553, 617-623
635-652: Blacklist operations should use canonical URLs end-to-endremoveRelayWithBlacklist uses raw matching for find, blacklist add, and removal; it will miss variants (trailing slash/case).
Future<void> removeRelayWithBlacklist(String url) async { - final relay = state.firstWhere((r) => r.url == url, orElse: () => Relay(url: '')); + final key = _normalizeRelayUrl(url); + final relay = state.firstWhere( + (r) => _normalizeRelayUrl(r.url) == key, + orElse: () => Relay(url: ''), + ); @@ - await settings.addToBlacklist(url); - _logger.i('Blacklisted ${relay.source} relay: $url'); + await settings.addToBlacklist(key); + _logger.i('Blacklisted ${relay.source} relay: $key'); @@ - await removeRelay(url); - _logger.i('Removed relay: $url (source: ${relay.source})'); + await removeRelay(key); + _logger.i('Removed relay: $key (source: ${relay.source})'); }
710-770: UI data mapping mixes raw and canonical URLs; can misclassify and misreport blacklist status
- Blacklist/active checks use raw URLs; normalization inconsistencies will show wrong isActive.
- Blacklisted-only entries are emitted with source: null; if you want to distinguish Mostro/default vs user-origin, track the last Mostro set and categorize accordingly.
List<MostroRelayInfo> get mostroRelaysWithStatus { - final blacklistedUrls = settings.state.blacklistedRelays; - final activeRelays = state.map((r) => r.url).toSet(); + final blacklistedUrls = settings.state.blacklistedRelays.map(_normalizeRelayUrl).toSet(); + final activeRelays = state.map((r) => _normalizeRelayUrl(r.url)).toSet(); @@ .map((r) => MostroRelayInfo( url: r.url, - // Check if this relay is blacklisted (even if it's still in state) - isActive: !blacklistedUrls.contains(r.url), + isActive: !blacklistedUrls.contains(_normalizeRelayUrl(r.url)), isHealthy: r.isHealthy, source: r.source, )) .toList(); @@ - final mostroBlacklistedRelays = blacklistedUrls - .where((url) => !activeRelays.contains(url)) + // Optional: restrict to last-known Mostro relays if tracked via _lastMostroRelays + final mostroBlacklistedRelays = blacklistedUrls + .where((url) => !activeRelays.contains(url) /* && _lastMostroRelays.contains(url) */) .map((url) => MostroRelayInfo( url: url, isActive: false, isHealthy: false, - source: null, // Unknown source for blacklisted-only relays + source: null, )) .toList(); @@ .map((r) => MostroRelayInfo( url: r.url, - isActive: !blacklistedUrls.contains(r.url), // User relays can also be blacklisted + isActive: !blacklistedUrls.contains(_normalizeRelayUrl(r.url)), // User relays can also be blacklisted isHealthy: r.isHealthy, source: r.source, )) .toList();
785-806: Blacklist toggling must use canonical URLs for correctnessUsing raw URL for contains/add/remove causes drift with normalized state and will fail on variants.
Future<void> toggleMostroRelayBlacklist(String url) async { - final isCurrentlyBlacklisted = settings.state.blacklistedRelays.contains(url); + final key = _normalizeRelayUrl(url); + final isCurrentlyBlacklisted = settings.state.blacklistedRelays + .map(_normalizeRelayUrl) + .toSet() + .contains(key); @@ - await settings.removeFromBlacklist(url); - _logger.i('Removed $url from blacklist, triggering re-sync'); + await settings.removeFromBlacklist(key); + _logger.i('Removed $key from blacklist, triggering re-sync'); @@ - await settings.addToBlacklist(url); - await removeRelay(url); - _logger.i('Blacklisted and removed Mostro relay: $url'); + await settings.addToBlacklist(key); + await removeRelay(key); + _logger.i('Blacklisted and removed Mostro relay: $key'); } }
81-103: Blacklist not applied canonically during persistence (can re-enable blocked relays)Saving uses raw string equality against the blacklist. Trailing slashes/case/format drift will bypass the blacklist and re-persist blocked relays. Normalize both sides and dedupe before saving.
[suggested fix below]
Future<void> _saveRelays() async { - // Get blacklisted relays - final blacklistedUrls = settings.state.blacklistedRelays; + // Canonical blacklist set + final blacklistedUrls = settings.state.blacklistedRelays + .map(_normalizeRelayUrl) + .toSet(); @@ - // Include ALL active relays (Mostro/default + user) that are NOT blacklisted - final allActiveRelayUrls = state - .where((r) => !blacklistedUrls.contains(r.url)) - .map((r) => r.url) - .toList(); + // Persist only active, non-blacklisted relays (canonical compare) and dedupe + final allActiveRelayUrls = state + .where((r) => !blacklistedUrls.contains(_normalizeRelayUrl(r.url))) + .map((r) => _normalizeRelayUrl(r.url)) + .toSet() + .toList(); @@ - _logger.i('Saving ${allActiveRelayUrls.length} active relays (excluding ${blacklistedUrls.length} blacklisted) and ${userRelays.length} user relays metadata'); + _logger.i('Saving ${allActiveRelayUrls.length} active relays (excluding ${blacklistedUrls.length} blacklisted) and ${userRelays.length} user relays metadata');
🧹 Nitpick comments (2)
lib/features/relays/relays_notifier.dart (2)
579-586: Avoid sentinel “empty Relay(url: '')”; use presence checksUsing a fake Relay as a sentinel complicates reasoning. Prefer where().isNotEmpty or firstWhereOrNull.
- final existingUserRelay = userRelays.firstWhere( - (r) => _normalizeRelayUrl(r.url) == relayUrl, - orElse: () => Relay(url: ''), // Empty relay if not found - ); - - if (existingUserRelay.url.isNotEmpty) { + final existingUserRelay = userRelays.where( + (r) => _normalizeRelayUrl(r.url) == relayUrl, + ); + if (existingUserRelay.isNotEmpty) { // PROMOTION: User relay → Mostro relay (move to beginning) userRelays.removeWhere((r) => _normalizeRelayUrl(r.url) == relayUrl); final promotedRelay = Relay.fromMostro(relayUrl); updatedRelays.insert(0, promotedRelay); // Insert at beginning _logger.i('Promoted user relay to Mostro relay: $relayUrl'); continue; }
674-676: Non-English comments in an otherwise English codebaseMinor, but comments in Spanish break consistency. Consider rewriting to English for team-wide clarity.
- Line 674: “RESET COMPLETO: …”
- Line 690: “LIMPIAR TODOS …”
- Line 701: “Iniciar sync completamente fresco …”
Also applies to: 690-697
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/features/relays/relays_notifier.dart(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
lib/!(generated)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/relays/relays_notifier.dart
🧠 Learnings (3)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mobile#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.
📚 Learning: 2025-08-21T14:45:43.964Z
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.964Z
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/relays/relays_notifier.dart
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
PR: MostroP2P/mobile#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.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/features/relays/relays_notifier.dart (2)
41-48: Init flow and deferred sync look solidConstructor sequencing is sensible: load persisted relays, initialize Mostro sync, start settings watcher, then defer the first sync via microtask to avoid provider init races. No changes requested.
120-157: Unify and relax URL canonicalizationAfter inspecting both implementations, we’ve confirmed that:
normalizeRelayUrl(lib/features/relays/relays_notifier.dart:121-137)
- Lowercases input and trims, but relies on
isValidDomainFormatwhich rejects IPs (e.g. “127.0.0.1”) and any host:port syntax (e.g. “relay.example.com:7000”).- Does not strip a trailing slash for already-prefixed URLs.
_normalizeRelayUrl(lib/features/relays/relays_notifier.dart:841-848)
- Only trims and removes one trailing slash.
This mismatch means valid relays with custom ports or IPs get rejected on manual addition, and any trailing-slash variants won’t be deduplicated correctly against internal URLs.
Proposed minimal refactor
Replace both routines with a single parser-based canonicalizer that:
- Accepts
wss://host[:port][/path](optionally IPs)- Lowercases only the scheme & host
- Strips exactly one trailing slash
- Returns
nullfor invalid URIs or non-wssschemesString? normalizeRelayUrl(String input) { - input = input.trim().toLowerCase(); - if (!isValidDomainFormat(input)) return null; - if (input.startsWith('wss://')) { - return input; - } else if (input.startsWith('ws://') || input.startsWith('http')) { - return null; - } else { - return 'wss://$input'; - } + final raw = input.trim(); + final candidate = raw.toLowerCase().startsWith('wss://') ? raw : 'wss://$raw'; + final uri = Uri.tryParse(candidate); + if (uri == null || uri.scheme != 'wss' || uri.host.isEmpty) return null; + // TODO: decide whether IP hosts and non-443 ports should be allowed + final portPart = uri.hasPort ? ':${uri.port}' : ''; + final pathPart = (uri.path == '/' || uri.path.isEmpty) ? '' : uri.path; + var url = 'wss://${uri.host}$portPart$pathPart'; + if (url.endsWith('/')) url = url.substring(0, url.length - 1); + return url; }
- Remove or repurpose
isValidDomainFormatonce this parser-based approach is in place.- Add or update unit tests to cover:
• Custom ports (e.g.wss://relay.example.com:7000)
• IP addresses (e.g.wss://192.168.0.1)
• Trailing slashes and optional pathsQuestions for implementation
- Do we intend to support IP addresses and non-default ports for manual relay entries?
- Should the canonicalizer lower-case the entire URL (including path) or only scheme/host?
Please confirm product requirements before proceeding.
- Prevent Mostro instance contamination in relay synchronization - Fix race condition causing orders to disappear from UI - Ensure proper initialization order between SessionNotifier and SubscriptionManager
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/subscriptions/subscription_manager.dart (1)
50-60: Do not tear down the RelayList subscription on session changesThe session-driven loops iterate over all SubscriptionType values, which includes relayList. This will cancel the dedicated relay-list subscription any time sessions change, defeating continuous Mostro sync. Limit session-managed updates to orders/chat only.
Apply this diff:
class SubscriptionManager { final Ref ref; final Map<SubscriptionType, Subscription> _subscriptions = {}; final _logger = Logger(); late final ProviderSubscription _sessionListener; + // Only these types are managed by session changes + Iterable<SubscriptionType> get _sessionManagedTypes => + SubscriptionType.values.where((t) => t != SubscriptionType.relayList); + final _ordersController = StreamController<NostrEvent>.broadcast(); final _chatController = StreamController<NostrEvent>.broadcast(); final _relayListController = StreamController<RelayListEvent>.broadcast(); @@ void _updateAllSubscriptions(List<Session> sessions) { if (sessions.isEmpty) { _logger.i('No sessions available, clearing all subscriptions'); - _clearAllSubscriptions(); + _clearSessionManagedSubscriptions(); return; } - for (final type in SubscriptionType.values) { + for (final type in _sessionManagedTypes) { _updateSubscription(type, sessions); } } - void _clearAllSubscriptions() { - for (final type in SubscriptionType.values) { + void _clearSessionManagedSubscriptions() { + for (final type in _sessionManagedTypes) { unsubscribeByType(type); } } @@ void subscribeAll() { - unsubscribeAll(); + _clearSessionManagedSubscriptions(); final currentSessions = ref.read(sessionNotifierProvider); _updateAllSubscriptions(currentSessions); }Also applies to: 62-66, 224-234
lib/features/relays/relays_notifier.dart (1)
173-186: Bug: cleanup in _testNostrProtocol can disconnect the global Nostr instanceOn error you call _cleanupTestConnection(Nostr.instance). This risks dropping the app’s global connections. Also, testNostr is scoped to try{} so it’s unavailable in catch.
Refactor to keep a local variable accessible to catch/finally and never touch the singleton:
- Future<bool> _testNostrProtocol(String url) async { - // Generate unique subscription ID for this test + Future<bool> _testNostrProtocol(String url) async { + // Generate unique subscription ID for this test final testSubId = 'relay_test_${DateTime.now().millisecondsSinceEpoch}'; bool receivedEvent = false; bool receivedEose = false; bool isConnected = false; - - try { - // Create isolated instance for testing - final testNostr = Nostr(); + Nostr? testNostr; + try { + // Create isolated instance for testing + testNostr = Nostr(); @@ - // Clean up connection - await _cleanupTestConnection(testNostr); + // Clean up connection + await _cleanupTestConnection(testNostr); // Relay is healthy if we received either EVENT or EOSE (or both) return receivedEvent || receivedEose; } catch (e) { - // Protocol test failed with error - try { - await _cleanupTestConnection(Nostr.instance); - } catch (_) { - // Ignore cleanup errors - } + // Protocol test failed with error + if (testNostr != null) { + try { await _cleanupTestConnection(testNostr); } catch (_) {} + } return false; } }Optional: if the SDK exposes a dispose/close on Nostr, call it after disconnecting to free resources.
Also applies to: 260-267
♻️ Duplicate comments (6)
lib/features/relays/relays_notifier.dart (6)
53-79: Bootstrap default relay is fine (intentional)Hardcoding wss://relay.mostro.network as bootstrap is consistent with prior rationale that Config.nostrRelays contains only this active relay. No change requested.
662-689: Stabilize pubkey change detection; avoid fragile null/empty checksSimplify and harden the condition.
- if (newPubkey != currentPubkey && - currentPubkey != null && - newPubkey.isNotEmpty && - currentPubkey!.isNotEmpty) { + if (newPubkey != currentPubkey && + currentPubkey?.isNotEmpty == true && + newPubkey.isNotEmpty) {
81-104: Persist using canonical URLs and dedupeSaving raw URLs risks duplicating entries and missing blacklist matches.
- final blacklistedUrls = settings.state.blacklistedRelays; + final blacklistedUrls = settings.state.blacklistedRelays + .map(_normalizeRelayUrl) + .toSet(); @@ - final allActiveRelayUrls = state - .where((r) => !blacklistedUrls.contains(r.url)) - .map((r) => r.url) - .toList(); + final allActiveRelayUrls = state + .where((r) => !blacklistedUrls.contains(_normalizeRelayUrl(r.url))) + .map((r) => _normalizeRelayUrl(r.url)) + .toSet() + .toList();
846-855: Unify normalization: delegate to normalizeRelayUrl and keep trailing-slash trimCurrently there are two normalization strategies (_normalizeRelayUrl and normalizeRelayUrl). Delegate to one to avoid drift.
- /// Normalize relay URL to prevent duplicates (removes trailing slash) - String _normalizeRelayUrl(String url) { - url = url.trim(); - // Remove trailing slash if present - if (url.endsWith('/')) { - url = url.substring(0, url.length - 1); - } - return url; - } + /// Normalize relay URL (canonicalize and strip trailing slash) + String _normalizeRelayUrl(String url) { + final canonical = normalizeRelayUrl(url); + return (canonical ?? url).trim().replaceAll(RegExp(r'/$'), ''); + }This preserves your prior trailing-slash behavior while using the canonical path when possible.
624-629: Change detection should compare canonical URL sets, not Relay object equalityList.contains with Relay objects relies on equality semantics and order; it will often re-save unnecessarily or miss changes.
- if (finalRelays.length != state.length || - !finalRelays.every((relay) => state.contains(relay))) { - state = finalRelays; - await _saveRelays(); - _logger.i('Updated relay list with ${finalRelays.length} relays (${blacklistedUrls.length} blacklisted)'); - } + final newUrls = finalRelays.map((r) => _normalizeRelayUrl(r.url)).toSet(); + final oldUrls = state.map((r) => _normalizeRelayUrl(r.url)).toSet(); + final hasChanges = + newUrls.length != oldUrls.length || + !newUrls.containsAll(oldUrls) || + !oldUrls.containsAll(newUrls); + if (hasChanges) { + state = finalRelays; + await _saveRelays(); + _logger.i('Updated relay list with ${finalRelays.length} relays (${blacklistedUrls.length} blacklisted)'); + }
382-384: Normalize URLs consistently for blacklist reads/writes and UI statusMultiple places compare raw strings to settings.state.blacklistedRelays, which can miss due to trailing slashes/case variance. Canonicalize both sides.
Key changes:
- When checking/adding/removing blacklist entries: call _normalizeRelayUrl on the target URL and normalize the blacklist set before contains/mutations.
- In mostroRelaysWithStatus and wouldLeaveNoActiveRelays: compute active/blacklist sets using normalized URLs.
I can provide a unified patch set if you prefer me to commit it across these blocks.
Also applies to: 716-720, 724-776, 778-789, 791-812
🧹 Nitpick comments (4)
lib/features/subscriptions/subscription_manager.dart (1)
236-253: Dedicated relay-list subscription path is clean; consider adding since for freshnessThe dedicated flow and controller are appropriate. As an optional improvement, add a since filter to bias toward recent 10002 events (in addition to limit:1), preventing very old pinned events from winning.
Example:
final filter = NostrFilter( kinds: [10002], authors: [mostroPubkey], limit: 1, // Only get the most recent relay list + since: DateTime.now() + .subtract(const Duration(days: 30)) + .millisecondsSinceEpoch ~/ 1000, );Only apply if your relay ecosystem doesn’t rely on long-lived pinned lists.
Also applies to: 255-294
lib/features/relays/relays_notifier.dart (3)
121-159: Domain regex excludes ports; some relays use non-default portsisValidDomainFormat currently rejects “example.com:7447”. If you intend to support explicit ports, allow an optional :port suffix.
- final domainRegex = RegExp( - r'^[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$'); - return domainRegex.hasMatch(input) && input.contains('.'); + final domainRegex = RegExp( + r'^[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?' + r'(\.[a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*' + r'(:\d{1,5})?$'); + return domainRegex.hasMatch(input) && input.contains('.');
544-549: Make dedup hash canonical to avoid order-only churnCurrently relayListHash uses event.validRelays.join(','). If the same set arrives in a different order, you’ll reprocess unnecessarily.
- final relayListHash = event.validRelays.join(','); + final relayListHash = (event.validRelays + .map(_normalizeRelayUrl) + .toSet() + .toList() + ..sort()) + .join(',');This aligns hashing with your normalization step below.
Also applies to: 553-559
584-592: Avoid sentinel Relay(url: '') and linear searchUsing a fake “empty” Relay makes logic brittle. Use indexWhere or any/firstWhereOrNull.
- final existingUserRelay = userRelays.firstWhere( - (r) => _normalizeRelayUrl(r.url) == relayUrl, - orElse: () => Relay(url: ''), // Empty relay if not found - ); - - if (existingUserRelay.url.isNotEmpty) { + final idx = userRelays.indexWhere( + (r) => _normalizeRelayUrl(r.url) == relayUrl, + ); + if (idx != -1) { // PROMOTION: User relay → Mostro relay (move to beginning) - userRelays.removeWhere((r) => _normalizeRelayUrl(r.url) == relayUrl); + userRelays.removeAt(idx); final promotedRelay = Relay.fromMostro(relayUrl); updatedRelays.insert(0, promotedRelay); // Insert at beginning _logger.i('Promoted user relay to Mostro relay: $relayUrl'); continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/features/relays/relays_notifier.dart(5 hunks)lib/features/subscriptions/subscription_manager.dart(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
lib/!(generated)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/relays/relays_notifier.dartlib/features/subscriptions/subscription_manager.dart
🧠 Learnings (6)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mobile#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.
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.964Z
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.
📚 Learning: 2025-08-21T14:45:43.964Z
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.964Z
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/relays/relays_notifier.dartlib/features/subscriptions/subscription_manager.dart
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
PR: MostroP2P/mobile#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.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, 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/subscriptions/subscription_manager.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
lib/features/subscriptions/subscription_manager.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/data/models.dart : Export models via the barrel file lib/data/models.dart
Applied to files:
lib/features/subscriptions/subscription_manager.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
lib/features/subscriptions/subscription_manager.dart (4)
134-140: Event routing for RelayList looks goodParsing via RelayListEvent.fromEvent and pushing into _relayListController isolates concerns cleanly.
301-307: Controller disposal is correctAll broadcast controllers, including the new _relayListController, are closed in dispose. Good resource hygiene.
166-173: Handle nullablesubscriptionIdbefore unsubscribingIt looks like
request.subscriptionIdisn’t set when you construct theNostrRequestinsubscription_manager.dart, so callingref.read(nostrServiceProvider).unsubscribe(request.subscriptionId!);will throw if the SDK doesn’t backfill it. I wasn’t able to find a guarantee in the code that
subscribeToEvents(...)populates the field beforeonCancelruns—please double-check that contract.If there is no such guarantee, I recommend:
- Generating and assigning a unique
subscriptionIdwhen you create the request.- Safely unwrapping
subscriptionIdin youronCancelhandlers.Locations to update:
- lib/features/subscriptions/subscription_manager.dart
- Lines 166–173
- Lines 281–285
- lib/features/subscriptions/subscription_manager.dart (the
_subscribeToRelayListhelper)- Also consider doing the same in lib/background/background.dart (around line 62), where
activeSubscriptions[request.subscriptionId!]is used.Suggested diff for one handler (apply similarly elsewhere):
final request = NostrRequest( + subscriptionId: 'sub_${type.name}_${DateTime.now().millisecondsSinceEpoch}', filters: [filter], ); @@ final subscription = Subscription( request: request, streamSubscription: streamSubscription, onCancel: () { - ref.read(nostrServiceProvider).unsubscribe(request.subscriptionId!); + final id = request.subscriptionId; + if (id != null) { + ref.read(nostrServiceProvider).unsubscribe(id); + } else { + _logger.w('Cannot unsubscribe—no subscriptionId for $type'); + } }, );If the SDK truly always backfills
subscriptionIdon the request before handlers run, please point to that guarantee so we can drop this defensive code.
42-48: Resolve: Initial subscriptions are already handled viasubscribeAllin LifecycleManagerThe potential gap with
fireImmediately: falseis mitigated by a manual invocation ofsubscribeAll()immediately after theSubscriptionManageris constructed in the app’s initialization flow. Specifically:
- In
lib/services/lifecycle_manager.dartat line 62, the app reads thesubscriptionManagerProviderand callssubscriptionManager.subscribeAll(), ensuring all active sessions are subscribed before any session change listener is needed.Since
subscribeAll()explicitly calls_updateAllSubscriptionswith the current session list, no additional changes are required aroundfireImmediately. You can safely leavefireImmediately: falseas-is.lib/features/relays/relays_notifier.dart (2)
416-438: No action needed: NostrService readiness contract verifiedThe
NostrServiceclass defines a private_isInitializedflag with a publicbool get isInitializedand itssubscribeToEventsmethod throws if called before initialization. Inrelays_notifier.dart, the code correctly checksnostrService.isInitializedbefore subscribing. Thus, the readiness contract is already in place and functioning as intended.
6-11: Confirm nested import necessityI wasn’t able to determine from the local codebase whether
package:dart_nostr/dart_nostr.dartre-exportsNostrRequestEoseCommanddefined innostr/model/ease.dart. Please verify in thedart_nostrpackage’s top-level library:
- Open
dart_nostr.dart(usually atlib/dart_nostr.dartin the dependency).- Search for an export or show directive for
NostrRequestEoseCommand(e.g.export 'nostr/model/ease.dart';orshow NostrRequestEoseCommand).Then:
- If it’s already re-exported, you can remove the direct nested import
and import only the top-level library:import 'package:dart_nostr/nostr/model/ease.dart';import 'package:dart_nostr/dart_nostr.dart';- If not, keep the nested import as-is to access
NostrRequestEoseCommand.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/relays/relays_notifier.dart (1)
121-134: Two normalizers: unify behavior to one canonical formYou have normalizeRelayUrl(...) (adds wss://, lowercases) and _normalizeRelayUrl(...) (trim + strip trailing slash). Divergent logic can cause mismatches. Delegate the private helper to the canonical one and always strip a trailing slash to preserve prior behavior.
- /// Normalize relay URL to prevent duplicates (removes trailing slash) - String _normalizeRelayUrl(String url) { - url = url.trim(); - // Remove trailing slash if present - if (url.endsWith('/')) { - url = url.substring(0, url.length - 1); - } - return url; - } + /// Normalize using the canonical rules, then ensure trailing slash is removed + String _normalizeRelayUrl(String url) { + final canonical = normalizeRelayUrl(url); + return (canonical ?? url).trim().replaceAll(RegExp(r'/$'), ''); + }
♻️ Duplicate comments (8)
lib/features/relays/relays_notifier.dart (8)
584-596: Avoid sentinel objects; use safe lookup for promotionCreating a dummy
Relay(url: '')is brittle. Prefer a where/isNotEmpty check.- final existingUserRelay = userRelays.firstWhere( - (r) => _normalizeRelayUrl(r.url) == relayUrl, - orElse: () => Relay(url: ''), // Empty relay if not found - ); - - if (existingUserRelay.url.isNotEmpty) { + final existingUser = userRelays.where( + (r) => _normalizeRelayUrl(r.url) == relayUrl, + ); + if (existingUser.isNotEmpty) { // PROMOTION: User relay → Mostro relay (move to beginning) - userRelays.removeWhere((r) => _normalizeRelayUrl(r.url) == relayUrl); + userRelays.removeWhere((r) => _normalizeRelayUrl(r.url) == relayUrl); final promotedRelay = Relay.fromMostro(relayUrl); updatedRelays.insert(0, promotedRelay); // Insert at beginning _logger.i('Promoted user relay to Mostro relay: $relayUrl'); continue; }
717-723: Read path uses raw URL for blacklist checksNormalize before contains to keep behavior consistent with storage.
- bool isRelayBlacklisted(String url) { - return settings.state.blacklistedRelays.contains(url); - } + bool isRelayBlacklisted(String url) { + final key = _normalizeRelayUrl(url); + return settings.state.blacklistedRelays.map(_normalizeRelayUrl).toSet().contains(key); + }
728-776: UI model: canonicalize URLs and separate user vs Mostro-only blacklisted
- Normalize both active and blacklisted sets.
- Optionally use
_lastMostroRelaysto categorize “blacklisted Mostro/default” vs user-blacklisted.- final blacklistedUrls = settings.state.blacklistedRelays; - final activeRelays = state.map((r) => r.url).toSet(); + final blacklistedUrls = settings.state.blacklistedRelays.map(_normalizeRelayUrl).toSet(); + final activeRelays = state.map((r) => _normalizeRelayUrl(r.url)).toSet(); @@ - isActive: !blacklistedUrls.contains(r.url), + isActive: !blacklistedUrls.contains(_normalizeRelayUrl(r.url)), @@ - final mostroBlacklistedRelays = blacklistedUrls - .where((url) => !activeRelays.contains(url)) + final mostroBlacklistedRelays = blacklistedUrls + .where((url) => !activeRelays.contains(url) && _lastMostroRelays.contains(url)) .map((url) => MostroRelayInfo( url: url, isActive: false, isHealthy: false, source: null, )) .toList(); @@ - isActive: !blacklistedUrls.contains(r.url), // User relays can also be blacklisted + isActive: !blacklistedUrls.contains(_normalizeRelayUrl(r.url)), // User relays can also be blacklisted
779-789: Simulation should use canonical URLsPrevent false positives when case or trailing slash differ.
- final currentActiveRelays = state.map((r) => r.url).toList(); - final currentBlacklist = settings.state.blacklistedRelays; + final currentActiveRelays = state.map((r) => _normalizeRelayUrl(r.url)).toList(); + final currentBlacklist = settings.state.blacklistedRelays.map(_normalizeRelayUrl).toList(); @@ - final wouldBeBlacklisted = [...currentBlacklist, urlToBlacklist]; + final wouldBeBlacklisted = [...currentBlacklist, _normalizeRelayUrl(urlToBlacklist)];
623-629: Change detection via object equality is brittle; compare canonical URL setsIf Relay doesn’t override equality, list-contains will misdetect changes. Compare sets of canonical URLs.
- if (finalRelays.length != state.length || - !finalRelays.every((relay) => state.contains(relay))) { - state = finalRelays; - await _saveRelays(); - _logger.i('Updated relay list with ${finalRelays.length} relays (${blacklistedUrls.length} blacklisted)'); - } + final oldSet = state.map((r) => _normalizeRelayUrl(r.url)).toSet(); + final newSet = finalRelays.map((r) => _normalizeRelayUrl(r.url)).toSet(); + if (oldSet.length != newSet.length || !oldSet.containsAll(newSet)) { + state = finalRelays; + await _saveRelays(); + _logger.i('Updated relay list with ${finalRelays.length} relays (${blacklistedUrls.length} blacklisted)'); + }Optionally track the last Mostro set for UI categorization:
// Add at class level: Set<String> _lastMostroRelays = {}; // After computing normalizedRelays: _lastMostroRelays = normalizedRelays.toSet();
644-657: Blacklist operations should use canonical URLs end-to-endAdd/remove via raw URL can drift from stored canonical blacklist; normalize before lookup and removal.
- final relay = state.firstWhere((r) => r.url == url, orElse: () => Relay(url: '')); + final key = _normalizeRelayUrl(url); + final relay = state.firstWhere((r) => _normalizeRelayUrl(r.url) == key, orElse: () => Relay(url: '')); @@ - await settings.addToBlacklist(url); - _logger.i('Blacklisted ${relay.source} relay: $url'); + await settings.addToBlacklist(key); + _logger.i('Blacklisted ${relay.source} relay: $key'); @@ - await removeRelay(url); - _logger.i('Removed relay: $url (source: ${relay.source})'); + await removeRelay(key); + _logger.i('Removed relay: $key (source: ${relay.source})');
795-811: Toggle path should normalize; also reset hash already handled—goodUse canonical URL for stable behavior across representations.
- final isCurrentlyBlacklisted = settings.state.blacklistedRelays.contains(url); + final key = _normalizeRelayUrl(url); + final isCurrentlyBlacklisted = settings.state.blacklistedRelays + .map(_normalizeRelayUrl) + .toSet() + .contains(key); @@ - await settings.removeFromBlacklist(url); - _logger.i('Removed $url from blacklist, triggering re-sync'); + await settings.removeFromBlacklist(key); + _logger.i('Removed $key from blacklist, triggering re-sync'); @@ - await settings.addToBlacklist(url); - await removeRelay(url); - _logger.i('Blacklisted and removed Mostro relay: $url'); + await settings.addToBlacklist(key); + await removeRelay(key); + _logger.i('Blacklisted and removed Mostro relay: $key');
81-104: Persisting active relays ignores canonicalization; blacklist match may missFiltering with raw strings risks persisting blacklisted relays that differ only by case or trailing slash. Canonicalize both sides and dedupe before saving.
- // Get blacklisted relays - final blacklistedUrls = settings.state.blacklistedRelays; + // Canonical blacklist + final blacklistedUrls = settings.state.blacklistedRelays + .map(_normalizeRelayUrl) + .toSet(); @@ - // Include ALL active relays (Mostro/default + user) that are NOT blacklisted - final allActiveRelayUrls = state - .where((r) => !blacklistedUrls.contains(r.url)) - .map((r) => r.url) - .toList(); + // Include ALL active relays not in the canonical blacklist; persist canonical URLs + final allActiveRelayUrls = state + .where((r) => !blacklistedUrls.contains(_normalizeRelayUrl(r.url))) + .map((r) => _normalizeRelayUrl(r.url)) + .toSet() // dedupe + .toList();
🧹 Nitpick comments (12)
CLAUDE.md (4)
20-25: Grammar: clarify frequency qualifiers and fix minor wordingTighten wording for consistency and correctness.
-### Essential Commands for Code Changes -- **`flutter analyze`** - ✅ **ALWAYS run after any code change** - Mandatory before commits -- **`flutter test`** - ✅ **ALWAYS run after any code change** - Mandatory before commits -- **`dart run build_runner build -d`** - 🟡 **Only when code generation needed** (models, providers, mocks, localization) -- **`flutter test integration_test/`** - 🟡 **Only for significant changes** (core services, main flows) +### Essential Commands for Code Changes +- **`flutter analyze`** — ✅ **Always run after any code change**; mandatory before commits +- **`flutter test`** — ✅ **Always run after any code change**; mandatory before commits +- **`dart run build_runner build -d`** — 🟡 **Only when code generation is needed** (models, providers, mocks, localization) +- **`flutter test integration_test/`** — 🟡 **Run for significant changes** (core services, main flows)
262-266: markdownlint: specify language for fenced blockAdd a language hint to satisfy MD040 and improve readability.
-``` +```text User removes Mostro relay → Added to blacklist → Never re-added during sync User manually adds relay → Removed from blacklist → Works as user relay--- `177-182`: **Enforce “English-only” code comments guideline consistently** You mandate English-only comments here—good. There are a few Spanish comments left in code (see relays_notifier.dart Lines 680–681 and settings_notifier.dart Lines 58–60). Replace them with English to comply. --- `487-491`: **Stability of “Latest stable” labels** Optional: record exact Flutter/Dart versions to reduce ambiguity in future debugging. If you prefer “latest stable,” consider linking to channel pinning instructions. </blockquote></details> <details> <summary>lib/features/settings/settings_notifier.dart (4)</summary><blockquote> `56-61`: **Non-English inline comments in a user-facing repo** Replace Spanish comments with English per CLAUDE.md policy. ```diff - // COMPLETE RESET: Clear blacklist and user relays when changing Mostro + // COMPLETE RESET: Clear blacklist and user relays when changing Mostro state = state.copyWith( mostroPublicKey: newValue, - blacklistedRelays: const [], // Blacklist vacío - userRelays: const [], // User relays vacíos + blacklistedRelays: const [], // Empty blacklist + userRelays: const [], // Empty user relays );
11-16: Unused ref field (confirm necessity)
final Ref? ref;is not referenced in this file. If it’s only there for future use, fine; otherwise, remove to avoid analyzer warnings.If unused, adjust:
- final Ref? ref; - final _logger = Logger(); + final _logger = Logger(); @@ - SettingsNotifier(this._prefs, {this.ref}) : super(_defaultSettings()); + SettingsNotifier(this._prefs) : super(_defaultSettings());
86-96: Blacklist normalization: good; consider dedupe on migrationYou normalize on add and remove—nice. If legacy data contained mixed-case or trailing slashes, you might want a one-time normalization/dedupe pass in init().
110-124: Consistency: normalized checks and getters
isRelayBlacklisted()uses normalized input—good. Optionally expose anormalizedBlacklistedRelaysgetter to simplify consumers that need canonical forms.lib/features/relays/relays_notifier.dart (4)
545-556: Dedup hash should use canonical URLsHashing raw
validRelays.join(',')can cause useless updates due to slash/case differences. Build the hash from canonical URLs after normalization.- final relayListHash = event.validRelays.join(','); + final canonicalSet = event.validRelays + .map(_normalizeRelayUrl) + .toSet(); + final relayListHash = canonicalSet.join(',');
672-686: English-only comments; also consider replacing polling with ref.listen
- Replace the Spanish comment.
- Optional: in Riverpod, ref.listen on the settings provider avoids a 5s polling timer (useful if you can structure providers to avoid cycles).
- _logger.i('Detected REAL Mostro pubkey change: $currentPubkey -> $newPubkey'); + _logger.i('Detected Mostro pubkey change: $currentPubkey -> $newPubkey'); @@ - // 🔥 RESET COMPLETO: Limpiar todos los relays y hacer sync fresco + // Full reset: clear all relays and perform a fresh sync _cleanAllRelaysAndResync();Alternative (outside this range):
// In constructor or init, if feasible: ref.listen<Settings>(settingsProvider.select((s) => s.state), (prev, next) { if (prev?.mostroPublicKey != null && prev!.mostroPublicKey.isNotEmpty && next.mostroPublicKey.isNotEmpty && prev.mostroPublicKey != next.mostroPublicKey) { _logger.i('Detected Mostro pubkey change: ${prev.mostroPublicKey} -> ${next.mostroPublicKey}'); _cleanAllRelaysAndResync(); } });
498-522: Service readiness poll: OK; consider a bounded exponential backoffCurrent fixed-interval polling is fine. For robustness on slow startups, a short exponential backoff could reduce wakeups.
403-414: Health refresh is stubbed to “always healthy”If the UI depends on health, consider wiring it to actual NostrService relay connection states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CLAUDE.md(11 hunks)lib/features/relays/relays_notifier.dart(5 hunks)lib/features/settings/settings_notifier.dart(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
lib/!(generated)/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/!(generated)/**/*.dart: Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Pass BuildContext to methods that need localization rather than accessing localization without context
Use latest, non-deprecated Flutter/Dart APIs (e.g., Color.withValues() instead of withOpacity())
Check mounted before using BuildContext after async gaps
Remove unused imports and dependencies
Use const constructors where possible
Files:
lib/features/relays/relays_notifier.dartlib/features/settings/settings_notifier.dart
🧠 Learnings (15)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mobile#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.
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.964Z
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.
📚 Learning: 2025-08-21T14:45:43.964Z
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.964Z
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/relays/relays_notifier.dartlib/features/settings/settings_notifier.dartCLAUDE.md
📚 Learning: 2025-08-19T17:54:15.016Z
Learnt from: Catrya
PR: MostroP2P/mobile#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.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/core/mostro_fsm.dart : Manage order state transitions via MostroFSM
Applied to files:
lib/features/settings/settings_notifier.dart
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Run dart run build_runner build -d after installing dependencies or when code generation is needed
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/l10n/*.arb : After modifying ARB files, regenerate localization outputs (e.g., dart run build_runner build -d or flutter gen-l10n)
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Run flutter analyze and keep zero issues before committing
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to **/*.mocks.dart : Never manually edit *.mocks.dart Mockito-generated files
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to test/mocks.mocks.dart : Do not add individual // ignore: must_be_immutable comments; rely on the existing file-level ignore
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to test/mocks.mocks.dart : Never manually edit test/mocks.mocks.dart; regenerate with build_runner when needed
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to **/*.mocks.dart : Do not add ignore comments to generated mock files; regenerate instead
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to **/*.g.dart : Do not add ignore comments to generated files (e.g., // ignore: ...); regenerate instead
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to **/*.g.dart : Never manually edit *.g.dart files
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/!(generated)/**/*.dart : Always use localized strings via S.of(context)!.keyName; avoid hardcoded user-facing text
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-15T13:19:31.999Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-15T13:19:31.999Z
Learning: Applies to lib/**/@(screens|widgets)/**/*.dart : Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs) instead of inline side effects in build methods
Applied to files:
CLAUDE.md
🪛 LanguageTool
CLAUDE.md
[grammar] ~23-~23: There might be a mistake here.
Context: ...nner build -d`** - 🟡 Only when code generation needed (models, providers, mocks, loca...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ...aysNotifier #### Manual Relay Addition - Users can manually add relays viaaddRe...
(QB_NEW_EN)
[grammar] ~74-~74: There might be a mistake here.
Context: ...ource tracking: Manual relays marked as RelaySource.user - Blacklist override: Manual addition auto...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...lacklist #### Dual Storage Architecture - Active Storage: settings.relays cont...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...- Active Storage: settings.relays contains active relay list used by NostrService ...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...Blacklist()adds Mostro/default relays to blacklist,removeRelay()` permanently ...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...orage locations #### Instance Isolation - Author pubkey validation prevents relay ...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...omState()` #### Relay Validation System - Two-tier connectivity testing: Primary N...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...est (REQ/EVENT/EOSE), WebSocket fallback - Domain-only policy: IP addresses complet...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ...policy: IP addresses completely rejected - URL normalization: Trailing slash remova...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...slash removal prevents duplicate entries - Instance-isolated testing: Test connecti...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...ion Process ### Initialization Sequence The app follows a specific initializatio...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ...zation**: Loads active trading sessions from Sembast database 4. **SubscriptionManag...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...ion**: Registers session listeners with fireImmediately: false 5. Background Services Setup: Configures ...
(QB_NEW_EN)
[grammar] ~106-~106: There might be a mistake here.
Context: ...ssions ### Critical Timing Requirements - SessionNotifier must complete initializa...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ...ontains comprehensive ignore directives at file level - Regeneration: Use `dar...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...rns ### Code Comments and Documentation - All code comments must be in English -...
(QB_NEW_EN)
[grammar] ~181-~181: There might be a mistake here.
Context: ...uld be in English - User-facing strings use localization system (`S.of(context).key...
(QB_NEW_EN)
[grammar] ~216-~216: There might be a mistake here.
Context: ...lay Synchronization System ### Overview Comprehensive system that automatically ...
(QB_NEW_EN)
[grammar] ~221-~221: There might be a mistake here.
Context: ...lib/core/models/relay_list_event.dart) - Parses NIP-65 (kind 10002) events from M...
(QB_NEW_EN)
[grammar] ~222-~222: There might be a mistake here.
Context: ...kind 10002) events from Mostro instances - Validates relay URLs (WebSocket only) - ...
(QB_NEW_EN)
[grammar] ~223-~223: There might be a mistake here.
Context: ... - Validates relay URLs (WebSocket only) - Robust handling of different timestamp f...
(QB_NEW_EN)
[grammar] ~224-~224: There might be a mistake here.
Context: ... handling of different timestamp formats - Null-safe parsing for malformed events ...
(QB_NEW_EN)
[grammar] ~242-~242: There might be a mistake here.
Context: ... (lib/features/settings/settings.dart) - New blacklistedRelays: List<String> fi...
(QB_NEW_EN)
[grammar] ~243-~243: There might be a mistake here.
Context: ... blacklistedRelays: List<String> field - Backward-compatible serialization - Auto...
(QB_NEW_EN)
[grammar] ~244-~244: There might be a mistake here.
Context: ...ield - Backward-compatible serialization - Automatic migration for existing users ...
(QB_NEW_EN)
[grammar] ~247-~247: There might be a mistake here.
Context: ...b/features/relays/relays_notifier.dart) - **syncWithMostroInstance()**: Manual sync trigger - **removeRelayWi...
(QB_NEW_EN)
[grammar] ~248-~248: There might be a mistake here.
Context: ...MostroInstance()**: Manual sync trigger - **removeRelayWithBlacklist(String url)**: Smart removal with blacklisting - **a...
(QB_NEW_EN)
[grammar] ~249-~249: There might be a mistake here.
Context: ...url)**: Smart removal with blacklisting - **addRelayWithSmartValidation(...)`**: Auto-removes from blacklist when user ...
(QB_NEW_EN)
[grammar] ~250-~250: There might be a mistake here.
Context: ...s from blacklist when user manually adds - _handleMostroRelayListUpdate(): Filters blacklisted relays during sync...
(QB_NEW_EN)
[grammar] ~255-~255: There might be a mistake here.
Context: ...chronization Flow #### Real-time Sync 1. App Launch: Automatic subscription to ...
(QB_NEW_EN)
[grammar] ~256-~256: There might be a mistake here.
Context: ...kind 10002 events from configured Mostro 2. Event Reception: Parse relay list and ...
(QB_NEW_EN)
[grammar] ~257-~257: There might be a mistake here.
Context: ... relay list and filter against blacklist 3. State Update: Merge new relays while p...
(QB_NEW_EN)
[grammar] ~258-~258: There might be a mistake here.
Context: ... new relays while preserving user relays 4. NostrService: Automatic reconnection t...
(QB_NEW_EN)
[grammar] ~269-~269: There might be a mistake here.
Context: ...### Key Features #### User Experience - Transparent Operation: Sync happens au...
(QB_NEW_EN)
[grammar] ~270-~270: There might be a mistake here.
Context: ...Operation**: Sync happens automatically in background - Full User Control: Can...
(QB_NEW_EN)
[grammar] ~275-~275: There might be a mistake here.
Context: ...cross syncs #### Technical Robustness - Real-time Updates: WebSocket subscript...
(QB_NEW_EN)
[grammar] ~276-~276: There might be a mistake here.
Context: ...WebSocket subscriptions for instant sync - Error Resilience: Graceful fallbacks a...
(QB_NEW_EN)
[grammar] ~277-~277: There might be a mistake here.
Context: ...llbacks and comprehensive error handling - Race Protection: Prevents concurrent s...
(QB_NEW_EN)
[grammar] ~278-~278: There might be a mistake here.
Context: ...n**: Prevents concurrent sync operations - Logging: Detailed logging for debuggin...
(QB_NEW_EN)
[grammar] ~293-~293: There might be a mistake here.
Context: ...dResync(); ``` ### Implementation Notes - Subscription Management: Uses `Subscri...
(QB_NEW_EN)
[grammar] ~380-~380: There might be a mistake here.
Context: ...dation protocol and localization support - Settings screens: Card-based layout wi...
(QB_NEW_EN)
[grammar] ~487-~487: There might be a mistake here.
Context: ...ments --- Last Updated: 2025-08-22 Flutter Version: Latest stable **Dar...
(QB_NEW_EN)
[grammar] ~488-~488: There might be a mistake here.
Context: ...-22 Flutter Version: Latest stable Dart Version: Latest stable **Key De...
(QB_NEW_EN)
[grammar] ~489-~489: There might be a mistake here.
Context: ...stable Dart Version: Latest stable Key Dependencies: Riverpod, GoRouter, ...
(QB_NEW_EN)
[grammar] ~509-~509: There might be a mistake here.
Context: ... Live updates via Nostr protocol - 🔗 Smart Relay Management: Automatic sync ...
(QB_NEW_EN)
[grammar] ~511-~511: There might be a mistake here.
Context: ...ic sync with blacklist control ### Recent Achievements - UI Modernization: Co...
(QB_NEW_EN)
[grammar] ~511-~511: There might be a mistake here.
Context: ...acklist control ### Recent Achievements - UI Modernization: Complete settings an...
(QB_NEW_EN)
[grammar] ~512-~512: There might be a mistake here.
Context: ...ete settings and account screen redesign - Icon Enhancement: Improved app launche...
(QB_NEW_EN)
[grammar] ~513-~513: There might be a mistake here.
Context: ...app launcher and notification visibility - Localization Excellence: 73+ new trans...
(QB_NEW_EN)
[grammar] ~514-~514: There might be a mistake here.
Context: ... new translation keys across 3 languages - Code Quality: Zero analyzer issues wit...
(QB_NEW_EN)
[grammar] ~515-~515: There might be a mistake here.
Context: ...zer issues with modern Flutter standards - Documentation: Comprehensive NOSTR.md ...
(QB_NEW_EN)
[grammar] ~516-~516: There might be a mistake here.
Context: ...rehensive NOSTR.md and updated README.md - Relay System Architecture: URL normali...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
CLAUDE.md
262-262: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
lib/features/settings/settings_notifier.dart (1)
44-47: No change needed:privacyModeSettingcorrectly updatesfullPrivacyModeThe
Settings.copyWithmethod does include abool? privacyModeSettingparameter and assigns it to thefullPrivacyModefield (fullPrivacyMode: privacyModeSetting ?? fullPrivacyMode), soupdatePrivacyModealready works as intended—no renaming tofullPrivacyModeis required.• lib/features/settings/settings.dart (copyWith signature, lines 22–34)
bool? privacyModeSetting→ maps tofullPrivacyMode: privacyModeSetting ?? fullPrivacyModeLikely an incorrect or invalid review comment.
lib/features/relays/relays_notifier.dart (1)
59-66: Bootstrap default relay is OK (per design learning)Hardcoding
wss://relay.mostro.networkfor bootstrap aligns with your stated intent and current Config defaults. No change needed.
fix #267
Add relay sync system that auto-discovers relays from Mostro via kind 10002 events with user blacklist for permanent blocking. Users can remove relays from blacklist and manually re-add them anytime.
Summary by CodeRabbit
New Features
UI / Behavior
APIs
Tests
Documentation