fix(order): the take screen keeps Taking… while its own take settles - #456
fix(order): the take screen keeps Taking… while its own take settles#456Catrya wants to merge 1 commit into
Conversation
After tapping Take order, the button switched to No longer available for a few seconds before the app moved on to the trade (#454). Rust updates the order's book entry with the trade's status as soon as the daemon confirms the take, then persists the trade, subscribes to the order and installs the session, and only then does take_order return. The screen's build read that change as the order leaving the pending book, although its own listener already held back while a take was in flight. The build now does the same: while the take is loading, the button stays on Taking… whatever the book says. Once the take settles, nothing else changes. A success navigates, OrderAlreadyTaken marks the order unavailable, and any other failure lands back on idle, where a book that has moved on shows No longer available, which is true then.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Actionable comments posted: 3
The diagnosis is right and the new rule is the right one: take_order writes the daemon's status into the book entry before persist_confirmed_take, subscribe_single_order and install_session, so the in-flight take was reading its own book write as the order going away. The rule is not enforced on every path, though. The same "No longer available" still reaches the user after a successful take and when the order expires mid-take. Both are verified below, with a fix that keeps all 19 tests in this file green.
🧹 Verified-OK notes (4)
- Rust claims in the description — confirmed on
2724772: the book entry takes the reply's status (order_book().upsert_order(info)) before persistence, the per-order subscription and the session install; and!waiting_bondskips that write, so a take parked atWaitingTakerBondkeeps its entrypending. ✅- Failure path —
finallyreturns_ctatoidleafter a non-OrderAlreadyTakenfailure, and the book decides again; the new second test pins it. ✅- CI green on the merge ref (Flutter, Rust, Web smoke);
mergeable: MERGEABLE; no commit onmainsince this branch's base touches either file. ✅- Overlap with #448 — both edit
take_order_screen_test.dart. The tests here sit inside an existing group, as the description says, but #448 replaces_pump'sMaterialAppwith a router harness, so expect a small conflict in the harness whichever lands second.
📜 Review details
Reviewed commit: 2724772
Files reviewed: 2 — lib/features/order/screens/take_order_screen.dart, test/features/order/screens/take_order_screen_test.dart
Profile: strict
How the findings were verified (throwaway worktree on 2724772, not pushed). A probe test pumps TakeOrderScreen under a GoRouter, as a seller so the success path makes no Rust settings call. The book is fed the way the real flow feeds it.
| Probe | This PR | With the suggested fix |
|---|---|---|
Take succeeds after the book moved to waitingPayment: frames showing No longer available while TakeOrderScreen is still mounted |
19 frames (~300 ms), then lands on the pay step | 0 frames, lands on the pay step |
Order's expiresAt passes while the take is in flight |
Taking… → No longer available | stays Taking… |
take_order_screen_test.dart (19 tests, including this PR's two) |
pass | pass |
| _cta == TakeOrderCta.unavailable || | ||
| live == null || | ||
| live.status != OrderStatus.pending; | ||
| (_cta != TakeOrderCta.loading && |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
A successful take still shows "No longer available", now during the navigation instead of before it.
The hold is keyed on _cta == TakeOrderCta.loading, but _onTakeOrder's finally still runs after the navigation:
context.go(AppRoute.tradeDetailPath(widget.orderId));
context.push(AppRoute.payInvoicePath(widget.orderId));
} catch (e) { … } finally {
if (mounted && _cta == TakeOrderCta.loading) {
setState(() => _cta = TakeOrderCta.idle); // still mounted: the route is animating out
}
}context.go doesn't unmount this screen synchronously; it stays in the tree while the next route animates in. The finally puts _cta back on idle, so this expression lets the book decide again. The book already reads waitingPayment/waitingBuyerInvoice from the user's own take, so the outgoing page renders No longer available, and the countdown switches to closed as well since isClosed: isUnavailable.
Reproduced on this commit: the book moves to waitingPayment, the take then succeeds, and 19 consecutive 16 ms frames show No longer available with TakeOrderScreen still mounted before the app lands on the pay step. That is #454's symptom, shortened but not gone. The manual test in the description can easily miss it under the transition.
Suggested fix: once the take succeeded and the screen is navigating away, don't fall back to idle.
setState(() => _cta = TakeOrderCta.loading);
+ var navigated = false;
try {
final trade = await ref.read(takeOrderActionProvider)(…);
if (!mounted) return;
…
if (trade.order.status == OrderStatus.waitingTakerBond) {
+ navigated = true;
context.go(AppRoute.tradeDetailPath(widget.orderId));
context.push(AppRoute.payBondPath(widget.orderId));
} else if (widget.isBuying) {
final settings = await settings_api.getSettings();
if (!mounted) return;
+ navigated = true;
context.go(AppRoute.tradeDetailPath(widget.orderId));
…
} else {
+ navigated = true;
context.go(AppRoute.tradeDetailPath(widget.orderId));
context.push(AppRoute.payInvoicePath(widget.orderId));
}
} catch (e) { … } finally {
- if (mounted && _cta == TakeOrderCta.loading) {
+ if (mounted && !navigated && _cta == TakeOrderCta.loading) {
setState(() => _cta = TakeOrderCta.idle);
}
}Set the flag after the buyer branch's await settings_api.getSettings(), as above. If that call throws, the screen never navigates and must still return to idle, or the button is stuck on Taking…. The variant I ran set the flag once before the branches; the success/expiry probes and all 19 tests in this file pass with it, and moving it past the only await in the buyer branch doesn't change the seller or bond paths they exercise.
🤖 Prompt for AI Agents
In lib/features/order/screens/take_order_screen.dart, _onTakeOrder: add `var navigated = false;`
before the try; set `navigated = true;` immediately before the first context.go(...) of each
success branch (in the buyer branch, after `await settings_api.getSettings()` and its mounted
check); in `finally`, only reset `_cta` to idle when `!navigated`. Add a routed widget test where
the book moves the order to waitingPayment, the take then succeeds (seller path), and assert that
"No longer available" is never found while TakeOrderScreen is still in the tree.
| live.status != OrderStatus.pending; | ||
| (_cta != TakeOrderCta.loading && | ||
| (live == null || live.status != OrderStatus.pending)); | ||
| final cta = isUnavailable ? TakeOrderCta.unavailable : _cta; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
The countdown can still override an in-flight take.
This line trusts _cta == TakeOrderCta.unavailable, and _syncCountdown sets exactly that when expiresAt passes, without checking for a take in flight:
if (left <= Duration.zero) {
_remaining.value = Duration.zero;
if (_cta != TakeOrderCta.unavailable) { // true while loading
setState(() => _cta = TakeOrderCta.unavailable);
}An order whose expiry falls inside the few seconds take_order takes flips from Taking… to No longer available mid-take (reproduced on this commit). The finally then leaves it there, since _cta is no longer loading. If the daemon accepted the take, the user is told the order is gone right before being navigated to it: the case this PR sets out to remove, on another path.
Suggested fix: the countdown only retires an idle button; a take in flight is settled by the daemon's answer.
- if (_cta != TakeOrderCta.unavailable) {
+ if (_cta == TakeOrderCta.idle) {
setState(() => _cta = TakeOrderCta.unavailable);
}Verified: with this change the expiry probe keeps Taking…, and the existing expiry tests still pass. After a failed take, finally returns to idle and this expression reads the expired book entry, so the button still dies in place.
| await tester.pump(); | ||
|
|
||
| expect(find.text('Taking…'), findsOneWidget); | ||
| expect(find.text('No longer available'), findsNothing); |
There was a problem hiding this comment.
🧪 Test Coverage | 🟡 Minor
Neither new test lets the take succeed after the book moved, which is where the symptom still shows.
The first test leaves the take unanswered, so it only proves the hold while loading. The second covers the failure path. The success path, with the book already moved, the take resolving and finally running while the screen is still mounted, is untested, and so is expiry during the take. Both paths still regress (see the comments above).
Suggested additions (both fail on this commit and pass with the suggested fixes):
testWidgets('a take that succeeds never reads unavailable on the way out', (
tester,
) async {
// Seller path under a router, so the navigation and the exit frames are real.
// Book → waitingPayment, then the take resolves successfully.
// Pump ~60 × 16 ms and fail if "No longer available" is found while
// `find.byType(TakeOrderScreen)` is still non-empty; end on the pay step.
});
testWidgets('an order expiring mid-take keeps Taking…', (tester) async {
var now = kFakeNow;
await withClock(Clock(() => now), () async {
// expiresAt: kFakeNow + 2 s; tap Take with the reply left pending;
// advance `now` past expiry and pump 3 s; expect 'Taking…', not
// 'No longer available'.
});
});The first needs the router harness #448 introduces, or a local one. Coordinate with that PR to avoid a second harness.
2 participants
Closes #454.
After tapping Take order, the button switched from Taking… to "No longer available" for a few seconds before the app moved on to the trade. The take itself was fine, but the screen told the user the order was gone right when they got it.
Cause
take_order(Rust) updates the order's book entry with the trade's status as soon as the daemon confirms the take. It then persists the trade, subscribes to the order and installs the session, and only after that does it return. The take screen'sbuildread that book change as the order leaving the pending book:Its own
ref.listenjust above already held back while a take was in flight (_cta != TakeOrderCta.loading), but this expression overrode it. The behaviour came with the take screen redesign (35c1aaf).Change
While the take is loading, the button stays on Taking… whatever the book says:
Once the take settles, nothing else changes:
OrderAlreadyTaken: marks the order unavailable, as before.idle, where a book that has moved on shows No longer available, which is true then.A take that parks at
WaitingTakerBondnever hit this: Rust leaves its book entrypendingduring the bond window.Tests
In
take_order_screen_test.dart,TakeOrderScreen takinggroup:waitingBuyerInvoicewhile the take is pending, and the button still reads Taking…. This test fails without the change.in-progress, then the take fails withNoDaemonResponse, and the screen shows No longer available. This pins the behaviour kept after a failure.flutter analyzeis clean andflutter testpasses (1114).Manual test
Took two orders against a local mostrod. Each went straight from Taking… to the trade, with no No longer available in between.
Notes
take_order_screen_test.dart(the_pumpharness and a new group at the end of the file). The tests here are added inside an existing group to keep the two apart; whichever lands second may still need a small merge.