fix(order): back from invoice screens lands on trade detail - #278
Conversation
…ancels the trade - After a successful take, trade detail becomes the base of the stack and the invoice screen is pushed on top, so back/close never returns to Take Order offering an already-taken order. - The Cancel buttons on add-invoice and pay-invoice now confirm via dialog and send the protocol cancel (same flow and strings as trade detail), then go home. - Defense in depth: Take Order redirects to trade detail when a persisted trade role shows the user already participates in the order.
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughChangesOrder flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TakeOrderScreen
participant TradeRoleAPI
participant TradeDetailScreen
participant InvoiceScreen
TakeOrderScreen->>TradeRoleAPI: retrieve user trade role
TradeRoleAPI-->>TakeOrderScreen: return role
TakeOrderScreen->>TradeDetailScreen: redirect existing participant
TakeOrderScreen->>TradeDetailScreen: replace route after taking order
TradeDetailScreen->>InvoiceScreen: open add or pay invoice
sequenceDiagram
participant InvoiceScreen
participant OrderCancellationAPI
participant Trades
participant Home
InvoiceScreen->>InvoiceScreen: confirm cancellation
InvoiceScreen->>OrderCancellationAPI: cancel order
OrderCancellationAPI-->>InvoiceScreen: return success or error
InvoiceScreen->>Trades: refresh trades
InvoiceScreen->>Home: navigate after cancellation
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/order/screens/take_order_screen.dart (1)
52-68: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlock Take Order until the participant check completes.
_redirectIfParticipant()starts asynchronously while_onTakeOrder()remains reachable. IfgetTradeRoleis slow, an existing participant can calltakeOrderfirst; the Rust path returnsOrderAlreadyTaken, and this catch block navigates toAppRoute.homeinstead oftradeDetailPath. Track the pending participant lookup and disable the submit button untilgetTradeRolereturns no role. Handle lookup errors explicitly. Add a widget test that delaysgetTradeRoleand asserts thattakeOrderdoes not run.🤖 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 `@lib/features/order/screens/take_order_screen.dart` around lines 52 - 68, Track the in-flight participant lookup started by _redirectIfParticipant and keep the Take Order submit action disabled until getTradeRole completes; if a role exists, navigate to tradeDetailPath and prevent _onTakeOrder from running. Handle lookup failures explicitly without leaving the screen permanently blocked, and add a widget test that delays getTradeRole and verifies takeOrder is not invoked.Source: Coding guidelines
🤖 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 `@lib/features/order/screens/add_lightning_invoice_screen.dart`:
- Around line 71-109: The order screens allow cancellation to race with
submission or another cancellation. In _cancelOrder and the corresponding
pay-invoice cancellation flow, add cancellation busy-state guards that reject
new state-changing requests while submission or cancellation is active, set and
clear the state around the awaited API call, and disable each related Cancel
button while busy; update the specified add-invoice and pay-invoice widget tests
to cover repeated confirmations and cancellation during invoice submission.
---
Outside diff comments:
In `@lib/features/order/screens/take_order_screen.dart`:
- Around line 52-68: Track the in-flight participant lookup started by
_redirectIfParticipant and keep the Take Order submit action disabled until
getTradeRole completes; if a role exists, navigate to tradeDetailPath and
prevent _onTakeOrder from running. Handle lookup failures explicitly without
leaving the screen permanently blocked, and add a widget test that delays
getTradeRole and verifies takeOrder is not invoked.
🪄 Autofix
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 Plus
Run ID: 2a8c9411-25a2-44a8-883d-f7297580a0bc
📒 Files selected for processing (3)
lib/features/order/screens/add_lightning_invoice_screen.dartlib/features/order/screens/pay_lightning_invoice_screen.dartlib/features/order/screens/take_order_screen.dart
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved — reviewed at head 70639844.
Verification
- CI green on this head: Rust (build/test/clippy/wasm), Flutter (analyze/test), Web (wasm build). Dart-only change, so Flutter analyze/test is the meaningful gate and it passes;
git diff --checkclean. - Reviewed against both CodeRabbit round-1 findings: the inline thread is resolved on this head, and the outside-diff take-order race is functionally closed (see below).
Review
- The stack rebuild (
context.go(tradeDetailPath)+context.push(invoicePath)) is the right fix for #268: back/close from either invoice screen now lands on trade detail, and the Take Order screen is gone from the stack entirely. Matches what the issue proposed. - The participant re-check inside
_onTakeOrdercloses CodeRabbit's race: even if theinitStateredirect lookup is still in flight, a participant can no longer dispatch a secondtakeOrder— they are redirected to their trade instead. That was the correct layer to fix it (disabling the button would still leave the window). _cancelOrderon both screens serializes properly (_submitting || _cancelingguards, buttons disabled while busy,finallyresets) and cooperates correctly with the push listener from #274 via the_navigatedone-shot guard. Cancel-from-invoice-screen now sends the protocol cancel instead of a barepop, which also fixes the "form sits here dying with NoDaemonResponse" failure mode.- All dialog/snackbar strings reuse the existing trade-detail l10n keys — no new untranslated strings.
Non-blocking nits
_onTakeOrder: the newgetTradeRoleawait widens the pre-_submittingwindow slightly — a double-tap during the role lookup can still reachtakeOrdertwice (second one getsOrderAlreadyTaken→ home). A one-lineif (_submitting) return;re-check after the await would close it. Pre-existing pattern, not a regression.- Both
getTradeRolecall sites (_redirectIfParticipant,_onTakeOrder) sit outside any try/catch — an FFI/DB error there would surface as an unhandled async error rather than a snackbar. - The cancel dialog copy ("The other party must also agree") is the cooperative-cancel text; in the waiting states these screens live in, the cancel is actually unilateral. Harmless, slightly misleading — fine to leave for a copy pass.
1 participant
fix #268
Summary by CodeRabbit