feat(settings): persist Mostro node selection and skip rating in priv… - #93
Conversation
…acy mode Add save_mostro_node/get_active_mostro_node to Storage trait with SQLite and IndexedDB implementations. Create settings key-value table in schema. Wire order book refresh in account screen and skip rating step when privacy mode is active.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds persistent Mostro node storage and FRB bindings, a Rust API to get/set the active node and restart order subscriptions, implements storage for SQLite and a WASM stub, updates seeding to persist defaults, and adjusts Flutter UI to refresh orders and respect privacy-mode routing. Changes
Sequence DiagramsequenceDiagram
participant Flutter as Flutter UI
participant FRB as Flutter-Rust Bridge
participant API as Rust API
participant DB as Storage/SQLite
participant Router as App Router
Flutter->>FRB: restartOrdersSubscription() (account refresh)
FRB->>API: restart_orders_subscription()
API->>API: clear SUBSCRIPTION_ACTIVE & subscribe_orders()
API->>DB: (when get/set node) get_active_mostro_node()/save_mostro_node()
DB-->>API: stored MostroNodeInfo / None
API-->>FRB: result
FRB-->>Flutter: completion
Flutter->>API: (on release/rate flow) query privacyModeProvider via ref
alt privacy mode enabled
Flutter->>Router: navigate Home
else
Flutter->>Router: navigate RateUser(orderId)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Good work overall. Two issues need addressing before merge.
🔴 Blocker — get_settings().privacyMode causes two async gaps; widget may be disposed between them
In trade_detail_screen.dart, both new call sites follow this pattern:
await orders_api.releaseOrder(orderId: widget.orderId);
if (!context.mounted) return;
final settings = await settings_api.getSettings(); // ← second async gap
if (!context.mounted) return;
if (settings.privacyMode) { ... }And in the Rate button:
onPressed: () async {
final settings = await settings_api.getSettings(); // async gap
if (!context.mounted) return;
...
}settings_api.getSettings() crosses the Rust bridge — it is an async call even if fast. Each await is an opportunity for the widget to be disposed between the !context.mounted check and the next line. The if (!context.mounted) return; guard is present, which is correct.
However, the privacyMode value is already in ref.watch(privacyModeProvider) — a synchronous read from in-memory state. Using getSettings() for this adds unnecessary async overhead and an extra mounted check.
Fix: read privacy mode synchronously from the provider instead of making an async settings call:
// In the release handler (already has WidgetRef via ConsumerStatefulWidget):
await orders_api.releaseOrder(orderId: widget.orderId);
if (!context.mounted) return;
final isPrivate = ref.read(privacyModeProvider);
if (isPrivate) {
context.go(AppRoute.home);
} else {
context.push(AppRoute.rateUserPath(widget.orderId));
}// In the Rate button onPressed:
onPressed: () {
final isPrivate = ref.read(privacyModeProvider);
if (isPrivate) {
context.go(AppRoute.home);
} else {
context.push(AppRoute.rateUserPath(widget.orderId));
}
},This is synchronous — no async, no mounted check needed, no extra bridge call.
Verify that privacyModeProvider is already watched/readable in TradeDetailScreen (it is a ConsumerStatefulWidget, so ref is available). If not, add ref.watch(privacyModeProvider) alongside other existing provider reads.
🟡 Major — WASM IndexedDbStorage silently drops Mostro node settings
indexeddb.rs stubs both methods as no-ops:
async fn save_mostro_node(&self, _node: &...) -> Result<()> {
Ok(()) // WASM: not persisted
}
async fn get_active_mostro_node(&self) -> Result<Option<...>> {
Ok(None) // WASM: not persisted
}This means on web (PWA), the app always boots with the default Mostro node — any node selection the user makes is silently lost on refresh. Since the app is designed to be a production PWA, this is a real UX bug, not just a stub.
The correct fix is to use IndexedDbStorage to persist the node in the browser's IndexedDB. However, if that is out of scope for this PR, at minimum add a // TODO(WASM) comment with a tracking issue number, and log a log::warn! so the developer is alerted when running the web build:
async fn save_mostro_node(&self, node: &crate::api::types::MostroNodeInfo) -> Result<()> {
// TODO(#NNN): persist to IndexedDB — currently lost on page refresh.
log::warn!("[indexeddb] save_mostro_node: not persisted on WASM (see #NNN)");
let _ = node;
Ok(())
}This should be tracked as a follow-up issue if not fixed here.
✅ What's good
settingstable DDL (CREATE TABLE IF NOT EXISTS settings (key TEXT PRIMARY KEY, value TEXT NOT NULL)) is clean and general-purpose — other settings can reuse it in the future.INSERT OR REPLACEinsave_mostro_nodeis correct for upsert semantics.seed_default_mostro_nodecorrectly skips if a node is already persisted (idempotent).set_mostro_nodecorrectly callscrate::config::set_active_mostro_pubkeyafter persisting — in-memory and DB stay in sync.get_mostro_nodefalls back toget_default_mostro_node()when DB is absent or returnsNone. Correct.account_screen.dartrefresh button is now async with proper error handling andmountedguard. Clean.- Stale TODO removed from
home_order_providers.dart. Good. - Comment in
take_order_screen.dartupdated to point to the actual implementation location. Good. frb_generated.rsregenerated. Good.
…tings Replace two unnecessary Rust bridge calls with synchronous ref.read(privacyModeProvider). Add log::warn and TODO(#93) to IndexedDB mostro-node stubs so WASM data loss is visible.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
rust/src/db/indexeddb.rs (1)
92-98: WASM stub causes repeated seeding on every launch.With
get_active_mostro_nodealways returningOk(None),seed_default_mostro_nodewill log "default Mostro node seeded" on every WASM app launch. This is harmless but may create log noise. Consider matching the error-returning pattern of other methods if seeding should be skipped entirely on WASM, or document this as expected temporary behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/db/indexeddb.rs` around lines 92 - 98, The WASM stubs save_mostro_node and get_active_mostro_node currently return success values which causes seed_default_mostro_node to run (and log) on every launch; change these stubs to mirror the error-returning pattern used elsewhere so the seeding is skipped on WASM: update get_active_mostro_node to return an Err(...) indicating "WASM: not persisted" (instead of Ok(None)) and make save_mostro_node return the same error Result path so callers treat persistence as unavailable in WASM; reference the async methods save_mostro_node and get_active_mostro_node when applying the change.rust/src/api/settings.rs (1)
215-226: Relay subscriptions may use stale pubkey after node change.
set_mostro_nodeupdates the in-memory pubkey viaset_active_mostro_pubkey, but existing relay filter subscriptions (as seen inrelay_pool.rs:subscribe_order_and_dm_feeds) are not refreshed. After changing the active Mostro node, the user may need to restart the app or manually trigger re-subscription to receive events from the new node.Consider documenting this limitation or wiring a subscription refresh when the node changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/settings.rs` around lines 215 - 226, set_mostro_node currently updates the in-memory pubkey via set_active_mostro_pubkey but does not refresh existing relay subscriptions, so relay_pool::subscribe_order_and_dm_feeds can keep using the old node; fix by triggering a subscription refresh after changing the pubkey—either call an existing relay pool method (e.g., RelayPool::refresh_subscriptions or a similarly named function in relay_pool.rs) from set_mostro_node, or emit a "mostro_node_changed" event on the app event bus that relay_pool listens for and re-runs subscribe_order_and_dm_feeds to re-subscribe with the new pubkey; ensure the call happens after set_active_mostro_pubkey and handle it asynchronously/await it as needed.rust/src/frb_generated.rs (1)
5420-5550: FRB codegen handles func_id synchronization automatically; verify Dart bindings were regenerated after API changes.The concern about func_id parity is valid in principle, but flutter_rust_bridge auto-generates Dart bindings (
lib/src/rust/) from the Rust API definitions, so func_ids sync automatically whenflutter_rust_bridge_codegen generateis run. Sincelib/src/rust/is gitignored and generated at build time, the risk is mitigated if the codegen step is part of the build workflow.The recent commit (2fb7946) shows proper synchronization: the new Rust API functions (
get_mostro_node,set_mostro_nodeinsettings.rs) and their dispatcher entries (func_id38, 89) were updated together, indicating FRB regeneration occurred. However, the pre-commit hook doesn't explicitly validate that FRB codegen has been run. Consider adding a check to ensure Dart bindings are regenerated whenever Rust APIs change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/frb_generated.rs` around lines 5420 - 5550, FRB func_id parity is handled by codegen but there's no enforcement that Dart bindings in lib/src/rust/ are regenerated; add a pre-commit or CI check that runs flutter_rust_bridge_codegen generate and verifies generated Dart bindings changed when Rust API symbols (e.g. get_mostro_node, set_mostro_node, other functions listed with func_id 38/89) are modified, failing the commit/CI if the generated files are out of sync with the Rust code; implement this by invoking flutter_rust_bridge_codegen in a hook or CI job and comparing the generated lib/src/rust/ output (or checking git diff) to ensure func_id updates are present.
🤖 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/account/screens/account_screen.dart`:
- Around line 464-468: The snackbar currently shows raw exception text (variable
e) to the end user; change the UI to show a generic message like "Refresh
failed" and only append the exception details when in debug mode (use
kDebugMode) while keeping the existing debugPrint for logs; update the call site
where ScaffoldMessenger.of(context).showSnackBar is called (and related error
handling in this account screen refresh method) so the SnackBar content does not
include $e in release builds.
- Around line 455-462: The snackbar shows success even when subscribeOrders()
returns early due to SUBSCRIPTION_ACTIVE being already true; add a new API in
orders.rs (e.g., restart_orders_subscription or unsubscribe_and_subscribe) that
force-tears down the existing subscription task (trigger ResetGuard cleanup and
clear SUBSCRIPTION_ACTIVE) and then spawns a fresh subscription loop using the
persisted relay pool config, and update the Dart caller (where
orders_api.subscribeOrders() is invoked in account_screen.dart) to call the new
restart API (or call an unsubscribe API followed by subscribe) and only show the
success SnackBar when the restart API returns success.
---
Nitpick comments:
In `@rust/src/api/settings.rs`:
- Around line 215-226: set_mostro_node currently updates the in-memory pubkey
via set_active_mostro_pubkey but does not refresh existing relay subscriptions,
so relay_pool::subscribe_order_and_dm_feeds can keep using the old node; fix by
triggering a subscription refresh after changing the pubkey—either call an
existing relay pool method (e.g., RelayPool::refresh_subscriptions or a
similarly named function in relay_pool.rs) from set_mostro_node, or emit a
"mostro_node_changed" event on the app event bus that relay_pool listens for and
re-runs subscribe_order_and_dm_feeds to re-subscribe with the new pubkey; ensure
the call happens after set_active_mostro_pubkey and handle it
asynchronously/await it as needed.
In `@rust/src/db/indexeddb.rs`:
- Around line 92-98: The WASM stubs save_mostro_node and get_active_mostro_node
currently return success values which causes seed_default_mostro_node to run
(and log) on every launch; change these stubs to mirror the error-returning
pattern used elsewhere so the seeding is skipped on WASM: update
get_active_mostro_node to return an Err(...) indicating "WASM: not persisted"
(instead of Ok(None)) and make save_mostro_node return the same error Result
path so callers treat persistence as unavailable in WASM; reference the async
methods save_mostro_node and get_active_mostro_node when applying the change.
In `@rust/src/frb_generated.rs`:
- Around line 5420-5550: FRB func_id parity is handled by codegen but there's no
enforcement that Dart bindings in lib/src/rust/ are regenerated; add a
pre-commit or CI check that runs flutter_rust_bridge_codegen generate and
verifies generated Dart bindings changed when Rust API symbols (e.g.
get_mostro_node, set_mostro_node, other functions listed with func_id 38/89) are
modified, failing the commit/CI if the generated files are out of sync with the
Rust code; implement this by invoking flutter_rust_bridge_codegen in a hook or
CI job and comparing the generated lib/src/rust/ output (or checking git diff)
to ensure func_id updates are present.
🪄 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: a3a5d5af-c269-4f5c-abf7-42b9f7aa9bd0
📒 Files selected for processing (11)
lib/features/account/screens/account_screen.dartlib/features/home/providers/home_order_providers.dartlib/features/order/screens/take_order_screen.dartlib/features/trades/screens/trade_detail_screen.dartrust/src/api/settings.rsrust/src/db/indexeddb.rsrust/src/db/mod.rsrust/src/db/schema.rsrust/src/db/seeds.rsrust/src/db/sqlite.rsrust/src/frb_generated.rs
💤 Files with no reviewable changes (1)
- lib/features/home/providers/home_order_providers.dart
Add restart_orders_subscription() that tears down the existing subscription guard before re-subscribing, so the UI refresh button actually restarts the feed instead of silently no-oping. Gate exception details in the refresh error snackbar behind kDebugMode so release builds show a generic message. Add TODO for relay subscription refresh after mostro node change.
1 participant
…acy mode
Add save_mostro_node/get_active_mostro_node to Storage trait with SQLite and IndexedDB implementations. Create settings key-value table in schema. Wire order book refresh in account screen and skip rating step when privacy mode is active.
Summary by CodeRabbit
New Features
Documentation