feat(ui): apply Mostro UX redesign from design handoff - #106
Merged
Conversation
Implements the redesign proposals from the Claude Design handoff bundle (Mostro UX Redesign EN): - Active trade (#1+#5): single state-dependent primary CTA, cancel/dispute collapsed behind the app-bar overflow menu, persistent chat chip with unread badge, step timeline, contextual timer with consequence copy and lime/amber/red thresholds - Take order (#5): contextual 'time to take this order' timer card, 3-column creator reputation, headline summary - Order book (#3): differentiator reason pills (best premium / most reputable / just published), color-coded premium pill, numeric reputation row - Chat (#6): sticky trade-state header with status, amounts, method, live countdown and view-order link - Notifications (#8): grouped by trade with collapsible event history, unread badges, go-to-trade action, system banner, filter chips - Create order (#7): Express / Conservative / Custom presets with prefill from last successful trade and live preview footer - Backup ritual (#4): trigger bottom sheet, 12-word grid with paper warning, 3-random-word verification, persisted backed-up state - Theme: new warningAmber token
Contributor
WalkthroughThis PR introduces a backup account verification ritual, order badging and list UI redesign, a sticky trade state header with live countdown, notifications refactoring with grouping, order preset selector, and a major trade detail screen state-machine refactor. It also extends the color theme with a warning amber token. ChangesAccount Backup and Verification Flow
Order Browsing and Listing Enhancements
Trade and Chat UI Updates
Order Creation and Taking
Notifications Refactoring
Trade Detail Screen State-Machine Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7abe653ad5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+99
to
+100
| await prefs.setInt(kBackupSnoozedUntilKey, until.millisecondsSinceEpoch); | ||
| state = false; |
There was a problem hiding this comment.
When a user taps “Remind me tomorrow” and keeps the app process alive past the 24-hour snooze window, this only writes the future timestamp and sets state = false; nothing schedules a wake-up or re-checks the timestamp, so the backup badge/banner stays hidden until some later provider reload or explicit reminder action. Since the comment says the reminder reappears once the snooze elapses, add a timer/invalidation for until (and cancel it on confirm/reset) so long-running sessions restore the active reminder on time.
Useful? React with 👍 / 👎.
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/features/trades/screens/trade_detail_screen.dart (2)
77-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrive the countdown from the trade timeout, not from a one-time local seed.
_loadExpiresAt()only runs ininitState()and only asksgetOrder()forexpiresAt. When the order has already left the book, Lines 104-105 leave the UI on the default 15-minute timer, and when an expiry does exist, setting_totalCountdownSecondsto the currentdiffresets the lime/amber/red thresholds to 100% every time the screen opens. Pull the active phase timeout from the trade-side source and refresh it when the trade status changes so the timer stays accurate after reopen/resume and across state transitions.Also applies to: 93-106, 676-684
🤖 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 77 - 80, The countdown is being seeded once in initState via _loadExpiresAt() which calls getOrder() and sets _totalCountdownSeconds from the current diff, causing wrong defaults and threshold resets when reopening; change the logic so the countdown is driven by the authoritative trade timeout/phase values from the trade object (use trade.expiresAt or trade.timeout/phaseTimeout) instead of a one-time local diff, refresh that value whenever the trade status/phase changes and on resume (hook into whatever trade update callback or status stream you already have), and stop resetting _totalCountdownSeconds to the instantaneous remaining diff on screen open—set _totalCountdownSeconds from the configured phase timeout and compute remainingSeconds from expiresAt for display so amber/lime/red thresholds remain stable (update _loadExpiresAt, _startCountdown and the trade status change handler to pull and apply the trade-side timeout).
416-436: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't assume buyer while the persisted role is still loading.
Lines 421-423 force
isBuyer = trueuntiltradeRoleFromDbProviderresolves, so reopened seller trades briefly render buyer copy and the wrong primary CTA. Derive the fallback fromorderonce it is available, or keep the role-dependent UI in a neutral/loading state until the role resolves.🤖 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 416 - 436, The code currently defaults isBuyer = true while tradeRoleFromDbProvider is unresolved, causing seller trades to render as buyer; change the logic in the trade detail screen where you compute isBuyer (using tradeRoleProvider and tradeRoleFromDbProvider) so that if dbRole is null you first try to derive the role from the live order (the local variable order from orderBookProvider) and only if order is also unavailable set isBuyer to null/unknown (or keep a neutral/loading state); update any downstream UI that reads isBuyer to handle a nullable/unknown role (show loading/neutral CTA) instead of assuming true. Reference symbols: tradeRoleProvider, tradeRoleFromDbProvider, isBuyer, order, orderBookProvider.lib/features/account/providers/backup_reminder_provider.dart (1)
2-2: 📐 Maintainability & Code Quality | 🟠 MajorSharedPreferences-backed backup reminder state violates the UI-layer persistence guideline
lib/features/account/providers/backup_reminder_provider.dartpersists reminder/completed/snooze flags usingshared_preferences, butlib/**/*.dartrequires Sembast for UI-layer state persistence—migrate this state to Sembast (or document an explicit exception).
confirmBackupComplete()writes toSharedPreferencesand setsstatewithout awaitingload(). In the normal app boot pathBackupReminderNotifieris constructed with aninitialValueinlib/main.dart(soload()isn’t in-flight), but for any ProviderContainer/notifier created without that override, an outstandingload()could updatestateafter confirmation—callawait load()(or gate on_loaded) to remove the risk.🤖 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/account/providers/backup_reminder_provider.dart` at line 2, The BackupReminderNotifier currently persists state using SharedPreferences and updates state in confirmBackupComplete() without awaiting load(), violating the UI-layer persistence guideline; migrate persistence from SharedPreferences to the app's Sembast-backed storage (or add explicit justification/documentation if SharedPreferences must be used) and update BackupReminderNotifier/confirmBackupComplete() to await load() (or gate on the notifier's _loaded flag) before writing persistence and setting state to avoid a stale in-flight load overwriting the confirmation; locate BackupReminderNotifier, confirmBackupComplete(), load(), and any SharedPreferences usage in backup_reminder_provider.dart to replace persistence calls with the Sembast API and add the await/gating logic.Source: Coding guidelines
🧹 Nitpick comments (3)
lib/features/chat/widgets/trade_state_header.dart (1)
27-46: 🚀 Performance & Scalability | ⚡ Quick winReuse the cached trade lookup instead of calling
listTrades()on each refresh.Because this family watches
tradeStatusProvider(orderId), every status refresh re-executes the fallback path. Once the order has left the book, Line 40 reloads the full trades list on each run even thoughtradeInfoProvider/rawTradesProvideralready cache that lookup, which turns the sticky header into repeated bridge I/O for the same trade. Prefer the existing trades provider as the fallback source here.♻️ Possible simplification
- final trades = await orders_api.listTrades(); - final trade = trades.where((t) => t.order.id == orderId).firstOrNull; + final trade = await ref.watch(tradeInfoProvider(orderId).future); if (trade != null) return _toOrderItem(trade.order);🤖 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/chat/widgets/trade_state_header.dart` around lines 27 - 46, The fallback path in chatTradeOrderProvider currently calls orders_api.listTrades() on each re-run (triggered by tradeStatusProvider(orderId)); replace that external call with the cached provider lookup instead: read or watch the existing tradeInfoProvider or rawTradesProvider (whichever holds the list of trades) and find the trade by orderId, returning _toOrderItem(trade.order) if found; keep the prior checks against orderBookProvider and preserve the try/catch around the API call only if you still need a last-resort network lookup, but prefer the cached provider to avoid repeated bridge I/O.lib/features/notifications/widgets/notification_group_card.dart (1)
13-25: 🩺 Stability & Availability | ⚡ Quick winEnforce the non-empty
notificationscontract at construction time.The widget dereferences
notifications.first; add an assert so misuse fails fast with a clear message instead of runtimeRangeError.Suggested patch
class NotificationGroupCard extends StatefulWidget { const NotificationGroupCard({ super.key, required this.notifications, required this.onMarkRead, required this.onDelete, required this.onTapNotification, required this.onGoToTrade, this.isDisputeGroup = false, - }); + }) : assert(notifications.isNotEmpty, 'notifications must not be empty');Also applies to: 40-43
🤖 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/notifications/widgets/notification_group_card.dart` around lines 13 - 25, The constructor for NotificationGroupCard must enforce that the notifications list is non-empty to avoid RangeError when accessing notifications.first; add an assert in the NotificationGroupCard constructor (the const NotificationGroupCard(...) initializer) such as assert(notifications.isNotEmpty, 'NotificationGroupCard requires a non-empty notifications list') so misuse fails fast with a clear message; apply the same assert where the other constructor/initializer for this widget handles notifications (the other NotificationGroupCard constructor lines referenced around 40-43) to keep behavior consistent.lib/features/notifications/widgets/system_notification_banner.dart (1)
5-6: 📐 Maintainability & Code Quality | ⚡ Quick winExtract
relativeTimeinto a shared notifications utility module.Importing another widget file just for a formatter couples two UI components unnecessarily. Move
relativeTimeto a shared utility (for example,lib/features/notifications/utils/relative_time.dart) and import it from both widgets.🤖 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/notifications/widgets/system_notification_banner.dart` around lines 5 - 6, The import of relativeTime from notification_group_card.dart couples widgets; extract the relativeTime function into a shared notifications utility module (e.g., create a relative_time utility module) and update both widgets to import relativeTime from that new module; locate the current relativeTime definition in notification_group_card.dart, move its implementation into the new utility, export it, then replace the existing import with the new utility import in system_notification_banner and notification_group_card.
🤖 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 `@lib/features/account/providers/backup_reminder_provider.dart`:
- Around line 105-111: confirmBackupComplete() can race with the
constructor-triggered load() and have its writes overwritten; ensure
confirmBackupComplete awaits the initial load completion before mutating
prefs/state by adding and using a private completion marker (e.g., a Future or
Completer like _loadComplete or _initCompleter) that the constructor assigns to
the load() call (e.g., _loadComplete = load()) and that load() completes when
finished; then at the start of confirmBackupComplete() await that marker before
touching SharedPreferences and updating state (references:
confirmBackupComplete, load, constructor initialization).
In `@lib/features/account/screens/backup_ritual_screen.dart`:
- Around line 70-73: The dispose() implementation only nulls _words but leaves
other mnemonic-derived state in memory; update dispose() to also clear or null
out _filled, _options, and _wrongPick so all mnemonic fragments are scrubbed
when the screen is disposed (identify the dispose() method in
backup_ritual_screen.dart and set those fields to safe empty values or null
before calling super.dispose()).
In `@lib/features/account/widgets/backup_trigger_sheet.dart`:
- Around line 183-188: The onPressed handler currently calls
snoozeUntilTomorrow() on backupReminderProvider.notifier and immediately closes
the sheet; change it to await the async snoozeUntilTomorrow() call (from
BackupReminderNotifier or the notifier returned by
backupReminderProvider.notifier) before calling Navigator.of(context).pop(), and
handle errors from the awaited call (e.g., show an error SnackBar or keep the
sheet open) so persistence failures don’t silently close the UI.
- Around line 161-164: Replace the direct Navigator push of BackupRitualScreen
(navigator.push(MaterialPageRoute(...))) with the app's GoRouter/AppRoute
navigation: call context.push or context.go using the centralized route for the
backup ritual (e.g., the AppRoute/GoRoute entry that maps to BackupRitualScreen,
such as AppRoute.backupRitual or the route name "backupRitual"), and remove the
MaterialPageRoute usage; ensure you have the go_router import and use the
route's name/path consistent with your AppRoute definitions.
In `@lib/features/home/widgets/order_list_item.dart`:
- Around line 153-154: Replace the hardcoded caption 'Market price' in
order_list_item.dart with a localized string from AppLocalizations: add a new
key (e.g., marketPrice) to your .arb files with translations, run the
localization generation, and update the Text widget in the OrderListItem (or the
Text instance showing 'Market price') to use
AppLocalizations.of(context).marketPrice (or the generated getter name) instead
of the literal string so the label is internationalized.
In `@lib/features/notifications/screens/notifications_screen.dart`:
- Around line 146-152: System notification taps are routing to the fallback
snackbar because systemItems lack orderId/disputeId; change the onTap passed to
SystemNotificationBanner to be conditional so it becomes a no-op when there is
no concrete destination. Update the loop where SystemNotificationBanner is
created (for final n in systemItems) to pass onTap: (n.orderId == null &&
n.disputeId == null) ? null : () => _handleTap(context, n) (or an empty closure
if the banner API requires a non-null callback) so taps only invoke _handleTap
when a real route exists; leave onMarkRead/onDelete as-is. Ensure
SystemNotificationBanner accepts a nullable onTap or adapt to accept an empty
closure.
In `@lib/features/order/screens/add_order_screen.dart`:
- Around line 115-123: _applyPreset currently only sets
selectedPaymentMethodsProvider when parsed methods are non-empty and never
clears customPaymentMethodProvider, so stale payment selections leak into
_submit; update _applyPreset to explicitly clear both
selectedPaymentMethodsProvider and customPaymentMethodProvider when the preset
yields no methods (and in the branch that sets methods ensure
customPaymentMethodProvider is cleared or set appropriately), referencing the
selectedPaymentMethodsProvider, customPaymentMethodProvider, and _submit
identifiers so the paymentMethod payload is always derived only from the current
preset or explicit user input.
---
Outside diff comments:
In `@lib/features/account/providers/backup_reminder_provider.dart`:
- Line 2: The BackupReminderNotifier currently persists state using
SharedPreferences and updates state in confirmBackupComplete() without awaiting
load(), violating the UI-layer persistence guideline; migrate persistence from
SharedPreferences to the app's Sembast-backed storage (or add explicit
justification/documentation if SharedPreferences must be used) and update
BackupReminderNotifier/confirmBackupComplete() to await load() (or gate on the
notifier's _loaded flag) before writing persistence and setting state to avoid a
stale in-flight load overwriting the confirmation; locate
BackupReminderNotifier, confirmBackupComplete(), load(), and any
SharedPreferences usage in backup_reminder_provider.dart to replace persistence
calls with the Sembast API and add the await/gating logic.
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 77-80: The countdown is being seeded once in initState via
_loadExpiresAt() which calls getOrder() and sets _totalCountdownSeconds from the
current diff, causing wrong defaults and threshold resets when reopening; change
the logic so the countdown is driven by the authoritative trade timeout/phase
values from the trade object (use trade.expiresAt or trade.timeout/phaseTimeout)
instead of a one-time local diff, refresh that value whenever the trade
status/phase changes and on resume (hook into whatever trade update callback or
status stream you already have), and stop resetting _totalCountdownSeconds to
the instantaneous remaining diff on screen open—set _totalCountdownSeconds from
the configured phase timeout and compute remainingSeconds from expiresAt for
display so amber/lime/red thresholds remain stable (update _loadExpiresAt,
_startCountdown and the trade status change handler to pull and apply the
trade-side timeout).
- Around line 416-436: The code currently defaults isBuyer = true while
tradeRoleFromDbProvider is unresolved, causing seller trades to render as buyer;
change the logic in the trade detail screen where you compute isBuyer (using
tradeRoleProvider and tradeRoleFromDbProvider) so that if dbRole is null you
first try to derive the role from the live order (the local variable order from
orderBookProvider) and only if order is also unavailable set isBuyer to
null/unknown (or keep a neutral/loading state); update any downstream UI that
reads isBuyer to handle a nullable/unknown role (show loading/neutral CTA)
instead of assuming true. Reference symbols: tradeRoleProvider,
tradeRoleFromDbProvider, isBuyer, order, orderBookProvider.
---
Nitpick comments:
In `@lib/features/chat/widgets/trade_state_header.dart`:
- Around line 27-46: The fallback path in chatTradeOrderProvider currently calls
orders_api.listTrades() on each re-run (triggered by
tradeStatusProvider(orderId)); replace that external call with the cached
provider lookup instead: read or watch the existing tradeInfoProvider or
rawTradesProvider (whichever holds the list of trades) and find the trade by
orderId, returning _toOrderItem(trade.order) if found; keep the prior checks
against orderBookProvider and preserve the try/catch around the API call only if
you still need a last-resort network lookup, but prefer the cached provider to
avoid repeated bridge I/O.
In `@lib/features/notifications/widgets/notification_group_card.dart`:
- Around line 13-25: The constructor for NotificationGroupCard must enforce that
the notifications list is non-empty to avoid RangeError when accessing
notifications.first; add an assert in the NotificationGroupCard constructor (the
const NotificationGroupCard(...) initializer) such as
assert(notifications.isNotEmpty, 'NotificationGroupCard requires a non-empty
notifications list') so misuse fails fast with a clear message; apply the same
assert where the other constructor/initializer for this widget handles
notifications (the other NotificationGroupCard constructor lines referenced
around 40-43) to keep behavior consistent.
In `@lib/features/notifications/widgets/system_notification_banner.dart`:
- Around line 5-6: The import of relativeTime from notification_group_card.dart
couples widgets; extract the relativeTime function into a shared notifications
utility module (e.g., create a relative_time utility module) and update both
widgets to import relativeTime from that new module; locate the current
relativeTime definition in notification_group_card.dart, move its implementation
into the new utility, export it, then replace the existing import with the new
utility import in system_notification_banner and notification_group_card.
🪄 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: 29788c1c-f602-4fc5-adf6-a00c61e5399b
📒 Files selected for processing (17)
lib/core/app_theme.dartlib/features/account/providers/backup_reminder_provider.dartlib/features/account/screens/account_screen.dartlib/features/account/screens/backup_ritual_screen.dartlib/features/account/widgets/backup_trigger_sheet.dartlib/features/chat/screens/chat_room_screen.dartlib/features/chat/widgets/trade_state_header.dartlib/features/home/providers/order_reason_provider.dartlib/features/home/screens/home_screen.dartlib/features/home/widgets/order_list_item.dartlib/features/notifications/screens/notifications_screen.dartlib/features/notifications/widgets/notification_group_card.dartlib/features/notifications/widgets/system_notification_banner.dartlib/features/order/screens/add_order_screen.dartlib/features/order/screens/take_order_screen.dartlib/features/order/widgets/order_preset_selector.dartlib/features/trades/screens/trade_detail_screen.dart
Comment on lines
105
to
+111
| Future<void> confirmBackupComplete() async { | ||
| final prefs = await SharedPreferences.getInstance(); | ||
| await prefs.setBool(kBackupReminderDismissedKey, true); | ||
| await prefs.setBool(kBackupCompletedKey, true); | ||
| await prefs.remove(kBackupSnoozedUntilKey); | ||
| state = false; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent confirmBackupComplete() from racing with initial load().
Line 105 mutates persisted flags/state without awaiting load(). If constructor-triggered load() is still in flight, it can overwrite state after confirmation.
Suggested fix
Future<void> confirmBackupComplete() async {
+ await load();
final prefs = await SharedPreferences.getInstance();
await prefs.setBool(kBackupReminderDismissedKey, true);
await prefs.setBool(kBackupCompletedKey, true);
await prefs.remove(kBackupSnoozedUntilKey);
state = false;
}🤖 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/account/providers/backup_reminder_provider.dart` around lines
105 - 111, confirmBackupComplete() can race with the constructor-triggered
load() and have its writes overwritten; ensure confirmBackupComplete awaits the
initial load completion before mutating prefs/state by adding and using a
private completion marker (e.g., a Future or Completer like _loadComplete or
_initCompleter) that the constructor assigns to the load() call (e.g.,
_loadComplete = load()) and that load() completes when finished; then at the
start of confirmBackupComplete() await that marker before touching
SharedPreferences and updating state (references: confirmBackupComplete, load,
constructor initialization).
Comment on lines
+70
to
+73
| void dispose() { | ||
| // Drop the mnemonic from memory as soon as the ritual is left. | ||
| _words = null; | ||
| super.dispose(); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
dispose() clears _words but leaves other mnemonic-derived fields populated.
Line 72 nulls _words, but _filled, _options, and _wrongPick can still retain secret-word fragments. Clear them too during dispose for full in-memory scrubbing.
Suggested fix
`@override`
void dispose() {
// Drop the mnemonic from memory as soon as the ritual is left.
_words = null;
+ _challenge = const [];
+ _filled = [null, null, null];
+ _activeSlot = 0;
+ _options = const [];
+ _wrongPick = null;
super.dispose();
}🤖 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/account/screens/backup_ritual_screen.dart` around lines 70 - 73,
The dispose() implementation only nulls _words but leaves other mnemonic-derived
state in memory; update dispose() to also clear or null out _filled, _options,
and _wrongPick so all mnemonic fragments are scrubbed when the screen is
disposed (identify the dispose() method in backup_ritual_screen.dart and set
those fields to safe empty values or null before calling super.dispose()).
Comment on lines
+161
to
+164
| navigator.push( | ||
| MaterialPageRoute<void>( | ||
| builder: (_) => const BackupRitualScreen(), | ||
| ), |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether BackupRitualScreen already has a go_router route entry.
rg -n --type=dart "BackupRitualScreen|GoRoute|AppRoute|go_router|context\.push|context\.go"Repository: MostroP2P/app
Length of output: 14584
Switch BackupRitualScreen navigation to go_router
lib/features/account/widgets/backup_trigger_sheet.dart (lines 161-164) launches BackupRitualScreen via navigator.push(MaterialPageRoute(...)), bypassing the centralized GoRouter/AppRoute navigation used elsewhere (and required by the **/*.dart guidelines). Use the corresponding GoRoute/AppRoute and navigate with context.push/go instead.
🤖 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/account/widgets/backup_trigger_sheet.dart` around lines 161 -
164, Replace the direct Navigator push of BackupRitualScreen
(navigator.push(MaterialPageRoute(...))) with the app's GoRouter/AppRoute
navigation: call context.push or context.go using the centralized route for the
backup ritual (e.g., the AppRoute/GoRoute entry that maps to BackupRitualScreen,
such as AppRoute.backupRitual or the route name "backupRitual"), and remove the
MaterialPageRoute usage; ensure you have the go_router import and use the
route's name/path consistent with your AppRoute definitions.
Source: Coding guidelines
Comment on lines
+183
to
+188
| onPressed: () { | ||
| ref | ||
| .read(backupReminderProvider.notifier) | ||
| .snoozeUntilTomorrow(); | ||
| Navigator.of(context).pop(); | ||
| }, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Await snooze persistence before dismissing the sheet.
Lines 183-187 fire snoozeUntilTomorrow() and immediately pop. If persistence fails, the failure is unhandled and the UI still closes as if it succeeded.
Suggested fix
TextButton(
- onPressed: () {
- ref
- .read(backupReminderProvider.notifier)
- .snoozeUntilTomorrow();
- Navigator.of(context).pop();
+ onPressed: () async {
+ try {
+ await ref
+ .read(backupReminderProvider.notifier)
+ .snoozeUntilTomorrow();
+ if (context.mounted) Navigator.of(context).pop();
+ } catch (_) {
+ if (!context.mounted) return;
+ ScaffoldMessenger.of(context).showSnackBar(
+ const SnackBar(
+ content: Text('Could not snooze reminder. Please try again.'),
+ ),
+ );
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onPressed: () { | |
| ref | |
| .read(backupReminderProvider.notifier) | |
| .snoozeUntilTomorrow(); | |
| Navigator.of(context).pop(); | |
| }, | |
| onPressed: () async { | |
| try { | |
| await ref | |
| .read(backupReminderProvider.notifier) | |
| .snoozeUntilTomorrow(); | |
| if (context.mounted) Navigator.of(context).pop(); | |
| } catch (_) { | |
| if (!context.mounted) return; | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| const SnackBar( | |
| content: Text('Could not snooze reminder. Please try again.'), | |
| ), | |
| ); | |
| } | |
| }, |
🤖 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/account/widgets/backup_trigger_sheet.dart` around lines 183 -
188, The onPressed handler currently calls snoozeUntilTomorrow() on
backupReminderProvider.notifier and immediately closes the sheet; change it to
await the async snoozeUntilTomorrow() call (from BackupReminderNotifier or the
notifier returned by backupReminderProvider.notifier) before calling
Navigator.of(context).pop(), and handle errors from the awaited call (e.g., show
an error SnackBar or keep the sheet open) so persistence failures don’t silently
close the UI.
Comment on lines
+153
to
+154
| 'Market price', | ||
| style: TextStyle(color: textSec, fontSize: 11), |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Consider localizing the "Market price" caption.
The caption 'Market price' is hardcoded and should be added to AppLocalizations for proper internationalization support.
🌐 Proposed fix to use localized string
Text(
- 'Market price',
+ l10n.marketPriceCaption,
style: TextStyle(color: textSec, fontSize: 11),
),You'll need to add the corresponding key to your .arb localization files.
🤖 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/home/widgets/order_list_item.dart` around lines 153 - 154,
Replace the hardcoded caption 'Market price' in order_list_item.dart with a
localized string from AppLocalizations: add a new key (e.g., marketPrice) to
your .arb files with translations, run the localization generation, and update
the Text widget in the OrderListItem (or the Text instance showing 'Market
price') to use AppLocalizations.of(context).marketPrice (or the generated getter
name) instead of the literal string so the label is internationalized.
Comment on lines
+146
to
+152
| for (final n in systemItems) ...[ | ||
| SystemNotificationBanner( | ||
| notification: n, | ||
| onMarkRead: () => notifier.markAsRead(n.id), | ||
| onDelete: () => notifier.delete(n.id), | ||
| onTap: () => _handleTap(context, n), | ||
| ), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid wiring dead-end taps for non-routable system notifications.
System items are built from notifications without orderId/disputeId, so this onTap currently routes users into the fallback snackbar path. Make taps conditional (or no-op) unless a concrete destination exists.
Suggested patch
SystemNotificationBanner(
notification: n,
onMarkRead: () => notifier.markAsRead(n.id),
onDelete: () => notifier.delete(n.id),
- onTap: () => _handleTap(context, n),
+ onTap: (n.orderId != null || n.disputeId != null)
+ ? () => _handleTap(context, n)
+ : null,
),🤖 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/notifications/screens/notifications_screen.dart` around lines
146 - 152, System notification taps are routing to the fallback snackbar because
systemItems lack orderId/disputeId; change the onTap passed to
SystemNotificationBanner to be conditional so it becomes a no-op when there is
no concrete destination. Update the loop where SystemNotificationBanner is
created (for final n in systemItems) to pass onTap: (n.orderId == null &&
n.disputeId == null) ? null : () => _handleTap(context, n) (or an empty closure
if the banner API requires a non-null callback) so taps only invoke _handleTap
when a real route exists; leave onMarkRead/onDelete as-is. Ensure
SystemNotificationBanner accepts a nullable onTap or adapt to accept an empty
closure.
Comment on lines
+115
to
+123
| final methods = source.paymentMethod | ||
| .split(',') | ||
| .map((m) => m.trim()) | ||
| .where((m) => m.isNotEmpty) | ||
| .toList(); | ||
| if (methods.isNotEmpty) { | ||
| ref.read(selectedPaymentMethodsProvider.notifier).state = methods; | ||
| } | ||
| ref.read(isMarketPriceProvider.notifier).state = true; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear stale payment method state when applying the Express preset
_applyPreset only sets selectedPaymentMethodsProvider when parsed methods are non-empty and never clears customPaymentMethodProvider. As a result, previous custom/selected methods can leak into _submit and produce an incorrect paymentMethod payload.
Proposed fix
final methods = source.paymentMethod
.split(',')
.map((m) => m.trim())
.where((m) => m.isNotEmpty)
.toList();
- if (methods.isNotEmpty) {
- ref.read(selectedPaymentMethodsProvider.notifier).state = methods;
- }
+ ref.read(selectedPaymentMethodsProvider.notifier).state = methods;
+ ref.read(customPaymentMethodProvider.notifier).state = '';
ref.read(isMarketPriceProvider.notifier).state = true;
ref.read(premiumValueProvider.notifier).state =
source.premium.clamp(-10.0, 10.0);Also applies to: 126-127, 151-175
🤖 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/order/screens/add_order_screen.dart` around lines 115 - 123,
_applyPreset currently only sets selectedPaymentMethodsProvider when parsed
methods are non-empty and never clears customPaymentMethodProvider, so stale
payment selections leak into _submit; update _applyPreset to explicitly clear
both selectedPaymentMethodsProvider and customPaymentMethodProvider when the
preset yields no methods (and in the branch that sets methods ensure
customPaymentMethodProvider is cleared or set appropriately), referencing the
selectedPaymentMethodsProvider, customPaymentMethodProvider, and _submit
identifiers so the paymentMethod payload is always derived only from the current
preset or explicit user input.
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
Implements the UX redesign proposals from the Claude Design handoff bundle (Mostro UX Redesign EN) onto the existing screens. The bundle's design doc prioritized proposals #1, #4 and #5; proposals #2 (first-trade onboarding) and #9 (ephemeral identity explainer) were not mocked in the canvas and are intentionally out of scope.
Per proposal
trade_detail_screen.dartrebuilt: one large state-dependent primary CTA (Add invoice / Pay hold invoice / Mark fiat sent / Confirm & release / Rate), waiting-on-counterpart states render a disabled spinner button, Cancel / Dispute / Release collapsed behind the app-bar ⋮ overflow menu, step timeline card, and a persistent chat chip (counterpart handle + unread badge) replacing the ghost CONTACT button.★ 4.9 · 47 trades · 312 days).Theme
New
warningAmbertoken inAppColors; everything else uses existing tokens. No new dependencies, no.arbchanges (new strings follow each file's existing hardcoded-English pattern).Test plan
flutter analyze— clean (3 remaining infos are pre-existing in untouched files)flutter test— passes🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Refactor