Skip to content

fix: improve canceled order cleanup and timeout detection logic#264

Merged
Catrya merged 2 commits intomainfrom
improve-timeout-detection
Aug 7, 2025
Merged

fix: improve canceled order cleanup and timeout detection logic#264
Catrya merged 2 commits intomainfrom
improve-timeout-detection

Conversation

@Catrya
Copy link
Member

@Catrya Catrya commented Aug 6, 2025

  • Fix early return bug in _subscribeToPublicEvents that left _isProcessingTimeout=true permanently
  • Simplify timeout detection: remove timestamp comparison, use status-based logic
  • Separate cancellation logic from timeout logic for better reliability
  • Only delete sessions for pending/waiting canceled orders, preserve active canceled orders
  • Update documentation to reflect simplified detection mechanism

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability of order timeout and cancellation detection by removing timestamp dependencies and simplifying status-based logic.
    • Refined session cleanup: pending/waiting orders are removed on cancellation, while active orders retain sessions and show canceled status.
    • Enhanced notification behavior for timeouts and cancellations.
  • Documentation

    • Updated documentation to clarify the simplified timeout and cancellation detection approach and session handling rules.

  - Fix early return bug in _subscribeToPublicEvents that left _isProcessingTimeout=true permanently
  - Simplify timeout detection: remove timestamp comparison, use status-based logic
  - Separate cancellation logic from timeout logic for better reliability
  - Only delete sessions for pending/waiting canceled orders, preserve active canceled orders
  - Update documentation to reflect simplified detection mechanism
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 6, 2025

Walkthrough

The timeout and cancellation detection logic for order processing has been refactored to use status and state comparisons instead of timestamp-based checks. The OrderNotifier class now uses separate flags for syncing and timeout processing, and session cleanup behavior is determined by specific order states and user roles. Documentation was updated accordingly.

Changes

Cohort / File(s) Change Summary
OrderNotifier Logic Refactor
lib/features/order/notfiers/order_notifier.dart
Introduced _isSyncing flag for the sync() method, separated from _isProcessingTimeout. Refactored _checkTimeoutAndCleanup to remove timestamp checks, simplify cancellation and timeout detection using state/status comparison, and adjust session cleanup logic based on order state and role. Updated _subscribeToPublicEvents to set flags after state/session validation.
Documentation Update
CLAUDE.md
Updated documentation to describe the new state-based timeout and cancellation detection, clarified session cleanup rules, and detailed UI behavior distinctions. Removed references to timestamp-based logic.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant OrderNotifier
    participant PublicEventStream
    participant SessionManager

    User->>OrderNotifier: Initiate sync()
    OrderNotifier->>OrderNotifier: Check _isSyncing flag
    alt Not syncing
        OrderNotifier->>OrderNotifier: Set _isSyncing = true
        OrderNotifier->>OrderNotifier: Iterate stored messages
        OrderNotifier->>OrderNotifier: (Timeout cleanup removed)
        OrderNotifier->>OrderNotifier: Complete sync
        OrderNotifier->>OrderNotifier: Set _isSyncing = false
    end

    PublicEventStream->>OrderNotifier: New public event
    OrderNotifier->>OrderNotifier: Check current state/session
    OrderNotifier->>OrderNotifier: Set _isProcessingTimeout = true
    alt Cancellation detected
        alt State is pending/waiting
            OrderNotifier->>SessionManager: Delete session
        else Active state
            OrderNotifier->>OrderNotifier: Update state to canceled
        end
        OrderNotifier->>User: Show cancellation notification
    else Timeout detected (pending/waiting mismatch)
        alt Maker
            OrderNotifier->>OrderNotifier: Update state to pending, persist reversal
        else Taker
            OrderNotifier->>SessionManager: Delete session
        end
        OrderNotifier->>User: Show timeout notification
    end
    OrderNotifier->>OrderNotifier: Set _isProcessingTimeout = false
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • chebizarro

Poem

A hop and a skip, the logic is new,
No more chasing timestamps, just states to review!
Orders now dance in a simpler parade,
With sessions and flags neatly remade.
From pending to canceled, the rules are now clear—
This bunny’s delighted, let’s all give a cheer! 🐇✨

Note

🔌 MCP (Model Context Protocol) integration is now available in Early Access!

Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d57e4e and 224e50d.

