feat(my-trades): receive attachments#80
Conversation
… echoes Add Phase A user order chat parity with admin dispute chat: parse and display encrypted attachments, Ctrl+S save popup with shared-key decrypt, ScrollView scrolling (PgUp/PgDn/End), and skip relay echoes of the local trade key so sent messages no longer appear on both You and Peer sides. Co-authored-by: Cursor <cursoragent@cursor.com>
WalkthroughThis PR implements attachment-saving functionality for the user role's "My Trades" order chat. It adds a new UI mode variant for an attachment-selection popup (Ctrl+S), refactors chat-update application to parse and display attachments with toast notifications, provides helpers for attachment visibility and chat scrolling, and updates the order-chat panel with attachment indicators and footer shortcuts. ChangesOrder chat attachment saving
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed strictly. No blocker.
What I checked:
Attachment receive + dedup
The relay-echo skip (sender_pubkey == update.local_trade_pubkey) matches the pattern used in admin chat. The dedup by (timestamp, content) is applied after the echo skip, so it only catches genuine duplicates from concurrent relay syncs, not our own messages. That is correct.
Decryption key resolution
order_chat_decryption_key_bytes tries order_chat_shared_key_hex first (negotiated key), then falls back to ECDH from trade_keys + counterparty_pubkey. That is the right priority. The decryption key is looked up at save time (lazy), not at parse time, which is fine.
UserSaveAttachmentPopup dispatch
I checked the dispatch order carefully. All keys including Enter are consumed and return Some(true) inside the UserSaveAttachmentPopup block in mod.rs, so the dead-code arm in enter_handlers.rs is never reached. Not a bug, just cosmetically redundant (same pattern as ObserverSaveAttachmentPopup).
Scroll tracking
The order_chat_scroll_tracker is keyed by order ID string and updated on every render pass. Auto-scroll fires on new messages and on order switch. The scroll_to_bottom() on send is also wired correctly via scroll_order_chat_after_send. I do not see a way for the scroll state to get out of sync.
Toast lifetime
The toast is set on attachment receive and rendered in the footer. It is not cleared on a timer, but this matches the existing attachment_toast pattern used elsewhere in the codebase — not a regression.
One minor nit (non-blocking): the arm in enter_handlers.rs for UserSaveAttachmentPopup is dead code (never reached because mod.rs already returns before calling handle_enter_key). Worth a comment like the ObserverSaveAttachmentPopup arm above it, but it does not affect correctness.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/DATABASE.md (1)
176-197:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
ordersschema snippet consistent with the field table.
order_chat_shared_key_hexis documented in the field list (Line 218) but missing from the SQL schema block above, so readers get conflicting schema definitions.Suggested doc patch
CREATE TABLE IF NOT EXISTS orders ( id TEXT PRIMARY KEY, kind TEXT, status TEXT, amount INTEGER NOT NULL, fiat_code TEXT NOT NULL, min_amount INTEGER, max_amount INTEGER, fiat_amount INTEGER NOT NULL, payment_method TEXT NOT NULL, premium INTEGER NOT NULL, trade_keys TEXT, counterparty_pubkey TEXT, + order_chat_shared_key_hex TEXT, is_mine INTEGER NOT NULL, buyer_invoice TEXT, request_id INTEGER, trade_index INTEGER, created_at INTEGER, expires_at INTEGER, last_seen_dm_ts INTEGER );Also applies to: 218-218
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/DATABASE.md` around lines 176 - 197, The CREATE TABLE orders SQL block is missing the documented field order_chat_shared_key_hex; update the orders table DDL in the SQL snippet to include the order_chat_shared_key_hex column and ensure its datatype and nullability match the field list (i.e., add a column named order_chat_shared_key_hex with the same type—likely TEXT—and the same NOT NULL or nullable constraint as documented) so the SQL schema and the field table stay consistent.src/ui/tabs/order_in_progress_tab.rs (1)
352-371:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAccount for the toast row when sizing the footer.
can_fit_two_line_footer/can_fit_three_line_footerare computed before the toast row is added. With only 3-4 spare rows,attachment_toastcan consume the last chat row and leave the chat pane at zero height until the toast expires.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/tabs/order_in_progress_tab.rs` around lines 352 - 371, The footer sizing currently computes can_fit_three_line_footer / can_fit_two_line_footer from spare_below_header_input before accounting for app.attachment_toast, which can steal the last chat row; update the logic so the toast row is reserved when deciding footer size by computing a spare_after_toast (e.g. spare_below_header_input.saturating_sub(if app.attachment_toast.is_some() { 1 } else { 0 })) and use spare_after_toast when setting can_fit_three_line_footer and can_fit_two_line_footer and subsequently determining footer_height (symbols: spare_below_header_input, can_fit_three_line_footer, can_fit_two_line_footer, footer_height, app.attachment_toast).
🧹 Nitpick comments (1)
src/util/chat_utils.rs (1)
74-88: ⚡ Quick winAdd a regression test for both decryption-key branches.
order_chat_decryption_key_bytesnow chooses between persisted shared-key hex and on-the-fly ECDH derivation. A small unit test that asserts both paths produce the same 32-byte key for the same order would harden the attachment-save path against silent crypto regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/chat_utils.rs` around lines 74 - 88, Add a unit test that constructs an Order and validates both branches of order_chat_decryption_key_bytes produce the same 32-byte key: (1) prepare an order with order_chat_shared_key_hex populated (use keys_from_shared_hex to generate or a known valid shared-hex) and call order_chat_decryption_key_bytes to get the persisted-hex path result; (2) prepare an equivalent order without order_chat_shared_key_hex but with trade_keys (SecretKey -> Keys) and counterparty_pubkey (PublicKey) that correspond to the same ECDH secret and call order_chat_decryption_key_bytes to exercise the derive_shared_keys path; assert both outputs are Some with length 32 and equal. Use the existing helpers keys_from_shared_hex, derive_shared_keys, Keys, SecretKey::from_str and PublicKey::parse to build matching key material so the test deterministically compares the two branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/MESSAGE_FLOW_AND_PROTOCOL.md`:
- Line 309: The doc contradicts itself about the poll interval: update the
wording so the user polling interval and the admin interval are consistent —
either rename the user timer to a distinct symbol (e.g., user_chat_interval used
by spawn_user_order_chat_fetch and referenced where
fetch_user_order_chat_updates is described) or state that
spawn_user_order_chat_fetch uses the same admin_chat_interval
(admin_chat_interval) and set the value consistently (e.g., 5 seconds) in both
places; ensure you update both occurrences of the interval statement and any
mention of admin_chat_interval, spawn_user_order_chat_fetch,
fetch_user_order_chat_updates, CHAT_MESSAGES_SEMAPHORE, and
order_chat_shared_key_hex/trade_keys+counterparty_pubkey to match the chosen
name and value.
In `@src/ui/helpers/startup.rs`:
- Around line 331-338: The dedupe check in the loop (using
messages_vec.iter().any(...) and the is_duplicate branch) is incorrectly
collapsing optimistic local “You” messages; change the predicate so it only
treats a message as duplicate if the existing message is from a remote peer
(e.g., check that the existing message's sender_pubkey != local_pubkey or that
it is not a local/You message) before comparing timestamp and content, and only
then update max_ts and continue; keep the existing relay/sender_pubkey filtering
intact by referencing the same sender_pubkey/local identity check used earlier
in this function.
In `@src/ui/key_handler/mod.rs`:
- Around line 592-600: The popup currently re-resolves the target order from
active_order_chat_list_snapshot using app.selected_order_chat_idx which can
change; modify the UiMode::UserSaveAttachmentPopup variant to carry the frozen
order_id (e.g., UiMode::UserSaveAttachmentPopup(order_id: OrderId)) and update
the code paths that open the popup to store the originating order id; then
change all places that call
active_order_chat_list_snapshot(...).get(app.selected_order_chat_idx) and any
subsequent get_order_attachment_messages(app, id) (including the blocks around
the current UserSaveAttachmentPopup handling and the other two spots mentioned)
to use the stored order_id from the UiMode instead of re-reading the live
snapshot so rendering and save operations always use the pinned order id.
---
Outside diff comments:
In `@docs/DATABASE.md`:
- Around line 176-197: The CREATE TABLE orders SQL block is missing the
documented field order_chat_shared_key_hex; update the orders table DDL in the
SQL snippet to include the order_chat_shared_key_hex column and ensure its
datatype and nullability match the field list (i.e., add a column named
order_chat_shared_key_hex with the same type—likely TEXT—and the same NOT NULL
or nullable constraint as documented) so the SQL schema and the field table stay
consistent.
In `@src/ui/tabs/order_in_progress_tab.rs`:
- Around line 352-371: The footer sizing currently computes
can_fit_three_line_footer / can_fit_two_line_footer from
spare_below_header_input before accounting for app.attachment_toast, which can
steal the last chat row; update the logic so the toast row is reserved when
deciding footer size by computing a spare_after_toast (e.g.
spare_below_header_input.saturating_sub(if app.attachment_toast.is_some() { 1 }
else { 0 })) and use spare_after_toast when setting can_fit_three_line_footer
and can_fit_two_line_footer and subsequently determining footer_height (symbols:
spare_below_header_input, can_fit_three_line_footer, can_fit_two_line_footer,
footer_height, app.attachment_toast).
---
Nitpick comments:
In `@src/util/chat_utils.rs`:
- Around line 74-88: Add a unit test that constructs an Order and validates both
branches of order_chat_decryption_key_bytes produce the same 32-byte key: (1)
prepare an order with order_chat_shared_key_hex populated (use
keys_from_shared_hex to generate or a known valid shared-hex) and call
order_chat_decryption_key_bytes to get the persisted-hex path result; (2)
prepare an equivalent order without order_chat_shared_key_hex but with
trade_keys (SecretKey -> Keys) and counterparty_pubkey (PublicKey) that
correspond to the same ECDH secret and call order_chat_decryption_key_bytes to
exercise the derive_shared_keys path; assert both outputs are Some with length
32 and equal. Use the existing helpers keys_from_shared_hex, derive_shared_keys,
Keys, SecretKey::from_str and PublicKey::parse to build matching key material so
the test deterministically compares the two branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 509f7263-5b5f-4e70-8ff0-3ee9153c284d
📒 Files selected for processing (21)
docs/DATABASE.mddocs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/README.mddocs/STARTUP_AND_CONFIG.mddocs/TUI_INTERFACE.mdsrc/ui/app_state.rssrc/ui/chat.rssrc/ui/constants.rssrc/ui/draw.rssrc/ui/help_popup.rssrc/ui/helpers/chat_visibility.rssrc/ui/helpers/mod.rssrc/ui/helpers/startup.rssrc/ui/key_handler/chat_helpers.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/esc_handlers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/navigation.rssrc/ui/save_attachment_popup.rssrc/ui/tabs/order_in_progress_tab.rssrc/util/chat_utils.rs
| - **Startup restore**: `load_user_order_chats_at_startup` restores cached chat into `AppState.order_chats` and seeds `order_chat_last_seen` before relay backfill. | ||
| - **Incremental merge**: `apply_user_order_chat_updates` deduplicates by `(timestamp, content)`, persists new entries, and advances per-order cursors. | ||
| - **Startup restore**: `load_user_order_chats_at_startup` restores cached chat into `AppState.order_chats`, seeds `order_chat_last_seen`, then runs an initial `fetch_user_order_chat_updates` relay backfill. | ||
| - **Periodic relay sync (User role)**: every 2 seconds (`admin_chat_interval` in `src/main.rs`), `spawn_user_order_chat_fetch` polls active orders via `fetch_user_order_chat_updates`. Uses the same single-flight `CHAT_MESSAGES_SEMAPHORE` as admin chat so overlapping fetches are skipped. Shared keys come from persisted `order_chat_shared_key_hex` when set, otherwise ECDH from local `trade_keys` + `counterparty_pubkey` (`src/util/chat_utils.rs`). |
There was a problem hiding this comment.
Clarify interval naming/value to avoid contradiction in this doc.
Line 309 says user polling is every 2 seconds via admin_chat_interval, but later this same doc describes admin_chat_interval as 5 seconds for admin chat. Please either use a distinct timer name for user chat or align the interval statement so both sections agree.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/MESSAGE_FLOW_AND_PROTOCOL.md` at line 309, The doc contradicts itself
about the poll interval: update the wording so the user polling interval and the
admin interval are consistent — either rename the user timer to a distinct
symbol (e.g., user_chat_interval used by spawn_user_order_chat_fetch and
referenced where fetch_user_order_chat_updates is described) or state that
spawn_user_order_chat_fetch uses the same admin_chat_interval
(admin_chat_interval) and set the value consistently (e.g., 5 seconds) in both
places; ensure you update both occurrences of the interval statement and any
mention of admin_chat_interval, spawn_user_order_chat_fetch,
fetch_user_order_chat_updates, CHAT_MESSAGES_SEMAPHORE, and
order_chat_shared_key_hex/trade_keys+counterparty_pubkey to match the chosen
name and value.
| let is_duplicate = messages_vec | ||
| .iter() | ||
| .any(|m| m.timestamp == msg.timestamp && m.content == msg.content); | ||
| if duplicated { | ||
| .any(|m| m.timestamp == ts && m.content == msg_content); | ||
| if is_duplicate { | ||
| if ts > max_ts { | ||
| max_ts = ts; | ||
| } | ||
| continue; |
There was a problem hiding this comment.
Limit deduplication to existing peer messages.
Relay echoes are already filtered above on sender_pubkey, so this second pass should not collapse against locally inserted You messages. As written, a peer message with the same second-level timestamp and content as an optimistic local send gets dropped.
💡 Suggested fix
let is_duplicate = messages_vec
.iter()
- .any(|m| m.timestamp == ts && m.content == msg_content);
+ .any(|m| {
+ m.sender == UserChatSender::Peer
+ && m.timestamp == ts
+ && m.content == msg_content
+ });📝 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.
| let is_duplicate = messages_vec | |
| .iter() | |
| .any(|m| m.timestamp == msg.timestamp && m.content == msg.content); | |
| if duplicated { | |
| .any(|m| m.timestamp == ts && m.content == msg_content); | |
| if is_duplicate { | |
| if ts > max_ts { | |
| max_ts = ts; | |
| } | |
| continue; | |
| let is_duplicate = messages_vec | |
| .iter() | |
| .any(|m| { | |
| m.sender == UserChatSender::Peer | |
| && m.timestamp == ts | |
| && m.content == msg_content | |
| }); | |
| if is_duplicate { | |
| if ts > max_ts { | |
| max_ts = ts; | |
| } | |
| continue; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/helpers/startup.rs` around lines 331 - 338, The dedupe check in the
loop (using messages_vec.iter().any(...) and the is_duplicate branch) is
incorrectly collapsing optimistic local “You” messages; change the predicate so
it only treats a message as duplicate if the existing message is from a remote
peer (e.g., check that the existing message's sender_pubkey != local_pubkey or
that it is not a local/You message) before comparing timestamp and content, and
only then update max_ts and continue; keep the existing relay/sender_pubkey
filtering intact by referencing the same sender_pubkey/local identity check used
earlier in this function.
| // User order chat save attachment popup: Up/Down to select, Enter to save, Esc to cancel | ||
| if matches!(app.mode, UiMode::UserSaveAttachmentPopup(_)) { | ||
| let order_id = active_order_chat_list_snapshot(app) | ||
| .get(app.selected_order_chat_idx) | ||
| .map(|row| row.order_id.clone()); | ||
| let list_len = order_id | ||
| .as_ref() | ||
| .map(|id| get_order_attachment_messages(app, id).len()) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
Pin the popup to an order_id, not the current sidebar index.
This mode recomputes the active order from a fresh active_order_chat_list_snapshot() both when opening and when saving. If a background update reorders My Trades while the popup is open, the same selected_order_chat_idx can resolve to a different order and save the wrong attachment.
Store the originating order_id in UiMode::UserSaveAttachmentPopup and use that frozen id for render/save lookup instead of re-reading the live snapshot.
Also applies to: 626-647, 779-787
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/key_handler/mod.rs` around lines 592 - 600, The popup currently
re-resolves the target order from active_order_chat_list_snapshot using
app.selected_order_chat_idx which can change; modify the
UiMode::UserSaveAttachmentPopup variant to carry the frozen order_id (e.g.,
UiMode::UserSaveAttachmentPopup(order_id: OrderId)) and update the code paths
that open the popup to store the originating order id; then change all places
that call active_order_chat_list_snapshot(...).get(app.selected_order_chat_idx)
and any subsequent get_order_attachment_messages(app, id) (including the blocks
around the current UserSaveAttachmentPopup handling and the other two spots
mentioned) to use the stored order_id from the UiMode instead of re-reading the
live snapshot so rendering and save operations always use the pinned order id.
1 participant
Summary
image_encrypted/file_encryptedmessages from relay sync, show yellow attachment lines + file count in chat title + receive toast, and save via Ctrl+S (UserSaveAttachmentPopup) with Blossom download and shared-key decrypt (order_chat_decryption_key_bytes).ScrollViewwith full content height and always-visible scrollbar (same pattern as admin/observer), auto-scroll on new messages / order switch / send.sender_pubkey == local_trade_pubkeyand dedupe by(timestamp, content)so optimistic You sends are not mirrored as Peer (matches admin chat + Mostro Mobile behavior).MESSAGE_FLOW_AND_PROTOCOL.md,TUI_INTERFACE.md,STARTUP_AND_CONFIG.md,DATABASE.md, andREADME.md.Summary by CodeRabbit
Documentation
New Features