src/util/filters.rs (1)

8-14: Rename/update docs or restore the 7‑day constraint

The function name/doc still promise “last 7 days,” but the filter now only constrains author/kind/limit. This likely pulls full history and can inflate query sizes or change UI behavior. If the 7‑day window is still intended, reintroduce a time constraint; otherwise rename/update docs (and call sites) to reflect the new behavior.

♻️ Minimal rename/doc alignment (if 7‑day window is intentionally removed)

-/// Create a filter for events from the last 7 days
-pub fn create_seven_days_filter(kind: u16, pubkey: PublicKey) -> Result<Filter> {
+/// Create a filter for events of a given kind authored by `pubkey`
+pub fn create_kind_filter(kind: u16, pubkey: PublicKey) -> Result<Filter> {
     Ok(Filter::new()
         .author(pubkey)
         .limit(50)
         .kind(nostr_sdk::Kind::Custom(kind)))
 }
@@
-        ListKind::Orders => create_seven_days_filter(NOSTR_ORDER_EVENT_KIND, pubkey),
-        ListKind::Disputes => create_seven_days_filter(NOSTR_DISPUTE_EVENT_KIND, pubkey),
+        ListKind::Orders => create_kind_filter(NOSTR_ORDER_EVENT_KIND, pubkey),
+        ListKind::Disputes => create_kind_filter(NOSTR_DISPUTE_EVENT_KIND, pubkey),

Also applies to: 23-24

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

285-291: Redundant default_mode calculation.

The default_mode is already computed at the top of handle_cancel_key (lines 251-254). The recalculation inside this branch is unnecessary.

♻️ Suggested simplification

     } else if let UiMode::AdminMode(AdminMode::ConfirmTakeDispute(_, _)) = &app.mode {
         // User cancelled taking the dispute
-        let default_mode = match app.user_role {
-            UserRole::User => UiMode::UserMode(UserMode::Normal),
-            UserRole::Admin => UiMode::AdminMode(AdminMode::Normal),
-        };
         app.mode = default_mode;
     }

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

177-184: Consider removing the redundant tab check in ManagingDispute handling.

Since ManagingDispute mode should only be active when on the DisputesInProgress tab (as set in handle_tab_switch), the inner tab check is redundant. However, keeping it provides defensive coding against potential state inconsistencies, so this is a minor observation.

Also applies to: 274-284

---

348-370: Tab and BackTab perform the same action.

Both Tab and BackTab toggle between Buyer and Seller chat parties, making them functionally identical. This is acceptable for a two-party toggle but differs from typical Tab/BackTab behavior (forward vs. backward navigation). If intentional for UX simplicity, consider adding a brief comment.

src/ui/tab_content.rs (1)

41-64: Block created twice with identical configuration.

The Block widget with title "Exit" and identical styling is constructed twice: once at lines 41-45 to compute inner_area, and again at lines 58-63 for rendering. Consider storing the block in a variable and reusing it.

♻️ Suggested refactor

 pub fn render_exit_tab(f: &mut ratatui::Frame, area: Rect) {
     let logo_lines = get_mostro_logo();
 
-    // Create a layout to center the logo vertically
-    let inner_area = Block::default()
+    let block = Block::default()
         .title("Exit")
         .borders(Borders::ALL)
-        .style(Style::default().bg(BACKGROUND_COLOR))
-        .inner(area);
+        .style(Style::default().bg(BACKGROUND_COLOR));
+
+    // Create a layout to center the logo vertically
+    let inner_area = block.inner(area);
 
     let logo_height = logo_lines.len() as u16;
     let available_height = inner_area.height;
@@ -57,12 +55,7 @@
     };
 
     // Render the block
-    f.render_widget(
-        Block::default()
-            .title("Exit")
-            .borders(Borders::ALL)
-            .style(Style::default().bg(BACKGROUND_COLOR)),
-        area,
-    );
+    f.render_widget(block, area);

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

1-25: Minor inconsistency in type path usage.

The import at line 1 brings InvoiceInputState and KeyInputState into scope, which is used in the function signatures (lines 57, 63). However, the trait implementations at lines 9 and 18 still use the fully qualified crate::ui::InvoiceInputState and crate::ui::KeyInputState paths. Consider using the imported names consistently.

♻️ Suggested fix for consistency

