Fix: Error restoring chat #438
Conversation
Addresses issues encountered during chat restoration, including: - Improves error handling when session or peer data is missing, displaying appropriate error messages. - Fixes keyboard visibility tracking to ensure proper scrolling when the keyboard appears. - Adds peer information to the session during restore. - Increases delay waiting for messages to arrive to 10 seconds.
Addresses an issue where the role and peer were not being correctly determined during the restore process. This commit refactors the logic for determining the user's role (buyer or seller) and their corresponding peer based on trade keys, and ensures that the role and peer are properly determined for each order during restore. This fix resolves an issue that was preventing chat messages from being restored correctly.
Ensures the chat listener is active before historical messages arrive during the restore process. This prevents message loss due to the broadcast stream dropping events when no listener is present. Fixes error where chat restoring would fail
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds runtime session and peer validation in chat UI with explicit ChatErrorScreen and localization; derives and persists Peer during restore and starts chat subscriptions; tightens chat notifiers to avoid broad fallbacks and to only surface chats with messages. Changes
Sequence DiagramsequenceDiagram
participant RM as RestoreManager
participant Store as SessionStore
participant Sub as ChatSubscriptionService
participant CRS as ChatRoomScreen
participant Log as Logger
RM->>RM: derive Peer from restore payload
RM->>Store: persist Session (with Peer?)
alt peer present
RM->>Sub: subscribe to chat updates for order
Sub-->>RM: subscription active
end
CRS->>Store: load Session by id
CRS->>CRS: validate session != null
alt session null
CRS-->>CRS: show ChatErrorScreen.sessionNotFound
CRS->>Log: log session missing
else
CRS->>CRS: validate session.peer != null
alt peer null
CRS-->>CRS: show ChatErrorScreen.peerUnavailable
CRS->>Log: log peer missing
else
CRS-->>CRS: render chat UI
CRS->>Sub: rely on subscription for updates
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
🤖 Fix all issues with AI agents
In `@lib/features/chat/screens/chat_room_screen.dart`:
- Around line 281-282: The alpha value passed to Colors.grey.withValues is out
of range (uses 8), so update the call (Colors.grey.withValues) to use a
floating-point alpha in [0.0,1.0], e.g., alpha: 0.03 to match the "0.03 opacity
- extremely subtle" comment, ensuring the border is rendered with the intended
low opacity rather than fully opaque.
In `@lib/features/restore/restore_manager.dart`:
- Around line 553-557: The log message in restore_manager.dart currently says
'waiting 8 seconds...' but the code uses await Future.delayed(const
Duration(seconds: 10));—update the logger.i call to state "waiting 10 seconds
for historical messages to be saved..." so the log matches the actual delay;
search for the logger.i call near the Future.delayed usage in the restore
manager method and adjust the message string accordingly.
🧹 Nitpick comments (3)
lib/features/chat/screens/chat_room_screen.dart (2)
48-147: Hardcoded user-facing strings should useS.of(context)for localization.Lines 60–62, 76, 84–85, 111–113, 127, 135–136 all contain hardcoded English strings (
'Back','Session not found','Unable to load chat session for this order','Peer information unavailable','Chat partner information could not be loaded'). Per coding guidelines, all user-facing strings must useS.of(context).yourKeywith ARB files.Additionally, the two error scaffolds (Lines 48–96 and 98–147) are nearly identical — they share the same AppBar, layout structure, and styling, differing only in icon, title, and subtitle. Consider extracting a shared
_buildErrorScaffoldhelper to reduce the duplication.♻️ Example: Extract a reusable error scaffold helper
Widget _buildErrorScaffold({ required IconData icon, required String title, required String subtitle, }) { return Scaffold( backgroundColor: AppTheme.backgroundDark, appBar: AppBar( backgroundColor: AppTheme.backgroundDark, elevation: 0, leading: IconButton( icon: const Icon(Icons.arrow_back, color: AppTheme.cream1), onPressed: () => Navigator.of(context).pop(), ), title: Text( S.of(context).back, style: const TextStyle(color: AppTheme.cream1), ), ), body: Center( child: Column( mainAxisAlignment: MainAxisAlignment.center, children: [ Icon(icon, size: 64, color: AppTheme.textSecondary), const SizedBox(height: 16), Text(title, style: TextStyle(color: AppTheme.textPrimary, fontSize: 18, fontWeight: FontWeight.w600)), const SizedBox(height: 8), Text(subtitle, style: TextStyle(color: AppTheme.textSecondary, fontSize: 14), textAlign: TextAlign.center), ], ), ), ); }As per coding guidelines: "Use
S.of(context).yourKeyfor all user-facing strings instead of hardcoded text."
46-46: Consider usingref.watchinstead ofref.readforsessionProviderinsidebuild.Using
ref.readinbuild()means the widget won't rebuild if the session state changes (e.g., if peer data is populated asynchronously after initial navigation). IfsessionProvideris a state provider that can update,ref.watchwould ensure the UI reacts to session changes — particularly relevant for the new null guards that could show/hide the error UI.lib/features/restore/restore_manager.dart (1)
451-452: Unclear comment — "No should be used" is ambiguous.The comment
//No should be used - Default to peer-initiatedis hard to parse. Consider rephrasing for clarity, e.g.,// Should not be used directly — defaults to peer-initiated.
Refines the chat UI's border opacity for better visual appeal. Increases the restore timeout to accommodate slower message delivery, mitigating potential issues during chat restoring. This addresses a problem where historical messages might not be fully restored due to relay delays.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/features/restore/restore_manager.dart (4)
499-547:⚠️ Potential issue | 🟡 MinorCore fix looks correct — peer derivation and early chat subscription.
The logic to determine role/peer by comparing derived trade keys against order pubkeys, then subscribing to chat before historical messages arrive, directly addresses the PR objective (fixing empty chat after restore). The approach is sound: save session first, then subscribe, so the listener is active when relay delivers historical events.
A few observations on this block:
No error handling around
subscribe()(Line 545): Ifsubscribe()throws, it will bubble up and abort the entire restore loop. Consider wrapping in try-catch so a single order's chat subscription failure doesn't prevent remaining orders from being restored.Potential for orphaned subscriptions on restore failure: If the loop fails partway through (e.g., Line 493's exception), orders that already had
subscribe()called will have active chat subscriptions but the restore may ultimately fail and clear state. This could lead to dangling listeners.🛡️ Suggested: guard chat subscription to avoid aborting the loop
if (peer != null) { - ref.read(chatRoomsProvider(orderDetail.id).notifier).subscribe(); - logger.d('Restore: initialized chat listener for order ${orderDetail.id}'); + try { + ref.read(chatRoomsProvider(orderDetail.id).notifier).subscribe(); + logger.d('Restore: initialized chat listener for order ${orderDetail.id}'); + } catch (e) { + logger.w('Restore: failed to initialize chat listener for order ${orderDetail.id}', error: e); + } }
641-641:⚠️ Potential issue | 🟡 MinorTypo: "IMPORTAN" → "IMPORTANT".
- // IMPORTAN : we need to create new message due to synchronization with stored messages + // IMPORTANT: we need to create new message due to synchronization with stored messages
782-800:⚠️ Potential issue | 🟡 MinorError handling in
initRestoreProcess:showError('')passes empty string.Line 785:
showError('')— the empty string means the user sees no meaningful error message. Consider passing a descriptive message or a localization key that the UI can resolve.🐛 Proposed fix
- ref.read(restoreProgressProvider.notifier).showError(''); + ref.read(restoreProgressProvider.notifier).showError('Restore failed. Please try again.');Based on learnings: "The notification system uses a two-layer localization pattern: providers/notifiers call
showCustomMessage()with string keys, and the UI layer maps these keys to localized strings usingS.of(context)." IfshowErrorfollows the same pattern, pass a key instead of a hardcoded string.
148-151:⚠️ Potential issue | 🟠 MajorFix guard condition logic to prevent null pointer exception on
_tempTradeKey.The guard conditions at lines 149, 241, and 303 use
&&, which allows the invalid state where_tempTradeKeyis null while_masterKeyis non-null. Since all three methods force-unwrap_tempTradeKey!unconditionally (lines 163, 255, 317), this will cause a null pointer exception. The condition should use||instead of&&to properly protect the force-unwrap.While current initialization in
initRestoreProcess()(lines 708, 719) guarantees both are set before these methods are called, the guard logic itself is insufficient and should be corrected for defensive programming.
🧹 Nitpick comments (4)
lib/features/restore/restore_manager.dart (2)
504-525: Role/peer derivation defaults tonullrole when neither pubkey matches.When the user's derived pubkey matches neither
buyerTradePubkeynorsellerTradePubkey(Lines 522–525),rolestaysnulland the session is still saved. Downstream,_getActionFromStatus(Line 637) receives anullrole, which works for some statuses but may produce incorrect actions for role-dependent ones (e.g.,waitingBuyerInvoice,waitingPayment,active). The warning log is good, but consider whether anullrole session should be saved at all — or at least document the expected behavior.
451-452: Clarify the comment on the dispute default action.The comment
//No should be used - Default to peer-initiatedis unclear. Consider rewording:- return Action - .disputeInitiatedByPeer; //No should be used - Default to peer-initiated + return Action + .disputeInitiatedByPeer; // Default; overridden in restore() when dispute data is availablelib/features/chat/screens/chat_room_screen.dart (2)
49-96: Hardcoded user-facing strings should useS.of(context)for localization.Lines 76, 85, 127, 136 contain hardcoded English strings:
'Session not found','Unable to load chat session for this order','Peer information unavailable','Chat partner information could not be loaded'. These should be localized via ARB files.Also, the two error screens share nearly identical structure (Scaffold + AppBar + centered Column with icon/text). Consider extracting a shared error widget to reduce duplication.
As per coding guidelines: "Use
S.of(context).yourKeyfor all user-facing strings instead of hardcoded text."Also applies to: 98-147
46-46:ref.readfor session won't rebuild when the session updates.Line 46 uses
ref.read(sessionProvider(widget.orderId))insidebuild(). If the session is updated (e.g., peer becomes available after restore completes), this widget won't rebuild to reflect the change. Consider usingref.watchso the screen reactively transitions from the error state to the chat UI when session/peer data becomes available.♻️ Proposed fix
- final session = ref.read(sessionProvider(widget.orderId)); + final session = ref.watch(sessionProvider(widget.orderId));
Addresses the issue of duplicated error screens in the chat feature. This commit introduces a `ChatErrorScreen` widget to handle displaying error messages related to missing sessions or unavailable peers. Replaces the inline error screen implementations with calls to the new `ChatErrorScreen`, improving code maintainability and reducing redundancy. Adds localization strings for the error messages.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/chat/screens/chat_room_screen.dart (1)
102-105:⚠️ Potential issue | 🟡 MinorHardcoded
'Back'string — use localization for consistency.The new
ChatErrorScreencorrectly usesS.of(context)!.backfor the same AppBar title. This screen should follow suit.Proposed fix
- title: const Text( - 'Back', - style: TextStyle(color: AppTheme.cream1), + title: Text( + S.of(context)!.back, + style: const TextStyle(color: AppTheme.cream1), ),As per coding guidelines: "Use
S.of(context).yourKeyfor all user-facing strings instead of hardcoded text".
🧹 Nitpick comments (1)
lib/features/chat/widgets/chat_error_screen.dart (1)
63-75: AddconsttoTextStyleinstances.Both
TextStyleconstructors use only compile-time constants, so they can beconstfor widget optimization.Proposed fix
Text( title, - style: TextStyle( + style: const TextStyle( color: AppTheme.textPrimary, fontSize: 18, fontWeight: FontWeight.w600, ), ), const SizedBox(height: 8), Text( subtitle, - style: TextStyle( + style: const TextStyle( color: AppTheme.textSecondary, fontSize: 14, ),
|
Ready for review ✅ |
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
@BraCR10 cuando recupero el usuario ya no sale el error al abrir el chat desde la pantalla de Order details, sin embargo en la pantalla de chat no sale ese chat nunca, aunque tenga mensajes, solo lista chats antiguos de ordenes que ya estan eliminadas
fijate en este caso que la contrapartre se llama loose-monkey
Addresses issues where chat data was not properly cleared during restore, leading to potential inconsistencies. This commit ensures that all chat-related providers are invalidated and cleared during the restore process. It also prevents loading chats with no messages during initial load and refresh.
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 (2)
lib/features/restore/restore_manager.dart (1)
656-656:⚠️ Potential issue | 🟡 MinorTypo: "IMPORTAN" → "IMPORTANT".
- // IMPORTAN : we need to create new message due to synchronization with stored messages + // IMPORTANT: we need to create new message due to synchronization with stored messageslib/features/chat/notifiers/chat_rooms_notifier.dart (1)
64-70:⚠️ Potential issue | 🟠 MajorUse the full session state instead of the
.sessionsgetter to include all session types.Line 66 reads
ref.read(sessionNotifierProvider.notifier).sessions, which returns only orderId-based sessions from the_sessionsmap. However, the provider's state combines three session sources:_sessions,_requestIdToSession, and_pendingChildSessions. This causesrefreshChatList()to miss sessions tracked by request ID and pending child sessions, creating an inconsistency withloadChats()at line 34, which correctly reads the full state viaref.read(sessionNotifierProvider).Change line 66 to:
final sessions = ref.read(sessionNotifierProvider);
🤖 Fix all issues with AI agents
In `@lib/features/restore/restore_manager.dart`:
- Around line 466-467: Replace the unclear inline comment after the return of
Action.disputeInitiatedByPeer with a concise, clear sentence; for example change
"//No should be used - Default to peer-initiated" to something like "// Default
to peer-initiated; use 'No' to indicate the dispute was not initiated locally"
so readers understand the intended meaning around the 'No' value and the default
behavior for Action.disputeInitiatedByPeer.
- Around line 798-800: The call to
ref.read(restoreProgressProvider.notifier).showError('') passes an empty string
so users see no error details; update the restore failure path (where
logger.e(...) is called) to pass a meaningful message or localization key to
showError — for example build a user-facing message using the caught error
(e.toString() or a sanitized message) or a localized string identifier and
include any useful context (e.g., "restoreFailed" or "${loc.restoreFailed}:
${e.toString()}") so the UI displays an actionable error instead of blank text;
ensure you reference the same notifier (restoreProgressProvider.notifier) and
keep logger.e(error: e, stackTrace: stack) unchanged.
🧹 Nitpick comments (1)
lib/features/chat/notifiers/chat_rooms_notifier.dart (1)
53-55: Filtering out empty-message chats could hide legitimate chats during initial load.The
chat.messages.isNotEmptyfilter runs synchronously, butChatRoomNotifier.initialize()loads historical messages asynchronously. IfloadChats()orrefreshChatList()runs before historical messages finish loading (e.g., during restore), valid chats with messages in storage will be filtered out until the next refresh.This seems intentional for the restore fix (avoid showing empty shells), and the 10-second delay in
restore_manager.dartplus therefreshChatList()call from_onChatEventshould eventually surface them. Just be aware this creates an ordering dependency — if messages load slower than expected, chats won't appear until the next event triggers a refresh.Also applies to: 88-90
| return Action | ||
| .disputeInitiatedByPeer; //No should be used - Default to peer-initiated |
There was a problem hiding this comment.
Comment is unclear: "No should be used" — rephrase.
The comment //No should be used - Default to peer-initiated is grammatically unclear. Consider rephrasing.
- return Action
- .disputeInitiatedByPeer; //No should be used - Default to peer-initiated
+ return Action
+ .disputeInitiatedByPeer; // Should not be used directly — default to peer-initiated📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return Action | |
| .disputeInitiatedByPeer; //No should be used - Default to peer-initiated | |
| return Action | |
| .disputeInitiatedByPeer; // Should not be used directly — default to peer-initiated |
🤖 Prompt for AI Agents
In `@lib/features/restore/restore_manager.dart` around lines 466 - 467, Replace
the unclear inline comment after the return of Action.disputeInitiatedByPeer
with a concise, clear sentence; for example change "//No should be used -
Default to peer-initiated" to something like "// Default to peer-initiated; use
'No' to indicate the dispute was not initiated locally" so readers understand
the intended meaning around the 'No' value and the default behavior for
Action.disputeInitiatedByPeer.
There was a problem hiding this comment.
@BraCR10, understood — I'll skip this one. Feel free to reach out if you need anything else! 👍
|
Hola @Catrya, ¡gracias por el feedback! Revisé el flujo normal en la rama
En este commit: La idea principal es que, al hacer restore, solo se creen las suscripciones a los chats correspondientes a las
Por mi parte lo he probando con múltiples ordenes y funciona sin presentar problemas. |
mostronator
left a comment
mostronator
left a comment
There was a problem hiding this comment.
✅ Approved — Critical fix for #372 (High Priority)
Problem solved
Chat messages were empty after first-time restore on a new device due to missing chat subscription initialization and Peer object creation during restore.
Review
Strengths:
- Root cause addressed: Initializes chat subscription immediately after saving session during restore — this was the missing piece
- Proper Peer creation: Creates
Peerobjects during restore process so chat screens can render correctly - Better error handling: New
ChatErrorScreenwidget with factory constructors forsessionNotFoundandpeerUnavailable— clean pattern - Cleanup of legacy code: Removed fallback query in
ChatRoomNotifierthat searched all chat events when order_id filter returned nothing — this was masking bugs - Chat list filtering: Only showing chats with messages (
.where((chat) => chat.messages.isNotEmpty)) prevents the UI glitch from #379 - Complete i18n: Error screen strings in all 3 languages
Minor observations (non-blocking):
- The
Future.delayed(Duration(milliseconds: 100))inrefreshChatListis a code smell — consider using a proper event/callback mechanism in the future _clearAll()now clears chat providers before storage — good ordering
This fixes a high-priority bug with a solid approach. Well done! 🎉
|
@BraCR10 please fix conflicts |
There was a problem hiding this comment.
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)
656-656:⚠️ Potential issue | 🟡 MinorTypo: "IMPORTAN" → "IMPORTANT".
- // IMPORTAN : we need to create new message due to synchronization with stored messages + // IMPORTANT: we need to create new message due to synchronization with stored messages
|
@grunch Conflicts fixed! |
4 participants
This PR fixes empty chat messages after first-time restore on a new device.
Closes:#372
What's Included
Peerobject creation in restore processTesting
Summary by CodeRabbit
New Features
Bug Fixes