fix(trades): handle takes left Canceled by the old optimistic cancel

[Catrya]

Closes #434.

Builds before #419 marked a take Canceled as soon as its cancel was published, even when the trade never went active. The daemon then put the order back in the book as pending, but the row stayed. #419 stops writing those rows; this PR fixes how the ones already on disk are shown and handled:

• The trade screen showed the book's public pending as the trade's status. The user saw their own pending order, with a Cancel the daemon refuses (IsNotYourOrder). Since feat(trades,chat): redesign the trades and chat tabs (handoff 11a/11b) #436 the My Trades list keeps an ended row's status, so the list said cancelled while the screen said pending.
• The take screen treated any row on the order as participation. Opening the order from the book, or tapping Take, redirected to that dead trade, so the user could never take the order again. That is Ex-taker cannot retake an order: it disappears from their order book, and a retake leaves duplicate trade rows #417 again, for rows written by the old cancel.

No Rust change: take_order already accepts this case (it only needs the order to be pending in the book), and persist_confirmed_take replaces every earlier row of the order.

Changes

1. fix(trades): a public pending is never a take's status

One rule, shownTradeStatus (trade_state_provider.dart), now decides what the My Trades list and TradeDetailScreen show. The trade row wins over the live status (tradeStatusProvider, which reads the order book first) in two cases:

• The row has ended (the success or cancelled family). Whatever the book says about the order later is no longer this trade. The list already did this; the screen now matches it.
• It is a take and the book says pending. A public pending means nobody holds the order, so it is never a take's status. This also covers a take parked at WaitingTakerBond, whose order is still pending in public.

Every other live status still wins over an open row. In the trade screen the rule applies everywhere the live status was read: the status itself, the post-cancel decision, the cancel dialog's copy, and the haptic nudge on a step change (so a book change the row outranks does not vibrate).

2. fix(order): a take that already ended no longer blocks taking the order

tradeRoleLookupProvider, which only the take screen uses, now answers through participatingRole:

