fix(dm): handle NewOrder book republish after pre-Active taker cancel#74
Conversation
When Mostro republishes a listing via Action::NewOrder (not Canceled), makers revert to pending and stay visible in My Trades via an event-driven SQLite cache; takers drop stale take rows from Messages. Extract helpers for clarity and allow WaitingTakerBond to revert to Pending. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a maker-book cache to AppState with DB→row projection helpers, populates it at startup, exposes a signaling sender to request cache refresh from background DM logic, updates the main loop to refresh on specific OperationResult values, and refactors trade-DM handling to special-case NewOrder/pre-Active flows. ChangesMaker-book cache and UI integration
OperationResult signaling and main wiring
Trade DM and NewOrder handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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)
src/ui/helpers/startup.rs (1)
121-132:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlways refresh maker-book cache even when startup active-orders query fails.
The early return on Line 130 can bypass Line 152, so maker
pendingrows are never loaded in that failure path.Suggested fix
- let Ok(rows) = Order::get_startup_active_orders(pool).await else { - return; - }; + let rows = match Order::get_startup_active_orders(pool).await { + Ok(rows) => rows, + Err(e) => { + log::warn!("Failed to load startup active orders: {}", e); + Vec::new() + } + };Also applies to: 148-153
🤖 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 `@src/ui/helpers/startup.rs` around lines 121 - 132, The startup function load_user_order_chats_at_startup currently returns early if Order::get_startup_active_orders(pool).await fails, which prevents loading the maker-book pending rows; remove or change that early return so that failure to get active orders does not skip the maker-book refresh — keep calling the maker pending/cache refresh logic (the code that loads maker `pending` rows and updates the maker-book cache) even when Order::get_startup_active_orders returns Err, e.g. handle the Result with a match or if let that logs the error but continues to run the maker-book loading path; ensure sync_user_order_history_messages_from_db(pool, app).await remains called as-is and that any references to Order::get_startup_active_orders or its result are guarded so they don’t prevent the maker pending cache update.
🤖 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 `@src/ui/helpers/order_chat_projection.rs`:
- Around line 179-184: The helper active_order_chat_list_len currently swallows
a poisoned app.messages.lock and returns 0; change it to treat a poisoned lock
as a fatal internal error like other code paths: when app.messages.lock()
returns Err(e) log the error (including e) and abort the process (panic or
std::process::exit(1)) instead of returning 0 so the UI does not continue in an
inconsistent state; apply the same change pattern to the other projection
helpers that lock app.messages (the block around lines 188-193) and keep calls
to build_active_order_chat_list and app.my_trades_maker_book intact.
---
Outside diff comments:
In `@src/ui/helpers/startup.rs`:
- Around line 121-132: The startup function load_user_order_chats_at_startup
currently returns early if Order::get_startup_active_orders(pool).await fails,
which prevents loading the maker-book pending rows; remove or change that early
return so that failure to get active orders does not skip the maker-book refresh
— keep calling the maker pending/cache refresh logic (the code that loads maker
`pending` rows and updates the maker-book cache) even when
Order::get_startup_active_orders returns Err, e.g. handle the Result with a
match or if let that logs the error but continues to run the maker-book loading
path; ensure sync_user_order_history_messages_from_db(pool, app).await remains
called as-is and that any references to Order::get_startup_active_orders or its
result are guarded so they don’t prevent the maker pending cache update.
🪄 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: 9e891980-8939-4136-8571-fba0af872dcb
📒 Files selected for processing (16)
src/main.rssrc/ui/app_state.rssrc/ui/helpers/mod.rssrc/ui/helpers/order_chat_projection.rssrc/ui/helpers/startup.rssrc/ui/key_handler/chat_helpers.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/navigation.rssrc/ui/operation_result.rssrc/ui/orders.rssrc/ui/tabs/order_in_progress_tab.rssrc/util/dm_utils/mod.rssrc/util/dm_utils/order_ch_mng.rssrc/util/dm_utils/order_result_tx.rssrc/util/mod.rssrc/util/order_utils/helper.rs
There was a problem hiding this comment.
I found a blocker in the new Action::NewOrder handling on the trade-DM path.
Right now handle_trade_dm_for_order() does this:
if matches!(action, Action::NewOrder) {
let _ = try_handle_new_order_trade_dm(...).await;
return;
}But try_handle_new_order_trade_dm() only consumes a very specific subset of NewOrder messages:
Payload::Order- with
status == pending - and then only the republish / range-child cases it knows how to handle
For every other NewOrder trade DM, the helper returns false and the caller still returns immediately, so the message is silently dropped.
That means this PR changes behavior from:
- "ignore
NewOrderonly by the previous explicit rule"
to:
- "always short-circuit
NewOrder, even when the new helper did not actually handle it"
So if Mostro emits any valid NewOrder trade DM outside this exact pending republish shape, it will now be discarded with no fallback path.
I think the caller must only early-return when try_handle_new_order_trade_dm() returns true, and otherwise continue through the normal trade-DM flow (or keep the previous ignore behavior explicitly limited to the cases you are sure about).
Because the new branch currently broadens silent dropping beyond the intended republish fix, I am marking this as request changes.
|
Thanks, I understand the intended protocol case better now. My concern is not that My concern is narrower: the current caller short-circuits all trade-DM So the code currently expresses:
not:
If the protocol guarantees that trade-DM So I am happy to re-evaluate if the guarantee is:
Without that, I still think the current unconditional early return is too broad for a correctness-sensitive DM path. |
|
Tested and fixed mostronator rant, LGTM! |
Summary
Trade DM listener: Handle Action::NewOrder with Payload::Order + status: pending on the trade-DM path (not only the create-order waiter). Pre-Active taker takes are deleted and removed from Messages; pre-Active maker listings revert to pending, drop from Messages, and refresh the My Trades sidebar cache.
My Trades UX: Event-driven my_trades_maker_book cache (SQLite maker + pending) merged into build_active_order_chat_list so republished listings stay visible without per-frame DB reads. OperationResult::MyTradesMakerBookChanged triggers a silent cache refresh.
Refactors: Extract helpers (try_handle_new_order_trade_dm, revert_maker_to_pending_on_book_republish, drop_pre_active_taker_take, projection snapshot helpers) for clarity and DRY.
Status transitions: WaitingTakerBond can revert to Pending; FiatSentOk maps to Status::FiatSent in inferred status.
Motivation
When a taker cancels before Active, Mostro often republishes the order with NewOrder rather than Canceled. The listener previously ignored all trade-DM NewOrder messages, so makers did not revert to book state and My Trades could lose the listing after Messages cleanup.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation