Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 6 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR implements lifecycle safety checks in the dispute chat notifier and wires solver public key data through the account restore flow to ensure session keys are available for re-establishing dispute chat post-restore, addressing the failure to send/receive dispute messages after account restoration. Changes
Sequence DiagramsequenceDiagram
participant RestoreManager
participant RestoredDispute
participant Session
participant DisputeChatNotifier
RestoreManager->>RestoredDispute: Load restore data with solverPubkey
RestoreManager->>Session: Create Session(adminPubkey: solverPubkey)
Note over Session: Derive adminSharedKey from adminPubkey
RestoreManager->>DisputeChatNotifier: Initialize with restored dispute
DisputeChatNotifier->>DisputeChatNotifier: Check if mounted
alt adminSharedKey available
DisputeChatNotifier->>DisputeChatNotifier: Subscribe to kind 1059 events
else adminSharedKey not available
DisputeChatNotifier->>DisputeChatNotifier: Listen for session data
end
Note over DisputeChatNotifier: Guard all state mutations with mounted check
DisputeChatNotifier->>DisputeChatNotifier: Load historical messages
DisputeChatNotifier->>DisputeChatNotifier: Handle incoming events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| // 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; |
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.
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Blocking: 4 CodeRabbit comments sin resolver.
Major issues:
- dispute_chat_notifier.dart:133 - doble subscription potencial (listenToChats + restoreAndListen). Fix sugerido por CodeRabbit: agregar condicion !isListening antes de listenToChats.
- disputeChatId persistido en cada mensaje en vez de solo en init() - puede dejar el wrong chat ID si el ultimo mensaje tuvo delay.
Minor:
3. solverPubkey agregado a RestoredDispute pero no usado en ningun otro archivo del PR.
4. PR sin descripcion - no queda claro que bug resuelve ni por que solverPubkey es necesario.
Lo que esta bien: persistir disputeChatId es la solucion correcta, los 5 commits son limpios, normalizacion de initiator a pubkey es buena.
Veredicto: Request Changes - resolver CodeRabbit comments primero, especialmente el de doble subscription que es un bug real.
BraCR10
force-pushed
the
fix/chat-admin-restore
branch
2 times, most recently
from
4ee29db to
d2cea2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/restore/restore_manager.dart (1)
861-873:⚠️ Potential issue | 🟠 MajorSet
successbefore the no-orders early return.With the new
_operationCompleter, concurrent callers await the in-flight restore result. In the no-orders path, the primary call returnstrue, butfinallycompletes the shared completer withsuccess == false.🐛 Proposed fix
final lastTradeIndex = lastTradeIndexResponse.tradeIndex; await keyManager.setCurrentKeyIndex(lastTradeIndex + 1); progress.completeRestore(); + success = true; return true; }Also applies to: 934-935
🤖 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 861 - 873, The no-orders early-return path fails to set the shared `_operationCompleter` result because `success` remains false when the method returns; before returning true in the block that checks `if (ordersMap.isEmpty)` you must set `success = true` (so the `finally` that completes `_operationCompleter` reports success), and apply the same fix at the other early-return site around lines where progress.completeRestore() is called (the second occurrence referenced at 934-935); locate the code in `restore_manager.dart` involving `_operationCompleter`, `progress.completeRestore()`, `keyManager.setCurrentKeyIndex(...)`, `_sendLastTradeIndexRequest()`, `_waitForEvent(...)`, and `_extractLastTradeIndex(...)` and assign `success = true` immediately before each early `return true`.
🤖 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 147-150: Move the mounted guard so the notifier is checked before
starting the background task and add guards inside the load routine: call if
(!mounted) return immediately before launching unawaited(...) that does await
_loadHistoricalMessages(); await _subscribe(); and modify
_loadHistoricalMessages() to check mounted before each post-await state = ...
assignment (i.e., after any await that may allow disposal) to avoid writing to
state when disposed; keep references to _loadHistoricalMessages, _subscribe,
mounted, and state to locate and update the code.
In `@lib/features/disputes/providers/dispute_chat_provider.dart`:
- Around line 8-15: The fire-and-forget initialize call on the notifier
(unawaited(notifier.initialize())) needs an error boundary to avoid unhandled
zone errors; wrap the async call in a safe initializer that catches errors and
logs them (e.g., notifier.initialize().catchError((e, st) => /* log using
ref-provided logger or debugPrint/print */) or use a try/catch inside an async
helper) so failures are logged and the provider (disputeChatNotifierProvider /
DisputeChatNotifier.initialize) won't silently fail; use the existing ref to
obtain any app logger if available, otherwise fall back to debugPrint/print.
---
Outside diff comments:
In `@lib/features/restore/restore_manager.dart`:
- Around line 861-873: The no-orders early-return path fails to set the shared
`_operationCompleter` result because `success` remains false when the method
returns; before returning true in the block that checks `if (ordersMap.isEmpty)`
you must set `success = true` (so the `finally` that completes
`_operationCompleter` reports success), and apply the same fix at the other
early-return site around lines where progress.completeRestore() is called (the
second occurrence referenced at 934-935); locate the code in
`restore_manager.dart` involving `_operationCompleter`,
`progress.completeRestore()`, `keyManager.setCurrentKeyIndex(...)`,
`_sendLastTradeIndexRequest()`, `_waitForEvent(...)`, and
`_extractLastTradeIndex(...)` and assign `success = true` immediately before
each early `return true`.
🪄 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: 005ab650-c1c8-497b-8029-7d908b16328f
📒 Files selected for processing (10)
lib/data/models/restore_response.dartlib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/disputes/providers/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.dartlib/shared/providers/session_notifier_provider.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/services/dispute_read_status_service.dart
- lib/features/disputes/widgets/dispute_message_input.dart
- lib/features/disputes/widgets/dispute_message_bubble.dart
- lib/features/disputes/widgets/dispute_messages_list.dart
- lib/data/models/restore_response.dart
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/restore/restore_manager.dart (1)
88-112:⚠️ Potential issue | 🟠 MajorInvalidate dispute chat providers before clearing storage.
Lines 96-99 clear sessions and event storage while the old dispute chat notifiers are still alive. Any stale subscription or in-flight
_onChatEventcan write old dispute-chat events back intoeventStorageProviderafterdeleteAll(), which defeats the restore cleanup. Move the dispute provider invalidation immediately after collectingdisputeIds, before destructive cleanup.🛠️ Proposed fix
final disputeIds = currentSessions .where((s) => s.disputeId != null) .map((s) => s.disputeId!) .toList(); + // Invalidate dispute chat providers before clearing storage so stale + // subscriptions cannot repopulate eventStorage during cleanup. + for (final disputeId in disputeIds) { + ref.invalidate(disputeChatNotifierProvider(disputeId)); + } + // Clear storage await ref.read(sessionNotifierProvider.notifier).reset(); await ref.read(mostroStorageProvider).deleteAll(); await ref.read(eventStorageProvider).deleteAll(); await ref.read(notificationsRepositoryProvider).clearAll(); @@ - // Invalidate dispute chat providers to cancel stale subscriptions before restore - for (final disputeId in disputeIds) { - ref.invalidate(disputeChatNotifierProvider(disputeId)); - } - logger.i( 'Restore: cleared ${chatOrderIds.length} chat providers, ' '${disputeIds.length} dispute chat providers', );🤖 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 88 - 112, You collected disputeIds from sessionNotifierProvider but invalidate dispute chat providers (disputeChatNotifierProvider) only after clearing storage; move the loop that invalidates disputeChatNotifierProvider(disputeId) so it runs immediately after building disputeIds and before calling sessionNotifierProvider.notifier.reset(), mostro/eventStorageProvider.deleteAll(), notificationsRepositoryProvider.clearAll(), and orderRepositoryProvider.clearCache() so stale dispute-chat subscriptions cannot write back into storage during cleanup; keep the existing collection of disputeIds and only reorder the invalidation block.
🤖 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-131: The listener is being attached after
_loadHistoricalMessages(), causing missed broadcast events; update _subscribe()
in DisputeChatNotifier to call
subscriptionManager.disputeChat.listen(_onChatEvent) before awaiting
_loadHistoricalMessages() and immediately pause the returned StreamSubscription
(_subscription.pause()) until historic loading finishes, then resume it (or
alternatively attach and let _onChatEvent buffer until a flag set by
_loadHistoricalMessages() indicates history is complete); ensure you reference
subscriptionManager.disputeChat, _onChatEvent, _subscription,
_loadHistoricalMessages, and _subscribe when making the change so the listener
is attached early and event processing is gated until history is loaded.
---
Outside diff comments:
In `@lib/features/restore/restore_manager.dart`:
- Around line 88-112: You collected disputeIds from sessionNotifierProvider but
invalidate dispute chat providers (disputeChatNotifierProvider) only after
clearing storage; move the loop that invalidates
disputeChatNotifierProvider(disputeId) so it runs immediately after building
disputeIds and before calling sessionNotifierProvider.notifier.reset(),
mostro/eventStorageProvider.deleteAll(),
notificationsRepositoryProvider.clearAll(), and
orderRepositoryProvider.clearCache() so stale dispute-chat subscriptions cannot
write back into storage during cleanup; keep the existing collection of
disputeIds and only reorder the invalidation block.
🪄 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: bee70c0c-436b-4ce6-b4f3-23220b187a4e
📒 Files selected for processing (10)
lib/data/models/restore_response.dartlib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/disputes/providers/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.dartlib/shared/providers/session_notifier_provider.dart
✅ Files skipped from review due to trivial changes (1)
- lib/features/disputes/widgets/dispute_message_bubble.dart
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/services/dispute_read_status_service.dart
- lib/shared/providers/session_notifier_provider.dart
- lib/features/disputes/widgets/dispute_content.dart
- lib/features/disputes/widgets/dispute_messages_list.dart
- lib/data/models/restore_response.dart
- lib/features/disputes/providers/dispute_chat_provider.dart
- lib/features/disputes/widgets/dispute_message_input.dart
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
When a user has an in-progress dispute and refreshes or imports the user, if they create a new order immediately afterward, the order is created correctly. However, all the disputes they had, whether ongoing or in any other status, are shown as “initiated.” I think the status is not being properly persisted.
I tested this on two phones and the behavior was consistent. Additionally, on one of them, after refreshing a user who had an ongoing dispute, I closed the application and, upon reopening it, all disputes appeared as “initiated,” or some as “initiated” and others still in progress.
before:
after:
|
Hi @Catrya, thanks for testing. Could you tell me if the order, which is linked to the dispute, also gets a wrong status when it happens? For instance: |
Yes, the statuses are also displayed incorrectly even though Mostro sends the correct statuses. Before restore: After restore: |
- Remove `_determineIfUserInitiatedDispute` and its manual pubkey/index verification logic - Use the `initiator` role directly from `RestoredDispute` to determine if the user started the dispute - Streamline the restoration flow by relying on the explicit role provided in the restored dispute data
- Update `RestoredDispute` model to include the `solverPubkey` field from the restore response. - In `RestoreService`, look up dispute metadata for restored orders to populate the `adminPubkey` and `disputeId` in the session. - Ensures dispute chat functionality and admin identification are preserved after account restoration.
- Collect existing dispute IDs before clearing storage during the restoration process. - Invalidate `disputeChatNotifierProvider` for each ID to cancel stale subscriptions and ensure a clean state before restoration. - Update cleanup logs to include the count of dispute chat providers cleared.
- Use `addPostFrameCallback` when updating state during the persistent callbacks phase to avoid "setState during build" errors. - Add `mounted` guards and prevent redundant transitions to already completed or error states. - Update `RestoreService` to set a success flag instead of manually triggering progress completion to prevent timing issues with the UI overlay.
- Add `DISPUTE_CHAT_RESTORE.md` detailing the restoration protocol flow, subscription management, and identified edge cases. - Add `mounted` guards to `DisputeChatNotifier` to prevent state updates after disposal during asynchronous initialization, history loading, and event processing.
BraCR10
force-pushed
the
fix/chat-admin-restore
branch
from
3aca3e8 to
307f036
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/restore/restore_manager.dart (1)
602-618:⚠️ Potential issue | 🟠 MajorDefensive validation needed: invalid
solverPubkeylength will abort the entire restore.
Session.setAdminPeerthrowsArgumentErrorifadminPubkey.length != 64. The current check at lines 603–605 only validates non-emptiness; a malformed or truncatedsolverPubkeyfrom the server will propagate throughSession(...)and unwind the entire session restoration for-loop—all remaining orders will be skipped, and the outertryblock rethrows, failing the whole restore process.The asymmetry is notable: the second for-loop (line 651+, order processing) wraps each entry in
try/catch, but the first for-loop (session restoration) has no per-entry error handling. Since sessions are prerequisites for processing, the first failure is fatal.Given this runs over server-provided data during a critical recovery flow, a single malformed dispute record will block user recovery entirely. Validate
solverPubkey.length == 64before passing it toSession, or wrap the per-order session creation intry/catchto skip invalid records and continue restoring the rest.🤖 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 602 - 618, Session creation can throw if restoredDispute.solverPubkey is malformed; before passing adminPubkey into the Session(...) constructor (or calling Session.setAdminPeer), validate that restoredDispute?.solverPubkey is non-empty and exactly 64 chars and if not, either set adminPubkey to null or skip this session with a safe log; alternatively wrap the per-order session creation block around a try/catch that catches ArgumentError (or any) from Session(...) / setAdminPeer so a single bad solverPubkey does not abort the entire restore loop (refer to restoredDispute, solverPubkey, Session, and Session.setAdminPeer to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/DISPUTE_CHAT_RESTORE.md`:
- Around line 154-157: The fenced code block containing the stack trace
(`FormatException: Failed to parse Peer from JSON: FormatException: Public key
cannot be empty ... MostroService._onData (mostro_service.dart:172)`) is missing
a language specifier which triggers MD040; update that fenced block to include a
language tag (e.g., add ```text before the stack-trace and keep the closing
```), ensuring the snippet is a proper ```text fenced code block so markdownlint
no longer flags it.
- Line 14: The fenced code block containing the protocol-flow diagram (the ASCII
diagram that begins "User enters mnemonic") is missing a language specifier;
update the opening fence from ``` to ```text so the block is ```text ... ``` to
satisfy markdownlint MD040 and clearly mark the diagram as plaintext.
In `@lib/features/restore/restore_manager.dart`:
- Around line 113-116: The logger.i call in restore_manager.dart that logs
'Restore: cleared ${chatOrderIds.length} chat providers, ${disputeIds.length}
dispute chat providers' has trailing whitespace after the closing ");" which
will be flagged by dart format; remove the trailing spaces so the line ends
immediately after the closing ); (and then run dart format or your formatter) to
ensure no analyzer warnings remain; reference: logger.i, chatOrderIds,
disputeIds in restore_manager.dart.
---
Outside diff comments:
In `@lib/features/restore/restore_manager.dart`:
- Around line 602-618: Session creation can throw if
restoredDispute.solverPubkey is malformed; before passing adminPubkey into the
Session(...) constructor (or calling Session.setAdminPeer), validate that
restoredDispute?.solverPubkey is non-empty and exactly 64 chars and if not,
either set adminPubkey to null or skip this session with a safe log;
alternatively wrap the per-order session creation block around a try/catch that
catches ArgumentError (or any) from Session(...) / setAdminPeer so a single bad
solverPubkey does not abort the entire restore loop (refer to restoredDispute,
solverPubkey, Session, and Session.setAdminPeer to locate the code).
🪄 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: 182ed2c6-387e-4cb1-9932-88f7416b5bdf
📒 Files selected for processing (5)
docs/architecture/DISPUTE_CHAT_RESTORE.mdlib/data/models/restore_response.dartlib/features/disputes/notifiers/dispute_chat_notifier.dartlib/features/restore/restore_manager.dartlib/features/restore/restore_progress_notifier.dart
✅ Files skipped from review due to trivial changes (2)
- lib/data/models/restore_response.dart
- lib/features/disputes/notifiers/dispute_chat_notifier.dart
|
Hi @Catrya, I changed the approach — please test it and let me know. A few things I want to point out. Details in 1. Isolated subscription and SubscriptionManagerIn the last approach I thought the right move was to migrate dispute chat into As you detected, it broke order creation. I don't know if the isolation is intentional, or maybe it could be improved by deleting the isolated subscription and using 2. Pre-existing error on restoreThis error shows up on 3. State lost after restore and app kill — HIGH PRIORITYAfter a restore, if the user kills the app and reopens it, order and dispute states are This also happens on |
Reproduced it on main without your branch. By Claude:
|
2 participants
Once a user restores orders that contain disputes and those disputes have solvers, the dispute chat must be re-established
Closes #508
Changes
solverPubkeytoRestoredDisputemodel to capture admin's public key from restore responseDisputeChatNotifiermigrated toSubscriptionManagerfor event handling (aligns with P2P chat pattern)sessionForDisputeProvidernow synchronous lookup bydisputeId— removes async timing failures during restoreinitiatorfield from server response instead of client-side trade-index calculation (lost in feat: implement NIP-13 proof-of-work mining for Mostro events #519, originally added in Check initiator to restored dispute data #497)dispute_chat_provider.dartto fix circular dependency on provider setupSummary by CodeRabbit
Bug Fixes
Documentation