• A take whose row has ended does not count as participation. An order whose trade truly ended never goes back to Pending in mostrod, so once the order can be taken again, such a row can only be a leftover of a take that never went active.
• A maker's row always counts, and so does any row still open.
• It reads every row of the order, not one: a database from before fix(#417): a lost take returns to the ex-taker's book, one trade row per order #419 can hold two rows for the same order.

take_order_screen.dart itself is untouched.

Known trade-off

With the first rule, a cooperative cancel requested on an active trade still marks the row Canceled right away (the remaining optimistic write in apply_local_cancel). If the peer never agrees and the trade completes, both the list and the screen show it as cancelled. The list already behaved this way since #436. The fix belongs to that optimistic write, and it will be tracked separately.

Tests

• trade_state_provider_test.dart:
  • shownTradeStatus over every combination of row status, live status and take/maker;
  • participatingRole and isEndedTake: no row, open takes, ended takes, maker rows, and two rows on the same order.
• trade_rows_provider_test.dart: a take keeps its own status over a public pending; a maker's order still follows the book.
• trade_detail_screen_test.dart:
  • a leftover Canceled take reads cancelled, without Cancel, over a public pending;
  • it stays cancelled when someone else completes the order;
  • an open take keeps its own step over a public pending;
  • a maker's order still reads pending;
  • a book change the row outranks does not trigger the haptic nudge.
• take_order_screen_test.dart: an open take lands on its trade; an ended take leaves the order takeable. The test harness now runs under a router, so the redirect is observable.

With each rule disabled, the tests that cover it fail. flutter analyze is clean and flutter test passes (1042 tests).

Manual test

Against a local mostrod, with the same database for both builds:

1. Pre-fix(#417): a lost take returns to the ex-taker's book, one trade row per order #419 build (40ccde9, in a separate worktree): took and cancelled two orders while they were still waiting. Both rows stayed Canceled (skip replayed Canceled … already Canceled) while the daemon republished the orders as pending.
2. This branch:
   • after a restart, both rows still read Canceled in the database;
   • opening each order from the book showed the take screen, with no redirect to the old trade;
   • both were taken again (fresh trade indexes), cancelled, and wiped as fix(#417): a lost take returns to the ex-taker's book, one trade row per order #419 does;
   • one order was taken a third time, and the earlier take's d-tag task was replaced (superseded by a retake);
   • the old rows were replaced by the new takes, and no IsNotYourOrder was sent from an old key.

Notes

• feat/bond-1b-pay-screen and feat/bond-1c-gate-flip touch take_order_screen.dart and trade_detail_screen.dart in other sections. Whichever lands second may need a small merge.
• Still open, outside this PR:
  • the optimistic Canceled on an active cooperative cancel;
  • cleaning up leftover Canceled rows of takes that never went active.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 8a3bbda3-f555-48ab-b1c9-597287e691c1

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[grunch]

Actionable comments posted: 4

Caution

Merge conflict with main — CI has not run on this PR. mergeable: CONFLICTING. GitHub does not run pull_request workflows on a head it cannot merge, so the only check reported is CodeRabbit (skipped). The PR's flutter analyze / flutter test claims are local-only until this is rebased.

The textual conflict is confined to test/features/order/screens/take_order_screen_test.dart: since this branch's base (68ab52b), main added bond-policy overrides to _pump (mostroNodeProvider, bondEstimateProvider, a node / bondEstimate parameter pair) and new bond-notice tests in the same group this PR extends. Keep both sides: the router harness and trades parameter from here, the node overrides from main. trade_rows_provider.dart, trade_detail_screen.dart, trade_rows_provider_test.dart and fake_trades.dart auto-merge, but please re-run the suites after rebasing — main's rows provider now also reads the claim store, and TradeDetailScreen._cancelOrder gained a maker-bond branch keyed on the status this PR reshapes.

🧹 Nitpick / verified-OK notes (3)

• isTerminalTradeStatus vs the removed _terminal — verified identical (the same 7 statuses), so the My Trades list's ended-row behaviour is unchanged by the refactor. ✅
• Interaction with the bond flow now on main — shownTradeStatus is correct for both bond windows: a taker at WaitingTakerBond whose order is publicly pending keeps the row's status (previously the trade screen showed pending, so this PR also fixes that); a maker at WaitingMakerBond has no book entry, so the live status already falls back to the row, and once the bond locks the book's pending (maker, isTake: false) wins as it should. ✅
• trade_action_listener.dart:59 still calls orders_api.getTradeRole directly — that bridge call is get_trade_by_order_id (LIMIT 1, no ORDER BY), the same "one arbitrary row" lookup participatingRole was written to avoid. Not a regression of this PR (#419 makes duplicate rows unlikely going forward), but worth a follow-up so the two role lookups agree on pre-#419 databases.

📜 Review details

Reviewed commit: c0aa804 (2 commits: 897bbba, c0aa804)
Files reviewed: 9 — trade_state_provider.dart, trade_rows_provider.dart, trade_detail_screen.dart, specs/004…/orders.md, and the 5 test / support files.
Profile: strict

Walkthrough

Area	Change
trade_state_provider.dart	New shownTradeStatus (row vs live), participatingRole / isEndedTake; tradeRoleLookupProvider now scans listTrades() instead of getTradeRole; _isTerminal → public isTerminalTradeStatus.
trade_rows_provider.dart	Drops its private _terminal set and delegates to shownTradeStatus.
trade_detail_screen.dart	Status, cancel-dialog copy, post-cancel decision and haptic nudge all go through _shown(live, row).
take_order_screen_test.dart	Harness moves under a GoRouter so the participant redirect is observable; two new tests.
specs/004…/orders.md	Documents both rules.

[grunch]

🎯 Functional Correctness | 🟠 Major

The leftover take can still flash pending with Cancel while the row is loading — the exact bug this PR fixes.

_status() resolves as soon as tradeStatusProvider has a value, but trade comes from tradeInfoProvider, which awaits rawTradesProvider (a full listTrades() round trip). While that future is pending, _shown(live, null) returns the live status unchanged — the book's public pending.

Nothing else holds the screen in loading during that window: build() gates on _isBuyer(), which resolves from tradeRoleProvider or tradeRoleFromDbProvider (the indexed getTradeRole), independently of rawTradesProvider. For a leftover Canceled take opened cold (from a notification, the chat header, or after a restart), the role typically arrives first, so for one or more frames the screen renders TradeStatus.pending → TradeView offers trade.cancel → a tap publishes the cancel the daemon answers with CantDo(IsNotYourOrder) and the local cancel removes the live order from the book (#434, third bullet).

The same null-row fallback also applies to the haptic listener (L470): a pending → in-progress book change that lands before the row loads still nudges.

The tests don't catch this because _pumpRoutedTradeDetail resolves loadTrades immediately.

Suggested fix: don't let a live pending through until the row is known.

TradeStatus _status() {
  final live = ref.watch(tradeStatusProvider(widget.orderId));
  // …
  if (!live.hasValue) return TradeStatus.loading;
  final tradeAsync = ref.watch(tradeInfoProvider(widget.orderId));
  // A public `pending` may be a take's leftover (#434): only the row can
  // say, so hold `loading` rather than offer a Cancel the daemon refuses.
  if (live.value == OrderStatus.pending && tradeAsync.isLoading) {
    return TradeStatus.loading;
  }
  final status = tradeStatusFromOrderStatus(
    _shown(live.value!, tradeAsync.valueOrNull),
  );
  // …
}

and add a widget test where loadTrades completes after the status stream has emitted pending, asserting _cancelButton() is never found.

🤖 Prompt for AI Agents

In lib/features/trades/screens/trade_detail_screen.dart, _status() (around line 429): when the
live status is OrderStatus.pending and tradeInfoProvider(widget.orderId) is still loading, return
TradeStatus.loading instead of passing a null row to _shown(). Apply the same guard to the haptic
listener (around line 470): skip the nudge while the row is unresolved. Add a widget test in
test/features/trades/trade_detail_screen_test.dart where the status stream emits pending before
loadTrades completes with a Canceled take row, and assert the Cancel button never appears.

[grunch]

🎯 Functional Correctness | 🟠 Major

The role lookup now throws on a storage error; before it returned null. The take screen doesn't handle that.

The old implementation, orders_api.getTradeRole, never failed: get_trade_role logs a DB error and returns Ok(None). orders_api.listTrades() propagates it (db.list_trades().await?), so this closure can now complete with an error.

Both call sites in take_order_screen.dart assume it can't:

• initState calls _redirectIfParticipant() unawaited with no catch, so the error surfaces as an unhandled async exception (reported to the zone / Crashlytics in release).
• _onTakeOrder awaits it before setState(() => _cta = TakeOrderCta.loading) with no try. The future throws, _cta stays idle, and the Take button silently does nothing — no snackbar, no retry hint.

Suggested fix: keep the old contract at the seam (an unreadable store means "not a known participant", as getTradeRole did), and log it.

final tradeRoleLookupProvider = Provider<Future<TradeRole?> Function(String)>(
  (ref) => (orderId) async {
    try {
      return participatingRole(await orders_api.listTrades(), orderId);
    } catch (e, st) {
      debugPrint('[tradeRoleLookup] listTrades failed for $orderId: $e\n$st');
      return null;
    }
  },
);

Please add a take-screen test with a throwing lookup that asserts the take is still dispatched (or, if you prefer failing closed, that an error snackbar appears — but not a dead button).

🤖 Prompt for AI Agents

In lib/features/order/providers/trade_state_provider.dart, tradeRoleLookupProvider (line ~48):
wrap the listTrades() call in try/catch, log the error, and return null so the lookup keeps the
never-throws contract getTradeRole had. Add a test in
test/features/order/screens/take_order_screen_test.dart overriding tradeRoleLookupProvider with a
throwing function, and assert tapping "Take order" still dispatches takeOrderActionProvider.

[grunch]

⚡ Performance | 🟡 Minor

Every take-screen lookup now deserialises the user's whole trade history — twice per take.

tradeRoleLookupProvider runs on initState and again on the Take tap. list_trades reads every trades row, serde_json-decodes each blob, sorts them, and FRB copies the full Vec<TradeInfo> into Dart, just to filter one order.id. get_trade_role hit the idx_trades_order_id expression index instead. For a long-lived account (hundreds of closed trades, each with bond / peer snapshots) that is a noticeable delay right when the user taps Take.

Not blocking for the fix, but since the rule lives in Dart only because Rust has no "all rows for an order" query, consider a narrow bridge call instead (e.g. list_trades_for_order(order_id) using the existing json_extract(data, '$.order.id') = ? index) and keep participatingRole as the pure rule over its result.

[grunch]

🧪 Test Coverage | 🟡 Minor

This override re-implements the production provider, so its wiring is untested.

The harness replaces tradeRoleLookupProvider with participatingRole(trades, orderId). The production body (participatingRole(await orders_api.listTrades(), orderId)) is never executed by any test, so a regression back to getTradeRole, a wrong argument order, or the error path from the comment above would all keep these tests green.

Consider putting the trades read behind its own seam (e.g. a tradeRowsLookupProvider = Provider<Future<List<TradeInfo>> Function()>), have tradeRoleLookupProvider compose it, and override that here, so the real composition — including the error handling — is what the take-screen tests exercise.