📒 Files selected for processing (1)
  • lib/features/order/notfiers/order_notifier.dart (9 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
lib/**/*.dart

📄 CodeRabbit Inference Engine (CLAUDE.md)

lib/**/*.dart: Use S.of(context).yourKey for all user-facing strings
Always check mounted before using context after async operations

Files:

  • lib/features/order/notfiers/order_notifier.dart
**/*.dart

📄 CodeRabbit Inference Engine (CLAUDE.md)

**/*.dart: Remove unused imports and dependencies
Use const constructors where possible

Files:

  • lib/features/order/notfiers/order_notifier.dart
🧠 Learnings (11)
📓 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.
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.
Learnt from: chebizarro
PR: MostroP2P/mobile#110
File: lib/features/trades/widgets/mostro_message_detail_widget.dart:134-141
Timestamp: 2025-06-08T23:54:09.987Z
Learning: In the OrderState refactor, public keys should be accessed via `tradeState.peer?.publicKey` from the OrderState instance rather than through `session?.peer?.publicKey`, as the OrderState encapsulates peer information directly.
📚 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/order_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:

  • lib/features/order/notfiers/order_notifier.dart
📚 Learning: 2025-08-03T09:43:38.727Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T09:43:38.727Z
Learning: Applies to lib/core/mostro_fsm.dart : Order state transitions must be managed by `core/mostro_fsm.dart`

Applied to files:

  • lib/features/order/notfiers/order_notifier.dart
📚 Learning: 2025-08-03T09:43:38.727Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T09:43:38.727Z
Learning: Encapsulate business logic in Notifiers

Applied to files:

  • lib/features/order/notfiers/order_notifier.dart
📚 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/order_notifier.dart
📚 Learning: 2025-05-08T16:29:52.154Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/background/background.dart:74-77
Timestamp: 2025-05-08T16:29:52.154Z
Learning: In the Mostro Mobile background service architecture, events aren't stored by the background process. Instead, the background service only checks if events exist in the eventStore and sends notifications for new ones, while the foreground process is responsible for storing and processing events. This is an intentional design decision to separate concerns.

Applied to files:

  • lib/features/order/notfiers/order_notifier.dart
📚 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/order_notifier.dart
📚 Learning: 2025-06-08T23:54:09.987Z
Learnt from: chebizarro
PR: MostroP2P/mobile#110
File: lib/features/trades/widgets/mostro_message_detail_widget.dart:134-141
Timestamp: 2025-06-08T23:54:09.987Z
Learning: In the OrderState refactor, public keys should be accessed via `tradeState.peer?.publicKey` from the OrderState instance rather than through `session?.peer?.publicKey`, as the OrderState encapsulates peer information directly.

Applied to files:

  • lib/features/order/notfiers/order_notifier.dart
📚 Learning: 2025-05-06T15:49:26.443Z
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.

Applied to files:

  • lib/features/order/notfiers/order_notifier.dart
📚 Learning: 2025-08-03T09:43:38.727Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T09:43:38.727Z
Learning: Applies to lib/**/*.dart : Always check `mounted` before using context after async operations

Applied to files:

  • lib/features/order/notfiers/order_notifier.dart
⏰ 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 (4)
lib/features/order/notfiers/order_notifier.dart (4)

15-16: LGTM! Clear separation of concerns.

The introduction of separate flags for syncing and timeout processing improves code maintainability and prevents race conditions between these operations.


53-53: LGTM! Proper guard pattern implementation.

The _isSyncing flag correctly prevents concurrent sync operations with proper flag management in the try-finally block.

Also applies to: 56-56, 90-90


172-282: Excellent refactoring! Simplified and more reliable logic.

The separation of cancellation and timeout detection logic is a significant improvement:

  1. Cancellation detection (lines 172-211): Now status-based and handles session preservation correctly based on order states
  2. Timeout detection (lines 214-282): Simplified to pure status comparison without timestamp dependencies
  3. Session management: Refined to only delete sessions for pending/waiting canceled orders while preserving active ones

The maker/taker distinction and timeout reversal message persistence ensure proper state management across app restarts.


329-329: LGTM! Proper state verification for cancellation detection.

The expanded state checks to include Status.pending correctly support the new cancellation detection logic. The dual verification (before and after message retrieval) ensures robust state checking throughout the timeout processing flow.

Also applies to: 336-340, 354-356

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch improve-timeout-detection

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

  - Move critical validations before _isProcessingTimeout flag to prevent permanent blocking
  - Add Status.pending to cancellation detection filters in _subscribeToPublicEvents
  - Enable cleanup for pending→canceled orders that were previously ignored
  - Maintain separation between timeout logic (waiting states only) and cancellation logic
@Catrya Catrya merged commit e5ae43a into main Aug 7, 2025
2 checks passed
@Catrya Catrya deleted the improve-timeout-detection branch August 8, 2025 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant