feat(ui): auto-submit AddInvoice on saved-LN YES - created enum for settings options#62
Conversation
- YES on ConfirmSavedLnAddressForInvoice: submit_add_invoice + WaitingAddInvoice - NO unchanged: manual invoice popup via apply_saved_ln_address_invoice_choice - Shared submit_add_invoice for popup Enter and confirm paths (incl. y key) - Settings tab Enter: SettingsMenuAction + settings_action_for_index - Docs: auto-submit flow, MESSAGE_FLOW, TUI, buy/sell specs, SETTINGS_ANALYSIS Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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 (8)
WalkthroughThis PR adds a saved Lightning-address confirmation + optional auto-submit path for AddInvoice, extracts invoice submission into ChangesAddInvoice Saved-Address + SettingsMenuAction Refactor
Sequence DiagramssequenceDiagram
actor User
participant UI as EnterHandler/UI
participant Settings as Settings/Disk
participant State as AppState
participant Invoice as InvoiceHandler
User->>UI: Press Enter on ConfirmSavedLnAddressForInvoice (YES)
activate UI
UI->>Settings: load_settings_from_disk()
activate Settings
Settings-->>UI: saved_ln_address
deactivate Settings
UI->>UI: trim & validate address
UI->>State: set buyer_invoice_preference = UseSavedLnAddress
activate State
State-->>UI: preference recorded
deactivate State
UI->>Invoice: submit_add_invoice(order_id, trimmed_address)
activate Invoice
Invoice->>Invoice: validate, set WaitingAddInvoice, spawn async task
Invoice-->>UI: report async result
deactivate Invoice
UI-->>User: show WaitingAddInvoice
deactivate UI
sequenceDiagram
actor User
participant EnterHandler as Enter Handler
participant SettingsTab as Settings Tab
participant Handler as Action Handler
User->>EnterHandler: Press Enter on Settings Menu
activate EnterHandler
EnterHandler->>SettingsTab: settings_action_for_index(user_role, idx)
activate SettingsTab
SettingsTab-->>EnterHandler: SettingsMenuAction
deactivate SettingsTab
alt SwitchMode
EnterHandler->>Handler: handle_mode_switch()
else SetBuyerLnAddress
EnterHandler->>Handler: create_key_input_state(AddLnAddress)
else ClearBuyerLnAddress
EnterHandler->>Handler: transition to ConfirmClearLnAddress(true)
else ViewSeedWords
EnterHandler->>Handler: spawn_seed_words_task()
else other
EnterHandler->>Handler: dispatch relevant handler
end
Handler-->>User: action executed
deactivate EnterHandler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/ui/key_handler/confirmation.rs (1)
187-209: ⚡ Quick winKeep this confirmation on the Enter/Esc path only.
This branch duplicates the
ConfirmSavedLnAddressForInvoiceflow already handled inhandle_enter_keyand reintroducesy-shortcut behavior for a new confirmation mode. Removing the special case here keeps the flow single-sourced and aligned with the repo’s confirmation model.Based on learnings: In this repo’s Rust TUI, confirmation dialogs are intentionally simplified: remove/ignore
y/nshortcut handling for confirm/cancel and support onlyEnter(confirm) andEsc(cancel).🤖 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/confirmation.rs` around lines 187 - 209, This branch handling UiMode::ConfirmSavedLnAddressForInvoice duplicates the Enter/Esc confirmation flow and reintroduces 'y' shortcut behavior; remove this special-case branch from the current key handler (or change it to no-op/return false) so that ConfirmSavedLnAddressForInvoice is only handled by the centralized confirmation logic (e.g., handle_enter_key or the existing Enter/Esc path) and avoid directly calling app.buyer_invoice_preference.insert(...) or submit_add_invoice(...) here.src/ui/tabs/settings_tab.rs (1)
23-50: ⚡ Quick winUse one source of truth for settings rows.
settings_action_for_index, the per-role label lists, and the manual*_SETTINGS_OPTIONS_COUNTconstants all encode the same menu separately. The next row insertion/removal can desync what the user sees from what Enter dispatches. Consider defining per-role static arrays of(SettingsMenuAction, label)and deriving both rendering and counts from them.Also applies to: 52-56, 107-132
🤖 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/settings_tab.rs` around lines 23 - 50, The code currently duplicates the settings menu in multiple places (settings_action_for_index, per-role label lists, and *_SETTINGS_OPTIONS_COUNT), risking desync; refactor by introducing two per-role static arrays (e.g., ADMIN_SETTINGS and USER_SETTINGS) of tuples (SettingsMenuAction, &'static str) and use them as the single source of truth: change settings_action_for_index to look up the tuple at idx in the appropriate array and return the action, derive the label lists from the same arrays when rendering, and compute the *_SETTINGS_OPTIONS_COUNT constants from array.len(); also update the other affected locations (the label generation around lines 52-56 and the menu handling around 107-132) to reference these arrays instead of hard-coded enums/strings.
🤖 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/key_handler/enter_handlers.rs`:
- Around line 423-430: The code currently collapses load_settings_from_disk()
errors into an empty address by calling .ok().unwrap_or_default(), so replace
that flow: call load_settings_from_disk() and match on the Result (or use
?/map_err) to surface load errors—if Err, set app.mode =
UiMode::OperationResult(OperationResult::Error(format!("Failed to load settings:
{}", err))) and return/stop further processing; if Ok(s), let trimmed =
s.ln_address.trim().to_string() and then check trimmed.is_empty() to set the "No
saved Lightning address in settings" error as before. Ensure you update the code
around the trimmed variable and the existing UiMode::OperationResult handling to
use the real load error when present.
---
Nitpick comments:
In `@src/ui/key_handler/confirmation.rs`:
- Around line 187-209: This branch handling
UiMode::ConfirmSavedLnAddressForInvoice duplicates the Enter/Esc confirmation
flow and reintroduces 'y' shortcut behavior; remove this special-case branch
from the current key handler (or change it to no-op/return false) so that
ConfirmSavedLnAddressForInvoice is only handled by the centralized confirmation
logic (e.g., handle_enter_key or the existing Enter/Esc path) and avoid directly
calling app.buyer_invoice_preference.insert(...) or submit_add_invoice(...)
here.
In `@src/ui/tabs/settings_tab.rs`:
- Around line 23-50: The code currently duplicates the settings menu in multiple
places (settings_action_for_index, per-role label lists, and
*_SETTINGS_OPTIONS_COUNT), risking desync; refactor by introducing two per-role
static arrays (e.g., ADMIN_SETTINGS and USER_SETTINGS) of tuples
(SettingsMenuAction, &'static str) and use them as the single source of truth:
change settings_action_for_index to look up the tuple at idx in the appropriate
array and return the action, derive the label lists from the same arrays when
rendering, and compute the *_SETTINGS_OPTIONS_COUNT constants from array.len();
also update the other affected locations (the label generation around lines
52-56 and the menu handling around 107-132) to reference these arrays instead of
hard-coded enums/strings.
🪄 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: af2a8e92-be94-4130-9cf4-60c7cf49c327
📒 Files selected for processing (10)
docs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/README.mddocs/SETTINGS_ANALYSIS.mddocs/TUI_INTERFACE.mddocs/buy order flow.mddocs/sell order flow.mdsrc/ui/key_handler/confirmation.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/message_handlers.rssrc/ui/tabs/settings_tab.rs
arkanoider
changed the title
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this one strictly and I do see a blocker in the current implementation.
The new Settings-menu enum cleanup looks fine, but I am not comfortable approving the saved-LN auto-submit path as it stands.
Blocker
When ConfirmSavedLnAddressForInvoice takes the YES path, the code now writes:
buyer_invoice_preference[order_id] = UseSavedLnAddress- then immediately calls
submit_add_invoice(...)
If submit_add_invoice later fails, that per-order preference remains set.
That matters because present_add_invoice_popup() uses this preference to skip the confirmation step on future AddInvoice openings for the same order and jumps straight to the saved-address path. So after one failed auto-submit, reopening the flow can silently auto-select the saved address again instead of showing the confirmation the user just came from.
In other words, the new preference is persisted before the send outcome is known, and on failure it can leave the UI in a sticky state that no longer matches the user’s last successful action.
I think the preference should only be committed after a successful send, or it should be rolled back on the error path.
Because this changes user-facing behavior in the invoice flow, I am marking this as request changes rather than a nit.
|
I re-checked the latest push and I still see the same blocker.
So if the auto-submit fails, the per-order preference is still left behind and The settings-menu enum refactor looks cleaner now, but the invoice-flow state bug is still present in the current head, so I cannot switch this to approval yet. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/TUI_INTERFACE.md`:
- Line 234: Update the TUI_INTERFACE.md text for the
ConfirmSavedLnAddressForInvoice flow to remove the stale "n" shortcut: replace
any mention that "NO (Enter or `n`)" selects the manual invoice UI with wording
that NO is selected by choosing NO and pressing Enter (and note that Esc
cancels), and ensure the description still references AddInvoice /
WaitingBuyerInvoice mapping, the UiMode::ConfirmSavedLnAddressForInvoice prompt,
auto-submit via submit_add_invoice when YES+Enter is chosen, and that
BuyerInvoicePreference per order_id is remembered until cancelled or the order
row is torn down.
🪄 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: e444a06f-7c91-492d-b52e-1566ed2a6b1f
📒 Files selected for processing (7)
docs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/README.mddocs/SETTINGS_ANALYSIS.mddocs/TUI_INTERFACE.mdsrc/ui/key_handler/confirmation.rssrc/ui/key_handler/enter_handlers.rssrc/ui/tabs/settings_tab.rs
💤 Files with no reviewable changes (1)
- src/ui/key_handler/confirmation.rs
There was a problem hiding this comment.
Re-checking the latest push: the blocker I raised is fixed now.
What changed correctly:
ConfirmSavedLnAddressForInvoiceno longer storesUseSavedLnAddressbefore the send outcome is known- the saved-address preference is now recorded only on the successful
InvoiceSubmittedpath - on failure, the preference is not left behind, so the flow will not silently skip the confirmation on the next reopen for that same order
I also re-checked the surrounding pieces:
- the NO path still keeps the manual invoice flow
- the settings-menu enum refactor is cleaner and removes the fragile raw-index dispatch
- the new
InvoiceSubmittednormalization inorder_ch_mngis coherent with the existing result-handling flow
So from the code side, I am good with this now.
Note: at the time of this re-review, some GitHub build jobs were still in progress on commit c24cd403bd39ea30ec7a33baa811d1e8ace4c718, so this approval is based on the code diff and the checks that had already completed successfully.
1 participant
Summary
ConfirmSavedLnAddressForInvoice): YES (Enter ory) now submitsAddInvoiceimmediately usingSettings.ln_addressvia sharedsubmit_add_invoice(message_handlers.rs), UI goes toWaitingAddInvoice— no second step on the invoice input popup.ManualInvoice+NewMessageNotificationwith empty field (apply_saved_ln_address_invoice_choice).SettingsMenuAction+settings_action_for_indexinsettings_tab.rs;enter_handlersdispatches by enum instead of raw row indices.Docs
Updated SETTINGS_ANALYSIS, MESSAGE_FLOW_AND_PROTOCOL, TUI_INTERFACE, buy/sell flow specs, docs/README index line.
Summary by CodeRabbit
New Features
Documentation