Fix orphan sessions with timeout detection and cleanup #305
Conversation
WalkthroughAdds a per-order orphan-session cleanup timer (10s) in the Mostro notifier, cancels timers on any Mostro response, updates navigation to '/', adds localized session-timeout messages and notification wiring, and expands architecture docs with orphan-prevention, session lifecycle (72h expiration) and timer management; tests/mocks guidance updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI
participant OrderNotifier
participant MostroNotifier as AbstractMostroNotifier
participant Timer as "10s Timer"
participant Mostro
participant Router
participant Notifications
User->>UI: Take order
UI->>OrderNotifier: takeSellOrder / takeBuyOrder
OrderNotifier->>MostroNotifier: create session
OrderNotifier->>MostroNotifier: startSessionTimeoutCleanup(orderId, ref)
MostroNotifier->>Timer: schedule 10s [orderId]
par Real-time responses
Mostro-->>MostroNotifier: any response/event
MostroNotifier->>Timer: _cancelSessionTimeoutCleanup(orderId)
MostroNotifier->>OrderNotifier: normal event handling
end
alt Timer expires before response
Timer-->>MostroNotifier: timeout(orderId)
MostroNotifier->>MostroNotifier: delete session / mark timeout
MostroNotifier->>Notifications: emit customMessage=sessionTimeoutMessage
MostroNotifier->>Router: navigate to "/"
else Response arrives in time
MostroNotifier->>UI: continue normal flow (session preserved or updated)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
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: 1
🧹 Nitpick comments (9)
lib/features/order/notfiers/abstract_mostro_notifier.dart (1)
359-378: Externalize 30s to Config for flexibility.Hardcoding 30s limits tuning and testing. Move to Config (e.g., Config.orphanSessionCleanupSeconds).
- _sessionTimeouts[orderId] = Timer(const Duration(seconds: 30), () { + _sessionTimeouts[orderId] = Timer(Duration(seconds: Config.orphanSessionCleanupSeconds), () {And define:
class Config { + static const int orphanSessionCleanupSeconds = 30; }lib/features/order/notfiers/order_notifier.dart (1)
107-109: Starting the 30s cleanup after session creation is correct.Timers start only for taker flows; aligns with objectives.
Wrap service calls in try/catch to clean up immediately on synchronous errors and avoid waiting 30s:
- await mostroService.takeSellOrder( + try { + await mostroService.takeSellOrder( orderId, amount, lnAddress, - ); + ); + } catch (e, st) { + // Cancel timer and delete session on immediate failure + // Consider exposing a public cancel method: + // AbstractMostroNotifier.cancelSessionTimeoutCleanup(orderId); + await ref.read(sessionNotifierProvider.notifier).deleteSession(orderId); + logger.e('takeSellOrder failed', error: e, stackTrace: st); + rethrow; + }Same pattern for takeBuyOrder(). Optionally expose a public cancel API on AbstractMostroNotifier to stop the timer here.
Also applies to: 124-126
docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md (1)
970-977: Fix snippet: cancel by orderId, not event.id.Keep docs aligned with the code fix; event.id ≠ orderId.
-// lib/features/order/notfiers/abstract_mostro_notifier.dart:86-90 -void handleEvent(MostroMessage event) { - // Cancel timer on ANY response from Mostro for this orderId - if (event.id != null) { - _cancelSessionTimeoutCleanup(event.id!); - } +// lib/features/order/notfiers/abstract_mostro_notifier.dart:86-90 +void handleEvent(MostroMessage event) { + // Cancel timer on ANY response from Mostro for this order + _cancelSessionTimeoutCleanup(orderId); // ... rest of event handling }CLAUDE.md (3)
138-141: Fix typo: remove “38383” from public event monitoring.“38383 public events” reads like a copy/paste from the derivation path and is misleading.
Apply this diff:
- - **Monitors**: 38383 public events for status changes + - **Monitors**: public order events for status changes
126-127: Avoid file+line references in docs (they rot quickly).Replace hardcoded line spans like “abstract_mostro_notifier.dart:359-378” and “order_notifier.dart:172-211” with symbol/path references only.
Suggested edits:
- **Implementation**: `AbstractMostroNotifier.startSessionTimeoutCleanup()` in `abstract_mostro_notifier.dart:359-378` + **Implementation**: `AbstractMostroNotifier.startSessionTimeoutCleanup()` (see `lib/features/order/notfiers/abstract_mostro_notifier.dart`) - **Implementation**: `_checkTimeoutAndCleanup()` method in `order_notifier.dart:172-211` + **Implementation**: `_checkTimeoutAndCleanup()` (see `lib/features/order/notfiers/order_notifier.dart`)Also applies to: 163-164
128-134: Add a code-fence language to satisfy markdownlint MD040.Label this block as text for better linting and readability.
Apply:
-``` +```text English: "No response received, check your connection and try again later" Spanish: "No hubo respuesta, verifica tu conexión e inténtalo más tarde" Italian: "Nessuna risposta ricevuta, verifica la tua connessione e riprova più tardi"</blockquote></details> <details> <summary>lib/shared/widgets/notification_listener_widget.dart (1)</summary><blockquote> `57-74`: **Optional: replace the switch with a map to reduce duplication/typos.** Mirroring CantDoNotificationMapper, a `Map<String, String Function(BuildContext)>` for customMessage would simplify additions and avoid stringly-typed switches. </blockquote></details> <details> <summary>docs/architecture/SESSION_AND_KEY_MANAGEMENT.md (2)</summary><blockquote> `195-197`: **Add language to code fence (markdownlint MD040).** Mark this as Dart. ```diff -``` +```dart // Automatically started when taking orders AbstractMostroNotifier.startSessionTimeoutCleanup(orderId, ref);--- `211-218`: **Tighten the example snippet and label as Dart.** The line `state = sessions;` may confuse readers unless `sessions` is a known getter. Either show an explicit list/emit or mark as pseudo. If it’s real Dart, prefer a concrete state update and add fence language. ```diff -``` +```dart // lib/shared/notifiers/session_notifier.dart:157-161 Future<void> deleteSession(String sessionId) async { _sessions.remove(sessionId); await _storage.deleteSession(sessionId); - state = sessions; // Update state to trigger UI updates + state = List.unmodifiable(_sessions.values); // Trigger UI updates }</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 08383f3bb2d86bc8741ae85e9c19edb90f70d208 and ed48f984ee4f21eca491ed337ffee18291fb0be7. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `CLAUDE.md` (3 hunks) * `docs/architecture/SESSION_AND_KEY_MANAGEMENT.md` (2 hunks) * `docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md` (1 hunks) * `lib/features/order/notfiers/abstract_mostro_notifier.dart` (5 hunks) * `lib/features/order/notfiers/order_notifier.dart` (3 hunks) * `lib/l10n/intl_en.arb` (1 hunks) * `lib/l10n/intl_es.arb` (1 hunks) * `lib/l10n/intl_it.arb` (1 hunks) * `lib/shared/widgets/notification_listener_widget.dart` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (4)</summary> <details> <summary>{lib/*.dart,lib/!(generated)/**/*.dart}</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > `{lib/*.dart,lib/!(generated)/**/*.dart}`: Use localized strings; replace hardcoded user-facing text with `S.of(context).keyName` > Prefer `const` constructors and `const` where possible > Use latest Flutter/Dart APIs (e.g., `withValues()` instead of deprecated `withOpacity()`) Files: - `lib/shared/widgets/notification_listener_widget.dart` - `lib/features/order/notfiers/order_notifier.dart` - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> <details> <summary>{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > `{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}`: Check `mounted` before using `BuildContext` after `await` (async gaps) > Remove unused imports and dependencies Files: - `lib/shared/widgets/notification_listener_widget.dart` - `lib/features/order/notfiers/order_notifier.dart` - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> <details> <summary>**/*.dart</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > All code comments, variable names, and function names must be in English Files: - `lib/shared/widgets/notification_listener_widget.dart` - `lib/features/order/notfiers/order_notifier.dart` - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> <details> <summary>lib/l10n/intl_{en,es,it}.arb</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Add new localization keys to all three ARB files (en, es, it) Files: - `lib/l10n/intl_en.arb` - `lib/l10n/intl_es.arb` - `lib/l10n/intl_it.arb` </details> </details><details> <summary>🧠 Learnings (9)</summary> <details> <summary>📚 Learning: 2025-06-26T15:03:23.529Z</summary>Learnt from: chebizarro
PR: #127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.**Applied to files:** - `lib/features/order/notfiers/order_notifier.dart` - `lib/features/order/notfiers/abstract_mostro_notifier.dart` - `docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md` - `CLAUDE.md` - `docs/architecture/SESSION_AND_KEY_MANAGEMENT.md` </details> <details> <summary>📚 Learning: 2025-05-06T15:49:26.443Z</summary>Learnt from: chebizarro
PR: #74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, Riverpod code generation is used withRiverpodannotations. Providers likeeventStorageProviderare generated in.g.dartfiles from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g.,mostro_service_provider.dart), not by importing a separate provider file.**Applied to files:** - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> <details> <summary>📚 Learning: 2025-08-28T20:32:34.625Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Use Riverpod for state management with Notifiers encapsulating business logic; access data only through repositories**Applied to files:** - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> <details> <summary>📚 Learning: 2025-08-15T01:37:12.243Z</summary>Learnt from: Catrya
PR: #270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.**Applied to files:** - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> <details> <summary>📚 Learning: 2025-07-20T23:33:17.689Z</summary>Learnt from: Catrya
PR: #215
File: lib/features/order/notfiers/order_notifier.dart:0-0
Timestamp: 2025-07-20T23:33:17.689Z
Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.**Applied to files:** - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> <details> <summary>📚 Learning: 2025-06-04T19:35:20.209Z</summary>Learnt from: chebizarro
PR: #110
File: test/notifiers/take_order_notifier_test.dart:72-74
Timestamp: 2025-06-04T19:35:20.209Z
Learning: MostroService methods like takeBuyOrder() and takeSellOrder() return Future and trigger side effects through other mechanisms rather than direct return values. When testing these methods, focus on verifying method calls and testing state changes through the provider system rather than mocking return values.**Applied to files:** - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> <details> <summary>📚 Learning: 2025-08-28T20:32:34.625Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Applies to lib/l10n/intl_{en,es,it}.arb : Add new localization keys to all three ARB files (en, es, it)**Applied to files:** - `lib/l10n/intl_es.arb` - `lib/l10n/intl_it.arb` </details> <details> <summary>📚 Learning: 2025-08-28T20:32:34.625Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Applies to {lib/.dart,lib/!(generated)/**/.dart} : Use latest Flutter/Dart APIs (e.g.,withValues()instead of deprecatedwithOpacity())**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-08-28T20:32:34.625Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-28T20:32:34.625Z
Learning: Applies to lib/main.dart : Configure timeago locales in main startup (usetimeago.setLocaleMessages(...))**Applied to files:** - `CLAUDE.md` </details> </details><details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>CLAUDE.md</summary> 129-129: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>docs/architecture/SESSION_AND_KEY_MANAGEMENT.md</summary> 189-189: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 199-199: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: build </details> <details> <summary>🔇 Additional comments (17)</summary><blockquote> <details> <summary>lib/l10n/intl_es.arb (2)</summary><blockquote> `1083-1084`: **Comma fix LGTM.** Valid JSON separation before the new keys. --- `1085-1086`: **Resolved — no change required** sessionTimeoutMessage is already present in lib/l10n/intl_en.arb, lib/l10n/intl_es.arb and lib/l10n/intl_it.arb. </blockquote></details> <details> <summary>lib/l10n/intl_en.arb (2)</summary><blockquote> `1105-1106`: **Comma fix LGTM.** Keeps JSON valid while appending new entries. --- `1107-1109`: **New timeout message LGTM — ensure Italian added too.** English key/value look good; mirror in intl_it.arb. See the shell check in my other comment to confirm presence in all locales. </blockquote></details> <details> <summary>lib/features/order/notfiers/abstract_mostro_notifier.dart (6)</summary><blockquote> `1-1`: **Import dart:async LGTM.** Needed for Timer usage. --- `23-24`: **Static timer map introduced — OK.** Centralized per-order timers are fine; you already clean up on dispose. --- `271-273`: **Route change to ‘/’ on cancellation LGTM.** Matches the documented reset behavior. --- `395-404`: **Cancel helper LGTM.** Clear, idempotent cancellation with logging. --- `408-411`: **Dispose cleanup LGTM.** Prevents timer leaks when notifier is disposed. --- `381-393`: **Verify NotificationListenerWidget maps 'sessionTimeoutMessage' to S.of(context).sessionTimeoutMessage** Confirm NotificationListenerWidget returns the localized string for 'sessionTimeoutMessage' used in lib/features/order/notfiers/abstract_mostro_notifier.dart:381–393; if absent, wire the mapping or switch to an Action-based temporary notification for consistency. </blockquote></details> <details> <summary>docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md (1)</summary><blockquote> `943-963`: **Doc snippet matches implementation.** Good example showing timer creation and navigation on expiry. </blockquote></details> <details> <summary>CLAUDE.md (2)</summary><blockquote> `165-175`: **Verified — referenced flags/utilities exist and are used as described.** _isProcessingTimeout and _isCreatedByUser(Session, NostrEvent) are in lib/features/order/notfiers/order_notifier.dart; static Map<String, Timer> _sessionTimeouts, startSessionTimeoutCleanup(...), and _cancelSessionTimeoutCleanup(...) are in lib/features/order/notfiers/abstract_mostro_notifier.dart. --- `122-127`: **Update CLAUDE.md: navigation on session timeout is '/' (main screen)** abstract_mostro_notifier.startSessionTimeoutCleanup calls navProvider.go('/'), so update the CLAUDE.md bullet (lines 122–127) from "navigates to order book" to "navigates to the main screen ('/')." Evidence: lib/features/order/notfiers/abstract_mostro_notifier.dart (startSessionTimeoutCleanup → navProvider.go('/')). </blockquote></details> <details> <summary>docs/architecture/SESSION_AND_KEY_MANAGEMENT.md (2)</summary><blockquote> `187-188`: **Confirm 72h session retention is implemented** Found session-cleanup scaffolding in lib/features/order/notfiers/abstract_mostro_notifier.dart (static Map _sessionTimeouts; call to cleanupRequestSession), but I did not find any constant or scheduler set to 72 hours (no "72h", "72 hour", or Duration(hours: 72) matched). Update the enforcement constant/scheduler to use 72 hours (and reference it from the cleanup logic) or point to the exact file/constant that enforces 72h. --- `220-227`: **Confirmed: static timer storage present and cleaned up on dispose.** lib/features/order/notfiers/abstract_mostro_notifier.dart declares `static final Map<String, Timer> _sessionTimeouts` and correctly: cancels/replaces existing timers in `startSessionTimeoutCleanup`, removes the entry on timeout, cancels+removes on Mostro response via `_cancelSessionTimeoutCleanup`, and cancels+removes the order's timer in `dispose` (see ~lines 24, 360–375, 396–403, 406–411). </blockquote></details> <details> <summary>lib/l10n/intl_it.arb (1)</summary><blockquote> `1137-1141`: **LGTM — sessionTimeoutMessage present in intl_en.arb, intl_es.arb, intl_it.arb; regenerate l10n** Confirmed via ripgrep: lib/l10n/intl_en.arb:1108, lib/l10n/intl_es.arb:1086, lib/l10n/intl_it.arb:1140. </blockquote></details> <details> <summary>lib/shared/widgets/notification_listener_widget.dart (1)</summary><blockquote> `69-71`: **LGTM — sessionTimeoutMessage key present in ARB files; generated l10n not found.** - Confirmed in lib/l10n/intl_en.arb:1108, lib/l10n/intl_es.arb:1086, lib/l10n/intl_it.arb:1140. - No generated localization Dart (no class S or getter found in repo); regenerate l10n (e.g., flutter gen-l10n) and ensure generated files are available/committed. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
It looks pretty good. I’m not sure if we should have the timer hardcoded, but other than that it seems fine. One question though: how does the timer work if the user leaves the app in the background? |
Have you tested it? If the app stays in the background, the counter keeps running, and when the user returns to the app, they see the message that there was no response. |
Claro pero no se si deberia ser asi porque una persona puede tomar una orden y recibir una llamada por ejemplo, la app queda en background y le sacas la orden por eso, no se si sea el mejor approach en esos casos |
El timer corre y la sesión se elimina si no hay respuesta de mostro (si el usiario estaba offline y quizo tomar una orden, o si está online y hay error como unwraping gift u otro que hace que no sea exitoso el take order). Pero si llega cualquier mensaje de mostro, como cant-do o addinvoice o cualquier otro, el timer se cancela y continúa con el mensaje que le llegó |
- Add 30s timer cleanup for phantom sessions when Mostro doesn't respond - Handle maker vs taker scenarios differently (session preservation vs cleanup) - Add localized timeout notifications and automatic navigation
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/features/order/notfiers/abstract_mostro_notifier.dart (1)
95-97: Fix: cancel timer by orderId (not event.id).This addresses the earlier bug—now the timer will properly cancel on any response for the order.
🧹 Nitpick comments (4)
lib/features/order/notfiers/abstract_mostro_notifier.dart (2)
309-320: Avoid navigating while app is backgrounded.Consider gating navigation behind an app‑lifecycle check and show only a notification if not foreground.
286-305: Make timeout configurable (avoid hardcoded 30s).Add an optional Duration parameter with a default of 30s; change is backward-compatible — callers currently invoke without a custom timeout (lib/features/order/notfiers/order_notifier.dart:108, 125).
- static void startSessionTimeoutCleanup(String orderId, Ref ref) { + static void startSessionTimeoutCleanup(String orderId, Ref ref, {Duration? timeout}) { // Cancel existing timer if any _sessionTimeouts[orderId]?.cancel(); - _sessionTimeouts[orderId] = Timer(const Duration(seconds: 30), () { + _sessionTimeouts[orderId] = Timer(timeout ?? const Duration(seconds: 30), () {docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md (1)
938-964: Doc consistency: session expiration hours conflict (36h vs 72h).Unify mentions to reflect the actual Config.sessionExpirationHours (72h per snippet later in doc).
CLAUDE.md (1)
129-135: Add language to fenced code block (markdownlint MD040).Mark the localized examples as json to satisfy linting.
-``` +```json { "en": "No response received, check your connection and try again later", "es": "No hubo respuesta, verifica tu conexión e inténtalo más tarde", "it": "Nessuna risposta ricevuta, verifica la tua connessione e riprova più tardi" }</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 68653670856444bf2690de0825bfd06428d11528 and 23533565858aee0a3696b947e3cedff3a46c1c7c. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `CLAUDE.md` (3 hunks) * `docs/architecture/SESSION_AND_KEY_MANAGEMENT.md` (2 hunks) * `docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md` (1 hunks) * `lib/features/order/notfiers/abstract_mostro_notifier.dart` (4 hunks) * `lib/features/order/notfiers/order_notifier.dart` (3 hunks) * `lib/l10n/intl_en.arb` (1 hunks) * `lib/l10n/intl_es.arb` (1 hunks) * `lib/l10n/intl_it.arb` (1 hunks) * `lib/shared/widgets/notification_listener_widget.dart` (1 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * docs/architecture/SESSION_AND_KEY_MANAGEMENT.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * lib/features/order/notfiers/order_notifier.dart * lib/shared/widgets/notification_listener_widget.dart * lib/l10n/intl_it.arb </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (3)</summary> <details> <summary>lib/l10n/intl_{en,es,it}.arb</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > When adding a localization key, add it to all three ARB files: intl_en.arb, intl_es.arb, intl_it.arb Files: - `lib/l10n/intl_es.arb` - `lib/l10n/intl_en.arb` </details> <details> <summary>lib/l10n/*.arb</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > After modifying ARB files, regenerate localization (e.g., `dart run build_runner build -d` or `flutter gen-l10n`) Files: - `lib/l10n/intl_es.arb` - `lib/l10n/intl_en.arb` </details> <details> <summary>**/*.dart</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > Run flutter format and keep code formatted Files: - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> </details><details> <summary>🧠 Learnings (5)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: chebizarro
PR: #127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.</details> <details> <summary>📚 Learning: 2025-06-26T15:03:23.529Z</summary>Learnt from: chebizarro
PR: #127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.**Applied to files:** - `lib/features/order/notfiers/abstract_mostro_notifier.dart` - `docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md` - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-05-06T15:49:55.079Z</summary>Learnt from: chebizarro
PR: #74
File: lib/services/mostro_service.dart:97-103
Timestamp: 2025-05-06T15:49:55.079Z
Learning: In the Mostro protocol, an order cannot be canceled unless it has a valid orderId, so null checks on orderId during cancel operations are unnecessary.**Applied to files:** - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> <details> <summary>📚 Learning: 2025-06-04T19:35:20.209Z</summary>Learnt from: chebizarro
PR: #110
File: test/notifiers/take_order_notifier_test.dart:72-74
Timestamp: 2025-06-04T19:35:20.209Z
Learning: MostroService methods like takeBuyOrder() and takeSellOrder() return Future and trigger side effects through other mechanisms rather than direct return values. When testing these methods, focus on verifying method calls and testing state changes through the provider system rather than mocking return values.**Applied to files:** - `lib/features/order/notfiers/abstract_mostro_notifier.dart` </details> <details> <summary>📚 Learning: 2025-09-18T17:17:38.944Z</summary>Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T17:17:38.944Z
Learning: Applies to lib/main.dart : Configure timeago locale messages initialization in main.dart**Applied to files:** - `CLAUDE.md` </details> </details><details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>CLAUDE.md</summary> 130-130: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ 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)</summary> * GitHub Check: build </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>lib/features/order/notfiers/abstract_mostro_notifier.dart (2)</summary><blockquote> `23-25`: **Static timer map is fine here.** Per‑order timers stored statically are acceptable; cleanup is handled elsewhere. --- `322-330`: **Timer cleanup on cancel/dispose — LGTM.** _cancelSessionTimeoutCleanup cancels the Timer and removes the map entry; startSessionTimeoutCleanup is invoked from order_notifier — no leak or follow-up required. </blockquote></details> <details> <summary>lib/l10n/intl_en.arb (1)</summary><blockquote> `1108-1112`: **English locale entry added — intl_it.arb verified.** Confirmed sessionTimeoutMessage exists in lib/l10n/intl_en.arb, lib/l10n/intl_es.arb, and lib/l10n/intl_it.arb; no changes required. </blockquote></details> <details> <summary>lib/l10n/intl_es.arb (1)</summary><blockquote> `1086-1090`: **Keys present in all locales — regenerate l10n.** sessionTimeoutMessage exists in lib/l10n/intl_en.arb, lib/l10n/intl_es.arb, and lib/l10n/intl_it.arb; run `flutter gen-l10n` (or `dart run build_runner build -d`) to update generated localization files and confirm the metadata key "@_comment_session_timeout" is present across locales. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md (1)
970-976: LGTM! Documentation now correctly shows timer cancellation.The code example properly demonstrates cancelling the timer using
orderId, which aligns with the implementation.
🧹 Nitpick comments (4)
docs/architecture/APP_INITIALIZATION_ANALYSIS.md (1)
355-366: Consider clarifying the dual-path architecture explanation.The documentation would benefit from explaining why both paths are necessary rather than using
fireImmediately: truewith proper state checks. This would help future maintainers understand the architectural decision.Consider adding a brief explanation like:
**Dual-Path Architecture**: - **Listener Path**: Handles future session changes (new trades, session deletions) - **Manual Path**: Handles existing sessions during app initialization +- **Why Not fireImmediately: true?**: Using fireImmediately would execute the listener before SessionNotifier completes init, potentially processing empty/incomplete session data - **Result**: Complete coverage of all session lifecycle scenarioslib/features/order/notfiers/abstract_mostro_notifier.dart (1)
308-320: Consider consolidating error handling.Both the timer callback and this method have separate try-catch blocks. The inner catches may prevent proper logging of failures.
Consider simplifying error handling:
static void _showTimeoutNotificationAndNavigate(Ref ref) { - try { - // Show snackbar with localized timeout message - final notificationNotifier = ref.read(notificationActionsProvider.notifier); - notificationNotifier.showCustomMessage('sessionTimeoutMessage'); - - // Navigate to main order book screen (home) - final navProvider = ref.read(navigationProvider.notifier); - navProvider.go('/'); - } catch (e) { - Logger().e('Failed to show timeout notification or navigate', error: e); - } + // Show snackbar with localized timeout message + final notificationNotifier = ref.read(notificationActionsProvider.notifier); + notificationNotifier.showCustomMessage('sessionTimeoutMessage'); + + // Navigate to main order book screen (home) + final navProvider = ref.read(navigationProvider.notifier); + navProvider.go('/'); }Then let the outer try-catch in the timer callback handle any errors.
docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md (1)
930-963: Documentation inconsistency: 10s vs 30s timer duration.The documentation correctly shows 10-second timer implementation in the code examples, but line 1038 mentions "30-second cleanup" which contradicts both the code and earlier documentation.
Apply this fix to maintain consistency:
-3. **Dual protection**: Ensures sessions are cleaned up either through real-time detection or automatic timeout +3. **Dual protection**: Ensures sessions are cleaned up either through real-time detection or automatic 10-second timeoutAnd update line 1038:
-2. **30-second cleanup**: Acts as a fallback to prevent orphan sessions when Mostro is completely unresponsive +2. **10-second cleanup**: Acts as a fallback to prevent orphan sessions when Mostro is completely unresponsiveCLAUDE.md (1)
112-135: Documentation inconsistency in timer duration.Line 142 mentions "30-second cleanup" but the implementation uses 10 seconds. Please update to maintain consistency.
Apply this fix:
-- **Integration**: Works alongside 30-second cleanup for comprehensive protection +- **Integration**: Works alongside 10-second cleanup for comprehensive protection
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CLAUDE.md(3 hunks)docs/architecture/APP_INITIALIZATION_ANALYSIS.md(1 hunks)docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md(1 hunks)lib/features/order/notfiers/abstract_mostro_notifier.dart(4 hunks)lib/features/order/notfiers/order_notifier.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/order/notfiers/order_notifier.dart
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Run flutter format and keep code formatted
Files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
🧠 Learnings (6)
📓 Common learnings
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
📚 Learning: 2025-06-26T15:03:23.529Z
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dartdocs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md
📚 Learning: 2025-05-06T15:49:55.079Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:97-103
Timestamp: 2025-05-06T15:49:55.079Z
Learning: In the Mostro protocol, an order cannot be canceled unless it has a valid orderId, so null checks on orderId during cancel operations are unnecessary.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dartdocs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md
📚 Learning: 2025-06-04T19:35:20.209Z
Learnt from: chebizarro
PR: MostroP2P/mobile#110
File: test/notifiers/take_order_notifier_test.dart:72-74
Timestamp: 2025-06-04T19:35:20.209Z
Learning: MostroService methods like takeBuyOrder() and takeSellOrder() return Future<void> and trigger side effects through other mechanisms rather than direct return values. When testing these methods, focus on verifying method calls and testing state changes through the provider system rather than mocking return values.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
📚 Learning: 2025-07-20T23:33:17.689Z
Learnt from: Catrya
PR: MostroP2P/mobile#215
File: lib/features/order/notfiers/order_notifier.dart:0-0
Timestamp: 2025-07-20T23:33:17.689Z
Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Applied to files:
docs/architecture/TIMEOUT_DETECTION_AND_SESSION_CLEANUP.md
📚 Learning: 2025-09-18T17:17:38.944Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T17:17:38.944Z
Learning: Applies to lib/main.dart : Configure timeago locale messages initialization in main.dart
Applied to files:
CLAUDE.md
⏰ 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)
docs/architecture/APP_INITIALIZATION_ANALYSIS.md (1)
344-349: LGTM!The documentation clearly explains why
fireImmediately: falseis used in the SubscriptionManager initialization, aligning well with the PR's goal of preventing orphan sessions.lib/features/order/notfiers/abstract_mostro_notifier.dart (4)
23-24: Good design: Static timer storage prevents memory leaks.Using a static map for timer storage ensures timers persist across notifier instances and can be properly cleaned up.
95-96: Timer cancellation logic looks correct.The timer is now properly cancelled using the
orderIddirectly, which matches the key used in the timer map. This fixes the critical issue where timers were never cancelled.
286-305: Verify the timer duration aligns with PR objectives.The code uses a 10-second timer, but the PR description mentions a "30s timer cleanup". Please confirm which duration is correct and update either the code or documentation accordingly.
Additionally, consider extracting the timer duration as a named constant for better maintainability:
+ static const Duration _orphanSessionTimeout = Duration(seconds: 10); + static void startSessionTimeoutCleanup(String orderId, Ref ref) { // Cancel existing timer if any _sessionTimeouts[orderId]?.cancel(); - _sessionTimeouts[orderId] = Timer(const Duration(seconds: 10), () { + _sessionTimeouts[orderId] = Timer(_orphanSessionTimeout, () {
335-337: Good cleanup implementation in dispose.The disposal logic properly cancels and removes the timer for the specific orderId, preventing memory leaks.
fix #64
Summary by CodeRabbit