Replace clobbered method #732
Conversation
WalkthroughThe PR refactors relay status retrieval APIs by replacing Changes
Sequence Diagram(s)sequenceDiagram
participant Provider as relay_status_provider
participant DartAPI as relays.dart (Dart API)
participant FFI as frb_generated (FFI Bridge)
participant RustAPI as relays.rs (Rust API)
participant Whitenoise as Whitenoise (Core)
rect rgb(200, 220, 255)
Note over Provider,Whitenoise: Old Flow (fetchRelayStatus)
Provider->>DartAPI: fetchRelayStatus(pubkey)
DartAPI->>FFI: crateApiRelaysFetchRelayStatus(pubkey)
FFI->>RustAPI: fetch_relay_status(pubkey)
RustAPI->>Whitenoise: query_relay_status(pubkey)
Whitenoise-->>RustAPI: Vec<(String, String)>
RustAPI-->>FFI: Result<Vec<(String, String)>>
FFI-->>DartAPI: List<(String, String)>
DartAPI-->>Provider: relay_statuses_map
end
rect rgb(220, 255, 220)
Note over Provider,Whitenoise: New Flow (getAccountRelayStatuses)
Provider->>DartAPI: getAccountRelayStatuses(pubkey)
DartAPI->>FFI: crateApiRelaysGetAccountRelayStatuses(pubkey)
FFI->>RustAPI: get_account_relay_statuses(pubkey)
RustAPI->>Whitenoise: query_relay_status(pubkey)
Whitenoise-->>RustAPI: Vec<(String, String)>
RustAPI-->>FFI: Result<Vec<(String, String)>>
FFI-->>DartAPI: List<(String, String)>
DartAPI-->>Provider: relay_statuses_map
end
rect rgb(255, 240, 220)
Note over DartAPI,Whitenoise: New ensureAllSubscriptions API
DartAPI->>FFI: crateApiRelaysEnsureAllSubscriptions()
FFI->>RustAPI: ensure_all_subscriptions()
RustAPI->>Whitenoise: ensure_all_subscriptions()
Whitenoise-->>RustAPI: Result<(), ApiError>
RustAPI-->>FFI: Result<()>
FFI-->>DartAPI: Future<void>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes span five files with heterogeneous modifications including FFI wiring updates, function renaming across language boundaries, codec adjustments, and dispatch table routing changes. While the primary pattern is consistent (function replacement), the complexity lies in validating FFI correctness, dispatch ID mapping, and ensuring the Dart-Rust bridge remains in sync across generated bindings. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
rust/src/api/relays.rs (2)
41-51: Renamed relay-status API looks good.Parsing pubkey, account lookup, and mapping to (String, String) are correct. No functional concerns.
Consider exposing a non-opaque status enum over FRB (e.g., Vec<(String, RelayConnectionStatus)>) to avoid string parsing on the Dart side. If feasible, mark the enum #[frb(non_opaque)] and let FRB generate the Dart enum.
53-79: Thin wrapper for ensure_all_subscriptions is fine; confirm logging lives in core.Wrapper delegates to Whitenoise and error mapping is correct. If the core doesn’t already emit logs/metrics, consider adding tracing there (optional).
Please confirm Whitenoise::ensure_all_subscriptions() already logs subscription-level failures as described in the doc comment.
Also applies to: 73-79
lib/src/rust/frb_generated.dart (2)
185-188: Return type uses Dart records; consider a typed alias/DTO for clarity.Future<List<(String, String)>> is valid but opaque. Recommend:
- Add a typedef outside generated code:
typedef RelayStatusRec = (String, String);- Or model as a struct in Rust (e.g., RelayStatus { url, status }) so FRB generates a named type in Dart.
Also verify pubspec SDK is >=3.0.0 since records require Dart 3.
1180-1209: get_account_relay_statuses wiring OK; consider mapping to Map or typed records.
- Encoding pubkey only, decode via sse_decode_list_record_string_string: OK.
- Consider returning Map<String,String> from Rust to generate a map on Dart, or add a mapper/typedef in a non-generated layer for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/config/providers/relay_status_provider.dart(1 hunks)lib/src/rust/api/relays.dart(1 hunks)lib/src/rust/frb_generated.dart(46 hunks)rust/src/api/relays.rs(2 hunks)rust/src/frb_generated.rs(11 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value)
Avoid using dynamic and Object without justification
Create necessary types instead of overusing primitives or dynamic
One export per file
Use PascalCase for classes
Use camelCase for variables, functions, and methods
Avoid magic numbers and define constants
Start each function name with a verb
Use verbs for boolean variables (e.g., isLoading, hasError, canDelete)
Write short functions with a single purpose (under ~20 instructions)
Name functions with a verb plus context; for booleans use isX/hasX/canX; for void use executeX/saveX
Avoid deep nesting via early returns and extraction to utility functions
Use higher-order functions (map, where/filter, reduce) to avoid nesting
Use arrow functions for simple functions (under ~3 statements); use named functions otherwise
Use default parameter values instead of null checks
Reduce function parameters using RO-RO: pass/return parameter objects with declared types
Maintain a single level of abstraction within functions
Encapsulate data in composite types; avoid overusing primitives
Prefer validating data within classes rather than in functions
Prefer immutability; use final for runtime constants and const for compile-time constants
Use const constructors and const literals where possible
Follow SOLID principles
Prefer composition over inheritance
Declare interfaces (abstract classes) to define contracts
Write small classes with a single purpose (under ~200 instructions, <10 public methods, <10 properties)
Use exceptions for unexpected errors
Only catch exceptions to fix expected problems or add context; otherwise use a global handler
Files:
lib/src/rust/api/relays.dartlib/config/providers/relay_status_provider.dartlib/src/rust/frb_generated.dart
lib/**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
lib/**/*.dart: Use flutter_rust_bridge to access core app functionality
Use Riverpod for state management; prefer StreamProviders for Rust API streams; use keepAlive if needed
Use freezed to model/manage UI states
Controllers should expose methods as inputs and update UI state that drives the UI
Use AutoRoute for navigation and use extras to pass data between pages
Use Dart extensions to manage reusable code
Use ThemeData to manage themes
Use AppLocalizations for translations
Use constants to manage constant values
Avoid deeply nested widget trees; aim for a flatter widget structure for performance and readability
Break down large widgets into smaller, focused, reusable components
Keep the widget tree shallow to simplify state management and data flow
Utilize const constructors and const widgets wherever possible to reduce rebuilds
Files:
lib/src/rust/api/relays.dartlib/config/providers/relay_status_provider.dartlib/src/rust/frb_generated.dart
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
Use the whitenoise Rust crate (via flutter_rust_bridge) as the sole data source and change trigger
Files:
rust/src/api/relays.rsrust/src/frb_generated.rs
🧠 Learnings (1)
📚 Learning: 2025-10-12T22:44:49.606Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-10-12T22:44:49.606Z
Learning: Applies to lib/**/*.dart : Use Riverpod for state management; prefer StreamProviders for Rust API streams; use keepAlive if needed
Applied to files:
lib/config/providers/relay_status_provider.dart
🧬 Code graph analysis (1)
rust/src/frb_generated.rs (3)
rust/src/api/relays.rs (2)
ensure_all_subscriptions(73-79)get_account_relay_statuses(41-51)rust/src/api/accounts.rs (1)
export_account_nsec(93-101)rust/src/api/messages.rs (1)
fetch_aggregated_messages_for_group(251-262)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Flutter CI
🔇 Additional comments (18)
lib/src/rust/api/relays.dart (2)
20-23: Binding for getAccountRelayStatuses looks correct.Signature and target symbol match FRB wire. No manual edits needed.
Ensure the Flutter toolchain is Dart ≥ 3.0 due to records in List<(String, String)>.
24-45: ensureAllSubscriptions binding added; OK.Doc mirrors Rust; binding routes to crateApiRelaysEnsureAllSubscriptions.
Confirm at least one call site (e.g., background task/periodic job) will invoke this to realize the maintenance behavior.
rust/src/frb_generated.rs (6)
44-45: Codegen hash updated (auto).No action; keep in sync with generated Dart.
740-774: Wire impl for ensure_all_subscriptions is correct.No params to decode; async wrapper and call path are consistent.
775-811: export_account_nsec wiring/debug name alignment looks correct.Decoding and call target match accounts::export_account_nsec.
812-852: fetch_aggregated_messages_for_group wire includes group_id; good.Added decode and forwarding of api_group_id; matches messages API.
971-1007: Wire impl for get_account_relay_statuses is correct.Decodes pubkey and forwards to relays::get_account_relay_statuses.
3493-3521: Dispatcher table verified; Rust/Dart codegen is synchronized.FRB version 2.11.1 matches across
rust/src/frb_generated.rsandlib/src/rust/api/relays.dart. All three dispatcher IDs (19:ensure_all_subscriptions, 21:fetch_aggregated_messages_for_group, 25:get_account_relay_statuses) are correctly wired on both sides. No stale bindings detected.lib/config/providers/relay_status_provider.dart (1)
89-97: API rename verified complete; all concerns resolved.Verification confirms:
- ✓ Dart SDK constraint (
^3.7.2) supports Dart 3 records- ✓ Old API symbols (
fetchRelayStatus,fetch_relay_status) completely removed- ✓ New API (
getAccountRelayStatuses) correctly wired from FRB binding to call site at line 89- ✓ Records destructuring
(url, status)is valid and supportedlib/src/rust/frb_generated.dart (9)
80-80: Hash bump looks consistent with regenerated native ABI.rustContentHash updated; OK. Ensure the matching native lib from the same generation is shipped in this PR/release artifact.
164-166: New API surface: ensure_all_subscriptions added.Signature Future with no args is fine. Please confirm the Rust op is idempotent and safe to call at app start/resume to avoid duplicate subscriptions.
988-1014: FFI wiring for ensure_all_subscriptions looks correct.funcId: 19, unit codec, no args, constMeta debugName matches. LGTM.
1017-1044: Export NSEC still exposed; confirm intent.crateApiAccountsExportAccountNsec remains (funcId: 20). If the plan was to remove/replace this flow, please confirm we still want this public API; otherwise remove on the Rust side and regenerate.
1047-1080: Messages fetch wiring OK after ID shift.funcId: 21, args encode order [pubkey, groupId], list codec; constMeta names match. LGTM.
Ensure downstream call sites updated if function IDs were previously hard-assumed in mocks.
1211-1238: Accounts list wiring OK after resequencing.funcId: 26, list codec; constMeta matches. LGTM.
1249-1665: Bulk funcId resequencing after new inserts.funcIds advance monotonically (27..64). Nothing suspicious. Please ensure:
- The regenerated native bindings (Rust side) are from the same run.
- No stale frb-generated files remain in platform-specific build dirs.
Also applies to: 1690-2470
2965-2970: Tuple decoders present for (String, String); OK.Both DCO and SSE paths include record decoders; aligns with new API. No action.
Also applies to: 3812-3823
1011-1014: ConstMeta debugName/argNames align with public methods.Names: ensure_all_subscriptions, export_account_nsec, fetch_aggregated_messages_for_group, get_account_relay_statuses. Good consistency for tooling/telemetry.
Also applies to: 1041-1044, 1075-1080, 1206-1209
| Future<List<(String, String)>> crateApiRelaysGetAccountRelayStatuses({ | ||
| required String pubkey, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Migration checks: remove old API usages and add tests.
- Verify no usages of the removed fetchRelayStatus remain.
- Add/adjust tests for getAccountRelayStatuses and ensureAllSubscriptions happy-path and error propagation.
Run this to verify/migrate:
Also applies to: 1180-1209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Dart SDK constraint (records require Dart >=3):"
rg -nC1 '^environment:' pubspec.yaml || true
rg -n "sdk:\s*['\"][^'\"]+['\"]" pubspec.yaml || true
echo -e "\nSearch for removed/old APIs:"
rg -nS "crateApiRelaysFetchRelayStatus|fetchRelayStatus" -g '!**/build/**' || true
echo -e "\nSearch for new APIs usages/wrappers:"
rg -nS "crateApiRelaysGetAccountRelayStatuses|ensureAllSubscriptions" -g '!**/build/**' || trueLength of output: 452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking Dart file type support ==="
which fd && echo "fd available" || echo "fd not available"
echo -e "\n=== Direct search for old API (fetchRelayStatus) ==="
rg "fetchRelayStatus" --type dart || echo "No matches found"
echo -e "\n=== Direct search for new APIs ==="
rg "crateApiRelaysGetAccountRelayStatuses" --type dart || echo "No matches found"
rg "ensureAllSubscriptions" --type dart || echo "No matches found"
echo -e "\n=== Check frb_generated.dart for method definitions ==="
fd "frb_generated.dart" -type f
echo -e "\n=== Inspect the method signatures in frb_generated.dart ==="
fd "frb_generated.dart" -type f -exec grep -n -A3 "crateApiRelaysGetAccountRelayStatuses\|crateApiRelaysFetchRelayStatus" {} +
echo -e "\n=== Search for test files ==="
fd "test" -type d
fd ".*_test.dart" -type f | head -20Length of output: 826
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for test files in common locations ==="
find . -name "*test*.dart" -o -name "*_test.dart" | grep -i relay || echo "No relay-specific test files found"
find . -path "*/test/*" -name "*.dart" 2>/dev/null | head -10 || echo "No test directory found"
echo -e "\n=== Check for test directory structure ==="
ls -la test/ 2>/dev/null || echo "No test/ directory"
ls -la lib/test/ 2>/dev/null || echo "No lib/test/ directory"
echo -e "\n=== Inspect lib/src/rust/api/relays.dart for context ==="
cat lib/src/rust/api/relays.dart
echo -e "\n=== Inspect the two method definitions around lines 185-188 and 1180-1209 ==="
sed -n '185,188p' lib/src/rust/frb_generated.dart
sed -n '1180,1209p' lib/src/rust/frb_generated.dartLength of output: 4931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for tests using getAccountRelayStatuses ==="
grep -r "getAccountRelayStatuses" test/ || echo "No tests found for getAccountRelayStatuses"
echo -e "\n=== Search for tests using ensureAllSubscriptions ==="
grep -r "ensureAllSubscriptions" test/ || echo "No tests found for ensureAllSubscriptions"
echo -e "\n=== Search for tests mocking crateApiRelaysGetAccountRelayStatuses ==="
grep -r "crateApiRelaysGetAccountRelayStatuses" test/ || echo "No mocks found"
echo -e "\n=== Check relay_refresh_logic_test.dart for relay API tests ==="
cat test/ui/settings/network/relay_refresh_logic_test.dart | head -100
echo -e "\n=== Check if provider tests cover relay APIs ==="
grep -l "relays\|Relay" test/config/providers/*test.dart 2>/dev/null || echo "No relay provider tests"Length of output: 3018
Add tests for the new relay APIs before merging.
Old API (fetchRelayStatus) is completely removed and no legacy usages remain. The new APIs (getAccountRelayStatuses and ensureAllSubscriptions) are properly integrated with error handling in lib/src/rust/api/relays.dart. However, tests are missing for these new methods—specifically happy-path and error propagation scenarios for both getAccountRelayStatuses and ensureAllSubscriptions. Add unit tests covering these cases to ensure the migration is complete and the APIs function correctly.
🤖 Prompt for AI Agents
In lib/src/rust/frb_generated.dart around lines 185–188, the review notes that
tests are missing for the newly added relay APIs; add unit tests that cover both
happy-path and error-propagation scenarios for getAccountRelayStatuses and
ensureAllSubscriptions: create tests that mock the underlying rust bindings or
HTTP layer to return successful responses for expected data shapes and assert
the methods return/parses correctly, then create tests that simulate failures
(exceptions, malformed data, network errors) and assert errors propagate or are
converted as expected; place tests alongside existing relay tests (e.g., test/
or test/lib/src/rust) and ensure both positive and negative cases are covered
for each method before merging.
Summary by CodeRabbit
New Features
Improvements