Admin dispute resolution and chatbox #27
Conversation
Implement fully functional peer-to-peer chat between admin and dispute parties (buyer/seller) using NIP-59 gift wrap events and ECDH-derived shared keys, following the mostro-chat project's simplified scheme. Key Changes: Core Implementation: - Add chat_utils.rs module with shared key derivation and NIP-59 helpers - derive_shared_chat_keys: ECDH-based shared secret derivation - send_admin_chat_message: Wrap and send NIP-59 gift wrap events - unwrap_admin_chat_event: Decrypt and verify incoming chat events - fetch_chat_messages_for_shared_key: Incremental message fetching - derive_and_send_admin_chat_message: Convenience helper for UI State Management: - Extend AppState with admin_chat_shared_keys HashMap to cache shared keys per (dispute_id, ChatParty) with last_seen_timestamp tracking - Add AdminChatSharedKey struct to store SharedChatKeys and timestamps Send Path: - Replace mock admin chat responses with real NIP-59 message sending - Derive shared keys on first message per dispute/party and cache them - Add logging for shared key verification (admin pubkey, counterparty, shared key hex) to aid testing with mostro-chat Receive Path: - Implement background listener in main.rs that polls every 5 seconds for new messages on cached shared keys - Use last_seen_timestamp for incremental fetches (only new messages) - Skip admin-signed messages to avoid duplicates - Convert incoming text notes to DisputeChatMessage and append to appropriate dispute chat history UI Integration: - Disputes In Progress tab now displays real messages from cache - Switching disputes instantly shows cached messages (no blocking fetch) - Background polling keeps messages up-to-date without UI blocking Testing: - Add unit tests for ECDH symmetry verification - Add roundtrip test for NIP-59 wrap/unwrap operations - Tests verify shared key derivation matches nostr_sdk expectations Documentation: - Update ADMIN_DISPUTES.md with NIP-59 chat flow details - Update TUI_INTERFACE.md with admin chat internals and stateless design - Document shared key derivation, sending, receiving, and restart behavior Code Quality: - Fix clippy issues: derive Hash for ChatParty, use PublicKey::parse - Remove unused imports - Add comprehensive logging for debugging shared key verification The implementation follows a stateless model: shared keys are re-derived on first send after restart, and recent messages are re-fetched using time-bounded incremental queries, ensuring consistency without a local message database.
- Move recover_admin_chat_from_files, apply_admin_chat_updates to ui/helpers - Move admin chat fetch to fetch_scheduler; fetch_admin_chat_updates to chat_utils - Add EnterKeyContext to fix clippy::too_many_arguments; add AdminChatByKey type alias - Persist admin message receiver in chat file (Admin to Buyer/Seller) so recovered admin messages show in the correct party view at startup
…ted commands for development
- Remove redundant p-tag filter in fetch_gift_wraps_to_admin (already filtered by pubkey) - Delete unused SharedChatKeys, derive_shared_chat_keys, send_admin_chat_message, build_chat_giftwrap_event, unwrap_admin_chat_event, fetch_chat_messages_for_shared_key - Remove related ECDH symmetry and roundtrip tests - Extract update_last_seen_timestamp helper for cleaner timestamp advancement - Update util::mod.rs re-exports to match remaining public API
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds an admin NIP‑59 chat subsystem (send/unwrap/fetch), per‑dispute/per‑party last‑seen cursors persisted to DB and files, extensive UI modularization and AppState/chat rendering updates, DB migrations for chat cursors, CI workflow, and supporting docs/tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Loop
participant Scheduler as Fetch Scheduler
participant Relay as Relay Network
participant Crypto as Decrypt/Unwrap
participant DB as Database
participant UI as UI State
Main->>Scheduler: spawn_admin_chat_fetch(admin_keys, disputes, cursors, tx)
Note over Scheduler: check CHAT_MESSAGES_SEMAPHORE (AtomicBool)
alt semaphore free
Scheduler->>Relay: fetch_gift_wraps_to_admin()
Relay-->>Scheduler: gift‑wrap events
Scheduler->>Crypto: unwrap_giftwrap_to_admin(event)
Crypto-->>Scheduler: (content, ts, sender_pubkey)
Scheduler->>DB: read AdminDisputes (map pubkeys→dispute/party)
DB-->>Scheduler: disputes list
Scheduler->>Scheduler: group by (dispute_id, party), filter by last_seen
Scheduler-->>Main: send AdminChatUpdate[]
else already running
Scheduler-->>Main: return early (no-op)
end
Main->>UI: apply_admin_chat_updates(updates)
UI->>UI: dedupe/append admin_dispute_chats
UI->>DB: update_chat_last_seen_by_dispute_id(...)
DB-->>UI: confirm persisted
UI->>UI: render updated chat
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/ADMIN_DISPUTES.md (1)
72-80:⚠️ Potential issue | 🟡 MinorHeading likely mismatched to section context.
This subsection is within “Disputes in Progress Tab,” so “Settings Tab Features” appears out of place. Consider renaming to “Disputes in Progress Features” or similar.📝 Suggested edit
-#### Settings Tab Features +#### Disputes in Progress Features
🤖 Fix all issues with AI agents
In @.cursor/commands/update_docs.md:
- Around line 3-10: Update the markdown in the "Overview" and "Steps" sections
to fix typos and tighten wording: change "markdown" to "Markdown", fix "gith" to
"Git", and standardize capitalization and sentence casing across list items; in
the Steps list, rephrase each bullet to be concise imperatives (e.g., "Identify
latest changes in Git history for files in the docs folder", "Analyze new
changes up to the latest commit", "Document new features, fixes, and
refactorings in the docs", "Add contextual notes for structural changes (e.g.,
update DATABASE.md for DB schema changes)") and ensure consistent punctuation
and style throughout.
In `@docs/TUI_INTERFACE.md`:
- Around line 223-227: The docs reference a non-existent AdminChatSharedKey and
SharedChatKeys; update the documentation to use the actual struct name
AdminChatLastSeen and its real fields instead of
AdminChatSharedKey/SharedChatKeys so the docs match the implementation; locate
the AdminChatLastSeen type in the codebase and mirror its field names and types
in the TUI_INTERFACE.md entry (replace AdminChatSharedKey and SharedChatKeys
with AdminChatLastSeen and its accurate field names).
In `@src/ui/disputes_in_progress_tab.rs`:
- Around line 512-527: Clamp the computed max_content_width to at least 1 to
avoid zero-width wrap problems on narrow terminals: change the calculation of
max_content_width (currently using chat_area.width.saturating_sub(2) / 2) to
ensure the resulting value is >= 1 before passing it into build_chat_list_items;
update the variable used in the branch that builds items from chat_messages (and
the else branch) so both calls to super::helpers::build_chat_list_items receive
a non-zero width (reference symbols: chat_area, max_content_width,
chat_messages, app.active_chat_party, build_chat_list_items).
In `@src/ui/helpers.rs`:
- Around line 106-116: The timestamp parsing silently falls back to 0 (ts via
unwrap_or(0)) which accepts malformed date/time blocks; update the parsing logic
in the function that builds the tuple (the code producing ts from date_str and
time_str and returning Some((sender, target_party, ts, content_block))) to
handle parse failures explicitly: either return None when
chrono::NaiveDate::parse_from_str or NaiveTime::parse_from_str fail so malformed
blocks are skipped, or emit a warning via the existing logger before returning
None; ensure you adjust the surrounding call sites to expect Option<(...,
timestamp, ...)> if you switch to returning None on parse failure.
In `@src/ui/key_handler/input_helpers.rs`:
- Around line 109-116: The code currently calls futures::executor::block_on
around crate::util::send_admin_chat_message_to_pubkey which blocks the main
event loop; replace that synchronous wait with tokio::spawn to run the async
send in the background (spawn a task that awaits
send_admin_chat_message_to_pubkey and logs or handles any Err), ensuring you
clone or move client, admin_keys, recipient_pubkey and message_content into the
spawned task as needed; update the call site in input_helpers.rs (the block
using futures::executor::block_on) to tokio::spawn and handle the Result inside
the spawned async closure instead of blocking the UI thread.
In `@src/util/chat_utils.rs`:
- Around line 119-130: The time-window constant seven_days_secs is incorrectly
set to 2 days (2 * 24 * 60 * 60); update the calculation to represent 7 days (7
* 24 * 60 * 60) so wide_since (computed from Timestamp::now().as_u64() and
seven_days_secs) uses a 7-day window; adjust the variable seven_days_secs in the
function that builds the Filter (see seven_days_secs, wide_since,
Timestamp::now(), Filter::new(), Kind::GiftWrap, admin_keys.public_key())
accordingly.
🧹 Nitpick comments (6)
src/ui/key_handler/input_helpers.rs (1)
122-127: Missing log for whenadmin_chat_keysis None.The function logs a warning when
counterparty_pubkeyis missing but silently does nothing whenadmin_chat_keysis None. This asymmetry could make debugging harder.💡 Add logging for missing admin keys
} else if counterparty_pubkey.is_none() { log::warn!( "Missing counterparty pubkey for dispute {} when sending chat message", dispute_id_key ); + } else if admin_chat_keys.is_none() { + log::warn!( + "Missing admin chat keys for dispute {} when sending chat message", + dispute_id_key + ); }src/models.rs (1)
537-575: Update methods don't verify rows were affected.Both
update_buyer_chat_last_seen_by_dispute_idandupdate_seller_chat_last_seen_by_dispute_idreturnOk(())even if no rows match thedispute_id. This could hide bugs where the dispute_id doesn't exist in the database.💡 Consider returning affected row count or checking for no-op
pub async fn update_buyer_chat_last_seen_by_dispute_id( pool: &SqlitePool, dispute_id: &str, ts: i64, ) -> Result<()> { - sqlx::query( + let result = sqlx::query( r#" UPDATE admin_disputes SET buyer_chat_last_seen = ? WHERE dispute_id = ? "#, ) .bind(ts) .bind(dispute_id) .execute(pool) .await?; + + if result.rows_affected() == 0 { + log::warn!("No dispute found with dispute_id {} when updating buyer_chat_last_seen", dispute_id); + } Ok(()) }src/ui/mod.rs (1)
383-390: Timestamp type inconsistency:u64inAdminChatUpdatevsi64inDisputeChatMessage.
AdminChatUpdate.messagesusesu64for timestamps whileDisputeChatMessage.timestampusesi64. This requires casting at the boundary (seen inapply_admin_chat_updatesat line 280:timestamp: ts as i64). Consider aligning types to avoid potential overflow issues with signed/unsigned conversion.💡 Consider using consistent timestamp type
Either change
AdminChatUpdateto usei64:pub struct AdminChatUpdate { pub dispute_id: String, pub party: ChatParty, - pub messages: Vec<(String, u64, PublicKey)>, + pub messages: Vec<(String, i64, PublicKey)>, }Or document that
u64is intentional for NIP-59 event timestamps and the cast is safe within the expected range.src/main.rs (2)
70-93: Unused parameter_admin_chat_keysinseed_admin_chat_last_seen.The function signature accepts
_admin_chat_keys: &Keysbut never uses it. Either remove the parameter or document why it's reserved for future use.💡 Remove unused parameter
-fn seed_admin_chat_last_seen(app: &mut AppState, _admin_chat_keys: &Keys) { +fn seed_admin_chat_last_seen(app: &mut AppState) { let disputes = app.admin_disputes_in_progress.clone();And update the call site:
- seed_admin_chat_last_seen(&mut app, &admin_chat_keys); + seed_admin_chat_last_seen(&mut app);
385-395: Admin chat fetch runs every 5 seconds regardless of activity.The
admin_chat_intervalfires every 5 seconds unconditionally when in Admin mode. For disputes with no activity, this creates unnecessary network traffic. Consider:
- Using exponential backoff when no new messages are received
- Only fetching for disputes that are actively displayed
src/util/chat_utils.rs (1)
167-192: Consider using HashMap for pubkey lookup if scale increases.The current implementation performs a linear scan of
pubkey_to_dispute_partyfor each message (line 185-188). This is O(n×m) where n = messages, m = disputes×2. For moderate scale this is fine, but if admin chat volume grows significantly, consider using aHashMap<PublicKey, (String, ChatParty)>for O(1) lookups.
- Correct 7-day fetch window (was 2 days) in chat_utils - Use tokio::spawn instead of block_on for non-blocking chat sends - Handle malformed timestamps gracefully in chat transcript parsing - Prevent zero-width wrap on narrow terminals - Update docs for simplified NIP-59 chat architecture Signed-off-by: arkanoider <github.913zc@simplelogin.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/ADMIN_DISPUTES.md`:
- Line 739: Replace the incorrect heading "Relay Management Features" under the
"Currency Filter Management" section with a correct, descriptive heading such as
"Currency Filter Features" (or "Currency Filter Management Features") so it
matches the following content about add/clear currency filters, dynamic
filtering, and status bar display; update the heading text where "Relay
Management Features" appears to the new heading.
- Line 72: The heading "#### Settings Tab Features" is misleading within the
"Disputes in Progress Tab" section; update that heading text (the string "####
Settings Tab Features") to accurately reflect the section content — e.g., "####
Disputes in Progress Tab Features" or "#### Disputes in Progress — Features" —
so it correctly describes chat and dispute management features.
- Line 751: Update the incorrect section heading "Relay Management
Implementation" to accurately reflect the content; change it to "Currency Filter
Implementation" (or similar) so it aligns with the functions shown
(save_currency_to_settings and clear_currency_filters) and the surrounding
"Currency Filter Management" section, ensuring the heading matches the code
example and context.
- Line 116: The heading "Currency Filter Features" is misleading for the section
that describes general settings tab behavior; rename that heading (replace the
"Currency Filter Features" heading text) to a broader, accurate title such as
"Settings Tab Features" or "General Settings Features" and update any internal
references or table-of-contents anchors that point to the old heading to match
the new text so links and cross-references remain correct.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…lookup - Combine buyer/seller update functions into one with is_buyer param - Return rows_affected to detect missing dispute_id - Use HashMap for O(1) pubkey lookups in chat routing
|
@coderabbitai review it please |
|
✅ Actions performedReview triggered.
|
So you have nothing else to say at the moment right? |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/ADMIN_DISPUTES.md`:
- Around line 647-653: Update the docs to reference the current fetch function:
replace the mention of fetch_chat_messages_for_shared_key with
fetch_admin_chat_updates in the paragraph describing
recover_admin_chat_from_files and the background listener; keep the surrounding
context about recover_admin_chat_from_files rebuilding admin_dispute_chats,
updating AppState.admin_chat_last_seen, and the DB fields
buyer_chat_last_seen/seller_chat_last_seen so the background listener uses those
DB cursors with fetch_admin_chat_updates to fetch only newer NIP‑59 events after
restart.
In `@src/ui/helpers.rs`:
- Around line 179-200: The update_last_seen_timestamp function currently only
updates existing entries in admin_chat_last_seen; if keys are missing those
recovered timestamps are never stored. Change update_last_seen_timestamp to
ensure entries exist before comparing: use the HashMap entry API (or insert
default AdminChatLastSeen) for the keys (dispute.dispute_id, ChatParty::Buyer)
and (dispute.dispute_id, ChatParty::Seller), then compare and set
last_seen_timestamp to buyer_max_timestamp/seller_max_timestamp when greater;
reference AdminChatLastSeen, ChatParty, and update_last_seen_timestamp to locate
and modify the logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/ui/helpers.rs`:
- Around line 162-176: In get_max_timestamp, avoid casting negative
DisputeChatMessage.timestamp values to u64; filter or clamp negatives before
conversion so an i64 negative timestamp cannot wrap to a huge u64. Update the
logic in get_max_timestamp (which iterates messages, filters by
ChatSender::Buyer/ChatSender::Seller and maps m.timestamp) to ignore messages
with m.timestamp < 0 or take max in i64 and then convert with a clamp to 0
(e.g., if max_i64 < 0 => 0 else max_i64 as u64), ensuring timestamp is validated
prior to casting.
- Around line 235-327: The in-memory last-seen map (app.admin_chat_last_seen)
may not have an entry for a dispute/party so updates never persist; in
apply_admin_chat_updates create or ensure an entry exists for
(dispute_key.clone(), party) before computing/updating max_ts and calling
AdminDispute::update_chat_last_seen_by_dispute_id — e.g., use
app.admin_chat_last_seen.entry((dispute_key.clone(), party)).or_insert_with(||
/* default AdminChatLastSeen */) so get_mut() always returns Some and the
updated last_seen_timestamp is stored and persisted.
Unify timestamp types across the codebase to i64, eliminating mixed i64/u64 usage that caused unsafe casts and defensive filtering. - Convert Nostr SDK u64 timestamps to i64 at API boundary - Update AdminChatLastSeen, AdminChatUpdate, OrderMessage, MessageNotification structs to use i64 - Simplify helpers by removing negative timestamp filtering - Use entry API to ensure last_seen updates persist for new disputes - Remove unnecessary casts now that types are consistent
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/util/dm_utils/mod.rs (1)
203-214:⚠️ Potential issue | 🟠 MajorAvoid panic on invalid
sinceranges.The
.unwrap()call can panic in two ways:
Duration::minutes(*since_time)panics if the minute value overflows when multiplied by 60checked_sub_signed()returnsNoneif the resulting datetime overflows Chrono's rangeWith an
i64parameter, arbitrarily large values are possible, making this a crash risk for the background task.Suggested safer handling
- let since_time = chrono::Utc::now() - .checked_sub_signed(chrono::Duration::minutes(*since_time)) - .unwrap() - .timestamp(); + let since_time = match chrono::Duration::try_minutes(*since_time) + .and_then(|d| chrono::Utc::now().checked_sub_signed(d)) + { + Some(dt) => dt.timestamp(), + None => { + log::warn!("Invalid `since` minutes: {}", since_time); + continue; + } + };src/ui/helpers.rs (1)
360-396:⚠️ Potential issue | 🟡 MinorIdempotent check breaks for wrapped content.
Line 366 compares
last_content(stored with wrapping) to rawmessage.content, so long messages will always append duplicates. Compare against the wrapped form instead.🛠️ Proposed fix (compare against wrapped content)
- if let Ok(existing) = fs::read_to_string(&file_path) { + let wrapped_content = wrap_text_to_lines(&message.content, 80).join("\n"); + if let Ok(existing) = fs::read_to_string(&file_path) { if let Some((last_sender, last_target_party, last_ts, last_content)) = parse_last_message_block(&existing) { if last_sender == message.sender && last_ts == message.timestamp - && last_content == message.content + && last_content == wrapped_content && last_target_party == message.target_party { return; } } } @@ - let content_lines = wrap_text_to_lines(&message.content, 80); - let content_block = content_lines.join("\n"); + let content_block = wrapped_content;
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Around line 29-33: The Cargo.toml currently adds reqwest without selecting
rustls, but your code in src/main.rs initializes rustls's ring provider; either
configure reqwest to use rustls or remove the rustls usage in code. Fix by
updating the reqwest dependency to explicitly disable default-features and
enable the rustls TLS features (so reqwest uses rustls), or conversely remove
the rustls crate and the rustls initialization in src/main.rs; reference the
reqwest entry in Cargo.toml and the rustls initialization in src/main.rs to
locate the changes.
In `@src/main.rs`:
- Around line 185-186: Add a single-flight guard so overlapping admin fetches
are skipped: create a shared Arc<AtomicBool> (or Arc<Mutex<bool>>) flag and pass
a clone into spawn_admin_chat_fetch; inside spawn_admin_chat_fetch check the
flag (AtomicBool.compare_exchange false->true) and return early if another fetch
is running, then run fetch_admin_chat_updates/fetch_gift_wraps_to_admin and
ensure the flag is cleared in a finally-like cleanup (or via RAII guard) even on
error; keep apply_admin_chat_updates unchanged but reference the same flag when
scheduling via admin_chat_interval and user_role so only one concurrent fetch
can run.
🧹 Nitpick comments (1)
src/main.rs (1)
169-180: Consider parsing admin_privkey once and reusing it.The key is parsed in multiple places (pubkey derivation, seeding, admin_chat_keys). A single parse with shared
Option<Keys>would reduce duplication and avoid repeated warnings.Also applies to: 196-207, 235-240
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/models.rs (1)
481-487:⚠️ Potential issue | 🟠 MajorFix SQL bind mismatch in
get_all.Line 486 binds a parameter, but the query has no
?placeholder.sqlxwill error at runtime. If the intent is to filter InProgress, add a WHERE clause; otherwise remove the bind.✅ Suggested fix (filter InProgress)
- let mut disputes = sqlx::query_as::<_, AdminDispute>( - r#"SELECT * FROM admin_disputes ORDER BY taken_at DESC"#, - ) - .bind(DisputeStatus::InProgress.to_string()) + let mut disputes = sqlx::query_as::<_, AdminDispute>( + r#"SELECT * FROM admin_disputes WHERE status = ? ORDER BY taken_at DESC"#, + ) + .bind(DisputeStatus::InProgress.to_string())
…ltiple chat messages fetch operations
… chat single-flight
arkanoider
changed the title
I opened a dispute in an order with fiat ARS but here we can see USD as fiat |
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.cursor/commands/build.md:
- Around line 6-13: Update the top line sentence "Build and test mostrix fixing
all error of the cargo and clippy." to correct grammar ("fixing all errors") and
reorder the step list so it matches CI: run "cargo fmt --all" first, then "cargo
clippy --all-targets --all-features", then "cargo test", and finally "cargo
build"; specifically edit the sentence and the bullet items shown (the header
sentence and the four steps) to reflect these changes.
In `@src/ui/app_state.rs`:
- Around line 91-100: In switch_role (in impl for AppState) also clear all
admin-specific state to avoid stale data: reset admin_dispute_chats (clear()),
admin_chat_last_seen (clear()), admin_disputes_in_progress (clear() or set to
default empty), admin_chat_list_state (reset to Default::default() or new
ListState), admin_chat_scroll_tracker (clear() or set to default), and
dispute_filter (set to default/None as appropriate); if preserving admin context
is intended instead, add a clarifying comment in switch_role mentioning that
admin state is intentionally retained. Ensure you reference these exact fields
(admin_dispute_chats, admin_chat_last_seen, admin_disputes_in_progress,
admin_chat_list_state, admin_chat_scroll_tracker, dispute_filter) when making
the changes.
🧹 Nitpick comments (13)
.github/workflows/ci.yml (1)
39-40:cargo testdoesn't use--all-features, butclippydoes.
clippy(Line 28) runs with--all-targets --all-features, butcargo testruns without feature flags. This means feature-gated code is linted but never tested in CI. Consider aligning the two:- name: cargo test - run: cargo test + run: cargo test --all-featuressrc/ui/key_handler/message_handlers.rs (1)
11-11: Consider importingEnterKeyContextinstead of using fully qualified paths.Both
handle_enter_viewing_message(Line 11) andhandle_enter_message_notification(Line 86) use the fully qualified pathcrate::ui::key_handler::EnterKeyContext<'_>. Auseimport at the top would improve readability.+use crate::ui::key_handler::EnterKeyContext; use crate::ui::OrderResult;src/ui/key_handler/admin_handlers.rs (1)
63-66: Nit:mostro_pubkey_cloneis misleading for aCopytype.
nostr_sdk::PublicKeyimplementsCopy, solet mostro_pubkey_clone = mostro_pubkey;is just a binding rename, not a clone. Consider naming it simplymostro_pubkey(shadowing) ormostro_pkto avoid implying a heap allocation.src/ui/admin_state.rs (1)
13-15: Consider replacing positional fields with named structs or enums for clarity.
ReviewingDisputeForFinalization(Uuid, usize)uses a magicusizeindex, andConfirmFinalizeDispute(Uuid, bool, bool)has two consecutive booleans — both are easy to mix up at call sites. A small enum and/or named fields would make intent clearer:Illustrative sketch
pub enum FinalizeOption { PayBuyer, // 0 RefundSeller, // 1 Exit, // 2 } // Then: ReviewingDisputeForFinalization(uuid::Uuid, FinalizeOption), ConfirmFinalizeDispute { dispute_id: uuid::Uuid, is_settle: bool, confirmed: bool, },src/ui/state.rs (1)
1-9: Consider consolidatingAdminModeandUserModere-exports instate.rsfor consistency.This barrel module re-exports UI types from
app_state,chat,navigation, andorders, butAdminModeandUserModeare re-exported separately insrc/ui/mod.rsinstead. While this works and the types are accessible, the split pattern may confuse contributors looking for the complete UI types API in one place. Moving these two re-exports intostate.rswould make the module truly comprehensive.src/ui/chat.rs (1)
1-59: Clean data model design for chat types.The type hierarchy is well-structured:
ChatPartywithHashfor use as HashMap keys, separateChatSenderwith theAdminvariant, and clear per-dispute tracking structs.One minor readability suggestion:
AdminChatUpdate.messagesuses a bare tuple(String, i64, PublicKey). A small named struct (e.g.,RawChatMessage { content, timestamp, sender_pubkey }) would make field access self-documenting at call sites and reduce the risk of field-order mix-ups.src/ui/draw.rs (2)
39-47: Mutex.unwrap()will panic on poisoned lock.Lines 40 and 92 use
.lock().unwrap()onArc<Mutex<...>>. If a writer thread panics while holding the lock, the mutex becomes poisoned and the TUI render loop will crash. In a single-threaded TUI draw path this is typically acceptable, but if you want resilience, consider using.lock().unwrap_or_else(|e| e.into_inner())to recover the inner data from a poisoned mutex.
96-301: Overlay rendering: sequentialif letchecks are correct but could be a singlematch.Since
app.modeis an enum, only one variant is active at a time, so at most one overlay renders per frame. The sequentialif letblocks are functionally equivalent to amatchbut more verbose. A singlematch &app.mode { ... }would consolidate the dispatch, reduce line count, and make it immediately clear that variants are mutually exclusive. That said, the current style with per-section comments is also readable — this is a matter of preference.src/ui/navigation.rs (2)
34-44:from_indexpanics on out-of-bounds — consider a safer alternative.Both
UserTab::from_index(line 42) andAdminTab::from_index(line 122) panic on an invalid index. While internal callers likeget_titlesbound the range viacount(), any future caller passing an unvalidated index will crash the application. ReturningOption<Self>would make the API more defensive.
57-59: Hardcodedcount()must be kept in sync with variants manually.
UserTab::count()returns6andAdminTab::count()returns4. If a variant is added or removed, these values and thefrom_index/as_indexmatch arms must all be updated in lockstep. Consider deriving the count from the enum (e.g., adding a sentinel variant, usingstrum::EnumCount, or a const based on the last index + 1) to reduce this maintenance burden.Also applies to: 135-137
src/ui/app_state.rs (1)
39-62:AppStatehas a large public surface — all fields arepub.Every field of
AppStateispub, which means any module can mutate any field directly. This makes it harder to enforce invariants (e.g.,selected_dispute_idxwithin bounds,admin_chat_input_enabledconsistency). For a TUI app this is common and pragmatic, but as the struct grows, consider encapsulating groups of related fields behind accessor methods or extracting sub-structs (e.g., anAdminChatStatestruct grouping lines 45–53).src/ui/mod.rs (1)
7-9: Public sub-modules andpub usere-exports create dual access paths.The sub-modules
app_state,chat,navigation, andordersare declared aspub mod, and their types are also re-exported throughpub use state::{...}. This means external code can reachAppStatevia bothcrate::ui::AppStateandcrate::ui::app_state::AppState. If the intent is for consumers to use only the top-level re-exports, consider making the sub-modulespub(crate)instead ofpubto prevent accidental coupling to the internal module structure.Also applies to: 23-24, 30-38
src/ui/orders.rs (1)
5-30:OrderResult::Successvariant has 11 fields — consider extracting to a named struct.The
Successvariant carries a large number of fields, which makes pattern-matching verbose and field ordering error-prone at construction sites. Extracting these into a dedicatedOrderSuccessstruct would improve readability and allow addingDefaultfor optional fields.Example extraction
#[derive(Clone, Debug)] pub struct OrderSuccess { pub order_id: Option<uuid::Uuid>, pub kind: Option<mostro_core::order::Kind>, pub amount: i64, pub fiat_code: String, pub fiat_amount: i64, pub min_amount: Option<i64>, pub max_amount: Option<i64>, pub payment_method: String, pub premium: i64, pub status: Option<Status>, pub trade_index: Option<i64>, } #[derive(Clone, Debug)] pub enum OrderResult { Success(OrderSuccess), PaymentRequestRequired { /* ... */ }, Info(String), Error(String), }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/order_result.rs (1)
135-140:⚠️ Potential issue | 🟡 MinorMinor:
inner.width as usize - 2can underflow for extremely narrow terminals.
usizesubtraction wraps to a huge value ifinner.width < 2, which would cause.chunks()to behave unexpectedly. The same pattern appears on line 169. Given the hardcodedpopup_width = 70, this is practically unreachable, butsaturating_subis a zero-cost safeguard.🛡️ Proposed fix
- .chunks(inner.width as usize - 2) + .chunks((inner.width as usize).saturating_sub(2).max(1))Apply the same on line 169.
docs/ADMIN_DISPUTES.md (1)
973-988:⚠️ Potential issue | 🟡 MinorStale "Planned Implementation" section contradicts PR implementation.
Lines 973-988 describe a
Chattab as a planned/future feature and list anAdminTabenum withDisputes, Chat, Settings. However, this PR implements admin chat within the "Disputes in Progress" tab (not a separate Chat tab), and the actualAdminTabenum in the codebase now includesDisputesPending, DisputesInProgress, Settings, Exit. The code snippet and note on line 988 are outdated.📝 Consider updating or removing this section
The "Planned Implementation" section references an old
AdminTabenum definition and a standalone "Chat" tab concept that has been superseded by the integrated chat in the Disputes in Progress tab. Consider updating this to reflect the current architecture or removing the stale reference.
🤖 Fix all issues with AI agents
In `@src/ui/draw.rs`:
- Around line 127-136: The prompt/placeholder for the Mostro pubkey is
misleading: when UiMode::AddMostroPubkey calls
key_input_popup::render_key_input_popup it shows "npub..." but
validate_mostro_pubkey expects a 64-character hex string; update the prompt text
and placeholder passed to render_key_input_popup to indicate a 64-character hex
(e.g., "Enter Mostro public key (64 hex chars):" and placeholder like "0123...
(64 hex chars)") so the UI matches validate_mostro_pubkey and prevents user
confusion.
🧹 Nitpick comments (4)
src/util/dm_utils/order_ch_mng.rs (1)
67-82: All match arms are identical — consider collapsing.Every branch of this
matchsetsapp.mode = UiMode::OrderResult(result). The per-variant arms add no distinct behavior and only increase noise.♻️ Suggested simplification
- // Set appropriate result mode based on current state - match app.mode { - UiMode::UserMode(UserMode::WaitingTakeOrder(_)) => { - app.mode = UiMode::OrderResult(result); - } - UiMode::UserMode(UserMode::WaitingAddInvoice) => { - app.mode = UiMode::OrderResult(result); - } - UiMode::NewMessageNotification(_, _, _) => { - // If we have a notification, replace it with the result - app.mode = UiMode::OrderResult(result); - } - _ => { - app.mode = UiMode::OrderResult(result); - } - } + // All modes transition to showing the order result + app.mode = UiMode::OrderResult(result);src/ui/key_handler/chat_helpers.rs (1)
158-197: Consider extracting the duplicated finalization guard.The
PayBuyerandRefundSellerarms are identical except foris_settle. A small helper or shared block would reduce duplication.♻️ Possible consolidation
- FinalizeDisputePopupButton::PayBuyer => { - if dispute_is_finalized { - let _ = order_result_tx.send(OrderResult::Error( - "Cannot finalize: dispute is already finalized".to_string(), - )); - app.mode = UiMode::AdminMode(AdminMode::ManagingDispute); - } else { - app.mode = UiMode::AdminMode(AdminMode::ConfirmFinalizeDispute { - dispute_id, - // is_settle: true = Pay Buyer - is_settle: true, - // selected_button: true = Yes - selected_button: true, - }); - } - true - } - FinalizeDisputePopupButton::RefundSeller => { - if dispute_is_finalized { - let _ = order_result_tx.send(OrderResult::Error( - "Cannot finalize: dispute is already finalized".to_string(), - )); - app.mode = UiMode::AdminMode(AdminMode::ManagingDispute); - } else { - app.mode = UiMode::AdminMode(AdminMode::ConfirmFinalizeDispute { - dispute_id, - // is_settle: false = Refund Seller - is_settle: false, - // selected_button: true = Yes - selected_button: true, - }); - } - true - } + FinalizeDisputePopupButton::PayBuyer | FinalizeDisputePopupButton::RefundSeller => { + let is_settle = button == FinalizeDisputePopupButton::PayBuyer; + if dispute_is_finalized { + let _ = order_result_tx.send(OrderResult::Error( + "Cannot finalize: dispute is already finalized".to_string(), + )); + app.mode = UiMode::AdminMode(AdminMode::ManagingDispute); + } else { + app.mode = UiMode::AdminMode(AdminMode::ConfirmFinalizeDispute { + dispute_id, + is_settle, + selected_button: true, + }); + } + true + }src/ui/key_handler/admin_handlers.rs (1)
1-13: Nit:execute_take_disputeimport could be consolidated with line 2.Lines 2 and 13 both import from
crate::util::order_utils. Merging them keeps the import block tidy.♻️ Consolidated import
-use crate::util::order_utils::{execute_admin_add_solver, execute_finalize_dispute}; ... -use crate::util::order_utils::execute_take_dispute; +use crate::util::order_utils::{execute_admin_add_solver, execute_finalize_dispute, execute_take_dispute};src/ui/key_handler/mod.rs (1)
339-384: Consider extractingEnterKeyContextconstruction to reduce duplication.The identical
EnterKeyContextis built on lines 340–348 and again on lines 375–383. Since all fields are references (zero allocation cost), the context could be constructed once before thematchto stay DRY.♻️ Optional: hoist ctx before the match
+ let ctx = EnterKeyContext { + orders, + disputes, + pool, + client, + mostro_pubkey, + order_result_tx, + admin_chat_keys, + }; + match code { // ... KeyCode::Enter => { - let ctx = EnterKeyContext { - orders, - disputes, - pool, - client, - mostro_pubkey, - order_result_tx, - admin_chat_keys, - }; let should_continue = handle_enter_key(app, &ctx); Some(should_continue) } // ... KeyCode::Char('y') | KeyCode::Char('Y') => { - let ctx = EnterKeyContext { - orders, - disputes, - pool, - client, - mostro_pubkey, - order_result_tx, - admin_chat_keys, - }; let should_continue = handle_confirm_key(app, &ctx); Some(should_continue) }
fixed! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/models.rs`:
- Around line 320-323: The code silently defaults fiat_code to "USD" when
fiat_code_from_relay is None or empty, which can mask data-fetch failures and
cause mismatched displays; update the logic around fiat_code_from_relay and
fiat_code so that when fiat_code_from_relay is None or empty you either return
an error (e.g., propagate a Result/Error from the enclosing function) with a
clear message about missing relay fiat code, or at minimum emit a warning log
(e.g., tracing::warn/processLogger.warn) and return a distinct error/None
upstream instead of setting "USD"; replace the unwrap_or_else("USD".to_string())
behavior accordingly and ensure callers handle the error/None path.
In `@src/util/order_utils/execute_take_dispute.rs`:
- Around line 109-114: The current call to fetch_order_fiat_from_relay uses
.await.ok().flatten(), which swallows any error and causes silent fallback to
AdminDispute::new's "USD" default; update the handling around
fiat_code_from_relay to capture the Result, log the error when Err (including
context like dispute_info.id and mostro_pubkey) before falling back, and then
extract the Some(value) if present (so keep fetch_order_fiat_from_relay,
fiat_code_from_relay variable, and AdminDispute::new usage intact but ensure
errors are logged rather than discarded).
🧹 Nitpick comments (1)
src/util/order_utils/helper.rs (1)
294-317: Newfetch_order_fiat_from_relaylooks correct, but verify the relay fetch is actually succeeding in practice.This function is the sole source of truth for fiat code in disputes. Per the PR comments, grunch reported that ARS orders display as USD — the silent
"USD"fallback inAdminDispute::new(line 323 ofsrc/models.rs) will kick in whenever this function returnsOk(None)or the caller swallows anErr(whichexecute_take_dispute.rsdoes via.ok().flatten()).Consider adding a
log::warn!when the relay returns no matching event, so the fallback is at least visible in logs:Suggested improvement
let event = match events.iter().next() { Some(ev) => ev, - None => return Ok(None), + None => { + log::warn!("No order event found on relay for order_id={order_id}; fiat_code will fall back to default"); + return Ok(None); + } };
arkanoider
changed the title
2 participants
Summary
This PR adds full admin dispute management to Mostrix: admins can view pending disputes, take ownership, chat with buyer and seller, and finalize (Pay Buyer or Refund Seller). Order fiat code for disputes is resolved from the relay when taking a dispute, so admins do not need the order in their local database.
Changes
Admin dispute workflow
Initiateddisputes; select and press Enter to take a dispute (signed withadmin_privkey). Fiat code is fetched from the relay (order eventd/ftags); no DB fallback for admins.npub), Change Admin Key. Mode switch (M) toggles User/Admin and persists tosettings.toml.Admin chat
~/.mostrix/<dispute_id>.txtand last-seen timestamps in DB.Other
admin_privkeyonce and reuse; clippy (e.g. module_inception), timestamps as i64, chat robustness and crypto backend fixes.Checklist