fix: Prevent corrupting restored order state on relaunch#589
Conversation
…restore During account restoration, live direct messages (DMs) were being written to local storage. This could interfere with the restoration process, potentially leading to an inconsistent or corrupted state where restored data is overwritten by new events, causing issues upon app relaunch. This change prevents new DMs from being saved while a restoration is active, ensuring the integrity of the restored account data.
|
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:
WalkthroughRestore mode now limits orders backfill, buffers live relay events in ChangesRestore-mode message handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/services/mostro_service.dart (1)
115-118:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLive events arriving during restore are permanently dropped from
mostroStorageThe event ID is committed to
eventStoreunconditionally at lines 115–118, before the restore guard at lines 166–169. WhenisRestoringProvideristrue, the guard returns early without writing tomostroStorage. After restore completes andisRestoringProvidertransitions tofalse, theeventStoreretains those cached IDs indefinitely. If the relay replays or resends those events,eventStore.hasItem(event.id!)at line 112 returnstrueand the handler exits before reaching the guard — the message is never stored, regardless of relay retries.Any genuinely live event (e.g. a counterparty payment confirmation arriving while restore was running) is silently and permanently excluded from
mostroStorage. The restore process does not invalidateeventStorageProvideror trigger a re-subscription withsincefilters to recover missed events.A minimal mitigation is to defer the event ID write until after the restore guard, so the event remains eligible for normal processing once
isRestoringProvidertransitions tofalse:🔧 Proposed fix
Future<void> _onData(NostrEvent event) async { final eventStore = ref.read(eventStorageProvider); if (await eventStore.hasItem(event.id!)) return; - // Reserve event ID immediately to prevent duplicate processing from multiple relays - await eventStore.putItem(event.id!, { - 'id': event.id, - 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, - }); // ... (decryption, JSON parsing, restore-payload filters) ... + final isRestoring = ref.read(isRestoringProvider); + + if (isRestoring) { + logger.i('Restore in progress, skipping storage write for ${msg.action}'); + return; + } + + // Reserve event ID to prevent duplicate processing from multiple relays. + // Deferred until after restore guard to allow post-restore reprocessing. + await eventStore.putItem(event.id!, { + 'id': event.id, + 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, + }); - if (ref.read(isRestoringProvider)) { - logger.i('Restore in progress, skipping storage write for ${msg.action}'); - return; - } await messageStorage.addMessage(messageKey, msg);This ensures that events received during restore are not marked as processed in
eventStore, allowing them to be correctly stored when they arrive again post-restore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/services/mostro_service.dart` around lines 115 - 118, The event ID is being committed to eventStore via eventStore.putItem before the isRestoringProvider guard, causing live events received during restore to be marked processed and permanently skipped; move the eventStore.putItem call so it runs only after the isRestoringProvider check and after the event has been successfully written to mostroStorage (i.e., only write the ID in the branch where you call mostroStorage write/success), ensuring eventStore.hasItem(event.id!) remains false during restore and the event can be processed/replayed later once isRestoringProvider is false.
🧹 Nitpick comments (1)
lib/services/mostro_service.dart (1)
156-169: 💤 Low valueMinor inefficiency: JSON parsing before the restore guard
MostroMessage.fromJson(result[0])at line 156 is invoked before theisRestoringProvidercheck at line 166. During high-volume restore replay, every incoming event pays the full deserialization cost even though the result is discarded. Moving the guard to just after the restore-payload filter (line 154) avoids this work.♻️ Proposed refactor
+ final isRestoring = ref.read(isRestoringProvider); + if (isRestoring) { + logger.i('Restore in progress, skipping storage write'); + return; + } + final msg = MostroMessage.fromJson(result[0]); final messageStorage = ref.read(mostroStorageProvider); final messageKey = decryptedEvent.id ?? event.id ?? 'msg_${DateTime.now().millisecondsSinceEpoch}'; - if (ref.read(isRestoringProvider)) { - logger.i('Restore in progress, skipping storage write for ${msg.action}'); - return; - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/services/mostro_service.dart` around lines 156 - 169, Move the isRestoringProvider guard so we skip expensive JSON deserialization when restoring: check ref.read(isRestoringProvider) (and the existing payload filter at the top of the handler) before calling MostroMessage.fromJson(result[0]). In practice, relocate the restore-check above the current MostroMessage.fromJson call and only construct msg and compute messageKey (using decryptedEvent.id/event) after the guard passes; preserve the existing logger.i('Restore in progress...') early-return behavior and keep references to ref.read(mostroStorageProvider), decryptedEvent, event, and messageKey intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/services/mostro_service.dart`:
- Around line 115-118: The event ID is being committed to eventStore via
eventStore.putItem before the isRestoringProvider guard, causing live events
received during restore to be marked processed and permanently skipped; move the
eventStore.putItem call so it runs only after the isRestoringProvider check and
after the event has been successfully written to mostroStorage (i.e., only write
the ID in the branch where you call mostroStorage write/success), ensuring
eventStore.hasItem(event.id!) remains false during restore and the event can be
processed/replayed later once isRestoringProvider is false.
---
Nitpick comments:
In `@lib/services/mostro_service.dart`:
- Around line 156-169: Move the isRestoringProvider guard so we skip expensive
JSON deserialization when restoring: check ref.read(isRestoringProvider) (and
the existing payload filter at the top of the handler) before calling
MostroMessage.fromJson(result[0]). In practice, relocate the restore-check above
the current MostroMessage.fromJson call and only construct msg and compute
messageKey (using decryptedEvent.id/event) after the guard passes; preserve the
existing logger.i('Restore in progress...') early-return behavior and keep
references to ref.read(mostroStorageProvider), decryptedEvent, event, and
messageKey intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 363e3505-e6a5-4e82-ad3b-1de7259320e3
📒 Files selected for processing (1)
lib/services/mostro_service.dart
During account restoration, `MostroService` saved historical relay events with current timestamps. This caused them to be replayed after the authoritative synthetic messages (which use original creation timestamps), leading to incorrect order state after an app relaunch. The `isRestoringProvider` flag now prevents `addMessage` during restore, ensuring only the correct restored state persists.
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
I agree with the direction of the fix, but there is still a real correctness issue in the current implementation that makes this unsafe to merge as-is.
The problem is the interaction between the new restore guard and event deduplication in MostroService._onData:
- event IDs are still registered in
eventStoragebefore the restore guard returns - when
isRestoringProvideris true, the message is not written tomostroStorage - after restore finishes, that event is now considered already seen and will not be reprocessed
So any genuinely live DM that arrives during the restore window can be permanently dropped from mostroStorage. This fixes one corruption mode, but introduces another one: missing live events that happened during restore.
That tradeoff is not safe unless the restore flow explicitly guarantees that every event seen during restore is either:
- replayed again after restore, or
- intentionally buffered and persisted later, or
- guaranteed to be fully represented by the synthetic restore state.
From the code in this PR, I do not see that guarantee. Right now the implementation looks like: dedup yes, persist no, replay no. That can lose data.
Please address that gap before merge, either by moving the restore guard earlier so skipped events are not marked as processed, or by buffering/replaying skipped live events once restore completes, or by documenting and enforcing a stronger invariant that makes the drop impossible in practice.
I understand the concern. The filter exists because Nostr relays sometimes replay old events with newer timestamps, which caused |
|
Thanks for the clarification. I agree that this patch fixes the specific relaunch corruption path you were targeting, and I also agree the probability of a genuinely live DM landing inside that restore window is likely low in practice. That said, I do not think the reply fully resolves the correctness concern I raised. The current behavior is still: an event can be marked as seen in So my view is:
Because of that, I am not going to claim my original objection was wrong. If the team decides the low-probability drop during the restore window is acceptable compared with the relaunch corruption it prevents, that is a project call. But I think it should be acknowledged as such, ideally with a follow-up issue tracking either buffering, replay, or moving the dedup boundary. |
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested
🔴 Critical
lib/services/mostro_service.dart— the newif (ref.read(isRestoringProvider)) return;guard is too broad. It skips all incoming DMs while restore is active, buteventStorage.putItem(...)already ran, so those events are permanently marked as seen and will not be replayed later. If a real live message arrives during the restore window, it is silently lost.
⚠️ Warning
- The fix still relies on a 10-second restore window. Even if the intention is only to block historical relay replay, the current implementation cannot distinguish replayed messages from legitimate live messages. This can reintroduce the same class of corruption or cause message loss.
What to change
- Scope the protection to restore-generated/historical payloads only, or buffer incoming messages during restore and process them after the restore completes. Do not drop them after reserving the event ID.
✅ Good
- The root-cause docs were updated consistently.
- Keeping event IDs in
eventStoragefor deduplication is correct in principle; the issue is the timing of the storage reservation relative to the restore gate.
|
Hey @BraCR10, sorry for the delay. I think Mostroneitor has a valid point: a user can restore their account in the middle of an order and, right within those ~10 seconds, receive a status change or another important DM, so I think we should account for that. The catch is that the event ID gets reserved in eventStorage (the putItem) before the isRestoring check, so the event is marked as processed but never stored → on the next launch hasItem discards it and it's never reprocessed. So it's dedup yes, persist no, replay no = the message is lost. I'd suggest buffer-and-replay, which seems cleanest:
That way we don't break the dedup contract and no message gets lost. As a bonus, it also fixes another gap: a live event arriving before its session is recreated is currently lost too, and with the buffer it'll match since the session exists by flush time. |
During account restoration, live Nostr events could interfere with the integrity of the restored state, leading to inconsistencies or potential state loss. This change introduces a mechanism to: - Buffer all incoming live events via `MostroService` while restoration is active, processing them only after restoration completes. - Temporarily halt live order subscriptions (`limit: 0`) to prevent new order events from conflicting with historical data. This ensures a clean restoration process, prevents state corruption, and guarantees that all live events are eventually processed without loss or interference.
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/subscriptions/subscription_manager.dart (1)
42-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOrders subscription is not refreshed when restore mode flips back to false.
limitis computed fromref.read(isRestoringProvider)at filter-build time (Line 132), but subscriptions are rebuilt from session changes only. If sessions are unchanged on restore completion, the orders subscription keeps restore-time filter settings.Also applies to: 122-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/subscriptions/subscription_manager.dart` around lines 42 - 47, The orders subscription keeps restore-time filters because limit is computed only when filters are built and subscriptions are only rebuilt on session changes; add a listener for the restore flag so subscriptions are rebuilt when restore mode flips. In _initSessionListener add a ref.listen<bool>(isRestoringProvider, ...) that calls _updateAllSubscriptions(ref.read(sessionNotifierProvider)) (or otherwise triggers the same rebuild path), and ensure _updateAllSubscriptions (and the filter-building code that computes limit) reads ref.read/isRestoringProvider at rebuild time rather than capturing it once earlier.lib/services/mostro_service.dart (1)
120-127:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBuffered events are effectively dropped due to dedupe short-circuit.
Line 123 reserves the event ID before the restore gate, then
_flushRestoreBuffer()replays with_onData(Line 197). On replay, Line 120 returns immediately (hasItem == true), so buffered live events never reachaddMessage.Also applies to: 174-177, 191-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/services/mostro_service.dart` around lines 120 - 127, The dedupe short-circuit causes buffered events replayed by _flushRestoreBuffer -> _onData to be dropped because eventStore.putItem is called before the restore/replay completes; change the flow so reservation does not block replay: either move the eventStore.putItem call to after the restore/restore-buffer gate (i.e., only reserve the ID once _flushRestoreBuffer has replayed buffered events), or add a replay flag/state (e.g., _isRestoring or pass isReplay into _onData) and modify the early-return that uses eventStore.hasItem to ignore dedupe when replaying; update references to eventStore.hasItem, eventStore.putItem, _flushRestoreBuffer, _onData, and addMessage accordingly so buffered events reach addMessage during replay.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/services/mostro_service.dart`:
- Around line 50-55: init() is attaching a new ref.listen to isRestoringProvider
every time, causing duplicate listeners because dispose() only cancels
_ordersSubscription; fix by storing the listener handle when calling ref.listen
(e.g., assign the returned subscription to a field like _restoreListener or
similar) and ensure you cancel/close it in dispose(), and also cancel any
existing _restoreListener before re-assigning in init() so you never accumulate
multiple ref.listen callbacks that call _flushRestoreBuffer().
---
Outside diff comments:
In `@lib/features/subscriptions/subscription_manager.dart`:
- Around line 42-47: The orders subscription keeps restore-time filters because
limit is computed only when filters are built and subscriptions are only rebuilt
on session changes; add a listener for the restore flag so subscriptions are
rebuilt when restore mode flips. In _initSessionListener add a
ref.listen<bool>(isRestoringProvider, ...) that calls
_updateAllSubscriptions(ref.read(sessionNotifierProvider)) (or otherwise
triggers the same rebuild path), and ensure _updateAllSubscriptions (and the
filter-building code that computes limit) reads ref.read/isRestoringProvider at
rebuild time rather than capturing it once earlier.
In `@lib/services/mostro_service.dart`:
- Around line 120-127: The dedupe short-circuit causes buffered events replayed
by _flushRestoreBuffer -> _onData to be dropped because eventStore.putItem is
called before the restore/replay completes; change the flow so reservation does
not block replay: either move the eventStore.putItem call to after the
restore/restore-buffer gate (i.e., only reserve the ID once _flushRestoreBuffer
has replayed buffered events), or add a replay flag/state (e.g., _isRestoring or
pass isReplay into _onData) and modify the early-return that uses
eventStore.hasItem to ignore dedupe when replaying; update references to
eventStore.hasItem, eventStore.putItem, _flushRestoreBuffer, _onData, and
addMessage accordingly so buffered events reach addMessage during replay.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 622889c4-d343-4b54-80df-6d0a732f9d51
📒 Files selected for processing (4)
docs/architecture/DISPUTE_CHAT_RESTORE.mddocs/architecture/SESSION_RECOVERY_ARCHITECTURE.mdlib/features/subscriptions/subscription_manager.dartlib/services/mostro_service.dart
✅ Files skipped from review due to trivial changes (1)
- docs/architecture/SESSION_RECOVERY_ARCHITECTURE.md
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent restored order/dispute state from being overwritten on app relaunch by avoiding persistence of relay-replayed events during the restore window, so that sync() replays only the authoritative synthetic restore messages.
Changes:
- Add restore-mode gating in
MostroService._onDatato avoid writing messages tomostroStorageduring restore (and introduce buffering/flush logic). - Adjust orders subscription filter during restore (
SubscriptionManager) to reduce/avoid historical replay. - Update architecture documentation to describe restore-mode protections and mark the dispute restore/relaunch issue as fixed.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/services/mostro_service.dart | Adds restore-mode checks around DM processing/storage and introduces a restore buffer + flush mechanism. |
| lib/features/subscriptions/subscription_manager.dart | Modifies orders subscription filter to apply a restore-dependent limit. |
| docs/architecture/SESSION_RECOVERY_ARCHITECTURE.md | Documents restore-mode protections and intended effects on message persistence/state reconstruction. |
| docs/architecture/DISPUTE_CHAT_RESTORE.md | Updates root-cause + fix narrative and marks the relaunch-after-restore issue as fixed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return NostrFilter( | ||
| kinds: [1059], | ||
| p: sessions.map((s) => s.tradeKey.public).toList(), | ||
| limit: ref.read(isRestoringProvider) ? 0 : null, | ||
| ); |
Update the documentation for "Issue 3 — Dispute State Not Persisted After Restore + App Kill" in `DISPUTE_CHAT_RESTORE.md`. This revision clarifies the two-part fix for robust account restoration: 1. `SubscriptionManager` now filters historical order events by passing `limit: 0` to relays during restore. 2. `MostroService._onData` buffers any incoming live events, applying them only after the restoration process has completed. This ensures a clean and consistent state on app relaunch by preventing interference from live or historical events during the restoration window.
…restore This refines the account restoration process by formalizing the management of incoming Nostr events and subscription states. Dedicated listeners in `SubscriptionManager` and `MostroService` now precisely control: - When subscriptions request `limit:0` to block historical backfill during restore. - When live events are buffered by `MostroService`. - When the buffer is flushed and subscriptions resume normal operation with `limit:null` after restoration completes. This ensures a clean, consistent state by preventing interference from live or historical events with the authoritative synthetic state built during restore.
There was a problem hiding this comment.
I re-reviewed the current head and I still see one real correctness issue that I would block on.
Blocking issue:
SubscriptionManagernow computes the orders filter withlimit: ref.read(isRestoringProvider) ? 0 : null, but that value is captured when the filter is built.- From the changes in this PR, I do not see a corresponding subscription refresh triggered when restore mode flips from
trueback tofalse. - If the session set is unchanged when restore completes, the existing orders subscription can remain alive with the restore-time
limit: 0filter instead of being rebuilt into the normal runtime filter.
Why this matters:
- The PR’s safety story depends on restore mode being temporary: block historical replay during restore, then return to the normal live subscription shape afterwards.
- If the orders subscription is not refreshed on restore exit, the app can stay in the restore-mode subscription shape longer than intended.
- That makes the fix incomplete because the relay-side protection is no longer scoped cleanly to the restore window.
What I would want:
- Rebuild or refresh the orders subscription when
isRestoringProviderchanges, not only when the session set changes. - In other words, restore-mode transition itself needs to be a first-class resubscription trigger.
The buffering work in MostroService is the right direction and does address the earlier data-loss issue, but this subscription-lifecycle gap is still a blocker for me.
The `_restoreModeListener` was responsible for updating subscriptions when the restoration process completed, specifically by rebuilding them without the `limit:0` constraint. This functionality has now been consolidated into the broader orchestration of event buffering and subscription transitions, making this dedicated listener redundant.
Resolve conflict in subscription_manager.dart: apply restore-mode limit:0 to both transport variants (giftWrap kind 1059, nip44 kind 14) from transport v2 Phase A.
…arsing Standardizes timestamps for synthetic messages generated during account restoration by using a single `_restoreStartTime`. This prevents inconsistencies from dynamically generated timestamps in message keys and content. Additionally, NIP-59 gift-wrapped events now use the `createdAt` of the inner, decrypted event as the message timestamp. This captures the true send time, as the outer event's `createdAt` is randomized for privacy. These changes improve overall state consistency, particularly during restoration.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/mostro_service.dart (1)
193-196: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMove restore buffering before
eventStoragereservation.Line 193 buffers after Lines 122-126 already stored the outer event id. When
_flushRestoreBuffer()replays the event on Line 221,_onData()hitshasItem(event.id!)and returns, so every buffered live event is dropped instead of persisted.Proposed fix
class MostroService { final Ref ref; Settings _settings; StreamSubscription<NostrEvent>? _ordersSubscription; ProviderSubscription<bool>? _restoreListener; final List<NostrEvent> _restoreBuffer = []; + final Set<String> _restoreBufferedEventIds = <String>{}; @@ Future<void> _onData(NostrEvent event) async { + final eventId = event.id; + if (eventId == null) { + logger.w('Received event without id, skipping'); + return; + } + + if (ref.read(isRestoringProvider)) { + if (_restoreBufferedEventIds.add(eventId)) { + _restoreBuffer.add(event); + logger.i('Restore: buffered live event $eventId'); + } + return; + } + final eventStore = ref.read(eventStorageProvider); - if (await eventStore.hasItem(event.id!)) return; + if (await eventStore.hasItem(eventId)) return; @@ - await eventStore.putItem(event.id!, { - 'id': event.id, + await eventStore.putItem(eventId, { + 'id': eventId, 'created_at': event.createdAt!.millisecondsSinceEpoch ~/ 1000, }); @@ - if (ref.read(isRestoringProvider)) { - _restoreBuffer.add(event); - logger.i('Restore: buffered live event ${event.id} for ${msg.action}'); - return; - } - @@ Future<void> _flushRestoreBuffer() async { if (_restoreBuffer.isEmpty) return; final buffer = List<NostrEvent>.from(_restoreBuffer); _restoreBuffer.clear(); + _restoreBufferedEventIds.clear();Also applies to: 215-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/services/mostro_service.dart` around lines 193 - 196, The restore buffering in MostroService::_onData is happening after the outer event has already been reserved in eventStorage, so buffered live events get skipped when _flushRestoreBuffer replays them. Move the restore check and _restoreBuffer.add(event) ahead of the eventStorage hasItem/add reservation path, and ensure the replayed event can be persisted normally instead of returning early on hasItem(event.id!).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/services/mostro_service.dart`:
- Around line 199-204: The timestamp fallback in MostroService’s message
handling should use the original event time for direct DMs instead of processing
time. Update the logic around msg.timestamp assignment so that when
innerCreatedAt is null (such as kind 14), it falls back to event.createdAt
before calling messageStorage.addMessage, keeping NIP-44 ordering consistent.
Use the existing MostroService event/message flow and the msg.timestamp ??=
assignment to locate the fix.
---
Outside diff comments:
In `@lib/services/mostro_service.dart`:
- Around line 193-196: The restore buffering in MostroService::_onData is
happening after the outer event has already been reserved in eventStorage, so
buffered live events get skipped when _flushRestoreBuffer replays them. Move the
restore check and _restoreBuffer.add(event) ahead of the eventStorage
hasItem/add reservation path, and ensure the replayed event can be persisted
normally instead of returning early on hasItem(event.id!).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bf14479-40b5-4539-a0c6-7695a73f9ac2
📒 Files selected for processing (3)
lib/features/restore/restore_manager.dartlib/features/subscriptions/subscription_manager.dartlib/services/mostro_service.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/subscriptions/subscription_manager.dart
| // Use inner rumor's created_at (real send time) — outer gift wrap is | ||
| // NIP-59 randomized. Historical events have old inner timestamps (sort | ||
| // below synthetics); live events have recent inner timestamps (gate passes). | ||
| msg.timestamp ??= innerCreatedAt?.millisecondsSinceEpoch; | ||
|
|
||
| await messageStorage.addMessage(messageKey, msg); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant service and storage behavior
git ls-files lib/services/mostro_service.dart lib/**/message_storage.dart lib/**/storage*.dart lib/**/message*.dart
echo '--- mostro_service.dart outline ---'
ast-grep outline lib/services/mostro_service.dart --view expanded || true
echo '--- search for addMessage and timestamp fallback ---'
rg -n "addMessage\\(|timestamp .*DateTime\\.now|msg\\.timestamp|innerCreatedAt|kind == 14|createdAt" lib/services lib -g '*.dart' -A 3 -B 3Repository: MostroP2P/mobile
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant section with line numbers once the file is located.
sed -n '160,240p' lib/services/mostro_service.dartRepository: MostroP2P/mobile
Length of output: 2891
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the implementation of messageStorage.addMessage if present.
rg -n "class .*MessageStorage|Future<.*addMessage|void addMessage|DateTime\\.now\\(" lib -g '*.dart' -A 5 -B 5Repository: MostroP2P/mobile
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the MostroMessage model and its timestamp parsing.
rg -n "class MostroMessage|MostroMessage\\.fromJson|timestamp" lib -g '*.dart' -A 6 -B 6 | sed -n '1,220p'Repository: MostroP2P/mobile
Length of output: 16989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the concrete storage implementation used by mostroStorageProvider and its addMessage fallback.
sed -n '1,220p' lib/shared/providers/storage_providers.dart
echo '---'
rg -n "class .*Storage|addMessage\\(|DateTime\\.now\\(" lib -g '*.dart' -A 8 -B 4 | sed -n '1,240p'Repository: MostroP2P/mobile
Length of output: 18652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the MostroMessage model and its timestamp field handling.
fd -a "mostro_message.dart" lib
fd -a "message.dart" lib | sed -n '1,80p'
rg -n "class MostroMessage|MostroMessage\\.fromJson|timestamp" lib -g '*.dart' -A 8 -B 6 | sed -n '1,220p'Repository: MostroP2P/mobile
Length of output: 17321
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the concrete storage implementation behind mostroStorageProvider and its addMessage behavior.
rg -n "mostroStorageProvider|class .*Storage|addMessage\\(" lib -g '*.dart' -A 8 -B 6 | sed -n '1,260p'Repository: MostroP2P/mobile
Length of output: 23071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the MostroMessage model and the concrete MostroStorage implementation.
sed -n '1,220p' lib/data/models/mostro_message.dart
echo '---'
fd -a "mostro_storage.dart" lib
echo '---'
sed -n '1,260p' lib/data/repositories/mostro_storage.dartRepository: MostroP2P/mobile
Length of output: 10636
Use event.createdAt for direct DMs. kind == 14 leaves innerCreatedAt null here, so MostroStorage.addMessage() falls back to DateTime.now() and reorders NIP-44 messages by processing time. Use event.createdAt as the fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/services/mostro_service.dart` around lines 199 - 204, The timestamp
fallback in MostroService’s message handling should use the original event time
for direct DMs instead of processing time. Update the logic around msg.timestamp
assignment so that when innerCreatedAt is null (such as kind 14), it falls back
to event.createdAt before calling messageStorage.addMessage, keeping NIP-44
ordering consistent. Use the existing MostroService event/message flow and the
msg.timestamp ??= assignment to locate the fix.
Closes #584
Summary
MostroService._onDatawas saving relay-replayed events tomostroStoragewithDateTime.now()timestamps. Those were newer than the synthetic restore messages (which useorderDetail.createdAt). On relaunch,sync()replayed them last, overwriting the correct restored state.addMessagein_onDatawhenisRestoringProvideris true. Event IDs still register ineventStoragefor dedup.How to test
Summary by CodeRabbit
limit: 0while restoring to prevent historical relay events from being processed in the restore window.