004 mostro p2p client - #54
Merged
Merged
Conversation
Rust: - api/nostr.rs: relay management facade (initialize, add/remove relay, connection state, streams) over RelayPool singleton - api/orders.rs: order book read path with OrderFilters, in-memory cache, upsert, and on_orders_updated stream - order_events.rs: already complete from Phase 3 (Kind 38383 parsing) Dart: - HomeScreen: AppBar (hamburger, Mostro logo with 500ms happy face, notification bell), BUY/SELL BTC tabs, filter pill, order list with pull-to-refresh, FAB, drawer overlay - OrderListItem: 5-row card (status pill, fiat amount+flag, premium, payment methods, star rating+trade count) - home_order_providers: mock data, OrderType tabs, 4 filter providers, filteredOrdersProvider - OrderFilter dialog: currency/payment chips, rating/premium sliders - BottomNavBar: 3 tabs with red dot badges - DrawerMenu: 70% width overlay, mascot header, Account/Settings/About - app_routes: wired HomeScreen at '/' route
…ility, safety Rust: - nostr.rs: handle RecvError::Lagged by looping instead of terminating streams; use get_or_try_init for atomic pool initialization - orders.rs: same Lagged handling for OrdersStream Dart: - drawer_menu: wrap InkWell with Material for ripple + Semantics for screen readers; null-safe fallback for headlineLarge text style - order_list_item: add HitTestBehavior.opaque to GestureDetector - home_order_providers: assert fiatAmount non-null in displayAmount; fix inaccurate "sorted" docstring - bottom_nav_bar: add default case with assert to switch
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 24 minutes and 26 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 a HomeScreen UI (tabs, list, drawer, FAB, bottom nav), new order-list widgets and Riverpod providers for filtering, fiat currency loaders, an order-filter dialog, drawer UI, and Rust APIs for Nostr relay management and a cached orders read path with subscription streams. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Flutter as Flutter App
participant Home as HomeScreen UI
participant Providers as Riverpod Providers
participant RustOrders as rust::api::orders
participant OrderCache as ORDER_BOOK
User->>Home: open order book
Home->>Providers: read filteredOrdersProvider
Providers->>RustOrders: get_orders(filters)
RustOrders->>OrderCache: read cached orders (RwLock)
OrderCache-->>RustOrders: filtered & sorted Vec<OrderInfo>
RustOrders-->>Providers: return List<OrderItem>
Providers-->>Home: provide filtered list
Home->>User: render OrderListItem(s)
sequenceDiagram
participant Flutter as Flutter App
participant NostrAPI as rust::api::nostr
participant RelayPool as Relay Pool
participant Relays as Nostr Relays
Flutter->>NostrAPI: initialize(relays)
NostrAPI->>RelayPool: init & connect
RelayPool->>Relays: open connections
Relays-->>RelayPool: status updates
RelayPool-->>NostrAPI: broadcast status
NostrAPI-->>Flutter: stream receiver (on_connection_state_changed)
loop subscription
Flutter->>NostrAPI: stream.next()
NostrAPI->>RelayPool: await broadcast
RelayPool-->>Flutter: ConnectionState
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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
🧹 Nitpick comments (5)
lib/features/drawer/screens/drawer_menu.dart (1)
64-65: Minor: Null coalescing chain has a potential null assertion risk.If
theme.textTheme.headlineLargeis null andheadlineMediumis also null, the!assertion will throw. While unlikely with standard Material themes, consider a safer fallback:style: (theme.textTheme.headlineLarge ?? theme.textTheme.headlineMedium ?? const TextStyle(fontSize: 24, fontWeight: FontWeight.bold)) .copyWith(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/drawer/screens/drawer_menu.dart` around lines 64 - 65, The null-coalescing chain using theme.textTheme.headlineLarge ?? theme.textTheme.headlineMedium! can throw if both are null; update the style expression in DrawerMenu (where you reference theme.textTheme.headlineLarge and headlineMedium) to provide a safe fallback TextStyle (e.g., const TextStyle(fontSize:24, fontWeight: FontWeight.bold)) before calling copyWith so you never use a null-assertion; ensure the final expression is (theme.textTheme.headlineLarge ?? theme.textTheme.headlineMedium ?? fallbackTextStyle).copyWith(...)lib/shared/widgets/order_filter.dart (1)
7-20: Consider loading currency and payment method options from a shared data source.The hardcoded
_currenciesand_paymentMethodslists duplicate data that should ideally come fromassets/data/fiat.json(for currencies) and a centralized payment methods registry. This creates maintenance overhead if the supported options change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shared/widgets/order_filter.dart` around lines 7 - 20, The _currencies and _paymentMethods arrays in order_filter.dart are hardcoded and should be loaded from shared data sources; replace the static lists (_currencies and _paymentMethods) with runtime-loaded values by reading currencies from assets/data/fiat.json (e.g., via rootBundle.loadString and json decode) and retrieving payment methods from the centralized registry/service (or a shared PaymentsRepository), then update the OrderFilter widget to accept these lists via constructor parameters or a FutureBuilder so the chip selector consumes the loaded lists instead of the hardcoded constants. Ensure you reference and remove/replace the _currencies and _paymentMethods symbols and add error handling/defaults if the assets or registry call fail.lib/features/home/widgets/order_list_item.dart (1)
7-20: Currency flags duplicate data that should come from a shared source.Similar to
order_filter.dart, this hardcoded_currencyFlagsmap should ideally be derived fromassets/data/fiat.jsonto ensure consistency and reduce maintenance burden when currencies are added or removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/home/widgets/order_list_item.dart` around lines 7 - 20, Hardcoded _currencyFlags duplicates data that should come from the shared fiat.json; remove the inline _currencyFlags map in order_list_item.dart and instead load the currency-to-flag mapping from the same source used by order_filter.dart (reuse its parsing/utility function or create a shared loader that reads assets/data/fiat.json and exposes a Map<String,String>), update references to use that shared map, and ensure the asset is declared in pubspec so order_list_item.dart imports the shared provider/utility rather than maintaining its own copy.lib/shared/widgets/bottom_nav_bar.dart (1)
8-9: Provider state may desync from actual route on back navigation or deep links.
bottomNavIndexProvideris only updated when the user taps a tab (line 37), but not when:
- User navigates away via
context.push()and returns via back button- App receives a deep link to a different route
- User navigates via drawer menu
Consider using
GoRouterStateor a route observer to keep the index synchronized with the current location.💡 Example: Sync index with current route
// In build(), derive index from current route instead of provider: final location = GoRouterState.of(context).uri.path; final currentIndex = switch (location) { AppRoute.home => 0, AppRoute.orderBook => 1, AppRoute.chatList => 2, _ => 0, // fallback };Or use
GoRouter.of(context).routerDelegate.currentConfigurationwith a listener.Also applies to: 36-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shared/widgets/bottom_nav_bar.dart` around lines 8 - 9, The bottomNavIndexProvider is only set on tab taps and can desync from actual route on back navigation, deep links, or drawer navigation; update the BottomNavBar widget to derive the active index from the current route instead of relying solely on bottomNavIndexProvider—use GoRouterState.of(context).uri.path or GoRouter.of(context).routerDelegate.currentConfiguration (or add a route observer) inside the widget's build method to map routes (e.g., AppRoute.home, AppRoute.orderBook, AppRoute.chatList) to an index and remove or stop using the manual set on tap (the tap handler that updates bottomNavIndexProvider) so the shown tab always reflects the current location.rust/src/api/orders.rs (1)
55-96: Release the read lock before sorting.The
ordersguard stays in scope for the whole function, soresult.sort_by(...)still runs while the read lock is held. That blocksset_ordersandupsert_orderlonger than necessary on the hot read path.Suggested reshape
pub async fn get_orders(&self, filters: Option<OrderFilters>) -> Vec<OrderInfo> { - let orders = self.orders.read().await; - let mut result: Vec<OrderInfo> = orders - .iter() - .filter(|o| matches!(o.status, OrderStatus::Pending)) - .filter(|o| { - let Some(ref f) = filters else { return true }; - if let Some(ref kind) = f.kind { - if &o.kind != kind { - return false; - } - } - if let Some(ref code) = f.fiat_code { - if !code.is_empty() && o.fiat_code != *code { - return false; - } - } - if let Some(ref pm) = f.payment_method { - if !pm.is_empty() - && !o.payment_method.to_lowercase().contains(&pm.to_lowercase()) - { - return false; - } - } - true - }) - .cloned() - .collect(); + let mut result: Vec<OrderInfo> = { + let orders = self.orders.read().await; + orders + .iter() + .filter(|o| matches!(o.status, OrderStatus::Pending)) + .filter(|o| { + let Some(ref f) = filters else { return true }; + if let Some(ref kind) = f.kind { + if &o.kind != kind { + return false; + } + } + if let Some(ref code) = f.fiat_code { + if !code.is_empty() && o.fiat_code != *code { + return false; + } + } + if let Some(ref pm) = f.payment_method { + if !pm.is_empty() + && !o.payment_method.to_lowercase().contains(&pm.to_lowercase()) + { + return false; + } + } + true + }) + .cloned() + .collect() + }; // Sort by ascending expiration (soonest-expiring first), then by // descending created_at for orders without expiration. result.sort_by(|a, b| {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/orders.rs` around lines 55 - 96, The read lock obtained by orders.read().await in get_orders remains held through result.sort_by(...) and blocks writers; fix it by dropping the read guard immediately after collecting the cloned Vec (e.g., call drop(orders) or limit the scope so the read guard is released) before calling result.sort_by; reference get_orders, orders.read().await, result.collect(), and result.sort_by(), and ensure set_orders/upsert_order can acquire the write lock without being blocked.
🤖 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/home/providers/home_order_providers.dart`:
- Around line 163-165: orderBookProvider currently returns the static
_mockOrders list so invalidation on pull-to-refresh does nothing; replace the
static Provider<List<OrderItem>> orderBookProvider with a provider that actually
fetches or streams orders from the Rust bridge (e.g., a StreamProvider or
FutureProvider that calls your Rust fetchOrders/fetchOrderStream function) so
invalidation triggers real network/bridge work, or remove the pull-to-refresh UI
until that bridge-backed provider (e.g., fetchOrdersFromRust or orderStream) is
implemented; update consumers expecting List<OrderItem> to handle the provider's
AsyncValue/stream type if needed.
- Around line 33-48: The OrderItem constructor currently allows inconsistent
amount states (fiatAmount and fiatAmountMin/fiatAmountMax can be mixed or all
null); add input validation in the OrderItem constructor (or convert to factory)
that asserts exactly one shape is present: either fiatAmount != null and both
fiatAmountMin and fiatAmountMax == null, or fiatAmount == null and both
fiatAmountMin != null and fiatAmountMax != null; throw/assert on violation so
the model fails fast. Update any related constructors/creators referenced in the
class (e.g., the secondary constructor/block around lines 65-73) and adjust
displayAmount to compute from the validated shape (use fiatAmount when present,
otherwise use a formatted range from fiatAmountMin/fiatAmountMax) instead of
falling back to 0.
In `@lib/shared/widgets/bottom_nav_bar.dart`:
- Around line 41-42: The "My Trades" tab in bottom_nav_bar.dart currently routes
index 1 to AppRoute.orderBook which points to the public "Order Book"; update
the navigation to match the tab intent by either (A) adding a new route constant
(e.g., AppRoute.myTrades) and wiring it to the user's trades screen, then change
the case 1 navigation to context.go(AppRoute.myTrades), or (B) if you prefer to
keep the public order book, rename the tab label from "My Trades" to "Order
Book" so the UI matches AppRoute.orderBook; locate the switch/case handling tab
index 1 in the widget (case 1) and update the route constant or the tab label
accordingly.
In `@lib/shared/widgets/order_filter.dart`:
- Line 88: The selectedColor assignment in order_filter.dart uses
Color.withValues(alpha:), which requires Flutter >=3.27; either update the
project's Flutter SDK constraint in pubspec.yaml to flutter: ">=3.27.0" or
change the usage to the legacy API (e.g., replace green.withValues(alpha: 0.2)
with green.withOpacity(0.2)) where selectedColor is set so the code remains
compatible with older Flutter 3.x versions.
In `@rust/src/api/nostr.rs`:
- Around line 61-67: The flush_message_queue function must not return Ok(0) when
it's unimplemented; update flush_message_queue to return an explicit error (e.g.
Err(anyhow::anyhow!("flush_message_queue not implemented: queue persistence not
wired in Phase 7"))) instead of Ok(0), keeping the existing pool() call and
signature; this makes callers (and the Dart UI) see a clear failure until the
actual flush wiring is implemented.
- Around line 22-35: The initialize function passes potentially blank entries
through because filter(|v| !v.is_empty()) only rejects an entirely empty Vec;
update the relays handling so you trim each String and remove empty strings
before deciding to use defaults: transform the Option<Vec<String>> by mapping
each entry with .trim() -> String and filtering out empty results, then if the
resulting Vec is empty call default_relays(), and feed that cleaned Vec into
RelayPool::new; adjust the code around the symbols initialize, relays,
default_relays, and POOL.get_or_try_init(|| async { RelayPool::new(urls).await
}) to use the cleaned list.
---
Nitpick comments:
In `@lib/features/drawer/screens/drawer_menu.dart`:
- Around line 64-65: The null-coalescing chain using
theme.textTheme.headlineLarge ?? theme.textTheme.headlineMedium! can throw if
both are null; update the style expression in DrawerMenu (where you reference
theme.textTheme.headlineLarge and headlineMedium) to provide a safe fallback
TextStyle (e.g., const TextStyle(fontSize:24, fontWeight: FontWeight.bold))
before calling copyWith so you never use a null-assertion; ensure the final
expression is (theme.textTheme.headlineLarge ?? theme.textTheme.headlineMedium
?? fallbackTextStyle).copyWith(...)
In `@lib/features/home/widgets/order_list_item.dart`:
- Around line 7-20: Hardcoded _currencyFlags duplicates data that should come
from the shared fiat.json; remove the inline _currencyFlags map in
order_list_item.dart and instead load the currency-to-flag mapping from the same
source used by order_filter.dart (reuse its parsing/utility function or create a
shared loader that reads assets/data/fiat.json and exposes a
Map<String,String>), update references to use that shared map, and ensure the
asset is declared in pubspec so order_list_item.dart imports the shared
provider/utility rather than maintaining its own copy.
In `@lib/shared/widgets/bottom_nav_bar.dart`:
- Around line 8-9: The bottomNavIndexProvider is only set on tab taps and can
desync from actual route on back navigation, deep links, or drawer navigation;
update the BottomNavBar widget to derive the active index from the current route
instead of relying solely on bottomNavIndexProvider—use
GoRouterState.of(context).uri.path or
GoRouter.of(context).routerDelegate.currentConfiguration (or add a route
observer) inside the widget's build method to map routes (e.g., AppRoute.home,
AppRoute.orderBook, AppRoute.chatList) to an index and remove or stop using the
manual set on tap (the tap handler that updates bottomNavIndexProvider) so the
shown tab always reflects the current location.
In `@lib/shared/widgets/order_filter.dart`:
- Around line 7-20: The _currencies and _paymentMethods arrays in
order_filter.dart are hardcoded and should be loaded from shared data sources;
replace the static lists (_currencies and _paymentMethods) with runtime-loaded
values by reading currencies from assets/data/fiat.json (e.g., via
rootBundle.loadString and json decode) and retrieving payment methods from the
centralized registry/service (or a shared PaymentsRepository), then update the
OrderFilter widget to accept these lists via constructor parameters or a
FutureBuilder so the chip selector consumes the loaded lists instead of the
hardcoded constants. Ensure you reference and remove/replace the _currencies and
_paymentMethods symbols and add error handling/defaults if the assets or
registry call fail.
In `@rust/src/api/orders.rs`:
- Around line 55-96: The read lock obtained by orders.read().await in get_orders
remains held through result.sort_by(...) and blocks writers; fix it by dropping
the read guard immediately after collecting the cloned Vec (e.g., call
drop(orders) or limit the scope so the read guard is released) before calling
result.sort_by; reference get_orders, orders.read().await, result.collect(), and
result.sort_by(), and ensure set_orders/upsert_order can acquire the write lock
without being blocked.
🪄 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: 6acfecdc-4dc4-481f-bd5c-e6555d7aa7e6
📒 Files selected for processing (11)
lib/core/app_routes.dartlib/features/drawer/screens/drawer_menu.dartlib/features/home/providers/home_order_providers.dartlib/features/home/screens/home_screen.dartlib/features/home/widgets/order_list_item.dartlib/shared/widgets/bottom_nav_bar.dartlib/shared/widgets/order_filter.dartrust/src/api/mod.rsrust/src/api/nostr.rsrust/src/api/orders.rsspecs/004-mostro-p2p-client/tasks.md
Comment on lines
+41
to
+42
| case 1: | ||
| context.go(AppRoute.orderBook); |
Contributor
There was a problem hiding this comment.
Route mismatch: "My Trades" tab navigates to public order book route.
The "My Trades" tab (index 1, line 66) navigates to AppRoute.orderBook, but per app_routes.dart line 97-98, this route currently shows a stub labeled "Order Book" (the public order book). This creates user confusion—the tab label says "My Trades" but the destination is the same public order book, not the user's personal/active trades.
Consider either:
- Creating a dedicated
/my_tradesroute for personal trades, or - Renaming the tab to match the actual destination
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/shared/widgets/bottom_nav_bar.dart` around lines 41 - 42, The "My Trades"
tab in bottom_nav_bar.dart currently routes index 1 to AppRoute.orderBook which
points to the public "Order Book"; update the navigation to match the tab intent
by either (A) adding a new route constant (e.g., AppRoute.myTrades) and wiring
it to the user's trades screen, then change the case 1 navigation to
context.go(AppRoute.myTrades), or (B) if you prefer to keep the public order
book, rename the tab label from "My Trades" to "Order Book" so the UI matches
AppRoute.orderBook; locate the switch/case handling tab index 1 in the widget
(case 1) and update the route constant or the tab label accordingly.
| return FilterChip( | ||
| label: Text(code), | ||
| selected: selected, | ||
| selectedColor: green.withValues(alpha: 0.2), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What Flutter version introduced Color.withValues method?
💡 Result:
Flutter version 3.27.0 introduced the Color.withValues method.
Citations:
- 1: The Color.withValues method was introduced in version 3.27.0. hasanmhallak/date_picker#35
- 2: https://stackoverflow.com/questions/79481509/the-method-withvalues-isnt-defined-for-the-type-color
- 3: https://docs.flutter.dev/release/breaking-changes/wide-gamut-framework
- 4: https://docs.flutter.dev/release/release-notes/release-notes-3.27.0
🏁 Script executed:
find . -maxdepth 2 -name "pubspec.yaml" -type f | head -5Repository: MostroP2P/app
Length of output: 99
🏁 Script executed:
cat ./pubspec.yaml | head -50Repository: MostroP2P/app
Length of output: 1064
🏁 Script executed:
grep -A 5 "^environment:" ./pubspec.yamlRepository: MostroP2P/app
Length of output: 136
🏁 Script executed:
grep -i "flutter" ./pubspec.yaml | grep -E "(version|sdk:)" | head -20Repository: MostroP2P/app
Length of output: 107
🏁 Script executed:
cat ./pubspec.yaml | grep -E "(sdk:|flutter:)" -A 1 -B 1Repository: MostroP2P/app
Length of output: 360
🏁 Script executed:
# Check if there's an explicit flutter version constraint anywhere in pubspec.yaml
cat ./pubspec.yamlRepository: MostroP2P/app
Length of output: 1298
🏁 Script executed:
# Check if there are any CI/CD or version constraint files
ls -la | grep -E "(\.github|\.flutter|\.pubspec|constraint|version|workflow)"Repository: MostroP2P/app
Length of output: 39
🏁 Script executed:
# Check for GitHub workflows or other configuration that might specify Flutter version
find . -name "*.yml" -o -name "*.yaml" | grep -E "(workflow|github|ci|action)" | head -10Repository: MostroP2P/app
Length of output: 39
🏁 Script executed:
# Check if there's any Flutter version constraint mentioned in analysis or other files
find . -type f \( -name "*.md" -o -name "README*" -o -name "CLAUDE.md" \) | xargs grep -l -i "flutter" 2>/dev/null | head -5Repository: MostroP2P/app
Length of output: 190
🏁 Script executed:
# Check CLAUDE.md for any version guidance
cat ./.specify/README.md | head -30Repository: MostroP2P/app
Length of output: 1329
Update Flutter version constraint or use legacy API.
Color.withValues(alpha:) requires Flutter 3.27 or later. The project's pubspec.yaml does not specify an explicit minimum Flutter version, only a Dart constraint (>=3.3.0). To use this API safely, either:
- Add
flutter: ">=3.27.0"to theenvironmentsection inpubspec.yaml, or - Replace with the legacy
withOpacity()method for compatibility with earlier Flutter 3.x versions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/shared/widgets/order_filter.dart` at line 88, The selectedColor
assignment in order_filter.dart uses Color.withValues(alpha:), which requires
Flutter >=3.27; either update the project's Flutter SDK constraint in
pubspec.yaml to flutter: ">=3.27.0" or change the usage to the legacy API (e.g.,
replace green.withValues(alpha: 0.2) with green.withOpacity(0.2)) where
selectedColor is set so the code remains compatible with older Flutter 3.x
versions.
fiat data, lock scope, relay input sanitization Rust: - nostr.rs: trim+filter blank relay entries before initialization; flush_message_queue returns explicit NotImplemented error - orders.rs: drop read lock before sorting to unblock writers Dart: - OrderItem: constructor assert enforces exactly one amount shape (fixed xor range); displayAmount uses non-null assertion - HomeScreen: remove pull-to-refresh (no-op on mock provider); pass currencyFlags from shared provider to OrderListItem - OrderListItem: accept currencyFlags map via constructor instead of hardcoded inline map - fiat_currencies.dart: new shared utility loading assets/data/fiat.json via Riverpod (currencyFlagsProvider, availableCurrencyCodesProvider) - order_filter.dart: currency chips sourced from fiat.json provider with fallback to hardcoded top currencies - drawer_menu.dart: fully null-safe TextStyle fallback chain
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/features/home/providers/home_order_providers.dart (1)
48-52:⚠️ Potential issue | 🟠 Major
assert-only model invariants are not sufficient for runtime safety.Lines 48-52 validate amount shape only with
assert; in release builds this won’t enforce the invariant, while Line 75 still force-unwrapsfiatAmount!. Convert to runtime validation (ArgumentError) in a non-const constructor/factory.🛠️ Proposed fix
-class OrderItem { - const OrderItem({ +class OrderItem { + OrderItem({ required this.id, required this.kind, this.fiatAmount, this.fiatAmountMin, this.fiatAmountMax, @@ - }) : assert( - (fiatAmount != null && fiatAmountMin == null && fiatAmountMax == null) || - (fiatAmount == null && fiatAmountMin != null && fiatAmountMax != null), - 'OrderItem must have either fiatAmount or both fiatAmountMin and fiatAmountMax', - ); + }) { + final hasSingle = + fiatAmount != null && fiatAmountMin == null && fiatAmountMax == null; + final hasRange = + fiatAmount == null && fiatAmountMin != null && fiatAmountMax != null; + if (!hasSingle && !hasRange) { + throw ArgumentError( + 'OrderItem must have either fiatAmount or both fiatAmountMin and fiatAmountMax', + ); + } + }#!/bin/bash # Verify whether changing from const constructor affects call sites. rg -n --type dart '\bconst\s+OrderItem\s*\('Also applies to: 71-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/home/providers/home_order_providers.dart` around lines 48 - 52, The constructor for OrderItem currently enforces the fiatAmount vs fiatAmountMin/fiatAmountMax invariant only via assert (so it can be bypassed in release) but later code force-unwraps fiatAmount!, causing runtime crashes; change the constructor (remove const if present) to perform runtime validation and throw an ArgumentError (or ArgumentError.value) when the shape is invalid, in the OrderItem constructor/factory that initializes fiatAmount, fiatAmountMin, fiatAmountMax (and similarly for the other constructor/overload around lines 71-75), and update any call sites that relied on a const constructor if necessary.
🧹 Nitpick comments (1)
lib/shared/widgets/order_filter.dart (1)
70-73: Avoid duplicating filter default literals in Reset logic.Lines 70-73 hardcode values that must stay in lockstep with provider defaults in
lib/features/home/providers/home_order_providers.dart. Prefer shared constants to prevent silent drift.♻️ Suggested direction
- ref.read(ratingFilterProvider.notifier).state = - (min: 0.0, max: 5.0); - ref.read(premiumRangeFilterProvider.notifier).state = - (min: -10.0, max: 10.0); + ref.read(ratingFilterProvider.notifier).state = defaultRatingRange; + ref.read(premiumRangeFilterProvider.notifier).state = defaultPremiumRange;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shared/widgets/order_filter.dart` around lines 70 - 73, The reset logic in order_filter.dart sets ratingFilterProvider and premiumRangeFilterProvider states with hardcoded tuples (min: 0.0, max: 5.0) and (min: -10.0, max: 10.0) which duplicate the canonical defaults in home_order_providers.dart; instead, extract the default ranges into shared constants (e.g., DEFAULT_RATING_RANGE, DEFAULT_PREMIUM_RANGE) exported from the providers module (or a new constants file) and replace the inline literals in the reset code to use those constants so the reset logic always stays in sync with Rating/Premium provider defaults.
🤖 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/home/providers/home_order_providers.dart`:
- Around line 190-194: The current payment-method filter uses substring matching
on o.paymentMethod which causes false positives; update the check in the block
that references selectedPaymentMethods and o.paymentMethod to tokenize and
normalize actual payment method tokens (e.g., split o.paymentMethod on common
delimiters like commas/semicolons/spaces, trim and toLowerCase each token) and
then test that any selectedPaymentMethods.toLowerCase() exactly equals one of
those normalized tokens (use a Set for efficient lookup) instead of using
String.contains.
---
Duplicate comments:
In `@lib/features/home/providers/home_order_providers.dart`:
- Around line 48-52: The constructor for OrderItem currently enforces the
fiatAmount vs fiatAmountMin/fiatAmountMax invariant only via assert (so it can
be bypassed in release) but later code force-unwraps fiatAmount!, causing
runtime crashes; change the constructor (remove const if present) to perform
runtime validation and throw an ArgumentError (or ArgumentError.value) when the
shape is invalid, in the OrderItem constructor/factory that initializes
fiatAmount, fiatAmountMin, fiatAmountMax (and similarly for the other
constructor/overload around lines 71-75), and update any call sites that relied
on a const constructor if necessary.
---
Nitpick comments:
In `@lib/shared/widgets/order_filter.dart`:
- Around line 70-73: The reset logic in order_filter.dart sets
ratingFilterProvider and premiumRangeFilterProvider states with hardcoded tuples
(min: 0.0, max: 5.0) and (min: -10.0, max: 10.0) which duplicate the canonical
defaults in home_order_providers.dart; instead, extract the default ranges into
shared constants (e.g., DEFAULT_RATING_RANGE, DEFAULT_PREMIUM_RANGE) exported
from the providers module (or a new constants file) and replace the inline
literals in the reset code to use those constants so the reset logic always
stays in sync with Rating/Premium provider defaults.
🪄 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: ff7632fa-b365-4171-81f4-7362d2b5db90
📒 Files selected for processing (8)
lib/features/drawer/screens/drawer_menu.dartlib/features/home/providers/home_order_providers.dartlib/features/home/screens/home_screen.dartlib/features/home/widgets/order_list_item.dartlib/shared/utils/fiat_currencies.dartlib/shared/widgets/order_filter.dartrust/src/api/nostr.rsrust/src/api/orders.rs
✅ Files skipped from review due to trivial changes (2)
- lib/features/drawer/screens/drawer_menu.dart
- rust/src/api/orders.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/home/screens/home_screen.dart
- rust/src/api/nostr.rs
runtime constructor validation, shared filter defaults - OrderItem: replace const+assert with runtime ArgumentError for fiatAmount/range shape validation (assert stripped in release) - Payment method filter: tokenize on commas with exact-match instead of substring contains to prevent false positives - Extract defaultRatingRange and defaultPremiumRange constants; use them in both provider declarations and filter reset handler
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/shared/widgets/order_filter.dart (1)
92-92:⚠️ Potential issue | 🟠 MajorVerify Flutter SDK compatibility for
Color.withValues.
green.withValues(alpha: 0.2)is only available in newer Flutter 3.x releases. If your minimum Flutter SDK is below that, this will break builds. Either pin Flutter to a compatible minimum or switch to a broadly compatible API.#!/bin/bash # Verify declared SDK constraints and API usage compatibility. fd -i pubspec.yaml rg -n "environment:|sdk:|flutter:" pubspec.yaml -A6 -B2 rg -n "withValues\\(" --type=dartExpected result: if
withValues(is used, ensure Flutter constraint is explicitly compatible (e.g., >=3.27.0).
As per coding guidelines "Dart/Flutter UI must use Dart 3.x and Flutter 3.x".Also applies to: 117-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shared/widgets/order_filter.dart` at line 92, The use of green.withValues(alpha: 0.2) is only available in newer Flutter 3.x releases; update the code to a broadly compatible API or the SDK constraint. Replace the call to green.withValues(alpha: 0.2) (used for selectedColor in the widget) with green.withOpacity(0.2) to maintain compatibility with older Flutter 3.x versions, or alternatively update the pubspec SDK constraint to a Flutter version that supports Color.withValues (e.g., >=3.27.0) so the API is allowed.lib/features/home/providers/home_order_providers.dart (1)
55-57:⚠️ Potential issue | 🟠 MajorConstructor invariant still allows partial mixed amount states.
Current validation allows invalid inputs like
fiatAmount+ only one offiatAmountMin/fiatAmountMax. Enforce exactly one complete shape.Proposed fix
- final hasFixed = fiatAmount != null; - final hasRange = fiatAmountMin != null && fiatAmountMax != null; - if (hasFixed == hasRange) { + final fixedOnly = + fiatAmount != null && fiatAmountMin == null && fiatAmountMax == null; + final rangeOnly = + fiatAmount == null && fiatAmountMin != null && fiatAmountMax != null; + if (!(fixedOnly || rangeOnly)) { throw ArgumentError( 'OrderItem must have either fiatAmount or both ' - 'fiatAmountMin and fiatAmountMax, not ${hasFixed ? "both" : "neither"}', + 'fiatAmountMin and fiatAmountMax, but not a mixed/partial shape', ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/home/providers/home_order_providers.dart` around lines 55 - 57, The constructor currently checks hasFixed == hasRange which still permits partial states (e.g. fiatAmount set with fiatAmountMin null and fiatAmountMax set). Replace this with an explicit validation that enforces exactly one complete shape: either a fixed amount (fiatAmount != null and both fiatAmountMin and fiatAmountMax are null) or a full range (fiatAmount == null and fiatAmountMin != null and fiatAmountMax != null); if neither holds, throw/raise an error. Update the logic around hasFixed/hasRange (and any error message) to reflect these exact invariants for fiatAmount, fiatAmountMin, and fiatAmountMax.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/features/home/providers/home_order_providers.dart`:
- Around line 55-57: The constructor currently checks hasFixed == hasRange which
still permits partial states (e.g. fiatAmount set with fiatAmountMin null and
fiatAmountMax set). Replace this with an explicit validation that enforces
exactly one complete shape: either a fixed amount (fiatAmount != null and both
fiatAmountMin and fiatAmountMax are null) or a full range (fiatAmount == null
and fiatAmountMin != null and fiatAmountMax != null); if neither holds,
throw/raise an error. Update the logic around hasFixed/hasRange (and any error
message) to reflect these exact invariants for fiatAmount, fiatAmountMin, and
fiatAmountMax.
In `@lib/shared/widgets/order_filter.dart`:
- Line 92: The use of green.withValues(alpha: 0.2) is only available in newer
Flutter 3.x releases; update the code to a broadly compatible API or the SDK
constraint. Replace the call to green.withValues(alpha: 0.2) (used for
selectedColor in the widget) with green.withOpacity(0.2) to maintain
compatibility with older Flutter 3.x versions, or alternatively update the
pubspec SDK constraint to a Flutter version that supports Color.withValues
(e.g., >=3.27.0) so the API is allowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1199b8e-8a14-485e-a9bf-7dbcf2fdfdf3
📒 Files selected for processing (2)
lib/features/home/providers/home_order_providers.dartlib/shared/widgets/order_filter.dart
…ates The previous hasFixed==hasRange check allowed orphaned fiatAmountMin or fiatAmountMax alongside fiatAmount. Now explicitly validates exactly one complete shape: fixed (fiatAmount only) or range (both min+max only).
This was referenced Mar 30, 2026
Merged
Merged
Merged
4 tasks
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