fix: reset Whitenoise lifecycle after deleting data#662
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConvert global Whitenoise access to async handle-based accessors with lifecycle read/write coordination; rewrite initialize_whitenoise() and delete_all_data() to safely recreate the instance; update many Rust API call sites to await wn()/wn_session() and call release_lifecycle() in subscription loops; remove Flutter delete timeout and update tests/docs. ChangesAsync Lifecycle Refactoring
Sequence Diagram(s)sequenceDiagram
participant Client as Flutter UI
participant Bridge as Flutter-Rust bridge
participant API as rust API fn
participant WnAccessor as wn()/wn_session() async accessor
participant Global as GLOBAL_WN (RwLock)
participant Lifecycle as WN_LIFECYCLE_LOCK
Client->>Bridge: user triggers delete-all-data
Bridge->>API: call exported delete_all_data()
API->>WnAccessor: await wn().await?
WnAccessor->>Global: acquire read permit (hold lifecycle)
Global-->>WnAccessor: return WnHandle
API->>Lifecycle: acquire write lock, perform delete, recreate instance
Lifecycle-->>Global: clear and replace GLOBAL_WN
API-->>Bridge: return success/failure
Bridge-->>Client: report result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/api/mod.rs`:
- Around line 230-247: The code takes the global singleton (GLOBAL_WN.take())
and then calls whitenoise.delete_all_data().await, but if delete_all_data fails
the function returns without restoring the taken instance, leaving GLOBAL_WN
unset; change the flow so that if delete_all_data().await returns Err you
reinsert the original whitenoise back into GLOBAL_WN before returning the
ApiError. Concretely, wrap the delete_all_data call (or match its Result) and on
Err acquire GLOBAL_WN.write() (handling the poisoned lock the same way as the
surrounding code) and set it to Some(whitenoise) before propagating the error;
keep the successful-path behavior of dropping the old instance and creating a
new one with Whitenoise::new(core_config) as currently implemented.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0687e31e-cfc4-456d-af12-ef40ccaff8e7
📒 Files selected for processing (18)
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dartrust/src/api/account_groups.rsrust/src/api/accounts.rsrust/src/api/chat_list.rsrust/src/api/chat_summary.rsrust/src/api/drafts.rsrust/src/api/group_state.rsrust/src/api/groups.rsrust/src/api/media_files.rsrust/src/api/messages.rsrust/src/api/mod.rsrust/src/api/mute_list.rsrust/src/api/notifications.rsrust/src/api/relays.rsrust/src/api/signer.rsrust/src/api/user_search.rsrust/src/api/users.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Use single quotes for strings in Dart/Flutter code
Enable and respectprefer_const_constructorslint rule in Dart/Flutter code
Enable and respectprefer_final_localslint rule in Dart/Flutter code
Set line width to 100 characters in Dart/Flutter code
Preserve trailing commas in Dart/Flutter code
Maintain minimum test coverage of 99%; never submit a PR that reduces test coverage
Avoid using// coverage:ignore,// coverage:ignore-line,// coverage:ignore-start, or// coverage:ignore-endto bypass coverage requirements; write tests instead (only exception: truly unreachable code)
Files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
lib/hooks/use_*.dart
📄 CodeRabbit inference engine (AGENTS.md)
lib/hooks/use_*.dart: Hook files should be prefixed withuse_(e.g.,use_chat_list.dart)
Hook functions should start withuse(e.g.,useChatList())
Files:
lib/hooks/use_delete_all_data.dart
**/*.{dart,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add comments except for code that is really complex or hard to understand
Files:
lib/hooks/use_delete_all_data.dartrust/src/api/chat_summary.rsrust/src/api/user_search.rsrust/src/api/drafts.rsrust/src/api/media_files.rsrust/src/api/mute_list.rsrust/src/api/signer.rsrust/src/api/relays.rslib/src/rust/api.dartrust/src/api/group_state.rsrust/src/api/notifications.rsrust/src/api/account_groups.rsrust/src/api/chat_list.rsrust/src/api/messages.rsrust/src/api/users.rsrust/src/api/groups.rsrust/src/api/mod.rsrust/src/api/accounts.rs
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
lib/**/*.dart: Useflutter_screenutilfor all size values (padding, margins, gaps, icon sizes, font sizes, border radius, container dimensions) to ensure responsive layouts. Use.wfor width,.hfor height,.spfor font size/letter spacing, and.rfor radius
Avoid StatefulWidget; prefer providers (for shared app-wide state) or hooks (for widget-local state) instead
Files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
lib/hooks/**
⚙️ CodeRabbit configuration file
lib/hooks/**: This is a flutter_hooks hook (ephemeral widget-local state).
Rules:
- Files must be prefixed with use_ (e.g. use_chat_list.dart)
- Hook functions must start with use (e.g. useChatList())
- Hooks receive data as parameters, not widget refs
- Ensure proper cleanup/dispose of subscriptions and resources
Files:
lib/hooks/use_delete_all_data.dart
rust/src/api/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
rust/src/api/**/*.rs: Rust API modules inrust/src/api/must be exposed to Flutter with#[frb]attribute on functions
Rust structs exposed to Flutter must use#[frb(non_opaque)]attribute for Flutter compatibility
Wrap Rust API errors inApiErrorenum usingthiserrorcrate
Files:
rust/src/api/chat_summary.rsrust/src/api/user_search.rsrust/src/api/drafts.rsrust/src/api/media_files.rsrust/src/api/mute_list.rsrust/src/api/signer.rsrust/src/api/relays.rsrust/src/api/group_state.rsrust/src/api/notifications.rsrust/src/api/account_groups.rsrust/src/api/chat_list.rsrust/src/api/messages.rsrust/src/api/users.rsrust/src/api/groups.rsrust/src/api/mod.rsrust/src/api/accounts.rs
rust/src/api/**
⚙️ CodeRabbit configuration file
rust/src/api/**: This is the Rust API layer exposed to Flutter via flutter_rust_bridge.
Rules:
- Functions use #[frb] attribute for bridge generation
- Structs use #[frb(non_opaque)] for Flutter compatibility
- Errors must be wrapped in the ApiError enum using thiserror
- This is a thin wrapper around the whitenoise crate — keep it thin
- No unwrap() in non-test code; use proper error handling
- Check for correct async patterns
Files:
rust/src/api/chat_summary.rsrust/src/api/user_search.rsrust/src/api/drafts.rsrust/src/api/media_files.rsrust/src/api/mute_list.rsrust/src/api/signer.rsrust/src/api/relays.rsrust/src/api/group_state.rsrust/src/api/notifications.rsrust/src/api/account_groups.rsrust/src/api/chat_list.rsrust/src/api/messages.rsrust/src/api/users.rsrust/src/api/groups.rsrust/src/api/mod.rsrust/src/api/accounts.rs
lib/src/rust/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Do not manually edit files in
lib/src/rust/as they are auto-generated by flutter_rust_bridge
Files:
lib/src/rust/api.dart
🧠 Learnings (25)
📚 Learning: 2026-01-05T20:05:32.918Z
Learnt from: untreu2
Repo: marmot-protocol/sloth PR: 22
File: lib/screens/settings_screen.dart:76-88
Timestamp: 2026-01-05T20:05:32.918Z
Learning: In the Sloth project, tooltips should not be used for UI elements. During code reviews, verify that UI components do not include Tooltip widgets and replace them with accessible alternatives (e.g., long-press hints, labels, or interactive guidance). Apply this guidance across Dart UI files under lib (not just settings_screen.dart) to maintain a consistent design approach.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-01-09T13:25:18.531Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 33
File: lib/services/message_service.dart:14-26
Timestamp: 2026-01-09T13:25:18.531Z
Learning: When errors are presented to users in the Sloth codebase (UI layers such as snackbars, dialogs, toasts), show friendly, user-facing messages instead of raw Rust error messages. Implement a mapping from internal error types or codes to clear, non-technical messages, and surface actionable guidance for end users where appropriate. Avoid exposing internal stack traces or language runtime details; centralize common user-facing error wording in a dedicated utility or service and translate backend errors into concise, helpful UI messages.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-04-06T09:40:41.044Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 552
File: lib/screens/profile_keys_screen.dart:71-71
Timestamp: 2026-04-06T09:40:41.044Z
Learning: In this Flutter/Dart codebase, when using flutter_screenutil (e.g., ScreenUtil or ScreenUtil-based sizing like `w`, `h`), do not require scaled units for literal zero values. Specifically, in EdgeInsets (and similar sizing/padding/margin/gap APIs), bare numeric `0` should be allowed (e.g., `EdgeInsets.all(0)`, `EdgeInsets.symmetric(vertical: 0)`, `SizedBox(width: 0)`), because scaling `0` remains `0`. Only flag ScreenUtil violations when a non-zero literal needs to be expressed via `0.w`/`0.h` equivalents; do not flag bare `0`.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-01-06T01:08:41.552Z
Learnt from: untreu2
Repo: marmot-protocol/sloth PR: 22
File: lib/widgets/wn_warning_box.dart:28-79
Timestamp: 2026-01-06T01:08:41.552Z
Learning: In reviews for the marmot-protocol/sloth repository, avoid suggesting accessibility, design, or general improvements unless they are strictly relevant to the PR description and its stated goals. Focus feedback on the specific PR objectives and requirements; ignore unrelated stylistic or broad improvement suggestions.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-01-15T14:42:54.111Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 39
File: lib/hooks/use_user_search.dart:47-53
Timestamp: 2026-01-15T14:42:54.111Z
Learning: When using npubFromHex(String hexPubkey) from lib/utils/formatting.dart, it already handles errors internally and returns null on failure. Do not wrap calls to this function in try/catch blocks. Instead, check for a null return and handle accordingly (e.g., treat as invalid hex). This guideline applies to all Dart files in the repository that may call this function.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-01-16T17:35:32.431Z
Learnt from: untreu2
Repo: marmot-protocol/sloth PR: 43
File: lib/hooks/use_network_relays.dart:155-184
Timestamp: 2026-01-16T17:35:32.431Z
Learning: In Dart/Flutter code, prefer void as the return type for fire-and-forget async functions that are intended to run in the background without blocking the caller. Using Future or Future<void> would allow callers to accidentally await, which could block for long periods. This pattern makes the call fire-and-forget by design. Apply to functions that intentionally should not be awaited; document the intent where appropriate. (Example context: pollRelayStatus in lib/hooks/use_network_relays.dart uses void to prevent callers from awaiting a potentially long-running background task.)
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-01-19T15:50:56.684Z
Learnt from: erskingardner
Repo: marmot-protocol/sloth PR: 52
File: lib/screens/app_settings_screen.dart:30-40
Timestamp: 2026-01-19T15:50:56.684Z
Learning: Target the Flutter SDK version >= 3.27 across the repo. Since features like Column.spacing and Row.spacing were added in Flutter 3.27, you can safely use them in Dart files (e.g., lib/screens/app_settings_screen.dart) as long as the pubspec.yaml environment specifies Flutter >= 3.27. If needed, enforce this by validating the environment constraint (e.g., flutter, sdk: flutter) in pubspec.yaml.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-01-29T03:02:34.290Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 100
File: lib/widgets/wn_dropdown_selector.dart:126-139
Timestamp: 2026-01-29T03:02:34.290Z
Learning: Global font-family usage guideline: Since Manrope is configured in lib/theme.dart for both light and dark themes, individual TextStyle declarations should not specify fontFamily: 'Manrope'. During reviews, verify that no TextStyle instances override fontFamily unnecessarily; rely on theme inheritance instead. If a TextStyle must specify a font for a specific case, ensure it is clearly justified and localized, and document why the override is needed. This guideline applies across Dart files in the project (e.g., lib/widgets/wn_dropdown_selector.dart and related components).
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-01-29T16:02:52.588Z
Learnt from: erskingardner
Repo: marmot-protocol/sloth PR: 129
File: test/widgets/wn_overlay_test.dart:84-91
Timestamp: 2026-01-29T16:02:52.588Z
Learning: In Dart/Flutter code, when constructing widgets inside an already-const context (for example, within a const Scaffold with a child that is a collection of widgets), avoid adding explicit const keywords to the children. The Dart analyzer treats these as implicitly const, and adding explicit const can trigger the unnecessary_const lint. Review test files and general Dart files to ensure you do not redundantly prefix children with const in const contexts; rely on the implicit const behavior instead.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-02-07T03:58:22.587Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 193
File: lib/services/android_signer_service.dart:13-13
Timestamp: 2026-02-07T03:58:22.587Z
Learning: In Dart files across the repository, retain NIP (Nostr Implementation Possibilities) and MIP (Marmot Implementation Possibilities) protocol reference comments that link code to their specifications. While general guidance discourages extraneous comments, these references improve traceability and maintainability by documenting the protocol context for related implementation details. Keep them in Dart sources (e.g., lib/services/android_signer_service.dart) and ensure they are kept up-to-date.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-02-11T17:29:43.985Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 225
File: lib/screens/app_settings_screen.dart:64-69
Timestamp: 2026-02-11T17:29:43.985Z
Learning: In the whitenoise Flutter app, after deleting all data and resetting auth, use Routes.goToHome(context) instead of Routes.goToLogin(context) because the home screen is the app entry point and will manage authentication routing. Apply this change to navigation calls that should land on the home screen after a data reset or auth reset. Update lib/screens/app_settings_screen.dart and any similar navigation points accordingly.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-02-18T18:36:13.394Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 315
File: lib/screens/edit_group_screen.dart:45-47
Timestamp: 2026-02-18T18:36:13.394Z
Learning: Whitenoise uses automated code formatting through the precommit workflow (just precommit). Do not tweak line wrapping or formatting manually; follow the formatter's output. Run the precommit formatter locally and ensure the code matches its styling decisions, and only deviate if the formatter cannot express the intended style (in which case adjust the code to satisfy the formatter).
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-04-06T09:36:06.726Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 553
File: lib/widgets/wn_copyable_field.dart:38-40
Timestamp: 2026-04-06T09:36:06.726Z
Learning: In Dart, `String.operator*` is a valid built-in operator for repeating strings (e.g., `'⬤' * 16`). In code reviews of Dart (`.dart`) files, do not treat expressions like `'text' * n` as invalid syntax and do not recommend replacing them with alternatives such as `List.filled(...).join()`; the operator is supported by `dart:core`.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-05-13T13:46:32.612Z
Learnt from: dannym-arx
Repo: marmot-protocol/whitenoise PR: 656
File: lib/hooks/use_block_actions.dart:3-3
Timestamp: 2026-05-13T13:46:32.612Z
Learning: In marmot-protocol/whitenoise code reviews, do not flag imports that reference FRB-generated bindings under `package:rust_lib_whitenoise/src/rust/...` as private-internals violations. In `rust_lib_whitenoise`, Dart bindings are intentionally generated into `lib/src/rust/`, and `analysis_options.yaml` sets `implementation_imports: false` to suppress the related Dart lint—so `package:rust_lib_whitenoise/src/rust/...` imports are the correct/only way to consume that FRB-generated API.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-05-13T13:46:39.250Z
Learnt from: dannym-arx
Repo: marmot-protocol/whitenoise PR: 656
File: lib/services/foreground_service.dart:13-16
Timestamp: 2026-05-13T13:46:39.250Z
Learning: In this repo (marmot-protocol/whitenoise), `rust_lib_whitenoise` is an FRB-generated Flutter package where the generated Dart bindings are intentionally exposed under `lib/src/rust/` as the public API surface. Do not treat imports like `package:rust_lib_whitenoise/src/rust/...` as breaking package API boundaries in code reviews (the project also sets `implementation_imports: false` in `analysis_options.yaml` to opt into this). Never raise concerns about `src/` imports from `rust_lib_whitenoise`.
Applied to files:
lib/hooks/use_delete_all_data.dartlib/src/rust/api.dart
📚 Learning: 2026-01-22T20:15:04.277Z
Learnt from: untreu2
Repo: marmot-protocol/sloth PR: 78
File: lib/hooks/use_accounts.dart:21-38
Timestamp: 2026-01-22T20:15:04.277Z
Learning: In the Sloth Flutter app, for hook files under lib/hooks (e.g., use_accounts.dart), prefer logging errors using the project's logger (e.g., _logger.severe) rather than displaying SnackBars. SnackBars should not be used in hooks or for error handling; log errors centrally and surface user notifications only at UI layers. Ensure consistent use of logging severity and avoid UI side effects within hooks.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-05T20:27:05.455Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 193
File: lib/hooks/use_android_signer.dart:38-41
Timestamp: 2026-02-05T20:27:05.455Z
Learning: In Dart files under lib/hooks that use useMemoized (flutter_hooks), if a platform check like platformIsAndroid is used only to construct a service and the platform will not change at runtime, you can omit that platform check from the useMemoized dependency list. Do not include invariant platform values in the dependencies, which prevents unnecessary recomputations. Apply this pattern to all relevant files in lib/hooks where the platform is constant over the app lifecycle.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-11T17:51:41.426Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 225
File: lib/hooks/use_delete_all_data.dart:34-49
Timestamp: 2026-02-11T17:51:41.426Z
Learning: In the whitenoise Flutter app, within custom hooks under lib/hooks, do not wrap returned functions with useCallback. Define functions inline inside the hook body to maintain consistency across all hooks. This aligns with the project-wide convention for consistency. If a function must be stable across rebuilds, consider using appropriate Dart/Flutter patterns (e.g., local closures or memoization strategies) per the framework's guidance.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-11T23:40:32.726Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 225
File: lib/hooks/use_delete_all_data.dart:7-24
Timestamp: 2026-02-11T23:40:32.726Z
Learning: In the whitenoise Flutter app, internal hook state classes (e.g., DeleteAllDataState in use_delete_all_data.dart) should implement manual copyWith methods rather than using Freezed. The Freezed package is appropriate for UI states, but not for hook-internal state classes. During reviews, flag hook state implementations that rely on Freezed and prefer explicit, hand-written copyWith for reliability and lighter dependencies. Apply this pattern broadly to all Dart files under lib/hooks/*.dart and any similar hook directories.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-04-02T18:58:10.024Z
Learnt from: dannym-arx
Repo: marmot-protocol/whitenoise PR: 494
File: lib/hooks/use_scroll_to_message.dart:17-18
Timestamp: 2026-04-02T18:58:10.024Z
Learning: When reviewing Flutter/Dart hooks that use `flutter_hooks` (e.g., `useRef` / “latest-value ref” pattern), do not treat `useRef.current`/`ref.value` reads inside async loops as a stale-closure bug. If a parameter like `hasMoreMessages` is passed into the hook and immediately synced at the top of the build (`ref.value = hasMoreMessages`), then async code should read the latest `ref.value` on each iteration, reflecting the most recent rebuild triggered by state changes. The stale-closure concern should apply to captured local variables that are not updated via a `useRef`, not to `useRef`-backed fields.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-07T04:45:18.077Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 193
File: lib/hooks/use_nsec.dart:76-79
Timestamp: 2026-02-07T04:45:18.077Z
Learning: In Flutter, localization strings should be resolved and presented by widgets/screens, not inside hooks. Hooks (like those under lib/hooks) should return error codes (e.g., nsec_load_failed). Widgets should provide helper functions to map these codes to localized strings via AppLocalizations (e.g., _noticeMessageL10n, _signerErrorL10n). Apply this pattern across all hook files: hub on returning error codes, while UI components handle localization lookups using AppLocalizations.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-01-15T23:30:10.907Z
Learnt from: jgmontoya
Repo: marmot-protocol/sloth PR: 42
File: rust/src/api/account_groups.rs:107-123
Timestamp: 2026-01-15T23:30:10.907Z
Learning: In the Rust API layer under rust/src/api/, ensure functions remain thin translation wrappers that delegate business logic to the whitenoise-rs crate. Do not implement business logic or fixes in the sloth API layer; instead, move such changes to whitenoise-rs and keep the API layer focused on request/response translation and orchestration.
Applied to files:
rust/src/api/chat_summary.rsrust/src/api/user_search.rsrust/src/api/drafts.rsrust/src/api/media_files.rsrust/src/api/mute_list.rsrust/src/api/signer.rsrust/src/api/relays.rsrust/src/api/group_state.rsrust/src/api/notifications.rsrust/src/api/account_groups.rsrust/src/api/chat_list.rsrust/src/api/messages.rsrust/src/api/users.rsrust/src/api/groups.rsrust/src/api/mod.rsrust/src/api/accounts.rs
📚 Learning: 2026-01-15T23:30:10.907Z
Learnt from: jgmontoya
Repo: marmot-protocol/sloth PR: 42
File: rust/src/api/account_groups.rs:107-123
Timestamp: 2026-01-15T23:30:10.907Z
Learning: Maintain a consistent function start-up order in the Rust module: for functions like accept_account_group, decline_account_group, and mark_message_read in rust/src/api/account_groups.rs (and other files in rust/src/api), call Whitenoise::get_instance() first, then parse input parameters. This pattern should be applied across the entire module to improve readability and consistency.
Applied to files:
rust/src/api/chat_summary.rsrust/src/api/user_search.rsrust/src/api/drafts.rsrust/src/api/media_files.rsrust/src/api/mute_list.rsrust/src/api/signer.rsrust/src/api/relays.rsrust/src/api/group_state.rsrust/src/api/notifications.rsrust/src/api/account_groups.rsrust/src/api/chat_list.rsrust/src/api/messages.rsrust/src/api/users.rsrust/src/api/groups.rsrust/src/api/mod.rsrust/src/api/accounts.rs
📚 Learning: 2026-01-19T10:45:49.603Z
Learnt from: erskingardner
Repo: marmot-protocol/sloth PR: 52
File: rust/src/api/utils.rs:62-83
Timestamp: 2026-01-19T10:45:49.603Z
Learning: In rust/src/api, ensure that FFI glue code for flutter_rust_bridge (e.g., callable functions exposing enum variants or wrapping Display/FromStr) lives in the bridge layer, not in whitenoise-rs. Only business logic should be delegated to whitenoise-rs. Simple enum variant constructors and FFI-specific wrappers belong in the bridge layer.
Applied to files:
rust/src/api/chat_summary.rsrust/src/api/user_search.rsrust/src/api/drafts.rsrust/src/api/media_files.rsrust/src/api/mute_list.rsrust/src/api/signer.rsrust/src/api/relays.rsrust/src/api/group_state.rsrust/src/api/notifications.rsrust/src/api/account_groups.rsrust/src/api/chat_list.rsrust/src/api/messages.rsrust/src/api/users.rsrust/src/api/groups.rsrust/src/api/mod.rsrust/src/api/accounts.rs
📚 Learning: 2026-02-10T23:51:16.714Z
Learnt from: jgmontoya
Repo: marmot-protocol/whitenoise PR: 209
File: rust/src/api/user_search.rs:51-111
Timestamp: 2026-02-10T23:51:16.714Z
Learning: In Rust code reviews, treat usize to u64 as a safe widening on all supported platforms (usize <= 64 bits). Do not suggest try_from for usize -> u64 conversions. Be cautious about the reverse direction (u64 -> usize) on 32-bit platforms where truncation can occur. When reviewing, prefer direct as-casting or explicit type as appropriate for usize to u64; avoid try_from for this specific conversion.
Applied to files:
rust/src/api/chat_summary.rsrust/src/api/user_search.rsrust/src/api/drafts.rsrust/src/api/media_files.rsrust/src/api/mute_list.rsrust/src/api/signer.rsrust/src/api/relays.rsrust/src/api/group_state.rsrust/src/api/notifications.rsrust/src/api/account_groups.rsrust/src/api/chat_list.rsrust/src/api/messages.rsrust/src/api/users.rsrust/src/api/groups.rsrust/src/api/mod.rsrust/src/api/accounts.rs
🔇 Additional comments (22)
rust/src/api/mod.rs (1)
14-79: LGTM!Also applies to: 108-114, 188-220, 286-343
rust/src/api/group_state.rs (1)
43-75: LGTM!rust/src/api/chat_summary.rs (1)
83-99: LGTM!rust/src/api/mute_list.rs (1)
29-77: LGTM!rust/src/api/drafts.rs (1)
51-92: LGTM!rust/src/api/media_files.rs (1)
118-160: LGTM!rust/src/api/chat_list.rs (1)
132-202: LGTM!Also applies to: 212-313
rust/src/api/account_groups.rs (1)
75-169: LGTM!rust/src/api/relays.rs (1)
43-43: LGTM!Also applies to: 52-52
lib/src/rust/api.dart (1)
11-13: LGTM!Also applies to: 45-48
lib/hooks/use_delete_all_data.dart (1)
9-9: LGTM!rust/src/api/signer.rs (1)
161-161: LGTM!Also applies to: 197-197, 222-222, 238-238
rust/src/api/user_search.rs (1)
154-154: LGTM!Also applies to: 166-166
rust/src/api/notifications.rs (1)
83-83: LGTM!Also applies to: 86-86, 164-164, 184-184, 195-195
rust/src/api/messages.rs (1)
434-434: LGTM!Also applies to: 453-453, 479-479, 500-500, 519-519, 553-553, 576-576, 610-610, 672-672
rust/src/api/groups.rs (1)
94-94: LGTM!Also applies to: 105-105, 116-116, 133-133, 221-221, 230-230, 239-239, 259-259, 300-300, 320-320, 340-340, 356-356, 373-373, 377-377, 387-387, 398-398, 409-409, 427-427, 441-441, 480-480, 503-503, 543-543, 583-583
rust/src/api/users.rs (2)
100-114: LGTM!
141-143: LGTM!Also applies to: 152-154, 164-166, 176-178
rust/src/api/accounts.rs (4)
128-131: LGTM!Also applies to: 135-139, 143-146, 154-157, 161-165, 172-179, 183-187, 191-194, 198-205
212-219: LGTM!Also applies to: 228-241, 245-250, 254-277
285-294: LGTM!Also applies to: 302-311, 315-320
325-328: LGTM!Also applies to: 333-339, 347-354, 359-362, 367-370, 378-385, 393-400, 408-415, 433-442, 449-461
mubarakcoded
force-pushed
the
fix/delete-all-data-reset-lifecycle
branch
from
2d44560 to
fa8f533
Compare
✅ Coverage: 99.03% → 99.03% (0.00%)History
|
mubarakcoded
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/api/mod.rs`:
- Around line 244-252: Currently the code clears GLOBAL_WN before calling
Whitenoise::new(core_config).await, which leaves GLOBAL_WN as None if
Whitenoise::new fails; instead preserve and restore state: read and store the
previous GLOBAL_WN value, only replace it with None after delete_all_data() if
you will immediately replace it with the new instance, or better, delay setting
GLOBAL_WN to None until after Whitenoise::new succeeds; if Whitenoise::new
returns Err, restore the saved previous value (or set a minimal fallback) and
return the error so callers aren't left in an uninitialized state (refer to
GLOBAL_WN, delete_all_data(), Whitenoise::new, and initialize_whitenoise()).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11cb81df-e00d-484f-b09a-24d36d76eca7
📒 Files selected for processing (1)
rust/src/api/mod.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add comments except for code that is really complex or hard to understand
Files:
rust/src/api/mod.rs
rust/src/api/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
rust/src/api/**/*.rs: Rust API modules inrust/src/api/must be exposed to Flutter with#[frb]attribute on functions
Rust structs exposed to Flutter must use#[frb(non_opaque)]attribute for Flutter compatibility
Wrap Rust API errors inApiErrorenum usingthiserrorcrate
Files:
rust/src/api/mod.rs
rust/src/api/**
⚙️ CodeRabbit configuration file
rust/src/api/**: This is the Rust API layer exposed to Flutter via flutter_rust_bridge.
Rules:
- Functions use #[frb] attribute for bridge generation
- Structs use #[frb(non_opaque)] for Flutter compatibility
- Errors must be wrapped in the ApiError enum using thiserror
- This is a thin wrapper around the whitenoise crate — keep it thin
- No unwrap() in non-test code; use proper error handling
- Check for correct async patterns
Files:
rust/src/api/mod.rs
🧠 Learnings (4)
📚 Learning: 2026-01-15T23:30:10.907Z
Learnt from: jgmontoya
Repo: marmot-protocol/sloth PR: 42
File: rust/src/api/account_groups.rs:107-123
Timestamp: 2026-01-15T23:30:10.907Z
Learning: In the Rust API layer under rust/src/api/, ensure functions remain thin translation wrappers that delegate business logic to the whitenoise-rs crate. Do not implement business logic or fixes in the sloth API layer; instead, move such changes to whitenoise-rs and keep the API layer focused on request/response translation and orchestration.
Applied to files:
rust/src/api/mod.rs
📚 Learning: 2026-01-15T23:30:10.907Z
Learnt from: jgmontoya
Repo: marmot-protocol/sloth PR: 42
File: rust/src/api/account_groups.rs:107-123
Timestamp: 2026-01-15T23:30:10.907Z
Learning: Maintain a consistent function start-up order in the Rust module: for functions like accept_account_group, decline_account_group, and mark_message_read in rust/src/api/account_groups.rs (and other files in rust/src/api), call Whitenoise::get_instance() first, then parse input parameters. This pattern should be applied across the entire module to improve readability and consistency.
Applied to files:
rust/src/api/mod.rs
📚 Learning: 2026-01-19T10:45:49.603Z
Learnt from: erskingardner
Repo: marmot-protocol/sloth PR: 52
File: rust/src/api/utils.rs:62-83
Timestamp: 2026-01-19T10:45:49.603Z
Learning: In rust/src/api, ensure that FFI glue code for flutter_rust_bridge (e.g., callable functions exposing enum variants or wrapping Display/FromStr) lives in the bridge layer, not in whitenoise-rs. Only business logic should be delegated to whitenoise-rs. Simple enum variant constructors and FFI-specific wrappers belong in the bridge layer.
Applied to files:
rust/src/api/mod.rs
📚 Learning: 2026-02-10T23:51:16.714Z
Learnt from: jgmontoya
Repo: marmot-protocol/whitenoise PR: 209
File: rust/src/api/user_search.rs:51-111
Timestamp: 2026-02-10T23:51:16.714Z
Learning: In Rust code reviews, treat usize to u64 as a safe widening on all supported platforms (usize <= 64 bits). Do not suggest try_from for usize -> u64 conversions. Be cautious about the reverse direction (u64 -> usize) on 32-bit platforms where truncation can occur. When reviewing, prefer direct as-casting or explicit type as appropriate for usize to u64; avoid try_from for this specific conversion.
Applied to files:
rust/src/api/mod.rs
🔇 Additional comments (8)
rust/src/api/mod.rs (8)
1-12: LGTM!
14-33: LGTM!
35-49: LGTM!
51-67: LGTM!
69-79: LGTM!
188-220: LGTM!
257-289: LGTM!
291-348: LGTM!
…-reset-lifecycle # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/api/mod.rs`:
- Around line 320-351: The test
stream_receive_loops_release_lifecycle_before_waiting performs fragile
source-string scanning for the pattern "let mut rx =", "rx.recv().await" and
expects "release_lifecycle()" after receiver setup; add a short inline comment
above this test describing the exact pattern it enforces (i.e., it looks for
receiver declaration "let mut rx =", subsequent "rx.recv().await" loop(s), and a
following "release_lifecycle()" call) and note that changing receiver naming,
loop structure, or multiple receivers will break the check so future changes
should update the test accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c09d79d3-c359-41d4-9632-d54e2eb546df
📒 Files selected for processing (2)
CHANGELOG.mdrust/src/api/mod.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{dart,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add comments except for code that is really complex or hard to understand
Files:
rust/src/api/mod.rs
rust/src/api/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
rust/src/api/**/*.rs: Rust API modules inrust/src/api/must be exposed to Flutter with#[frb]attribute on functions
Rust structs exposed to Flutter must use#[frb(non_opaque)]attribute for Flutter compatibility
Wrap Rust API errors inApiErrorenum usingthiserrorcrate
Files:
rust/src/api/mod.rs
rust/src/api/**
⚙️ CodeRabbit configuration file
rust/src/api/**: This is the Rust API layer exposed to Flutter via flutter_rust_bridge.
Rules:
- Functions use #[frb] attribute for bridge generation
- Structs use #[frb(non_opaque)] for Flutter compatibility
- Errors must be wrapped in the ApiError enum using thiserror
- This is a thin wrapper around the whitenoise crate — keep it thin
- No unwrap() in non-test code; use proper error handling
- Check for correct async patterns
Files:
rust/src/api/mod.rs
🧠 Learnings (4)
📚 Learning: 2026-01-15T23:30:10.907Z
Learnt from: jgmontoya
Repo: marmot-protocol/sloth PR: 42
File: rust/src/api/account_groups.rs:107-123
Timestamp: 2026-01-15T23:30:10.907Z
Learning: In the Rust API layer under rust/src/api/, ensure functions remain thin translation wrappers that delegate business logic to the whitenoise-rs crate. Do not implement business logic or fixes in the sloth API layer; instead, move such changes to whitenoise-rs and keep the API layer focused on request/response translation and orchestration.
Applied to files:
rust/src/api/mod.rs
📚 Learning: 2026-01-15T23:30:10.907Z
Learnt from: jgmontoya
Repo: marmot-protocol/sloth PR: 42
File: rust/src/api/account_groups.rs:107-123
Timestamp: 2026-01-15T23:30:10.907Z
Learning: Maintain a consistent function start-up order in the Rust module: for functions like accept_account_group, decline_account_group, and mark_message_read in rust/src/api/account_groups.rs (and other files in rust/src/api), call Whitenoise::get_instance() first, then parse input parameters. This pattern should be applied across the entire module to improve readability and consistency.
Applied to files:
rust/src/api/mod.rs
📚 Learning: 2026-01-19T10:45:49.603Z
Learnt from: erskingardner
Repo: marmot-protocol/sloth PR: 52
File: rust/src/api/utils.rs:62-83
Timestamp: 2026-01-19T10:45:49.603Z
Learning: In rust/src/api, ensure that FFI glue code for flutter_rust_bridge (e.g., callable functions exposing enum variants or wrapping Display/FromStr) lives in the bridge layer, not in whitenoise-rs. Only business logic should be delegated to whitenoise-rs. Simple enum variant constructors and FFI-specific wrappers belong in the bridge layer.
Applied to files:
rust/src/api/mod.rs
📚 Learning: 2026-02-10T23:51:16.714Z
Learnt from: jgmontoya
Repo: marmot-protocol/whitenoise PR: 209
File: rust/src/api/user_search.rs:51-111
Timestamp: 2026-02-10T23:51:16.714Z
Learning: In Rust code reviews, treat usize to u64 as a safe widening on all supported platforms (usize <= 64 bits). Do not suggest try_from for usize -> u64 conversions. Be cautious about the reverse direction (u64 -> usize) on 32-bit platforms where truncation can occur. When reviewing, prefer direct as-casting or explicit type as appropriate for usize to u64; avoid try_from for this specific conversion.
Applied to files:
rust/src/api/mod.rs
🔇 Additional comments (9)
CHANGELOG.md (1)
32-32: LGTM!rust/src/api/mod.rs (8)
1-12: LGTM!
14-49: LGTM!
51-79: LGTM!
108-114: LGTM!
188-220: LGTM!
222-259: LGTM!
261-293: LGTM!
301-318: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/api/mod.rs`:
- Around line 327-334: The test currently hardcodes stream_files as a fixed
array which can miss new API modules; replace that array with dynamic directory
scanning: use std::fs::read_dir on
Path::new(env!("CARGO_MANIFEST_DIR")).join("src").join("api") to collect all
entries ending with ".rs", filter out "mod.rs" (and any non-file entries), and
build a Vec<String> of filenames (e.g., "messages.rs") to use wherever
stream_files was used; update any references to stream_files (the variable in
the test and the release_lifecycle() check) to use this dynamically generated
Vec so new API modules are automatically included.
In `@test/hooks/use_delete_all_data_test.dart`:
- Around line 172-176: The test currently asserts expect(resultFuture,
doesNotComplete) but then completes the same future
(mockApi.deleteAllDataCompleter!.complete()), which contradicts the expectation;
instead, register a completion callback on resultFuture (e.g.
resultFuture.then((_) => completed = true)) or check the Completer's state
directly (mockApi.deleteAllDataCompleter!.isCompleted) after awaiting
tester.pump(const Duration(seconds: 2)), assert that the future has not
completed yet (expect(completed, isFalse) or expect(isCompleted, isFalse)), then
call mockApi.deleteAllDataCompleter!.complete() and await resultFuture to finish
the test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fea41965-ab9d-490a-8d7c-d6096a71d31b
📒 Files selected for processing (3)
lib/hooks/use_delete_all_data.dartrust/src/api/mod.rstest/hooks/use_delete_all_data_test.dart
📜 Review details
⏰ 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). (5)
- GitHub Check: Test (2/4)
- GitHub Check: Test (1/4)
- GitHub Check: Test (4/4)
- GitHub Check: Test (3/4)
- GitHub Check: Check
🧰 Additional context used
📓 Path-based instructions (9)
**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
**/*.dart: Use single quotes for strings in Dart/Flutter code
Enable and respectprefer_const_constructorslint rule in Dart/Flutter code
Enable and respectprefer_final_localslint rule in Dart/Flutter code
Set line width to 100 characters in Dart/Flutter code
Preserve trailing commas in Dart/Flutter code
Maintain minimum test coverage of 99%; never submit a PR that reduces test coverage
Avoid using// coverage:ignore,// coverage:ignore-line,// coverage:ignore-start, or// coverage:ignore-endto bypass coverage requirements; write tests instead (only exception: truly unreachable code)
Files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
**/*.{dart,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Do not add comments except for code that is really complex or hard to understand
Files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dartrust/src/api/mod.rs
test/**/*_test.dart
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*_test.dart: Test files should mirror source structure with_test.dartsuffix
Use helpers fromtest/test_helpers.dartin tests:setUpTestView(),mountTestApp(),mountHook(),mountWidget(),mountStackedWidget()
Mock Rust API by extendingMockWnApifromtest/mocks/mock_wn_api.dartinstead of implementingRustLibApidirectly
Preferfind.byKey()overfind.byIcon()in tests; add keys to icons in widgets and usefind.byKey(const Key('icon_name'))in tests
Use valid 64-character hex strings for pubkeys in tests, not dummy values like'abc'or'test-pubkey'
Files:
test/hooks/use_delete_all_data_test.dart
test/**
⚙️ CodeRabbit configuration file
test/**: IMPORTANT: CI enforces coverage regression (coverage must never decrease). It does not enforce a fixed 95% minimum threshold.
Rules:
- Test files mirror source structure with _test.dart suffix
- Use helpers from test/test_helpers.dart (setUpTestView, mountTestApp, etc.)
- Mock Rust API using RustLib.initMock(api: mockApi)
- Always extend MockWnApi from test/mocks/mock_wn_api.dart
- Prefer find.byKey() over find.byIcon() for widget testing
- Use valid 64-char hex strings for pubkeys, not dummy values like 'abc'
- Tests must be deterministic — no external service dependencies
Files:
test/hooks/use_delete_all_data_test.dart
lib/hooks/use_*.dart
📄 CodeRabbit inference engine (AGENTS.md)
lib/hooks/use_*.dart: Hook files should be prefixed withuse_(e.g.,use_chat_list.dart)
Hook functions should start withuse(e.g.,useChatList())
Files:
lib/hooks/use_delete_all_data.dart
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
lib/**/*.dart: Useflutter_screenutilfor all size values (padding, margins, gaps, icon sizes, font sizes, border radius, container dimensions) to ensure responsive layouts. Use.wfor width,.hfor height,.spfor font size/letter spacing, and.rfor radius
Avoid StatefulWidget; prefer providers (for shared app-wide state) or hooks (for widget-local state) instead
Files:
lib/hooks/use_delete_all_data.dart
lib/hooks/**
⚙️ CodeRabbit configuration file
lib/hooks/**: This is a flutter_hooks hook (ephemeral widget-local state).
Rules:
- Files must be prefixed with use_ (e.g. use_chat_list.dart)
- Hook functions must start with use (e.g. useChatList())
- Hooks receive data as parameters, not widget refs
- Ensure proper cleanup/dispose of subscriptions and resources
Files:
lib/hooks/use_delete_all_data.dart
rust/src/api/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
rust/src/api/**/*.rs: Rust API modules inrust/src/api/must be exposed to Flutter with#[frb]attribute on functions
Rust structs exposed to Flutter must use#[frb(non_opaque)]attribute for Flutter compatibility
Wrap Rust API errors inApiErrorenum usingthiserrorcrate
Files:
rust/src/api/mod.rs
rust/src/api/**
⚙️ CodeRabbit configuration file
rust/src/api/**: This is the Rust API layer exposed to Flutter via flutter_rust_bridge.
Rules:
- Functions use #[frb] attribute for bridge generation
- Structs use #[frb(non_opaque)] for Flutter compatibility
- Errors must be wrapped in the ApiError enum using thiserror
- This is a thin wrapper around the whitenoise crate — keep it thin
- No unwrap() in non-test code; use proper error handling
- Check for correct async patterns
Files:
rust/src/api/mod.rs
🧠 Learnings (32)
📚 Learning: 2026-01-05T21:28:05.652Z
Learnt from: untreu2
Repo: marmot-protocol/sloth PR: 22
File: test/screens/profile_keys_screen_test.dart:14-23
Timestamp: 2026-01-05T21:28:05.652Z
Learning: In test mocks for the marmot-protocol/sloth project, allow skipping bounds checking for substring operations when test inputs are controlled and have proven sufficient length (e.g., 'test_pubkey' is always 12 characters). Favor simpler, deterministic mock implementations in such controlled test contexts, but avoid relaxing bounds checks in production code or tests with variable inputs. Apply this guideline primarily to test files under the test/ directory.
Applied to files:
test/hooks/use_delete_all_data_test.dart
📚 Learning: 2026-01-07T16:49:18.694Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 33
File: test/screens/chat_screen_test.dart:245-282
Timestamp: 2026-01-07T16:49:18.694Z
Learning: In the marmot-protocol/sloth repository, it is acceptable to use force-unwrapping (!) in Dart test files as an implicit assertion: if a value is unexpectedly null, the test will fail. This guideline applies only to test code under the test/ directory and should not be used in production code.
Applied to files:
test/hooks/use_delete_all_data_test.dart
📚 Learning: 2026-01-11T22:43:09.610Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 36
File: test/screens/chat_list_screen_test.dart:74-76
Timestamp: 2026-01-11T22:43:09.610Z
Learning: In Dart tests (e.g., test/screens/chat_list_screen_test.dart and related files), use setUp() to call _api.reset() to clean up StreamController resources. Do not suggest or rely on tearDownAll for this cleanup.
Applied to files:
test/hooks/use_delete_all_data_test.dart
📚 Learning: 2026-04-14T03:52:37.716Z
Learnt from: josefinalliende
Repo: marmot-protocol/whitenoise PR: 565
File: test/providers/offline_provider_test.dart:63-177
Timestamp: 2026-04-14T03:52:37.716Z
Learning: In this repository, it’s acceptable to use fixed-time waits for test timing using `Future.delayed` (e.g., `await Future.delayed(const Duration(milliseconds: 10))`) inside test files. During code reviews, don’t recommend replacing these fixed sleeps with event-based or poll-based synchronization when the change is localized to Dart test files under the `test/` directory.
Applied to files:
test/hooks/use_delete_all_data_test.dart
📚 Learning: 2026-01-06T01:08:41.552Z
Learnt from: untreu2
Repo: marmot-protocol/sloth PR: 22
File: lib/widgets/wn_warning_box.dart:28-79
Timestamp: 2026-01-06T01:08:41.552Z
Learning: In reviews for the marmot-protocol/sloth repository, avoid suggesting accessibility, design, or general improvements unless they are strictly relevant to the PR description and its stated goals. Focus feedback on the specific PR objectives and requirements; ignore unrelated stylistic or broad improvement suggestions.
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-01-15T14:42:54.111Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 39
File: lib/hooks/use_user_search.dart:47-53
Timestamp: 2026-01-15T14:42:54.111Z
Learning: When using npubFromHex(String hexPubkey) from lib/utils/formatting.dart, it already handles errors internally and returns null on failure. Do not wrap calls to this function in try/catch blocks. Instead, check for a null return and handle accordingly (e.g., treat as invalid hex). This guideline applies to all Dart files in the repository that may call this function.
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-01-16T17:35:32.431Z
Learnt from: untreu2
Repo: marmot-protocol/sloth PR: 43
File: lib/hooks/use_network_relays.dart:155-184
Timestamp: 2026-01-16T17:35:32.431Z
Learning: In Dart/Flutter code, prefer void as the return type for fire-and-forget async functions that are intended to run in the background without blocking the caller. Using Future or Future<void> would allow callers to accidentally await, which could block for long periods. This pattern makes the call fire-and-forget by design. Apply to functions that intentionally should not be awaited; document the intent where appropriate. (Example context: pollRelayStatus in lib/hooks/use_network_relays.dart uses void to prevent callers from awaiting a potentially long-running background task.)
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-01-19T15:50:56.684Z
Learnt from: erskingardner
Repo: marmot-protocol/sloth PR: 52
File: lib/screens/app_settings_screen.dart:30-40
Timestamp: 2026-01-19T15:50:56.684Z
Learning: Target the Flutter SDK version >= 3.27 across the repo. Since features like Column.spacing and Row.spacing were added in Flutter 3.27, you can safely use them in Dart files (e.g., lib/screens/app_settings_screen.dart) as long as the pubspec.yaml environment specifies Flutter >= 3.27. If needed, enforce this by validating the environment constraint (e.g., flutter, sdk: flutter) in pubspec.yaml.
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-01-29T03:02:34.290Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 100
File: lib/widgets/wn_dropdown_selector.dart:126-139
Timestamp: 2026-01-29T03:02:34.290Z
Learning: Global font-family usage guideline: Since Manrope is configured in lib/theme.dart for both light and dark themes, individual TextStyle declarations should not specify fontFamily: 'Manrope'. During reviews, verify that no TextStyle instances override fontFamily unnecessarily; rely on theme inheritance instead. If a TextStyle must specify a font for a specific case, ensure it is clearly justified and localized, and document why the override is needed. This guideline applies across Dart files in the project (e.g., lib/widgets/wn_dropdown_selector.dart and related components).
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-01-29T16:02:52.588Z
Learnt from: erskingardner
Repo: marmot-protocol/sloth PR: 129
File: test/widgets/wn_overlay_test.dart:84-91
Timestamp: 2026-01-29T16:02:52.588Z
Learning: In Dart/Flutter code, when constructing widgets inside an already-const context (for example, within a const Scaffold with a child that is a collection of widgets), avoid adding explicit const keywords to the children. The Dart analyzer treats these as implicitly const, and adding explicit const can trigger the unnecessary_const lint. Review test files and general Dart files to ensure you do not redundantly prefix children with const in const contexts; rely on the implicit const behavior instead.
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-07T03:58:22.587Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 193
File: lib/services/android_signer_service.dart:13-13
Timestamp: 2026-02-07T03:58:22.587Z
Learning: In Dart files across the repository, retain NIP (Nostr Implementation Possibilities) and MIP (Marmot Implementation Possibilities) protocol reference comments that link code to their specifications. While general guidance discourages extraneous comments, these references improve traceability and maintainability by documenting the protocol context for related implementation details. Keep them in Dart sources (e.g., lib/services/android_signer_service.dart) and ensure they are kept up-to-date.
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-11T17:29:43.985Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 225
File: lib/screens/app_settings_screen.dart:64-69
Timestamp: 2026-02-11T17:29:43.985Z
Learning: In the whitenoise Flutter app, after deleting all data and resetting auth, use Routes.goToHome(context) instead of Routes.goToLogin(context) because the home screen is the app entry point and will manage authentication routing. Apply this change to navigation calls that should land on the home screen after a data reset or auth reset. Update lib/screens/app_settings_screen.dart and any similar navigation points accordingly.
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-18T18:36:13.394Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 315
File: lib/screens/edit_group_screen.dart:45-47
Timestamp: 2026-02-18T18:36:13.394Z
Learning: Whitenoise uses automated code formatting through the precommit workflow (just precommit). Do not tweak line wrapping or formatting manually; follow the formatter's output. Run the precommit formatter locally and ensure the code matches its styling decisions, and only deviate if the formatter cannot express the intended style (in which case adjust the code to satisfy the formatter).
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-04-06T09:36:06.726Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 553
File: lib/widgets/wn_copyable_field.dart:38-40
Timestamp: 2026-04-06T09:36:06.726Z
Learning: In Dart, `String.operator*` is a valid built-in operator for repeating strings (e.g., `'⬤' * 16`). In code reviews of Dart (`.dart`) files, do not treat expressions like `'text' * n` as invalid syntax and do not recommend replacing them with alternatives such as `List.filled(...).join()`; the operator is supported by `dart:core`.
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-05-13T13:46:32.612Z
Learnt from: dannym-arx
Repo: marmot-protocol/whitenoise PR: 656
File: lib/hooks/use_block_actions.dart:3-3
Timestamp: 2026-05-13T13:46:32.612Z
Learning: In marmot-protocol/whitenoise code reviews, do not flag imports that reference FRB-generated bindings under `package:rust_lib_whitenoise/src/rust/...` as private-internals violations. In `rust_lib_whitenoise`, Dart bindings are intentionally generated into `lib/src/rust/`, and `analysis_options.yaml` sets `implementation_imports: false` to suppress the related Dart lint—so `package:rust_lib_whitenoise/src/rust/...` imports are the correct/only way to consume that FRB-generated API.
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-05-13T13:46:39.250Z
Learnt from: dannym-arx
Repo: marmot-protocol/whitenoise PR: 656
File: lib/services/foreground_service.dart:13-16
Timestamp: 2026-05-13T13:46:39.250Z
Learning: In this repo (marmot-protocol/whitenoise), `rust_lib_whitenoise` is an FRB-generated Flutter package where the generated Dart bindings are intentionally exposed under `lib/src/rust/` as the public API surface. Do not treat imports like `package:rust_lib_whitenoise/src/rust/...` as breaking package API boundaries in code reviews (the project also sets `implementation_imports: false` in `analysis_options.yaml` to opt into this). Never raise concerns about `src/` imports from `rust_lib_whitenoise`.
Applied to files:
test/hooks/use_delete_all_data_test.dartlib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-05T11:59:09.400Z
Learnt from: erskingardner
Repo: marmot-protocol/sloth PR: 189
File: test/widgets/wn_filter_chip_test.dart:1-3
Timestamp: 2026-02-05T11:59:09.400Z
Learning: In Dart/Flutter test files, Offset is exported by package:flutter_test/flutter_test.dart. If you already import flutter_test, you do not need to import dart:ui for Offset. Ensure there are no conflicting Offset symbols when both imports occur.
Applied to files:
test/hooks/use_delete_all_data_test.dart
📚 Learning: 2026-02-19T02:12:52.425Z
Learnt from: josefinalliende
Repo: marmot-protocol/whitenoise PR: 316
File: test/hooks/use_chat_scroll_test.dart:96-114
Timestamp: 2026-02-19T02:12:52.425Z
Learning: In hook tests under test/hooks, using tester.pumpWidget directly is acceptable for simple hook logic tests that do not rely on localizations, providers, or screen utilities. If the test requires those features, use the mountWidget helper from test/test_helpers.dart (which wires in localizations, providers, and screen utilities). Apply this guideline to all files matching *_test.dart in test/hooks.
Applied to files:
test/hooks/use_delete_all_data_test.dart
📚 Learning: 2026-04-28T17:39:39.941Z
Learnt from: pepina-dev
Repo: marmot-protocol/whitenoise PR: 600
File: test/hooks/use_leave_group_test.dart:67-78
Timestamp: 2026-04-28T17:39:39.941Z
Learning: In this repo’s Dart test files under `test/` (e.g., `test/hooks/use_leave_group_test.dart`), it’s acceptable to avoid calling `mockApi.reset()` at the start of `setUp()` when the test suite already manually resets all relevant individual mock fields in `setUp()`. Reviewers should not flag the absence of a `reset()` call as long as every field that could leak state between tests is explicitly reinitialized and the tests are passing.
Applied to files:
test/hooks/use_delete_all_data_test.dart
📚 Learning: 2026-01-05T20:05:32.918Z
Learnt from: untreu2
Repo: marmot-protocol/sloth PR: 22
File: lib/screens/settings_screen.dart:76-88
Timestamp: 2026-01-05T20:05:32.918Z
Learning: In the Sloth project, tooltips should not be used for UI elements. During code reviews, verify that UI components do not include Tooltip widgets and replace them with accessible alternatives (e.g., long-press hints, labels, or interactive guidance). Apply this guidance across Dart UI files under lib (not just settings_screen.dart) to maintain a consistent design approach.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-01-09T13:25:18.531Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 33
File: lib/services/message_service.dart:14-26
Timestamp: 2026-01-09T13:25:18.531Z
Learning: When errors are presented to users in the Sloth codebase (UI layers such as snackbars, dialogs, toasts), show friendly, user-facing messages instead of raw Rust error messages. Implement a mapping from internal error types or codes to clear, non-technical messages, and surface actionable guidance for end users where appropriate. Avoid exposing internal stack traces or language runtime details; centralize common user-facing error wording in a dedicated utility or service and translate backend errors into concise, helpful UI messages.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-04-06T09:40:41.044Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 552
File: lib/screens/profile_keys_screen.dart:71-71
Timestamp: 2026-04-06T09:40:41.044Z
Learning: In this Flutter/Dart codebase, when using flutter_screenutil (e.g., ScreenUtil or ScreenUtil-based sizing like `w`, `h`), do not require scaled units for literal zero values. Specifically, in EdgeInsets (and similar sizing/padding/margin/gap APIs), bare numeric `0` should be allowed (e.g., `EdgeInsets.all(0)`, `EdgeInsets.symmetric(vertical: 0)`, `SizedBox(width: 0)`), because scaling `0` remains `0`. Only flag ScreenUtil violations when a non-zero literal needs to be expressed via `0.w`/`0.h` equivalents; do not flag bare `0`.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-01-22T20:15:04.277Z
Learnt from: untreu2
Repo: marmot-protocol/sloth PR: 78
File: lib/hooks/use_accounts.dart:21-38
Timestamp: 2026-01-22T20:15:04.277Z
Learning: In the Sloth Flutter app, for hook files under lib/hooks (e.g., use_accounts.dart), prefer logging errors using the project's logger (e.g., _logger.severe) rather than displaying SnackBars. SnackBars should not be used in hooks or for error handling; log errors centrally and surface user notifications only at UI layers. Ensure consistent use of logging severity and avoid UI side effects within hooks.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-05T20:27:05.455Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 193
File: lib/hooks/use_android_signer.dart:38-41
Timestamp: 2026-02-05T20:27:05.455Z
Learning: In Dart files under lib/hooks that use useMemoized (flutter_hooks), if a platform check like platformIsAndroid is used only to construct a service and the platform will not change at runtime, you can omit that platform check from the useMemoized dependency list. Do not include invariant platform values in the dependencies, which prevents unnecessary recomputations. Apply this pattern to all relevant files in lib/hooks where the platform is constant over the app lifecycle.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-11T17:51:41.426Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 225
File: lib/hooks/use_delete_all_data.dart:34-49
Timestamp: 2026-02-11T17:51:41.426Z
Learning: In the whitenoise Flutter app, within custom hooks under lib/hooks, do not wrap returned functions with useCallback. Define functions inline inside the hook body to maintain consistency across all hooks. This aligns with the project-wide convention for consistency. If a function must be stable across rebuilds, consider using appropriate Dart/Flutter patterns (e.g., local closures or memoization strategies) per the framework's guidance.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-11T23:40:32.726Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 225
File: lib/hooks/use_delete_all_data.dart:7-24
Timestamp: 2026-02-11T23:40:32.726Z
Learning: In the whitenoise Flutter app, internal hook state classes (e.g., DeleteAllDataState in use_delete_all_data.dart) should implement manual copyWith methods rather than using Freezed. The Freezed package is appropriate for UI states, but not for hook-internal state classes. During reviews, flag hook state implementations that rely on Freezed and prefer explicit, hand-written copyWith for reliability and lighter dependencies. Apply this pattern broadly to all Dart files under lib/hooks/*.dart and any similar hook directories.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-04-02T18:58:10.024Z
Learnt from: dannym-arx
Repo: marmot-protocol/whitenoise PR: 494
File: lib/hooks/use_scroll_to_message.dart:17-18
Timestamp: 2026-04-02T18:58:10.024Z
Learning: When reviewing Flutter/Dart hooks that use `flutter_hooks` (e.g., `useRef` / “latest-value ref” pattern), do not treat `useRef.current`/`ref.value` reads inside async loops as a stale-closure bug. If a parameter like `hasMoreMessages` is passed into the hook and immediately synced at the top of the build (`ref.value = hasMoreMessages`), then async code should read the latest `ref.value` on each iteration, reflecting the most recent rebuild triggered by state changes. The stale-closure concern should apply to captured local variables that are not updated via a `useRef`, not to `useRef`-backed fields.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-02-07T04:45:18.077Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 193
File: lib/hooks/use_nsec.dart:76-79
Timestamp: 2026-02-07T04:45:18.077Z
Learning: In Flutter, localization strings should be resolved and presented by widgets/screens, not inside hooks. Hooks (like those under lib/hooks) should return error codes (e.g., nsec_load_failed). Widgets should provide helper functions to map these codes to localized strings via AppLocalizations (e.g., _noticeMessageL10n, _signerErrorL10n). Apply this pattern across all hook files: hub on returning error codes, while UI components handle localization lookups using AppLocalizations.
Applied to files:
lib/hooks/use_delete_all_data.dart
📚 Learning: 2026-01-15T23:30:10.907Z
Learnt from: jgmontoya
Repo: marmot-protocol/sloth PR: 42
File: rust/src/api/account_groups.rs:107-123
Timestamp: 2026-01-15T23:30:10.907Z
Learning: In the Rust API layer under rust/src/api/, ensure functions remain thin translation wrappers that delegate business logic to the whitenoise-rs crate. Do not implement business logic or fixes in the sloth API layer; instead, move such changes to whitenoise-rs and keep the API layer focused on request/response translation and orchestration.
Applied to files:
rust/src/api/mod.rs
📚 Learning: 2026-01-15T23:30:10.907Z
Learnt from: jgmontoya
Repo: marmot-protocol/sloth PR: 42
File: rust/src/api/account_groups.rs:107-123
Timestamp: 2026-01-15T23:30:10.907Z
Learning: Maintain a consistent function start-up order in the Rust module: for functions like accept_account_group, decline_account_group, and mark_message_read in rust/src/api/account_groups.rs (and other files in rust/src/api), call Whitenoise::get_instance() first, then parse input parameters. This pattern should be applied across the entire module to improve readability and consistency.
Applied to files:
rust/src/api/mod.rs
📚 Learning: 2026-01-19T10:45:49.603Z
Learnt from: erskingardner
Repo: marmot-protocol/sloth PR: 52
File: rust/src/api/utils.rs:62-83
Timestamp: 2026-01-19T10:45:49.603Z
Learning: In rust/src/api, ensure that FFI glue code for flutter_rust_bridge (e.g., callable functions exposing enum variants or wrapping Display/FromStr) lives in the bridge layer, not in whitenoise-rs. Only business logic should be delegated to whitenoise-rs. Simple enum variant constructors and FFI-specific wrappers belong in the bridge layer.
Applied to files:
rust/src/api/mod.rs
📚 Learning: 2026-02-10T23:51:16.714Z
Learnt from: jgmontoya
Repo: marmot-protocol/whitenoise PR: 209
File: rust/src/api/user_search.rs:51-111
Timestamp: 2026-02-10T23:51:16.714Z
Learning: In Rust code reviews, treat usize to u64 as a safe widening on all supported platforms (usize <= 64 bits). Do not suggest try_from for usize -> u64 conversions. Be cautious about the reverse direction (u64 -> usize) on 32-bit platforms where truncation can occur. When reviewing, prefer direct as-casting or explicit type as appropriate for usize to u64; avoid try_from for this specific conversion.
Applied to files:
rust/src/api/mod.rs
mubarakcoded
requested review from
cypherpinkdev,
dannym-arx,
erskingardner,
jgmontoya and
pepina-dev
as code owners
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
This is the full feedback my agent came up with. I think it's probably better to just pass this entire thing to your agent and let them work it out:
Code Review: PR #662 — fix: reset Whitenoise lifecycle after deleting data
Summary
This PR fixes a real and impactful bug: after deleting all app data, the old Whitenoise instance (with its now-closed database) was reused by subsequent bridge calls. The solution replaces the process-global handle with a reader-writer lifecycle lock that drains in-flight calls before tearing down the instance and installs a fresh one before releasing the write permit. A secondary fix validates any cached DM group ID before returning it, falling back to creating a fresh DM instead of surfacing a "Group not found" crash.
The core design is sound. The lock hierarchy (GLOBAL_WN_INIT_LOCK → WN_LIFECYCLE_LOCK → GLOBAL_WN read/write) is well-thought-out, the WnHandle/WnSessionHandle RAII wrappers are clean, and the release_lifecycle() pattern for long-lived stream loops is clever. Most concerns below are about missing pieces or edge cases in an otherwise good implementation.
Issues
Correctness: initialize_whitenoise does not hold the lifecycle write lock during initialization
rust/src/api/mod.rs (new initialize_whitenoise)
let _init_guard = GLOBAL_WN_INIT_LOCK.lock().await;
// double-checked locking — good
let core_config = to_core_config(&config);
let whitenoise = Whitenoise::new(core_config.clone()).await...?;
*GLOBAL_WN.write()...? = Some(whitenoise);During the window between the second is_some() check and the final write(), a concurrent wn() call could acquire a lifecycle read permit, then read None from GLOBAL_WN, and return "Whitenoise not initialized". This is unlikely in practice (Flutter's init is normally serialized) but it is a correctness hole. delete_all_data correctly holds _lifecycle_guard for its entire mutation sequence. initialize_whitenoise should do the same — acquire the lifecycle write lock before Whitenoise::new(...) and hold it until GLOBAL_WN is written. Then GLOBAL_WN_INIT_LOCK can be released first, since _lifecycle_guard already serializes with readers.
Correctness: delete_all_data holds the StdRwLock write guard across an .await
rust/src/api/mod.rs lines ~1100–1108 (in delete_all_data)
*GLOBAL_WN.write()...? = None;
drop(whitenoise);
let whitenoise = Whitenoise::new(core_config).await...?; // .await while _lifecycle_guard is held
*GLOBAL_WN.write()...? = Some(whitenoise);GLOBAL_WN is a StdRwLock. The two write() calls are point-in-time (the guard is dropped immediately after each assignment via the ?/= pattern), so the lock itself is not held across the .await. However, drop(whitenoise) happens before Whitenoise::new(...), and Whitenoise::new is async. This means there is a window where GLOBAL_WN is None inside the lifecycle write lock. Any wn() calls that somehow sneak through during this window (they shouldn't, because the lifecycle write lock blocks them) will get "Whitenoise not initialized". This is safe as designed, but the comment in the docstring ("the global instance remains unset — call initialize_whitenoise to recover") only applies to the Whitenoise::new failure path. It is worth calling out that failure of drop(whitenoise) (e.g. panic in the Whitenoise destructor) would also leave GLOBAL_WN as None with no recovery path documented. Minor, but worth a comment.
Correctness: lifecycle permit is taken before checking GLOBAL_WN in wn(), creating a subtle ordering issue
rust/src/api/mod.rs (new wn())
pub(crate) async fn wn() -> Result<WnHandle, error::ApiError> {
let lifecycle_permit = Arc::clone(&WN_LIFECYCLE_LOCK).read_owned().await;
let whitenoise = GLOBAL_WN.read()...?.as_ref().map(Arc::clone).ok_or_else(...)?;The lifecycle read permit is acquired first, then GLOBAL_WN is read. If GLOBAL_WN is None (uninitialized or in the gap during reset), the function returns an error but the lifecycle_permit is still held until the error propagates and the WnHandle would have been dropped — but since WnHandle is not constructed, the permit is dropped immediately when the local variable goes out of scope at the ?. This is actually fine, but the control flow is non-obvious: a failed wn() still briefly acquires the lifecycle read lock. This won't cause a deadlock, but it does mean a concurrent delete_all_data waiting on the write lock is briefly blocked by a call that will fail anyway. The permit could be acquired after the GLOBAL_WN read to avoid this, though it would require a slightly different structure.
Missing: initialize_whitenoise uses Whitenoise::new() but the old code used Whitenoise::ensure_initialized()
rust/src/api/mod.rs (new initialize_whitenoise)
let whitenoise = Whitenoise::new(core_config.clone()).await...?;The old code called Whitenoise::ensure_initialized(core_config). The PR switches to Whitenoise::new(...) in both initialize_whitenoise and delete_all_data. If these two constructors have different semantics (e.g. ensure_initialized is idempotent at the Rust layer and handles already-existing data), this change may silently alter startup behavior. The PR description does not mention this change. It should be confirmed that Whitenoise::new() is the right call for the initial startup path and is safe to call on an already-existing data directory.
Missing: No release_lifecycle() before the receive loop in subscribe_to_notifications
rust/src/api/notifications.rs (new code)
let whitenoise = wn().await?;
let subscription = whitenoise.subscribe_to_notifications();
let mut rx = subscription.updates;
whitenoise.release_lifecycle(); // <-- diff shows this IS addedThe diff (line 1262 in the raw diff) does show release_lifecycle() is added to subscribe_to_notifications. However, the current local file (rust/src/api/notifications.rs) does not have this yet — it's the pre-PR version. Double-check this is actually present in the PR branch; the stream_receive_loops_release_lifecycle_before_waiting test should catch it if missing.
Dart: Removed timeout has no UX fallback
lib/hooks/use_delete_all_data.dart
- await api.deleteAllData().timeout(timeout);
+ await api.deleteAllData();The timeout was 10 seconds. Without it, if deleteAllData() hangs (e.g. the Rust side deadlocks, a background lock is never released), the Flutter UI will spin in isDeleting: true forever with no escape hatch. The Rust-side fix prevents the known hang, but removing the Flutter-side timeout eliminates all defensive recovery. Consider whether the UI has a way to force-quit or whether this is an acceptable risk given the correctness fix in Rust.
Suggestions
WnSessionHandle holds two indirections unnecessarily
rust/src/api/mod.rs
pub(crate) struct WnSessionHandle {
session: Arc<whitenoise::whitenoise::session::AccountSession>,
_wn: WnHandle, // WnHandle itself wraps Arc<Whitenoise> + OwnedRwLockReadGuard
}WnHandle already contains the lifecycle permit. WnSessionHandle re-wraps it. This is intentional for the ownership story, but WnSessionHandle has no release_lifecycle() of its own. If a session-based stream (not a whitenoise-based one) ever needed to enter a long-running receive loop, there would be no way to release the lifecycle permit without dropping the whole handle. Consider adding a release_lifecycle() to WnSessionHandle as well, or documenting that session streams must restructure to use wn() directly.
The release_lifecycle() method is a silent no-op with only a comment explaining it
rust/src/api/mod.rs
pub(crate) fn release_lifecycle(self) {}This is a legitimate use of consuming self to force a drop, but it reads as a mistake to anyone unfamiliar with the pattern. The existing comment is good but lives in the block above. Adding a brief doc comment directly on the method would make it self-documenting and prevent a future editor from "fixing" it by removing the seemingly-empty body.
GLOBAL_WN_INIT_LOCK could be a Mutex<()> held within a Once or just a Mutex<()> — consider naming
The static is named GLOBAL_WN_INIT_LOCK but it now also guards the reset path in delete_all_data. "INIT" in the name is slightly misleading. Something like GLOBAL_WN_WRITE_LOCK or WN_REPLACE_LOCK would more accurately convey that it serializes any replacement of GLOBAL_WN, not just initialization.
to_core_config is private but the old From<WhitenoiseConfig> impl still exists and is not used by the new paths
rust/src/api/mod.rs
The From<whitenoise::WhitenoiseConfig> for WhitenoiseConfig impl converts in the wrong direction (from Rust type → Flutter type) and is still present. The new to_core_config goes the other direction (Flutter → Rust). Both coexist, but only to_core_config is used in the new code. This is fine — the From impl may be used by the bridge elsewhere — but confirm it is still needed.
Dart test: unawaited import not shown
test/hooks/use_delete_all_data_test.dart
unawaited(resultFuture.then((_) => completed = true));unawaited requires import 'package:flutter/foundation.dart' or dart:async. The diff does not show an import being added. If this import was already present, no change is needed; if not, this will be a compile error. Verify the test file compiles.
What's Done Well
- The RAII
WnHandle/WnSessionHandleapproach is idiomatic Rust and makes the lifecycle semantics impossible to forget — you can't use the session without holding the permit, and you can't forget to release it for stream loops because the type system forces you to drop the handle. - The
stream_receive_loops_release_lifecycle_before_waitingtest is an excellent belt-and-suspenders enforcement mechanism. Scanning source files for pattern compliance at test time is a creative and effective way to prevent regressions. - The stale-DM guard in
use_start_dm.dartis minimal and targeted — it doesn't add caching or retry logic, just falls through to the existing create path. - Test coverage for both changed Dart hooks is thorough: the new behavior (wait instead of timeout) and the stale-group fallback are both tested with appropriate assertions.
- The
core_configcapture indelete_all_data(cloned before deletion, passed toWhitenoise::newafter) is a clean way to avoid re-taking config from a widget/Flutter layer during the reset.
pepina-dev
left a comment
pepina-dev
left a comment
There was a problem hiding this comment.
I don't understand the fix on start a DM. By looking a the code, that existing id wasn't cached but requested to the rust crate and if not found should just be null. If it raises an error from rust crate, it is an unexpected error, I rather show that there was an error than create a new group.
I personally think that is better to leave the use start dm hook as it was, unless I'm miss understing it and it is really fixing a real bug. My agent raised a comment here too.
🤖 ### 🔴 MUST —
useStartDmswallows allgetGrouperrors → duplicate DM groupsWhere:
lib/hooks/use_start_dm.dart:29-45.if (existingGroupId != null) { try { await groups_api.getGroup(accountPubkey: accountPubkey, groupId: existingGroupId); return existingGroupId; } catch (e) { _logger.warning('Ignoring stale DM group $existingGroupId for peer $peerPubkey: $e'); } } // falls through and CREATES a new DMAny exception —
ApiError_Whitenoise { message: "MLS group not found" }, but alsoNetwork,Lock contention,Whitenoise not initialized(relevant given the lifecycle changes in this same branch!), generic FFI panics, etc. — falls through intocreateGroup. Result: when a user starts a DM and the FFI is briefly unavailable (or any transient failure occurs), they get a second DM group with the same peer. Both will show up in the chat list with the same name/avatar and will diverge.The CHANGELOG mentions "ignoring stale DM group ids" as the motivation, but the current implementation is far broader than that.
Related to the delete app data🎨 More like UX question for @vladimir-krstic . By testing this PR I realized that delete app data logs me out of all accounts, not only from the account that I chose to delete data. This is how it works in master too, but I had forgotten about this feature and now that I tried it again, the behavior felt unexpected to me. I thought this was going to delete only the data for this account. I feel it needs a bigger warning, cause other actions in settings are scoped to affect just a single account. Edit: this is not a blocker for this PR BTW, I left the comment in Figma too and we can continue the discussion there. The only blocker for me is the start dm fix that I don't quite understand and I'm not convinced that it really fixes things |
|
@pepina-dev you are 100% right, I'm not sure does it even belong here. Adding it on my to-do list. |
You’re right, thanks for catching this. I was trying to guard against a stale DM id after reset, but the hook was catching every getGroup error, which could hide real Rust/FFI failures and accidentally create duplicate DMs. I'll removed that fallback and restore the previous useStartDm behavior in this PR. If stale DM ids are still a real issue, I think we should handle that closer to the Rust/core lookup path, where we can distinguish “not found” from unexpected errors. |
Agreed, I was surprised by this too. I checked out master and tested again just to confirm I hadn't changed the behavior in this PR. It does feel unexpected from the account settings context, since that screen mostly feels account-scoped. I'll keep this PR focused on the lifecycle/reset fix for now, and I'm happy to follow the Figma/design discussion for a separate UX improvement. |
…-reset-lifecycle # Conflicts: # lib/src/rust/api.dart # lib/src/rust/frb_generated.dart # rust/src/api/mod.rs
…-reset-lifecycle # Conflicts: # lib/src/rust/frb_generated.dart # lib/src/rust/frb_generated.io.dart # rust/Cargo.lock # rust/Cargo.toml # rust/src/frb_generated.rs
4 participants
Description
Fixes the reset lifecycle after deleting all app data.
The bridge now replaces the process-global Whitenoise handle safely: it waits for in-flight bridge calls before closing the old database, clears the global instance after delete, and exposes
reinitializeWhitenoise()so Flutter can install a fresh instance after the local wipe completes.Flutter wraps the delete flow with a reset-pending marker. If the app is killed or interrupted during reset, startup recovery clears the local reset surface before initializing Whitenoise again. If deletion succeeds but reinitialization fails in the same process, the app routes to the fatal error flow instead of leaving the user on settings with an uninitialized Rust singleton.
This PR also keeps the start-DM stale-group guard: cached DM group IDs are validated before opening. If the cached group no longer exists in Rust/MDK state, the app falls back to creating a fresh DM instead of failing with
Group not found.This app-side lifecycle fix depends on the companion
whitenoise-rsbounded-delete PR, which prevents local data deletion from hanging behind stuck scheduler/background/relay shutdown work:marmot-protocol/whitenoise-rs#840
Closes #660
Type of Change
How I Tested This
cargo test --lockedjust precommitpasses.Group not foundpath.Screenshots
Not applicable; no UI layout changes.
Checklist
just precommitpassesCloses #660)CHANGELOG.mdupdated (if user-visible change)Summary by CodeRabbit
Improvements
Bug Fixes
Behavior Changes
Tests