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

101-135: Optimize duplicate handling using HashMap pattern.

The current approach clones the entire list (line 119) and uses nested iteration (O(n²) complexity) to filter duplicates. For consistency and efficiency, consider adopting the same HashMap-based deduplication pattern used in parse_orders_events (lines 144-196).

♻️ Proposed optimization using HashMap

 pub fn parse_disputes_events(events: Events) -> Vec<Dispute> {
-    let mut disputes_list = Vec::<Dispute>::new();
+    let mut latest_by_id: std::collections::HashMap<uuid::Uuid, Dispute> = std::collections::HashMap::new();
 
     // Scan events to extract all disputes
     for event in events.iter() {
         let mut dispute = match dispute_from_tags(event.tags.clone()) {
             Ok(d) => d,
             Err(e) => {
                 log::warn!("Failed to parse dispute from tags: {:?}", e);
                 continue;
             }
         };
         // Get created at field from Nostr event
         dispute.created_at = event.created_at.as_u64() as i64;
-        disputes_list.push(dispute);
+        
+        // Keep only the latest dispute for each id
+        latest_by_id
+            .entry(dispute.id)
+            .and_modify(|existing| {
+                if dispute.created_at > existing.created_at {
+                    *existing = dispute.clone();
+                }
+            })
+            .or_insert(dispute);
     }
 
-    let buffer_dispute_list = disputes_list.clone();
-    // Order all elements (disputes) received to filter - discard disaligned messages
-    // if a dispute has an older message with the state we received is discarded for the latest one
-    disputes_list.retain(|keep| {
-        !buffer_dispute_list
-            .iter()
-            .any(|x| x.id == keep.id && x.created_at > keep.created_at)
-    });
-
-    // Sort by id to remove duplicates
-    disputes_list.sort_by(|a, b| b.id.cmp(&a.id));
-    disputes_list.dedup_by(|a, b| a.id == b.id);
-
+    let mut disputes_list: Vec<Dispute> = latest_by_id.into_values().collect();
+    
     // Finally sort list by creation time (newest first)
     disputes_list.sort_by(|a, b| b.created_at.cmp(&a.created_at));
     disputes_list
 }

src/ui/disputes_tab.rs (1)

20-20: Consider handling mutex poisoning gracefully.

Using .unwrap() on a mutex lock will panic if the mutex is poisoned (i.e., a thread panicked while holding the lock). While this is often acceptable in single-threaded UI contexts, you could consider using lock().expect("disputes mutex poisoned") for a clearer panic message, or handle the error to avoid crashing the UI.

src/ui/settings_tab.rs (1)

63-88: Redundant highlight styling.

The items are manually styled based on selection (lines 67-74), and highlight_style is also set on the list (line 81). Since the highlighting is already applied to individual items, the highlight_style on the List widget is redundant and could be removed for clarity.

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

39-52: Fire-and-forget operation lacks server confirmation.

The DM is sent without waiting for a response from Mostro. While the comment acknowledges this, consider adding logging for debugging purposes and ensure the calling UI code provides appropriate feedback that the operation was sent but not confirmed.

Suggested improvement

     // Send the DM using admin keys (signed gift wrap)
     // Note: Following the example pattern, we don't wait for a response
+    log::info!("Sending AdminAddSolver request for solver: {}", solver_pubkey);
     send_dm(
         client,
         Some(&admin_keys),
         &admin_keys,
         &mostro_pubkey,
         add_solver_message,
         None,
         false,
     )
     .await?;
 
+    log::info!("AdminAddSolver request sent successfully");
     Ok(())
 }

src/ui/key_input_popup.rs (1)

31-43: Layout constraint comments could be clearer.

The layout works correctly, but the inline comments are slightly misleading. When is_sensitive=true, chunks[2] (labeled "spacer") is actually used for the label, while chunks[1] (labeled "warning or label") gets the warning. Consider updating comments to reflect the actual usage pattern.

📝 Suggested clarification

     let chunks = Layout::new(
         Direction::Vertical,
         [
             Constraint::Length(1),                                // spacer
-            Constraint::Length(if is_sensitive { 2 } else { 1 }), // warning (if sensitive) or label
-            Constraint::Length(1),                                // spacer
+            Constraint::Length(if is_sensitive { 2 } else { 1 }), // warning (if sensitive) or label (if not)
+            Constraint::Length(1),                                // label (if sensitive) or spacer
             Constraint::Length(3),                                // input field
             Constraint::Length(1),                                // spacer
             Constraint::Length(1),                                // help text
             Constraint::Length(1),                                // help text
         ],
     )

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

5-18: Consider returning a Result or logging when SETTINGS is not initialized.

When SETTINGS.get() returns None, the function silently does nothing. While this may be acceptable for a TUI app where settings should always be initialized, consider at minimum logging a warning for debugging purposes.

📝 Suggested improvement

 pub fn save_settings_with<F>(update_fn: F, error_msg: &str, success_msg: &str)
 where
     F: FnOnce(&mut crate::settings::Settings),
 {
     if let Some(settings) = SETTINGS.get() {
         let mut new_settings = settings.clone();
         update_fn(&mut new_settings);
         if let Err(e) = crate::settings::save_settings(&new_settings) {
             log::error!("{}: {}", error_msg, e);
         } else {
             log::info!("{}", success_msg);
         }
+    } else {
+        log::warn!("Settings not initialized, cannot save");
     }
 }

---

51-67: Remove unused _settings parameter.

The _settings parameter is not used in handle_mode_switch. If it's intentionally reserved for future use, consider documenting that. Otherwise, remove it to clean up the API.

♻️ Suggested fix

-/// Handle mode switching (M key in Settings tab)
-pub fn handle_mode_switch(app: &mut AppState, _settings: &crate::settings::Settings) {
+/// Handle mode switching (M key in Settings tab)
+pub fn handle_mode_switch(app: &mut AppState) {
     let new_role = match app.user_role {
         UserRole::User => UserRole::Admin,
         UserRole::Admin => UserRole::User,
     };

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

40-56: Consider handling mutex poison and logging fetch errors.

1. .unwrap() on lock() will panic if a thread previously panicked while holding the lock. Consider using .lock().expect("mutex poisoned") for clearer panic messages, or handle the PoisonError.

2. Fetch failures are silently ignored. Consider logging errors for debugging network issues.

♻️ Suggested improvements

             if let Ok(fetched_orders) = get_orders(
                 &client_for_orders,
                 mostro_pubkey_for_orders,
                 Some(Status::Pending),
             )
             .await
             {
-                let mut orders_lock = orders_clone.lock().unwrap();
+                let mut orders_lock = orders_clone.lock().expect("orders mutex poisoned");
                 orders_lock.clear();
                 orders_lock.extend(fetched_orders);
+            } else {
+                log::debug!("Failed to fetch orders, will retry");
             }

---

63-76: Same improvements apply to disputes fetcher.

Apply consistent error logging and mutex handling as suggested for the orders fetcher.

♻️ Suggested improvements

             if let Ok(fetched_disputes) =
                 get_disputes(&client_for_disputes, mostro_pubkey_for_disputes).await
             {
-                let mut disputes_lock = disputes_clone.lock().unwrap();
+                let mut disputes_lock = disputes_clone.lock().expect("disputes mutex poisoned");
                 disputes_lock.clear();
                 disputes_lock.extend(fetched_disputes);
+            } else {
+                log::debug!("Failed to fetch disputes, will retry");
             }

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

204-216: Extract magic numbers for settings options count.

The max_options values (3 for Admin, 1 for User) are hardcoded. If the settings UI changes, this will be a source of bugs. Consider defining these as constants or deriving from the actual settings options.

♻️ Suggested improvement

+// At module level or in a constants module
+const ADMIN_SETTINGS_OPTIONS_COUNT: usize = 3;
+const USER_SETTINGS_OPTIONS_COUNT: usize = 1;

 // In handle_down_key:
-                let max_options = if app.user_role == UserRole::Admin {
-                    3
-                } else {
-                    1
-                };
+                let max_options = if app.user_role == UserRole::Admin {
+                    ADMIN_SETTINGS_OPTIONS_COUNT
+                } else {
+                    USER_SETTINGS_OPTIONS_COUNT
+                };

---

117-126: Consider consistent mutex handling pattern.

Multiple .unwrap() calls on lock() throughout the navigation handlers. While mutex poisoning is unlikely in a TUI app, using .expect() with descriptive messages would aid debugging if issues arise.

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

44-54: Consider validating decimal point input to prevent multiple dots.

The current logic allows multiple decimal points (e.g., "12.34.56"), which would fail parsing later. Consider checking if a decimal already exists before allowing another.

♻️ Suggested fix

             } else if let UiMode::UserMode(UserMode::TakingOrder(ref mut take_state)) = app.mode {
                 // Allow typing in the amount input field for range orders
                 if take_state.is_range_order {
                     // Only allow digits and decimal point
-                    if c.is_ascii_digit() || c == '.' {
+                    if c.is_ascii_digit() || (c == '.' && !take_state.amount_input.contains('.')) {
                         take_state.amount_input.push(c);
                         // Validate after typing
                         validate_range_amount(take_state);
                     }
                 }
             }

src/ui/message_notification.rs (1)

288-316: Consider reusing render_order_id_header for consistency.

The order ID rendering at lines 307-316 duplicates the logic in render_order_id_header (lines 9-21) but with slightly different styling (missing bg(BACKGROUND_COLOR)). Consider using the existing helper for consistency.

♻️ Suggested refactor

 fn render_default_notification(
     f: &mut ratatui::Frame,
     popup: Rect,
     notification: &MessageNotification,
 ) {
     let chunks = Layout::new(
         Direction::Vertical,
         [
             Constraint::Length(1), // spacer
             Constraint::Length(1), // order id
             Constraint::Length(1), // message preview
             Constraint::Length(1), // spacer
             Constraint::Length(1), // help text
         ],
     )
     .split(popup);

     let order_id_str = helpers::format_order_id(notification.order_id);
-    f.render_widget(
-        Paragraph::new(Line::from(vec![Span::styled(
-            order_id_str,
-            Style::default()
-                .fg(PRIMARY_COLOR)
-                .add_modifier(Modifier::BOLD),
-        )]))
-        .alignment(ratatui::layout::Alignment::Center),
-        chunks[1],
-    );
+    render_order_id_header(f, chunks[1], &order_id_str);

src/ui/tab_content.rs (1)

45-56: Redundant character collection - .take() with full length has no effect.

The code collects characters up to the full length of the string, which is equivalent to just using .to_string(). This appears to be leftover from a truncation that was removed. Consider using the helpers::format_order_id function for consistency with other parts of the codebase.

♻️ Suggested fix

             let order_id_str = if let Some(order_id) = msg.order_id {
-                format!(
-                    "Order: {}",
-                    order_id
-                        .to_string()
-                        .chars()
-                        .take(order_id.to_string().len())
-                        .collect::<String>()
-                )
+                helpers::format_order_id(Some(order_id))
             } else {
-                "Order: Unknown".to_string()
+                helpers::format_order_id(None)
             };

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

354-358: Inconsistent mode reset - uses UiMode::Normal instead of role-based default_mode.

When the user selects NO in the TakingOrder confirmation, the mode is set to UiMode::Normal instead of the role-based default_mode. This could cause issues if an admin is taking an order (though currently TakingOrder is user-mode only, this inconsistency could cause problems if the code evolves).

♻️ Suggested fix

     } else {
         // NO selected - cancel
-        app.mode = UiMode::Normal;
+        let default_mode = match app.user_role {
+            UserRole::User => UiMode::UserMode(UserMode::Normal),
+            UserRole::Admin => UiMode::AdminMode(AdminMode::Normal),
+        };
+        app.mode = default_mode;
     }

---

484-489: Empty handler for PayInvoice and silent error for other actions.

The PayInvoice arm is empty (Enter does nothing), which seems intentional since PayInvoice is display-only. However, the error case for other actions (lines 485-488) sends an error but the mode transition was already handled by the caller's std::mem::replace. Consider adding a comment to clarify this is intentional.

📝 Consider adding clarifying comments

         Action::PayInvoice => {
+            // Enter does nothing on PayInvoice - it's display-only
+            // User must press Esc to dismiss or C to copy
         }
         _ => {
+            // Unexpected action in notification context - mode already reset by caller
             let _ =
                 order_result_tx.send(crate::ui::OrderResult::Error("Invalid action".to_string()));
         }

---

258-276: Magic numbers for settings options could be fragile.

The settings option indices (0, 1, 2, 3) are hardcoded and must stay in sync with the rendering order in settings_tab.rs. Consider using an enum or constants for these options to prevent drift.

src/main.rs (1)

194-223: Paste handling logic is duplicated for invoice and admin key inputs.

The paste handling for invoice input (lines 196-206) and admin key input (lines 209-221) share identical filtering logic. Consider extracting a helper function to reduce duplication.

♻️ Example helper extraction

fn filter_pasted_text(text: &str) -> String {
    text.chars()
        .filter(|c| !c.is_control() || *c == '\t')
        .collect()
}

Then use it in both paste handlers:

let filtered_text = filter_pasted_text(&pasted_text);
invoice_state.invoice_input.push_str(&filtered_text);

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

112-169: Significant code duplication with enter_handlers.rs.

The TakingOrder handling logic (validation, parsing, async task spawning) is nearly identical to handle_enter_taking_order in enter_handlers.rs (lines 289-358). Consider extracting a shared helper to avoid maintaining two copies of this logic.

♻️ Suggested approach

Extract the core order-taking logic into a shared function in a common module:

// In a shared module or within enter_handlers.rs
pub fn execute_take_order(
    app: &mut AppState,
    take_state: TakeOrderState,
    pool: &SqlitePool,
    client: &Client,
    mostro_pubkey: nostr_sdk::PublicKey,
    order_result_tx: &UnboundedSender<crate::ui::OrderResult>,
) -> bool {
    // ... shared validation and execution logic
}

Then call this from both handle_enter_taking_order and handle_confirm_key.

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

185-191: Double handling of Backspace key.

Both handle_char_input and handle_backspace are called when Backspace is pressed. Looking at the referenced code in form_input.rs, handle_char_input only matches on Char(' ') and Char(c), so Backspace falls through to _ => {}. This works but is confusing - the code structure suggests both handle the same key.

♻️ Clearer structure

-        KeyCode::Char(_) | KeyCode::Backspace => {
-            handle_char_input(code, app, validate_range_amount);
-            if code == KeyCode::Backspace {
-                handle_backspace(app, validate_range_amount);
-            }
-            Some(true)
-        }
+        KeyCode::Char(_) => {
+            handle_char_input(code, app, validate_range_amount);
+            Some(true)
+        }
+        KeyCode::Backspace => {
+            handle_backspace(app, validate_range_amount);
+            Some(true)
+        }

src/ui/mod.rs (1)

419-425: KeyInputState structure mirrors InvoiceInputState - consider unifying.

KeyInputState has nearly identical fields to InvoiceInputState (lines 412-417), differing only in the copied_to_clipboard field. Consider whether these could share a common base or trait to reduce duplication.

♻️ Potential unified structure

#[derive(Clone, Debug)]
pub struct TextInputState {
    pub input: String,
    pub focused: bool,
    pub just_pasted: bool,
}

// Then InvoiceInputState could wrap this with additional fields:
#[derive(Clone, Debug)]
pub struct InvoiceInputState {
    pub base: TextInputState,
    pub copied_to_clipboard: bool,
}

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/ui/settings_tab.rs (1)

64-89: Consider consolidating the highlighting approach.

The code manually applies styling to each ListItem based on selected_option (lines 68-75), then also sets highlight_style and highlight_symbol on the List widget (lines 82-83). This is redundant—typically you either style items manually or rely on the List's built-in highlighting, not both. Simplifying to one approach would improve maintainability.

♻️ Option 1: Use only manual item styling

 let list = List::new(items)
-    .block(Block::default().borders(Borders::NONE))
-    .highlight_style(Style::default().bg(PRIMARY_COLOR).fg(Color::Black))
-    .highlight_symbol(">> ");
+    .block(Block::default().borders(Borders::NONE));
 
 f.render_stateful_widget(
     list,
     chunks[3],
-    &mut ratatui::widgets::ListState::default().with_selected(Some(selected_option)),
+    &mut ratatui::widgets::ListState::default(),
 );

♻️ Option 2: Use only List highlighting

 let items: Vec<ListItem> = options
     .iter()
-    .enumerate()
-    .map(|(idx, opt)| {
-        let style = if idx == selected_option {
-            Style::default()
-                .bg(PRIMARY_COLOR)
-                .fg(Color::Black)
-                .add_modifier(Modifier::BOLD)
-        } else {
-            Style::default().fg(Color::White)
-        };
-        ListItem::new(Line::from(vec![Span::styled(*opt, style)]))
-    })
+    .map(|opt| ListItem::new(Line::from(*opt)))
     .collect();

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

tests/helper_tests.rs (1)

5-46: Consider using an exhaustive match or macro to ensure all variants are tested.

The manual vector of CantDoReason variants is brittle—if new variants are added to the enum in mostro-core, this test won't fail to remind developers to add coverage. Consider refactoring to use a match statement with #[deny(non_exhaustive_omitted_patterns)] or a macro that iterates all variants automatically, ensuring compile-time completeness.

♻️ Alternative approach using strum

If mostro-core's CantDoReason derives strum::IntoEnumIterator, you could iterate all variants automatically:

+use strum::IntoEnumIterator;
+
 #[test]
 fn test_get_cant_do_description_all_reasons() {
-    // Test all CantDoReason variants return non-empty descriptions
-    let reasons = vec![
-        CantDoReason::InvalidSignature,
-        CantDoReason::InvalidTradeIndex,
-        // ... (all variants)
-        CantDoReason::TooManyRequests,
-    ];
-
-    for reason in reasons {
+    for reason in CantDoReason::iter() {
         let description = get_cant_do_description(&reason);
         assert!(
             !description.is_empty(),
             "Description should not be empty for {:?}",
             reason
         );
     }
 }

Alternatively, if strum is not available, consider a compile-time exhaustive match that ensures all variants are covered.

tests/common/mod.rs (1)

6-43: Monitor for schema drift between test and production databases.

The hardcoded SQL schema in create_test_db may diverge from the production schema over time, causing tests to pass while production code fails. Consider one of the following approaches to maintain consistency:

1. Shared schema definition: Extract the schema to a common location used by both tests and migrations.
2. Run production migrations: Apply the same migrations used in production to the test database.
3. Schema validation tests: Add a test that compares the test schema against the production schema.

tests/db_tests.rs (1)

92-92: Consider using deterministic keys for more reliable tests.

Using Keys::generate() introduces non-determinism into the tests. While the current tests may not be sensitive to specific key values, deterministic keys (derived from a fixed seed or mnemonic) improve debuggability and ensure consistent test behavior across runs.

♻️ Use deterministic key generation

You could create a helper similar to test_mnemonic():

// In tests/common/mod.rs
pub fn test_trade_keys() -> Keys {
    Keys::from_mnemonic_advanced(
        "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about",
        None,
        Some(1), // Different account than identity keys
        Some(0),
        Some(0)
    ).unwrap()
}

Then use it in tests:

-let trade_keys = Keys::generate();
+let trade_keys = common::test_trade_keys();

Also applies to: 117-117, 152-152

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

68-99: Validate required dispute fields.

The function allows disputes with missing "d" tags to succeed with a default UUID value. This could lead to issues if the dispute ID is used as a unique key downstream or if multiple disputes end up with the same default ID.

Consider validating that required fields are present before returning Ok:

💡 Suggested validation enhancement

 pub fn dispute_from_tags(tags: Tags) -> Result<Dispute> {
     let mut dispute = Dispute::default();
+    let mut has_id = false;
+    let mut has_status = false;
+
     for tag in tags {
         let t = tag.to_vec();
 
         // Check if tag has at least 2 elements
         if t.len() < 2 {
             continue;
         }
 
         let key = t.first().map(|s| s.as_str()).unwrap_or("");
         let value = t.get(1).map(|s| s.as_str()).unwrap_or("");
 
         match key {
             "d" => {
                 let id = value
                     .parse::<Uuid>()
                     .map_err(|_| anyhow::anyhow!("Invalid dispute id"))?;
                 dispute.id = id;
+                has_id = true;
             }
             "s" => {
                 let status = DisputeStatus::from_str(value)
                     .map_err(|_| anyhow::anyhow!("Invalid dispute status"))?;
                 dispute.status = status.to_string();
+                has_status = true;
             }
             _ => {}
         }
     }
+
+    if !has_id {
+        return Err(anyhow::anyhow!("Missing required dispute id tag"));
+    }
+    if !has_status {
+        return Err(anyhow::anyhow!("Missing required dispute status tag"));
+    }
 
     Ok(dispute)
 }

---

101-135: Refactor to use HashMap for better performance.

The current implementation clones the entire dispute list (line 119) for deduplication, which is inefficient for large event sets. The similar function parse_orders_events (lines 138-196 in this file) uses a HashMap<Uuid, T> to track the latest entry per ID without cloning, which is more efficient.

♻️ Refactor to use HashMap approach

 pub fn parse_disputes_events(events: Events) -> Vec<Dispute> {
-    let mut disputes_list = Vec::<Dispute>::new();
+    let mut latest_by_id: HashMap<Uuid, Dispute> = HashMap::new();
 
     // Scan events to extract all disputes
     for event in events.iter() {
         let mut dispute = match dispute_from_tags(event.tags.clone()) {
             Ok(d) => d,
             Err(e) => {
                 log::warn!("Failed to parse dispute from tags: {:?}", e);
                 continue;
             }
         };
         // Get created at field from Nostr event
         dispute.created_at = event.created_at.as_u64() as i64;
-        disputes_list.push(dispute);
+        
+        // Update latest dispute by id
+        latest_by_id
+            .entry(dispute.id)
+            .and_modify(|existing| {
+                if dispute.created_at > existing.created_at {
+                    *existing = dispute.clone();
+                }
+            })
+            .or_insert(dispute);
     }
 
-    let buffer_dispute_list = disputes_list.clone();
-    // Order all elements (disputes) received to filter - discard disaligned messages
-    // if a dispute has an older message with the state we received is discarded for the latest one
-    disputes_list.retain(|keep| {
-        !buffer_dispute_list
-            .iter()
-            .any(|x| x.id == keep.id && x.created_at > keep.created_at)
-    });
-
-    // Sort by id to remove duplicates
-    disputes_list.sort_by(|a, b| b.id.cmp(&a.id));
-    disputes_list.dedup_by(|a, b| a.id == b.id);
+    let mut disputes_list: Vec<Dispute> = latest_by_id.into_values().collect();
 
     // Finally sort list by creation time (newest first)
     disputes_list.sort_by(|a, b| b.created_at.cmp(&a.created_at));
     disputes_list
 }

tests/validation_tests.rs (1)

1-34: LGTM: Comprehensive test coverage for npub validation.

The test suite effectively covers the key validation scenarios:

• Valid npub generation and validation
• Empty and whitespace inputs
• Various invalid format patterns
• Whitespace trimming behavior

The tests use nostr-sdk's key generation to ensure realistic test data.

Optional: Verify error messages for better diagnostic coverage

You could optionally enhance the tests to verify specific error messages:

#[test]
fn test_validate_npub_error_messages() {
    let err = validate_npub("").unwrap_err();
    assert_eq!(err, "Public key cannot be empty");
    
    let err = validate_npub("invalid").unwrap_err();
    assert_eq!(err, "Invalid pubkey format");
}

This ensures the error messages remain stable and helpful.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/ui/admin_key_confirm.rs (3)

65-87: Clarify the comment to match the simple logic.

The comment on lines 66-67 suggests complex conditional logic ("only show key if no custom message... or if custom message is provided but we still want to show it"), but the actual implementation is straightforward: the key is displayed only when custom_message.is_none(). Additionally, mentioning "AddSolver" specifically in a generic rendering function reduces clarity.

📝 Suggested comment simplification

-    // Display truncated key (show first 30 chars + ...)
-    // Only show key if no custom message (for settings saves) or if custom message is provided but we still want to show it
-    // For AddSolver, we hide the key display
+    // Display truncated key only when using the default message
+    // When a custom message is provided, the key is hidden
     if custom_message.is_none() {

---

69-73: Consider showing more characters or using prefix+suffix pattern for better key identification.

Truncating npub keys to 30 characters may not provide enough visual distinction for users to confidently verify which key they're confirming. Consider either:

• Showing more characters (e.g., 50) if the 80-width popup allows
• Using a prefix+suffix pattern (e.g., first 20 chars + "..." + last 8 chars) to show both ends of the key

🔍 Example prefix+suffix pattern

-        let display_key = if key_string.len() > 30 {
-            format!("{}...", &key_string[..30])
+        let display_key = if key_string.len() > 50 {
+            format!("{}...{}", &key_string[..20], &key_string[key_string.len()-8..])
         } else {
             key_string.to_string()
         };

---

113-177: Consider extracting a helper function to reduce button rendering duplication.

The YES and NO button rendering logic (lines 113-146 and 147-177) follows the same pattern with only minor variations (selection condition, color, label). Extracting a helper function would improve maintainability and reduce ~30 lines of duplication.

♻️ Example helper extraction

fn render_confirmation_button(
    f: &mut ratatui::Frame,
    area: Rect,
    label: &str,
    color: Color,
    is_selected: bool,
) {
    let style = if is_selected {
        Style::default()
            .bg(color)
            .fg(Color::Black)
            .add_modifier(Modifier::BOLD)
    } else {
        Style::default()
            .fg(color)
            .add_modifier(Modifier::BOLD)
    };

    let block = Block::default().borders(Borders::ALL).style(style);
    f.render_widget(block, area);

    let inner = Layout::new(Direction::Vertical, [Constraint::Min(0)])
        .margin(1)
        .split(area);

    f.render_widget(
        Paragraph::new(Line::from(vec![Span::styled(
            label,
            Style::default()
                .fg(if is_selected { Color::Black } else { color })
                .add_modifier(Modifier::BOLD),
        )]))
        .alignment(ratatui::layout::Alignment::Center),
        inner[0],
    );
}

Then use it:

render_confirmation_button(f, button_chunks[0], "✓ YES", Color::Green, selected_button);
render_confirmation_button(f, button_chunks[2], "✗ NO", Color::Red, !selected_button);

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

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

296-364: Consider extracting duplicated take_order logic.

The take order validation and async task spawning logic (lines 296-364) is duplicated in confirmation.rs (lines 111-168). Both implementations validate range orders, parse amounts, and spawn the same async task.

♻️ Potential refactor approach

Consider extracting a shared helper function:

fn spawn_take_order_task(
    app: &mut AppState,
    take_state: TakeOrderState,
    pool: &SqlitePool,
    client: &Client,
    mostro_pubkey: nostr_sdk::PublicKey,
    order_result_tx: &UnboundedSender<crate::ui::OrderResult>,
) {
    // Validation
    if take_state.is_range_order {
        if take_state.amount_input.is_empty() || take_state.validation_error.is_some() {
            app.mode = UiMode::UserMode(UserMode::TakingOrder(take_state));
            return;
        }
    }
    
    // Set waiting mode and spawn task
    // ... (rest of the logic)
}

This would reduce duplication and ensure consistency between Enter and 'y' key handlers.

---

410-411: Misleading mode name for send message operation.

The code sets WaitingAddInvoice mode but then calls execute_send_msg (not execute_add_invoice). This mode name is misleading since it's used for sending FiatSent/Release messages, not adding invoices.

Consider using a more generic waiting mode (e.g., WaitingForMostro or a new WaitingSendMessage mode) to better represent the actual operation being performed.

---

265-283: Consider using named constants for settings option indices.

The settings option selection uses magic numbers (0, 1, 2, 3) which reduces readability and maintainability. If the settings menu order changes, these indices need manual updates.

♻️ Suggested approach

Define constants at the module level or use an enum:

const SETTINGS_MOSTRO_PUBKEY: usize = 0;
const SETTINGS_RELAY: usize = 1;
const SETTINGS_ADD_SOLVER: usize = 2;  // Admin only
const SETTINGS_ADMIN_KEY: usize = 3;    // Admin only

// Then use in the match:
match app.selected_settings_option {
    SETTINGS_MOSTRO_PUBKEY => {
        app.mode = UiMode::AddMostroPubkey(key_state);
    }
    SETTINGS_RELAY => {
        app.mode = UiMode::AddRelay(key_state);
    }
    // ...
}

This makes the code more self-documenting and easier to maintain.

---

31-34: Consider extracting default mode calculation to a helper.

The default mode calculation based on user_role is repeated throughout this file (lines 31-34, 388-392, 402-405) and in confirmation.rs (lines 75-78, 170-173, 184-187, 198-201, 220-223).

♻️ Suggested approach

Add a method to AppState or a helper function:

// In AppState impl
impl AppState {
    pub fn default_mode(&self) -> UiMode {
        match self.user_role {
            UserRole::User => UiMode::UserMode(UserMode::Normal),
            UserRole::Admin => UiMode::AdminMode(AdminMode::Normal),
        }
    }
}

// Usage:
let default_mode = app.default_mode();

This eliminates duplication and ensures consistency across all default mode calculations.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

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

68-99: Validate required fields before returning a Dispute.

The function can return a Dispute with default (potentially invalid) values if the "d" or "s" tags are missing entirely. This concern was previously raised and remains unaddressed.

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

44-53: Inconsistent mode reset for notification dismissal.

Lines 46 and 51 set app.mode to UiMode::Normal directly, while other Esc handlers use the role-specific default_mode. This could lead to inconsistent UI state after dismissing notifications vs. escaping from other modes.

Suggested fix

         UiMode::NewMessageNotification(_, _, _) => {
             // Dismiss notification
-            app.mode = UiMode::Normal;
+            app.mode = default_mode.clone();
             true
         }
         UiMode::ViewingMessage(_) => {
             // Dismiss message view popup
-            app.mode = UiMode::Normal;
+            app.mode = default_mode.clone();
             true
         }

README.md (1)

52-52: Clarify that the example nsec key is a placeholder.

The nsec key in the example configuration should be clearly marked as a placeholder to prevent accidental use. Consider adding explicit "EXAMPLE ONLY - REPLACE THIS" marking or using an obviously invalid format.

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

610-610: PayInvoice action has no implementation.

The PayInvoice branch is empty while AddInvoice (lines 573-608) has a complete implementation. At line 346, both AddInvoice and PayInvoice are checked as actionable message types, but only AddInvoice is handled here.

If PayInvoice requires special handling (e.g., copying invoice to clipboard, displaying payment instructions), please implement it. Otherwise, consider adding a comment explaining intentional no-op behavior.

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

232-250: Missing AdminMode::ConfirmAddSolver handling for 'y' key.

The handle_confirm_key function handles 'y' key confirmations for ConfirmMostroPubkey, ConfirmRelay, ConfirmCurrency, ConfirmClearCurrencies, and ConfirmAdminKey, but not for AdminMode::ConfirmAddSolver.

Meanwhile, handle_cancel_key (lines 273-276) does handle ConfirmAddSolver for the 'n' key, and enter_handlers.rs handles it for Enter. This creates an inconsistency where users can confirm with Enter and cancel with 'n', but cannot confirm with 'y'.

🔧 Suggested fix

Add handling for ConfirmAddSolver after line 244:

         UiMode::AdminMode(AdminMode::ConfirmAdminKey(key_string, _)) => {
             // ... existing code ...
             true
         }
+        UiMode::AdminMode(AdminMode::ConfirmAddSolver(solver_pubkey, _)) => {
+            // YES selected - send AddSolver message
+            let solver_pubkey_clone = solver_pubkey.clone();
+            let client_clone = client.clone();
+            let result_tx = order_result_tx.clone();
+
+            app.mode = UiMode::AdminMode(AdminMode::Normal);
+
+            tokio::spawn(async move {
+                match crate::util::order_utils::execute_admin_add_solver(
+                    &solver_pubkey_clone,
+                    &client_clone,
+                    mostro_pubkey,
+                )
+                .await
+                {
+                    Ok(_) => {
+                        let _ = result_tx.send(crate::ui::OrderResult::Info(
+                            "Solver added successfully".to_string(),
+                        ));
+                    }
+                    Err(e) => {
+                        log::error!("Failed to add solver: {}", e);
+                        let _ = result_tx.send(crate::ui::OrderResult::Error(e.to_string()));
+                    }
+                }
+            });
+            true
+        }
         mode => {
             app.mode = mode;
             false
         }

settings.toml (1)

2-2: Placeholder format inconsistency with validation expectations.

The mostro_pubkey placeholder suggests hex format, but nsec_privkey uses nsec1_ prefix which is bech32 (not hex). Since the code supports both formats for pubkey validation, consider making placeholder names format-agnostic or documenting both accepted formats clearly.

Suggested clarification

 # Mostro pubkey, it can be hex or npub
-mostro_pubkey = "mostro_pubkey_hex_format"
+mostro_pubkey = "your_mostro_pubkey_here"
 # Admin private key - leave empty for user mode
 admin_privkey=""
 # Relays to connect to
 relays = ["wss://relay.mostro.network"]
 log_level = "info"
 currencies = ["VES", "ARS", "USD"]
-nsec_privkey='nsec1_privkey_format'
+nsec_privkey='your_nsec_privkey_here'

Also applies to: 9-9

src/settings.rs (2)

46-51: Consider propagating the home directory error instead of panicking.

Line 48 uses expect() which will panic if the home directory cannot be determined. Since this function already returns a Result, it would be more consistent to propagate this error.

Suggested fix

 fn init_or_load_settings_from_disk() -> Result<Settings, anyhow::Error> {
     // HOME and package name at compile time
-    let home_dir = dirs::home_dir().expect("Could not find home directory");
+    let home_dir = dirs::home_dir()
+        .ok_or_else(|| anyhow::anyhow!("Could not find home directory"))?;
     let package_name = env!("CARGO_PKG_NAME");
     let hidden_dir = home_dir.join(format!(".{package_name}"));
     let hidden_file = hidden_dir.join("settings.toml");

---

85-90: Duplicate path construction logic.

The path construction for hidden_file is duplicated between init_or_load_settings_from_disk() and save_settings(). Consider extracting a helper function to reduce duplication and ensure consistency.

Suggested helper function

/// Get the path to the settings file
fn get_settings_file_path() -> Result<PathBuf, anyhow::Error> {
    let home_dir = dirs::home_dir()
        .ok_or_else(|| anyhow::anyhow!("Could not find home directory"))?;
    let package_name = env!("CARGO_PKG_NAME");
    Ok(home_dir.join(format!(".{package_name}")).join("settings.toml"))
}

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

43-53: Minor optimization: avoid duplicate string allocation.

The relay_string.to_string() is called twice when the relay is not already present. Consider using a local variable to avoid the extra allocation.

Suggested fix

 pub fn save_relay_to_settings(relay_string: &str) {
+    let relay = relay_string.to_string();
     save_settings_with(
         |s| {
-            if !s.relays.contains(&relay_string.to_string()) {
-                s.relays.push(relay_string.to_string());
+            if !s.relays.contains(&relay) {
+                s.relays.push(relay.clone());
             }
         },
         "Failed to save relay to settings",
         "Relay added to settings file",
     );
 }

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

119-130: Consider a more efficient deduplication strategy.

The current approach clones the entire disputes_list to buffer_dispute_list for comparison. For large dispute sets, this is O(n²) with an extra O(n) memory overhead. Consider using a HashMap keyed by id to track the latest created_at per dispute, similar to the approach in parse_orders_events.

Suggested optimization

 pub fn parse_disputes_events(events: Events) -> Vec<Dispute> {
-    let mut disputes_list = Vec::<Dispute>::new();
+    use std::collections::HashMap;
+    let mut latest_by_id: HashMap<Uuid, Dispute> = HashMap::new();

     // Scan events to extract all disputes
     for event in events.iter() {
         let mut dispute = match dispute_from_tags(event.tags.clone()) {
             Ok(d) => d,
             Err(e) => {
                 log::warn!("Failed to parse dispute from tags: {:?}", e);
                 continue;
             }
         };
         // Get created at field from Nostr event
         dispute.created_at = event.created_at.as_u64() as i64;
-        disputes_list.push(dispute);
+        
+        latest_by_id
+            .entry(dispute.id)
+            .and_modify(|existing| {
+                if dispute.created_at > existing.created_at {
+                    *existing = dispute.clone();
+                }
+            })
+            .or_insert(dispute);
     }

-    let buffer_dispute_list = disputes_list.clone();
-    // Order all elements (disputes) received to filter - discard disaligned messages
-    // if a dispute has an older message with the state we received is discarded for the latest one
-    disputes_list.retain(|keep| {
-        !buffer_dispute_list
-            .iter()
-            .any(|x| x.id == keep.id && x.created_at > keep.created_at)
-    });
-
-    // Sort by id to remove duplicates
-    disputes_list.sort_by(|a, b| b.id.cmp(&a.id));
-    disputes_list.dedup_by(|a, b| a.id == b.id);
-
-    // Finally sort list by creation time (newest first)
-    disputes_list.sort_by(|a, b| b.created_at.cmp(&a.created_at));
-    disputes_list
+    let mut disputes: Vec<Dispute> = latest_by_id.into_values().collect();
+    disputes.sort_by(|a, b| b.created_at.cmp(&a.created_at));
+    disputes
 }

src/ui/settings_tab.rs (1)

76-95: Redundant highlight_style configuration.

The list items are pre-styled with selection highlighting at lines 80-87, making the highlight_style on line 94 ineffective. The highlight_symbol on line 95 will still work, but highlight_style won't override the already-applied inline styles.

Consider removing the redundant highlight_style for clarity, or removing the inline selection styling and relying solely on highlight_style.

♻️ Option 1: Remove redundant highlight_style

     let list = List::new(items)
         .block(Block::default().borders(Borders::NONE))
-        .highlight_style(Style::default().bg(PRIMARY_COLOR).fg(Color::Black))
         .highlight_symbol(">> ");

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

56-67: Silent failure on fetch errors hinders debugging.

Failed fetches are silently ignored with if let Ok(...). Consider logging errors to help diagnose connectivity or protocol issues.

♻️ Proposed improvement for orders fetch

-            if let Ok(fetched_orders) = get_orders(
+            match get_orders(
                 &client_for_orders,
                 mostro_pubkey_for_orders,
                 Some(Status::Pending),
                 currencies,
             )
             .await
             {
-                let mut orders_lock = orders_clone.lock().unwrap();
-                orders_lock.clear();
-                orders_lock.extend(fetched_orders);
+                Ok(fetched_orders) => {
+                    let mut orders_lock = orders_clone.lock().unwrap();
+                    orders_lock.clear();
+                    orders_lock.extend(fetched_orders);
+                }
+                Err(e) => {
+                    tracing::warn!("Failed to fetch orders: {}", e);
+                }
             }

Also applies to: 80-86

---

46-54: Disk I/O on every fetch cycle.

load_settings_from_disk() is called every 10 seconds to reload currencies. While acceptable, consider caching settings with a file watcher or longer refresh interval if this becomes a performance concern.

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

219-220: Remove duplicate doc comment.

Line 220 is an exact duplicate of line 219.

🧹 Suggested fix

 /// Handle Enter key for settings-related modes (Mostro pubkey, relay, currency, etc.)
-/// Handle Enter key for settings-related modes (Mostro pubkey, relay, currency, etc.)
 fn handle_enter_settings_mode(

---

161-182: Consider adding a waiting state for AddSolver operation.

Unlike order operations that set WaitingForMostro or WaitingTakeOrder states before spawning async tasks (e.g., line 441), the AddSolver flow immediately returns to Normal mode at line 162. If the network call is slow, users may be confused about whether the action was triggered.

This is a minor UX consideration—the current implementation works correctly, and the result popup will eventually show success/error feedback.

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

127-131: Clarify return value semantics from handle_confirm_key.

The handle_confirm_key function returns bool indicating whether the key was handled (per its doc comment "Returns true if should continue"), but the caller wraps it in Some(should_continue). This is confusing because:

• true from handle_confirm_key means "key was handled, skip further processing"
• But it's returned as Some(true) which means "continue the main loop"

The current logic works correctly, but a comment clarifying the semantics would improve maintainability.

---

187-192: Restructure Backspace handling for clarity.

When KeyCode::Backspace is pressed, the code calls handle_char_input(code, ...) even though that function doesn't handle Backspace (it falls through to the default case). While Backspace is ultimately handled only once by handle_backspace, separating this into different match arms improves code clarity and avoids the unnecessary function call:

🔧 Suggested fix

-        KeyCode::Char(_) | KeyCode::Backspace => {
-            handle_char_input(code, app, validate_range_amount);
-            if code == KeyCode::Backspace {
-                handle_backspace(app, validate_range_amount);
-            }
+        KeyCode::Char(_) => {
+            handle_char_input(code, app, validate_range_amount);
+            Some(true)
+        }
+        KeyCode::Backspace => {
+            handle_backspace(app, validate_range_amount);
             Some(true)
         }

docs/ADMIN_DISPUTES.md (1)

383-393: Minor discrepancy in error message between documentation and code.

The error message shown in the documentation (line 389) is "Invalid key format", but the actual code in src/validation.rs returns "Invalid npub key format". Consider syncing these for accuracy.

src/main.rs (1)

278-293: Settings reloaded from disk on every draw cycle.

load_settings_from_disk() is called every 500ms (per refresh_interval). While this ensures newly added settings are reflected immediately, it introduces disk I/O on every refresh cycle.

For a TUI application this trade-off is likely acceptable, but consider caching with a file modification timestamp check if performance becomes a concern.

src/ui/mod.rs (3)

285-291: Consider using direct string literals for clarity.

The current pattern using to_string() in match guards works but is indirect. Using string literals directly would be clearer and avoid the runtime to_string() allocation.

♻️ Suggested refactor

     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        match s.to_ascii_lowercase() {
-            val if val == UserRole::User.to_string() => Ok(UserRole::User),
-            val if val == UserRole::Admin.to_string() => Ok(UserRole::Admin),
-            _ => Err(anyhow::anyhow!("Invalid user role: {s}")),
-        }
+        match s.to_ascii_lowercase().as_str() {
+            "user" => Ok(UserRole::User),
+            "admin" => Ok(UserRole::Admin),
+            _ => Err(anyhow::anyhow!("Invalid user role: {s}")),
+        }
     }

---

502-507: Consider resetting selected_dispute_idx when switching roles.

selected_settings_option is reset to 0 in switch_role(), but selected_dispute_idx is not. For consistency, consider resetting it as well to avoid a stale index when switching to admin mode.

♻️ Suggested fix

     pub fn switch_role(&mut self, new_role: UserRole) {
         self.user_role = new_role;
         self.active_tab = Tab::first(new_role);
         self.mode = UiMode::Normal;
+        self.selected_dispute_idx = 0;
         self.selected_settings_option = 0;
     }

---

708-717: Consider adding an emoji to the "Add Solver" title for consistency.

Other popup titles use emojis (🔐, 🌐, 📡, 💱), but "Add Solver" on line 711 doesn't. For visual consistency, consider adding an appropriate emoji.

♻️ Suggested change

     if let UiMode::AdminMode(AdminMode::AddSolver(key_state)) = &app.mode {
         key_input_popup::render_key_input_popup(
             f,
-            "Add Solver",
+            "👤 Add Solver",
             "Enter solver public key (npub...):",
             "npub...",
             key_state,
             false,
         );
     }

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}