fix: prevent wrong role text on canceled order detail screen#580
fix: prevent wrong role text on canceled order detail screen#580
Conversation
|
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 32 minutes and 3 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 (2)
WalkthroughUpdated TradeDetailScreen title logic to detect trades canceled without a session and use a dedicated localized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
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/l10n/intl_de.arb`:
- Line 289: Add a metadata block for the ARB key canceledTradeTitle that
declares its sats placeholder; specifically add an `@canceledTradeTitle` entry
with a placeholders object containing sats (e.g. "@canceledTradeTitle": {
"placeholders": { "sats": {} } }) so the ARB file properly documents the {sats}
variable used by canceledTradeTitle.
In `@lib/l10n/intl_en.arb`:
- Line 289: The new ARB key canceledTradeTitle is missing its required metadata
entry (`@canceledTradeTitle`) and must be added (including a description and
placeholders spec for {sats}); update intl_en.arb to add the `@canceledTradeTitle`
metadata object, then mirror the new key plus corresponding `@canceledTradeTitle`
metadata into intl_es.arb and intl_it.arb with appropriate translated values and
matching placeholder definitions so all three localization files remain
consistent.
In `@lib/l10n/intl_es.arb`:
- Line 655: The ES ARB is missing the metadata block for the
"canceledTradeTitle" key; add an `@canceledTradeTitle` metadata object matching
the IT file's metadata (description, type, and placeholders) so the placeholder
{sats} is documented—ensure the metadata includes a "description" explaining the
use, "type": "text", and a "placeholders" entry for "sats" (with any example or
content description) to mirror intl_it.arb and keep all three ARB files
consistent.
- Line 655: The localized string "canceledTradeTitle" currently contains a space
before the {sats} placeholder ("Intercambio de {sats} sats") which causes double
spaces when satAmount already includes a leading space; update the string to
remove that extra space (e.g., "Intercambio de{sats} sats") so interpolation
yields correct spacing for both empty and non-empty satAmount, and consider
applying the same fix to the related keys "youAreSellingTitle" and
"youAreBuyingTitle" for consistency.
In `@lib/l10n/intl_fr.arb`:
- Line 289: Add a placeholder metadata block for the canceledTradeTitle ARB key:
create an "@canceledTradeTitle" JSON object immediately after
"canceledTradeTitle" that declares the "sats" placeholder (e.g., under
"placeholders": {"sats": {"type": "int"}}) and, optionally, include a short
"description" explaining the placeholder; this matches the placeholder-based
convention used for other keys and keeps ARB files consistent.
In `@lib/l10n/intl_it.arb`:
- Line 713: The Italian messages include a literal space before the {sats}
placeholder which causes double spaces when satAmount already contains a leading
space or an empty result; update the three keys canceledTradeTitle,
youAreSellingTitle, and youAreBuyingTitle to remove the space before {sats}
(matching the EN pattern "Trade of{sats} sats") so the variable-provided spacing
logic controls spacing and avoids double/empty-space output.
🪄 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: 52229150-2401-48f7-b2ec-0d8737775cf4
📒 Files selected for processing (6)
lib/features/trades/screens/trade_detail_screen.dartlib/l10n/intl_de.arblib/l10n/intl_en.arblib/l10n/intl_es.arblib/l10n/intl_fr.arblib/l10n/intl_it.arb
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Request Changes
1. Out-of-scope change — Commit d42a465c
The commit "fix: remove duplicate currency code in order amount display" modifies:
lib/features/order/screens/take_order_screen.dart(removes${order.currency})lib/shared/widgets/order_cards.dart(removes$currency)
This is not related to issue #577. The issue is about incorrect role text on canceled orders. The duplicate currency changes are a separate UI fix that should go in its own PR.
Action: Revert this commit from the branch or move it to a separate PR.
2. Main fix — Correct but with a caveat
Issue #577 reports: "When a buy order is canceled in status waiting-payment or waiting-buyer-invoice, it incorrectly displays to the seller: 'You are buying sats'".
Your fix detects session == null and shows a neutral title (canceledTradeTitle). This works when there is no session, but what if there IS a session (session != null) and the role is still wrong?
If bug #577 also occurs with a session present, this fix does not cover it. I recommend verifying with whoever reported #577 whether their specific case is session == null or if there is a deeper issue in session.role determination.
Not blocking — the fix is reasonable for the session == null case, but please document in the PR description that the scope is specifically canceled orders without an active session.
3. Double-space fix — Commit 6d1ead81 ✅
Confirmed correct. The code generates satAmount with a leading space:
final satAmount = hasFixedSatsAmount ? ' ${tradeState.order!.amount}' : '';So 'Estás vendiendo{sats} sats' with satAmount = ' 10000' produces "Estás vendiendo 10000 sats" (one space). Before this commit there was a double space. This commit is correct.
4. Placeholder metadata inconsistency
canceledTradeTitle has @canceledTradeTitle (metadata) only in intl_it.arb, not in the other locale files. Please check whether this is the project pattern — if the other strings in en/de/es/fr do not have metadata for placeholders, then it is fine as-is. But if consistency is expected, either add metadata to all locales or remove it from it.
✅ What is good
- The
canceledTradeTitlefix is conceptually correct - All translations are present (5 languages)
- No breaking changes in string API
Summary
| Item | Status |
|---|---|
| Commit 1 (role fix) | ✅ Correct |
| Commit 2 (double space) | ✅ Correct |
| Commit 3 (currency dup) | 🔴 Out of scope — remove or split |
| Locale coverage | ✅ Complete |
| Metadata consistency |
Required action: Remove commit d42a465c from the branch.
2 participants
fix #577
Summary by CodeRabbit