fix: resolve chat history loss after reconnection or app restart#538
fix: resolve chat history loss after reconnection or app restart#538
Conversation
- reload() now calls _loadHistoricalMessages() before re-subscribing - sendMessage() persists wrapped event to disk after successful publish - reloadAllChats() awaits all reloads before refreshing the chat list
|
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:
WalkthroughChat reloads were converted to async and are now awaited; per-chat reloads load history before subscribing. Sent wrapped events are persisted to local storage immediately after publish success and before optimistic in-memory updates. Foreground transitions await full chat reloads. Changes
Sequence Diagram(s)sequenceDiagram
participant Lifecycle as App Lifecycle
participant ChatRooms as ChatRoomsNotifier
participant ChatRoom as ChatRoomNotifier
participant Relay as Relay
participant Storage as EventStorage
participant UI as UI
Lifecycle->>ChatRooms: reloadAllChats() [await]
ChatRooms->>ChatRoom: reload() for each chat
par Parallel reloads
ChatRoom->>Relay: cancel subscription
ChatRoom->>ChatRoom: _loadHistoricalMessages()
ChatRoom->>Relay: resubscribe
end
ChatRooms-->>Lifecycle: all reloads complete
ChatRooms->>ChatRooms: refreshChatList() [await]
Note over ChatRoom,Storage: outgoing message publish flow
ChatRoom->>Relay: publish(wrapped event)
Relay-->>ChatRoom: publish success
ChatRoom->>Storage: persist wrapped event (disk)
ChatRoom->>UI: optimistic in-memory update
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)
📝 Coding Plan
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.
🧹 Nitpick comments (2)
lib/features/chat/notifiers/chat_rooms_notifier.dart (1)
18-39: Good async coordination - addresses the race condition correctly.The parallel reload with
Future.wait()followed by awaitingrefreshChatList()properly sequences the operations. This ensures all messages are loaded before the chat list filters chats without messages.Note: The retrieved learning stating "
reload()method... is synchronous" is now outdated—this PR intentionally changesreload()to async. The learning reflected old behavior.One minor robustness suggestion: the try-catch at lines 25-32 only catches synchronous errors. If
reload()throws asynchronously,Future.wait()will fail-fast. Since_loadHistoricalMessages()catches its own errors internally, this is low risk, but wrapping each future could improve resilience:♻️ Optional: wrap each reload for guaranteed completion
final futures = <Future<void>>[]; for (final chat in state) { try { final notifier = ref.read(chatRoomsProvider(chat.orderId).notifier); if (notifier.mounted) { - futures.add(notifier.reload()); + futures.add( + notifier.reload().catchError((e) { + logger.e('Async reload failed for orderId ${chat.orderId}: $e'); + }), + ); } } catch (e) { logger.e('Failed to reload chat for orderId ${chat.orderId}: $e'); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/chat/notifiers/chat_rooms_notifier.dart` around lines 18 - 39, The try-catch only covers synchronous errors when building futures, so if notifier.reload() rejects later Future.wait will fail-fast; to make reloadAllChats resilient, wrap each notifier.reload() call into a future that handles its own errors (e.g., notifier.reload().catchError(...) or using .then(...).catchError(...)) before adding it to the futures list, referencing the reloadAllChats method and the notifier.reload() calls; ensure the wrapper logs failures (using logger.e with orderId) and returns a completed Future so Future.wait waits for all wrapped tasks without short-circuiting.lib/features/chat/notifiers/chat_room_notifier.dart (1)
217-237: Good persistence strategy for message durability.Persisting immediately after successful publish (before optimistic UI update) ensures the message survives app crashes or connection drops. The
hasItem()check in_onChatEvent()correctly deduplicates when the relay echo arrives.Minor observation: this persistence code is inside the inner try-catch (lines 214-254), so storage failures will be logged as "Failed to publish message" which could be misleading during debugging. Consider narrowing the try scope or adding separate error handling for storage:
♻️ Optional: clearer error attribution
await ref.read(nostrServiceProvider).publishEvent(wrappedEvent); logger.d('Message sent successfully to network'); - - // Persist the wrapped event to disk immediately after successful publish. - // This prevents message loss if the relay echo doesn't arrive - // (e.g., connection drop after send). _onChatEvent will skip it via hasItem(). - final eventStore = ref.read(eventStorageProvider); - await eventStore.putItem( - wrappedEvent.id!, - { - 'id': wrappedEvent.id, - 'created_at': - wrappedEvent.createdAt!.millisecondsSinceEpoch ~/ 1000, - 'kind': wrappedEvent.kind, - 'content': wrappedEvent.content, - 'pubkey': wrappedEvent.pubkey, - 'sig': wrappedEvent.sig, - 'tags': wrappedEvent.tags, - 'type': 'chat', - 'order_id': orderId, - }, - ); - logger.d('Wrapped event persisted to storage for orderId: $orderId'); } catch (publishError, publishStack) { logger.e('Failed to publish message: $publishError', stackTrace: publishStack); rethrow; } + // Persist the wrapped event to disk immediately after successful publish. + // This prevents message loss if the relay echo doesn't arrive. + try { + final eventStore = ref.read(eventStorageProvider); + await eventStore.putItem( + wrappedEvent.id!, + { + 'id': wrappedEvent.id, + 'created_at': + wrappedEvent.createdAt!.millisecondsSinceEpoch ~/ 1000, + 'kind': wrappedEvent.kind, + 'content': wrappedEvent.content, + 'pubkey': wrappedEvent.pubkey, + 'sig': wrappedEvent.sig, + 'tags': wrappedEvent.tags, + 'type': 'chat', + 'order_id': orderId, + }, + ); + logger.d('Wrapped event persisted to storage for orderId: $orderId'); + } catch (storageError) { + logger.e('Failed to persist message to storage: $storageError'); + // Continue - message was published, just won't survive crash + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/chat/notifiers/chat_room_notifier.dart` around lines 217 - 237, The persistence of wrappedEvent via eventStore.putItem is inside the same try-catch that wraps publishing, so storage failures get reported as publish failures; move the persistence out of the publish try or add a distinct try/catch around eventStore.putItem (and log a clear storage-specific error via logger.e) so failures in eventStore.putItem are attributed to storage (while keeping the existing hasItem() dedupe in _onChatEvent). Ensure you reference wrappedEvent.id, eventStore.putItem and the existing logger calls when adding the narrower scope or new catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/features/chat/notifiers/chat_room_notifier.dart`:
- Around line 217-237: The persistence of wrappedEvent via eventStore.putItem is
inside the same try-catch that wraps publishing, so storage failures get
reported as publish failures; move the persistence out of the publish try or add
a distinct try/catch around eventStore.putItem (and log a clear storage-specific
error via logger.e) so failures in eventStore.putItem are attributed to storage
(while keeping the existing hasItem() dedupe in _onChatEvent). Ensure you
reference wrappedEvent.id, eventStore.putItem and the existing logger calls when
adding the narrower scope or new catch.
In `@lib/features/chat/notifiers/chat_rooms_notifier.dart`:
- Around line 18-39: The try-catch only covers synchronous errors when building
futures, so if notifier.reload() rejects later Future.wait will fail-fast; to
make reloadAllChats resilient, wrap each notifier.reload() call into a future
that handles its own errors (e.g., notifier.reload().catchError(...) or using
.then(...).catchError(...)) before adding it to the futures list, referencing
the reloadAllChats method and the notifier.reload() calls; ensure the wrapper
logs failures (using logger.e with orderId) and returns a completed Future so
Future.wait waits for all wrapped tasks without short-circuiting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50f39cf5-1399-40f2-8b23-37f6998cd89c
📒 Files selected for processing (3)
lib/features/chat/notifiers/chat_room_notifier.dartlib/features/chat/notifiers/chat_rooms_notifier.dartlib/services/lifecycle_manager.dart
- Wrap each chat reload with catchError to prevent one failure from blocking others - Separate publish and storage try-catch so persist failures don't block optimistic UI
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 (2)
lib/features/chat/notifiers/chat_room_notifier.dart (2)
288-359:⚠️ Potential issue | 🟠 MajorTrim the history-loader diagnostics before release.
This block logs shared keys, event IDs and raw
unwrappedMessage.content, and it does a fullfind(type == 'chat')just to print a count. WithreloadAllChats()awaiting every room, that turns foreground reloads into repeated full-store scans and leaks private chat data into logs.Possible cleanup
- logger.i('Session found with shared key: ${session.sharedKey?.public}'); + logger.d('Loading historical messages for orderId: $orderId'); - final allChatEvents = await eventStore.find( - filter: eventStore.eq('type', 'chat'), - ); - logger.i('Total chat events in storage: ${allChatEvents.length}'); - logger.i('Processing event $i: ${eventData['id']}'); + logger.d('Processing historical event $i for orderId: $orderId'); - logger.i('Event data keys: ${eventData.keys.toList()}'); + logger.d('Historical event payload loaded for orderId: $orderId'); - logger.i( - 'Reconstructed event: ${storedEvent.id}, recipient: ${storedEvent.recipient}'); + logger.d('Reconstructed historical event ${storedEvent.id}'); - logger.i( - 'Successfully unwrapped message: ${unwrappedMessage.content}'); + logger.d( + 'Successfully unwrapped historical message ${unwrappedMessage.id}');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/chat/notifiers/chat_room_notifier.dart` around lines 288 - 359, The code logs sensitive data and does an expensive full-store scan: remove or redact logs that expose session.sharedKey?.public, eventData['id'], storedEvent.recipient and unwrappedMessage.content and stop the unnecessary all-chat query; instead call eventStore.find only with the order-specific filter (remove the allChatEvents query), replace detailed values in logger.i/w with non-sensitive summaries (e.g., counts or masked IDs), and ensure NostrEventExtensions.fromMap and storedEvent.p2pUnwrap are still used but without logging their raw outputs; keep historicalMessages population intact but do not log decrypted content or full keys.
117-156:⚠️ Potential issue | 🔴 CriticalDon't claim the event until this notifier proves it owns it.
Every
ChatRoomNotifierlistens tosubscriptionManager.chat, buthasItem()/putItem()run before thep-tag/shared-key check. The first room that sees a kind1059event can persist it under its ownorderId, and the real room then exits onhasItem(event.id!), which drops or misfiles messages across chats.Minimal fix
- // Check if event is already processed to prevent duplicate notifications - final eventStore = ref.read(eventStorageProvider); - if (await eventStore.hasItem(event.id!)) { - return; - } - - // Store the complete event to prevent future duplicates and enable historical loading - await eventStore.putItem( - event.id!, - { - 'id': event.id, - 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, - 'kind': event.kind, - 'content': event.content, - 'pubkey': event.pubkey, - 'sig': event.sig, - 'tags': event.tags, - 'type': 'chat', - 'order_id': orderId, - }, - ); - final session = ref.read(sessionProvider(orderId)); if (session == null || session.sharedKey == null) { logger.e('Session or shared key is null when processing chat event'); return; } @@ if (pTag.isEmpty || pTag.length < 2 || pTag[1] != session.sharedKey!.public) { logger.w('Event not addressed to our shared key, ignoring'); return; } + + final eventStore = ref.read(eventStorageProvider); + if (await eventStore.hasItem(event.id!)) { + return; + } + + await eventStore.putItem( + event.id!, + { + 'id': event.id, + 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, + 'kind': event.kind, + 'content': event.content, + 'pubkey': event.pubkey, + 'sig': event.sig, + 'tags': event.tags, + 'type': 'chat', + 'order_id': orderId, + }, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/chat/notifiers/chat_room_notifier.dart` around lines 117 - 156, The notifier is claiming events (eventStore.hasItem/putItem) before verifying the event is addressed to this room; move the session and p-tag/shared-key validation (use sessionProvider(orderId), session.sharedKey, and pTag check) before calling eventStore.hasItem and eventStore.putItem so the ChatRoomNotifier only stores/marks events it actually owns (check pTag where tag[0]=='p' and compare tag[1] to session.sharedKey!.public, then perform hasItem/putItem).
🤖 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/chat/notifiers/chat_room_notifier.dart`:
- Around line 31-34: _reloadHistoricalMessages() currently returns early if
session/shared key is null and reload() (and similar paths) calls
_loadHistoricalMessages() before subscribing to session readiness via
_listenForSession()/subscribe(), so historical messages can be missed; change
reload(), and the other affected methods that call _loadHistoricalMessages()
(and any restore/foreground handlers) to either await session readiness before
calling _loadHistoricalMessages() or attach a one-shot retry that calls
_loadHistoricalMessages() when the session/shared key becomes non-null (use the
existing _listenForSession()/subscribe() flow to trigger the retry), ensuring
you still cancel previous subscriptions via _subscription?.cancel() and avoid
double-loading.
---
Outside diff comments:
In `@lib/features/chat/notifiers/chat_room_notifier.dart`:
- Around line 288-359: The code logs sensitive data and does an expensive
full-store scan: remove or redact logs that expose session.sharedKey?.public,
eventData['id'], storedEvent.recipient and unwrappedMessage.content and stop the
unnecessary all-chat query; instead call eventStore.find only with the
order-specific filter (remove the allChatEvents query), replace detailed values
in logger.i/w with non-sensitive summaries (e.g., counts or masked IDs), and
ensure NostrEventExtensions.fromMap and storedEvent.p2pUnwrap are still used but
without logging their raw outputs; keep historicalMessages population intact but
do not log decrypted content or full keys.
- Around line 117-156: The notifier is claiming events
(eventStore.hasItem/putItem) before verifying the event is addressed to this
room; move the session and p-tag/shared-key validation (use
sessionProvider(orderId), session.sharedKey, and pTag check) before calling
eventStore.hasItem and eventStore.putItem so the ChatRoomNotifier only
stores/marks events it actually owns (check pTag where tag[0]=='p' and compare
tag[1] to session.sharedKey!.public, then perform hasItem/putItem).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38a2eeae-d366-4e20-8ef9-23997fe31c5c
📒 Files selected for processing (2)
lib/features/chat/notifiers/chat_room_notifier.dartlib/features/chat/notifiers/chat_rooms_notifier.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/chat/notifiers/chat_rooms_notifier.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/P2P_CHAT_SYSTEM.md`:
- Around line 25-44: The markdown has unlabeled fenced code blocks (e.g., the
Relay/Chat flow block, the "User types message" block, the numbered init steps
block, and the Sembast query block referenced near
ChatRoomNotifier._onChatEvent) which triggers markdownlint MD040; update each
triple-backtick fence to include a language specifier (use `text` or a specific
language token as appropriate) for every code block in this file so the blocks
around the Relay flow, the user message flow, the initialization list, and the
Sembast query are all labeled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e9da0c0-a5b7-4610-87f6-3b8bbe69b8f1
📒 Files selected for processing (1)
docs/architecture/P2P_CHAT_SYSTEM.md
- Move session and p-tag validation ahead of hasItem/putItem in _onChatEvent
- Prevents broadcast stream race condition where multiple ChatRoomNotifier
instances store the same event with different order_id values
- Only the notifier whose sharedKey matches the event's p-tag writes to disk
- Fixes root cause of messages disappearing after app restart with 2+ active chats
- chatRoomsProvider already calls subscribe() via _initializeChatRoomSafely
- The explicit subscribe() call created a second listener per chat on the
broadcast stream, doubling disk write contention during the race condition
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/features/chat/notifiers/chat_room_notifier.dart (2)
30-35: Await_subscription?.cancel()to avoid interleaved events during reload.
StreamSubscription.cancel()returns aFuture. Without awaiting it, events from the old subscription could still arrive while_loadHistoricalMessages()is running, potentially causing duplicate processing or state inconsistencies.Suggested fix
Future<void> reload() async { - _subscription?.cancel(); + await _subscription?.cancel(); await _loadHistoricalMessages(); subscribe(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/chat/notifiers/chat_room_notifier.dart` around lines 30 - 35, The reload() method currently calls _subscription?.cancel() without awaiting it, which can allow events from the old subscription to arrive during _loadHistoricalMessages(); change reload() to await the cancel Future (await _subscription?.cancel();) before calling await _loadHistoricalMessages() and subscribe() so the old stream is fully torn down prior to reloading and re-subscribing (refer to reload(), _subscription, _loadHistoricalMessages(), and subscribe()).
217-223: Therethrowis caught locally and has no external effect.The inner
rethrowat line 222 is immediately caught by the outercatchblock at line 269, which only logs the error. Callers ofsendMessage()won't be notified of publish failures. If this is intentional (fire-and-forget semantics), consider removing the rethrow to avoid confusion. If callers should handle failures, propagate the error.Option A: Remove misleading rethrow (fire-and-forget)
try { await ref.read(nostrServiceProvider).publishEvent(wrappedEvent); logger.d('Message sent successfully to network'); } catch (publishError, publishStack) { logger.e('Failed to publish message: $publishError', stackTrace: publishStack); - rethrow; // Re-throw to be caught by outer catch + return; // Exit early on publish failure }Option B: Propagate error to callers
- Future<void> sendMessage(String text) async { + /// Sends a message. Throws on publish failure. + Future<void> sendMessage(String text) async { ... } catch (e, stackTrace) { logger.e('Failed to send message: $e', stackTrace: stackTrace); + rethrow; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/chat/notifiers/chat_room_notifier.dart` around lines 217 - 223, The inner catch in sendMessage catches publishError/publishStack and rethrows, but the outer catch only logs and swallows the error, so callers never see publish failures; to propagate failures, either remove the outer catch or rethrow after logging there (i.e., in the outer catch around sendMessage add a `rethrow` after `logger.e(...)`), ensuring publishError/publishStack (from the inner catch) bubble to callers; reference sendMessage, the inner catch (publishError/publishStack) and the outer catch that currently only logs so you can locate and update the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/features/chat/notifiers/chat_room_notifier.dart`:
- Around line 30-35: The reload() method currently calls _subscription?.cancel()
without awaiting it, which can allow events from the old subscription to arrive
during _loadHistoricalMessages(); change reload() to await the cancel Future
(await _subscription?.cancel();) before calling await _loadHistoricalMessages()
and subscribe() so the old stream is fully torn down prior to reloading and
re-subscribing (refer to reload(), _subscription, _loadHistoricalMessages(), and
subscribe()).
- Around line 217-223: The inner catch in sendMessage catches
publishError/publishStack and rethrows, but the outer catch only logs and
swallows the error, so callers never see publish failures; to propagate
failures, either remove the outer catch or rethrow after logging there (i.e., in
the outer catch around sendMessage add a `rethrow` after `logger.e(...)`),
ensuring publishError/publishStack (from the inner catch) bubble to callers;
reference sendMessage, the inner catch (publishError/publishStack) and the outer
catch that currently only logs so you can locate and update the behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8ecf83b-f63c-47d2-8dcb-a27c6435e4da
📒 Files selected for processing (1)
lib/features/chat/notifiers/chat_room_notifier.dart
- ChatRoomNotifier.initialize() now calls refreshChatList() after loading messages, so chats are not permanently filtered out by loadChats() which runs before the async init finishes - reloadAllChats() iterates over sessions instead of state, which may be empty if the initial loadChats() filtered everything out
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Architecture
Good fix for chat history persistence. The changes correctly address the race conditions identified in #536:
reload()now loads history first — prevents empty chat state on foreground resumesendMessage()persists immediately — prevents message loss if relay echo never arrivesreloadAllChats()awaits completions — eliminates race where chats appear empty- Early ownership check in
_onChatEvent()— prevents storing events under wrong orderId
CI Status
✅ Build passing
Documentation
Excellent addition of docs/architecture/P2P_CHAT_SYSTEM.md — this will help future contributors understand the message flow.
CodeRabbit Issues
| Issue | Status | Notes |
|---|---|---|
| Retry historical load when session ready | ✅ Resolved | Code already has await _loadHistoricalMessages() in session callback |
| MD040: code blocks without language | ❌ Not fixed | Lines 25, 67, 164, 202 need language specifier (e.g., text) |
Code Review Notes
-
app_init_provider.dartchange: Changed from.subscribe()to just reading the provider. This relies on the provider initialization to trigger subscription. Verify this works correctly on cold start. -
unawaited()in_listenForSession(): Usingunawaited()with an async IIFE is correct here — avoids blocking the listener callback while still ensuring proper async sequencing. -
refreshChatList()after init: Good defensive call to ensure chat appears in list even ifloadChats()ran before async init completed.
Required Fix
Add language specifier to code blocks in P2P_CHAT_SYSTEM.md per AGENTS.md guidelines:
-```
+```text
Relay
│ kind 1059 gift wrap events (encrypted)Apply to all unlabeled blocks (lines 25, 67, 164, 202).
Verdict
Request Changes — The logic is correct, but the markdown lint issue (MD040) must be fixed per repo guidelines before merge.
BraCR10
left a comment
BraCR10
left a comment
There was a problem hiding this comment.
Hi @Catrya,
While testing the PR, I found an issue, could you check it?:
- I started a chat with some messages.
- I turned off the Wi-Fi on my phone.
- I waited a few seconds and turned it back on.
- Even though the app was able to send messages and the other phone received them, the phone that lost the connection no longer received messages.
After restoring the orders, the chat worked properly again.
@BraCR10 Thanks for the review, I really need to find these kinds of cases where it's still failing. I'll keep working on it. If you find any others, let me know. |
|
Hi @BraCR10, I can't reproduce that problem, I tested it with 2 phones, but I receive the messages as soon as the internet comes back on, can you provide more details? |
|
Hi @Catrya, I tested it again with three new orders. The original issue has been fixed,chats now persist even when the signal is lost. Previously, I reported that I was not receiving messages after re-establishing the connection. However, after re-testing, it seems that messages are actually delayed. They start arriving about 10–15 seconds after the connection is restored, although sometimes they arrive immediately. |
There was a problem hiding this comment.
Previous issue resolved ✅
The MD040 issue (code blocks without language specifier) has been fixed in commit 6c0c765b. All code blocks now have language specifiers (text or dart).
Review Summary
Architecture ✅
Correct fix for chat history persistence race conditions:
reload()loads history first — prevents empty chat on foreground resumesendMessage()persists immediately — prevents loss if relay echo never arrivesreloadAllChats()awaits completions — eliminates race where chats appear empty- Early ownership check — prevents storing events under wrong orderId
Key Changes
| File | Change |
|---|---|
chat_room_notifier.dart |
reload() now async, loads history before subscribe |
chat_rooms_notifier.dart |
reloadAllChats() awaits all reloads in parallel |
lifecycle_manager.dart |
Awaits reloadAllChats() on foreground resume |
app_init_provider.dart |
Simplified: just reads provider (triggers init internally) |
Documentation ✅
Excellent addition of P2P_CHAT_SYSTEM.md — comprehensive architecture doc with message flow diagrams.
CI ✅
Build passing.
Summary
LGTM. The fix correctly addresses the race conditions causing chat history loss.
3 participants
fix #536
reload()now calls_loadHistoricalMessages()before re-subscribing, so messages stored while the app was in background are loaded into state on foreground resumesendMessage()persists the wrapped event to disk immediately after successful publish, preventing message loss if the relay echo never arrivesreloadAllChats()awaits all parallelreload()completions before callingrefreshChatList(), eliminating the race condition where chats appear empty and get filtered outTest:
-Background the app for several minutes while messages arrive, resume — verify all messages appear without missing any
Summary by CodeRabbit
Bug Fixes
Documentation