feat(ui): hold-invoice YES/NO/CANCEL popup and cooperative cancel#54
Merged
arkanoider merged 14 commits intomainfrom Apr 26, 2026
Merged
feat(ui): hold-invoice YES/NO/CANCEL popup and cooperative cancel#54arkanoider merged 14 commits intomainfrom
arkanoider merged 14 commits intomainfrom
Conversation
- Add ViewingMessageButtonSelection (Two/Three) for Messages ViewingMessage - HoldInvoicePaymentAccepted: trinary UI (FiatSent / dismiss / cooperative Cancel) - Copy in VIEW_MESSAGE_HOLD_INVOICE_PREVIEW; Left/Right cycles three choices - render_yes_no_cancel_buttons; tab_content multi-line Text and tight popup height - execute_send_msg: accept CooperativeCancelInitiatedByYou after Cancel DM Made-with: Cursor
Contributor
|
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:
WalkthroughAdds tri-state (YES/NO/CANCEL) message confirmation and invoice-popup action selection, new button rendering/layout, DB-backed order-history sync and single/bulk deletion flows, expanded key/mouse/paste handling for invoice input, DM handling safeguards, and send-message validation updates for cooperative-cancel semantics. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KeyHandler as Key Handler
participant UI as UI Renderer
participant MsgHandler as Message Handlers
participant Send as Send Message
participant DB as Database
User->>KeyHandler: Left/Right (select) or Enter on popup
KeyHandler->>KeyHandler: Update ViewingMessageButtonSelection / Invoice action_selection
KeyHandler->>UI: Request re-render
rect rgba(100,200,150,0.5)
UI->>UI: Compute multi-line layout and buttons
UI->>User: Render YES / NO [/ CANCEL] buttons
end
User->>KeyHandler: Press Enter
KeyHandler->>MsgHandler: handle_enter_viewing_message()
alt Selection = CANCEL
MsgHandler->>Send: execute_send_msg(Action::Cancel)
Send-->>MsgHandler: result
MsgHandler->>UI: Emit OperationResult::Info
else Selection = YES (primary)
MsgHandler->>Send: execute_send_msg(primary action)
Send-->>MsgHandler: result
MsgHandler->>UI: Emit success OperationResult (maybe include static_header)
else Selection = NO / dismiss
MsgHandler->>UI: Dismiss popup
end
opt Deletion confirmed (single/bulk)
KeyHandler->>DB: async delete_terminal_order_by_id / delete_bulk_history_cleanup_orders
DB-->>KeyHandler: deleted IDs
KeyHandler->>MsgHandler: Emit OperationResult::OrderHistoryDeleted
MsgHandler->>UI: Remove messages and update order_chat_static
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
Collaborator
Author
|
@coderabbitai review please! |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/key_handler/mod.rs`:
- Around line 796-808: The Two-button branch currently toggles the yes_selected
flag and overrides directional navigation; in UiMode::ViewingMessage when
matching ViewingMessageButtonSelection::Two { yes_selected } change the logic to
set yes_selected = true when KeyCode::Left is pressed and yes_selected = false
when KeyCode::Right is pressed (leaving other keys untouched) so directional
Left=>YES and Right=>NO behavior is preserved and not inverted before
handle_navigation runs; locate the button_selection handling in
UiMode::ViewingMessage and replace the toggle with explicit directional
assignments for KeyCode::Left and KeyCode::Right.
🪄 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: 3b293c9a-75a1-465d-9a31-097f22585ef7
📒 Files selected for processing (13)
src/ui/constants.rssrc/ui/helpers/layout.rssrc/ui/helpers/mod.rssrc/ui/key_handler/chat_helpers.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/message_handlers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/navigation.rssrc/ui/mod.rssrc/ui/orders.rssrc/ui/state.rssrc/ui/tabs/tab_content.rssrc/util/order_utils/execute_send_msg.rs
…ab - added cancel for single order or all completed ones
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/ui/helpers/order_chat_projection.rs (1)
77-79: Dead code:is_order_chat_actionable+ filter call are now no-ops.Since the function unconditionally returns
true, the.filter(...)inbuild_active_order_chat_list(L120) never excludes a row. You can drop both the function and the filter to remove the unused_statusparameter and keep intent clear. If broader-status visibility in My Trades is the intended behavior, consider a brief doc comment onbuild_active_order_chat_listexplaining that sidebar entries are no longer restricted byStatus(so future readers don't reintroduce filtering here).♻️ Proposed cleanup
-fn is_order_chat_actionable(_status: Option<Status>) -> bool { - true -} - /// Shared projection for the "My Trades" sidebar and Enter/action handlers. /// /// Important: ordering must stay stable and match the sidebar ordering, otherwise /// `selected_order_chat_idx` can desync from the action target. pub fn build_active_order_chat_list(messages: &[OrderMessage]) -> Vec<OrderChatListItem> { @@ - let mut rows: Vec<OrderChatListItem> = by_order - .into_values() - .filter(|row| is_order_chat_actionable(row.status)) - .collect(); + let mut rows: Vec<OrderChatListItem> = by_order.into_values().collect();Also applies to: 118-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/helpers/order_chat_projection.rs` around lines 77 - 79, The helper function is_order_chat_actionable always returns true, making the .filter(...) call in build_active_order_chat_list a no-op; remove the dead function is_order_chat_actionable and delete the corresponding .filter(...) invocation inside build_active_order_chat_list (or, if the intent is to deliberately allow all statuses, remove the function/filter and add a brief doc comment on build_active_order_chat_list stating that sidebar entries are no longer restricted by Status so future maintainers don't reintroduce filtering). Ensure any imports or references to is_order_chat_actionable are also cleaned up.src/ui/key_handler/async_tasks.rs (1)
50-124: Consolidate the twoclear_runtime_*helpers.
clear_runtime_tracking_state_preserve_messagesis a near-verbatim copy ofclear_runtime_session_stateminus themessages.clear()branch, including the identical poisoned-lock fatal-restart boilerplate foractive_order_trade_indicesandpending_notifications. A single parameterized helper keeps the fatal-handling logic in one place so future changes (e.g., to the error message text orfatal_exit_on_closesemantics) don't have to be mirrored.♻️ Proposed consolidation
-fn clear_runtime_session_state(app: &mut AppState) { - match app.messages.lock() { - Ok(mut messages) => { - messages.clear(); - } - Err(e) => { - request_fatal_restart(format!( - "Mostrix encountered an internal error (poisoned messages lock: {e}). Please restart the app." - )); - app.fatal_exit_on_close = true; - app.mode = UiMode::OperationResult(OperationResult::Error( - "Internal error. Please restart Mostrix.".to_string(), - )); - return; - } - } - match app.active_order_trade_indices.lock() { - Ok(mut active) => active.clear(), - Err(e) => { /* ...repeat... */ } - } - match app.pending_notifications.lock() { - Ok(mut pending) => *pending = 0, - Err(e) => { /* ...repeat... */ } - } - app.selected_message_idx = 0; - app.pending_post_take_operation_result = None; -} - -fn clear_runtime_tracking_state_preserve_messages(app: &mut AppState) { - /* duplicated body minus messages.clear() */ -} +fn set_fatal_poisoned(app: &mut AppState, what: &str, e: impl std::fmt::Display) { + request_fatal_restart(format!( + "Mostrix encountered an internal error (poisoned {what} lock: {e}). Please restart the app." + )); + app.fatal_exit_on_close = true; + app.mode = UiMode::OperationResult(OperationResult::Error( + "Internal error. Please restart Mostrix.".to_string(), + )); +} + +fn clear_runtime_state(app: &mut AppState, clear_messages: bool) { + if clear_messages { + match app.messages.lock() { + Ok(mut m) => m.clear(), + Err(e) => return set_fatal_poisoned(app, "messages", e), + } + } + match app.active_order_trade_indices.lock() { + Ok(mut active) => active.clear(), + Err(e) => return set_fatal_poisoned(app, "active order indices", e), + } + match app.pending_notifications.lock() { + Ok(mut pending) => *pending = 0, + Err(e) => return set_fatal_poisoned(app, "pending notifications", e), + } + app.selected_message_idx = 0; + app.pending_post_take_operation_result = None; +}Callers then become
clear_runtime_state(app, true)/clear_runtime_state(ctx.app, false).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/async_tasks.rs` around lines 50 - 124, Replace the two near-duplicate functions clear_runtime_session_state and clear_runtime_tracking_state_preserve_messages with a single parameterized helper clear_runtime_state(app: &mut AppState, preserve_messages: bool) that performs the same locked operations on messages.lock(), active_order_trade_indices.lock(), and pending_notifications.lock(); when preserve_messages is false call messages.clear(), otherwise leave messages untouched; preserve the existing poisoned-lock handling by calling request_fatal_restart(...) and setting app.fatal_exit_on_close = true and app.mode = UiMode::OperationResult(OperationResult::Error(...)) on Err for each lock, returning immediately after those fatal branches where the originals returned; keep the final resets (app.selected_message_idx = 0 and app.pending_post_take_operation_result = None) and update all call sites to use clear_runtime_state(app, false) or clear_runtime_state(app, true) as appropriate.src/ui/helpers/startup.rs (1)
238-250: Minor: redundant sort.
history_messages.sort_by(...)at Line 242 is unused — the merged vector is re-sorted at Line 250 afterpushanyway. You can drop the first sort without affecting ordering.♻️ Diff
- history_messages.sort_by(|a, b| b.timestamp.cmp(&a.timestamp)); - match app.messages.lock() { Ok(mut messages) => { for msg in history_messages { messages.retain(|m| m.order_id != msg.order_id); messages.push(msg); } messages.sort_by(|a, b| b.timestamp.cmp(&a.timestamp)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/helpers/startup.rs` around lines 238 - 250, The initial sort on history_messages is redundant because after merging into app.messages (inside the match on app.messages.lock()) you re-sort the combined messages; remove the call to history_messages.sort_by(|a, b| b.timestamp.cmp(&a.timestamp)) so only the final messages.sort_by(...) remains, leaving db_order_to_history_message, history_messages, and the messages lock/merge logic unchanged.src/util/dm_utils/mod.rs (1)
693-711: Dead branch insideshould_replace_row.Line 454–456 already short-circuits when
action == Action::NewOrder, somatches!(action, Action::NewOrder)at Line 700 is never true when this match arm runs. The guard is effectively dead code and the accompanying comment is stale.Either drop the NewOrder check here (preferred — the early return already enforces the invariant) or remove the early return and rely on this guard to keep both points in sync. As written, future readers may assume both are required.
♻️ Suggested simplification
let should_replace_row = match &existing_message_data { None => true, - Some((existing_timestamp, existing_action, _, _, _, _, _, _)) => { - // Never let replayed `NewOrder` replace an already-established trade row. - // Otherwise Messages can regress to only NewOrder rows, which the UI strips. - if matches!(action, Action::NewOrder) && !matches!(existing_action, Action::NewOrder) { - false - } else if timestamp > *existing_timestamp { + Some((existing_timestamp, existing_action, _, _, _, _, _, _)) => { + if timestamp > *existing_timestamp { true } else if timestamp == *existing_timestamp { action != *existing_action } else { // Older-than-current replay: only replace if it advances accepted status. should_accept_candidate } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/dm_utils/mod.rs` around lines 693 - 711, The match arm computing should_replace_row contains a dead check for matches!(action, Action::NewOrder) because an earlier short-circuit already prevents action == Action::NewOrder; remove that branch and its stale comment and simplify the arm to: compare timestamps (timestamp > existing_timestamp => true, timestamp == existing_timestamp => action != *existing_action, else => should_accept_candidate). Update the block that references existing_message_data, should_replace_row, action, existing_action, timestamp, and should_accept_candidate accordingly so the logic is consistent and the obsolete comment/branch is removed.src/models.rs (1)
201-213: Optional: add a drift test forTERMINAL_ORDER_HISTORY_STATUSES.The file already has
terminal_dm_statuses_match_mostro_core_display(Lines 1017–1039) guardingTERMINAL_DM_STATUSESagainst silent drift frommostro_core::order::Status'sDisplay. The two new delete helpers gate onlower(status) IN (...), so any future renaming/normalization ofStatus::to_string()would silently stop matching rows without a compile-time or test failure. A mirror test is cheap insurance.🧪 Suggested additional test
#[test] fn terminal_order_history_statuses_match_mostro_core_display() { let variants = [ Status::Success, Status::Canceled, Status::CanceledByAdmin, Status::SettledByAdmin, Status::CompletedByAdmin, Status::Expired, Status::CooperativelyCanceled, ]; let expected: Vec<String> = variants.iter().map(ToString::to_string).collect(); let actual: Vec<String> = super::TERMINAL_ORDER_HISTORY_STATUSES .iter() .map(|s| (*s).to_string()) .collect(); assert_eq!( expected, actual, "TERMINAL_ORDER_HISTORY_STATUSES must match mostro_core::order::Status::to_string()" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models.rs` around lines 201 - 213, Add a mirror unit test (similar to terminal_dm_statuses_match_mostro_core_display) that verifies TERMINAL_ORDER_HISTORY_STATUSES exactly matches the stringified mostro_core::order::Status variants: implement a test named terminal_order_history_statuses_match_mostro_core_display which builds the expected Vec<String> by mapping the Status variants (Success, Canceled, CanceledByAdmin, SettledByAdmin, CompletedByAdmin, Expired, CooperativelyCanceled) to strings and compares it to the actual Vec<String> from super::TERMINAL_ORDER_HISTORY_STATUSES, using assert_eq! with a clear message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 355-358: Currently sync_user_order_history_messages_from_db(&pool,
&mut app) is being called on every order_result_rx event (after
handle_operation_result), causing expensive DB work and message contention;
change this so the sync is only triggered for history-mutating OperationResult
variants (e.g. OrderHistoryDeleted) or via a dedicated flag/channel instead of
unconditionally checking app.user_role for every event. Locate the branch that
calls handle_operation_result and the unconditional
sync_user_order_history_messages_from_db call, and gate that call by matching
the OperationResult variant (or emit a separate history-sync signal from where
history deletions occur) so only genuine history mutations invoke
sync_user_order_history_messages_from_db.
In `@src/ui/helpers/startup.rs`:
- Around line 219-258: sync_user_order_history_messages_from_db currently
converts all persisted orders (via get_user_history_orders) into synthesized
OrderMessage entries (db_order_to_history_message) and unconditionally replaces
any existing message with the same order_id, which overwrites active DM-driven
actions; restrict projections to terminal orders or preserve fresher live
messages and remove a redundant sort. Fix by filtering the rows returned from
get_user_history_orders inside sync_user_order_history_messages_from_db to only
include statuses in TERMINAL_ORDER_HISTORY_STATUSES (or, alternatively, when
merging history_messages into app.messages.lock() check existing messages and
skip replacing if an existing message for the same order_id has a timestamp >=
msg.timestamp), reference db_order_to_history_message and OrderMessage when
building the history list, and remove the redundant
history_messages.sort_by(...) (keep only the final messages.sort_by(...) before
releasing the lock).
In `@src/ui/key_handler/mod.rs`:
- Around line 701-716: The bulk-delete confirmation currently pre-selects YES by
constructing UiMode::ConfirmBulkDeleteHistory(true), which is unsafe; change the
creation to UiMode::ConfirmBulkDeleteHistory(false) so the default focus is NO
when Ctrl+Delete is pressed (leave single-order behavior
UiMode::ConfirmDeleteHistoryOrder(order_id, true) unchanged), i.e., update the
branch in the key handler that creates ConfirmBulkDeleteHistory to pass false
instead of true.
---
Nitpick comments:
In `@src/models.rs`:
- Around line 201-213: Add a mirror unit test (similar to
terminal_dm_statuses_match_mostro_core_display) that verifies
TERMINAL_ORDER_HISTORY_STATUSES exactly matches the stringified
mostro_core::order::Status variants: implement a test named
terminal_order_history_statuses_match_mostro_core_display which builds the
expected Vec<String> by mapping the Status variants (Success, Canceled,
CanceledByAdmin, SettledByAdmin, CompletedByAdmin, Expired,
CooperativelyCanceled) to strings and compares it to the actual Vec<String> from
super::TERMINAL_ORDER_HISTORY_STATUSES, using assert_eq! with a clear message.
In `@src/ui/helpers/order_chat_projection.rs`:
- Around line 77-79: The helper function is_order_chat_actionable always returns
true, making the .filter(...) call in build_active_order_chat_list a no-op;
remove the dead function is_order_chat_actionable and delete the corresponding
.filter(...) invocation inside build_active_order_chat_list (or, if the intent
is to deliberately allow all statuses, remove the function/filter and add a
brief doc comment on build_active_order_chat_list stating that sidebar entries
are no longer restricted by Status so future maintainers don't reintroduce
filtering). Ensure any imports or references to is_order_chat_actionable are
also cleaned up.
In `@src/ui/helpers/startup.rs`:
- Around line 238-250: The initial sort on history_messages is redundant because
after merging into app.messages (inside the match on app.messages.lock()) you
re-sort the combined messages; remove the call to history_messages.sort_by(|a,
b| b.timestamp.cmp(&a.timestamp)) so only the final messages.sort_by(...)
remains, leaving db_order_to_history_message, history_messages, and the messages
lock/merge logic unchanged.
In `@src/ui/key_handler/async_tasks.rs`:
- Around line 50-124: Replace the two near-duplicate functions
clear_runtime_session_state and clear_runtime_tracking_state_preserve_messages
with a single parameterized helper clear_runtime_state(app: &mut AppState,
preserve_messages: bool) that performs the same locked operations on
messages.lock(), active_order_trade_indices.lock(), and
pending_notifications.lock(); when preserve_messages is false call
messages.clear(), otherwise leave messages untouched; preserve the existing
poisoned-lock handling by calling request_fatal_restart(...) and setting
app.fatal_exit_on_close = true and app.mode =
UiMode::OperationResult(OperationResult::Error(...)) on Err for each lock,
returning immediately after those fatal branches where the originals returned;
keep the final resets (app.selected_message_idx = 0 and
app.pending_post_take_operation_result = None) and update all call sites to use
clear_runtime_state(app, false) or clear_runtime_state(app, true) as
appropriate.
In `@src/util/dm_utils/mod.rs`:
- Around line 693-711: The match arm computing should_replace_row contains a
dead check for matches!(action, Action::NewOrder) because an earlier
short-circuit already prevents action == Action::NewOrder; remove that branch
and its stale comment and simplify the arm to: compare timestamps (timestamp >
existing_timestamp => true, timestamp == existing_timestamp => action !=
*existing_action, else => should_accept_candidate). Update the block that
references existing_message_data, should_replace_row, action, existing_action,
timestamp, and should_accept_candidate accordingly so the logic is consistent
and the obsolete comment/branch is removed.
🪄 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: ba8574da-c793-476a-a976-b4d31c2c896a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
src/main.rssrc/models.rssrc/ui/app_state.rssrc/ui/constants.rssrc/ui/draw.rssrc/ui/helpers/layout.rssrc/ui/helpers/mod.rssrc/ui/helpers/order_chat_projection.rssrc/ui/helpers/startup.rssrc/ui/key_handler/async_tasks.rssrc/ui/key_handler/chat_helpers.rssrc/ui/key_handler/confirmation.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/esc_handlers.rssrc/ui/key_handler/message_handlers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/navigation.rssrc/ui/operation_result.rssrc/ui/orders.rssrc/ui/tabs/tab_content.rssrc/util/dm_utils/mod.rssrc/util/dm_utils/order_ch_mng.rssrc/util/types.rs
✅ Files skipped from review due to trivial changes (1)
- src/util/types.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/ui/helpers/mod.rs
- src/ui/constants.rs
- src/ui/key_handler/chat_helpers.rs
- src/ui/key_handler/enter_handlers.rs
- src/ui/key_handler/message_handlers.rs
Comment on lines
+219
to
+258
| pub async fn sync_user_order_history_messages_from_db(pool: &SqlitePool, app: &mut AppState) { | ||
| let identity_keys = match User::get_identity_keys(pool).await { | ||
| Ok(k) => k, | ||
| Err(e) => { | ||
| log::warn!( | ||
| "Failed to derive identity keys for DB history sender attribution: {}", | ||
| e | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
| let sender = identity_keys.public_key(); | ||
| let rows = match Order::get_user_history_orders(pool).await { | ||
| Ok(rows) => rows, | ||
| Err(e) => { | ||
| log::warn!("Failed to load user order history rows at startup: {}", e); | ||
| return; | ||
| } | ||
| }; | ||
| let mut history_messages: Vec<OrderMessage> = rows | ||
| .iter() | ||
| .filter_map(|row| db_order_to_history_message(row, sender)) | ||
| .collect(); | ||
| history_messages.sort_by(|a, b| b.timestamp.cmp(&a.timestamp)); | ||
|
|
||
| match app.messages.lock() { | ||
| Ok(mut messages) => { | ||
| for msg in history_messages { | ||
| messages.retain(|m| m.order_id != msg.order_id); | ||
| messages.push(msg); | ||
| } | ||
| messages.sort_by(|a, b| b.timestamp.cmp(&a.timestamp)); | ||
| } | ||
| Err(e) => { | ||
| crate::util::request_fatal_restart(format!( | ||
| "Mostrix encountered an internal error (poisoned messages lock: {e}). Please restart the app." | ||
| )); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find where sync_user_order_history_messages_from_db is called
rg "sync_user_order_history_messages_from_db" -A 3 -B 3Repository: MostroP2P/mostrix
Length of output: 2200
🏁 Script executed:
# Examine the get_user_history_orders implementation
fd ".*\.rs$" | xargs rg "fn get_user_history_orders" -A 20Repository: MostroP2P/mostrix
Length of output: 1213
🏁 Script executed:
# Examine db_order_to_history_message implementation
fd ".*\.rs$" | xargs rg "fn db_order_to_history_message" -A 30Repository: MostroP2P/mostrix
Length of output: 1995
🏁 Script executed:
# Check TERMINAL_ORDER_HISTORY_STATUSES and what statuses exist
rg "TERMINAL_ORDER_HISTORY_STATUSES|Terminal\s*Order\s*Status" -B 2 -A 5Repository: MostroP2P/mostrix
Length of output: 789
🏁 Script executed:
# Check what statuses get_user_history_orders might filter by
rg "get_user_history_orders|ORDER_HISTORY_STATUSES" -B 5 -A 15Repository: MostroP2P/mostrix
Length of output: 5981
🏁 Script executed:
# Check message_action_compact_label_for_message to see if it uses action for active statuses
rg "message_action_compact_label_for_message" -A 40Repository: MostroP2P/mostrix
Length of output: 10815
🏁 Script executed:
# Check how the Messages tab displays messages and whether it uses these labels
rg "Messages.*tab|message_action_compact_label" -B 3 -A 3Repository: MostroP2P/mostrix
Length of output: 21315
🏁 Script executed:
# Confirm the redundant sort issue (line 242 vs line 250)
sed -n '235,255p' src/ui/helpers/startup.rsRepository: MostroP2P/mostrix
Length of output: 825
Sync unconditionally overwrites active order DM actions with generic synthesized projections.
get_user_history_orders fetches all rows with persisted trade_keys — no status filter — so it includes active, non-terminal orders. For each row, db_order_to_history_message synthesizes action from kind alone (TakeBuy / TakeSell / WaitingSellerToPay), discarding any existing DM-driven action. The merge then unconditionally replaces:
for msg in history_messages {
messages.retain(|m| m.order_id != msg.order_id);
messages.push(msg);
}This function runs not only at startup but on every order_result_rx event for the User role. For any live trade in Active, Pending, FiatSent, InProgress, SettledHoldInvoice, or Dispute status, message_action_compact_label_for_message falls back to the action label. Overwriting a DM-driven row (e.g., HoldInvoicePaymentAccepted, AddInvoice, PayInvoice, FiatSent) with a projection whose action is TakeBuy / TakeSell causes the sidebar to regress from "Hold Invoice Accepted" / "Invoice Request" to "Take Buy" / "Take Sell" after each operation result, until a fresh DM arrives. Timeline step resolution is similarly affected when order_kind is None.
Suggested fix: Restrict the projection to terminal orders only (filter rows by TERMINAL_ORDER_HISTORY_STATUSES in sync_user_order_history_messages_from_db), or skip replacement when an existing row for the same order_id has a fresher timestamp.
Also, the sort at line 242 is redundant with the final sort at line 250; remove one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/helpers/startup.rs` around lines 219 - 258,
sync_user_order_history_messages_from_db currently converts all persisted orders
(via get_user_history_orders) into synthesized OrderMessage entries
(db_order_to_history_message) and unconditionally replaces any existing message
with the same order_id, which overwrites active DM-driven actions; restrict
projections to terminal orders or preserve fresher live messages and remove a
redundant sort. Fix by filtering the rows returned from get_user_history_orders
inside sync_user_order_history_messages_from_db to only include statuses in
TERMINAL_ORDER_HISTORY_STATUSES (or, alternatively, when merging
history_messages into app.messages.lock() check existing messages and skip
replacing if an existing message for the same order_id has a timestamp >=
msg.timestamp), reference db_order_to_history_message and OrderMessage when
building the history list, and remove the redundant
history_messages.sort_by(...) (keep only the final messages.sort_by(...) before
releasing the lock).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/ui/key_handler/message_handlers.rs (1)
79-82: Use the newrole_default_mode/role_waiting_modehelpers here too.
handle_enter_viewing_messagestill inlines the samematch app.user_roleblocks that were just extracted as helpers at Lines 13-25. Replacing the inline matches keeps role mapping in a single place.♻️ Proposed diff
- let default_mode = match app.user_role { - UserRole::User => UiMode::UserMode(UserMode::Normal), - UserRole::Admin => UiMode::AdminMode(AdminMode::Normal), - }; + let default_mode = role_default_mode(app.user_role); @@ - let default_mode = match app.user_role { - UserRole::User => UiMode::UserMode(UserMode::Normal), - UserRole::Admin => UiMode::AdminMode(AdminMode::Normal), - }; - app.mode = default_mode; + app.mode = role_default_mode(app.user_role); @@ - let waiting_mode = match app.user_role { - UserRole::User => UiMode::UserMode(UserMode::WaitingAddInvoice), - UserRole::Admin => UiMode::AdminMode(AdminMode::Normal), - }; - app.mode = waiting_mode; + app.mode = role_waiting_mode(app.user_role);Also applies to: 132-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/message_handlers.rs` around lines 79 - 82, Replace the duplicated role->mode match expressions in handle_enter_viewing_message by calling the existing helpers role_default_mode(...) and role_waiting_mode(...): instead of matching on app.user_role to build default_mode and the waiting_mode, call role_default_mode(app.user_role) for the default UiMode and role_waiting_mode(app.user_role) for the waiting UiMode; update both occurrences (the one creating default_mode and the other at the second occurrence around lines 132-135) to use these helpers so role mapping is centralized in role_default_mode and role_waiting_mode.src/ui/orders.rs (1)
265-272: Consider a typed variant instead ofselected: u8.The three states (YES/NO/CANCEL) are modeled as a raw
u8with the convention0|1|2. The current cycling insrc/ui/key_handler/mod.rskeeps it in range, but the Enter dispatch inmessage_handlers.rsrelies on magic literals (selected: 1,selected: 2) and a catch-all_pattern silently maps any out-of-range value toFiatSent. A dedicated enum (e.g.Yes | No | Cancel) would make invalid states unrepresentable and let the compiler enforce exhaustive match on the three-button dispatch path.♻️ Sketch
- /// Hold-invoice confirmation only: `0` = YES, `1` = NO, `2` = CANCEL (cooperative cancel). - Three { selected: u8 }, + /// Hold-invoice confirmation only: YES sends FiatSent, NO dismisses, CANCEL sends cooperative Cancel. + Three { selected: ThreeButton }, } + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ThreeButton { + Yes, + No, + Cancel, +}Callers in
navigation.rs/mod.rswould cycle vianext()/prev()on the enum, andmessage_handlers.rsmatches on named variants instead ofselected: 1 | 2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/orders.rs` around lines 265 - 272, Replace the raw Three { selected: u8 } variant on ViewingMessageButtonSelection with a typed three-state enum (e.g. ViewingMessageButtonSelection::Three(ThreeState) where ThreeState is Yes | No | Cancel), add helper methods next()/prev() on ThreeState (or impl on ViewingMessageButtonSelection) for cycling used by the key handler/navigation code, and update all call sites (key handler cycling in src/ui/key_handler/mod.rs and navigation.rs) to call those methods; then change the matches in message_handlers.rs from magic literals (selected: 1, selected: 2) and the catch-all arm to exhaustive pattern matches on the named variants (Yes/No/Cancel) so out-of-range values are impossible and the compiler enforces completeness.src/ui/message_notification.rs (1)
125-140: Vertical layout is now tight against the popup frame.Constraints sum to 17 rows for both
render_add_invoiceandrender_pay_invoiceagainstpopup_height = 18(fromrender_message_notification). After accounting for the top/bottom border of the block, only 1 row of slack remains — which is fine today but means any future addition (extra help line, validation status row, etc.) will silently push content into the border. Consider bumpingpopup_heightslightly so there's visible breathing room above the new action-buttons row.Also applies to: 232-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/message_notification.rs` around lines 125 - 140, The vertical Layout in render_add_invoice and render_pay_invoice currently sums to 17 rows against popup_height = 18 used by render_message_notification leaving no breathing room against the popup block borders; update the popup height (popup_height) or reduce one Constraint length so the total content rows are at least 2 less than the popup height (e.g., bump popup_height from 18 to 19 or decrement one Constraint::Length in the arrays) to restore visible padding above the action-buttons row and avoid content touching the border; ensure you update the same change wherever the same Layout array appears (references: render_add_invoice, render_pay_invoice, render_message_notification, and the Constraint::Length arrays used in those Layout::new calls).src/main.rs (2)
366-369: Commented-out DB sync should be tracked as a follow-up.Leaving the prior
sync_user_order_history_messages_from_dbcall as aTODOblock addresses the earlier performance concern about running it on everyOperationResult, but now order-history cleanup (via the newOperationResult::OrderHistoryDeleted) has no code path invoking the sync. Either gate the call onOrderHistoryDeleted(as suggested previously) or open a tracking issue — otherwise the history-deletion flow silently relies on whatever other mechanism repopulatesapp.messages.Want me to open a tracking issue for this and/or draft the gated-call variant so
OrderHistoryDeletedtriggers the sync?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 366 - 369, The commented-out call to sync_user_order_history_messages_from_db means OperationResult::OrderHistoryDeleted no longer triggers a refresh of app.messages; either reintroduce a gated sync on that specific result or create a tracking issue. Locate the match/handler that processes OperationResult variants and, for OperationResult::OrderHistoryDeleted, call sync_user_order_history_messages_from_db(&pool, &mut app).await guarded by the existing app.user_role == UserRole::User check to avoid running on every operation; if you prefer not to change code now, open a tracking issue noting that OrderHistoryDeleted must trigger the sync so app.messages is repopulated.
63-71: Duplicated clipboard helper.
read_clipboard_text_best_effortis defined identically insrc/ui/key_handler/mod.rs(Lines 273-281). Extract a single implementation into a shared utility module (e.g.src/util/clipboard.rsorsrc/ui/helpers/mod.rs) and import from both callers so logging and error behavior stay in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 63 - 71, The function read_clipboard_text_best_effort is duplicated; extract the single implementation into a shared utility module (e.g., util::clipboard or ui::helpers), make it pub (pub fn read_clipboard_text_best_effort() -> Option<String>) and move the existing implementation there unchanged so logging and error behavior remain identical, then replace the duplicate definitions in main and the key_handler with imports of the shared function (add the appropriate use path) and remove the redundant code; run cargo check to ensure the module is exported and both callers compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/key_handler/message_handlers.rs`:
- Around line 61-63: The two cancel entry points use inconsistent success
messages: change the send(OperationResult::Info(...)) calls in
spawn_cancel_from_notification and handle_enter_viewing_message so they use the
same phrasing for the Action::Cancel pre-Active path (the case that results in
CooperativeCancelInitiatedByYou); update both occurrences (also the similar
block around lines 186-190) to the single chosen string (replace the duplicate
differing string) so both code paths send the identical OperationResult::Info
message.
- Around line 272-285: The test module annotated with #[cfg(test)] (containing
the tests for should_send_cancel_from_invoice_popup) currently appears before
the function handle_enter_rating_order, triggering
clippy::items_after_test_module; fix by moving the entire #[cfg(test)] mod tests
block to the very end of the file (after handle_enter_rating_order) or extract
it into a separate child tests module (e.g., create
src/ui/key_handler/message_handlers/tests.rs and replace the inline block with
mod tests;), ensuring the tests still import super::* so
should_send_cancel_from_invoice_popup remains accessible.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 366-369: The commented-out call to
sync_user_order_history_messages_from_db means
OperationResult::OrderHistoryDeleted no longer triggers a refresh of
app.messages; either reintroduce a gated sync on that specific result or create
a tracking issue. Locate the match/handler that processes OperationResult
variants and, for OperationResult::OrderHistoryDeleted, call
sync_user_order_history_messages_from_db(&pool, &mut app).await guarded by the
existing app.user_role == UserRole::User check to avoid running on every
operation; if you prefer not to change code now, open a tracking issue noting
that OrderHistoryDeleted must trigger the sync so app.messages is repopulated.
- Around line 63-71: The function read_clipboard_text_best_effort is duplicated;
extract the single implementation into a shared utility module (e.g.,
util::clipboard or ui::helpers), make it pub (pub fn
read_clipboard_text_best_effort() -> Option<String>) and move the existing
implementation there unchanged so logging and error behavior remain identical,
then replace the duplicate definitions in main and the key_handler with imports
of the shared function (add the appropriate use path) and remove the redundant
code; run cargo check to ensure the module is exported and both callers compile.
In `@src/ui/key_handler/message_handlers.rs`:
- Around line 79-82: Replace the duplicated role->mode match expressions in
handle_enter_viewing_message by calling the existing helpers
role_default_mode(...) and role_waiting_mode(...): instead of matching on
app.user_role to build default_mode and the waiting_mode, call
role_default_mode(app.user_role) for the default UiMode and
role_waiting_mode(app.user_role) for the waiting UiMode; update both occurrences
(the one creating default_mode and the other at the second occurrence around
lines 132-135) to use these helpers so role mapping is centralized in
role_default_mode and role_waiting_mode.
In `@src/ui/message_notification.rs`:
- Around line 125-140: The vertical Layout in render_add_invoice and
render_pay_invoice currently sums to 17 rows against popup_height = 18 used by
render_message_notification leaving no breathing room against the popup block
borders; update the popup height (popup_height) or reduce one Constraint length
so the total content rows are at least 2 less than the popup height (e.g., bump
popup_height from 18 to 19 or decrement one Constraint::Length in the arrays) to
restore visible padding above the action-buttons row and avoid content touching
the border; ensure you update the same change wherever the same Layout array
appears (references: render_add_invoice, render_pay_invoice,
render_message_notification, and the Constraint::Length arrays used in those
Layout::new calls).
In `@src/ui/orders.rs`:
- Around line 265-272: Replace the raw Three { selected: u8 } variant on
ViewingMessageButtonSelection with a typed three-state enum (e.g.
ViewingMessageButtonSelection::Three(ThreeState) where ThreeState is Yes | No |
Cancel), add helper methods next()/prev() on ThreeState (or impl on
ViewingMessageButtonSelection) for cycling used by the key handler/navigation
code, and update all call sites (key handler cycling in
src/ui/key_handler/mod.rs and navigation.rs) to call those methods; then change
the matches in message_handlers.rs from magic literals (selected: 1, selected:
2) and the catch-all arm to exhaustive pattern matches on the named variants
(Yes/No/Cancel) so out-of-range values are impossible and the compiler enforces
completeness.
🪄 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: 16d1a959-132a-4a49-bb7c-995721135c39
📒 Files selected for processing (14)
docs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/TUI_INTERFACE.mddocs/buy order flow.mdsrc/main.rssrc/ui/helpers/layout.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/message_handlers.rssrc/ui/key_handler/mod.rssrc/ui/message_notification.rssrc/ui/mod.rssrc/ui/orders.rssrc/ui/state.rssrc/util/dm_utils/notifications_ch_mng.rssrc/util/dm_utils/order_ch_mng.rs
✅ Files skipped from review due to trivial changes (4)
- src/ui/mod.rs
- src/ui/state.rs
- docs/MESSAGE_FLOW_AND_PROTOCOL.md
- docs/TUI_INTERFACE.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/util/dm_utils/order_ch_mng.rs
- src/ui/helpers/layout.rs
- src/ui/key_handler/enter_handlers.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/ui/key_handler/message_handlers.rs (1)
97-110: Remove the unreachableThree(No) → FiatSentbranch to keep intent explicit.
Three(ThreeState::No)andTwo { yes_selected: false }are both dismissed at lines 82-93 before this match is reached, so theThree(No) | Three(Yes) => Action::FiatSentarm and theTwo { yes_selected: false } → dismisspath here are dead. That's fine today, but the current shape silently maps NO toFiatSent— if the early-dismiss block is ever refactored or a caller bypasses it, pressing NO on a hold-invoice popup would accept fiat-sent. Consider making the unreachable cases explicit so a future refactor can't regress safety.♻️ Proposed adjustment
Action::HoldInvoicePaymentAccepted => match &view_state.button_selection { - ViewingMessageButtonSelection::Three(ThreeState::Yes) - | ViewingMessageButtonSelection::Three(ThreeState::No) => Action::FiatSent, + ViewingMessageButtonSelection::Three(ThreeState::Yes) => Action::FiatSent, ViewingMessageButtonSelection::Three(ThreeState::Cancel) => Action::Cancel, - ViewingMessageButtonSelection::Two { yes_selected } => { - if *yes_selected { - Action::FiatSent - } else { - app.mode = default_mode; - return; - } - } + ViewingMessageButtonSelection::Two { yes_selected: true } => Action::FiatSent, + // `Three(No)` and `Two { yes_selected: false }` are dismissed above. + ViewingMessageButtonSelection::Three(ThreeState::No) + | ViewingMessageButtonSelection::Two { yes_selected: false } => { + app.mode = default_mode; + return; + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/message_handlers.rs` around lines 97 - 110, The current match for view_state.action == Action::HoldInvoicePaymentAccepted collapses Three(ThreeState::Yes) and Three(ThreeState::No) into Action::FiatSent which silently accepts NO; change the arms so only Three(ThreeState::Yes) maps to Action::FiatSent and make Three(ThreeState::No) an explicit dismiss (set app.mode = default_mode and return) instead of mapping to FiatSent, while keeping the Two { yes_selected } branch logic (yes -> FiatSent, false -> dismiss) intact; update the match on ViewingMessageButtonSelection::Three (ThreeState::Yes/No/Cancel) and ViewingMessageButtonSelection::Two { yes_selected } and ensure Action::Cancel remains for Three(ThreeState::Cancel).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/key_handler/enter_handlers.rs`:
- Around line 12-17: The import list in the crate::ui use block is misformatted
— move ViewingMessageButtonSelection onto the previous line (so it folds with
the rest of the imports) or simply run cargo fmt to apply rustfmt’s expected
layout; update the use declaration that includes AdminMode, AdminTab, ...,
UserTab, ViewingMessageButtonSelection to match rustfmt output so cargo fmt
--check passes.
- Around line 936-945: The current default button_selection for
Action::BuyerTookOrder uses ViewingMessageButtonSelection::Two { yes_selected:
true } which makes Enter send a Cancel; update the branch that matches
Action::BuyerTookOrder to mirror the CooperativeCancelInitiatedByPeer branch and
set ViewingMessageButtonSelection::Two { yes_selected: false } instead (so
yes_selected is false for BuyerTookOrder), leaving other branches unchanged;
reference the Action::BuyerTookOrder match, ViewingMessageButtonSelection::Two
and the yes_selected flag so the change prevents the Enter path (handled by
handle_enter_viewing_message mapping BuyerTookOrder => Action::Cancel) from
sending an unintended cooperative cancel.
---
Nitpick comments:
In `@src/ui/key_handler/message_handlers.rs`:
- Around line 97-110: The current match for view_state.action ==
Action::HoldInvoicePaymentAccepted collapses Three(ThreeState::Yes) and
Three(ThreeState::No) into Action::FiatSent which silently accepts NO; change
the arms so only Three(ThreeState::Yes) maps to Action::FiatSent and make
Three(ThreeState::No) an explicit dismiss (set app.mode = default_mode and
return) instead of mapping to FiatSent, while keeping the Two { yes_selected }
branch logic (yes -> FiatSent, false -> dismiss) intact; update the match on
ViewingMessageButtonSelection::Three (ThreeState::Yes/No/Cancel) and
ViewingMessageButtonSelection::Two { yes_selected } and ensure Action::Cancel
remains for Three(ThreeState::Cancel).
🪄 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: d9af4426-3287-4902-8e40-26e2e2b4591d
📒 Files selected for processing (9)
src/main.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/message_handlers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/navigation.rssrc/ui/mod.rssrc/ui/orders.rssrc/ui/state.rssrc/ui/tabs/tab_content.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/ui/mod.rs
- src/ui/state.rs
- src/main.rs
- src/ui/key_handler/navigation.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/ui/key_handler/enter_handlers.rs (1)
935-944:⚠️ Potential issue | 🟠 MajorDefault
BuyerTookOrderto NO, not the destructive branch.
BuyerTookOrderstill inheritsTwo { yes_selected: true }. In this popup the highlighted action is the cooperative cancel path, so pressing Enter to acknowledge the message will immediately sendCancel. This should mirrorCooperativeCancelInitiatedByPeerand start onyes_selected: false.🛡️ Suggested fix
- } else if matches!(action, Action::CooperativeCancelInitiatedByPeer) { + } else if matches!( + action, + Action::CooperativeCancelInitiatedByPeer | Action::BuyerTookOrder + ) { // Safer default: Enter should not accept cooperative cancel implicitly. ViewingMessageButtonSelection::Two { yes_selected: false, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/enter_handlers.rs` around lines 935 - 944, The default selection logic for button_selection makes Action::BuyerTookOrder inherit Two { yes_selected: true }, which causes Enter to trigger the destructive/cancel path; update the match so that when action matches Action::BuyerTookOrder it returns ViewingMessageButtonSelection::Two { yes_selected: false } (same behavior as Action::CooperativeCancelInitiatedByPeer) — modify the branch in the button_selection assignment (the match against Action::HoldInvoicePaymentAccepted / Action::CooperativeCancelInitiatedByPeer) to include Action::BuyerTookOrder and set yes_selected: false so Enter acknowledges without sending Cancel.src/ui/helpers/startup.rs (1)
259-271:⚠️ Potential issue | 🟠 MajorDB history sync still overwrites fresher DM rows with synthesized placeholders.
This merge path still replaces any existing
OrderMessagefor the sameorder_idwith the projection fromdb_order_to_history_message(). Because that projection collapses rows to generic actions likeTakeBuy/TakeSell, active trades can regress from DM-driven states such asAddInvoice,PayInvoice,HoldInvoicePaymentAccepted, orFiatSentafter startup or post-delete resync. Please either restrict this sync to terminal history rows or skip replacement when the in-memory row is newer/fresher.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/helpers/startup.rs` around lines 259 - 271, The DB-derived history sync currently unconditionally replaces in-memory OrderMessage entries (via history_messages from db_order_to_history_message and the loop that does messages.retain / messages.push under app.messages.lock()), which can regress fresher DM-driven states; fix by changing the sync loop to, for each msg in history_messages, check the existing in-memory message with the same order_id and only replace it if the DB-derived msg is terminal (use a terminal predicate like is_terminal_state on the OrderMessage/state) or its timestamp is newer or equal to the existing one (compare msg.timestamp with existing.timestamp); update the loop that uses messages.retain / messages.push to perform this conditional logic instead of blindly retaining and pushing every DB projection.
🧹 Nitpick comments (2)
src/ui/key_handler/async_tasks.rs (1)
50-127: Optional: deduplicate withclear_runtime_session_state.
clear_runtime_tracking_state_preserve_messagesrepeats theactive_order_trade_indicesandpending_notificationslock+poison-fatal blocks verbatim fromclear_runtime_session_state, plus three identicalmode = OperationResult::Error("Internal error...")assignments. Easy to drift over time. Consider extracting shared helpers likeclear_active_indices(app)andreset_pending_notifications(app)returningResult<(), ()>so both reset paths can compose them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/async_tasks.rs` around lines 50 - 127, Both functions duplicate the same poisoned-lock handling and error-mode setting for active_order_trade_indices and pending_notifications; extract helpers (e.g., clear_active_order_trade_indices(app) and reset_pending_notifications(app)) that attempt the lock, call request_fatal_restart on Err, set app.fatal_exit_on_close and app.mode to UiMode::OperationResult(OperationResult::Error(...)) and return Result<(), ()> so clear_runtime_session_state and clear_runtime_tracking_state_preserve_messages call these helpers and only keep their unique bits (clearing messages, preserving messages path, dropped_user_history_order_ids handling, and selected_message_idx/pending_post_take_operation_result).src/ui/helpers/order_chat_projection.rs (1)
72-74:is_order_chat_actionableis now a no-op — restore the bounded status set.
is_order_chat_actionable(_status)unconditionally returnstrue, so the.filter(|row| is_order_chat_actionable(row.status))at line 111 never excludes anything. The sidebar should filter to a bounded set (WaitingPayment,WaitingBuyerInvoice,SettledHoldInvoice,InProgress,Active,FiatSent); the current implementation allows terminal states likeCanceled,Success, andPendingto surface.Either remove the dead filter call, or restore the bounded status check:
♻️ Restore bounded filter
-fn is_order_chat_actionable(_status: Option<Status>) -> bool { - true -} +fn is_order_chat_actionable(status: Option<Status>) -> bool { + matches!( + status, + Some(Status::WaitingPayment) + | Some(Status::WaitingBuyerInvoice) + | Some(Status::SettledHoldInvoice) + | Some(Status::InProgress) + | Some(Status::Active) + | Some(Status::FiatSent) + ) +}Also applies to: 109–112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/helpers/order_chat_projection.rs` around lines 72 - 74, The is_order_chat_actionable function currently returns true unconditionally which neutralizes the .filter(|row| is_order_chat_actionable(row.status)) usage; restore a bounded-status check by matching the Option<Status> in is_order_chat_actionable and return true only for the actionable variants (WaitingPayment, WaitingBuyerInvoice, SettledHoldInvoice, InProgress, Active, FiatSent) and false otherwise (including None and terminal states like Canceled, Success, Pending); update the function signature is_order_chat_actionable(_status: Option<Status>) -> bool to match on the _status and return accordingly so the existing .filter on row.status behaves as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/key_handler/async_tasks.rs`:
- Around line 124-126: The reconnect code currently clears the session-scoped
set dropped_user_history_order_ids (the lines with dropped.clear()), which
violates its documented session lifetime; remove the clear() so the set is
preserved across reconnects (or update the docstring if you intentionally want
reconnect to reset it), and also make the lock-error handling consistent with
the surrounding locks by replacing the lenient if let Ok(...) on
app.dropped_user_history_order_ids with the same match pattern (and fatal
restart behavior) used for the active/pending locks so lock failures are handled
uniformly.
In `@src/util/dm_utils/mod.rs`:
- Around line 695-709: The current replacement logic in should_replace_row
(matching existing_message_data) allows older messages to replace the row when
should_accept_candidate is true even if the candidate's accepted status equals
the existing status; update the older-than-current branch so it only returns
true when the candidate strictly advances the accepted-status. Concretely, in
the else branch (the comment "Older-than-current replay..."), replace the
single-condition use of should_accept_candidate with a strict comparison that
checks the candidate's accepted-status is greater than the existing
accepted-status (i.e., require a strict advancement), using the variables that
hold the candidate and existing accepted-phase/status values instead of allowing
equality.
In `@src/util/order_utils/take_order.rs`:
- Around line 259-272: The call to build_order_chat_static_header currently uses
expect and can panic; instead propagate a proper error from take_order by
converting the Option into a Result (e.g., with ok_or or ok_or_else) and
returning an anyhow::Error if None is returned. Locate the call in take_order
where you build static_header for OperationResult::PaymentRequestRequired (use
symbols build_order_chat_static_header, order_to_save, next_idx, trade_keys,
OperationResult::PaymentRequestRequired) and replace the expect(...) with code
that maps a None into an Err(anyhow::anyhow!("failed to build static header for
order id {:?}", order_to_save.id)) so the function returns an error instead of
panicking.
---
Duplicate comments:
In `@src/ui/helpers/startup.rs`:
- Around line 259-271: The DB-derived history sync currently unconditionally
replaces in-memory OrderMessage entries (via history_messages from
db_order_to_history_message and the loop that does messages.retain /
messages.push under app.messages.lock()), which can regress fresher DM-driven
states; fix by changing the sync loop to, for each msg in history_messages,
check the existing in-memory message with the same order_id and only replace it
if the DB-derived msg is terminal (use a terminal predicate like
is_terminal_state on the OrderMessage/state) or its timestamp is newer or equal
to the existing one (compare msg.timestamp with existing.timestamp); update the
loop that uses messages.retain / messages.push to perform this conditional logic
instead of blindly retaining and pushing every DB projection.
In `@src/ui/key_handler/enter_handlers.rs`:
- Around line 935-944: The default selection logic for button_selection makes
Action::BuyerTookOrder inherit Two { yes_selected: true }, which causes Enter to
trigger the destructive/cancel path; update the match so that when action
matches Action::BuyerTookOrder it returns ViewingMessageButtonSelection::Two {
yes_selected: false } (same behavior as
Action::CooperativeCancelInitiatedByPeer) — modify the branch in the
button_selection assignment (the match against
Action::HoldInvoicePaymentAccepted / Action::CooperativeCancelInitiatedByPeer)
to include Action::BuyerTookOrder and set yes_selected: false so Enter
acknowledges without sending Cancel.
---
Nitpick comments:
In `@src/ui/helpers/order_chat_projection.rs`:
- Around line 72-74: The is_order_chat_actionable function currently returns
true unconditionally which neutralizes the .filter(|row|
is_order_chat_actionable(row.status)) usage; restore a bounded-status check by
matching the Option<Status> in is_order_chat_actionable and return true only for
the actionable variants (WaitingPayment, WaitingBuyerInvoice,
SettledHoldInvoice, InProgress, Active, FiatSent) and false otherwise (including
None and terminal states like Canceled, Success, Pending); update the function
signature is_order_chat_actionable(_status: Option<Status>) -> bool to match on
the _status and return accordingly so the existing .filter on row.status behaves
as intended.
In `@src/ui/key_handler/async_tasks.rs`:
- Around line 50-127: Both functions duplicate the same poisoned-lock handling
and error-mode setting for active_order_trade_indices and pending_notifications;
extract helpers (e.g., clear_active_order_trade_indices(app) and
reset_pending_notifications(app)) that attempt the lock, call
request_fatal_restart on Err, set app.fatal_exit_on_close and app.mode to
UiMode::OperationResult(OperationResult::Error(...)) and return Result<(), ()>
so clear_runtime_session_state and
clear_runtime_tracking_state_preserve_messages call these helpers and only keep
their unique bits (clearing messages, preserving messages path,
dropped_user_history_order_ids handling, and
selected_message_idx/pending_post_take_operation_result).
🪄 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: 720f46da-d5e0-4672-85ae-fc904028a6f6
📒 Files selected for processing (23)
docs/DATABASE.mddocs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/README.mddocs/TUI_INTERFACE.mdsrc/main.rssrc/models.rssrc/ui/app_state.rssrc/ui/helpers/order_chat_projection.rssrc/ui/helpers/startup.rssrc/ui/key_handler/async_tasks.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/navigation.rssrc/ui/message_notification.rssrc/ui/mod.rssrc/ui/operation_result.rssrc/ui/orders.rssrc/ui/state.rssrc/ui/tabs/order_in_progress_tab.rssrc/util/dm_utils/mod.rssrc/util/dm_utils/order_ch_mng.rssrc/util/order_utils/helper.rssrc/util/order_utils/send_new_order.rssrc/util/order_utils/take_order.rs
✅ Files skipped from review due to trivial changes (3)
- docs/README.md
- docs/DATABASE.md
- docs/TUI_INTERFACE.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/ui/state.rs
- src/ui/mod.rs
- src/models.rs
- src/ui/key_handler/navigation.rs
- docs/MESSAGE_FLOW_AND_PROTOCOL.md
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Nice improvement overall. I like the new hold-invoice YES / NO / CANCEL flow, the cooperative-cancel shortcut from Messages, and the startup/history work to keep My Trades more stable.
I only have one non-blocking UX/safety note:
Ctrl+Deleteopens the bulk local-history cleanup dialog with YES preselected (ConfirmBulkDeleteHistory(true)). Since this action removes multiple local rows at once, I would default that confirmation to NO instead, so an accidentalCtrl+Deletefollowed by an automaticEnterdoes not immediately wipe local history.
I do not consider that a blocker for this PR, but I think the safer default would be worth applying in a follow-up.
Contributor
There was a problem hiding this comment.
Re-reviewed the latest commits. The follow-up fixes look good to me:
- bulk history cleanup now defaults to NO, which resolves the safety nit I raised earlier
- the older replay replacement logic is tighter now, since equal-status stale messages no longer clobber the current row
- the
take_orderstatic-header path now fails gracefully instead of panicking
I do not see a new blocker in the latest changes, and checks are green, so my approval still stands. Nice cleanup pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When Mostro reports Hold invoice payment accepted, the Messages tab confirmation is now a three-option flow: YES (send
FiatSent), NO (dismiss), or CANCEL (send cooperativeCancel, same idea as My Trades Shift+C). This lets users either continue the trade or start a cooperative cancel without leaving Messages.Changes
MessageViewStateusesViewingMessageButtonSelection—Two { yes_selected }for existing YES/NO popups,Three { selected }for hold-invoice (0/1/2).VIEW_MESSAGE_HOLD_INVOICE_PREVIEWinconstants.rs;order_message_to_notificationuses it forHoldInvoicePaymentAccepted.render_yes_no_cancel_buttons; hold-invoice popup uses multi-lineText(real line breaks), left-aligned body with padding, and height derived from line count so the frame stays compact.execute_send_msgtreatsCooperativeCancelInitiatedByYouas a valid response after sendingCancel.Motivation
At hold-invoice-accepted, users may need to cooperatively cancel instead of marking fiat sent; previously cancel was only convenient from My Trades.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation