Admin mode - Phase1 : Introducing Settings tab #24
Conversation
- Add dispute fetching and parsing from Nostr events - Create disputes tab UI component (admin mode only) - Add Up/Down navigation for disputes list - Refactor orders/disputes fetching into fetch_scheduler module - Update AppState with selected_dispute_idx tracking Disputes are fetched every 10 seconds and displayed in a dedicated admin tab following the same patterns as orders for consistency.
- Add confirmation popup when saving admin key to settings - Implement Yes/No button navigation with Left/Right keys - Save admin key to settings.toml file on confirmation - Refactor message notification rendering into separate module - Extract common UI patterns into helpers module - Optimize code organization per coding standards
…gs UX - Refactor key_handler.rs (1439 lines) into focused modules: * input_helpers: unified text input handling (eliminates duplication) * navigation: arrow keys and tab navigation * enter_handlers: Enter key logic * esc_handlers: Esc key handling * form_input: form character input * confirmation: confirmation/cancel handlers * settings: settings save helpers * validation: npub format validation * mod: main dispatcher and public API - Add npub validation for AddSolver and AddMostroPubkey inputs * Shows error popup with clear message on invalid format - Improve AddSolver confirmation UX: * Custom message: "Are you sure you want to add this pubkey as dispute solver?" * Hide pubkey display in confirmation popup * Preserve Settings tab after confirmation (no tab switching) - Fix tab navigation to preserve Settings tab when closing OrderResult popups All modules are under 500 lines, following coding standards. Functionality remains unchanged while improving maintainability and code organization.
WalkthroughRefactors background fetching into a scheduler exposing shared orders and disputes; splits monolithic key handling into modular submodules; adds dispute parsing, disputes UI, new popups/confirmations, admin add-solver DM flow, helpers, tests, and related util exports. Changes
Sequence Diagram(s)sequenceDiagram
participant App as main.rs (App)
participant Scheduler as FetchScheduler
participant Client as Nostr Client
participant Shared as Shared State (Arc<Mutex>)
App->>Scheduler: start_fetch_scheduler(client, mostro_pubkey)
activate Scheduler
loop immediate + every 10s
Scheduler->>Client: get_orders(Status::Pending)
Client-->>Scheduler: Vec<SmallOrder>
Scheduler->>Shared: replace orders vector
end
loop immediate + every 10s
Scheduler->>Client: get_disputes()
Client-->>Scheduler: Vec<Dispute>
Scheduler->>Shared: replace disputes vector
end
Scheduler-->>App: return FetchSchedulerResult (orders, disputes)
deactivate Scheduler
App->>App: ui_draw(..., orders, disputes)
sequenceDiagram
participant User as User Input
participant KeyH as Key Handler
participant Confirm as Confirmation Module
participant Action as Async Action (DB / Nostr)
participant Settings as SETTINGS (OnceLock)
User->>KeyH: KeyEvent
KeyH->>KeyH: dispatch by UiMode
alt Confirmation flow
KeyH->>Confirm: handle_confirm_key
Confirm->>Confirm: YES?
alt YES
Confirm->>Action: spawn async (send/take/add-solver)
Action->>Settings: read/update settings (if needed)
Action-->>KeyH: send OrderResult on completion
else NO
Confirm->>KeyH: revert to input mode
end
else Form input
KeyH->>KeyH: handle_char_input / handle_backspace
KeyH->>KeyH: update form fields
else Navigation
KeyH->>KeyH: handle_navigation (up/down/left/right/tab)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/ui/admin_key_confirm.rs:
- Around line 65-73: Update the misleading comment to match the actual
truncation logic: change the comment that says "show first 20 chars" to indicate
it shows the first 30 characters (or "first 30 chars + ...") for the display_key
calculation that uses key_string and the conditional truncation (custom_message,
key_string, display_key).
In @src/ui/key_handler/esc_handlers.rs:
- Around line 44-53: The Esc handlers for UiMode::NewMessageNotification and
UiMode::ViewingMessage currently set app.mode = UiMode::Normal directly; change
both to set app.mode = default_mode.clone() instead so dismissal uses the
role-specific default (e.g., UiMode::UserMode(UserMode::Normal) or
UiMode::AdminMode(AdminMode::Normal)) consistent with other Esc handlers; locate
the branches matching UiMode::NewMessageNotification(_, _, _) and
UiMode::ViewingMessage(_) and replace UiMode::Normal assignments with
default_mode.clone().
In @src/ui/key_handler/mod.rs:
- Around line 159-170: The current clipboard write uses
arboard::Clipboard::new() and a blocking
std::thread::sleep(Duration::from_millis(100)) to avoid "dropped too quickly";
remove this sleep and instead adopt arboard's recommended approach: either store
a long-lived Clipboard instance on your app state (create Clipboard once and
reuse it when handling the Copy action, replacing the local
arboard::Clipboard::new() and clipboard variable), or after set_text call on
platforms that support it call the SetExtLinux::wait() method to hold ownership
until the selection is served; update the copy logic that uses
clipboard.set_text(invoice.clone()) to use the shared Clipboard (or call the
wait extension) and remove the blocking sleep to avoid blocking the event loop.
In @src/ui/settings_tab.rs:
- Around line 90-106: The footer Paragraph is being rendered into the same area
as the settings list (both using chunks[3]), causing overlap; update the layout
to create a dedicated chunk for the footer (e.g., extend the layout split to
produce chunks[4] or rename the footer chunk variable) and render the footer
Paragraph there instead of chunks[3]; adjust the call that currently checks
user_role == UserRole::User and the f.render_widget(... Paragraph::new(...))
invocation to target the new footer chunk so the list widget and footer do not
overlap.
In @src/util/order_utils/helper.rs:
- Around line 68-99: The dispute_from_tags function can return a default Dispute
when required tags are absent; update it to validate that Dispute.id and
Dispute.status were populated before returning: after the loop check if
dispute.id is not the zero/default UUID (or track a boolean like saw_id) and
dispute.status is non-empty (or track saw_status), and if either is missing
return Err(anyhow::anyhow!("Missing dispute id") or similar for status) instead
of Ok(dispute); reference the dispute_from_tags function and the Dispute struct
fields id and status when adding these checks and error messages.
🧹 Nitpick comments (21)
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 usinglock().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_styleis also set on the list (line 81). Since the highlighting is already applied to individual items, thehighlight_styleon 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()returnsNone, 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_settingsparameter.The
_settingsparameter is not used inhandle_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.
.unwrap()onlock()will panic if a thread previously panicked while holding the lock. Consider using.lock().expect("mutex poisoned")for clearer panic messages, or handle thePoisonError.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_optionsvalues (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 onlock()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 reusingrender_order_id_headerfor 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 (missingbg(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 thehelpers::format_order_idfunction 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 - usesUiMode::Normalinstead of role-baseddefault_mode.When the user selects NO in the TakingOrder confirmation, the mode is set to
UiMode::Normalinstead of the role-baseddefault_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
PayInvoicearm 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'sstd::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 withenter_handlers.rs.The TakingOrder handling logic (validation, parsing, async task spawning) is nearly identical to
handle_enter_taking_orderinenter_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_orderandhandle_confirm_key.src/ui/key_handler/mod.rs (1)
185-191: Double handling of Backspace key.Both
handle_char_inputandhandle_backspaceare called when Backspace is pressed. Looking at the referenced code inform_input.rs,handle_char_inputonly matches onChar(' ')andChar(c), soBackspacefalls 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:KeyInputStatestructure mirrorsInvoiceInputState- consider unifying.
KeyInputStatehas nearly identical fields toInvoiceInputState(lines 412-417), differing only in thecopied_to_clipboardfield. 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, }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.tomlsrc/main.rssrc/settings.rssrc/ui/admin_key_confirm.rssrc/ui/disputes_tab.rssrc/ui/helpers.rssrc/ui/key_handler.rssrc/ui/key_handler/confirmation.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/esc_handlers.rssrc/ui/key_handler/form_input.rssrc/ui/key_handler/input_helpers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/navigation.rssrc/ui/key_handler/settings.rssrc/ui/key_handler/validation.rssrc/ui/key_input_popup.rssrc/ui/message_notification.rssrc/ui/mod.rssrc/ui/settings_tab.rssrc/ui/tab_content.rssrc/util/filters.rssrc/util/mod.rssrc/util/order_utils/execute_admin_add_solver.rssrc/util/order_utils/fetch_scheduler.rssrc/util/order_utils/helper.rssrc/util/order_utils/mod.rs
💤 Files with no reviewable changes (1)
- src/ui/key_handler.rs
🧰 Additional context used
🧬 Code graph analysis (16)
src/ui/key_handler/esc_handlers.rs (2)
src/ui/key_handler/confirmation.rs (2)
create_key_input_state(58-64)handle_confirmation_esc(50-55)src/ui/mod.rs (3)
first(83-85)first(155-157)first(221-226)
src/util/order_utils/fetch_scheduler.rs (3)
src/main.rs (2)
tokio(142-142)tokio(146-146)src/util/order_utils/helper.rs (2)
get_disputes(259-283)get_orders(227-255)src/ui/mod.rs (1)
new(483-497)
src/ui/key_input_popup.rs (3)
src/ui/helpers.rs (2)
create_centered_popup(9-18)render_help_text(21-36)src/settings.rs (1)
default(26-37)src/ui/mod.rs (1)
new(483-497)
src/util/mod.rs (3)
src/util/order_utils/helper.rs (3)
fetch_events_list(199-223)get_disputes(259-283)get_orders(227-255)src/util/order_utils/send_new_order.rs (1)
send_new_order(17-236)src/util/order_utils/take_order.rs (1)
take_order(36-195)
src/ui/settings_tab.rs (2)
src/settings.rs (1)
default(26-37)src/ui/mod.rs (1)
new(483-497)
src/ui/admin_key_confirm.rs (1)
src/ui/helpers.rs (1)
create_centered_popup(9-18)
src/ui/key_handler/settings.rs (1)
src/settings.rs (1)
save_settings(76-90)
src/util/order_utils/execute_admin_add_solver.rs (2)
src/util/dm_utils/mod.rs (1)
send_dm(25-69)src/main.rs (1)
settings(116-118)
src/ui/message_notification.rs (1)
src/ui/helpers.rs (3)
format_order_id(39-48)render_help_text(21-36)create_centered_popup(9-18)
src/ui/key_handler/navigation.rs (2)
src/main.rs (1)
take_state(73-73)src/settings.rs (1)
default(26-37)
src/ui/key_handler/form_input.rs (1)
src/main.rs (2)
validate_range_amount(67-92)take_state(73-73)
src/ui/disputes_tab.rs (1)
src/ui/mod.rs (2)
apply_status_color(508-532)new(483-497)
src/util/order_utils/mod.rs (4)
src/util/order_utils/execute_admin_add_solver.rs (1)
execute_admin_add_solver(12-53)src/util/order_utils/send_new_order.rs (1)
send_new_order(17-236)src/util/order_utils/fetch_scheduler.rs (1)
start_fetch_scheduler(32-79)src/util/order_utils/helper.rs (5)
fetch_events_list(199-223)get_disputes(259-283)get_orders(227-255)parse_disputes_events(102-135)parse_orders_events(138-196)
src/ui/key_handler/confirmation.rs (1)
src/ui/key_handler/settings.rs (3)
save_admin_key_to_settings(21-27)save_mostro_pubkey_to_settings(30-36)save_relay_to_settings(39-49)
src/ui/key_handler/mod.rs (8)
src/models.rs (1)
sqlx(240-240)src/ui/key_handler/confirmation.rs (2)
handle_cancel_key(220-248)handle_confirm_key(67-217)src/ui/key_handler/enter_handlers.rs (1)
handle_enter_key(23-205)src/ui/key_handler/esc_handlers.rs (1)
handle_esc_key(5-89)src/ui/key_handler/form_input.rs (2)
handle_backspace(61-87)handle_char_input(5-58)src/ui/key_handler/input_helpers.rs (2)
handle_invoice_input(57-62)handle_key_input(66-68)src/ui/key_handler/navigation.rs (1)
handle_navigation(9-22)src/ui/key_handler/settings.rs (1)
handle_mode_switch(52-68)
src/main.rs (3)
src/util/dm_utils/notifications_ch_mng.rs (1)
handle_message_notification(32-56)src/util/dm_utils/order_ch_mng.rs (1)
handle_order_result(6-82)src/ui/mod.rs (2)
new(483-497)ui_draw(542-738)
🔇 Additional comments (33)
src/util/order_utils/helper.rs (2)
215-220: LGTM!The Disputes handling follows the same pattern as Orders, maintaining consistency in the codebase.
257-283: LGTM!The implementation is consistent with
get_ordersand correctly extracts Dispute items from the Event enum.src/ui/helpers.rs (3)
21-36: LGTM!Clean implementation for consistent help text rendering across the UI.
39-48: LGTM!The use of
chars().take(8).collect()is safer than string slicing for displaying the UUID prefix.
9-18: Code looks good. The function correctly usesFlex::Center(available in ratatui 0.30.0) to center the popup and properly clamps dimensions usingmin().src/util/filters.rs (1)
35-35: LGTM!The Disputes filter follows the same pattern as Orders, maintaining consistency.
src/settings.rs (1)
8-8: LGTM!Adding
Cloneto Settings is appropriate for the settings persistence and UI update flows described in the PR. All fields are Clone-compatible.Cargo.toml (1)
12-14: Ratatui 0.30.0 compatibility verified; mostro-core 0.6.57 cannot be verified due to lack of public release notes.The ratatui 0.30.0 breaking changes (Backend trait changes, TestBackend refactoring, Bar widget API updates) do not affect this codebase. The code uses only
CrosstermBackend(concrete type, not generic), does not use TestBackend, and does not use the Bar widget. All UI components (disputes_tab, key_input_popup, admin_key_confirm) rely on standard ratatui widgets (Block, Borders, Paragraph, List, Table, etc.) that remain stable. No public changelog is available for mostro-core 0.6.57, but the codebase's simple type usage (Dispute, DisputeStatus, Action) suggests compatibility is likely. Consider running integration tests to confirm.src/util/mod.rs (1)
14-14: LGTM!The addition of
get_disputesto the re-exports is consistent with the existing pattern and properly exposes the new dispute-fetching functionality.src/ui/key_handler/validation.rs (1)
5-13: LGTM!The validation function correctly validates npub format using
PublicKey::from_bech32. The error messages are clear and user-friendly.src/ui/key_handler/input_helpers.rs (1)
27-53: LGTM!The generic text input handler cleanly abstracts the paste-aware input handling logic. The just_pasted flag handling correctly prevents spurious Enter keypresses immediately after paste operations.
src/ui/disputes_tab.rs (1)
42-67: LGTM!The dispute row rendering logic is clean. The status styling via
apply_status_colorand date formatting with fallback handling are well implemented.src/util/order_utils/execute_admin_add_solver.rs (1)
29-37: Verify the message construction method for AdminAddSolver against mostro-core specification.The code uses
Message::new_disputewithAction::AdminAddSolverandPayload::TextMessage(solver_pubkey.to_string()). Confirm this is the correct Message builder and payload type for AdminAddSolver actions in mostro-core v0.6.57, as thenew_disputemethod name seems incongruent with theAdminAddSolveraction semantics.src/util/order_utils/mod.rs (1)
1-19: LGTM!The module organization is clean and follows the existing patterns. The new modules (
execute_admin_add_solver,fetch_scheduler) are properly declared and their public APIs are correctly re-exported alongside the expanded helper functions.src/ui/admin_key_confirm.rs (1)
9-16: LGTM!Clean wrapper pattern delegating to the more flexible
render_admin_key_confirm_with_messagefunction.src/ui/key_input_popup.rs (1)
8-126: LGTM!The key input popup implementation is well-structured with clear visual feedback for focus state, appropriate sensitive data warnings, and helpful user guidance.
src/util/order_utils/fetch_scheduler.rs (1)
9-14: LGTM!The
FetchSchedulerResultstruct provides a clean API for sharing fetched data with the UI layer.src/ui/key_handler/navigation.rs (2)
248-263: LGTM!The tab switch handler correctly clears pending notifications and marks messages as read when entering the Messages tab, with proper guard to avoid redundant operations.
265-292: LGTM!The Tab/BackTab handling correctly cycles through form fields with modular arithmetic and properly skips the range field when not in range mode.
src/ui/key_handler/form_input.rs (2)
1-87: Overall structure is clean.The form input handling logic is well-organized with clear separation between character input and backspace handling. The callback pattern for validation is appropriate.
31-41: Theunreachable!()is justified given the guard check and navigation bounds.The code is sound: the guard on line 28 (
if form.focused == 0) prevents value 0 from reaching the match statement, and navigation.rs uses modulo 9 (form.focused = (form.focused + 1) % 9) to ensurefocusedstays within 0-8. The match arms cover all values (1-8) that can reach the unreachable case, making the panic branch unreachable under normal operation.Likely an incorrect or invalid review comment.
src/ui/message_notification.rs (2)
1-6: LGTM - Clean imports and module structure.The imports are well-organized, using the super module pattern for local dependencies and explicit imports from ratatui.
350-391: Well-structured public API with clean dispatch logic.The
render_message_notificationfunction provides a clean entry point that properly delegates to specialized rendering functions based on the action type. The popup sizing adapts appropriately for different notification types.src/ui/tab_content.rs (1)
124-125: LGTM - Helper function integration.The refactoring to use
helpers::create_centered_popupis correctly applied, improving code consistency with other popup rendering.src/ui/key_handler/enter_handlers.rs (1)
22-205: Well-structured Enter key dispatcher with proper async handling.The
handle_enter_keyfunction usesstd::mem::replacecorrectly to take ownership of the mode while avoiding borrowing issues. The async task spawning for order operations is properly implemented with cloned resources and channel-based result reporting.src/main.rs (2)
130-132: Clean integration of the background fetch scheduler.The destructuring pattern for
FetchSchedulerResultis clean and the removal of the manual order-fetching loop in favor of the scheduler improves code organization.
271-277: Good addition of dispute index bounds checking.The bounds checking for
selected_dispute_idxmirrors the existing pattern forselected_order_idx, maintaining consistency when the disputes list shrinks.src/ui/key_handler/confirmation.rs (2)
220-248: Correct cancellation logic with appropriate state transitions.The
handle_cancel_keyfunction properly handles cancellation for different confirmation modes, returning users to the input state while preserving their entered data.
12-64: Well-designed generic confirmation helpers.The
handle_input_to_confirmation,handle_confirmation_enter, andhandle_confirmation_escfunctions provide a clean, reusable pattern for confirmation flows using closures for customization. Thecreate_key_input_statefactory function ensures consistent initialization.src/ui/key_handler/mod.rs (1)
27-40: Function signature with many parameters is acceptable given the complexity.The
#[allow(clippy::too_many_arguments)]annotation is appropriate here. The function needs access to multiple pieces of state to properly dispatch key events. Consider if a context struct might improve ergonomics in the future, but this is fine for now.src/ui/mod.rs (3)
638-717: Well-organized popup overlay rendering.The popup overlay rendering is cleanly organized with appropriate calls to specialized rendering functions. Using separate
if letblocks ensures only the active popup is rendered, and the ordering is correct (confirmations render after input popups).
725-731: Refactored message notification rendering using dedicated module.The message notification rendering now correctly delegates to
message_notification::render_message_notification, improving code organization by moving the complex rendering logic to a dedicated module.
482-504: Clean AppState initialization with new fields.The
AppState::newandswitch_rolemethods properly initialize and reset the newselected_dispute_idxandselected_settings_optionfields.
| UiMode::NewMessageNotification(_, _, _) => { | ||
| // Dismiss notification | ||
| app.mode = UiMode::Normal; | ||
| true | ||
| } | ||
| UiMode::ViewingMessage(_) => { | ||
| // Dismiss message view popup | ||
| app.mode = UiMode::Normal; | ||
| true | ||
| } |
There was a problem hiding this comment.
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 (which is UiMode::UserMode(UserMode::Normal) or UiMode::AdminMode(AdminMode::Normal)). This could lead to inconsistent UI state after dismissing notifications vs. escaping from other modes.
Consider using default_mode.clone() here for consistency:
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
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| UiMode::NewMessageNotification(_, _, _) => { | |
| // Dismiss notification | |
| app.mode = UiMode::Normal; | |
| true | |
| } | |
| UiMode::ViewingMessage(_) => { | |
| // Dismiss message view popup | |
| app.mode = UiMode::Normal; | |
| true | |
| } | |
| UiMode::NewMessageNotification(_, _, _) => { | |
| // Dismiss notification | |
| app.mode = default_mode.clone(); | |
| true | |
| } | |
| UiMode::ViewingMessage(_) => { | |
| // Dismiss message view popup | |
| app.mode = default_mode.clone(); | |
| true | |
| } |
🤖 Prompt for AI Agents
In @src/ui/key_handler/esc_handlers.rs around lines 44 - 53, The Esc handlers
for UiMode::NewMessageNotification and UiMode::ViewingMessage currently set
app.mode = UiMode::Normal directly; change both to set app.mode =
default_mode.clone() instead so dismissal uses the role-specific default (e.g.,
UiMode::UserMode(UserMode::Normal) or UiMode::AdminMode(AdminMode::Normal))
consistent with other Esc handlers; locate the branches matching
UiMode::NewMessageNotification(_, _, _) and UiMode::ViewingMessage(_) and
replace UiMode::Normal assignments with default_mode.clone().
| /// Parse dispute from nostr tags | ||
| pub(super) fn dispute_from_tags(tags: Tags) -> Result<Dispute> { | ||
| let mut dispute = Dispute::default(); | ||
| 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; | ||
| } | ||
| "s" => { | ||
| let status = DisputeStatus::from_str(value) | ||
| .map_err(|_| anyhow::anyhow!("Invalid dispute status"))?; | ||
| dispute.status = status.to_string(); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| Ok(dispute) | ||
| } |
There was a problem hiding this comment.
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. Unlike parsing errors which return Err, missing tags silently produce a default Dispute that may not represent valid data.
Consider validating that required fields (id and status) were actually populated before returning Ok.
🛡️ Proposed fix to validate required fields
pub(super) 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)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Parse dispute from nostr tags | |
| pub(super) fn dispute_from_tags(tags: Tags) -> Result<Dispute> { | |
| let mut dispute = Dispute::default(); | |
| 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; | |
| } | |
| "s" => { | |
| let status = DisputeStatus::from_str(value) | |
| .map_err(|_| anyhow::anyhow!("Invalid dispute status"))?; | |
| dispute.status = status.to_string(); | |
| } | |
| _ => {} | |
| } | |
| } | |
| Ok(dispute) | |
| } | |
| /// Parse dispute from nostr tags | |
| pub(super) 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) | |
| } |
🤖 Prompt for AI Agents
In @src/util/order_utils/helper.rs around lines 68 - 99, The dispute_from_tags
function can return a default Dispute when required tags are absent; update it
to validate that Dispute.id and Dispute.status were populated before returning:
after the loop check if dispute.id is not the zero/default UUID (or track a
boolean like saw_id) and dispute.status is non-empty (or track saw_status), and
if either is missing return Err(anyhow::anyhow!("Missing dispute id") or similar
for status) instead of Ok(dispute); reference the dispute_from_tags function and
the Dispute struct fields id and status when adding these checks and error
messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/ui/settings_tab.rs (1)
64-89: Consider consolidating the highlighting approach.The code manually applies styling to each
ListItembased onselected_option(lines 68-75), then also setshighlight_styleandhighlight_symbolon theListwidget (lines 82-83). This is redundant—typically you either style items manually or rely on theList'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();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ui/settings_tab.rs
🔇 Additional comments (2)
src/ui/settings_tab.rs (2)
22-32: Footer overlap issue resolved.The layout now correctly allocates separate chunks for the list (chunks[3]) and footer (chunks[4]), addressing the previous review comment about overlapping rendering areas.
Also applies to: 116-119
8-13: Theselected_settings_optionstate is properly managed by the caller. When the user role changes,AppState::switch_role()explicitly resetsselected_settings_optionto0(line 503 inui/mod.rs). Since both User mode (2 options) and Admin mode (4 options) have index0as valid, no out-of-bounds condition can occur. No action needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/helper_tests.rs (1)
5-46: Consider using an exhaustive match or macro to ensure all variants are tested.The manual vector of
CantDoReasonvariants is brittle—if new variants are added to the enum inmostro-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'sCantDoReasonderivesstrum::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
strumis 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_dbmay diverge from the production schema over time, causing tests to pass while production code fails. Consider one of the following approaches to maintain consistency:
- Shared schema definition: Extract the schema to a common location used by both tests and migrations.
- Run production migrations: Apply the same migrations used in production to the test database.
- 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 aHashMap<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.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/ADMIN_DISPUTES.mdsrc/lib.rssrc/ui/key_handler/mod.rssrc/util/order_utils/helper.rssrc/util/order_utils/mod.rstests/common/mod.rstests/db_tests.rstests/helper_tests.rstests/parsing_tests.rstests/validation_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/util/order_utils/mod.rs
- src/ui/key_handler/mod.rs
🧰 Additional context used
🧬 Code graph analysis (7)
tests/helper_tests.rs (1)
src/util/types.rs (1)
get_cant_do_description(29-77)
tests/validation_tests.rs (1)
src/ui/key_handler/validation.rs (1)
validate_npub(5-13)
tests/parsing_tests.rs (1)
src/util/order_utils/helper.rs (1)
order_from_tags(14-66)
src/util/order_utils/helper.rs (1)
src/util/filters.rs (1)
create_filter(28-38)
src/lib.rs (1)
src/main.rs (1)
settings(116-118)
tests/common/mod.rs (1)
src/models.rs (1)
sqlx(240-240)
tests/db_tests.rs (2)
tests/common/mod.rs (2)
create_test_db(6-43)test_mnemonic(46-49)src/models.rs (4)
get(58-65)update_last_trade_index(67-75)get_identity_keys(77-84)get_by_id(239-255)
🪛 LanguageTool
docs/ADMIN_DISPUTES.md
[style] ~692-~692: Consider replacing this word to strengthen your wording.
Context: ...vation is a critical security component and is thoroughly tested: - Same mnemonic ...
(AND_THAT)
🔇 Additional comments (8)
tests/parsing_tests.rs (1)
1-41: LGTM! Well-structured parsing tests.The tests comprehensively validate
order_from_tagsfor both basic orders and range-based orders. The assertions cover all relevant fields, and the test data is appropriate.tests/db_tests.rs (2)
9-87: User model tests look comprehensive.The test coverage for
Useris thorough, validating creation, retrieval, updates, identity key derivation, trade key derivation consistency, and persistence. The assertions appropriately verify expected behavior.
89-180: Order model tests provide good coverage.The tests validate order creation, retrieval, error handling for missing records, and updates. The test data is realistic and the assertions are appropriate.
src/lib.rs (1)
11-12: The code is correct as written.OnceLock::new()properly initializes the lock itself, and the settings are safely initialized viaget_or_init()insrc/settings.rs:42rather than requiring manualSETTINGS.set()calls. Usingget_or_init()with a closure is the idiomatic and safer pattern for lazy initialization with OnceLock.src/util/order_utils/helper.rs (3)
14-14: LGTM: Visibility change exposes order parsing helper.Making
order_from_tagspublic is consistent with the newdispute_from_tagsfunction and allows external modules to reuse the order parsing logic.
215-220: LGTM: Consistent implementation for dispute fetching.The
ListKind::Disputesbranch follows the same pattern as theOrdersbranch, ensuring consistent behavior across different event types.
257-283: LGTM: Clean API for dispute retrieval.The
get_disputesfunction provides a straightforward wrapper that mirrors theget_orderspattern, maintaining consistency across the codebase.docs/ADMIN_DISPUTES.md (1)
1-713: LGTM: Comprehensive and accurate documentation.The documentation updates accurately reflect the implementation changes:
- ✅ Settings tab integration for Add Solver is properly documented
- ✅ Code references and snippets are accurate
- ✅ Sequence diagrams clearly illustrate the flows
- ✅ Security considerations are well-covered
- ✅ Implementation status is clearly marked with checkmarks
- ✅ Test coverage section provides clear guidance
The documentation will serve as an excellent guide for users and future developers working with the admin dispute resolution features.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
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);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ui/admin_key_confirm.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/ui/admin_key_confirm.rs (1)
src/ui/helpers.rs (1)
create_centered_popup(9-18)
🔇 Additional comments (1)
src/ui/admin_key_confirm.rs (1)
8-16: LGTM! Clean API design.The wrapper function provides a convenient interface for the common case while delegating to the more flexible implementation.
feat: improve clipboard handling with arboard wait() method Replace arbitrary 100ms sleep with arboard SetExtLinux::wait() method for reliable clipboard operations on Linux. The wait() method ensures the clipboard instance remains alive until actually overwritten. - Use builder pattern with wait() on Linux, fallback to set_text() elsewhere - Remove unused settings parameter from key handler functions - Update documentation to reflect improved implementation - Fix documentation to match code changes (error messages, function signatures) Files changed: - src/ui/key_handler/mod.rs: Clipboard implementation with wait() - docs/TUI_INTERFACE.md: Updated clipboard docs and function signature example - docs/ADMIN_DISPUTES.md: Updated error message examples to match code - src/ui/key_handler/*.rs: Remove unused settings parameters - src/main.rs: Code cleanup
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/ui/key_handler/confirmation.rs:
- Around line 67-216: Add a match arm for AdminMode::ConfirmAddSolver in
handle_confirm_key: when encountering
UiMode::AdminMode(AdminMode::ConfirmAddSolver(key_string, _)) call
handle_confirmation_enter(true, &key_string, default_mode,
save_solver_to_settings, |input|
UiMode::AdminMode(AdminMode::AddSolver(create_key_input_state(input)))) (use the
same default_mode computation as the other Admin cases), assign the result to
app.mode and return true so 'y' behaves like Enter for the Add Solver flow.
In @src/ui/key_handler/enter_handlers.rs:
- Line 490: The Action::PayInvoice branch in enter_handlers.rs is empty; either
implement the intended behavior (e.g., invoke the clipboard helper to copy the
invoice payload, open/display payment instructions, or send a PayInvoice
command) inside the handler where other actions are handled, or explicitly mark
it as no-op with a comment if no Enter action is required; alternatively remove
PayInvoice from the actionable-message check near the earlier filter (where
AddInvoice and PayInvoice are considered) so it never reaches this handler.
Locate the match arm for Action::PayInvoice and add the concrete call(s) to the
existing helpers used by similar cases (see how AddInvoice is handled) or add a
clear comment explaining intentional emptiness and update the filter logic
accordingly.
- Around line 197-201: The AddRelay flow currently jumps straight to
confirmation in the UiMode::AddRelay match arm; change it to validate the relay
string before creating UiMode::ConfirmRelay by reusing the same pattern as the
npub/admin/solver handlers: call a validation routine (create or use
validate_relay_url in the validation module) on key_state.key_input and only
invoke handle_input_to_confirmation(...) -> UiMode::ConfirmRelay(input, true)
when validation passes; otherwise keep the mode and surface the validation error
(e.g., set the same error handling path used by validate_npub flows) so invalid
relay URLs are rejected before saving.
🧹 Nitpick comments (4)
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
WaitingAddInvoicemode but then callsexecute_send_msg(notexecute_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.,
WaitingForMostroor a newWaitingSendMessagemode) 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_roleis repeated throughout this file (lines 31-34, 388-392, 402-405) and inconfirmation.rs(lines 75-78, 170-173, 184-187, 198-201, 220-223).♻️ Suggested approach
Add a method to
AppStateor 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.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/ADMIN_DISPUTES.mddocs/TUI_INTERFACE.mdsrc/main.rssrc/ui/key_handler/confirmation.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/settings.rssrc/ui/key_handler/validation.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/key_handler/settings.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/ui/key_handler/enter_handlers.rs (7)
src/util/order_utils/execute_add_invoice.rs (1)
execute_add_invoice(25-110)src/util/order_utils/execute_send_msg.rs (1)
execute_send_msg(44-141)src/main.rs (3)
tokio(142-142)tokio(146-146)settings(116-118)src/ui/key_handler/confirmation.rs (3)
create_key_input_state(58-64)handle_confirmation_enter(28-47)handle_input_to_confirmation(12-25)src/ui/key_handler/settings.rs (3)
save_admin_key_to_settings(21-27)save_mostro_pubkey_to_settings(30-36)save_relay_to_settings(39-49)src/ui/key_handler/validation.rs (1)
validate_npub(5-13)src/util/order_utils/take_order.rs (1)
take_order(36-195)
src/ui/key_handler/mod.rs (7)
src/ui/key_handler/confirmation.rs (2)
handle_cancel_key(219-247)handle_confirm_key(67-216)src/ui/key_handler/enter_handlers.rs (1)
handle_enter_key(23-212)src/ui/key_handler/esc_handlers.rs (1)
handle_esc_key(5-89)src/ui/key_handler/form_input.rs (2)
handle_backspace(61-87)handle_char_input(5-58)src/ui/key_handler/input_helpers.rs (2)
handle_invoice_input(57-62)handle_key_input(66-68)src/ui/key_handler/navigation.rs (2)
handle_navigation(9-22)handle_tab_navigation(266-292)src/ui/key_handler/validation.rs (1)
validate_npub(5-13)
src/main.rs (3)
src/util/dm_utils/notifications_ch_mng.rs (1)
handle_message_notification(32-56)src/util/order_utils/fetch_scheduler.rs (1)
start_fetch_scheduler(32-79)src/ui/mod.rs (1)
new(483-497)
🪛 LanguageTool
docs/ADMIN_DISPUTES.md
[style] ~692-~692: Consider replacing this word to strengthen your wording.
Context: ...vation is a critical security component and is thoroughly tested: - Same mnemonic ...
(AND_THAT)
🔇 Additional comments (16)
docs/ADMIN_DISPUTES.md (3)
27-101: Documentation accurately reflects the Settings tab implementation.The reorganization of the Add Solver functionality from a dedicated Admin tab to the Settings tab is clearly documented with comprehensive feature lists and source references.
321-435: LGTM! Comprehensive documentation of the Add Solver workflow.The sequence diagram and implementation details clearly document the complete flow from Settings tab navigation through NPUB validation, confirmation, and network communication. The code snippets and key points provide excellent reference material.
555-708: Excellent implementation status tracking and testing documentation.The completed features section provides clear traceability with source file references, and the testing documentation offers practical guidance for running and expanding the test suite.
docs/TUI_INTERFACE.md (1)
132-132: Documentation accurately reflects the clipboard handling improvement.The update correctly documents the Linux-specific
SetExtLinux::wait()method for reliable clipboard operations, replacing the previous arbitrary delay approach.src/ui/key_handler/validation.rs (1)
5-13: LGTM! Clean and straightforward NPUB validation.The function correctly trims input, validates emptiness, and uses the standard nostr-sdk validation. The generic error message is appropriate for UI-facing validation.
src/ui/key_handler/mod.rs (3)
1-26: Excellent modular organization of key handling logic.The separation into focused submodules (confirmation, enter_handlers, esc_handlers, form_input, input_helpers, navigation, settings, validation) improves maintainability and provides a clean public API through selective re-exports.
28-84: Clean early input handling with proper focus management.The function correctly prioritizes focused input fields (invoice and key inputs) before general key handling, and properly manages the clipboard copy indicator state.
86-192: Comprehensive key event handling with proper control flow.The match structure covers all key inputs with appropriate delegation to specialized handlers. The clipboard handling correctly uses platform-specific methods (SetExtLinux::wait() on Linux), and the mode switch is properly scoped to the Settings tab.
src/main.rs (6)
9-10: Import changes correctly reflect the fetch scheduler refactoring.The removal of
Mutexandinterval_at/Instantimports aligns with moving the fetch scheduling logic tostart_fetch_scheduler, and the addition ofFetchSchedulerResultandAdminModesupports the new functionality.Also applies to: 16-16, 34-34, 39-39
130-132: LGTM! Clean refactoring to background fetch scheduler.Moving the fetch logic to
start_fetch_schedulerimproves separation of concerns and makes the main loop more focused on UI event handling.
208-222: Paste handling correctly implemented for admin key inputs.The implementation follows the same safe pattern as invoice paste handling: filtering control characters and setting the
just_pastedflag to prevent immediate Enter key processing.
237-247: Disputes parameter correctly passed to key handler.The addition of the
&disputesparameter enables the key handler to navigate and manage disputes in the UI.
270-277: Proper bounds checking for dispute selection index.This validation mirrors the orders index validation and correctly prevents out-of-bounds access when the disputes list changes.
284-284: Disputes data correctly passed to UI drawing function.This enables the UI to render disputes-related tabs and content as part of the admin workflow.
src/ui/key_handler/enter_handlers.rs (1)
330-332: Documented limitation: Invoice input for buy orders.The TODO indicates that invoice input is needed for buy orders. This same limitation is noted in
confirmation.rsline 138.Is invoice input for buy orders planned for a future PR, or should it be addressed in this phase? If it's acceptable to defer, consider creating an issue to track this work to ensure it's not forgotten.
src/ui/key_handler/confirmation.rs (1)
11-64: LGTM! Clean helper functions.The confirmation flow helpers are well-designed with good use of generics and callbacks. They provide a consistent pattern for:
- Transitioning from input to confirmation (
handle_input_to_confirmation)- Handling YES/NO selections (
handle_confirmation_enter)- Escaping back to input (
handle_confirmation_esc)- Creating input states (
create_key_input_state)
| pub fn handle_confirm_key( | ||
| app: &mut AppState, | ||
| pool: &SqlitePool, | ||
| client: &Client, | ||
| mostro_pubkey: nostr_sdk::PublicKey, | ||
| order_result_tx: &UnboundedSender<crate::ui::OrderResult>, | ||
| ) -> bool { | ||
| // Returns true if should continue (skip further processing) | ||
| let default_mode = match app.user_role { | ||
| UserRole::User => UiMode::UserMode(UserMode::Normal), | ||
| UserRole::Admin => UiMode::AdminMode(AdminMode::Normal), | ||
| }; | ||
| match std::mem::replace(&mut app.mode, default_mode.clone()) { | ||
| UiMode::UserMode(UserMode::ConfirmingOrder(form)) => { | ||
| // User confirmed, send the order | ||
| let form_clone = form.clone(); | ||
| app.mode = UiMode::UserMode(UserMode::WaitingForMostro(form_clone.clone())); | ||
|
|
||
| // Spawn async task to send order | ||
| let pool_clone = pool.clone(); | ||
| let client_clone = client.clone(); | ||
| let result_tx = order_result_tx.clone(); | ||
|
|
||
| tokio::spawn(async move { | ||
| match crate::util::send_new_order( | ||
| &pool_clone, | ||
| &client_clone, | ||
| SETTINGS.get().unwrap(), | ||
| mostro_pubkey, | ||
| &form_clone, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(result) => { | ||
| let _ = result_tx.send(result); | ||
| } | ||
| Err(e) => { | ||
| log::error!("Failed to send order: {}", e); | ||
| let _ = result_tx.send(crate::ui::OrderResult::Error(e.to_string())); | ||
| } | ||
| } | ||
| }); | ||
| true | ||
| } | ||
| UiMode::UserMode(UserMode::TakingOrder(take_state)) => { | ||
| // User confirmed taking the order (same as Enter key) | ||
| // Check validation first | ||
| if take_state.is_range_order { | ||
| if take_state.amount_input.is_empty() { | ||
| // Can't proceed without amount | ||
| app.mode = UiMode::UserMode(UserMode::TakingOrder(take_state)); | ||
| return true; | ||
| } | ||
| if take_state.validation_error.is_some() { | ||
| // Can't proceed with invalid amount | ||
| app.mode = UiMode::UserMode(UserMode::TakingOrder(take_state)); | ||
| return true; | ||
| } | ||
| } | ||
| // Proceed with taking the order | ||
| let take_state_clone = take_state.clone(); | ||
| app.mode = UiMode::UserMode(UserMode::WaitingTakeOrder(take_state_clone.clone())); | ||
|
|
||
| // Parse amount if it's a range order | ||
| let amount = if take_state_clone.is_range_order { | ||
| take_state_clone.amount_input.trim().parse::<i64>().ok() | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| // For buy orders (taking sell), we'd need invoice, but for now we'll pass None | ||
| // TODO: Add invoice input for buy orders | ||
| let invoice = None; | ||
|
|
||
| // Spawn async task to take order | ||
| let pool_clone = pool.clone(); | ||
| let client_clone = client.clone(); | ||
| let result_tx = order_result_tx.clone(); | ||
|
|
||
| tokio::spawn(async move { | ||
| match crate::util::take_order( | ||
| &pool_clone, | ||
| &client_clone, | ||
| SETTINGS.get().unwrap(), | ||
| mostro_pubkey, | ||
| &take_state_clone.order, | ||
| amount, | ||
| invoice, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(result) => { | ||
| let _ = result_tx.send(result); | ||
| } | ||
| Err(e) => { | ||
| log::error!("Failed to take order: {}", e); | ||
| let _ = result_tx.send(crate::ui::OrderResult::Error(e.to_string())); | ||
| } | ||
| } | ||
| }); | ||
| true | ||
| } | ||
| UiMode::ConfirmMostroPubkey(key_string, _) => { | ||
| let default_mode = match app.user_role { | ||
| UserRole::User => UiMode::UserMode(UserMode::Normal), | ||
| UserRole::Admin => UiMode::AdminMode(AdminMode::Normal), | ||
| }; | ||
| app.mode = handle_confirmation_enter( | ||
| true, // 'y' key means YES | ||
| &key_string, | ||
| default_mode, | ||
| save_mostro_pubkey_to_settings, | ||
| |input| UiMode::AddMostroPubkey(create_key_input_state(input)), | ||
| ); | ||
| true | ||
| } | ||
| UiMode::ConfirmRelay(relay_string, _) => { | ||
| let default_mode = match app.user_role { | ||
| UserRole::User => UiMode::UserMode(UserMode::Normal), | ||
| UserRole::Admin => UiMode::AdminMode(AdminMode::Normal), | ||
| }; | ||
| app.mode = handle_confirmation_enter( | ||
| true, // 'y' key means YES | ||
| &relay_string, | ||
| default_mode, | ||
| save_relay_to_settings, | ||
| |input| UiMode::AddRelay(create_key_input_state(input)), | ||
| ); | ||
| true | ||
| } | ||
| UiMode::AdminMode(AdminMode::ConfirmAdminKey(key_string, _)) => { | ||
| let default_mode = match app.user_role { | ||
| UserRole::User => UiMode::UserMode(UserMode::Normal), | ||
| UserRole::Admin => UiMode::AdminMode(AdminMode::Normal), | ||
| }; | ||
| app.mode = handle_confirmation_enter( | ||
| true, // 'y' key means YES | ||
| &key_string, | ||
| default_mode, | ||
| save_admin_key_to_settings, | ||
| |input| UiMode::AdminMode(AdminMode::SetupAdminKey(create_key_input_state(input))), | ||
| ); | ||
| true | ||
| } | ||
| mode => { | ||
| app.mode = mode; | ||
| false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing AdminMode::ConfirmAddSolver handling for 'y' key.
The handle_confirm_key function handles 'y' key confirmations for ConfirmMostroPubkey, ConfirmRelay, and ConfirmAdminKey, but not for AdminMode::ConfirmAddSolver.
However, enter_handlers.rs (lines 114-149) handles ConfirmAddSolver for the Enter key. This creates an inconsistency where users can confirm with Enter but not with 'y' for the Add Solver flow, unlike other confirmation dialogs.
🔧 Suggested fix
Add handling for ConfirmAddSolver in the match statement:
UiMode::AdminMode(AdminMode::ConfirmAdminKey(key_string, _)) => {
let default_mode = match app.user_role {
UserRole::User => UiMode::UserMode(UserMode::Normal),
UserRole::Admin => UiMode::AdminMode(AdminMode::Normal),
};
app.mode = handle_confirmation_enter(
true, // 'y' key means YES
&key_string,
default_mode,
save_admin_key_to_settings,
|input| UiMode::AdminMode(AdminMode::SetupAdminKey(create_key_input_state(input))),
);
true
}
+ UiMode::AdminMode(AdminMode::ConfirmAddSolver(solver_pubkey, _)) => {
+ // YES selected - send AddSolver message (same logic as enter_handlers.rs lines 115-143)
+ 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;
+
+ 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_clone,
+ )
+ .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
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/ui/key_handler/confirmation.rs around lines 67 - 216, Add a match arm
for AdminMode::ConfirmAddSolver in handle_confirm_key: when encountering
UiMode::AdminMode(AdminMode::ConfirmAddSolver(key_string, _)) call
handle_confirmation_enter(true, &key_string, default_mode,
save_solver_to_settings, |input|
UiMode::AdminMode(AdminMode::AddSolver(create_key_input_state(input)))) (use the
same default_mode computation as the other Admin cases), assign the result to
app.mode and return true so 'y' behaves like Enter for the Add Solver flow.
| UiMode::AddRelay(key_state) => { | ||
| app.mode = handle_input_to_confirmation(&key_state.key_input, default_mode, |input| { | ||
| UiMode::ConfirmRelay(input, true) | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/ui/key_handler/enter_handlers.rs | head -220 | tail -121Repository: MostroP2P/mostrix
Length of output: 6155
🏁 Script executed:
rg "validate" src/ui/key_handler/ -lRepository: MostroP2P/mostrix
Length of output: 189
🏁 Script executed:
rg "relay.*url|relay.*ws|validate_relay" --type rustRepository: MostroP2P/mostrix
Length of output: 118
🏁 Script executed:
rg "ConfirmRelay" src/ -B 2 -A 5 --type rustRepository: MostroP2P/mostrix
Length of output: 6604
🏁 Script executed:
cat -n src/ui/key_handler/validation.rsRepository: MostroP2P/mostrix
Length of output: 574
🏁 Script executed:
rg "save_relay_to_settings" src/ -B 2 -A 15 --type rustRepository: MostroP2P/mostrix
Length of output: 6469
Add relay URL validation before confirmation.
Unlike the NPUB validation for Mostro pubkey (lines 173-186), Admin key (lines 150-163), and Add Solver (lines 99-113), the AddRelay flow at lines 197-201 proceeds directly to confirmation without validating the relay URL format. This allows invalid relay URLs to be saved to settings without format verification.
Consider adding a validation step matching the pattern used for npub inputs:
✨ Suggested validation approach
UiMode::AddRelay(key_state) => {
- app.mode = handle_input_to_confirmation(&key_state.key_input, default_mode, |input| {
- UiMode::ConfirmRelay(input, true)
- });
+ if key_state.key_input.trim().starts_with("ws://") || key_state.key_input.trim().starts_with("wss://") {
+ app.mode = handle_input_to_confirmation(&key_state.key_input, default_mode, |input| {
+ UiMode::ConfirmRelay(input, true)
+ });
+ } else {
+ app.mode = UiMode::OrderResult(crate::ui::OrderResult::Error(
+ "Invalid relay URL format. Must start with ws:// or wss://".to_string()
+ ));
+ }
}Alternatively, create a validate_relay_url function in the validation module for more comprehensive URL parsing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| UiMode::AddRelay(key_state) => { | |
| app.mode = handle_input_to_confirmation(&key_state.key_input, default_mode, |input| { | |
| UiMode::ConfirmRelay(input, true) | |
| }); | |
| } | |
| UiMode::AddRelay(key_state) => { | |
| if key_state.key_input.trim().starts_with("ws://") || key_state.key_input.trim().starts_with("wss://") { | |
| app.mode = handle_input_to_confirmation(&key_state.key_input, default_mode, |input| { | |
| UiMode::ConfirmRelay(input, true) | |
| }); | |
| } else { | |
| app.mode = UiMode::OrderResult(crate::ui::OrderResult::Error( | |
| "Invalid relay URL format. Must start with ws:// or wss://".to_string() | |
| )); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/ui/key_handler/enter_handlers.rs around lines 197 - 201, The AddRelay
flow currently jumps straight to confirmation in the UiMode::AddRelay match arm;
change it to validate the relay string before creating UiMode::ConfirmRelay by
reusing the same pattern as the npub/admin/solver handlers: call a validation
routine (create or use validate_relay_url in the validation module) on
key_state.key_input and only invoke handle_input_to_confirmation(...) ->
UiMode::ConfirmRelay(input, true) when validation passes; otherwise keep the
mode and surface the validation error (e.g., set the same error handling path
used by validate_npub flows) so invalid relay URLs are rejected before saving.
| } | ||
| } | ||
| } | ||
| Action::PayInvoice => {} |
There was a problem hiding this comment.
PayInvoice action has no implementation.
The PayInvoice action branch is empty. Earlier in the file (line 234), both AddInvoice and PayInvoice are checked as actionable message types, but only AddInvoice has an implementation here.
If PayInvoice requires special handling (e.g., copying invoice to clipboard, displaying payment instructions), please implement it. If it's intentionally display-only and no action is needed on Enter, consider adding a comment explaining this, or filter it out earlier (line 234) to avoid showing it in the actionable messages flow.
Do you want me to help implement the PayInvoice handling logic?
🤖 Prompt for AI Agents
In @src/ui/key_handler/enter_handlers.rs at line 490, The Action::PayInvoice
branch in enter_handlers.rs is empty; either implement the intended behavior
(e.g., invoke the clipboard helper to copy the invoice payload, open/display
payment instructions, or send a PayInvoice command) inside the handler where
other actions are handled, or explicitly mark it as no-op with a comment if no
Enter action is required; alternatively remove PayInvoice from the
actionable-message check near the earlier filter (where AddInvoice and
PayInvoice are considered) so it never reaches this handler. Locate the match
arm for Action::PayInvoice and add the concrete call(s) to the existing helpers
used by similar cases (see how AddInvoice is handled) or add a clear comment
explaining intentional emptiness and update the filter logic accordingly.
Add Settings Tab Features: Mostro Pubkey, Relay, and Add Solver
Overview
This PR introduces new settings management features that allow users and admins to configure Mostro instance, relays, and dispute solvers directly from the Settings tab. All features include input validation, confirmation popups, and proper error handling.
✨ New Features
🌐 Add Mostro Pubkey (User & Admin)
Allows users to change the Mostro instance used by the client.
settings.tomlfileUse Case: Switch between different Mostro instances (mainnet, testnet, custom instances)
📡 Add Relay (User & Admin)
Allows users to add new Nostr relays to the client configuration.
wss://relay.example.com)settings.toml(no duplicates)Use Case: Add custom relays for better connectivity or privacy
➕ Add Solver (Admin Only)
Allows admins to add new dispute solvers to the Mostro network.
AdminAddSolvermessage to Mostro daemonUse Case: Onboard new dispute solvers to the network
🎨 User Experience Improvements
Consistent Confirmation Flow
All settings operations follow the same user-friendly pattern:
Input Validation
Tab Preservation
🔧 Technical Implementation
Settings Persistence
All settings are saved to
settings.toml:mostro_pubkey: Mostro instance public keyrelays: Array of relay URLs (appended, no duplicates)Admin Operations
execute_admin_add_solver()functionCode Organization
Settings-related handlers are organized in:
src/ui/key_handler/settings.rs- Settings save helperssrc/ui/key_handler/validation.rs- Input validationsrc/ui/key_handler/enter_handlers.rs- Settings action handlerssrc/util/order_utils/execute_admin_add_solver.rs- Network operation📝 Files Changed
New Files
src/util/order_utils/execute_admin_add_solver.rs- Admin AddSolver implementationModified Files
src/ui/settings_tab.rs- Added new settings optionssrc/ui/admin_key_confirm.rs- Custom message support for AddSolversrc/ui/key_handler/settings.rs- Settings save functionssrc/ui/key_handler/validation.rs- npub validationsrc/ui/key_handler/enter_handlers.rs- Settings action handlerssrc/ui/mod.rs- UI mode updates for new settings🧪 Testing Checklist
🎯 Use Cases
For Regular Users
For Admins
Summary by CodeRabbit
New Features
Improvements
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.