Open
Conversation
When restoring an account, ensure that the dispute solver's pubkey is recovered and associated with the session. This allows the user to continue dispute chats with the assigned admin after the restoration process.
Refactor DisputeChatNotifier to use a centralized SubscriptionManager for event handling and ensure the notifier is active during the account restore process. This prevents missing historical messages when restoring disputes with assigned admins. - Use SubscriptionManager for dispute chat subscriptions. - Pre-initialize DisputeChatNotifier in RestoreService when admin keys are present. - Load historical messages before subscribing to the live event stream.
- Centralize dispute chat providers and safe initialization handling into a dedicated provider file. - Simplify dispute initiator detection during account restoration by using the server-provided initiator field instead of manual trade-index comparisons. - Improve dispute chat subscription management and increase the restoration delay to ensure all historical messages are captured. - Update dart_nostr and websocket dependencies.
Changes: - Assign disputeId to sessions during restore to enable direct lookup - Change sessionForDisputeProvider to use synchronous search by disputeId instead of async disputeDetailsProvider - Remove dependency on FutureProvider that caused race condition during initialization This fixes the issue where dispute chat messages would not appear after killing and reopening the app, as the async provider dependency prevented finding the session during initial load.
Contributor
WalkthroughA Changes
Sequence DiagramsequenceDiagram
participant RM as RestoreManager
participant SP as sessionForDisputeProvider
participant SN as sessionNotifierProvider
participant DCN as DisputeChatNotifier
participant SM as SubscriptionManager
RM->>RM: restore(restoredDisputes)
RM->>SP: Derive session for disputeId
SP->>SN: Scan sessionNotifierProvider
SN-->>SP: Match session by disputeId
SP-->>RM: Return Session?
alt Session with adminSharedKey available
RM->>DCN: initialize()
DCN->>SP: ref.read(sessionForDisputeProvider)
SP-->>DCN: Session?
DCN->>DCN: _loadHistoricalMessages
DCN->>SM: disputeChat.listen(_onChatEvent)
SM-->>DCN: Chat event stream
DCN->>DCN: _onChatEvent (validate kind 1059, p tag)
else solverPubkey present
RM->>DCN: subscribe()
DCN->>SM: Attach listener
end
RM-->>RM: Restore complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
lib/features/disputes/notifiers/dispute_chat_provider.dart (1)
1-35: Move this provider module underfeatures/disputes/providers/.This file introduces Riverpod providers from the
notifiers/tree, which makes provider wiring harder to discover and diverges from the repository layout.As per coding guidelines, "Use Riverpod for all state management and organize providers by feature in
features/{feature}/providers/".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/disputes/notifiers/dispute_chat_provider.dart` around lines 1 - 35, Move this provider module out of features/disputes/notifiers and into features/disputes/providers to follow the repository layout; update any imports that reference this file accordingly. Ensure the exported symbols (sessionForDisputeProvider, disputeChatNotifierProvider, disputeChatInitializedProvider) and the internal initializer usage (_initializeDisputeChatSafely and DisputeChatNotifier) still resolve after the move by adjusting import paths for mostro_mobile/features/disputes/notifiers/dispute_chat_notifier.dart (where DisputeChatNotifier and the initializer live) and any files that currently import this provider file. Keep the re-export of dispute_chat_notifier.dart consistent (exporting the notifier types) and run a project-wide search to update all import locations to the new features/disputes/providers/<filename>.dart path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart`:
- Around line 129-133: The notifier currently creates a second independent
listener by calling subscriptionManager.disputeChat.listen(_onChatEvent)
directly, which can lead to duplicate event processing; replace those direct
listens with a call to the notifier's existing _subscribe() method so the
notifier maintains a single subscription (update the block using
subscriptionManager.disputeChat and the similar block at 145-157 to call
_subscribe() instead of assigning _subscription =
subscriptionManager.disputeChat.listen(...)), ensuring _subscription and
_onChatEvent remain the single source of truth for subscription lifecycle.
In `@lib/features/disputes/notifiers/dispute_chat_provider.dart`:
- Around line 47-50: The mounted check currently re-reads
disputeChatNotifierProvider(disputeId).notifier which can create a new notifier;
instead use the existing notifier parameter directly (notifier.mounted) when
gating initialization and state updates; update both checks that read
ref.container.read(disputeChatNotifierProvider(disputeId).notifier).mounted to
use notifier.mounted and then set
ref.read(disputeChatInitializedProvider(disputeId).notifier).state = true only
when notifier.mounted is true.
- Around line 22-35: The family providers for dispute chats are not being torn
down and leave stale notifiers with active subscriptions across restore; update
disputeChatNotifierProvider and disputeChatInitializedProvider to be
auto-disposed (add .autoDispose() to their declarations) so Riverpod will clean
them up automatically, or alternatively implement tracking of active dispute IDs
and call ref.invalidate for each tracked dispute notifier
(ref.invalidate(disputeChatNotifierProvider(id)) and
ref.invalidate(disputeChatInitializedProvider(id))) inside _clearAll() before
restore() runs to ensure all notifiers are unsubscribed and disposed; locate the
symbols disputeChatNotifierProvider, disputeChatInitializedProvider,
_clearAll(), restore(), and the call to .subscribe() to apply the chosen fix.
In `@lib/features/restore/restore_manager.dart`:
- Around line 624-625: Remove the explicit subscribe() call: when adminPubkey
and restoredDispute are non-null simply read the provider to trigger
_initializeDisputeChatSafely(); do not call
ref.read(disputeChatNotifierProvider(restoredDispute.disputeId).notifier).subscribe()
because _initializeDisputeChatSafely() (invoked by reading the provider) already
awaits notifier.initialize(), and initialize() calls _subscribe() internally—so
delete the subscribe() invocation to avoid the race condition between
subscribe() and initialize() for disputeChatNotifierProvider,
_initializeDisputeChatSafely, initialize, and _subscribe.
---
Nitpick comments:
In `@lib/features/disputes/notifiers/dispute_chat_provider.dart`:
- Around line 1-35: Move this provider module out of features/disputes/notifiers
and into features/disputes/providers to follow the repository layout; update any
imports that reference this file accordingly. Ensure the exported symbols
(sessionForDisputeProvider, disputeChatNotifierProvider,
disputeChatInitializedProvider) and the internal initializer usage
(_initializeDisputeChatSafely and DisputeChatNotifier) still resolve after the
move by adjusting import paths for
mostro_mobile/features/disputes/notifiers/dispute_chat_notifier.dart (where
DisputeChatNotifier and the initializer live) and any files that currently
import this provider file. Keep the re-export of dispute_chat_notifier.dart
consistent (exporting the notifier types) and run a project-wide search to
update all import locations to the new
features/disputes/providers/<filename>.dart path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c451ba63-46cc-416c-8b24-6c922fccdd87
📒 Files selected for processing (9)
lib/data/models/restore_response.dartlib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/disputes/notifiers/dispute_chat_provider.dartlib/features/disputes/widgets/dispute_content.dartlib/features/disputes/widgets/dispute_message_bubble.dartlib/features/disputes/widgets/dispute_message_input.dartlib/features/disputes/widgets/dispute_messages_list.dartlib/features/restore/restore_manager.dartlib/services/dispute_read_status_service.dart
Comment on lines
+129
to
+133
| // Use SubscriptionManager to subscribe to dispute chat events | ||
| // This is managed centrally and handles all sessions with adminSharedKey | ||
| final subscriptionManager = ref.read(subscriptionManagerProvider); | ||
| _subscription = subscriptionManager.disputeChat.listen(_onChatEvent); | ||
| logger.i('Subscribed to dispute chat via SubscriptionManager for dispute: $disputeId'); |
Contributor
There was a problem hiding this comment.
Re-use _subscribe() here to keep the notifier single-subscribed.
There are now two independent disputeChat.listen(_onChatEvent) paths. Since SubscriptionManager.disputeChat is broadcast (lib/features/subscriptions/subscription_manager.dart:26-31), a restore-triggered subscribe() can overlap with this callback and the same live event will be processed twice by this notifier.
♻️ Proposed fix
unawaited(() async {
await _loadHistoricalMessages();
if (!mounted) return;
- final subscriptionManager = ref.read(subscriptionManagerProvider);
- _subscription = subscriptionManager.disputeChat.listen(_onChatEvent);
+ await _subscribe();
}());Also applies to: 145-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/disputes/notifiers/dispute_chat_notifier.dart` around lines 129
- 133, The notifier currently creates a second independent listener by calling
subscriptionManager.disputeChat.listen(_onChatEvent) directly, which can lead to
duplicate event processing; replace those direct listens with a call to the
notifier's existing _subscribe() method so the notifier maintains a single
subscription (update the block using subscriptionManager.disputeChat and the
similar block at 145-157 to call _subscribe() instead of assigning _subscription
= subscriptionManager.disputeChat.listen(...)), ensuring _subscription and
_onChatEvent remain the single source of truth for subscription lifecycle.
Comment on lines
+22
to
+35
| final disputeChatNotifierProvider = | ||
| StateNotifierProvider.family<DisputeChatNotifier, DisputeChatState, String>( | ||
| (ref, disputeId) { | ||
| final notifier = DisputeChatNotifier(disputeId, ref); | ||
|
|
||
| // Initialize the notifier with proper error handling and safety checks | ||
| _initializeDisputeChatSafely(ref, notifier, disputeId); | ||
|
|
||
| return notifier; | ||
| }, | ||
| ); | ||
|
|
||
| // Provider to track initialization status of dispute chats | ||
| final disputeChatInitializedProvider = StateProvider.family<bool, String>((ref, disputeId) => false); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expectation: either these providers are auto-disposed, or restore cleanup explicitly invalidates them.
printf '\n--- dispute chat provider declarations ---\n'
sed -n '15,38p' lib/features/disputes/notifiers/dispute_chat_provider.dart
printf '\n--- restore cleanup ---\n'
sed -n '78,105p' lib/features/restore/restore_manager.dartRepository: MostroP2P/mobile
Length of output: 2179
🏁 Script executed:
#!/bin/bash
# Check the full provider declarations for autoDispose usage
printf '\n--- Full dispute chat provider file ---\n'
cat -n lib/features/disputes/notifiers/dispute_chat_provider.dart | head -50
printf '\n--- Search for disputeChatNotifier invalidations ---\n'
rg -i 'disputeChat.*invalidate|invalidate.*disputeChat' -A 2 -B 2
printf '\n--- Search for autoDispose in dispute providers ---\n'
rg 'autoDispose' lib/features/disputes/notifiers/dispute_chat_provider.dart
printf '\n--- Check if there is any active dispute tracking ---\n'
rg -i 'activeDispute|dispute.*id.*list|track.*dispute' lib/features/disputes/ -A 2 | head -30Repository: MostroP2P/mobile
Length of output: 4963
🏁 Script executed:
#!/bin/bash
# Check restore_manager.dart for dispute-related restoration and cleanup
printf '\n--- Full restore_manager.dart cleanup section ---\n'
cat -n lib/features/restore/restore_manager.dart | grep -A 50 '_clearAll'
printf '\n--- Search for dispute restoration in restore_manager ---\n'
rg -i 'dispute|chat.*restore' lib/features/restore/restore_manager.dart -B 2 -A 2
printf '\n--- Check DisputeChatNotifier for subscription cleanup ---\n'
grep -n 'dispose\|subscription\|stream' lib/features/disputes/notifiers/dispute_chat_notifier.dartRepository: MostroP2P/mobile
Length of output: 11978
Add an explicit teardown strategy for dispute chat family providers.
These family providers lack autoDispose() and are not cleaned up during restore. When restore() is called, it subscribes to dispute chat notifiers via ref.read(disputeChatNotifierProvider(restoredDispute.disputeId).notifier).subscribe() for each restored dispute, but _clearAll() only invalidates chat room providers—not dispute providers. This leaves stale dispute notifiers with active subscriptions in memory across restores, creating a resource leak.
Either add .autoDispose() to both disputeChatNotifierProvider and disputeChatInitializedProvider, or track active dispute IDs and explicitly invalidate them in _clearAll() before restore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/disputes/notifiers/dispute_chat_provider.dart` around lines 22 -
35, The family providers for dispute chats are not being torn down and leave
stale notifiers with active subscriptions across restore; update
disputeChatNotifierProvider and disputeChatInitializedProvider to be
auto-disposed (add .autoDispose() to their declarations) so Riverpod will clean
them up automatically, or alternatively implement tracking of active dispute IDs
and call ref.invalidate for each tracked dispute notifier
(ref.invalidate(disputeChatNotifierProvider(id)) and
ref.invalidate(disputeChatInitializedProvider(id))) inside _clearAll() before
restore() runs to ensure all notifiers are unsubscribed and disposed; locate the
symbols disputeChatNotifierProvider, disputeChatInitializedProvider,
_clearAll(), restore(), and the call to .subscribe() to apply the chosen fix.
Comment on lines
+47
to
+50
| // Check if the provider is still mounted before updating state | ||
| // This prevents state updates on disposed objects | ||
| if (ref.container.read(disputeChatNotifierProvider(disputeId).notifier).mounted) { | ||
| ref.read(disputeChatInitializedProvider(disputeId).notifier).state = true; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expectation: mounted checks should use the notifier already in hand, not recreate/read the provider again.
rg -n -C3 'container\.read\(disputeChatNotifierProvider\(disputeId\)\.notifier\)\.mounted|_initializeDisputeChatSafely' lib/features/disputes/notifiers/dispute_chat_provider.dartRepository: MostroP2P/mobile
Length of output: 1336
Use the notifier parameter directly instead of re-reading the provider for mounted checks.
The code re-reads disputeChatNotifierProvider(disputeId).notifier to check if it's mounted, which can instantiate a fresh notifier after disposal. Since the notifier is already available as a function parameter, use notifier.mounted directly. Re-reading the provider may succeed on a new notifier and incorrectly rerun initialization logic.
Apply at lines 49 and 64:
- Replace
ref.container.read(disputeChatNotifierProvider(disputeId).notifier).mountedwithnotifier.mounted
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/disputes/notifiers/dispute_chat_provider.dart` around lines 47 -
50, The mounted check currently re-reads
disputeChatNotifierProvider(disputeId).notifier which can create a new notifier;
instead use the existing notifier parameter directly (notifier.mounted) when
gating initialization and state updates; update both checks that read
ref.container.read(disputeChatNotifierProvider(disputeId).notifier).mounted to
use notifier.mounted and then set
ref.read(disputeChatInitializedProvider(disputeId).notifier).state = true only
when notifier.mounted is true.
Comment on lines
+624
to
+625
| if (adminPubkey != null && restoredDispute != null) { | ||
| ref.read(disputeChatNotifierProvider(restoredDispute.disputeId).notifier).subscribe(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expectation: if `initialize()` already subscribes (directly or via SubscriptionManager),
# restore should only instantiate/read the provider here, not call `subscribe()` again.
printf '\n--- provider bootstrap ---\n'
sed -n '21,34p' lib/features/disputes/notifiers/dispute_chat_provider.dart
printf '\n--- initialize/subscribe definitions ---\n'
rg -n -C3 '\bFuture<.*initialize\b|\bvoid\s+subscribe\b|\bsubscribe\s*\(' lib/features/disputes/notifiers/dispute_chat_notifier.dartRepository: MostroP2P/mobile
Length of output: 1163
🏁 Script executed:
#!/bin/bash
# Find the _initializeDisputeChatSafely function implementation
printf '\n--- _initializeDisputeChatSafely implementation ---\n'
rg -n -A 15 '_initializeDisputeChatSafely' lib/features/disputes/notifiers/dispute_chat_provider.dart
printf '\n--- Full DisputeChatNotifier initialize and _subscribe methods ---\n'
rg -n -A 10 'Future<void> initialize|void _subscribe|Future<void> subscribe' lib/features/disputes/notifiers/dispute_chat_notifier.dartRepository: MostroP2P/mobile
Length of output: 1986
🏁 Script executed:
#!/bin/bash
# Check _subscribe implementation for duplicate call safeguards
printf '\n--- _subscribe implementation (full) ---\n'
rg -n -A 30 'Future<void> _subscribe' lib/features/disputes/notifiers/dispute_chat_notifier.dart | head -50
# Check if there are any subscription state guards
printf '\n--- Check for subscription state tracking ---\n'
rg -n '_subscription|_isSubscribed|_subscriptionId' lib/features/disputes/notifiers/dispute_chat_notifier.dartRepository: MostroP2P/mobile
Length of output: 1840
Remove the explicit subscribe() call since _initializeDisputeChatSafely() already subscribes via initialize().
When you read the provider, _initializeDisputeChatSafely() is fired (though not awaited in the factory), which calls await notifier.initialize(). The initialize() method already calls _subscribe() internally, so the explicit subscribe() call creates a race condition. If subscribe() executes before initialization completes, it may subscribe before historical messages are loaded. If it executes after, it unnecessarily cancels and recreates the subscription. Simply reading the provider is sufficient to trigger initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/restore/restore_manager.dart` around lines 624 - 625, Remove the
explicit subscribe() call: when adminPubkey and restoredDispute are non-null
simply read the provider to trigger _initializeDisputeChatSafely(); do not call
ref.read(disputeChatNotifierProvider(restoredDispute.disputeId).notifier).subscribe()
because _initializeDisputeChatSafely() (invoked by reading the provider) already
awaits notifier.initialize(), and initialize() calls _subscribe() internally—so
delete the subscribe() invocation to avoid the race condition between
subscribe() and initialize() for disputeChatNotifierProvider,
_initializeDisputeChatSafely, initialize, and _subscribe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Once a user restores orders that contain disputes and those disputes have solvers, the dispute chat must be re-established
Changes
Summary by CodeRabbit
New Features
Improvements