fix: cleanup session for pending_order_exists cant-do responses#287
Conversation
- Add specific session cleanup for order taking failures - When cant-do reason is pending_order_exists, delete session by orderId - Prevents phantom orders from appearing in "My Trades" after failed order attempts - Separates cleanup logic: requestId for order creation, orderId for order taking
WalkthroughAdds a new cleanup branch in Mostro notifier: when cantDoReason is pendingOrderExists, it deletes the session for the given orderId via sessionNotifierProvider.notifier.deleteSession(orderId). Existing cleanup for outOfRangeSatsAmount remains unchanged. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant MN as MostroNotifier
participant SN as SessionNotifier
U->>MN: Take order
MN-->>MN: Process response
alt cantDo: pendingOrderExists (new)
MN->>SN: deleteSession(orderId)
SN-->>MN: Session deleted
else cantDo: outOfRangeSatsAmount (existing)
MN->>SN: deleteSession(orderId)
SN-->>MN: Session deleted
end
MN-->>U: Return cantDo result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/features/order/notfiers/abstract_mostro_notifier.dart (2)
311-314: Also clear persisted messages for the attempted order (prevents stale badges/threads)If any messages were cached before the cant_do arrives, they can leave residual chat state. Consider aligning this branch with the canceled path by clearing message storage as well.
Apply:
if (cantDo?.cantDoReason == CantDoReason.pendingOrderExists) { + ref.read(mostroStorageProvider).deleteAllMessagesByOrderId(orderId); ref.read(sessionNotifierProvider.notifier).deleteSession(orderId); }
13-17: Nit: Folder name typoThe path segment “notfiers” looks like a typo of “notifiers.” Consider renaming in a follow-up to avoid future grep/import surprises.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/features/order/notfiers/abstract_mostro_notifier.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{lib/*.dart,lib/!(generated)/**/*.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
{lib/*.dart,lib/!(generated)/**/*.dart}: Use localized strings; replace hardcoded user-facing text withS.of(context).keyName
Preferconstconstructors andconstwhere possible
Use latest Flutter/Dart APIs (e.g.,withValues()instead of deprecatedwithOpacity())
Files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
{lib/*.dart,lib/!(generated)/**/*.dart,test/**/*.dart}: Checkmountedbefore usingBuildContextafterawait(async gaps)
Remove unused imports and dependencies
Files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
All code comments, variable names, and function names must be in English
Files:
lib/features/order/notfiers/abstract_mostro_notifier.dart
🧠 Learnings (1)
📓 Common learnings
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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
lib/features/order/notfiers/abstract_mostro_notifier.dart (3)
311-314: Targeted cleanup for pendingOrderExists — LGTMDeleting the session by orderId on cant_do: pendingOrderExists matches the PR objective and should remove the phantom entry from “My Trades.” Good separation from requestId-based cleanup.
311-314: Double-check we’re deleting the “attempted” order’s sessionIn this notifier the orderId corresponds to the order being taken (the one that failed). That’s the session we want to drop—not the already-pending order that triggered the cant_do. Please confirm events for pendingOrderExists here are indeed tied to the attempted order’s id (current notifier) in all transports.
18-19: Potential LateInitializationError:sessionmay be uninitialized
late Session session;is assigned only whenoldSession != null. Several branches dereferencesession(saveSession(session),session.role) and could throw if no previous session exists. If construction without a preexisting session is possible in any flow, this is a crash risk.Options:
- Ensure a session is always created before this notifier is constructed.
- Or initialize a default session in the constructor when
oldSessionis null.- Or remove the field and always fetch from
sessionNotifierProviderat use sites.
Do you want a patch for one of these approaches?Also applies to: 32-37, 128-129, 200-207, 228-236, 338-345
fix #248
Summary by CodeRabbit