-impl TextInputState for crate::ui::InvoiceInputState {
+impl TextInputState for InvoiceInputState {
     fn get_input_mut(&mut self) -> &mut String {
         &mut self.invoice_input
     }
     fn get_just_pasted_mut(&mut self) -> &mut bool {
         &mut self.just_pasted
     }
 }
 
-impl TextInputState for crate::ui::KeyInputState {
+impl TextInputState for KeyInputState {
     fn get_input_mut(&mut self) -> &mut String {
         &mut self.key_input
     }

docs/TUI_INTERFACE.md (1)

248-248: Overlapping line references may cause confusion.

Line 248 references src/ui/key_handler/mod.rs:155-172 for Shift+I toggle and mod.rs:168-172 for handle_admin_chat_input. The ranges overlap (168-172 is within 155-172), which could confuse readers trying to locate the specific code sections.

Consider clarifying with distinct, non-overlapping ranges or combining into a single reference if they describe related functionality in the same code block.

src/main.rs (1)

141-159: Consider extracting duplicate filtering logic.

The same filtering logic for "InProgress" disputes appears here and again at lines 202-210. Extracting this to a helper method (e.g., filter_in_progress_disputes) would reduce duplication and ensure consistent behavior.

♻️ Suggested helper

fn filter_in_progress_disputes(disputes: Vec<crate::models::AdminDispute>) -> Vec<crate::models::AdminDispute> {
    disputes
        .into_iter()
        .filter(|d| d.status.as_ref().map(|s| s == "InProgress").unwrap_or(false))
        .collect()
}

Then use it in both locations:

-app.admin_disputes_in_progress = all_disputes
-    .into_iter()
-    .filter(|d| {
-        d.status
-            .as_ref()
-            .map(|s| s == "InProgress")
-            .unwrap_or(false)
-    })
-    .collect();
+app.admin_disputes_in_progress = filter_in_progress_disputes(all_disputes);

src/ui/dispute_finalization_popup.rs (1)

373-382: Duplicate helper and missing bounds check.

This center_rect function duplicates helpers::create_centered_popup but lacks the safety check that clamps dimensions to the available area. On small terminals, this could cause layout issues.

Consider reusing the existing helper from super::helpers.

♻️ Suggested change

-/// Helper function to center a rect
-fn center_rect(area: Rect, width: u16, height: u16) -> Rect {
-    let [popup] = Layout::horizontal([Constraint::Length(width)])
-        .flex(Flex::Center)
-        .areas(area);
-    let [popup] = Layout::vertical([Constraint::Length(height)])
-        .flex(Flex::Center)
-        .areas(popup);
-    popup
-}

Then update the import and usage:

+use super::helpers;
 // ...
-let popup = center_rect(area, popup_width, popup_height);
+let popup = helpers::create_centered_popup(area, popup_width, popup_height);

src/ui/exit_confirm.rs (1)

26-37: Layout constraints may exceed available height.

The vertical layout allocates 8 lines total (1+1+1+3+1+1), but the popup height is 11 with 2 lines consumed by borders, leaving 9 lines for content. This works, but the magic numbers make it fragile.

Consider using Constraint::Min(0) for a spacer to absorb any discrepancy, or add a comment documenting the height calculation.

src/util/order_utils/execute_admin_settle.rs (1)

33-73: LGTM with optional consolidation opportunity.

The implementation correctly handles the AdminSettle action. However, this function shares ~90% of its code with execute_admin_cancel. Consider extracting the common logic into a shared helper that accepts the Action variant as a parameter.

♻️ Optional consolidation

async fn execute_admin_action(
    dispute_id: &Uuid,
    client: &Client,
    mostro_pubkey: PublicKey,
    action: Action,
    action_name: &str,
) -> Result<()> {
    let settings = SETTINGS.get().ok_or(anyhow::anyhow!("Settings not initialized"))?;
    if settings.admin_privkey.is_empty() {
        return Err(anyhow::anyhow!("Admin private key not configured"));
    }
    let admin_keys = Keys::parse(&settings.admin_privkey)?;
    
    let message = Message::new_dispute(Some(*dispute_id), None, None, action, None)
        .as_json()
        .map_err(|_| anyhow::anyhow!("Failed to serialize message"))?;
    
    send_dm(client, Some(&admin_keys), &admin_keys, &mostro_pubkey, message, None, false).await?;
    log::info!("✅ Admin {} message sent for dispute {}", action_name, dispute_id);
    Ok(())
}

// Then wrap as:
pub async fn execute_admin_settle(...) -> Result<()> {
    execute_admin_action(dispute_id, client, mostro_pubkey, Action::AdminSettle, "settle (pay buyer)").await
}

src/ui/mod.rs (1)

568-578: Remove redundant selected_dispute_idx reset.
There are two consecutive resets of the same field; one can be removed.

♻️ Suggested cleanup

         self.selected_dispute_idx = 0;
         self.selected_settings_option = 0;
-        self.selected_dispute_idx = 0;
         self.selected_in_progress_idx = 0;

src/ui/dispute_finalization_popup.rs (1)

22-35: Error popup still shows no message body.
Users only see the title; please render a short message (e.g., “Dispute not found”) inside the popup.

src/ui/dispute_finalization_popup.rs (1)

1-5: Enable wrapping to avoid clipped lines in the details view.
Long IDs, pubkeys, or payment method strings will be cut off. Consider Wrap { trim: true } for the Paragraph.

♻️ Proposed change

-use ratatui::widgets::{Block, Borders, Clear, Paragraph};
+use ratatui::widgets::{Block, Borders, Clear, Paragraph, Wrap};
@@
-    let paragraph = Paragraph::new(lines);
+    let paragraph = Paragraph::new(lines).wrap(Wrap { trim: true });
     f.render_widget(paragraph, area);

Also applies to: 288-289

src/ui/dispute_finalization_popup.rs (1)

121-156: Avoid mislabeling initiator/ratings when buyer or seller pubkeys are missing.
When a pubkey is None, is_initiator_buyer still gets forced into a buyer/seller decision, which can misattribute privacy and rating info. Consider a tri‑state (buyer/seller/unknown) role and show “Unknown” for privacy/ratings when role is not determinable.

🔧 Suggested direction (sketch)

-    let buyer_pubkey = dispute.buyer_pubkey.as_deref().unwrap_or("Unknown");
-    let seller_pubkey = dispute.seller_pubkey.as_deref().unwrap_or("Unknown");
-    let is_initiator_buyer = dispute.initiator_pubkey == buyer_pubkey;
+    let buyer_pubkey = dispute.buyer_pubkey.as_deref();
+    let seller_pubkey = dispute.seller_pubkey.as_deref();
+    let initiator_role = if buyer_pubkey == Some(dispute.initiator_pubkey.as_str()) {
+        Some(true) // buyer
+    } else if seller_pubkey == Some(dispute.initiator_pubkey.as_str()) {
+        Some(false) // seller
+    } else {
+        None
+    };

Then derive privacy/rating/role labels with a match initiator_role { Some(true) => ..., Some(false) => ..., None => "Unknown" }.

docs/ADMIN_DISPUTES.md (1)

603-607: Docs still mention ▶/◀ party indicators that the UI doesn’t render.
The chat header is “Sender - date - time” without arrow prefixes. Please remove or update this line to match the actual UI.

src/util/dm_utils/mod.rs (1)

203-207: Minor: Existing unwrap() on checked_sub_signed could panic.

While unlikely with reasonable since_time values, checked_sub_signed returns None on overflow. Consider using unwrap_or_else with a fallback or propagating the error for consistency with the improved error handling elsewhere.

♻️ Suggested fix

         if let Some(since_time) = since {
-            let since_time = chrono::Utc::now()
+            let since_time = match chrono::Utc::now()
                 .checked_sub_signed(chrono::Duration::minutes(*since_time))
-                .unwrap()
-                .timestamp() as u64;
+            {
+                Some(t) => t.timestamp() as u64,
+                None => {
+                    log::warn!("Invalid since_time value: {}", since_time);
+                    continue;
+                }
+            };

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

295-301: Minor: Redundant default_mode computation.

default_mode is already computed at line 261-264 and is in scope. The re-computation at lines 297-300 is unnecessary.

♻️ Suggested fix

     } else if let UiMode::AdminMode(AdminMode::ConfirmTakeDispute(_, _)) = &app.mode {
         // User cancelled taking the dispute
-        let default_mode = match app.user_role {
-            UserRole::User => UiMode::UserMode(UserMode::Normal),
-            UserRole::Admin => UiMode::AdminMode(AdminMode::Normal),
-        };
         app.mode = default_mode;
     }

docs/ADMIN_DISPUTES.md (1)

609-612: Remove arrow-prefix wording; UI uses sender/date/time headers instead.

The chat UI doesn’t render ▶/◀ prefixes; it shows "Sender - date - time" headers with color. Please adjust the doc accordingly.

✏️ Suggested doc update

-- **Party indicators**: `▶` for admin, `◀` for parties
+- **Chat header format**: "Sender - date - time" with sender-specific color

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

74-104: Minor: Redundant clone of solver_pubkey.

solver_pubkey is already an owned String passed by value. The clone on line 84 is unnecessary—you can move it directly into the async block.

♻️ Suggested simplification

 pub(crate) fn execute_add_solver_action(
     app: &mut AppState,
     solver_pubkey: String,
     client: &Client,
     mostro_pubkey: nostr_sdk::PublicKey,
     order_result_tx: &UnboundedSender<crate::ui::OrderResult>,
 ) {
     // Stay on Settings tab after confirmation
     app.mode = UiMode::AdminMode(AdminMode::Normal);

-    let solver_pubkey_clone = solver_pubkey.clone();
     let client_clone = client.clone();
     let result_tx = order_result_tx.clone();
     let mostro_pubkey_clone = mostro_pubkey;

     tokio::spawn(async move {
-        match execute_admin_add_solver(&solver_pubkey_clone, &client_clone, mostro_pubkey_clone)
+        match execute_admin_add_solver(&solver_pubkey, &client_clone, mostro_pubkey_clone)
             .await

---

325-444: Consider extracting the ManagingDispute handler into a separate function.

This match arm spans ~120 lines and handles multiple concerns: chat input validation, message persistence, mock response generation, auto-scrolling, and fallback to finalization popup. Extracting it to a dedicated handle_enter_managing_dispute() helper would improve readability and testability, consistent with other handle_enter_* functions in this file.

src/db.rs (1)

115-138: Missing "no such table" error handling in migration.

The check_column_exists helper only handles "no such column" errors. For databases that existed before the admin_disputes table was introduced, this function will fail with "no such table" and propagate the error, preventing the application from starting.

🔧 Suggested fix

             match result {
             Ok(_) => Ok(true), // Column exists (query succeeded)
             Err(e) => {
                 let error_msg = e.to_string().to_lowercase();
                 if error_msg.contains("no such column") {
                     Ok(false)
+                } else if error_msg.contains("no such table") {
+                    // Table doesn't exist yet - migration not needed for this column
+                    // The table will be created when needed by other code paths
+                    Ok(true)
                 } else {
                     Err(e.into())
                 }
             }
         }

src/main.rs (1)

192-220: String-based message detection remains fragile.

The detection of dispute-taken results still relies on substring matching (msg.contains("Dispute") && msg.contains("taken successfully")). This was flagged in a previous review and acknowledged for improvement.

src/ui/disputes_in_progress_tab.rs (1)

121-128: Long words exceeding line width not fully handled.

When a single word (e.g., a pubkey or long URL) exceeds available_width, the current logic increments lines by 1 and sets current_width = word_width. However, if word_width > available_width, this doesn't account for the word wrapping across multiple lines in ratatui's Paragraph widget with Wrap { trim: true }.

This was noted in a previous review but appears unaddressed. The input box height may be underestimated for very long tokens.

src/ui/disputes_tab.rs (1)

56-81: Consider documenting the purpose of _original_idx.

The original index is captured but unused. If it's reserved for future use (e.g., mapping selection back to the unfiltered list), a brief comment would clarify intent. Otherwise, simplify to .map(|(display_idx, (_, dispute))| ...) or even drop the enumerate on the initial filter if unneeded.

src/ui/dispute_finalization_confirm.rs (1)

17-62: Consider extracting shared dispute lookup and error popup logic.

This dispute lookup pattern and error popup rendering is duplicated in dispute_finalization_popup.rs. Extract to a shared helper in src/ui/helpers.rs to reduce duplication.

♻️ Example helper signature

// In src/ui/helpers.rs
pub fn find_dispute_by_id<'a>(
    disputes: &'a [AdminDispute],
    dispute_id: &uuid::Uuid,
) -> Option<&'a AdminDispute> {
    disputes.iter().find(|d| {
        d.dispute_id == dispute_id.to_string() || d.id == dispute_id.to_string()
    })
}

pub fn render_error_popup(
    f: &mut ratatui::Frame,
    title: &str,
    message: &str,
    dismiss_hint: &str,
);

src/main.rs (1)

141-159: Extract duplicate dispute filtering logic.

The filter for InProgress disputes is duplicated at startup (lines 145-153) and after dispute-taken refresh (lines 202-210). Extract to a helper for consistency.

♻️ Suggested extraction

fn filter_in_progress_disputes(disputes: Vec<AdminDispute>) -> Vec<AdminDispute> {
    disputes
        .into_iter()
        .filter(|d| d.status.as_ref().map(|s| s == "InProgress").unwrap_or(false))
        .collect()
}

Also applies to: 199-220

src/ui/dispute_finalization_popup.rs (1)

428-437: Consider using the existing create_centered_popup helper.

This center_rect function duplicates helpers::create_centered_popup (shown in relevant snippets from src/ui/helpers.rs). Consolidate to avoid duplicate implementations.

♻️ Suggested change

-fn center_rect(area: Rect, width: u16, height: u16) -> Rect {
-    let [popup] = Layout::horizontal([Constraint::Length(width)])
-        .flex(Flex::Center)
-        .areas(area);
-    let [popup] = Layout::vertical([Constraint::Length(height)])
-        .flex(Flex::Center)
-        .areas(popup);
-    popup
-}
+// Use: super::helpers::create_centered_popup(area, width, height)

Then replace center_rect(...) calls with super::helpers::create_centered_popup(...).

src/util/order_utils/execute_take_dispute.rs (1)

104-127: Potentially redundant status update.

The code sets status = "InProgress" on the cloned dispute_info_clone (line 106) before calling AdminDispute::new(), which should persist with that status. Then it immediately calls set_status_in_progress() again on lines 119-120.

If AdminDispute::new() succeeds with an insert, the second update is unnecessary. If it hits the upsert path (unique constraint violation → update), the update_db method already persists all fields including status from the clone.

Also note: line 118's comment says "Use dispute_info.id to ensure we're updating the correct record" but dispute_info.id is the order ID (not the dispute UUID), which is correct since that's the primary key in admin_disputes, but the comment could be clearer.

♻️ Consider removing redundant update

If you want to keep the defensive double-write for robustness, add a clarifying comment. Otherwise:

                 if let Err(e) =
                     AdminDispute::new(pool, dispute_info_clone, dispute_id.to_string()).await
                 {
                     log::error!("Failed to save dispute to database: {}", e);
                     return Err(anyhow::anyhow!("Failed to save dispute to database: {}", e));
                 }

-                // Also explicitly update status to ensure it's set (in case of update path)
-                // Use dispute_info.id to ensure we're updating the correct record
-                if let Err(e) =
-                    AdminDispute::set_status_in_progress(pool, &dispute_info.id.to_string()).await
-                {
-                    log::error!("Failed to update dispute status to InProgress: {}", e);
-                    return Err(anyhow::anyhow!(
-                        "Failed to update dispute status to InProgress: {}",
-                        e
-                    ));
-                }

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

284-340: Duplicated chat message filtering logic.

The visible_count calculation with the filter closure (lines 297-313) is repeated identically in PageUp/PageDown (lines 356-372) and End (lines 432-448) handlers. This makes the code harder to maintain.

♻️ Extract filtering into a helper

Consider extracting a helper function:

fn get_visible_message_count(
    messages: &[super::DisputeChatMessage],
    active_chat_party: super::ChatParty,
) -> usize {
    messages
        .iter()
        .filter(|msg| match msg.sender {
            super::ChatSender::Admin => msg.target_party == Some(active_chat_party),
            super::ChatSender::Buyer => active_chat_party == super::ChatParty::Buyer,
            super::ChatSender::Seller => active_chat_party == super::ChatParty::Seller,
        })
        .count()
}

Then reuse it in all three navigation handlers.

src/models.rs (1)

299-307: Silent failure on JSON serialization.

Using .ok() to convert serialization errors to None silently drops errors. If UserInfo fails to serialize (unlikely but possible with custom types), the data is lost without any indication.

♻️ Log serialization failures

         // Serialize UserInfo to JSON
         let initiator_info_json = dispute_info
             .initiator_info
             .as_ref()
-            .and_then(|info| serde_json::to_string(info).ok());
+            .and_then(|info| {
+                serde_json::to_string(info)
+                    .map_err(|e| log::warn!("Failed to serialize initiator_info: {}", e))
+                    .ok()
+            });
         let counterpart_info_json = dispute_info
             .counterpart_info
             .as_ref()
-            .and_then(|info| serde_json::to_string(info).ok());
+            .and_then(|info| {
+                serde_json::to_string(info)
+                    .map_err(|e| log::warn!("Failed to serialize counterpart_info: {}", e))
+                    .ok()
+            });

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

143-161: Redundant bounds clamping in dispute navigation.

The up-key handling for DisputesPending has multiple redundant checks:

1. Line 152: checks > 0 before decrementing
2. Line 154-156: else sets to 0 (but if already 0, this is a no-op)
3. Lines 158-160: clamps with .min() again

The logic is correct but overly defensive. The same pattern appears in the down-key handler (lines 254-262).

♻️ Simplified version

             } else if let Tab::Admin(AdminTab::DisputesPending) = app.active_tab {
                 // Only count disputes with "initiated" status
                 let disputes_lock = disputes.lock().unwrap();
                 let initiated_count = disputes_lock
                     .iter()
                     .filter(|d| d.status == "initiated")
                     .count();
-                if initiated_count > 0 {
-                    // Ensure index doesn't go below 0
-                    if app.selected_dispute_idx > 0 {
-                        app.selected_dispute_idx -= 1;
-                    } else {
-                        app.selected_dispute_idx = 0;
-                    }
-                    // Clamp to valid range
-                    app.selected_dispute_idx = app
-                        .selected_dispute_idx
-                        .min(initiated_count.saturating_sub(1));
-                }
+                if initiated_count > 0 && app.selected_dispute_idx > 0 {
+                    app.selected_dispute_idx = (app.selected_dispute_idx - 1)
+                        .min(initiated_count.saturating_sub(1));
+                }
             }

src/ui/disputes_in_progress_tab.rs (1)

466-473: Auto-scroll may interfere with manual navigation.

The condition on line 469 (current_selection.unwrap_or(0) >= total_items.saturating_sub(1)) resets to the bottom whenever the user is at or near the last message. This means if a user manually scrolls up and new messages arrive, or if they're viewing the second-to-last message, they'll be snapped to the bottom.

Consider only auto-scrolling when selection is None (initial state or after party switch) to preserve user's manual scroll position.

♻️ Preserve manual scroll position

         if total_items > 0 {
             let current_selection = app.admin_chat_list_state.selected();
-            // Reset to bottom if: no selection, selection is out of bounds, or selection is at the end
-            if current_selection.is_none()
-                || current_selection.unwrap_or(0) >= total_items
-                || current_selection.unwrap_or(0) >= total_items.saturating_sub(1)
-            {
+            // Reset to bottom only if: no selection or selection is out of bounds
+            if current_selection.is_none() || current_selection.unwrap_or(0) >= total_items {
                 app.admin_chat_list_state
                     .select(Some(total_items.saturating_sub(1)));
             }

src/ui/helpers.rs (1)

170-188: Minor: Unnecessary string cloning.

msg.content.clone() on lines 176 and 185 clones the message content for each displayed message. Since ListItem can work with borrowed content and the messages slice outlives the returned items, you could potentially avoid these clones.

However, given the chat message volumes are typically small and this is UI code that runs once per frame, this is a very minor optimization concern.

src/main.rs (1)

186-208: String-based message detection remains fragile.

This is a duplicate of the previous review comment. The detection logic still relies on matching substrings "Dispute" and "taken successfully" or "settled"/"canceled" in the info message. The developer has acknowledged this will be improved.

src/ui/dispute_finalization_popup.rs (1)

136-141: Initiator role detection is unreliable when pubkeys are unknown.

When buyer_pubkey is "Unknown" (line 136), the comparison dispute.initiator_pubkey == buyer_pubkey (line 139) will always be false. This can misattribute the initiator badge, privacy settings, and rating information to the wrong party.

Consider comparing against the original Option values before defaulting to "Unknown":

🔧 Suggested fix

-    let buyer_pubkey = dispute.buyer_pubkey.as_deref().unwrap_or("Unknown");
-    let seller_pubkey = dispute.seller_pubkey.as_deref().unwrap_or("Unknown");
-
-    let is_initiator_buyer = dispute.initiator_pubkey == buyer_pubkey;
+    let is_initiator_buyer = dispute
+        .buyer_pubkey
+        .as_ref()
+        .map(|bp| dispute.initiator_pubkey == *bp)
+        .unwrap_or(false);
+    let is_initiator_seller = dispute
+        .seller_pubkey
+        .as_ref()
+        .map(|sp| dispute.initiator_pubkey == *sp)
+        .unwrap_or(false);
+    let initiator_role_unknown = !is_initiator_buyer && !is_initiator_seller;
+
+    let buyer_pubkey = dispute.buyer_pubkey.as_deref().unwrap_or("Unknown");
+    let seller_pubkey = dispute.seller_pubkey.as_deref().unwrap_or("Unknown");

Then update the buyer/seller role display (lines 258-283) to show "Role Unknown" when initiator_role_unknown is true.

src/ui/disputes_in_progress_tab.rs (1)

128-157: Long single-word tokens can cause input height miscalculation.

The word-wrapping logic increments lines by 1 when a word doesn't fit, but doesn't account for words wider than available_width (e.g., a pubkey pasted without spaces). This can cause the input box to be too short and clip text.

🔧 Proposed fix

                 if current_width + space_width + word_width > available_width {
                     // Word doesn't fit, wrap to next line
                     lines += 1;
-                    current_width = word_width;
+                    // If the word itself exceeds the width, account for extra lines
+                    if word_width > available_width {
+                        let full_lines = word_width / available_width;
+                        lines += full_lines;
+                        current_width = word_width % available_width;
+                        if current_width == 0 {
+                            current_width = available_width;
+                        }
+                    } else {
+                        current_width = word_width;
+                    }
                 } else {

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

276-280: Don't re-enable chat input when input is empty.

Pressing Enter with an empty input still forces admin_chat_input_enabled = true, which defeats the purpose of the Shift+I toggle. If the admin explicitly disabled input, it should remain disabled.

🐛 Suggested fix

                 } else {
-                    // If input is empty, ensure input stays enabled and mode is correct
+                    // If input is empty or disabled, keep the current enabled state
                     app.mode = UiMode::AdminMode(AdminMode::ManagingDispute);
-                    app.admin_chat_input_enabled = true;
                 }

src/models.rs (3)

299-307: Silent failure on JSON serialization.

If serde_json::to_string fails, the error is silently swallowed via .ok(), and None is stored. Consider logging a warning when serialization fails to aid debugging.

Proposed improvement

         let initiator_info_json = dispute_info
             .initiator_info
             .as_ref()
-            .and_then(|info| serde_json::to_string(info).ok());
+            .and_then(|info| {
+                serde_json::to_string(info)
+                    .map_err(|e| log::warn!("Failed to serialize initiator_info: {}", e))
+                    .ok()
+            });
         let counterpart_info_json = dispute_info
             .counterpart_info
             .as_ref()
-            .and_then(|info| serde_json::to_string(info).ok());
+            .and_then(|info| {
+                serde_json::to_string(info)
+                    .map_err(|e| log::warn!("Failed to serialize counterpart_info: {}", e))
+                    .ok()
+            });

---

496-502: Silent failure on JSON deserialization.

Similar to serialization, deserialization errors are silently ignored. For debugging purposes, consider logging when JSON parsing fails.

Proposed improvement

     fn deserialize_user_info(&mut self) {
         if let Some(ref json_str) = self.initiator_info {
-            self.initiator_info_data = serde_json::from_str(json_str).ok();
+            self.initiator_info_data = serde_json::from_str(json_str)
+                .map_err(|e| log::warn!("Failed to deserialize initiator_info: {}", e))
+                .ok();
         }
         if let Some(ref json_str) = self.counterpart_info {
-            self.counterpart_info_data = serde_json::from_str(json_str).ok();
+            self.counterpart_info_data = serde_json::from_str(json_str)
+                .map_err(|e| log::warn!("Failed to deserialize counterpart_info: {}", e))
+                .ok();
         }
     }

---

521-527: Consider using constants or the DisputeStatus enum for status strings.

The status values 'in-progress', 'settled', and 'seller-refunded' are hardcoded strings. Using DisputeStatus::InProgress.to_string() (as done in execute_take_dispute.rs line 106) would ensure consistency and catch typos at compile time.

Proposed improvement

     pub async fn set_status_in_progress(pool: &SqlitePool, dispute_id: &str) -> Result<()> {
-        sqlx::query(r#"UPDATE admin_disputes SET status = 'in-progress' WHERE id = ?"#)
+        sqlx::query(r#"UPDATE admin_disputes SET status = ? WHERE id = ?"#)
+            .bind(DisputeStatus::InProgress.to_string())
             .bind(dispute_id)
             .execute(pool)
             .await?;
         Ok(())
     }

Apply similar changes to set_status_settled and set_status_seller_refunded.

Also applies to: 533-539, 545-551

src/util/order_utils/execute_take_dispute.rs (2)

104-127: Redundant status update to InProgress.

The status is set to InProgress in the cloned dispute_info_clone (line 106), then AdminDispute::new is called which persists this status. The subsequent call to set_status_in_progress (lines 119-127) is redundant since the record was just created/updated with InProgress status.

The comment mentions "in case of update path", but AdminDispute::new already handles the upsert and includes the status in the update.

Proposed simplification

                 // Clone and override status to InProgress before saving - this admin is now resolving it
                 let mut dispute_info_clone = dispute_info.clone();
                 dispute_info_clone.status = DisputeStatus::InProgress.to_string();

                 // Save dispute info to database with InProgress status
                 // Pass the dispute_id (from the function parameter) to distinguish it from order_id
                 if let Err(e) =
                     AdminDispute::new(pool, dispute_info_clone, dispute_id.to_string()).await
                 {
                     log::error!("Failed to save dispute to database: {}", e);
                     return Err(anyhow::anyhow!("Failed to save dispute to database: {}", e));
                 }

-                // Also explicitly update status to ensure it's set (in case of update path)
-                // Use dispute_info.id to ensure we're updating the correct record
-                if let Err(e) =
-                    AdminDispute::set_status_in_progress(pool, &dispute_info.id.to_string()).await
-                {
-                    log::error!("Failed to update dispute status to InProgress: {}", e);
-                    return Err(anyhow::anyhow!(
-                        "Failed to update dispute status to InProgress: {}",
-                        e
-                    ));
-                }
                 log::info!(

---

94-102: Mismatch in error message references.

On line 100, the error message uses dispute_info.id but the comparison on line 96 uses *id (from the payload). While both should be the same value, using id in the error message would be more consistent with what was actually compared.

Proposed fix

                 // Verify the dispute ID matches
                 if *id != *dispute_id {
                     return Err(anyhow::anyhow!(
                         "Dispute ID mismatch: expected {}, got {}",
                         dispute_id,
-                        dispute_info.id
+                        id
                     ));
                 }

src/ui/disputes_tab.rs (2)

22-24: Redundant imports.

mostro_core::prelude::* is already imported at line 4, and std::str::FromStr is likely also available through the prelude or could be imported at the top of the file.

Proposed fix

+use std::str::FromStr;
 use std::sync::{Arc, Mutex};

 use chrono::DateTime;
 use mostro_core::prelude::*;
 // ... rest of imports

 pub fn render_disputes_tab(
     // ...
 ) {
     let disputes_lock = disputes.lock().unwrap();

     // Filter to only show disputes with "initiated" status
-    use mostro_core::prelude::*;
-    use std::str::FromStr;
     let initiated_disputes: Vec<(usize, &Dispute)> = disputes_lock

---

25-33: Unused _original_idx in the collected tuple.

The original index is stored but never used. If it's not needed, you can simplify the collection to just filter the disputes directly.

Proposed simplification

-    let initiated_disputes: Vec<(usize, &Dispute)> = disputes_lock
+    let initiated_disputes: Vec<&Dispute> = disputes_lock
         .iter()
-        .enumerate()
-        .filter(|(_, dispute)| {
+        .filter(|dispute| {
             DisputeStatus::from_str(dispute.status.as_str())
                 .map(|s| s == DisputeStatus::Initiated)
                 .unwrap_or(false)
         })
         .collect();
 
 // ... and in the rendering:
 
-        let rows: Vec<Row> = initiated_disputes
-            .iter()
-            .enumerate()
-            .map(|(display_idx, (_original_idx, dispute))| {
+        let rows: Vec<Row> = initiated_disputes
+            .iter()
+            .enumerate()
+            .map(|(display_idx, dispute)| {

Also applies to: 62-65

src/ui/dispute_finalization_confirm.rs (1)

64-67: Fixed popup dimensions may not fit small terminals.

The popup uses fixed dimensions (70x15) which may overflow on terminals smaller than these dimensions. Consider making the popup responsive like the error popup case (lines 25-26) which uses relative sizing.

Proposed improvement

     let area = f.area();
-    let popup_width = 70;
-    let popup_height = 15;
+    let popup_width = 70.min(area.width.saturating_sub(4));
+    let popup_height = 15.min(area.height.saturating_sub(2));

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

135-135: Empty handler for PayInvoice action.

The PayInvoice match arm is empty. If this is intentional (e.g., PayInvoice doesn't require user action), consider adding a comment explaining why. If it's a TODO, mark it explicitly.

Proposed improvement

-        Action::PayInvoice => {}
+        Action::PayInvoice => {
+            // PayInvoice is display-only; no user action required
+        }

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

261-287: Consider extracting finalized status check to a shared helper.

The finalized status check logic (lines 267-283) is duplicated from dispute_finalization_popup.rs (lines 107-117). Consider extracting this to a shared utility function in the models or helpers module to reduce duplication.

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

90-96: Minor: Redundant .min() call in PageUp handling.

The .min(visible_count.saturating_sub(1)) on line 95 is redundant since current.saturating_sub(10) already produces a value ≤ current, which is already ≤ visible_count - 1. This doesn't cause issues but adds unnecessary computation.

♻️ Suggested simplification

         crossterm::event::KeyCode::PageUp => {
             // Scroll up (show older messages) - move selection up by ~10 items
-            current
-                .saturating_sub(10)
-                .min(visible_count.saturating_sub(1))
+            current.saturating_sub(10)
         }

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

168-171: Duplicate navigation logic for DisputesInProgress tab.

The up-navigation for DisputesInProgress is duplicated between AdminMode::Normal (lines 168-171) and AdminMode::ManagingDispute (lines 199-206). Consider consolidating by matching both modes together:

♻️ Suggested consolidation

         UiMode::Normal
         | UiMode::UserMode(UserMode::Normal)
-        | UiMode::AdminMode(AdminMode::Normal) => {
+        | UiMode::AdminMode(AdminMode::Normal)
+        | UiMode::AdminMode(AdminMode::ManagingDispute) => {
             // ... existing code ...
         }
-        UiMode::AdminMode(AdminMode::ManagingDispute) => {
-            // Navigate within disputes in progress list
-            if let Tab::Admin(AdminTab::DisputesInProgress) = app.active_tab {
-                if !app.admin_disputes_in_progress.is_empty() && app.selected_in_progress_idx > 0 {
-                    app.selected_in_progress_idx -= 1;
-                }
-            }
-        }

Also applies to: 199-206

src/ui/disputes_in_progress_tab.rs (2)

263-275: Consider clamping rating before star calculation.

The star display uses (info.rating / 2.0).round() as usize then .repeat(). While .take(5) caps the output, an unexpectedly large rating value would cause unnecessary allocation (e.g., rating of 1000 → .repeat(500) before truncation).

♻️ Suggested improvement

-                    let stars = "⭐"
-                        .repeat((info.rating / 2.0).round() as usize)
-                        .chars()
-                        .take(5)
-                        .collect::<String>();
+                    let star_count = ((info.rating / 2.0).round() as usize).min(5);
+                    let stars = "⭐".repeat(star_count);

---

561-574: Simplify redundant selection bounds check.

The condition at lines 565-566 is redundant: >= total_items is always true when >= total_items.saturating_sub(1) is true (for total_items > 0). The intent seems to be "reset to bottom if selection is invalid or at end."

♻️ Simplified condition

-                if current_selection.is_none()
-                    || current_selection.unwrap_or(0) >= total_items
-                    || current_selection.unwrap_or(0) >= total_items.saturating_sub(1)
-                {
+                // Reset to bottom if: no selection or selection is at/past the end
+                if current_selection.map_or(true, |sel| sel >= total_items.saturating_sub(1)) {

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

78-82: Silent parse failure could mask invalid amount input.

Using .parse::<i64>().ok() silently converts parse failures to None. Since the validation at lines 60-70 already checks for empty input and validation errors, a parse failure here indicates the validation logic didn't catch an invalid format. Consider logging or handling this case explicitly.

♻️ More explicit handling

     let amount = if take_state_clone.is_range_order {
-        take_state_clone.amount_input.trim().parse::<i64>().ok()
+        match take_state_clone.amount_input.trim().parse::<i64>() {
+            Ok(amt) => Some(amt),
+            Err(e) => {
+                log::warn!("Amount parse failed after validation: {}", e);
+                None
+            }
+        }
     } else {
         None
     };

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

202-211: Mock responses are placeholder - ensure tracking for replacement.

The mock response array is clearly temporary per the TODO at lines 198-199. Consider adding a tracking issue or more visible marker to ensure this gets replaced with real Nostr DM integration.

Would you like me to open an issue to track replacing the mock chat responses with real Nostr DM integration?

src/main.rs (1)

187-210: String-based message detection remains fragile.

This detection logic still relies on substring matching (msg.contains("Dispute"), msg.contains("taken successfully"), etc.). This was flagged in a previous review, and you acknowledged you'd improve it with a structured approach (e.g., a dedicated enum variant like OrderResult::DisputeTaken).

src/ui/dispute_finalization_popup.rs (1)

125-130: Initiator detection still unreliable when buyer_pubkey is unknown.

When buyer_pubkey is "Unknown", the comparison dispute.initiator_pubkey == buyer_pubkey on line 128 will always be false, incorrectly marking the seller as the initiator. This was noted in past reviews.

Consider comparing against the original Option values:

🔧 Suggested fix

     let buyer_pubkey = dispute.buyer_pubkey.as_deref().unwrap_or("Unknown");
     let seller_pubkey = dispute.seller_pubkey.as_deref().unwrap_or("Unknown");

-    let is_initiator_buyer = dispute.initiator_pubkey == buyer_pubkey;
+    // Compare against original Option to avoid "Unknown" false matches
+    let is_initiator_buyer = dispute
+        .buyer_pubkey
+        .as_ref()
+        .map(|bp| dispute.initiator_pubkey == *bp)
+        .unwrap_or(false);
+    
+    let is_initiator_seller = dispute
+        .seller_pubkey
+        .as_ref()
+        .map(|sp| dispute.initiator_pubkey == *sp)
+        .unwrap_or(false);
+    
+    // When neither matches, initiator role is unknown
+    let initiator_role_known = is_initiator_buyer || is_initiator_seller;
     let buyer_pubkey_display = truncate_pubkey(buyer_pubkey);
     let seller_pubkey_display = truncate_pubkey(seller_pubkey);

src/ui/disputes_in_progress_tab.rs (1)

125-156: Handle long tokens in input height calculation.

Long words (e.g., pubkeys) wider than available_width still increment lines by 1, so the input box can be too short and text clips.

🛠️ Suggested fix

-                    if current_width + space_width + word_width > available_width {
-                        // Word doesn't fit, wrap to next line
-                        lines += 1;
-                        current_width = word_width;
+                    if current_width + space_width + word_width > available_width {
+                        // Word doesn't fit, wrap to next line
+                        lines += 1;
+                        // If the word itself exceeds the width, account for extra lines
+                        if word_width > available_width {
+                            let full_lines = word_width / available_width;
+                            lines += full_lines;
+                            current_width = word_width % available_width;
+                            if current_width == 0 {
+                                current_width = available_width;
+                            }
+                        } else {
+                            current_width = word_width;
+                        }
                     } else {

src/models.rs (2)

299-307: Silent serialization failures could lead to data loss.

If serde_json::to_string fails, the error is silently swallowed with .ok(), resulting in None being stored. This could cause user info to be lost without any indication.

Consider logging when serialization fails:

🔧 Suggested fix

         let initiator_info_json = dispute_info
             .initiator_info
             .as_ref()
-            .and_then(|info| serde_json::to_string(info).ok());
+            .and_then(|info| {
+                serde_json::to_string(info)
+                    .map_err(|e| log::warn!("Failed to serialize initiator_info: {}", e))
+                    .ok()
+            });
         let counterpart_info_json = dispute_info
             .counterpart_info
             .as_ref()
-            .and_then(|info| serde_json::to_string(info).ok());
+            .and_then(|info| {
+                serde_json::to_string(info)
+                    .map_err(|e| log::warn!("Failed to serialize counterpart_info: {}", e))
+                    .ok()
+            });

---

519-529: Status update methods don't verify affected rows.

The set_status_in_progress, set_status_settled, and set_status_seller_refunded methods don't check if the UPDATE actually modified any rows. If the dispute ID doesn't exist, the operation silently succeeds.

Consider checking rows_affected() and returning an error or logging when no rows were updated:

🔧 Suggested fix for set_status_in_progress

     pub async fn set_status_in_progress(pool: &SqlitePool, dispute_id: &str) -> Result<()> {
-        sqlx::query(r#"UPDATE admin_disputes SET status = ? WHERE id = ?"#)
+        let result = sqlx::query(r#"UPDATE admin_disputes SET status = ? WHERE id = ?"#)
             .bind(DisputeStatus::InProgress.to_string())
             .bind(dispute_id)
             .execute(pool)
             .await?;
+        if result.rows_affected() == 0 {
+            log::warn!("No dispute found with id {} to update", dispute_id);
+        }
         Ok(())
     }

Also applies to: 531-555

src/util/order_utils/execute_take_dispute.rs (1)

86-90: Only first response message is processed.

If multiple DM events arrive, only the first is processed. While this is likely fine for the typical case, you may want to log if additional messages are received for debugging purposes.

src/ui/dispute_finalization_popup.rs (1)

436-444: Duplicate center_rect helper function.

This center_rect function duplicates the logic in src/ui/helpers.rs::create_centered_popup. Consider reusing the existing helper to reduce code duplication.

src/ui/mod.rs (1)

580-589: Consider resetting admin chat flags on role switch.

switch_role clears chat input and resets selection, but leaves admin_chat_input_enabled, admin_chat_list_state, and dispute_filter untouched. That can restore a stale/disabled admin UI when switching back.

♻️ Suggested adjustment

         self.selected_in_progress_idx = 0;
         self.active_chat_party = ChatParty::Buyer;
         self.admin_chat_input.clear();
+        self.admin_chat_input_enabled = true;
+        self.admin_chat_list_state = ratatui::widgets::ListState::default();
+        self.dispute_filter = DisputeFilter::InProgress;

src/ui/disputes_in_progress_tab.rs (1)

125-157: Input height calculation doesn't handle single long words spanning multiple lines.

The word-wrapping calculation at lines 141-148 treats any word that doesn't fit on the current line as wrapping to the next line, but doesn't account for words longer than available_width that would span multiple lines themselves. This was noted in a past review comment.

♻️ Suggested fix for long word handling

                     if current_width + space_width + word_width > available_width {
                         // Word doesn't fit, wrap to next line
                         lines += 1;
-                        current_width = word_width;
+                        // Handle words longer than available width
+                        if word_width > available_width {
+                            let extra_lines = word_width / available_width;
+                            lines += extra_lines;
+                            current_width = word_width % available_width;
+                            if current_width == 0 {
+                                current_width = available_width;
+                            }
+                        } else {
+                            current_width = word_width;
+                        }
                     } else {

src/ui/dispute_finalization_popup.rs (1)

176-184: Use of .expect() after validation is acceptable but fragile.

The code validates buyer_pubkey and seller_pubkey at lines 70-118 before reaching this point, so the .expect() calls should never panic. However, if future refactoring changes the control flow, these could become runtime panics.

Consider using a more defensive approach that returns early with an error display rather than panicking:

♻️ Optional defensive approach

-    let buyer_pubkey = dispute
-        .buyer_pubkey
-        .as_deref()
-        .expect("buyer_pubkey should be validated before rendering");
-    let seller_pubkey = dispute
-        .seller_pubkey
-        .as_deref()
-        .expect("seller_pubkey should be validated before rendering");
+    let (buyer_pubkey, seller_pubkey) = match (
+        dispute.buyer_pubkey.as_deref(),
+        dispute.seller_pubkey.as_deref(),
+    ) {
+        (Some(bp), Some(sp)) => (bp, sp),
+        _ => return, // Should not happen due to earlier validation
+    };

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

161-170: Minor redundancy in up navigation bounds checking.

The else branch at line 163-164 is redundant since if app.selected_dispute_idx > 0 is false, it's already 0. The subsequent clamp at lines 167-169 also makes the earlier conditional partially redundant.

♻️ Optional simplification

                 if initiated_count == 0 {
                     app.selected_dispute_idx = 0;
                 } else {
-                    // Ensure index doesn't go below 0
-                    if app.selected_dispute_idx > 0 {
-                        app.selected_dispute_idx -= 1;
-                    } else {
-                        app.selected_dispute_idx = 0;
-                    }
-                    // Clamp to valid range
-                    app.selected_dispute_idx = app
-                        .selected_dispute_idx
-                        .min(initiated_count.saturating_sub(1));
+                    app.selected_dispute_idx = app
+                        .selected_dispute_idx
+                        .saturating_sub(1)
+                        .min(initiated_count.saturating_sub(1));
                 }

src/models.rs (2)

317-329: Hardcoded "USD" fallback may cause incorrect fiat currency display.

When the related order is not found, the code defaults fiat_code to "USD". This could lead to incorrect currency display in the admin UI if the actual dispute involves a different fiat currency.

Consider logging at warn level instead of debug since this represents a data integrity concern, or storing the fiat_code in the dispute info received from Mostro if available.

♻️ Suggested improvement

             Err(_) => {
-                // Order not found, use default
-                log::debug!(
+                // Order not found - this may cause incorrect currency display
+                log::warn!(
                     "Order not found for dispute {}, using default fiat_code",
                     dispute_info.id
                 );
                 "USD".to_string()
             }

---

527-537: Confusing parameter naming in set_status_in_progress.

The parameter is named dispute_id but is used in WHERE id = ?, where id is the order ID (primary key). The other status methods (set_status_settled, set_status_seller_refunded) correctly name their parameter order_id.

♻️ Suggested fix for consistency

-    pub async fn set_status_in_progress(pool: &SqlitePool, dispute_id: &str) -> Result<()> {
+    pub async fn set_status_in_progress(pool: &SqlitePool, order_id: &str) -> Result<()> {
         sqlx::query(r#"UPDATE admin_disputes SET status = ? WHERE id = ?"#)
             .bind(DisputeStatus::InProgress.to_string())
-            .bind(dispute_id)
+            .bind(order_id)
             .execute(pool)
             .await?;
         Ok(())
     }

src/util/order_utils/execute_finalize_dispute.rs (1)

58-63: Consider adding error context for the order_id lookup.

The raw SQL query will return an error if no dispute with the given dispute_id exists, but the error message won't indicate what went wrong. Adding context would improve debugging.

♻️ Suggested improvement

     let order_id: String = sqlx::query_scalar::<_, String>(
         r#"SELECT id FROM admin_disputes WHERE dispute_id = ? LIMIT 1"#,
     )
     .bind(&dispute_id_str)
     .fetch_one(pool)
-    .await?;
+    .await
+    .map_err(|e| anyhow::anyhow!("Failed to find dispute {}: {}", dispute_id, e))?;