fix(trades): cancel order updates status, unify detail screens - #97
Merged
Conversation
- Optimistic cancel: update trade DB to Canceled and remove from order book immediately in cancel_order(), don't wait for daemon response - Daemon Canceled handler also syncs status to DB for late responses - tradeStatusProvider falls back to trade DB when order is removed from the in-memory book, stops polling on terminal statuses - My Trades list navigates to MyOrderScreen instead of TradeDetailScreen so both Order Book and My Trades open the same detail screen - MyOrderScreen falls back to trade DB when order is not in order book - Invalidate rawTradesProvider after cancel so list reloads from DB - Resilient list_trades: skip rows with deserialization errors instead of failing the entire query - Retry button in trades screen invalidates rawTradesProvider too - Add pending status to TradeDetailScreen with cancel button
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 23 seconds. ⌛ 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)
WalkthroughAdds DB-backed fallbacks and terminal-state short-circuiting to order status polling; performs optimistic local DB sync on cancels (user and daemon paths); improves resilience when reading persisted trades; updates UI/providers/navigation to use persisted trade info and invalidate caches after cancels. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (UI)
participant Provider as State Provider
participant DartAPI as Orders API (Dart bridge)
participant MemBook as In-Memory Order Book
participant DB as Local Trades DB
participant Daemon as Daemon/Gift-Wrap
rect rgba(100,200,100,0.5)
Note over UI,DB: Fallback lookup when order missing
UI->>DartAPI: getOrder(orderId)
DartAPI->>MemBook: lookup order
MemBook-->>DartAPI: null
DartAPI-->>Provider: null
Provider->>DartAPI: listTrades()
DartAPI->>DB: query trades
DB-->>DartAPI: trades list
DartAPI->>Provider: matching tradeInfo
Provider->>Provider: _isTerminal(status)?
Provider-->>UI: emit status (stop polling if terminal)
end
rect rgba(200,100,100,0.5)
Note over UI,DB: Optimistic cancel flow
UI->>DartAPI: cancelOrder(orderId)
DartAPI->>MemBook: publish cancel action
MemBook-->>DartAPI: ack
par optimistic update
DartAPI->>MemBook: remove order
DartAPI->>DB: write OrderStatus::Canceled (best-effort)
DB-->>DartAPI: ok/warn
and daemon sync
Daemon->>MemBook: gift-wrap cancel rumor
MemBook->>DB: sync canceled status
end
DartAPI-->>UI: cancel confirmed
UI->>Provider: invalidate rawTradesProvider
Provider->>DB: reload trades
UI->>UI: show updated state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/order/providers/trade_state_provider.dart`:
- Around line 37-49: When in-memory order info is present (the info branch in
the trade-state polling logic) we currently yield info.status but do not stop
polling; add a terminal-status check and early return there like we do after the
DB fallback. Specifically, after yielding info.status in the block that handles
the result from getOrder()/info, call _isTerminal(info.status) and return if
true so the stream stops polling for terminal statuses; keep the existing
fallback to orders_api.listTrades() and its terminal check unchanged.
In `@lib/features/order/screens/my_order_screen.dart`:
- Around line 83-89: The UI is treating the fallback DB call as "not found"
because tradeInfoProvider(widget.orderId).valueOrNull is null while loading;
change the logic to first read the AsyncValue (e.g. final tradeInfo =
ref.watch(tradeInfoProvider(widget.orderId))) and only consider the order
missing when tradeInfo is not loading and tradeInfo.valueOrNull is null;
otherwise, if tradeInfo is loading, avoid rendering the not-found scaffold (or
show a loading state) and when tradeInfo has data use
OrderItem.fromInfo(tradeInfo.value!.order) to populate order.
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 445-460: The CANCEL button's handler (_cancelOrder) doesn't
refresh the trades cache, so update _cancelOrder to invalidate/refresh the same
provider used by MyOrderScreen (rawTradesProvider) after a successful cancel;
locate the _cancelOrder method in TradeDetailScreen and, on successful
cancellation, call the provider invalidation/refresh logic (the same
ref.invalidate(rawTradesProvider)/controller refresh used elsewhere) so the
cached trade list and filters are updated immediately.
In `@lib/features/trades/widgets/trades_list_item.dart`:
- Line 61: The tap handler always navigates to the creator-style order route
(context.push(AppRoute.myOrderPath(trade.orderId))) which hides taken-order
flows; change the onTap in trades_list_item.dart to branch on whether the trade
is taken (e.g. trade.takerId == currentUser.id or trade.isTaken / trade.status
== 'taken') and navigate to the appropriate taken-order route (or pass a
mode/flag into MyOrderScreen) so taken trades use the taken-order UI (e.g.
context.push(AppRoute.takenOrderPath(trade.orderId)) or
context.push(AppRoute.myOrderPath(trade.orderId, mode: OrderViewMode.taken))).
Ensure you obtain current user id from the existing auth/user context and keep
MyOrderScreen-compatible params.
In `@rust/src/api/orders.rs`:
- Around line 752-768: The optimistic-cancel path currently uses the
possibly-unreconciled local placeholder order_id (from order_book().remove_order
and the db.update_trade_fields call) which can publish a Cancel for the local
UUID and remove/mark the trade locally while the daemon still has the real
order; fix this by detecting unresolved local IDs before publishing or mutating
local state: translate or look up the canonical daemon UUID for order_id (or
block the cancel and return an error if reconciliation hasn’t happened) and only
call order_book().remove_order and db.update_trade_fields with the canonical ID
(also apply the same check to the Action::Canceled DB sync code referenced
around the Action::Canceled handling at lines ~926-942 so pre-reconciled rows
aren’t missed).
- Around line 756-768: The optimistic cancel path calls update_trade_fields via
crate::db::app_db::db() with OrderStatus::Canceled but the IndexedDB backend
currently stubs persistence for wasm, so web builds never persist canceled
status; update the wasm IndexedDB implementation to perform an async write using
indexed_db_futures (or alter update_trade_fields to branch on target_arch =
"wasm32" and call the indexed_db_futures-based persistence routine) so the
cancel status is persisted for the DB-backed fallback; apply the same fix to the
other occurrence around lines referencing the second update_trade_fields call
(also at the noted 930-942 region) so both cancel-status updates use
indexed_db_futures on web.
🪄 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: ec4f7d41-70a4-43cd-bd45-90cd4d51b9dd
📒 Files selected for processing (7)
lib/features/order/providers/trade_state_provider.dartlib/features/order/screens/my_order_screen.dartlib/features/trades/screens/trade_detail_screen.dartlib/features/trades/screens/trades_screen.dartlib/features/trades/widgets/trades_list_item.dartrust/src/api/orders.rsrust/src/db/sqlite.rs
…taker routing - Stop polling tradeStatusProvider when in-memory order has terminal status - Show loading state in MyOrderScreen while trade DB query is in flight - Invalidate rawTradesProvider after cancel in TradeDetailScreen - Route taker trades to TradeDetailScreen instead of MyOrderScreen
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 218-221: The UI is showing the pending text/Cancel CTA before the
async status resolves; update the branches that currently check for
TradeStatus.pending (and OrderStatus.pending) to first handle the provider
unresolved state: read tradeStatus via tradeStatusProvider(widget.orderId),
treat TradeStatus.loading (and error) as "not pending", and only render the
pending instruction and Cancel CTA when tradeStatus == TradeStatus.pending and
the provider is not loading/error (i.e., confirmed pending). Apply this change
to both locations where pending is checked (the branch around the current
pending text and the similar branch at the later block around lines 447-462).
In `@lib/features/trades/widgets/trades_list_item.dart`:
- Around line 61-65: The current onTap routing only checks trade.role and always
sends creator trades to MyOrderScreen; change routing to consider the trade
lifecycle/status as well: if trade.role == TradeRole.creator AND trade.status ==
TradeStatus.pending (or lifecycle == 'pending'), push
AppRoute.myOrderPath(trade.orderId); otherwise (creator in
active/fiat-sent/completed/disputed OR any taker), push
AppRoute.tradeDetailPath(trade.orderId). Update the onTap in
trades_list_item.dart to branch on both trade.role and trade.status (use the
existing trade.status or trade.lifecycle property and the
AppRoute.tradeDetailPath / AppRoute.myOrderPath helpers).
🪄 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: e6f69404-81ef-497c-b7ad-f8d4039483aa
📒 Files selected for processing (4)
lib/features/order/providers/trade_state_provider.dartlib/features/order/screens/my_order_screen.dartlib/features/trades/screens/trade_detail_screen.dartlib/features/trades/widgets/trades_list_item.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/order/providers/trade_state_provider.dart
1. trade_detail_screen.dart:329-331 — Replaced valueOrNull ?? OrderStatus.pending with an explicit check on the AsyncValue state. Now uses TradeStatus.loading while the provider hasn't resolved, preventing the pending instruction text and CANCEL button from flashing before the real status is known. 2. trades_list_item.dart:61-65 — Changed routing logic: only creator trades that are still pending go to MyOrderScreen; all other trades (creators in active/fiatSent/etc. and all takers) route to TradeDetailScreen which has the full buyer/seller action flow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/trades/widgets/trades_list_item.dart`:
- Around line 61-65: The routing condition currently checks the stale DB
snapshot field trade.status but the UI uses the live effectiveStatus (from
tradeStatusProvider), causing inconsistent navigation; update the onTap logic in
trades_list_item.dart to use the same effectiveStatus value used for the status
chip (the computed effectiveStatus from the tradeStatusProvider) instead of
trade.status so the ternary that chooses between
AppRoute.myOrderPath(trade.orderId) and AppRoute.tradeDetailPath(trade.orderId)
uses effectiveStatus == TradeStatusFilter.pending together with trade.role ==
TradeRole.creator; if effectiveStatus isn't in scope, fetch/derive it the same
way the chip does (via tradeStatusProvider) and reuse that variable in the onTap
check.
- Line 22: Update the doc comment above the TradesListItem widget to reflect the
conditional navigation behavior: state that tapping navigates to either
`/my_order/:orderId` or `/trade_detail/:orderId` depending on the current user's
role and the trade status (i.e., the same condition used in the widget's tap
handler/onTap logic inside TradesListItem). Mention the routing is decided in
the widget's onTap handler (or the method that performs navigation) so readers
know the navigation target is conditional.
🪄 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: f0ddea96-ec37-4a44-a375-b8b616393bac
📒 Files selected for processing (2)
lib/features/trades/screens/trade_detail_screen.dartlib/features/trades/widgets/trades_list_item.dart
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 by CodeRabbit
New Features
Bug Fixes
Improvements