src/ui/helpers/formatting.rs (1)

21-23: is_dispute_finalized can return bool directly.

This helper always returns Some(...); returning bool simplifies call sites and avoids unnecessary wrapping.

♻️ Proposed refactor

-pub fn is_dispute_finalized(selected_dispute: &AdminDispute) -> Option<bool> {
-    Some(selected_dispute.is_finalized())
+pub fn is_dispute_finalized(selected_dispute: &AdminDispute) -> bool {
+    selected_dispute.is_finalized()
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/helpers/formatting.rs` around lines 21 - 23, Change
is_dispute_finalized to return a plain bool instead of Option<bool>: update the
function signature from pub fn is_dispute_finalized(selected_dispute:
&AdminDispute) -> Option<bool> to -> bool and simply return
selected_dispute.is_finalized(). Also search for call sites that expect
Option<bool> and remove the Some(...) handling (e.g., replace comparisons with
unwraps or direct bool usage) to match the new bool return type.

src/ui/chat.rs (1)

106-109: Consider replacing raw message tuples with a named payload type.

Vec<(String, i64, PublicKey)> is compact but brittle. A named struct improves readability and reduces ordering mistakes.

♻️ Suggested refactor

+#[derive(Clone, Debug)]
+pub struct PolledChatMessage {
+    pub content: String,
+    pub timestamp: i64,
+    pub sender_pubkey: PublicKey,
+}
+
 pub struct OrderChatUpdate {
     pub order_id: String,
-    /// (content, timestamp, sender_pubkey)
-    pub messages: Vec<(String, i64, PublicKey)>,
+    pub messages: Vec<PolledChatMessage>,
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/chat.rs` around lines 106 - 109, Replace the brittle tuple type used
in OrderChatUpdate.messages with a named struct to improve readability and
safety: define a ChatMessage (or similar) struct containing fields content:
String, timestamp: i64, and sender_pubkey: PublicKey, then change
OrderChatUpdate.messages from Vec<(String, i64, PublicKey)> to Vec<ChatMessage>
and update all constructors, pattern matches, serializations, and usages (e.g.,
creation sites, destructuring, and any serde derives) to use the new struct
fields instead of tuple indexes.

src/main.rs (1)

248-248: Consider guarding startup hydration by user role.

load_user_order_chats_at_startup is called unconditionally, but user order chats are only relevant for UserRole::User. While this is not a bug (admin mode will simply have no active user orders), you could skip the DB/relay work when starting in admin mode for a minor efficiency gain.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main.rs` at line 248, Only call load_user_order_chats_at_startup when the
running role is a regular user: wrap the call in a conditional that checks the
app's active role (match or if let with UserRole::User) and skip the DB/relay
hydration for admin mode; reference the UserRole enum and the function
load_user_order_chats_at_startup(&client, &pool, &mut app).await and ensure you
import or reference UserRole where needed so admin startup path avoids the
unnecessary work.

src/ui/tabs/order_in_progress_tab.rs (1)

117-149: Consider extracting duplicate wrap_text_to_lines helper.

This function duplicates the implementation in src/ui/helpers/chat_render.rs. Consider importing and reusing the existing helper to maintain DRY principles.

Suggested approach

+use crate::ui::helpers::chat_render::wrap_text_to_lines;
+
 fn build_order_chat_content(
     messages: &[UserOrderChatMessage],
     content_width: u16,
 ) -> (Vec<Line<'static>>, u16, Vec<usize>) {
-    fn wrap_text_to_lines(content: &str, max_width: u16) -> Vec<String> {
-        // ... 30+ lines of duplicate code
-    }
     // rest of function using imported wrap_text_to_lines

Note: You may need to adjust visibility in chat_render.rs if wrap_text_to_lines is not already pub.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/tabs/order_in_progress_tab.rs` around lines 117 - 149, Duplicate
wrap_text_to_lines implementation should be removed and the existing helper
reused: change this file to call the central wrap_text_to_lines function from
the chat_render module (e.g. use
crate::ui::helpers::chat_render::wrap_text_to_lines) instead of redefining it;
if the helper in chat_render.rs is not public, make it public (pub or pub(crate)
as appropriate) and update the use/import in order_in_progress_tab.rs so the
single shared function is used. Ensure tests/build still pass after adjusting
visibility and remove the local wrap_text_to_lines function definition.

src/ui/helpers/startup.rs (3)

266-276: Automatic focus jump on attachment detection may be surprising.

When an attachment is received, the code automatically switches selected_in_progress_idx and active_chat_party, pulling the user's focus to that dispute/party. If this is intentional behavior to draw attention to attachments, consider documenting it. If not, you may want to only show the toast without changing selection.

🤖 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 266 - 276, The code currently sets
app.attachment_toast and then unconditionally updates
app.selected_in_progress_idx and app.active_chat_party when attachment_opt is
Some, which forces the UI focus jump; to prevent this, change the block handling
attachment_opt so it only calls app.attachment_toast =
Some(build_attachment_toast(filename_for_toast)) and does not modify
app.selected_in_progress_idx or app.active_chat_party (or guard those
assignments behind an explicit flag/setting if intentional); update the handling
around attachment_opt, build_attachment_toast, and the
admin_disputes_in_progress position lookup (the .position(|d| d.dispute_id ==
dispute_key)) to be removed or conditional so receiving an attachment shows the
toast only without changing selection.

---

59-64: Unnecessary clone of messages vector.

load_chat_from_file returns an owned Vec<DisputeChatMessage>. The .clone() on line 60 creates an extra copy when computing timestamps, but the original msgs could be moved into the map and a reference used for max_party_timestamps.

♻️ Proposed fix

         if let Some(msgs) = load_chat_from_file(&dispute.dispute_id) {
-            admin_dispute_chats.insert(dispute.dispute_id.clone(), msgs.clone());
-            let (buyer_max, seller_max) = max_party_timestamps(&msgs);
+            let (buyer_max, seller_max) = max_party_timestamps(&msgs);
+            admin_dispute_chats.insert(dispute.dispute_id.clone(), msgs);
             update_last_seen_timestamp(buyer_max, seller_max, dispute, admin_chat_last_seen);
         }

🤖 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 59 - 64, Move the owned Vec returned
by load_chat_from_file into the admin_dispute_chats map instead of cloning it:
call let msgs = load_chat_from_file(&dispute.dispute_id).unwrap_or_default();
then insert with admin_dispute_chats.insert(dispute.dispute_id.clone(), msgs);
obtain a reference to the stored messages (e.g. let stored =
admin_dispute_chats.get(&dispute.dispute_id).unwrap()) and pass that reference
to max_party_timestamps(&stored) so you avoid the unnecessary msgs.clone() while
keeping update_last_seen_timestamp(buyer_max, seller_max, dispute,
admin_chat_last_seen) unchanged.

---

125-146: Consider logging errors instead of silently discarding them.

Lines 125-127 and 143-145 silently discard errors. While it's reasonable not to block startup on failures, logging would help diagnose issues:

♻️ Proposed fix

-    let Ok(rows) = Order::get_startup_active_orders(pool).await else {
+    let rows = match Order::get_startup_active_orders(pool).await {
+        Ok(r) => r,
+        Err(e) => {
+            log::warn!("Failed to load active orders at startup: {}", e);
+            return;
+        }
+    };
     ...
-    let updates = fetch_user_order_chat_updates(client, pool, &app.order_chat_last_seen)
-        .await
-        .unwrap_or_default();
+    let updates = match fetch_user_order_chat_updates(client, pool, &app.order_chat_last_seen).await {
+        Ok(u) => u,
+        Err(e) => {
+            log::warn!("Failed to fetch user order chat updates: {}", e);
+            return;
+        }
+    };

🤖 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 125 - 146, The code currently
swallows errors from Order::get_startup_active_orders(...) and
fetch_user_order_chat_updates(...). Replace the terse patterns with proper error
handling: when calling Order::get_startup_active_orders(pool).await, match the
Result and log the error (e.g., tracing::error! or log::error!) with contextual
info instead of just returning; when calling
fetch_user_order_chat_updates(client, pool, &app.order_chat_last_seen).await,
handle the Result explicitly (match/if let Err(e)) and log the error before
falling back to default updates so startup continues; keep existing behavior of
loading chats via load_order_chat_from_file(&order_id) but ensure any IO errors
there are surfaced/logged by that function if applicable. Use the function names
Order::get_startup_active_orders and fetch_user_order_chat_updates in your
changes so the edits are easy to find.

src/ui/helpers/chat_render.rs (1)

178-206: Consider extracting shared logic with build_chat_scrollview_content.

This function is nearly identical to build_chat_scrollview_content, differing only in the absence of party filtering and the placeholder message. A helper could reduce duplication:

fn build_scrollview_content_impl(
    messages: impl Iterator<Item = &DisputeChatMessage>,
    content_width: u16,
    max_content_width: Option<u16>,
    empty_placeholder: &'static str,
) -> ChatScrollViewContent { ... }

That said, the current duplication is minimal and the separate functions are clear about their purpose—this is a nice-to-have refinement.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/helpers/chat_render.rs` around lines 178 - 206, Both
build_observer_scrollview_content and build_chat_scrollview_content duplicate
the same logic; extract the shared flow into a helper like
build_scrollview_content_impl that accepts an iterator over &DisputeChatMessage,
content_width, max_content_width, and an empty_placeholder &str; have the helper
populate lines by calling format_message_lines, record line_start_per_message,
push the styled placeholder using Line::from(Span::styled(...)) when empty,
compute content_height and return ChatScrollViewContent, and then replace the
bodies of build_observer_scrollview_content and build_chat_scrollview_content to
call this helper with the appropriate placeholder and message iterator.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/ui/key_handler/message_handlers.rs (1)

43-52: Optional cleanup: remove duplicated default_mode recomputation.

Line 47-Line 50 re-derives default_mode even though it is already computed at Line 18-Line 21.

♻️ Proposed simplification

     let Some(order_id) = view_state.order_id else {
         let _ = ctx
             .order_result_tx
             .send(OperationResult::Error("No order ID in message".to_string()));
-        let default_mode = match app.user_role {
-            UserRole::User => UiMode::UserMode(UserMode::Normal),
-            UserRole::Admin => UiMode::AdminMode(AdminMode::Normal),
-        };
         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 43 - 52, The code
duplicates the derivation of default_mode when view_state.order_id is None;
instead reuse the earlier computed default_mode rather than recomputing it in
the None branch—remove the second match on app.user_role that creates
UiMode::UserMode(UserMode::Normal) / UiMode::AdminMode(AdminMode::Normal) and
simply assign app.mode = default_mode after sending OperationResult::Error via
ctx.order_result_tx, preserving the send of "No order ID in message" and the
early return behavior around view_state.order_id.

src/ui/tabs/order_in_progress_tab.rs (1)

200-249: Consider avoiding full clone of messages under lock.

Line 202-203 clones the entire messages vector while holding the lock. For large message sets, this could cause brief UI stutters. Consider iterating once to extract only the needed data, or using a more targeted snapshot.

This is a minor performance consideration and may be acceptable for typical message counts.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/tabs/order_in_progress_tab.rs` around lines 200 - 249, The
render_order_in_progress function currently clones the entire messages vector
while holding app.messages lock (messages_snapshot = match app.messages.lock()
... g.clone()), which can cause UI stutter for large message sets; instead,
inside render_order_in_progress take the mutex briefly and create a minimal
snapshot of only the data needed by build_active_orders (e.g., map each message
to an OrderSummary or the specific fields build_active_orders requires) or
otherwise copy only identifiers/timestamps/text rather than the full message
structs, then drop the lock and call build_active_orders on that lightweight
snapshot (refer to app.messages, messages_snapshot, render_order_in_progress,
and build_active_orders to locate and update the logic).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/ui/tabs/order_in_progress_tab.rs (1)

269-278: ⚠️ Potential issue | 🟡 Minor

Footer area is too small for two-line footer layout.

The layout allocates Constraint::Length(1) for the footer area (line 275), but when footer_line2 is Some (wide terminals ≥90 chars), lines 525-531 attempt to split this single row into two Length(1) rows. The second line will be clipped.

🐛 Proposed fix for dynamic footer height

+    // Pre-calculate footer height before layout
+    let footer_height = if main_area.width >= 90 { 2 } else { 1 };
+
     let main_chunks = Layout::new(
         Direction::Vertical,
         [
             Constraint::Length(7),
             Constraint::Min(0),
             Constraint::Length(3),
-            Constraint::Length(1),
+            Constraint::Length(footer_height),
         ],
     )
     .split(main_area);

Also applies to: 524-534

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/tabs/order_in_progress_tab.rs` around lines 269 - 278, The footer is
allocated only Constraint::Length(1) in the main_chunks layout but code later
renders two footer lines when footer_line2 is Some, causing clipping; modify the
layout creation (the Layout::new call that produces main_chunks) to compute
footer_height dynamically (e.g., let footer_height = if footer_line2.is_some() {
2 } else { 1 }) and replace the trailing Constraint::Length(1) with
Constraint::Length(footer_height) so the footer area grows to two rows when
footer_line2 exists.

src/ui/key_handler/enter_handlers.rs (2)

92-111: Redundant mode assignment on line 109.

The app.mode is already set to mode_after_send on line 98, and since reset_input (line 108) only clears input state without modifying the mode, the assignment on line 109 is redundant.

♻️ Proposed simplification

     app.mode = mode_after_send.clone();
     if content.is_empty() || !input_enabled {
         return;
     }

     let Some(target) = resolve_target(app) else {
         return;
     };

     apply_local(app, &target, &content);
     reset_input(app);
-    app.mode = mode_after_send;
     spawn_remote(target, content);
 }

🤖 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 92 - 111, The code sets
app.mode = mode_after_send twice: once just after destructuring
EnterChatSendConfig and again after reset_input; remove the redundant assignment
after reset_input (the one immediately before spawn_remote) since reset_input
does not alter app.mode; keep the initial app.mode = mode_after_send (which uses
mode_after_send.clone()) and leave resolve_target, apply_local, reset_input, and
spawn_remote unchanged.

---

162-182: Add warning logs for silent early returns.

The function logs warnings for some failures (order not found, send failure) but silently returns for missing trade_keys (line 168) and missing shared_keys (line 181). This makes debugging difficult when chat messages fail to send.

♻️ Proposed fix to add warning logs

         let trade_sk = match order
             .trade_keys
             .as_deref()
             .and_then(|h| SecretKey::from_str(h).ok())
         {
             Some(sk) => sk,
-            None => return,
+            None => {
+                log::warn!("order chat send skipped (missing or invalid trade_keys)");
+                return;
+            }
         };
         let trade_keys = Keys::new(trade_sk);
         let shared_keys = order
             .order_chat_shared_key_hex
             .as_deref()
             .and_then(crate::util::chat_utils::keys_from_shared_hex)
             .or_else(|| {
                 let cp = order.counterparty_pubkey.as_deref()?;
                 let pk = PublicKey::parse(cp).ok()?;
                 crate::util::chat_utils::derive_shared_keys(Some(&trade_keys), Some(&pk))
             });
         let Some(shared_keys) = shared_keys else {
+            log::warn!("order chat send skipped (could not derive shared keys)");
             return;
         };

🤖 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 162 - 182, The code
silently returns when trade key parsing and shared key derivation fail; add
warning logs at both failure points to aid debugging. Specifically, where
SecretKey::from_str(...) is matched (the block producing trade_sk / returning
None) add a warn! (or the project's logger) logging that trade_keys parsing
failed and include identifying info from order (e.g., order id or order.uid).
Likewise, before the early return when shared_keys is None (the `let
Some(shared_keys) = shared_keys else { return; }` branch), log a warn!
indicating shared key derivation failed (include whether
order_chat_shared_key_hex was missing and/or counterparty_pubkey used and the
order identifier). Use the same logger style as other warnings in this file so
messages are consistent.

src/ui/key_handler/chat_helpers.rs (1)

120-129: Consider consistent error handling for poisoned locks.

This function silently returns None when the lock is poisoned (via .ok()), while similar lock acquisitions elsewhere (e.g., resolve_selected_order_chat_target in enter_handlers.rs) call request_fatal_restart. This inconsistency could mask serious state corruption issues.

♻️ Proposed fix for consistent error handling

 pub fn resolve_selected_mytrades_order_id(app: &AppState) -> Option<Uuid> {
-    app.active_order_trade_indices.lock().ok().and_then(|map| {
+    let map = match app.active_order_trade_indices.lock() {
+        Ok(g) => g,
+        Err(e) => {
+            crate::util::request_fatal_restart(format!(
+                "Mostrix encountered an internal error (poisoned active_order_trade_indices lock: {e}). Please restart the app."
+            ));
+            return None;
+        }
+    };
-        // HashMap iteration order is arbitrary; sort keys to match the
-        // stable ordering used in the MyTrades sidebar (by order_id UUID).
-        let mut order_ids: Vec<Uuid> = map.keys().copied().collect();
-        order_ids.sort();
-        order_ids.get(app.selected_order_chat_idx).copied()
-    })
+    // HashMap iteration order is arbitrary; sort keys to match the
+    // stable ordering used in the MyTrades sidebar (by order_id UUID).
+    let mut order_ids: Vec<Uuid> = map.keys().copied().collect();
+    order_ids.sort();
+    order_ids.get(app.selected_order_chat_idx).copied()
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/key_handler/chat_helpers.rs` around lines 120 - 129, The function
resolve_selected_mytrades_order_id currently swallows poisoned lock errors by
calling .ok(); change it to mirror resolve_selected_order_chat_target by
detecting a poisoned lock on app.active_order_trade_indices and invoking
request_fatal_restart with a descriptive context instead of returning None, then
proceed to lock and compute the sorted order_ids and selected UUID as before;
reference the resolve_selected_mytrades_order_id function and the
AppState.active_order_trade_indices field and use the existing
request_fatal_restart helper to handle the poison case.

src/ui/tabs/order_in_progress_tab.rs (1)

199-202: Silent failure on lock error.

Similar to other lock handling in the codebase, a poisoned lock typically indicates serious state corruption. Consider using request_fatal_restart for consistency, or at minimum log the error.

♻️ Proposed fix for consistent error handling

     let messages_snapshot = match app.messages.lock() {
         Ok(g) => g.clone(),
-        Err(_) => Vec::new(),
+        Err(e) => {
+            log::error!("Failed to lock messages: {}", e);
+            Vec::new()
+        }
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/tabs/order_in_progress_tab.rs` around lines 199 - 202, The match on
app.messages.lock() silently drops errors into an empty Vec, but a poisoned lock
should be handled consistently (e.g., call request_fatal_restart or at least
log). Replace the Err(_) arm in the messages lock match used to populate
messages_snapshot with a call to request_fatal_restart (or process logger)
passing the lock error context and a brief message, so that when
app.messages.lock() fails the app triggers the same fatal-restart path used
elsewhere instead of silently continuing; keep the Ok(g) => g.clone() arm
unchanged and ensure the request_fatal_restart reference and any captured error
are included for debugging.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/ui/helpers/order_chat_projection.rs (1)

98-103: Sidebar ordering is lexicographic by order_id, not time-based.

Sorting by the UUID string yields a stable order, but it isn't chronological, so newly opened trades can appear in the middle of the sidebar rather than at the top/bottom. Since build_active_order_chat_list already captures created_at, consider sorting by created_at (with order_id as tiebreaker) for a more intuitive "My Trades" list. Stability of ordering — which is what matters for selected_order_chat_idx alignment — is preserved either way because both the sidebar and Enter handler consume the same projection.

♻️ Optional: sort by created_at desc, fallback to order_id

-    rows.sort_by(|a, b| a.order_id.cmp(&b.order_id));
+    rows.sort_by(|a, b| {
+        b.created_at
+            .cmp(&a.created_at)
+            .then_with(|| a.order_id.cmp(&b.order_id))
+    });

🤖 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 98 - 103, The sidebar
is currently sorted lexicographically by order_id; change the final sort in
build_active_order_chat_list to sort by created_at (newest first) and use
order_id as a deterministic tiebreaker so actionable rows (filtered via
is_order_chat_actionable) are ordered chronologically while remaining stable for
selected_order_chat_idx; replace the rows.sort_by(...) call with a comparator
that first compares b.created_at.cmp(&a.created_at) for descending created_at
and falls back to a.order_id.cmp(&b.order_id).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes