spec(phase18): add real order book bridge + shimmer loading phase - #70
Merged
Conversation
The previous 118 tasks left two concrete gaps: 1. orderBookProvider still uses mock data (no Kind 38383 subscription) 2. DESIGN_SYSTEM.md §9.1 shimmer never implemented Add Phase 18 (T126–T132) to tasks.md and a matching section in plan.md: - T126: shimmer: ^3.0.0 in pubspec.yaml - T127: OrderListSkeleton widget per DESIGN_SYSTEM.md §9.1 - T128: subscribe_orders() Rust loop (Kind 38383 via relay pool) - T129: on_orders_updated() FRB stream / OrdersStream wrapper - T130: wire subscription on ConnectionState::Online in nostr.rs - T131: replace mock Provider with real StreamProvider.autoDispose - T132: home screen shows shimmer on load, empty state, or real orders .specify/ files unchanged — PROTOCOL.md and DESIGN_SYSTEM.md were already correct. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add subscribe_orders() to rust/src/api/orders.rs: idempotent Kind 38383 subscription loop (AtomicBool guard) that parses incoming Nostr events via parse_order_event and upserts them into OrderBook, broadcasting to all OrdersStream subscribers - Wire subscribe_orders() call in nostr.rs ConnectionState::Online handler so the subscription starts on first relay connection - Add freezed/freezed_annotation/build_runner deps and run flutter_rust_bridge_codegen generate to produce lib/src/rust/ bindings - Initialize RustLib in main.dart before runApp - Replace mock orderBookProvider (Provider<List<OrderItem>> with hardcoded data) with StreamProvider.autoDispose backed by orders_api.onOrdersUpdated(); add OrderItem.fromInfo(OrderInfo) factory for the mapping - Update filteredOrdersProvider and take_order_screen to unwrap AsyncValue - Add OrderListSkeleton shimmer widget (DESIGN_SYSTEM.md §9.1): 5 placeholder cards, baseColor #1E2230, highlightColor #2A2D35 - Wire home_screen.dart orderBookProvider.when(loading: OrderListSkeleton, error: ..., data: orderContent) replacing the static Expanded child - Add loadingOrders l10n key to all 5 locales
- relay_management_card: use l10n keys for switch/toggle semantics labels and remove tooltip - home_screen: replace raw error string with friendly message + retry button (ref.invalidate) - about_screen: hideCurrentSnackBar before showing nested copy-link snackbar - currency_selector_dialog: remove redundant `c` alias, use `colors` directly - settings_screen: wrap InkWell in Material for proper ripple rendering - connect_wallet_screen: normalise NWC URI scheme to lowercase on QR scan and paste - countdown_timer: move onExpired callback outside setState to avoid callback-during-build - platform_aware_qr_scanner: add mounted check after async clipboard read - l10n (all 5 locales): add errorLoadingOrders, retry, disableRelayLabel, enableRelayLabel, removeRelayTooltip; fix de invalidLightningAddressFormat, it relayErrorUrlTooShort gender, es relayErrorDuplicate consistency - orders.rs: add ResetGuard (Drop impl) to reset SUBSCRIPTION_ACTIVE on panic
- get_active_keys() and get_active_trade_keys() are internal Rust helpers; mark pub(crate) so FRB codegen no longer generates bridge stubs that try to SSE-encode nostr_sdk::Keys (which has a crate version conflict) - process_order_event() is an internal Rust helper; mark pub(crate) - OrderBook::subscribe() returns broadcast::Receiver which is not serialisable to Dart; mark pub(crate) - Regenerate rust/src/frb_generated.rs — removes ~500 lines of stale bridge stubs for the above types, fixing the Android build failure
…x loading state - main.dart: call nostr_api.initialize(relays: null) after RustLib.init() so the relay pool is actually created — without this no relay connections were ever made and orders could never arrive - orderBookProvider: yield current cached snapshot immediately so the UI exits shimmer/loading state right away (empty list on first run → "no orders" instead of spinning forever); subsequent emissions stream live relay updates - lib.rs: add #[frb(init)] init_app() with android_logger setup (tag: mostro_rust) so Rust log output appears in adb logcat - nostr.rs: add log::info!/warn! to connection state watcher - orders.rs: add log::info!/debug!/error!/warn! to subscribe_orders() and the relay notification loop so every step is traceable in logcat - Cargo.toml: add log = "0.4" + android_logger = "0.14" (Android only)
The Mostro daemon is the author and publisher of Kind 38383 events — makers send a new-order NIP-59 gift-wrap to the node, and the node publishes the order signed with its own keypair. - pending_orders_filter() now takes &PublicKey and adds .author(mostro_pubkey) - _run_order_subscription() parses DEFAULT_MOSTRO_PUBKEY and passes it to filter - relay_pool.rs: fix pending_orders_filter() call to pass the pubkey - parse_order_event(): add z=order validation to skip non-Mostro 38383 events
- main.dart: log relay URLs + initial state after initialize(); spawn a background watcher that prints every ConnectionState change to flutter output — no adb logcat filter needed to see relay connectivity - relay_pool: add 500ms delay before first broadcast so WebSocket handshakes have time to complete (avoids always starting as Reconnecting) - relay_pool: reduce STATUS_POLL_INTERVAL from 5s → 2s so Online state is detected and subscribe_orders() fires sooner after relay connects
- order_events.rs: z=order check is now a debug log, not a rejection — the author filter already scopes to the trusted node; older events may lack this tag - orders.rs: log every received event (kind, author prefix) and the exact rejection reason when parse_order_event returns None (shows tag names) - main.dart: when state goes Online, log each relay URL+status and poll the order cache after 5s to confirm whether the subscription delivered any events (visible in flutter logs without adb logcat)
…ntion mostro-core derives #[serde(rename_all = "kebab-case")] on the Status enum, so all wire-format values are kebab-case: "pending", "in-progress", "waiting-buyer-invoice", "fiat-sent", etc. Our filter was sending #s=Pending (PascalCase) which matched zero events on the relay. parse_status() was also comparing against PascalCase strings so even events that bypassed the filter would be silently rejected. Fix both: - pending_orders_filter(): "Pending" → "pending" - parse_status(): all 15 variants fixed to kebab-case wire values - add "cooperatively-canceled" mapping to Canceled
- Add publish_event_json() helper in orders.rs; wire send_fiat_sent, release_order, send_invoice, take_order and create_order to actually publish NIP-59 gift-wrapped messages to the relay pool - Add cancel and add_invoice action builders to mostro/actions.rs - Wire send_message in messages.rs to publish Kind 14 gift wrap to peer when session and peer pubkey are available; falls back to local-only - Replace all Future.delayed stubs in trade_detail_screen.dart with real bridge calls: releaseOrder(), sendFiatSent() - Wire add_lightning_invoice_screen.dart to sendInvoice() bridge; guard Submit button against null/zero amountSats - Add Phase 19 and Phase 20 to plan.md, tasks.md (T133–T145 + wiring tasks); add R13/R14 to research.md documenting Kind 38383 authorship and mostro-core kebab-case serde conventions
Contributor
WalkthroughThis PR replaces mock order data with a Rust-backed real-time order stream, wires Mostro action dispatch (create/take/sendInvoice/sendFiatSent/release/cancel) through the Rust bridge, adds async app/Rust/nostr initialization and background watchers, introduces loading/error UI states with a shimmer skeleton, and adds localization and protocol fixes (kebab-case statuses, trusted-author filter). Changes
Sequence Diagram(s)sequenceDiagram
participant App as Flutter App
participant RustLib as RustLib (FFI)
participant RelayPool as Relay Pool
participant Mostro as Mostro Node
App->>RustLib: RustLib.init()
App->>RustLib: nostr_api.initialize(relays: null)
RustLib->>RelayPool: client.connect().await
RelayPool->>Mostro: WebSocket handshake
Mostro-->>RelayPool: Connected
RelayPool-->>RustLib: broadcast_connection_state(Online)
RustLib-->>App: Online signal
App->>RustLib: orders_api.getOrders(filters: null)
RustLib->>RelayPool: Query Kind 38383 events (author: mostro_pubkey)
RelayPool->>Mostro: Request pending orders
Mostro-->>RelayPool: Event list
RelayPool-->>RustLib: Parsed OrderInfo[]
RustLib-->>App: Initial snapshot via FRB
Note over RustLib,RelayPool: subscribe_orders() runs in background
Mostro-->>RelayPool: New order event
RelayPool-->>RustLib: RelayPoolNotification::Event
RustLib->>RustLib: parse_order_event & upsert
RustLib-->>App: onOrdersUpdated stream (FRB)
sequenceDiagram
participant User as User
participant UI as Home Screen
participant Provider as orderBookProvider
participant RustAPI as orders_api (Rust)
User->>UI: Open home screen
UI->>Provider: watch(orderBookProvider)
alt Loading
Provider-->>UI: AsyncValue.loading
UI->>UI: Render OrderListSkeleton
end
Provider->>RustAPI: getOrders(...)
RustAPI->>Provider: AsyncValue.data(orders)
UI->>UI: Render order list
par Stream updates
RustAPI->>Provider: onOrdersUpdated() yields
Provider-->>UI: AsyncValue.data(updated orders)
UI->>UI: Refresh list
end
alt Error
RustAPI-->>Provider: AsyncValue.error
UI->>User: Show error + Retry
User->>UI: Tap Retry
UI->>Provider: invalidate()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/features/order/screens/take_order_screen.dart (1)
43-72:⚠️ Potential issue | 🟡 MinorCountdown may fail to start if provider is still loading.
_startCountdown()is called once ininitState(). IforderBookProvideris loading at that moment,valueOrNullreturnsnull,orderisnull, and the method returns early. When data eventually arrives andbuild()re-runs,_startCountdown()is never re-invoked—leaving the countdown stuck at zero.Consider initializing the countdown reactively (e.g., in
build()with a guard, or via aref.listencallback) to handle late-arriving data.💡 Possible approach using didChangeDependencies or a listen callback
// Option A: Use ref.listen in initState (after super.initState) `@override` void initState() { super.initState(); // Defer to first frame so ref is available WidgetsBinding.instance.addPostFrameCallback((_) { _tryStartCountdown(); ref.listenManual(orderBookProvider, (_, next) { if (_countdownTimer == null && next.hasValue) { _tryStartCountdown(); } }); }); } void _tryStartCountdown() { if (_countdownTimer != null) return; // already running _startCountdown(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/order/screens/take_order_screen.dart` around lines 43 - 72, initState currently calls _startCountdown once, but if orderBookProvider is still loading the method returns early and the timer is never started when data arrives; change initialization to start the countdown reactively: in initState schedule a post-frame callback (WidgetsBinding.instance.addPostFrameCallback) and register a ref.listen (or ref.listenManual) on orderBookProvider to call a new helper _tryStartCountdown which checks if _countdownTimer is null and the order exists, then invokes _startCountdown; keep dispose cancelling _countdownTimer and ensure _startCountdown itself remains idempotent (does nothing if _countdownTimer != null) and locates the order by widget.orderId as before.lib/features/trades/screens/trade_detail_screen.dart (2)
227-245:⚠️ Potential issue | 🟠 MajorDon't drive live mutations from placeholder trade state.
The screen still hard-codes
_statusand_isBuyerat Line 55-61, but this CTA now calls the real backend. That means the buyer action can be shown for the wrong role/state, while the seller branches remain unreachable until real trade info is wired in. The same placeholder gating also affects the release paths below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/trades/screens/trade_detail_screen.dart` around lines 227 - 245, The buyer CTA is being shown/acted on based on hard-coded placeholder fields (_status and _isBuyer) rather than the actual trade data; update the screen to derive visibility and enabled state from the real trade model or fetched trade state (e.g., replace checks against _status and _isBuyer with the live trade.status and trade.isBuyer or a loadedTrade variable), fetch or accept the real trade data earlier (initState or via widget parameter), disable/hide MostroReactiveButton until the trade is loaded, and ensure orders_api.sendFiatSent uses widget.orderId only when the current live trade indicates the user is buyer and status == TradeStatus.active and then call setState to update the live trade status (not the placeholder _status) after success; also keep the mounted checks and proper error SnackBar handling in onError.
467-488:⚠️ Potential issue | 🟠 Major
releaseOrdercannot succeed from the disputed branch.
rust/src/api/orders.rs:420-435only allowsrelease_orderwhen the order status isFiatSent. Calling it from the_status == TradeStatus.disputedbranch guarantees a backend rejection once this path is reachable. This button needs the dispute-resolution action instead of the normal release path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/trades/screens/trade_detail_screen.dart` around lines 467 - 488, The RELEASE button in the disputed branch calls orders_api.releaseOrder, but backend only allows release when status == FiatSent so this will fail; change the onPressed handler inside the disputed branch to call the dispute-resolution API instead (e.g., orders_api.resolveDispute or the existing dispute-resolution method), swap the confirmation dialog to a dispute-specific one (e.g., showResolveDisputeConfirmationDialog) and keep the same post-success navigation (context.push(AppRoute.rateUserPath(widget.orderId))) and error handling; update references to orders_api.releaseOrder, showReleaseConfirmationDialog, and the onPressed closure in MostroReactiveButton to use the dispute-resolution method and confirmation dialog.
🧹 Nitpick comments (3)
lib/l10n/app_fr.arb (1)
196-198: Minor terminology inconsistency: "relay" vs "relais".These keys use "relay" while other keys in this file use the French translation "relais" (e.g., line 121
relaysSettingTitle). Consider using "relais" consistently:
disableRelayLabel: "Désactiver le relais {url}"enableRelayLabel: "Activer le relais {url}"removeRelayTooltip: "Supprimer le relais"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/l10n/app_fr.arb` around lines 196 - 198, The three translation values for keys disableRelayLabel, enableRelayLabel, and removeRelayTooltip use "relay" in French; update their strings to use the consistent French term "relais" (e.g., change "Désactiver le relay {url}" → "Désactiver le relais {url}", "Activer le relay {url}" → "Activer le relais {url}", and "Supprimer le relay" → "Supprimer le relais") so they match other keys like relaysSettingTitle.lib/shared/widgets/order_list_skeleton.dart (1)
27-33: Consider using theme/design constants for consistency.The shimmer colors are correctly documented per DESIGN_SYSTEM.md §9.1. However, the hardcoded
height: 100,margin: 6, andborderRadius: 12could be extracted to named constants or aligned with existingAppSpacing/AppRadiusvalues for easier maintenance.♻️ Optional: Use design system constants
- itemBuilder: (_, __) => Container( - height: 100, - margin: const EdgeInsets.symmetric(vertical: 6), - decoration: BoxDecoration( - color: Colors.white, - borderRadius: BorderRadius.circular(12), - ), - ), + itemBuilder: (_, __) => Container( + height: 100, // Order card placeholder height + margin: const EdgeInsets.symmetric(vertical: AppSpacing.xs), + decoration: BoxDecoration( + color: Colors.white, + borderRadius: BorderRadius.circular(AppRadius.card), + ), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shared/widgets/order_list_skeleton.dart` around lines 27 - 33, Replace hardcoded layout values in the OrderListSkeleton's itemBuilder (the Container with height: 100, margin: const EdgeInsets.symmetric(vertical: 6), and BorderRadius.circular(12)) with design-system constants: use AppSpacing (or equivalent spacing constants) for the vertical margin and height (or a named constant like orderListItemHeight) and AppRadius (or equivalent) for the borderRadius to ensure consistency and easier maintenance; update any imports if needed and keep names descriptive (e.g., orderListItemHeight, AppSpacing.smallVertical, AppRadius.medium) so the Container references only design constants.lib/features/home/screens/home_screen.dart (1)
217-218: Add a semantic label to the shimmer.The new loading branch is visual-only, so screen readers have nothing to announce while the order book is fetching. Since
loadingOrdersalready exists, expose it viaSemanticsaroundOrderListSkeleton.♿ Suggested tweak
- loading: () => const OrderListSkeleton(), + loading: () => Semantics( + container: true, + liveRegion: true, + label: AppLocalizations.of(context).loadingOrders, + child: const ExcludeSemantics( + child: OrderListSkeleton(), + ), + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/home/screens/home_screen.dart` around lines 217 - 218, The OrderListSkeleton shimmer is visual-only and needs a semantic label so screen readers can announce loading state; wrap the loading branch returned from ref.watch(orderBookProvider).when(...) with a Semantics widget that provides a descriptive label using the existing loadingOrders text (e.g. Semantics(label: loadingOrders, child: OrderListSkeleton())), ensuring you modify the loading case where OrderListSkeleton is returned and keep other branches unchanged.
🤖 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 129-141: The provider subscribes to live updates after fetching
the initial snapshot, risking dropped first update because
orders_api.onOrdersUpdated() is a non-replaying broadcast; change
orderBookProvider to call await orders_api.onOrdersUpdated() and start listening
(obtain the stream/iterator) before calling await orders_api.getOrders() so you
have the subscription in place, then yield the initial snapshot and continue
consuming stream.next() in the existing loop (ensure you close/cancel the
subscription on dispose).
In `@lib/features/order/screens/add_lightning_invoice_screen.dart`:
- Around line 44-47: The screen must explicitly handle widget.amountSats == null
instead of silently showing the manual form and falling back to 0; update the
_isValid getter and the build flow so that while widget.amountSats is null you
render a loading or error state (or a clear "waiting for amount" UI) and do not
display the manual submit controls, and remove any use of a fallback like
(widget.amountSats ?? 0) — only pass a non-null amountSats into downstream calls
when it exists (update references around _isValid and the code that currently
uses a fallback on widget.amountSats so validation and submission rely on the
resolved value).
In `@lib/features/settings/screens/connect_wallet_screen.dart`:
- Around line 193-200: The paste branch that reads Clipboard.getData() should
trim whitespace/newlines before checking and normalizing the scheme so copied
NWC URIs with trailing spaces don't get rejected; modify the Clipboard handling
to call trim() on the retrieved text before using
text.toLowerCase().startsWith(scheme) and before computing
text.substring(scheme.length) to assign _uriController.text (consistent with how
_connect() and _onQrDetected() already trim).
In `@lib/l10n/app_de.arb`:
- Line 133: The German localization string for invalidLightningAddressFormat
currently implies a .com TLD; update the value for the
"invalidLightningAddressFormat" key so it describes the format without
specifying .com (for example use "Muss im Format benutzer@domain oder
benutzer@domain.tld vorliegen" or similar), ensuring the message reflects a
format-based validation rather than a TLD-specific requirement.
In `@lib/l10n/app_localizations_fr.dart`:
- Around line 595-605: The French strings use the English word "relay"
inconsistently; update the three localization entries disableRelayLabel,
enableRelayLabel and removeRelayTooltip to use "relais" instead of "relay" so
they match existing translations—modify the return values for
disableRelayLabel(String url) and enableRelayLabel(String url) and the getter
removeRelayTooltip to replace "relay" with "relais".
In `@lib/main.dart`:
- Around line 15-21: The background diagnostics watcher started by
_watchConnectionState() in MostroApp (a ConsumerWidget) creates an infinite
listener with no teardown and contains a Future.delayed() followed by
orders_api.getOrders() outside the try/catch, risking unhandled exceptions and
indefinite resource usage in production; move this logic into a Riverpod-owned
provider (or wrap it behind kDebugMode) so Riverpod can manage its lifecycle and
disposal, and ensure the delayed/orders_api.getOrders() call is executed inside
the try/catch block (or otherwise error-handled) inside that provider to capture
exceptions and avoid leaking background work from a stateless ConsumerWidget.
In `@rust/src/api/messages.rs`:
- Around line 133-186: The code currently converts many real failures into
Ok(()) so the UI records messages as "sent" even when they weren't; update the
publish_result construction so that only the explicit "session exists but peer
unknown" branch returns Ok(()) for local-only storage, and all other failure
points propagate an Err: specifically propagate errors from
get_active_trade_keys (crate::api::identity::get_active_trade_keys), peer pubkey
parsing, crate::nostr::gift_wrap::wrap,
serde_json::from_str::<nostr_sdk::Event>, and pool.client().send_event so they
return Err(...) instead of being swallowed by Ok(()); leave the single safe
branch that logs "session exists but peer unknown — local-only" as Ok(()) and
ensure the outer error handling for publish_result logs/returns these real
errors accordingly.
In `@rust/src/api/orders.rs`:
- Around line 221-239: The optimistic upsert in create_order currently returns
success even if identity retrieval (crate::api::identity::get_active_keys),
action construction (actions::new_order) or publish (publish_event_json) fail;
update create_order to handle those failures by either (A) propagating an Err to
the caller when any of get_active_keys, nostr_sdk::PublicKey::from_hex,
actions::new_order, or publish_event_json fail (remove the optimistic order from
order_book() or roll it back), or (B) if you want offline queuing, persist an
explicit queued/outbox state in the shared cache (e.g., mark order.status =
Queued and store the event JSON and failure reason) before returning success;
locate the logic around order_book().upsert_order(order.clone()),
get_active_keys(), actions::new_order(...) and publish_event_json(...) and
implement one of these two behaviors consistently (propagate error or record
queued state) so callers are not misled by a "dispatched" log when dispatch
never occurred.
- Around line 227-228: The code currently uses the compile-time
DEFAULT_MOSTRO_PUBKEY when calling actions::new_order and when creating the Kind
38383 subscription filter; replace those hardcoded uses with the
runtime-resolved Mostro node public key from the app's node-selection state
(e.g., obtain the active daemon's pubkey via the node selector or config
accessor used elsewhere in the app) and pass that value into actions::new_order
and into the nostr subscription filter construction (where DEFAULT_MOSTRO_PUBKEY
is referenced). Do the same replacement for the other occurrences noted (the
other calls/filters around the same areas) so reads/writes target the currently
selected daemon instead of the compiled default.
- Around line 522-538: The loop only subscribes to
pending_orders_filter(&mostro_pubkey) and upserts parsed events, so orders that
transition out of Pending never get removed; update the subscription or
handling: subscribe to a broader feed (e.g., a non-pending/all orders filter) or
keep the pending subscription but, after parse_order_event(&event, None) returns
Some(info), inspect info.status and call the appropriate removal/update method
on the cache (e.g., order_book().remove_order(info.id) for cancelled/taken
states or order_book().upsert_order(info) for still-pending/updated states).
Locate pending_orders_filter, parse_order_event, and order_book().upsert_order
to implement the change so the cache evicts or replaces orders when status !=
Pending.
In `@rust/src/nostr/order_events.rs`:
- Around line 71-77: The current code derives is_mine by comparing my_pubkey to
event.pubkey, but event.pubkey is the Mostro node (not the maker) so this check
is invalid; change the logic in the block that computes is_mine (the variable
named is_mine that currently uses my_pubkey.map(|pk| pk ==
&event.pubkey).unwrap_or(false)) to always set is_mine to false for these
order-book events and rely on later trade-message confirmation to set the real
ownership flag.
In `@specs/004-mostro-p2p-client/research.md`:
- Around line 378-390: The docs conflict: R3 claims CooperativelyCanceled is
client-side only but the status table lists `cooperatively-canceled` as an
on-wire value that maps to `Canceled`; reconcile by choosing one behavior and
making both places consistent — e.g., if you want the wire to carry
`cooperatively-canceled`, update R3 and `parse_status()` docs to explicitly
accept `cooperatively-canceled` and map it to OrderStatus::Canceled (or
OrderStatus::CooperativelyCanceled if you prefer a distinct enum variant), and
make the status table and any examples reflect the same mapping; ensure
references to `parse_status()` and the `OrderStatus` variants (`Canceled`,
`CooperativelyCanceled`) are updated accordingly.
In `@specs/004-mostro-p2p-client/tasks.md`:
- Around line 493-497: The fenced code block under "Phase 18 (Real Order Book +
Shimmer)" containing the lines with T127..T132 has no language tag and is
flagged by markdownlint MD040; update that fence to include a language
identifier (e.g., text) so the block becomes ```text ... ```, which will silence
the linter for the block showing "T127 OrderListSkeleton widget T128
subscribe_orders() Rust T129 on_orders_updated() stream T130 Wire startup
subscription T131 StreamProvider orderBook T132 Shimmer in home screen".
---
Outside diff comments:
In `@lib/features/order/screens/take_order_screen.dart`:
- Around line 43-72: initState currently calls _startCountdown once, but if
orderBookProvider is still loading the method returns early and the timer is
never started when data arrives; change initialization to start the countdown
reactively: in initState schedule a post-frame callback
(WidgetsBinding.instance.addPostFrameCallback) and register a ref.listen (or
ref.listenManual) on orderBookProvider to call a new helper _tryStartCountdown
which checks if _countdownTimer is null and the order exists, then invokes
_startCountdown; keep dispose cancelling _countdownTimer and ensure
_startCountdown itself remains idempotent (does nothing if _countdownTimer !=
null) and locates the order by widget.orderId as before.
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 227-245: The buyer CTA is being shown/acted on based on hard-coded
placeholder fields (_status and _isBuyer) rather than the actual trade data;
update the screen to derive visibility and enabled state from the real trade
model or fetched trade state (e.g., replace checks against _status and _isBuyer
with the live trade.status and trade.isBuyer or a loadedTrade variable), fetch
or accept the real trade data earlier (initState or via widget parameter),
disable/hide MostroReactiveButton until the trade is loaded, and ensure
orders_api.sendFiatSent uses widget.orderId only when the current live trade
indicates the user is buyer and status == TradeStatus.active and then call
setState to update the live trade status (not the placeholder _status) after
success; also keep the mounted checks and proper error SnackBar handling in
onError.
- Around line 467-488: The RELEASE button in the disputed branch calls
orders_api.releaseOrder, but backend only allows release when status == FiatSent
so this will fail; change the onPressed handler inside the disputed branch to
call the dispute-resolution API instead (e.g., orders_api.resolveDispute or the
existing dispute-resolution method), swap the confirmation dialog to a
dispute-specific one (e.g., showResolveDisputeConfirmationDialog) and keep the
same post-success navigation
(context.push(AppRoute.rateUserPath(widget.orderId))) and error handling; update
references to orders_api.releaseOrder, showReleaseConfirmationDialog, and the
onPressed closure in MostroReactiveButton to use the dispute-resolution method
and confirmation dialog.
---
Nitpick comments:
In `@lib/features/home/screens/home_screen.dart`:
- Around line 217-218: The OrderListSkeleton shimmer is visual-only and needs a
semantic label so screen readers can announce loading state; wrap the loading
branch returned from ref.watch(orderBookProvider).when(...) with a Semantics
widget that provides a descriptive label using the existing loadingOrders text
(e.g. Semantics(label: loadingOrders, child: OrderListSkeleton())), ensuring you
modify the loading case where OrderListSkeleton is returned and keep other
branches unchanged.
In `@lib/l10n/app_fr.arb`:
- Around line 196-198: The three translation values for keys disableRelayLabel,
enableRelayLabel, and removeRelayTooltip use "relay" in French; update their
strings to use the consistent French term "relais" (e.g., change "Désactiver le
relay {url}" → "Désactiver le relais {url}", "Activer le relay {url}" → "Activer
le relais {url}", and "Supprimer le relay" → "Supprimer le relais") so they
match other keys like relaysSettingTitle.
In `@lib/shared/widgets/order_list_skeleton.dart`:
- Around line 27-33: Replace hardcoded layout values in the OrderListSkeleton's
itemBuilder (the Container with height: 100, margin: const
EdgeInsets.symmetric(vertical: 6), and BorderRadius.circular(12)) with
design-system constants: use AppSpacing (or equivalent spacing constants) for
the vertical margin and height (or a named constant like orderListItemHeight)
and AppRadius (or equivalent) for the borderRadius to ensure consistency and
easier maintenance; update any imports if needed and keep names descriptive
(e.g., orderListItemHeight, AppSpacing.smallVertical, AppRadius.medium) so the
Container references only design constants.
🪄 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: 2541c921-a250-4976-94ee-24aac1f46b9b
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
lib/features/about/screens/about_screen.dartlib/features/home/providers/home_order_providers.dartlib/features/home/screens/home_screen.dartlib/features/order/screens/add_lightning_invoice_screen.dartlib/features/order/screens/take_order_screen.dartlib/features/settings/screens/connect_wallet_screen.dartlib/features/settings/screens/settings_screen.dartlib/features/settings/widgets/currency_selector_dialog.dartlib/features/settings/widgets/relay_management_card.dartlib/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/main.dartlib/shared/widgets/countdown_timer.dartlib/shared/widgets/order_list_skeleton.dartlib/shared/widgets/platform_aware_qr_scanner.dartpubspec.yamlrust/Cargo.tomlrust/src/api/identity.rsrust/src/api/messages.rsrust/src/api/nostr.rsrust/src/api/orders.rsrust/src/frb_generated.rsrust/src/lib.rsrust/src/mostro/actions.rsrust/src/nostr/order_events.rsrust/src/nostr/relay_pool.rsspecs/004-mostro-p2p-client/plan.mdspecs/004-mostro-p2p-client/research.mdspecs/004-mostro-p2p-client/tasks.md
Comment on lines
+44
to
+47
| bool get _isValid => | ||
| _invoiceController.text.trim().isNotEmpty && | ||
| widget.amountSats != null && | ||
| widget.amountSats! > 0; |
Contributor
There was a problem hiding this comment.
Handle unresolved amountSats explicitly.
amountSats is documented as null until the trade provider resolves it, but the manual form still renders in that state while Submit stays disabled. That makes the flow look broken for slow/failed loads, and Line 57 still falls back to 0, which leaves missing-amount validation to Rust instead of this screen. Prefer a loading/error state until the amount exists, then pass the non-null value directly.
Also applies to: 54-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/order/screens/add_lightning_invoice_screen.dart` around lines 44
- 47, The screen must explicitly handle widget.amountSats == null instead of
silently showing the manual form and falling back to 0; update the _isValid
getter and the build flow so that while widget.amountSats is null you render a
loading or error state (or a clear "waiting for amount" UI) and do not display
the manual submit controls, and remove any use of a fallback like
(widget.amountSats ?? 0) — only pass a non-null amountSats into downstream calls
when it exists (update references around _isValid and the code that currently
uses a fallback on widget.amountSats so validation and submission rely on the
resolved value).
Comment on lines
+483
to
+497
| pub async fn subscribe_orders() { | ||
| // Only one loop at a time — subsequent Online transitions are no-ops. | ||
| if SUBSCRIPTION_ACTIVE | ||
| .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) | ||
| .is_err() | ||
| { | ||
| log::debug!("[orders] subscribe_orders: already active, skipping"); | ||
| return; | ||
| } | ||
| log::info!("[orders] subscribe_orders: spawning subscription loop"); | ||
|
|
||
| tokio::spawn(async { | ||
| let _guard = ResetGuard; | ||
| _run_order_subscription().await; | ||
| }); |
Contributor
There was a problem hiding this comment.
This guard blocks the reconnect re-subscribe path.
SUBSCRIPTION_ACTIVE only resets when the spawned task exits. During a transient Offline→Online cycle the receiver usually stays alive, so later subscribe_orders() calls are no-ops and client.subscribe(...) is never reissued. After the first disconnect, live order updates can stop until app restart.
Comment on lines
+522
to
+538
| let filter = crate::nostr::order_events::pending_orders_filter(&mostro_pubkey); | ||
| if let Err(e) = client.subscribe(filter, None).await { | ||
| log::error!("[orders] subscribe failed: {e}"); | ||
| return; | ||
| } | ||
| log::info!("[orders] Kind 38383 subscription active — waiting for events"); | ||
|
|
||
| use nostr_sdk::RelayPoolNotification; | ||
|
|
||
| loop { | ||
| match rx.recv().await { | ||
| Ok(RelayPoolNotification::Event { event, .. }) => { | ||
| log::info!("[orders] event kind={} author={}", event.kind, &event.pubkey.to_hex()[..8]); | ||
| match parse_order_event(&event, None) { | ||
| Some(info) => { | ||
| log::info!("[orders] parsed order id={} kind={:?} status={:?}", info.id, info.kind, info.status); | ||
| order_book().upsert_order(info).await; |
Contributor
There was a problem hiding this comment.
The cache never evicts orders that leave Pending.
This loop only ingests pending_orders_filter(...) events and then upserts them. When another user takes or cancels an order, no non-pending update reaches this cache, so the old entry stays in memory and continues to be surfaced as available. You need either a removal path here or a full-snapshot replacement strategy.
🤖 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 522 - 538, The loop only subscribes to
pending_orders_filter(&mostro_pubkey) and upserts parsed events, so orders that
transition out of Pending never get removed; update the subscription or
handling: subscribe to a broader feed (e.g., a non-pending/all orders filter) or
keep the pending subscription but, after parse_order_event(&event, None) returns
Some(info), inspect info.status and call the appropriate removal/update method
on the cache (e.g., order_book().remove_order(info.id) for cancelled/taken
states or order_book().upsert_order(info) for still-pending/updated states).
Locate pending_orders_filter, parse_order_event, and order_book().upsert_order
to implement the change so the cache evicts or replaces orders when status !=
Pending.
…ion, diagnostics guard - home_order_providers: subscribe to onOrdersUpdated() before getOrders() to fix race - add_lightning_invoice_screen: use ! instead of ?? 0 since _isValid already guards null - take_order_screen: idempotent _tryStartCountdown with listenManual for late provider data - connect_wallet_screen: trim clipboard text before scheme check - order_list_skeleton: extract _SkeletonCard with AppRadius.card and design constants - main.dart: guard _watchConnectionState with kDebugMode; wrap delayed callback in try/catch - messages.rs: propagate key retrieval errors separately from peer-unknown fallback - order_events.rs: is_mine always false for Kind 38383 (event author is Mostro node) - app_de.arb: fix invalidLightningAddressFormat to use generic domain.tld - app_fr.arb, app_localizations_fr.dart: relay → relais (correct French term) - research.md: reconcile R3 with R14 re cooperatively-canceled wire status - tasks.md: fix bare code fences to \`\`\`text (MD040 compliance)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/main.dart (1)
49-59: Multiple delayed callbacks may fire on rapid reconnects.If the relay pool transitions to
onlinemultiple times in quick succession (e.g., network flapping), each transition spawns a newFuture.delayedthat polls orders after 5 seconds. While not harmful (read-only), it's wasteful. Consider debouncing or tracking an active delayed poll.💡 Simple debounce approach
+bool _orderPollScheduled = false; + void _watchConnectionState() { if (!kDebugMode) return; Future.microtask(() async { try { final stream = await nostr_api.onConnectionStateChanged(); while (true) { final state = await stream.next(); if (state == null) break; debugPrint('[nostr] connection state → $state'); if (state.name == 'online') { // Log relay details when we come online. final relays = await nostr_api.getRelays(); for (final r in relays) { debugPrint('[nostr] relay ${r.url} → ${r.status}'); } - // Wait 5 seconds then poll the order cache - Future.delayed(const Duration(seconds: 5), () async { + if (!_orderPollScheduled) { + _orderPollScheduled = true; + Future.delayed(const Duration(seconds: 5), () async { + _orderPollScheduled = false; try { final orders = await orders_api.getOrders(filters: null); debugPrint('[diag] order cache after 5s: ${orders.length} orders'); if (orders.isNotEmpty) { debugPrint('[diag] first order: id=${orders.first.id} kind=${orders.first.kind} fiat=${orders.first.fiatCode}'); } } catch (e) { debugPrint('[diag] order cache poll error: $e'); } }); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/main.dart` around lines 49 - 59, The delayed orders poll scheduled via Future.delayed in the online transition can spawn multiple concurrent timers on rapid reconnects; modify the logic in the online handler to track and prevent overlapping polls (e.g., add a boolean or a Timer field like _isPollingOrders or _ordersPollTimer) before calling Future.delayed, set it when scheduling, and clear it after the async getOrders finishes or on error; reference the Future.delayed call, orders_api.getOrders, and the surrounding online transition handler to locate where to add the debounce/active-poll guard.lib/features/order/screens/add_lightning_invoice_screen.dart (1)
84-102: Consider showing a loading state whenamountSatsis still resolving.When
amountSats == nulland NWC is not connected, the manual form renders but the Submit button is disabled without explanation. Users may not understand why they cannot submit.💡 Suggested UX improvement
+ // If amount is not yet resolved, show a loading indicator + final sats = widget.amountSats; + if (sats == null) { + return Scaffold( + appBar: AppBar(title: const Text('Add Invoice')), + body: const Center( + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + CircularProgressIndicator(), + SizedBox(height: 16), + Text('Waiting for order details...'), + ], + ), + ), + ); + } + // If NWC wallet is connected, amount is known, and we haven't fallen back // to manual, show the auto-invoice widget instead of the manual form. - final sats = widget.amountSats; if (isWalletConnected && !_manualMode && sats != null && sats > 0) {Also applies to: 104-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/order/screens/add_lightning_invoice_screen.dart` around lines 84 - 102, When amountSats is still resolving the UI falls back to the manual form with a disabled Submit; update the conditional rendering around amountSats / isWalletConnected / _manualMode so that when amountSats == null and !_manualMode you show a clear loading state (e.g., Center with CircularProgressIndicator and brief explanatory text) instead of the manual form; keep the existing NwcInvoiceWidget branch (NwcInvoiceWidget, onFallbackToManual) and ensure _invoiceController / _submit logic remains unchanged so that manual form and Submit remain available only after user explicitly switches to _manualMode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/features/order/screens/add_lightning_invoice_screen.dart`:
- Around line 84-102: When amountSats is still resolving the UI falls back to
the manual form with a disabled Submit; update the conditional rendering around
amountSats / isWalletConnected / _manualMode so that when amountSats == null and
!_manualMode you show a clear loading state (e.g., Center with
CircularProgressIndicator and brief explanatory text) instead of the manual
form; keep the existing NwcInvoiceWidget branch (NwcInvoiceWidget,
onFallbackToManual) and ensure _invoiceController / _submit logic remains
unchanged so that manual form and Submit remain available only after user
explicitly switches to _manualMode.
In `@lib/main.dart`:
- Around line 49-59: The delayed orders poll scheduled via Future.delayed in the
online transition can spawn multiple concurrent timers on rapid reconnects;
modify the logic in the online handler to track and prevent overlapping polls
(e.g., add a boolean or a Timer field like _isPollingOrders or _ordersPollTimer)
before calling Future.delayed, set it when scheduling, and clear it after the
async getOrders finishes or on error; reference the Future.delayed call,
orders_api.getOrders, and the surrounding online transition handler to locate
where to add the debounce/active-poll guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca7a8b1f-60d4-4d6c-9556-ab617067a58f
📒 Files selected for processing (13)
lib/features/home/providers/home_order_providers.dartlib/features/order/screens/add_lightning_invoice_screen.dartlib/features/order/screens/take_order_screen.dartlib/features/settings/screens/connect_wallet_screen.dartlib/l10n/app_de.arblib/l10n/app_fr.arblib/l10n/app_localizations_fr.dartlib/main.dartlib/shared/widgets/order_list_skeleton.dartrust/src/api/messages.rsrust/src/nostr/order_events.rsspecs/004-mostro-p2p-client/research.mdspecs/004-mostro-p2p-client/tasks.md
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/l10n/app_fr.arb
- lib/features/order/screens/take_order_screen.dart
- lib/shared/widgets/order_list_skeleton.dart
- lib/l10n/app_de.arb
- rust/src/nostr/order_events.rs
- rust/src/api/messages.rs
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.
The previous 118 tasks left two concrete gaps:
Add Phase 18 (T126–T132) to tasks.md and a matching section in plan.md:
Summary by CodeRabbit
New Features
Bug Fixes
Localization