feat(ui): confirm saved Lightning address before AddInvoice - step 3#61
Merged
Conversation
- Track BuyerInvoicePreference per order_id; UiMode::ConfirmSavedLnAddressForInvoice - Route AddInvoice through present_add_invoice_popup / apply_saved_ln_address_invoice_choice - Clear preference on invoice-popup cancel and closed-trade cleanup (order_ch_mng) - Draw saved-address confirm with wrapped body (admin_key_confirm, load_settings_from_disk) - Docs: settings analysis, message flow, TUI, buy/sell flow specs Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR implements an optional pre-confirmation flow for invoice entry when a saved Lightning address is configured in settings. Users see a confirmation popup asking whether to use the saved address or enter manually; the choice is cached per order and cleared when trades are canceled or removed. ChangesSaved Lightning Address Invoice Confirmation Flow
Sequence DiagramsequenceDiagram
participant User
participant UIKeyHandler as UI Key Handler
participant NotificationMgr as Notification Manager
participant AppState
participant SettingsDisk as Settings Disk
participant UIRender as UI Renderer
User->>UIKeyHandler: Press Enter on AddInvoice
UIKeyHandler->>NotificationMgr: present_add_invoice_popup()
NotificationMgr->>SettingsDisk: load_settings_from_disk()
SettingsDisk-->>NotificationMgr: ln_address (trimmed)
NotificationMgr->>AppState: Check buyer_invoice_preference[order_id]
alt Preference cached
NotificationMgr->>AppState: Set mode to NewMessageNotification<br/>(with prefilled or empty address)
AppState-->>UIKeyHandler: UiMode updated
else Preference not cached & address exists
NotificationMgr-->>UIKeyHandler: Return ConfirmSavedLnAddressForInvoice mode
UIKeyHandler->>AppState: Set mode
AppState-->>UIRender: Render confirmation popup
UIRender-->>User: Show "Use saved Lightning address?" popup
User->>UIKeyHandler: Press Y or N
UIKeyHandler->>NotificationMgr: apply_saved_ln_address_invoice_choice()
NotificationMgr->>AppState: Update buyer_invoice_preference[order_id]<br/>Set mode to NewMessageNotification<br/>(with appropriate invoice input)
AppState-->>UIRender: Render AddInvoice popup
UIRender-->>User: Show invoice input popup
else No saved address
NotificationMgr->>AppState: Set mode to NewMessageNotification<br/>(empty invoice input)
AppState-->>UIRender: Render AddInvoice popup
UIRender-->>User: Show invoice input popup
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
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 (1)
src/ui/key_handler/mod.rs (1)
912-937:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLeft/Right should set explicit YES/NO, not toggle.
In this branch, both Left and Right currently flip
selected_button, so pressing Left can switch from YES to NO. That conflicts with the deterministic behavior implemented elsewhere and makes selection unpredictable for this new confirmation mode too.Suggested fix
- | UiMode::ConfirmExit(ref mut selected_button) => { - *selected_button = !*selected_button; // Toggle between YES and NO + | UiMode::ConfirmExit(ref mut selected_button) => { + *selected_button = code == KeyCode::Left; // Left=YES, Right=NO return Some(true); }🤖 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 912 - 937, The Left/Right branch currently toggles selected_button for many confirmation UiMode variants, making Left sometimes switch YES->NO; change the handler for KeyCode::Left | KeyCode::Right to set selected_button explicitly based on which key was pressed (e.g., KeyCode::Left => *selected_button = false, KeyCode::Right => *selected_button = true) instead of flipping it, keeping the AddSolver permission toggle branch unchanged; apply this change to the match arm that touches UiMode::AdminMode(.. ConfirmAddSolver ..) and all the listed confirmation variants that bind ref mut selected_button and still return Some(true).
🧹 Nitpick comments (3)
src/ui/admin_key_confirm.rs (1)
274-367: ⚡ Quick winButton + help-text rendering (~90 lines) is duplicated from
render_admin_key_confirm_with_message.The YES/NO button layout (style computation,
Block/yes_inner/no_innersplits, centredParagraph) and the help-textParagraphat the bottom are character-for-character copies of lines ~114–211 in the same file. Any future change to key hints, colours, or button sizing must be made in two places.Consider extracting private helpers, e.g.:
fn render_yes_no_buttons(f: &mut ratatui::Frame, button_area: Rect, selected: bool) { … } fn render_confirmation_help_line(f: &mut ratatui::Frame, area: Rect) { … }Both
render_admin_key_confirm_with_messageandrender_saved_ln_address_invoice_confirmcan then delegate to them.🤖 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/admin_key_confirm.rs` around lines 274 - 367, The YES/NO button rendering and bottom help-text are duplicated; extract them into two private helper functions and call them from both render_admin_key_confirm_with_message and render_saved_ln_address_invoice_confirm: implement fn render_yes_no_buttons(f: &mut ratatui::Frame, button_area: Rect, selected: bool) to contain the yes_style/no_style logic, Block creation, yes_inner/no_inner splits and centered Paragraphs that use selected_button (rename parameter to selected), and implement fn render_confirmation_help_line(f: &mut ratatui::Frame, area: Rect) to render the final help Paragraph (the long Span chain using PRIMARY_COLOR); replace the duplicated code in the original functions with calls to these helpers so styling, sizing and key hints are maintained in one place.src/ui/draw.rs (1)
279-284: 🏗️ Heavy liftAvoid disk reads in the render loop for this popup.
load_settings_from_disk()inui_drawruns every frame while the modal is open. Consider resolving/cachingln_addresswhen enteringUiMode::ConfirmSavedLnAddressForInvoiceand render the cached value here.🤖 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/draw.rs` around lines 279 - 284, The render loop is calling load_settings_from_disk() inside the UiMode::ConfirmSavedLnAddressForInvoice branch which causes a disk read every frame; instead capture and cache the ln_address when you transition into UiMode::ConfirmSavedLnAddressForInvoice and render that cached value. Update the code that sets app.mode to pass the resolved ln_address (or populate a new App field like cached_ln_address) so UiMode::ConfirmSavedLnAddressForInvoice carries the string (or reads from app.cached_ln_address), remove load_settings_from_disk() from draw.rs, and have the draw code use the cached ln_address (referencing UiMode::ConfirmSavedLnAddressForInvoice, load_settings_from_disk, ln_address, and app.mode to locate the changes).src/util/dm_utils/notifications_ch_mng.rs (1)
176-190: ⚡ Quick winExtract a shared PayInvoice state builder to avoid drift across files.
Line 180 rebuilds the PayInvoice
InvoiceInputStateliteral that is also built insrc/ui/key_handler/enter_handlers.rs(Lines 941-948). A shared helper will keep defaults (focused,scroll_y, selection) synchronized.♻️ Proposed refactor
+fn invoice_state_for_pay_invoice() -> InvoiceInputState { + InvoiceInputState { + invoice_input: String::new(), + focused: false, + just_pasted: false, + copied_to_clipboard: false, + scroll_y: 0, + action_selection: InvoiceNotificationActionSelection::Primary, + } +} ... - let invoice_state = InvoiceInputState { - invoice_input: String::new(), - focused: false, - just_pasted: false, - copied_to_clipboard: false, - scroll_y: 0, - action_selection: InvoiceNotificationActionSelection::Primary, - }; + let invoice_state = invoice_state_for_pay_invoice();🤖 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/dm_utils/notifications_ch_mng.rs` around lines 176 - 190, Extract a shared constructor for the PayInvoice InvoiceInputState to avoid duplicated literals: add a helper function (e.g., new_pay_invoice_state or build_default_invoice_input_state) that returns InvoiceInputState with invoice_input: String::new(), focused: false, just_pasted: false, copied_to_clipboard: false, scroll_y: 0, and action_selection: InvoiceNotificationActionSelection::Primary; replace the inline literal in notifications_ch_mng.rs where UiMode::NewMessageNotification(...) is created and the literal in src/ui/key_handler/enter_handlers.rs with calls to this helper so both places use the same centralized builder (keep the helper visible to both modules or move it into a shared util module).
🤖 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 `@src/ui/admin_key_confirm.rs`:
- Around line 231-241: The popup layout currently uses Constraint::Min(7) in the
Layout::new constraint array which expands to fill all popup rows and causes
chunk[4] (help text) to overwrite the bottom border; change that
Constraint::Min(7) to Constraint::Length(9) in the Layout::new call so the
constraints sum to less than the popup height and leave the bottom border row
unallocated (this is analogous to how render_admin_key_confirm_with_message uses
fixed Length constraints).
---
Outside diff comments:
In `@src/ui/key_handler/mod.rs`:
- Around line 912-937: The Left/Right branch currently toggles selected_button
for many confirmation UiMode variants, making Left sometimes switch YES->NO;
change the handler for KeyCode::Left | KeyCode::Right to set selected_button
explicitly based on which key was pressed (e.g., KeyCode::Left =>
*selected_button = false, KeyCode::Right => *selected_button = true) instead of
flipping it, keeping the AddSolver permission toggle branch unchanged; apply
this change to the match arm that touches UiMode::AdminMode(.. ConfirmAddSolver
..) and all the listed confirmation variants that bind ref mut selected_button
and still return Some(true).
---
Nitpick comments:
In `@src/ui/admin_key_confirm.rs`:
- Around line 274-367: The YES/NO button rendering and bottom help-text are
duplicated; extract them into two private helper functions and call them from
both render_admin_key_confirm_with_message and
render_saved_ln_address_invoice_confirm: implement fn render_yes_no_buttons(f:
&mut ratatui::Frame, button_area: Rect, selected: bool) to contain the
yes_style/no_style logic, Block creation, yes_inner/no_inner splits and centered
Paragraphs that use selected_button (rename parameter to selected), and
implement fn render_confirmation_help_line(f: &mut ratatui::Frame, area: Rect)
to render the final help Paragraph (the long Span chain using PRIMARY_COLOR);
replace the duplicated code in the original functions with calls to these
helpers so styling, sizing and key hints are maintained in one place.
In `@src/ui/draw.rs`:
- Around line 279-284: The render loop is calling load_settings_from_disk()
inside the UiMode::ConfirmSavedLnAddressForInvoice branch which causes a disk
read every frame; instead capture and cache the ln_address when you transition
into UiMode::ConfirmSavedLnAddressForInvoice and render that cached value.
Update the code that sets app.mode to pass the resolved ln_address (or populate
a new App field like cached_ln_address) so
UiMode::ConfirmSavedLnAddressForInvoice carries the string (or reads from
app.cached_ln_address), remove load_settings_from_disk() from draw.rs, and have
the draw code use the cached ln_address (referencing
UiMode::ConfirmSavedLnAddressForInvoice, load_settings_from_disk, ln_address,
and app.mode to locate the changes).
In `@src/util/dm_utils/notifications_ch_mng.rs`:
- Around line 176-190: Extract a shared constructor for the PayInvoice
InvoiceInputState to avoid duplicated literals: add a helper function (e.g.,
new_pay_invoice_state or build_default_invoice_input_state) that returns
InvoiceInputState with invoice_input: String::new(), focused: false,
just_pasted: false, copied_to_clipboard: false, scroll_y: 0, and
action_selection: InvoiceNotificationActionSelection::Primary; replace the
inline literal in notifications_ch_mng.rs where
UiMode::NewMessageNotification(...) is created and the literal in
src/ui/key_handler/enter_handlers.rs with calls to this helper so both places
use the same centralized builder (keep the helper visible to both modules or
move it into a shared util module).
🪄 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: e43c1f3d-e46d-44d6-95d6-02f4bd4e5dda
📒 Files selected for processing (21)
docs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/README.mddocs/SETTINGS_ANALYSIS.mddocs/TUI_INTERFACE.mddocs/buy order flow.mddocs/sell order flow.mdsrc/ui/admin_key_confirm.rssrc/ui/app_state.rssrc/ui/draw.rssrc/ui/key_handler/confirmation.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/esc_handlers.rssrc/ui/key_handler/message_handlers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/navigation.rssrc/ui/mod.rssrc/ui/orders.rssrc/ui/state.rssrc/util/dm_utils/mod.rssrc/util/dm_utils/notifications_ch_mng.rssrc/util/dm_utils/order_ch_mng.rs
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I reviewed this carefully and I do not see a blocker in the current diff.
What I checked:
- the new confirmation step before AddInvoice when a saved
ln_addressexists - the per-order in-memory preference (
UseSavedLnAddressvsManualInvoice) and where it gets cleared - Enter /
y/Esc/ Left-Right handling for the new confirm modal - the AddInvoice notification path, both when auto-opened from an incoming DM and when reopened from the Messages tab
- the actual AddInvoice execution path, to make sure it still verifies Lightning addresses before sending the DM
The behavior looks coherent to me:
- we no longer silently assume the saved address should be used
- saying No preserves the existing manual BOLT11 / Lightning-address path
- saying Yes pre-fills from Settings, but verification still happens later in
execute_add_invoice, so the network-dependent validation stays in the send path - the preference is scoped per order and gets dropped on cancel / trade teardown, which avoids stale carry-over into later trades
Checks are green too, so this looks good from my side. Nice UX step, and it keeps the safety properties added in the previous LN-address work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When the user has a saved buyer
ln_addressin settings and receives anAddInvoicenotification (sell-side taker flow), Mostrix now asks whether to use that saved address as the invoice target before opening the manual invoice popup. Choosing no keeps the existing BOLT11 / Lightning-address flow.Motivation
Avoid silently assuming the saved address matches what the user wants for this trade, while still making the happy path one confirmation away when settings already hold a verified address.
Changes
BuyerInvoicePreferencekeyed byorder_idinAppState(UseSavedLnAddressvsManualInvoice).UiMode::ConfirmSavedLnAddressForInvoicewith YES/NO handling wired through enter / esc / navigation / confirmation paths.present_add_invoice_popup/apply_saved_ln_address_invoice_choiceinnotifications_ch_mng(narrowHashMapborrow to avoid conflicting with Messages-tab mutex use).order_ch_mng).render_saved_ln_address_invoice_confirminadmin_key_confirm).SETTINGS_ANALYSIS,MESSAGE_FLOW_AND_PROTOCOL,TUI_INTERFACE, buy/sell flow specs,docs/READMEindex line.Summary by CodeRabbit
New Features
Documentation