Dispute Chat implementation #329
Conversation
WalkthroughImplements Mostro Gift Wrap wrap/unwrap flows (1059→13→1) with JSON sanitization and layered errors; changes mostroWrap signature to accept sender keys + receiver pubkey; replaces dispute-chat stubs with a Riverpod-backed DisputeChatNotifier/state, read-status provider/service, UI widget migrations, and MostroService message filtering/storage key fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Sender as User A
participant AppA as App (A)
participant Relays as Nostr Relays
participant AppB as App (B)
actor Receiver as User B
rect rgb(240,248,255)
note right of AppA: Wrap (send)
Sender->>AppA: Compose message
AppA->>AppA: Build RUMOR (kind 1) payload (signed)
AppA->>AppA: Encrypt RUMOR -> SEAL (kind 13) using sender priv + receiver pub
AppA->>AppA: Encrypt SEAL -> Gift Wrap (kind 1059) with ephemeral key + receiver pub
AppA->>Relays: Publish Gift Wrap (1059)
end
rect rgb(245,255,240)
note right of AppB: Unwrap (receive)
Relays-->>AppB: Deliver Gift Wrap (1059)
AppB->>AppB: Decrypt Gift Wrap -> SEAL (13)
AppB->>AppB: Decrypt SEAL -> RUMOR (1) using sender pub
AppB->>AppB: Sanitize & validate RUMOR (ensure unsigned fields normalized)
AppB-->>Receiver: Render message
end
sequenceDiagram
autonumber
participant UI as Dispute UI
participant Notifier as DisputeChatNotifier
participant Store as Local Storage
participant Relays as Nostr Relays
rect rgb(255,250,240)
note over Notifier: Initialization
UI->>Notifier: initialize()
Notifier->>Store: load historical messages
Notifier->>Relays: subscribe kind 1059 for session/dispute
end
rect rgb(250,240,255)
note over Notifier: Receiving events
Relays-->>Notifier: Gift Wrap 1059
Notifier->>Notifier: unwrap (1059→13→1) & parse payload
alt valid dispute DM
Notifier->>Store: save message
Notifier-->>UI: update state (deduped, sorted)
else skip/invalid
Notifier-->>Notifier: ignore/log
end
end
rect rgb(240,255,255)
note over Notifier: Sending
UI->>Notifier: sendMessage(text)
Notifier->>Notifier: wrap (RUMOR→SEAL→Gift Wrap)
Notifier->>Relays: publish 1059
Notifier->>Store: save optimistic message
Notifier-->>UI: update state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/features/order/models/order_state.dart (1)
115-131: Existing peer may be lost when Order payload lacks trade pubkeysIn the Order branch, if neither buyerTradePubkey nor sellerTradePubkey exists, newPeer stays null and overrides the existing peer in the final state. Preserve the current peer in that case and avoid misleading logs.
Apply this diff:
- Peer? newPeer; + Peer? newPeer; if (message.payload is Peer && message.getPayload<Peer>()!.publicKey.isNotEmpty) { newPeer = message.getPayload<Peer>(); _logger.i('👤 New Peer found in message'); } else if (message.payload is Order) { if (message.getPayload<Order>()!.buyerTradePubkey != null) { newPeer = Peer(publicKey: message.getPayload<Order>()!.buyerTradePubkey!); } else if (message.getPayload<Order>()!.sellerTradePubkey != null) { newPeer = Peer(publicKey: message.getPayload<Order>()!.sellerTradePubkey!); } - _logger.i('👤 New Peer found in message'); + if (newPeer != null) { + _logger.i('👤 New Peer found in message'); + } else { + newPeer = peer; // Preserve existing if no trade pubkey present + _logger.i('👤 Peer unchanged (no trade pubkey in Order)'); + } } else { newPeer = peer; // Preserve existing }lib/features/chat/notifiers/chat_room_notifier.dart (2)
153-156: Dedup by nullable id collapses chat history; add robust key and null-safe sortUnwrapped/sent inner events may have null id (RUMOR). Current dedup keeps only the last null-id message and sort uses createdAt! risking NPEs.
Use a stable fallback key and null-safe sort.
Apply these replacements at the noted spots:
- final deduped = {for (var m in allMessages) m.id: m}.values.toList(); - deduped.sort((a, b) => b.createdAt!.compareTo(a.createdAt!)); + String _msgKey(NostrEvent m) => + m.id ?? + '${m.createdAt?.millisecondsSinceEpoch ?? 0}:${m.pubkey}:${m.content.hashCode}'; + final deduped = {for (var m in allMessages) _msgKey(m): m}.values.toList(); + deduped.sort((a, b) => + (b.createdAt ?? DateTime.fromMillisecondsSinceEpoch(0)) + .compareTo(a.createdAt ?? DateTime.fromMillisecondsSinceEpoch(0)));Do this in both runtime receive and historical load paths.
(Line ranges: 153-156, 197-201, 336-340)Also applies to: 197-201, 336-340
213-216: Send-failure removal may delete unrelated messages with null idsFiltering by id removes all messages whose id is null. Remove by a stable key aligned with your dedup key.
- final updatedMessages = - state.messages.where((msg) => msg.id != innerEvent.id).toList(); + String _msgKey(NostrEvent m) => + m.id ?? + '${m.createdAt?.millisecondsSinceEpoch ?? 0}:${m.pubkey}:${m.content.hashCode}'; + final targetKey = _msgKey(innerEvent); + final updatedMessages = + state.messages.where((m) => _msgKey(m) != targetKey).toList();
🧹 Nitpick comments (11)
lib/data/models/dispute.dart (1)
380-409: Normalize status and use a Set to avoid duplicates and edge casesterminalStatusList contains both hyphen and underscore variants. Normalize once and compare against a Set to reduce duplication and avoid mismatches.
Apply this diff:
- // Terminal dispute states where admin should not be used as counterparty - // Terminal statuses where the dispute is finished and admin should not be shown as counterparty - final terminalStatusList = [ - 'resolved', - 'closed', - 'seller-refunded', - 'seller_refunded', - 'admin-canceled', - 'admin_canceled', - 'admin-settled', - 'admin_settled', - 'solved', - 'completed', - ]; + // Terminal dispute states where admin should not be shown as counterparty + const terminalStatusSet = <String>{ + 'resolved', + 'closed', + 'seller-refunded', + 'admin-canceled', + 'admin-settled', + 'solved', + 'completed', + }; @@ - // 3. Dispute is not in a terminal state (normalize status to lowercase for comparison) - final normalizedStatus = dispute.status?.toLowerCase().trim() ?? ''; + // 3. Dispute is not in a terminal state (normalize to lowercase, unify underscores to hyphens) + final normalizedStatus = + (dispute.status ?? '').toLowerCase().replaceAll('_', '-').trim(); if (orderState?.peer == null && dispute.adminPubkey != null && - !terminalStatusList.contains(normalizedStatus)) { + !terminalStatusSet.contains(normalizedStatus)) { counterpartyName = dispute.adminPubkey; }lib/features/order/notfiers/abstract_mostro_notifier.dart (1)
150-174: DRY up role-aware peer derivation/updateSame logic appears in buyerTookOrder and holdInvoicePaymentAccepted. Extract a helper to compute/update peer once.
Example:
Peer? _derivePeerFromOrder(Session session, Order order) { final peerPubkey = session.role == Role.buyer ? order.sellerTradePubkey : session.role == Role.seller ? order.buyerTradePubkey : null; return peerPubkey != null ? Peer(publicKey: peerPubkey) : null; } void _applyPeer(SessionProvider notifier, String orderId, Peer? peer) { notifier.updateSession(orderId, (s) => s.peer = peer); state = state.copyWith(peer: peer); }Then call _applyPeer(..., _derivePeerFromOrder(session, order));
Also applies to: 198-221
lib/features/disputes/providers/dispute_providers.dart (1)
98-113: Prefer typing order state over dynamicUse a typed OrderState? instead of dynamic for clarity and safety.
- Session? matchingSession; - dynamic matchingOrderState; + Session? matchingSession; + OrderState? matchingOrderState; ... - return DisputeData.fromDispute( - dispute, - orderState: matchingOrderState, - userRole: userRole, - ); + return DisputeData.fromDispute( + dispute, + orderState: matchingOrderState, + userRole: userRole, + );Also applies to: 133-146
lib/features/disputes/screens/dispute_chat_screen.dart (5)
96-108: Make dependent reads reactive (watch) to avoid stale UIUsing
ref.readforsessionNotifierProviderandorderNotifierProvidermeans changes in sessions/orders won’t rebuild this screen. Preferref.watchsodisputeDatastays current.Example:
- final sessions = ref.read(sessionNotifierProvider); + final sessions = ref.watch(sessionNotifierProvider); ... - final orderState = ref.read(orderNotifierProvider(session.orderId!)); + final orderState = ref.watch(orderNotifierProvider(session.orderId!));If reactivity here is intentionally avoided to reduce rebuilds, add a comment explaining the decision.
Also applies to: 165-171
68-71: Avoid stringly-typed status checks
disputeData.status == 'in-progress'risks drift. Prefer an enum or centralized constants (e.g.,DisputeStatus.inProgressorDisputeStatuses.inProgress).- if (disputeData.status == 'in-progress') + if (disputeData.status == DisputeStatuses.inProgress) DisputeMessageInput(disputeId: disputeId)
217-219: Don’t swallow errors silentlyThe catch block drops the error. Log it for diagnostics.
- } catch (e) { - // Fallback to basic conversion on error - } + } catch (e, st) { + debugPrint('DisputeChatScreen: _convertToDisputeData fallback due to error: $e\n$st'); + // Fallback to basic conversion on error + }
241-252: EnsurecounterpartyDisplayreflects overriddencounterpartyYou override
counterpartybut downstream UI (DisputeStatusContent) readscounterpartyDisplay. IfcounterpartyDisplayisn’t derived fromcounterparty, nickname resolution may still see “unknown”.
- Verify
DisputeData.counterpartyDisplayderives fromcounterpartyor add acopyWith(counterpartyDisplay: …)equivalent if needed.- Alternatively, use a
copyWithif available instead of rebuilding by hand.
100-101: Avoid dynamic formatchingOrderState
Replacedynamic matchingOrderState;with
OrderState matchingOrderState;to restore type safety and IntelliSense.
lib/features/disputes/widgets/dispute_status_content.dart (3)
61-66: UsewithOpacityfor broader Flutter SDK compatibility
Color.withValues(alpha: …)requires newer Flutter. To avoid build issues on older SDKs, preferwithOpacity.- color: AppTheme.mostroGreen.withValues(alpha: 0.1), + color: AppTheme.mostroGreen.withOpacity(0.1), ... - color: AppTheme.mostroGreen.withValues(alpha: 0.3), + color: AppTheme.mostroGreen.withOpacity(0.3),If your minimum SDK guarantees
withValues, keep as-is.
26-28: Centralize status stringsComparing lowercase strings (
'resolved'|'closed'|'seller-refunded') is brittle. Prefer enum/constant source of truth shared with DisputeData.final isResolved = { DisputeStatuses.resolved, DisputeStatuses.closed, DisputeStatuses.sellerRefunded, }.contains(dispute.status);
158-195: Switch mixes descriptionKey and action/status; consider normalizingYou branch on
descriptionKeyhere, but resolved text also depends onactionand sometimesstatus. Consider centralizing the resolution of “state + action → message” to a single helper to avoid divergent logic across widgets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
lib/data/models/dispute.dart(3 hunks)lib/data/models/nostr_event.dart(1 hunks)lib/data/models/payload.dart(2 hunks)lib/features/chat/notifiers/chat_room_notifier.dart(1 hunks)lib/features/disputes/notifiers/dispute_chat_notifier.dart(1 hunks)lib/features/disputes/providers/dispute_providers.dart(3 hunks)lib/features/disputes/screens/dispute_chat_screen.dart(3 hunks)lib/features/disputes/widgets/dispute_info_card.dart(1 hunks)lib/features/disputes/widgets/dispute_message_input.dart(4 hunks)lib/features/disputes/widgets/dispute_messages_list.dart(6 hunks)lib/features/disputes/widgets/dispute_status_content.dart(6 hunks)lib/features/order/models/order_state.dart(2 hunks)lib/features/order/notfiers/abstract_mostro_notifier.dart(5 hunks)lib/l10n/intl_en.arb(2 hunks)lib/l10n/intl_es.arb(2 hunks)lib/l10n/intl_it.arb(2 hunks)lib/services/mostro_service.dart(1 hunks)lib/shared/utils/nostr_utils.dart(0 hunks)
💤 Files with no reviewable changes (1)
- lib/shared/utils/nostr_utils.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-08T23:54:09.987Z
Learnt from: chebizarro
PR: MostroP2P/mobile#110
File: lib/features/trades/widgets/mostro_message_detail_widget.dart:134-141
Timestamp: 2025-06-08T23:54:09.987Z
Learning: In the OrderState refactor, public keys should be accessed via `tradeState.peer?.publicKey` from the OrderState instance rather than through `session?.peer?.publicKey`, as the OrderState encapsulates peer information directly.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
🔇 Additional comments (11)
lib/features/order/models/order_state.dart (1)
160-167: Admin pubkey extraction on adminTookDispute looks goodPulling admin pubkey from Peer payload and preserving existing when absent is correct. Setting adminTookAt now and status to in-progress matches the flow.
Also applies to: 172-173
lib/l10n/intl_it.arb (1)
758-759: New dispute localization keys added (IT) — verify locale parityStrings look good and placeholders are defined. Please ensure the same keys exist in the base/EN ARB (and any other supported locales) so gen_l10n doesn’t fail at build time.
Would you confirm intl_en.arb contains:
- disputeChatClosed
- disputeSettledBuyerMessage / disputeSettledSellerMessage
- disputeCanceledBuyerMessage / disputeCanceledSellerMessage
- disputeOpenedByYouAgainstSeller / disputeOpenedByYouAgainstBuyer
Also applies to: 771-775, 781-792
lib/data/models/dispute.dart (1)
340-347: Role-aware DisputeData factory: LGTM
- Passing userRole into DisputeData and defaulting to unknown is sensible.
- Counterparty preference: peer first, then admin (non-terminal) is correct.
- Debug prints aid diagnosis.
Also applies to: 374-379, 395-398, 410-412, 429-431
lib/l10n/intl_es.arb (1)
704-705: New dispute localization keys added (ES) — verify locale parityTranslations and placeholder blocks look consistent. Please ensure base/EN and other locales define the same keys to keep gen_l10n happy.
Confirm intl_en.arb includes:
- disputeChatClosed
- disputeSettledBuyerMessage / disputeSettledSellerMessage
- disputeCanceledBuyerMessage / disputeCanceledSellerMessage
- disputeOpenedByYouAgainstSeller / disputeOpenedByYouAgainstBuyer
Also applies to: 717-720, 727-738
lib/data/models/payload.dart (1)
9-10: Call sites correctly pass inner payload dispute_chat_notifier unwraps thedmwrapper (CLI format handlespayload['text_message']directly; old format calls MostroMessage.fromJson on the innerpayload), so Payload.fromJson never receives the outer wrapper—no further action needed.lib/features/disputes/widgets/dispute_info_card.dart (1)
22-26: Counterparty fallback logic looks good; verify provider return typeCondition is correct and prevents unnecessary provider work. Please confirm nickNameProvider returns a String (not AsyncValue); otherwise unwrap appropriately.
lib/features/chat/notifiers/chat_room_notifier.dart (1)
191-194: Wrap call updated to (senderKeys, receiverPubkey) — OKThis matches the new mostroWrap signature and NIP-59 flow.
lib/services/mostro_service.dart (1)
82-88: Message key fallback for storage is solidUsing decryptedEvent.id with fallback to envelope id (and timestamp) avoids missing IDs from admin messages.
lib/features/order/notfiers/abstract_mostro_notifier.dart (1)
293-303: Dispute createdAt propagation and logging — OKUsing event timestamp when present and persisting createdAt/action/status improves consistency and UI behavior.
Also applies to: 336-376
lib/features/disputes/providers/dispute_providers.dart (1)
44-59: Session/order watchers for fine-grained invalidation — OKTargeted ref.watch to the containing order state is a good optimization; fallback path is sensible.
lib/features/disputes/widgets/dispute_status_content.dart (1)
21-23: Remove AsyncValue handling —nickNameProvideris synchronous
nickNameProvideris defined asProvider.family<String, String>, returning aStringdirectly. There’s noAsyncValueto unwrap; the existing usage is correct.
… from seconds to ms
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/features/disputes/screens/dispute_chat_screen.dart (2)
111-114: Consider logging errors before continuing.Empty catch blocks silently swallow errors. Add debug logging to aid troubleshooting.
} catch (e) { - // Continue checking other sessions + debugPrint('DisputeChatScreen: Error reading order state for session ${session.orderId}: $e'); continue; }Also applies to: 210-213
227-257: Consider optimizing object creation.Creating
standardDisputeDataand then recreating a newDisputeDatawith most identical fields is inefficient. IfDisputeDatahas acopyWithmethod, use it instead.Example optimization if
copyWithexists:DisputeData _createDisputeDataWithChatInfo( Dispute dispute, dynamic orderState, String peerPubkey, UserRole? userRole, ) { return DisputeData.fromDispute( dispute, orderState: orderState, userRole: userRole, ).copyWith(counterparty: peerPubkey); }Alternatively, consider adding a
counterpartyparameter toDisputeData.fromDispute.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/features/disputes/notifiers/dispute_chat_notifier.dart(1 hunks)lib/features/disputes/screens/dispute_chat_screen.dart(3 hunks)lib/features/order/models/order_state.dart(2 hunks)lib/services/mostro_service.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/features/disputes/notifiers/dispute_chat_notifier.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/order/models/order_state.dart
🔇 Additional comments (4)
lib/features/disputes/screens/dispute_chat_screen.dart (2)
23-90: LGTM! Clean provider integration and AsyncValue handling.The migration to ConsumerWidget with AsyncValue.when pattern is well-implemented. The conditional input visibility based on dispute status is appropriate.
102-105: Previous NPE concern addressed.The null checks have been added as suggested in the previous review, preventing NPE when both
orderIdvalues are null.lib/services/mostro_service.dart (2)
72-82: LGTM! Empty payload guard correctly implemented.The validation logic correctly addresses the past review comment by checking both that
resultis aListand that it's not empty before accessingresult[0]. The subsequent dispute message skip logic safely accesses the first element after validation.The defensive check
result[0] is Mapbefore thecontainsKeycall is good defensive programming.
87-93: LGTM! Sensible ID fallback chain.The three-tier fallback chain (decryptedEvent.id → event.id → timestamp-based key) appropriately handles edge cases where admin messages might not include an ID in the decrypted event.
Given that line 47 expects
event.idto be non-null (via the!operator), the timestamp-based fallback should rarely execute in practice, making it good defensive programming for unexpected edge cases.The updated logging on line 92 correctly reflects the fallback logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/features/disputes/screens/dispute_chat_screen.dart (1)
121-129: NPE guard on orderId compare is correctly fixed.Both sides are null‑checked before deref; safe to call orderNotifierProvider.
🧹 Nitpick comments (11)
lib/features/disputes/providers/dispute_read_status_provider.dart (1)
5-7: Auto‑dispose this family provider (it’s a rebuild signal, not long‑lived state).Prevents stale instances/memory retention and aligns with Riverpod best practices.
-final disputeReadStatusProvider = StateProvider.family<int, String>((ref, disputeId) { - return DateTime.now().millisecondsSinceEpoch; -}); +final disputeReadStatusProvider = + StateProvider.autoDispose.family<int, String>((ref, disputeId) { + return DateTime.now().millisecondsSinceEpoch; +});lib/features/disputes/widgets/dispute_list_item.dart (2)
20-24: Ensure navigation still happens if marking as read fails.Guard with try/finally so onTap always runs.
- onTap: () async { - // Mark dispute as read when user opens it - await DisputeReadStatusService.markDisputeAsRead(dispute.disputeId); - onTap(); - }, + onTap: () async { + try { + // Mark dispute as read when user opens it + await DisputeReadStatusService.markDisputeAsRead(dispute.disputeId); + } finally { + onTap(); + } + },
1-4: Optional: Make this a ConsumerWidget and bump the read‑status provider for instant UI updates.This triggers list/UI to drop the unread dot immediately after marking as read.
import 'package:flutter/material.dart'; import 'package:mostro_mobile/features/disputes/widgets/dispute_icon.dart'; import 'package:mostro_mobile/features/disputes/widgets/dispute_content.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:mostro_mobile/features/disputes/providers/dispute_read_status_provider.dart'; import 'package:mostro_mobile/services/dispute_read_status_service.dart'; import 'package:mostro_mobile/data/models/dispute.dart'; -class DisputeListItem extends StatelessWidget { +class DisputeListItem extends ConsumerWidget { final DisputeData dispute; final VoidCallback onTap; const DisputeListItem({ super.key, required this.dispute, required this.onTap, }); @override - Widget build(BuildContext context) { + Widget build(BuildContext context, WidgetRef ref) { return GestureDetector( - onTap: () async { + onTap: () async { // Mark dispute as read when user opens it - await DisputeReadStatusService.markDisputeAsRead(dispute.disputeId); - onTap(); + try { + await DisputeReadStatusService.markDisputeAsRead(dispute.disputeId); + // Notify provider to refresh dependent widgets + ref.read(disputeReadStatusProvider(dispute.disputeId).notifier).state = + DateTime.now().millisecondsSinceEpoch; + } finally { + onTap(); + } },Also applies to: 7-15, 20-24
lib/services/dispute_read_status_service.dart (1)
1-20: Prefer SharedPreferencesAsync (newer API) for forward compatibility.Library guidance recommends Async/WithCache variants over legacy API. This eases future migrations and aligns with package direction. Based on learnings
- final prefs = await SharedPreferences.getInstance(); + final prefs = await SharedPreferencesAsync.getInstance(); final key = '$_keyPrefix$disputeId'; final timestamp = DateTime.now().millisecondsSinceEpoch; await prefs.setInt(key, timestamp);- final prefs = await SharedPreferences.getInstance(); + final prefs = await SharedPreferencesAsync.getInstance(); final key = '$_keyPrefix$disputeId'; return prefs.getInt(key);Additionally, consider injecting a clock for testability:
typedef Now = int Function(); static Future<void> markDisputeAsRead(String disputeId, {Now now}) async { final prefs = await SharedPreferencesAsync.getInstance(); final key = '$_keyPrefix$disputeId'; final timestamp = (now ?? () => DateTime.now().millisecondsSinceEpoch)(); await prefs.setInt(key, timestamp); }Please verify the project is on shared_preferences >= 2.3.0 before migrating.
lib/features/disputes/widgets/dispute_content.dart (2)
60-79: Model unread as a Riverpod FutureProvider instead of a FutureBuilder.Keeps UI declarative and avoids recreating Futures in build. As per coding guidelines
Example:
final hasUnreadDisputeMessagesProvider = FutureProvider.family<bool, String>((ref, disputeId) async { final messages = ref.watch(disputeChatNotifierProvider(disputeId)).messages; return DisputeReadStatusService.hasUnreadMessages(disputeId, messages); }); // In build: final hasUnread = ref.watch(hasUnreadDisputeMessagesProvider(widget.dispute.disputeId)); if (normalizedStatus == 'in-progress') hasUnread.when( data: (v) => v ? _UnreadDot() : const SizedBox.shrink(), loading: () => const SizedBox.shrink(), error: (_, __) => const SizedBox.shrink(), );
70-77: Add Semantics for the unread dot (accessibility).Expose a short label for screen readers.
- return Container( - width: 8, - height: 8, - decoration: const BoxDecoration( - color: Colors.red, - shape: BoxShape.circle, - ), - ); + return Semantics( + label: 'Unread messages', + child: Container( + width: 8, + height: 8, + decoration: const BoxDecoration( + color: Colors.red, + shape: BoxShape.circle, + ), + ), + );lib/features/disputes/screens/dispute_chat_screen.dart (3)
30-39: Good post‑frame side‑effect. Consider also auto‑marking as read on new incoming messages.Listen to chat messages while the screen is visible and refresh read‑status.
@override void initState() { super.initState(); WidgetsBinding.instance.addPostFrameCallback((_) { DisputeReadStatusService.markDisputeAsRead(widget.disputeId); ref.read(disputeReadStatusProvider(widget.disputeId).notifier).state = DateTime.now().millisecondsSinceEpoch; }); // Mark as read when new messages arrive (admin -> user) ref.listen( disputeChatNotifierProvider(widget.disputeId).select((s) => s.messages.length), (prev, next) { DisputeReadStatusService.markDisputeAsRead(widget.disputeId); ref.read(disputeReadStatusProvider(widget.disputeId).notifier).state = DateTime.now().millisecondsSinceEpoch; }, ); }
52-53: Remove hardcoded fallbacks; use localized strings only.Aligns with project guideline to avoid hardcoded text. As per coding guidelines
- S.of(context)?.disputeDetails ?? 'Dispute Details', + S.of(context).disputeDetails,- S.of(context)?.disputeNotFound ?? 'Dispute not found', + S.of(context).disputeNotFound,- S.of(context)?.errorLoadingDispute(error.toString()) ?? 'Error loading dispute: ${error.toString()}', + S.of(context).errorLoadingDispute(error.toString()),Also applies to: 69-72, 100-106
111-223: Extract dispute→UI conversion into a provider/service for clarity and testability.This logic is sizable, touches sessions/orders/roles/peers, and is easier to validate in isolation. Keep widgets lean and side‑effect free.
Sketch:
final disputeDataProvider = Provider.family<DisputeData, String>((ref, disputeId) { final dispute = ref.watch(disputeDetailsProvider(disputeId)).requireValue!; final sessions = ref.read(sessionNotifierProvider); // ...existing matching logic moved here... return resolvedDisputeData; });Then in build:
final disputeData = ref.watch(disputeDataProvider(widget.disputeId));lib/features/disputes/widgets/dispute_messages_list.dart (2)
214-260: Consider passing messages instead of re-watching the provider.The provider is already watched at line 68 (
ref.watch(disputeChatNotifierProvider(widget.disputeId))), and then watched again at lines 224-225 within this builder. While Riverpod efficiently deduplicates watches, you could improve clarity by passing themessageslist as a parameter to_buildAdminAssignmentNotificationfrom the already-watched value.For example:
Widget _buildAdminAssignmentNotification(BuildContext context, List<DisputeChat> messages) { // ... existing logic using messages parameter instead of re-watching }Then call it from line 80 and line 146:
_buildAdminAssignmentNotification(context, messages),
301-308: Hardcoded terminal states may need updates.The
_isResolvedStatusmethod hardcodes four terminal states:resolved,closed,solved, andseller-refunded. If the backend adds new terminal states in the future, this method will need to be updated. Consider whether these states should be centralized in a constants file or configuration.For better maintainability:
// In a constants file: class DisputeStatus { static const terminalStates = {'resolved', 'closed', 'solved', 'seller-refunded'}; static bool isTerminal(String status) { final normalized = status.trim().toLowerCase().replaceAll(RegExp(r'[\s_]+'), '-'); return terminalStates.contains(normalized); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/features/disputes/providers/dispute_read_status_provider.dart(1 hunks)lib/features/disputes/screens/dispute_chat_screen.dart(2 hunks)lib/features/disputes/widgets/dispute_content.dart(1 hunks)lib/features/disputes/widgets/dispute_list_item.dart(2 hunks)lib/features/disputes/widgets/dispute_messages_list.dart(6 hunks)lib/services/dispute_read_status_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
lib/features/**/providers/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Riverpod providers by feature under
features/{feature}/providers/
Files:
lib/features/disputes/providers/dispute_read_status_provider.dart
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possibleName Riverpod providers as Provider or Notifier
Files:
lib/features/disputes/providers/dispute_read_status_provider.dartlib/features/disputes/screens/dispute_chat_screen.dartlib/features/disputes/widgets/dispute_messages_list.dartlib/features/disputes/widgets/dispute_list_item.dartlib/features/disputes/widgets/dispute_content.dartlib/services/dispute_read_status_service.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
Files:
lib/features/disputes/providers/dispute_read_status_provider.dartlib/features/disputes/screens/dispute_chat_screen.dartlib/features/disputes/widgets/dispute_messages_list.dartlib/features/disputes/widgets/dispute_list_item.dartlib/features/disputes/widgets/dispute_content.dartlib/services/dispute_read_status_service.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/disputes/providers/dispute_read_status_provider.dartlib/features/disputes/screens/dispute_chat_screen.dartlib/features/disputes/widgets/dispute_messages_list.dartlib/features/disputes/widgets/dispute_list_item.dartlib/features/disputes/widgets/dispute_content.dart
lib/services/**
📄 CodeRabbit inference engine (AGENTS.md)
Place service-layer code in lib/services/
Files:
lib/services/dispute_read_status_service.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 (5)
lib/services/dispute_read_status_service.dart (1)
22-45: Unread logic looks correct (non‑user messages only).Edge cases (no last read time, empty messages) are handled well.
lib/features/disputes/widgets/dispute_messages_list.dart (4)
11-26: LGTM! Proper Riverpod migration.The migration from
StatefulWidgettoConsumerStatefulWidgetis correctly implemented, enabling direct access to Riverpod providers. The addition of an optionalscrollControllerparameter provides good flexibility for parent widgets.
32-49: LGTM! Proper lifecycle management.The scroll controller is correctly initialized (using provided or creating new) and disposed conditionally. The post-frame callback ensures scrolling occurs after the initial layout, and the conditional disposal prevents double-free errors.
131-168: LGTM! Well-structured empty state layout.The use of
LayoutBuilder,ConstrainedBox, andIntrinsicHeightensures proper sizing and scrollability for empty states. TheSpacerappropriately pushes the resolution notification to the bottom for resolved disputes.
338-393: LGTM! Clear and consistent UI components.The chat-closed UI components use appropriate styling, localization, and visual indicators. The combination of lock icon with grey colors effectively communicates the closed state.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/disputes/widgets/dispute_messages_list.dart(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possibleName Riverpod providers as Provider or Notifier
Files:
lib/features/disputes/widgets/dispute_messages_list.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
Files:
lib/features/disputes/widgets/dispute_messages_list.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/disputes/widgets/dispute_messages_list.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/disputes/widgets/dispute_messages_list.dart (2)
2-2: LGTM!The Riverpod import and notifier import changes correctly support the widget's migration to a Riverpod-aware implementation.
Also applies to: 6-6
15-16: LGTM!The migration to
ConsumerStatefulWidgetandConsumerStatefollows the correct Riverpod pattern for stateful widgets that need to watch providers.Also applies to: 31-31, 34-34
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/features/disputes/widgets/dispute_messages_list.dart (3)
77-79: Make loading UI const.Minor allocation win; both widgets support const.
- return Center( - child: CircularProgressIndicator(), - ); + return const Center( + child: CircularProgressIndicator(), + );
38-46: Auto-scroll on new messages (+ mounted check).Add mounted check in the post‑frame callback and listen to message count to scroll when new messages arrive.
// Scroll to bottom on first load WidgetsBinding.instance.addPostFrameCallback((_) { - _scrollToBottom(animate: false); + if (!mounted) return; + _scrollToBottom(animate: false); }); + + // Auto-scroll when new messages arrive + ref.listen<int>( + disputeChatNotifierProvider(widget.disputeId).select((s) => s.messages.length), + (previous, next) { + if (previous == null || next > previous) { + if (!mounted) return; + WidgetsBinding.instance.addPostFrameCallback((_) => _scrollToBottom()); + } + }, + );
251-267: Prefer themed colors over hard-coded Colors. for consistency/dark-mode.*Consider moving these to AppTheme to keep styling centralized and adaptable.
Also applies to: 359-371
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/disputes/widgets/dispute_messages_list.dart(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possibleName Riverpod providers as Provider or Notifier
Files:
lib/features/disputes/widgets/dispute_messages_list.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
Files:
lib/features/disputes/widgets/dispute_messages_list.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/disputes/widgets/dispute_messages_list.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 (3)
lib/features/disputes/widgets/dispute_messages_list.dart (3)
72-92: LGTM: Provider state handling added correctly.Clean loading/error branches with localized strings and themed styles. This prevents crashes and improves UX.
233-245: LGTM: Duplicate provider watch removed.Passing messages into _buildAdminAssignmentNotification avoids extra ref.watch and rebuilds. Good cleanup.
143-151: LGTM: Simplified message indexing is correct.Direct index offset is clear and aligns with _getItemCount contract.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
En el chat solo se deben permitir mensajes que vengan del solver que tome la disputa, porque si no un estafador puede enviarle mensajes a los usuarios en disputa haciendose pasar por un admin.
En la captura de pantalla, el admin no habia tomado la disputa todavia, pero desde la contraparte le envie un gift wrap a este usuario y en el chat de disputa se le muestra:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/features/disputes/notifiers/dispute_chat_notifier.dart (1)
118-118: DeclareFuture<void>return type for async handler.The method uses
async/awaitbut declaresvoidreturn type. While Dart permits this, explicitly declaringFuture<void>improves clarity and matches the async convention.Apply this diff:
- void _onChatEvent(NostrEvent event) async { + Future<void> _onChatEvent(NostrEvent event) async {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/disputes/notifiers/dispute_chat_notifier.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possibleName Riverpod providers as Provider or Notifier
Files:
lib/features/disputes/notifiers/dispute_chat_notifier.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
Files:
lib/features/disputes/notifiers/dispute_chat_notifier.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/disputes/notifiers/dispute_chat_notifier.dart
🔇 Additional comments (6)
lib/features/disputes/notifiers/dispute_chat_notifier.dart (6)
19-42: LGTM: Clean state class implementation.The immutable state class with
copyWithfollows best practices for Riverpod StateNotifier pattern.
90-115: Session hand-off now properly handled.The listener now correctly watches
sessionNotifierProviderfor changes and retries subscription when a matching session appears, resolving the previously flagged issue where initialization would fail if the session arrived asynchronously.
145-187: LGTM: Dual-format message parsing.The parser correctly handles both CLI format (with
"dm"key) and legacy MostroMessage format, with appropriate error handling and null-safe navigation.
194-210: LGTM: Robust authorization checks.The security validation properly enforces that admin messages must come from the assigned
dispute.adminPubkey, with appropriate logging for audit trails.
436-466: LGTM: Defensive session lookup.The method safely iterates through sessions with appropriate null checks and error handling. The per-session try-catch (lines 453-456) ensures that errors in one session don't prevent finding the match in others.
468-483: LGTM: Proper cleanup and provider setup.The
dispose()method correctly cancels both subscriptions, and the provider properly initializes each per-dispute instance.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
lib/data/models/dispute_chat.dart (2)
66-71: Avoid double–int comparison for timestamp heuristic.Use an int literal to prevent subtle numeric coercion.
- int milliseconds = v < 1e12 ? v * 1000 : v; + // 1e12 ms ~= Sat Sep 09 2001; values below are likely seconds + final int milliseconds = v < 1000000000000 ? v * 1000 : v;
45-64: Consider equality semantics for list diffing and dedupe.Override ==/hashCode (e.g., by id) or use Equatable to make UI updates and dedup predictable and cheaper. Optional, but helps when message lists grow.
lib/features/disputes/notifiers/dispute_chat_notifier.dart (3)
223-240: Persist admin_pubkey for inbound events (consistency).You read
admin_pubkeyduring historical load, but it’s never written here. Write it so rehydration is consistent.{ 'id': eventId, 'content': messageText, 'created_at': eventTimestamp.millisecondsSinceEpoch ~/ 1000, 'kind': unwrappedEvent.kind, 'pubkey': senderPubkey, + 'admin_pubkey': isFromAdmin ? senderPubkey : null, 'sig': unwrappedEvent.sig, 'tags': unwrappedEvent.tags, 'type': 'dispute_chat', 'dispute_id': disputeId, 'is_from_user': !isFromAdmin, 'isPending': false, },
429-446: Persist admin_pubkey for outbound events (symmetry).Store the assigned admin’s key alongside user-sent messages for uniform querying.
{ 'id': rumorId, 'content': text, 'created_at': rumorTimestamp.millisecondsSinceEpoch ~/ 1000, 'kind': rumor.kind, 'pubkey': rumor.pubkey, + 'admin_pubkey': dispute.adminPubkey, 'sig': rumor.sig, 'tags': rumor.tags, 'type': 'dispute_chat', 'dispute_id': disputeId, 'is_from_user': true, 'isPending': false, },
84-91: Optional: add asinceto reduce backlog on subscribe.To avoid replaying a large history, consider a
sincewindow (e.g., last 7–14 days) if acceptable for disputes.final request = NostrRequest( filters: [ NostrFilter( kinds: [1059], // Gift Wrap - p: [session.tradeKey.public], // Messages to our tradeKey + p: [session.tradeKey.public], // or tags: {'p': [...]} + since: DateTime.now().subtract(const Duration(days: 14)).millisecondsSinceEpoch ~/ 1000, ), ], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/data/models/dispute_chat.dart(3 hunks)lib/features/disputes/notifiers/dispute_chat_notifier.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possibleName Riverpod providers as Provider or Notifier
Files:
lib/data/models/dispute_chat.dartlib/features/disputes/notifiers/dispute_chat_notifier.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
Files:
lib/data/models/dispute_chat.dartlib/features/disputes/notifiers/dispute_chat_notifier.dart
lib/data/**
📄 CodeRabbit inference engine (AGENTS.md)
Store persistence and API code under lib/data/
Files:
lib/data/models/dispute_chat.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/disputes/notifiers/dispute_chat_notifier.dart
🔇 Additional comments (3)
lib/data/models/dispute_chat.dart (1)
8-19: Nice: pending/error state + copyWith added cleanly.These additions enable clear UX for send/ack/failure flows and integrate well with state updates.
Also applies to: 45-64
lib/features/disputes/notifiers/dispute_chat_notifier.dart (2)
82-95: The code is correct as written—no changes needed.dart_nostr's Filter has a dedicated property for pubkey tags (the "p" / #p filter), so using
p: [session.tradeKey.public]directly is the proper approach. The suggested diff in the review comment is incorrect.Likely an incorrect or invalid review comment.
269-279: No issues found—sort direction is correct.The
SortOrderconstructor's second parameter is a booleanascendingflag:true(or omitted) sorts ascending,falsesorts descending. Your code correctly usesSortOrder('created_at', true)to sort in ascending order, which for timestamps means oldest first, exactly as your comment states. No changes needed.
| // Publish to network | ||
| ref.read(nostrServiceProvider).publishEvent(wrappedEvent); | ||
| _logger.i('Dispute message sent successfully to admin for dispute: $disputeId'); | ||
|
|
||
| // Update message to isPending=false (success) | ||
| final sentMessage = pendingMessage.copyWith(isPending: false); | ||
| final updatedMessages = state.messages.map((m) => m.id == rumorId ? sentMessage : m).toList(); | ||
| state = state.copyWith(messages: updatedMessages); |
There was a problem hiding this comment.
🧩 Analysis chain
Don’t mark sent before publish completes; await publish and ack.
publishEvent(wrappedEvent) isn’t awaited, so errors won’t be caught and isPending flips to false immediately. Await the publish and, if available, relay OK/ACK before clearing pending.
- // Publish to network
- ref.read(nostrServiceProvider).publishEvent(wrappedEvent);
+ // Publish to network and await completion/ack
+ final nostr = ref.read(nostrServiceProvider);
+ await nostr.publishEvent(wrappedEvent); // if this returns Future
+ // If your service supports ACKs, prefer:
+ // final ok = await nostr.publishEventWithAck(wrappedEvent, timeout: const Duration(seconds: 5));
+ // if (!ok) throw Exception('Relay did not ACK publish');Follow-up: only set isPending=false after the awaited success; on failure keep isPending=true or set error and offer retry. Based on learnings.
🏁 Script executed:
#!/bin/bash
# Verify publish API returns a Future and find an ACK-capable variant
rg -nP -C2 'class\s+.*NostrService|publishEvent\s*\(' --type=dartLength of output: 5737
[/run_scripts]
Once the script results are provided, I'll examine the implementation to determine if:
- The publishEvent method can throw exceptions or propagate errors
- Whether errors from the underlying Nostr instance are handled/propagated
- Whether there's any relay ACK handling available
Then I'll be able to finalize the review comment.
🏁 Script executed:
#!/bin/bash
# Read full NostrService.publishEvent() implementation
cat lib/services/nostr_service.dart | sed -n '119,150p'Length of output: 1147
Await publishEvent() to catch network errors; isPending flag change must follow successful publish.
publishEvent(wrappedEvent) is not awaited (lib/services/nostr_service.dart:119 confirms it returns Future<void>). Since publishEvent() throws on initialization failure, empty relays, or network errors, unawaiting it silently discards these exceptions. The isPending=false assignment happens immediately, marking the message as sent before the actual publish completes or fails.
Await the publish and handle errors appropriately:
- // Publish to network
- ref.read(nostrServiceProvider).publishEvent(wrappedEvent);
- _logger.i('Dispute message sent successfully to admin for dispute: $disputeId');
-
- // Update message to isPending=false (success)
- final sentMessage = pendingMessage.copyWith(isPending: false);
- final updatedMessages = state.messages.map((m) => m.id == rumorId ? sentMessage : m).toList();
- state = state.copyWith(messages: updatedMessages);
+ // Publish to network and await completion
+ try {
+ await ref.read(nostrServiceProvider).publishEvent(wrappedEvent);
+ _logger.i('Dispute message sent successfully to admin for dispute: $disputeId');
+
+ // Update message to isPending=false only after successful publish
+ final sentMessage = pendingMessage.copyWith(isPending: false);
+ final updatedMessages = state.messages.map((m) => m.id == rumorId ? sentMessage : m).toList();
+ state = state.copyWith(messages: updatedMessages);
+ } catch (e, stackTrace) {
+ _logger.e('Failed to publish dispute message for dispute: $disputeId', error: e, stackTrace: stackTrace);
+ // Keep isPending=true or set error state; offer user retry option
+ rethrow;
+ }Note: A similar unawaited call exists at lib/features/chat/notifiers/chat_room_notifier.dart:208 that should receive the same fix.
🤖 Prompt for AI Agents
In lib/features/disputes/notifiers/dispute_chat_notifier.dart around lines 420
to 427, publishEvent(wrappedEvent) is called without awaiting so
network/initialization errors are lost and the message is immediately marked
isPending=false; await ref.read(nostrServiceProvider).publishEvent(wrappedEvent)
and wrap it in try/catch, only set the message to isPending=false after the
await completes successfully, and on error log the exception and update the
message state to reflect failure (e.g., keep isPending=true or set an error
flag) so UI/state remains consistent; apply the same awaited+try/catch pattern
to the similar unawaited call in
lib/features/chat/notifiers/chat_room_notifier.dart around line 208.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/features/disputes/notifiers/dispute_chat_notifier.dart (1)
528-558: Session lookup works correctly; consider optimization for many sessions.The implementation correctly searches all sessions to find the one containing this dispute (lines 536-550). The current O(n) iteration is acceptable for typical use cases with few active sessions.
For scenarios with many concurrent sessions, consider maintaining a
disputeId → sessionIdindex in a provider to avoid repeated iteration:// In a separate provider final disputeSessionIndexProvider = Provider<Map<String, String>>((ref) { final sessions = ref.watch(sessionNotifierProvider); final index = <String, String>{}; for (final session in sessions) { if (session.orderId != null) { try { final order = ref.read(orderNotifierProvider(session.orderId!)); if (order.dispute?.disputeId != null) { index[order.dispute!.disputeId!] = session.orderId!; } } catch (_) {} } } return index; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/features/chat/notifiers/chat_room_notifier.dart(1 hunks)lib/features/disputes/notifiers/dispute_chat_notifier.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use post-frame callbacks for UI side effects (e.g., SnackBars/dialogs)
UseS.of(context).keyNamefor all user-facing strings (no hardcoded text)
Always use localized strings instead of hardcoded text
Pass BuildContext to methods that need localization (useS.of(context))
Checkmountedbefore using BuildContext after async gaps
Useconstconstructors where possibleName Riverpod providers as Provider or Notifier
Files:
lib/features/chat/notifiers/chat_room_notifier.dartlib/features/disputes/notifiers/dispute_chat_notifier.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.dart: All code comments must be in English
Use clear, concise English for variable and function names
Maintain zeroflutter analyzeissues
Use latest Flutter/Dart APIs (e.g., preferwithValues()overwithOpacity())
Remove unused imports and dependencies
**/*.dart: Use Dart formatter defaults (two-space indentation, trailing commas) and run flutter format before committing
Resolve all Flutter analyzer warnings (flutter analyze must be clean)
Files:
lib/features/chat/notifiers/chat_room_notifier.dartlib/features/disputes/notifiers/dispute_chat_notifier.dart
lib/features/**
📄 CodeRabbit inference engine (AGENTS.md)
Place application feature code under lib/features//, grouped by domain
Files:
lib/features/chat/notifiers/chat_room_notifier.dartlib/features/disputes/notifiers/dispute_chat_notifier.dart
🔇 Additional comments (10)
lib/features/chat/notifiers/chat_room_notifier.dart (2)
191-194: LGTM! Signature matches new Gift Wrap API.The updated
mostroWrapcall correctly passessenderKeysandreceiverPubkeyparameters, aligning with the new NIP-59 Gift Wrap flow.
201-226: Good: Publish is now awaited and errors are handled.The awaited
publishEventcall with proper error handling addresses previous review feedback. The inner catch (lines 205-211) removes failed messages and rethrows, while the outer catch (lines 220-226) provides a final safety net for wrapping errors. The duplicate removal logic is harmless since the second filter is a no-op if the message was already removed.lib/features/disputes/notifiers/dispute_chat_notifier.dart (8)
19-42: LGTM! Well-structured state class.The
DisputeChatStatefollows standard patterns with immutability,copyWithsupport, and const constructor.
66-95: Subscription leak fix confirmed.The code now properly cancels the previous subscription before creating a new one (lines 76-80), addressing the prior review feedback.
97-122: Session hand-off now works correctly; consider error handling for unawaited subscribe.The listener on
sessionNotifierProvider(line 106) correctly watches for session availability and calls_subscribe()when found, addressing the previous critical review feedback. However, line 118 usesunawaited(_subscribe()), which means if_subscribethrows an exception (e.g.,nostrService.subscribeToEventsfails), the error will be silently swallowed and the user won't receive messages.Consider wrapping the
unawaitedcall in a try-catch or using.catchError:unawaited(_subscribe().catchError((error, stack) { _logger.e('Failed to subscribe after session available: $error', stackTrace: stack); // Optionally set state.error or retry }));
124-261: LGTM! Solid security validation for incoming messages.The event handler correctly unwraps Gift Wrap events, parses both CLI and legacy formats, and validates sender pubkeys (lines 201-217). The security check ensures admin messages originate from the expected
dispute.adminPubkey, preventing impersonation.
263-337: LGTM! Historical messages properly filtered by authorized pubkeys.The loader applies the same security validation as
_onChatEvent(lines 293-310), ensuring only messages from the user or the assigned admin are displayed. Note that if an admin changes, messages from the previous admin will be filtered out—this appears intentional for security consistency.
420-460: Excellent: Publish is awaited with comprehensive error handling.The awaited
publishEvent(line 422) with proper error handling addresses previous critical review feedback. The implementation correctly:
- Marks messages as
isPending=falsewith error details on failure (lines 428-431)- Updates both state and storage to reflect the error
- Returns early to prevent marking as success (line 459)
374-526: LGTM! Optimistic UI with proper rollback on failure.The send flow now properly implements:
- Optimistic update with
isPending=true(line 380)- Awaited publish with error handling (lines 421-460)
- Success confirmation with
isPending=false(line 463)- Persistent error state in storage and UI (lines 438-458)
This addresses all previous review feedback about optimistic updates misleading users.
560-575: LGTM! Proper cleanup and provider initialization.The
disposemethod correctly cancels subscriptions (lines 562-563), and the provider properly initializes the notifier (line 572).
Dispute Chat Communication (Admin ↔ User)
Implements bidirectional encrypted messaging between admin (CLI) and users (mobile) for dispute resolution, following NIP-59 (Gift Wrap) protocol.
Changes
Encryption/Decryption (nostr_event.dart)
id/sigin unsigned RUMORs without breaking validationDispute Chat (dispute_chat_notifier.dart)
[{"dm": {"version": 1, "action": "send-dm", "payload": {"text_message": "..."}}}, null]matching CLI expectations{"dm": {...}}) and legacy MostroMessage formatMessage Routing (mostro_service.dart)
"dm"key) and delegate to DisputeChatNotifierChat Room (
chat_room_notifier.dart)Testing
Summary by CodeRabbit
New Features
Improvements
Bug Fixes