feat(trades): surface cancel/dispute/release as visible buttons - #199
Conversation
Cancel, dispute, release, fiat-sent, and share-order button labels in the trade detail screen were hardcoded English strings. Add ARB keys in all 5 supported languages (en/es/fr/de/it) and regenerate AppLocalizations. Ref #134
Cancel, dispute, and release were only reachable through the app-bar overflow menu, making them hard to spot during an active trade. Move them into a secondary row below the primary CTA, using a new MostroReactiveButton outlined variant so loading/error feedback stays consistent with the rest of the screen. Also: - Fix MostroReactiveButton showing a false "success" state on failure: the trade-detail handlers were swallowing their own errors instead of letting the button's own catch block see them. - Remove the button's error-state color/icon change. The SnackBar already reports failures, so the button now just re-enables after a cooldown instead of flashing red. - Restore a minimal overflow menu scoped to a single "Share order" item, which shows a coming-soon message (the underlying share_order contract in contracts/orders.md is not implemented yet). - Sync specs/004 (spec.md, tasks.md) with the layout actually shipped. Closes #134
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
WalkthroughTrade detail actions were moved from the overflow menu into status- and role-specific secondary buttons. Reactive buttons gained destructive and outlined variants, action labels were localized, failures now rethrow, and widget tests cover the updated UI. ChangesTrade action UI
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TradeDetailScreen
participant MostroReactiveButton
participant TradeActions
TradeDetailScreen->>MostroReactiveButton: Render role- and status-specific action
MostroReactiveButton->>TradeActions: Invoke cancel, dispute, or release
TradeActions-->>TradeDetailScreen: Complete or throw action result
TradeDetailScreen-->>TradeDetailScreen: Show localized feedback
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Suggestion, not blocking this PR. This PR adds 5 files just from running flutter gen-l10n: app_localizations.dart plus the 4 locale variants. Every future ARB change will keep touching these same generated files and adding review noise. Worth considering the same treatment for lib/l10n/app_localizations*.dart, generated by flutter gen-l10n from the arb files. The arb source files would stay tracked as they are the actual input. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/trades/screens/trade_detail_screen.dart (1)
147-182: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDeclining Cancel/Release confirmation dialogs flashes a false "success" checkmark.
_cancelOrderand_releaseOrderbothreturnnormally when the user declines the confirmation dialog. SinceMostroReactiveButton._handlePresstreats any non-throwing completion as success, both actions incorrectly show a green checkmark for a no-op — misleading for financial actions like cancelling a trade or releasing escrowed sats.
lib/features/trades/screens/trade_detail_screen.dart#L147-L182: in_cancelOrder, throw instead of silently returning whenconfirmed != true, so the button doesn't animate success.lib/features/trades/screens/trade_detail_screen.dart#L215-L234: apply the same fix in_releaseOrder, used by both the disputed-row Release button and the "Confirm & release sats" primary CTA.🐛 Proposed fix pattern (apply to both functions)
Future<void> _cancelOrder() async { final l10n = AppLocalizations.of(context); final confirmed = await showDialog<bool>(...); - if (confirmed != true || !mounted) return; + if (!mounted) return; + if (confirmed != true) { + // User declined — signal non-success so the reactive button doesn't + // flash a misleading "success" checkmark; it will just cool down + // briefly instead. + throw StateError('Cancel dialog declined'); + } try {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/trades/screens/trade_detail_screen.dart` around lines 147 - 182, When the user declines confirmation, make both _cancelOrder and _releaseOrder throw instead of returning normally, so MostroReactiveButton._handlePress does not display a success checkmark. Preserve the existing mounted checks and action behavior for confirmed dialogs; update both affected sites in lib/features/trades/screens/trade_detail_screen.dart (lines 147-182 and 215-234).
🧹 Nitpick comments (2)
test/shared/widgets/mostro_reactive_button_test.dart (1)
8-29: 📐 Maintainability & Code Quality | 🔵 TrivialNo test exercises
MostroButtonVariant.destructive.All tests default to
primary; the destructive-red styling path (used for Cancel/Dispute/Release intrade_detail_screen.dart) is untested.✅ Suggested additional test
testWidgets('destructive variant uses the destructive accent color', (tester) async { await _pump(tester, onPressed: () async {}, outlined: true, variant: MostroButtonVariant.destructive); final button = tester.widget<OutlinedButton>(find.byType(OutlinedButton)); final side = button.style?.side?.resolve({})?.color; expect(side, isNot(const Color(0xFF8CC63F))); // not the primary green });Also applies to: 44-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/shared/widgets/mostro_reactive_button_test.dart` around lines 8 - 29, Extend the MostroReactiveButton widget tests to cover MostroButtonVariant.destructive, including the outlined styling path used by destructive actions. Add an assertion that the resolved button accent color differs from the primary green, while keeping existing primary-variant coverage unchanged.specs/004-mostro-p2p-client/tasks.md (1)
225-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the seller CTA requirement with the implemented label.
The current UI contract uses
confirmReleaseSatsButtonfor the seller’s fiat-sent primary action, while this task still specifiesRELEASE. Update the requirement or implementation so QA does not validate conflicting labels.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/004-mostro-p2p-client/tasks.md` around lines 225 - 228, Align the seller fiat-sent CTA contract between task T064 and the implementation using confirmReleaseSatsButton: update the requirement to specify the implemented label, or rename the implementation so both consistently use RELEASE. Ensure QA has one unambiguous primary-action label.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 147-182: When the user declines confirmation, make both
_cancelOrder and _releaseOrder throw instead of returning normally, so
MostroReactiveButton._handlePress does not display a success checkmark. Preserve
the existing mounted checks and action behavior for confirmed dialogs; update
both affected sites in lib/features/trades/screens/trade_detail_screen.dart
(lines 147-182 and 215-234).
---
Nitpick comments:
In `@specs/004-mostro-p2p-client/tasks.md`:
- Around line 225-228: Align the seller fiat-sent CTA contract between task T064
and the implementation using confirmReleaseSatsButton: update the requirement to
specify the implemented label, or rename the implementation so both consistently
use RELEASE. Ensure QA has one unambiguous primary-action label.
In `@test/shared/widgets/mostro_reactive_button_test.dart`:
- Around line 8-29: Extend the MostroReactiveButton widget tests to cover
MostroButtonVariant.destructive, including the outlined styling path used by
destructive actions. Add an assertion that the resolved button accent color
differs from the primary green, while keeping existing primary-variant coverage
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 376d47ea-31e7-4f71-9131-964a88d8096f
📒 Files selected for processing (17)
lib/features/trades/screens/trade_detail_screen.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_it.dartlib/shared/widgets/mostro_reactive_button.dartspecs/004-mostro-p2p-client/spec.mdspecs/004-mostro-p2p-client/tasks.mdtest/features/trades/trade_detail_screen_test.darttest/shared/widgets/mostro_reactive_button_test.dart
MostroReactiveButton treats any normal return from onPressed as success. Cancel and release confirmation dialogs returned normally when the user picked No, so declining showed a false success checkmark. Both now throw in that case, keeping the existing mounted check and confirmed dialog behavior unchanged. test(trades): cover MostroReactiveButton destructive variant No test exercised MostroButtonVariant.destructive, the outlined style used by the cancel and dispute buttons. Add a test asserting the resolved accent color differs from the primary green, without changing existing primary variant coverage. docs(specs): align T064 primary CTA label with the shipped button T064 specified RELEASE as the seller fiat sent primary CTA, but the implementation uses confirmReleaseSatsButton, Confirm & release sats, already covered by an existing test. Update the task to match the shipped label instead of changing tested UI copy.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Summary
The layout change itself is good — promoting Cancel/Dispute/Release out of the ⋮ menu is the right call for #134, the spec/tasks updates are thorough, and the new widget tests are well-structured (the comments explaining why pumpAndSettle() is unsafe against the countdown Timer.periodic are genuinely useful).
However, the last commit (be9a31d, "throw when cancel or release confirmation is declined") trades one bug for a worse one, and it ships with no test coverage. Requesting changes on that plus a layout risk.
Blocking
- Declining a confirmation dialog leaves the button dead for 4s. Throwing
StateErrorfor "user said No" routes a normal interaction through the failure path, which disables the button for the full error cooldown with no visual feedback. Details inline attrade_detail_screen.dart:168. - Secondary row will overflow on long locales / small screens. Two
Expandedbuttons with German/French labels won't fit at ~140dp each. Details inline attrade_detail_screen.dart:625.
Non-blocking findings
| # | Severity | Finding |
|---|---|---|
| 3 | MEDIUM | Duplicate l10n key: cancelTradeButton and cancelOrderButton both mean "Cancel order" in en but diverge in de (Bestellung stornieren vs Angebot stornieren) |
| 4 | MEDIUM | Two competing error-reporting conventions now coexist in the same switch (onError vs SnackBar-in-handler + rethrow) |
| 5 | MEDIUM | Accessibility regression: the Semantics(label: 'Error', liveRegion: true) announcement was removed |
| 6 | MEDIUM | Test gaps — see below |
| 7 | MINOR | label: 'View dispute' (trade_detail_screen.dart:861) and label: 'Pay hold invoice' (:826) are still hardcoded English, sitting directly beside the labels this PR just localized. Outside the diff hunks so I couldn't anchor inline, but they read as an oversight given the PR's stated l10n goal |
| 8 | MINOR | Scope creep: restoring a ⋮ menu whose only entry is a non-functional "Share order" adds always-visible dead UI plus 5 translations for an unimplemented contract. Consider splitting it out or gating it |
| 9 | MINOR | Undocumented visual change: the release CTA lost icon: Icons.lock_open. Intentional? Not mentioned in the PR body or the spec updates |
| 10 | MINOR | Locale register: the screen title is fr DÉTAILS DE L'ORDRE but the new buttons use commande. The fr file already mixes ordre (trading) and commande (retail); new keys should follow ordre, matching navOrderBook / orderDetailsTitle |
Test coverage
- The entire last commit is untested. No test covers "decline the dialog" for either cancel or release — which is exactly where the blocking bug lives.
- No test covers the three
rethrows added to_cancelOrder/_openDispute/_releaseOrder. trade_detail_screen_test.dart:227invokespopupButton.onSelected!(0)directly. The reasoning is documented, but it means thePopupMenuItem(value: 0)→onSelectedwiring is never exercised; a wrongvaluewould still pass.tester.tap(find.text('Share order'))followed byawait tester.pump(const Duration(seconds: 1))is usually stable enough.- No layout/locale test — the 6 new tests all run at the default 800×600 surface in English, so they structurally cannot catch finding #2.
Nice work overall — the gating matrix and the role/status test table are solid. Happy to re-review once the decline path is reworked.
Grunch flagged that throwing StateError on a declined confirmation traded the false success checkmark for a worse bug: onError is null on the secondary row buttons, so the button silently disabled itself for 4 seconds with no visual feedback on a normal decline. Add MostroActionAborted, a marker exception for user intent rather than failure. MostroReactiveButton catches it before the generic error path and returns straight to idle, no checkmark, no cooldown. Cancel and release now throw it when the user picks No. Also wrap secondary action button labels instead of letting them overflow. Two Expanded outlined buttons at roughly 140dp cannot fit German or French labels at one line. Add a layout test at 320dp with Locale de that pumps the secondary row and asserts no layout exception.
Localization - cancelTradeButton duplicated cancelOrderButton in English while diverging in German, since both meant Cancel order. Reworded it to Cancel trade in all 5 locales, matching cancelTradeDialogTitle's existing wording for the same flow. - French shareOrderButton used commande, the retail register, while the screen itself uses ordre for order-book concepts. Changed to Partager l'ordre. - Wired the two remaining hardcoded English CTAs, View dispute and Pay hold invoice, to l10n. View dispute reuses the previously orphaned viewDisputeButtonLabel key, recased from all caps to match the shown text. Pay hold invoice is a new key across all 5 locales. MostroReactiveButton - Restored the Icons.lock_open icon on the seller release CTA, lost without mention when it moved off the old popup menu. - The removed error styling left screen readers with no signal at all during the 4s cooldown. Add a Semantics liveRegion announcement, localized, scoped to the cooldown state only. - Renamed _ButtonState.error to cooldown, since it now renders identically to idle and the old name was misleading. - Extracted the success display and error cooldown durations into named constants. Error reporting convention - markFiatSentButton was the only handler using the onError callback; the rest show their own SnackBar and rethrow. Migrated it to the same pattern via a new _markFiatSent method, so one convention applies across the whole switch. Overflow menu - Restored a typed _OverflowAction enum instead of a magic PopupMenuButton<int> with value 0, matching the enum this PR had removed. Dropped the redundant context parameter and duplicate AppLocalizations.of(context) calls. Tests - The Share order popup test invoked onSelected directly, which cannot catch a wrong PopupMenuItem value. Switched to a real tap; needed a fully settled opening animation first, or the tap misses the item's actual position. - Added coverage for the three rethrows in cancel, open dispute, and release: with no RustLib.init() in this harness, the bridge calls fail for real, exercising the actual SnackBar and rethrow path. - mostro_reactive_button_test.dart's MaterialApp had no localization delegates, needed once the widget started reading AppLocalizations.
|
Addressed everything from the review. @grunch Both blocking issues are fixed, the decline path no longer freezes the button and the secondary row wraps instead of overflowing on long locales. All non-blocking findings are resolved too. Ready for another look whenever you have time. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/l10n/app_es.arb (1)
215-221: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
markFiatSentButtonshould beMarcar como enviado.
Marcar fiat enviadoreads unnaturally in Spanish; thecomobridge matches the rest of the locale and the source intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/l10n/app_es.arb` around lines 215 - 221, Update the Spanish localization value for markFiatSentButton from “Marcar fiat enviado” to “Marcar como enviado”, leaving the surrounding translation entries unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/features/trades/trade_detail_screen_test.dart`:
- Around line 296-308: Update the release confirmation buttons in
ReleaseConfirmationDialog to use the appropriate AppLocalizations labels instead
of hardcoded “Yes” and “No” strings, then update the trade detail screen test to
retrieve and tap the localized confirmation label rather than find.text('Yes').
---
Outside diff comments:
In `@lib/l10n/app_es.arb`:
- Around line 215-221: Update the Spanish localization value for
markFiatSentButton from “Marcar fiat enviado” to “Marcar como enviado”, leaving
the surrounding translation entries unchanged.
🪄 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: c95563b5-3b8b-43de-a436-ced909abad17
📒 Files selected for processing (15)
lib/features/trades/screens/trade_detail_screen.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_it.dartlib/shared/widgets/mostro_reactive_button.darttest/features/trades/trade_detail_screen_test.darttest/shared/widgets/mostro_reactive_button_test.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/l10n/app_it.arb
- lib/l10n/app_de.arb
- lib/l10n/app_localizations_en.dart
- test/shared/widgets/mostro_reactive_button_test.dart
- lib/features/trades/screens/trade_detail_screen.dart
The Yes/No buttons in ReleaseConfirmationDialog were hardcoded English,
the one confirmation dialog in this flow that was not going through
AppLocalizations. Added yesButtonLabel across all 5 locales and reused
the existing noButtonLabel.
The test asserted and tapped find.text('Yes') directly. Switched it to
read AppLocalizationsEn().yesButtonLabel, so it verifies the actual
localized label instead of coincidentally matching a hardcoded string.
AndreaDiazCorreia
left a comment
AndreaDiazCorreia
left a comment
There was a problem hiding this comment.
Hi @BraCR10, could you resolve the merge conflicts? I think several of them are probably related to the i18n changes
HI, Yes I'll resolve them ASAP |
…visible-actions # Conflicts: # lib/features/trades/screens/trade_detail_screen.dart # lib/shared/widgets/mostro_reactive_button.dart
3 participants
What
trade detail overflow menu (⋮) to first-class, always-visible buttons below
the primary action.
MostroReactiveButtonso loading/result states stayconsistent with the primary CTA.
Why
Closes #134. In Active/FiatSent trades, Cancel and Open dispute were hidden in the overflow menu, making it hard for users to escalate a disputed trade or back out of a bad one quickly.
The Contact chat chip in the header is untouched by this PR. It stays exactly where it was. Instead of four same weight buttons, the fix is a secondary action row for Cancel, Dispute, and Release only, keeping the existing chat chip for Contact. This PR implements that reduced scope.
Behavior by role + status
Test plan
flutter analyze— no issuesflutter test— 46 tests pass, including newtrade_detail_screen_test.dart(role/status button matrix) andmostro_reactive_button_test.dartSummary by CodeRabbit
Release Notes