Skip to content

fix: display selected fiat amount instead of range for taken orders#477

Merged
grunch merged 1 commit intomainfrom
fix-range-displays
Feb 17, 2026
Merged

fix: display selected fiat amount instead of range for taken orders#477
grunch merged 1 commit intomainfrom
fix-range-displays

Conversation

@Catrya
Copy link
Member

@Catrya Catrya commented Feb 16, 2026

fix #441

Summary by CodeRabbit

  • Refactor
    • Consolidated fiat amount display logic in the trade detail screen for improved code maintainability.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 16, 2026

Walkthrough

A single-file refactoring that consolidates fiat amount display logic in the trade detail screen. A new helper method _displayFiatAmount() determines whether to display a range (for pending orders with differing min/max amounts) or a single amount, replacing three inline conditional expressions.

Changes

Cohort / File(s) Summary
Fiat Amount Display Logic
lib/features/trades/screens/trade_detail_screen.dart
Added _displayFiatAmount(Order) helper method that returns a min-max range string for pending orders with differing amounts, or the actual fiatAmount otherwise. Consolidated three inline amount-rendering expressions into calls to this helper. Added Order model import.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • AndreaDiazCorreia
  • grunch
  • mostronator

Poem

🐰 A range once scattered across the screen,
Now bundled cleanly, clear to be seen!
The fiat amounts, both small and wide,
Now show with grace, side by side. ✨

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing the display of a selected fiat amount instead of a range for taken orders, which directly corresponds to the code refactoring that introduces _displayFiatAmount logic.
Linked Issues check ✅ Passed The changes directly address issue #441 by introducing _displayFiatAmount method that displays selected fiat amount for non-pending orders and ranges only for pending orders, matching the requirement to show actual selected amount instead of full range for taken orders.
Out of Scope Changes check ✅ Passed All changes are scoped to the trade detail screen's fiat amount display logic, with only one helper method addition and refactoring of existing display calls, all directly related to issue #441.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-range-displays

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
lib/features/trades/screens/trade_detail_screen.dart (1)

105-116: Remove redundant pending status check; use caller's isPending value instead.

The method re-derives isPending from order.status (line 111), but the caller already determined it from tradeState.status (line 53). Since OrderState encapsulates the authoritative state, using inconsistent sources (checking order.status within the method while tradeState.status drives the caller's logic) risks silent bugs if these fields ever diverge. Pass the already-resolved isPending flag as a parameter:

Suggested fix
-  String _displayFiatAmount(Order order) {
+  String _displayFiatAmount(Order order, {required bool isPending}) {
     final isRangeOrder = order.minAmount != null &&
         order.maxAmount != null &&
         order.minAmount != order.maxAmount;
-    final isPending = order.status == Status.pending;
     if (isRangeOrder && isPending) {
       return "${order.minAmount} - ${order.maxAmount}";
     }
     return order.fiatAmount.toString();
   }

Update call sites to pass the resolved flag:

-          amount: _displayFiatAmount(tradeState.order!),
+          amount: _displayFiatAmount(tradeState.order!, isPending: isPending),

@grunch grunch requested a review from mostronator February 17, 2026 14:55
Copy link
Member

@grunch grunch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@grunch grunch merged commit 236459a into main Feb 17, 2026
2 checks passed
@grunch grunch deleted the fix-range-displays branch February 17, 2026 15:10
grunch pushed a commit that referenced this pull request Feb 17, 2026
…#480)

Use the caller's isPending (from tradeState.status) as single source
of truth instead of re-computing it from order.status inside the
method. Prevents potential silent bugs if the two sources ever diverge.

Addresses CodeRabbit nitpick from PR #477.

Co-authored-by: mostronator <mostronator@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When there's a range order, the selected fiat currency amount should be displayed

2 participants