fix(ios): stop 0xdead10cc account-creation crash and fix lost-login error message#701
Open
vincenzopalazzo wants to merge 2 commits into
Open
Conversation
The encrypted SQLite database lives in the shared App Group container so the
notification service extension can read it. Its files defaulted to
NSFileProtectionComplete, which becomes unavailable while the device is locked.
Holding a SQLite/WAL lock on such a file in a shared container across suspension
makes iOS terminate the process with 0xdead10cc ("deadlock") via RUNNINGBOARD,
surfaced to the user as "White Noise Crashed" during account creation/login.
Apply NSFileProtectionCompleteUntilFirstUserAuthentication to the data and logs
directories (and, once, to files that predate this fix) in both the app and the
NSE before the database is opened. New files inherit the directory's class, so
the recursive downgrade is gated by a marker to avoid walking the media cache on
every launch / push.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vincenzopalazzo
requested review from
cypherpinkdev,
dannym-arx,
erskingardner,
jgmontoya,
mubarakcoded and
pepina-dev
as code owners
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (6)lib/**/*.dart📄 CodeRabbit inference engine (AGENTS.md)
Files:
lib/hooks/use_*.dart📄 CodeRabbit inference engine (AGENTS.md)
Files:
lib/hooks/**⚙️ CodeRabbit configuration file
Files:
test/**/*_test.dart📄 CodeRabbit inference engine (AGENTS.md)
Files:
test/**⚙️ CodeRabbit configuration file
Files:
lib/screens/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (33)📚 Learning: 2026-01-05T20:05:32.918ZApplied to files:
📚 Learning: 2026-01-09T13:25:18.531ZApplied to files:
📚 Learning: 2026-04-06T09:40:41.044ZApplied to files:
📚 Learning: 2026-01-06T01:08:41.552ZApplied to files:
📚 Learning: 2026-01-15T14:42:54.111ZApplied to files:
📚 Learning: 2026-01-16T17:35:32.431ZApplied to files:
📚 Learning: 2026-01-19T15:50:56.684ZApplied to files:
📚 Learning: 2026-01-29T03:02:34.290ZApplied to files:
📚 Learning: 2026-01-29T16:02:52.588ZApplied to files:
📚 Learning: 2026-02-07T03:58:22.587ZApplied to files:
📚 Learning: 2026-02-11T17:29:43.985ZApplied to files:
📚 Learning: 2026-02-18T18:36:13.394ZApplied to files:
📚 Learning: 2026-04-06T09:36:06.726ZApplied to files:
📚 Learning: 2026-05-13T13:46:32.612ZApplied to files:
📚 Learning: 2026-05-13T13:46:39.250ZApplied to files:
📚 Learning: 2026-01-22T20:15:04.277ZApplied to files:
📚 Learning: 2026-02-05T20:27:05.455ZApplied to files:
📚 Learning: 2026-02-11T17:51:41.426ZApplied to files:
📚 Learning: 2026-02-11T23:40:32.726ZApplied to files:
📚 Learning: 2026-04-02T18:58:10.024ZApplied to files:
📚 Learning: 2026-02-07T04:45:18.077ZApplied to files:
📚 Learning: 2025-12-23T14:37:11.241ZApplied to files:
📚 Learning: 2026-01-05T21:28:05.652ZApplied to files:
📚 Learning: 2026-01-07T16:49:18.694ZApplied to files:
📚 Learning: 2026-01-11T22:43:09.610ZApplied to files:
📚 Learning: 2026-04-14T03:52:37.716ZApplied to files:
📚 Learning: 2026-02-05T11:59:09.400ZApplied to files:
📚 Learning: 2026-04-28T17:39:39.941ZApplied to files:
📚 Learning: 2026-01-15T15:00:17.356ZApplied to files:
📚 Learning: 2026-02-10T04:57:31.475ZApplied to files:
📚 Learning: 2026-02-19T10:33:16.889ZApplied to files:
📚 Learning: 2026-04-06T09:40:51.965ZApplied to files:
📚 Learning: 2026-02-19T02:12:52.425ZApplied to files:
🔇 Additional comments (4)
📝 WalkthroughWalkthroughTwo independent features are added: (1) iOS file data-protection — a ChangesiOS File Data Protection
LoginNoLoginInProgress Error Handling
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over initializeAppContainer,WhiteNoiseDataProtection: iOS startup data-protection flow
participant initializeAppContainer
participant applyIosDataProtection
participant AppGroupChannel
participant AppDelegate
participant WhiteNoiseDataProtection
initializeAppContainer->>applyIosDataProtection: paths=[dataDir, logsDir]
applyIosDataProtection->>AppGroupChannel: invokeMethod("applyDataProtection", {paths})
AppGroupChannel->>AppDelegate: applyDataProtection(arguments)
AppDelegate->>WhiteNoiseDataProtection: apply(atPath:) per path
WhiteNoiseDataProtection-->>AppDelegate: Bool (success/failure)
AppDelegate-->>AppGroupChannel: Bool result
AppGroupChannel-->>applyIosDataProtection: result or PlatformException
applyIosDataProtection-->>initializeAppContainer: completes (exception swallowed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you:
Chapters generated by Stage for commit 2ee9e10 on Jun 15, 2026 12:06pm UTC. |
vincenzopalazzo
commented
Jun 15, 2026
vincenzopalazzo
left a comment
vincenzopalazzo
left a comment
Author
There was a problem hiding this comment.
Self-review: one issue to fix before merge, plus one non-blocking note. The inline specifics are below.
Code Review
Positive aspects
- Root cause is backed by real device crash reports (
0xdead10ccRUNNINGBOARD SIGKILL on bothRunnerand the NSE), not a guess. - Setting the protection class on the directory and gating the recursive downgrade behind a marker (
.wn_data_protection_v1) is the right call — new-wal/-shm/media files inherit the class, so neither startup nor the time-boxed NSE walks the media cache on every run. - The error-map fix reuses existing l10n keys (no new translations), and traces cleanly to the single reachable cause (
LoginError::NoLoginInProgress).
Issues & suggestions
1. Drop the unreachable LoginInvalidKeyFormat arm on the relay screen (Medium)
lib/hooks/use_relay_resolution.dart:45 and lib/screens/relay_resolution_screen.dart add a LoginInvalidKeyFormat -> loginErrorInvalidKey mapping. That error can't reach this screen: the key is validated in login_start, and the relay-publish path (login_publish_default_relays / login_with_custom_relay) never re-parses it. So the arm is dead, and it's untested — the repo's 99% coverage gate (just coverage) will fail on the new branch. Keep only LoginNoLoginInProgress (the actual bug) and drop LoginInvalidKeyFormat from both the hook map and _resolveError.
2. Concurrent one-time recursion in app + NSE (Cleanup, non-blocking)
If a push fires while the app is launching, both processes can run the one-time recursive downgrade before either writes the marker. It's idempotent (setAttributes to the same class), so harmless — just noting it's a known, benign race, not a correctness issue.
Summary
LGTM after dropping the unreachable LoginInvalidKeyFormat arm (item 1) so coverage stays green. Item 2 is non-blocking.
Decision Log
Hardest decision: Whether to fix 0xdead10cc at the file-protection layer or by closing DB connections on background. File protection (completeUntilFirstUserAuthentication) is the minimal, well-trodden fix and also satisfies the NSE's need to read the DB while the device is locked; forcing connection teardown would fight the core's persistent sqlx pool in whitenoise-rs, which this app layer doesn't own.
Alternatives rejected:
- Recurse the data dir on every launch/push: simpler, but unbounded main-thread/NSE work over the media cache — rejected for the marker-gated one-time pass.
- Persist
pending_loginsso login survives a restart: the right deep fix, but it lives inwhitenoise-rs, out of scope for this app PR. This PR makes the symptom accurate and removes the crash that wipes the session. - Mirror the login screen's full error map verbatim: led to the dead
InvalidKeyFormatarm in item 1 — scope-trimmed to what's reachable.
Least confident about: The runtime behavior of the protection change can't be verified here — data protection is a no-op on the Simulator and a device reinstall wipes the App Group DB. I'm relying on the documented 0xdead10cc semantics and the crash-log evidence; on-device confirmation via xcrun devicectl device install over the existing install is the real test.
… screen The Relay Setup screen mapped only LoginNoRelayConnections/Timeout/Internal, so LoginNoLoginInProgress fell through to the generic "An error occurred during login. Please try again." The pending login is in-memory in the core, so once it is gone (e.g. after the app was killed) retrying on that screen can never succeed -- the user must restart from key entry, which the message did not say. Map LoginNoLoginInProgress on the relay screen, reusing the existing l10n string already shown on the login screen: "No login in progress. Please start over." Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vincenzopalazzo
force-pushed
the
fix/ios-account-creation-crash
branch
from
f0332c4 to
2ee9e10
Compare
Author
|
Addressed the self-review. Item 1 (Medium) — fixed in 2ee9e10. Dropped the unreachable Item 2 (Cleanup) — non-blocking, left as-is. The app/NSE one-time recursion race is idempotent ( Local checks on the new head: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@ios/Runner/AppDelegate.swift`:
- Around line 284-314: In the apply(atPath:) method of the
WhiteNoiseDataProtection enum, the marker file is currently created only when
all setAttributes calls succeed. To ensure the recursive directory walk is
bounded to a single launch and not repeated on subsequent launches when
transient failures occur, modify the logic so that the marker file is created
after the enumeration loop regardless of the success variable's final value, or
create it unconditionally before returning. This way, even if some files fail to
have their attributes set, the migration attempt is marked as completed and the
expensive recursive walk will not be repeated on the next launch.
In `@ios/WhiteNoiseNotificationServiceExtension/NotificationService.swift`:
- Around line 274-328: The WhiteNoiseDataProtection enum is duplicated verbatim
between this NotificationService.swift file and AppDelegate.swift in the main
app target. To fix this, either extract the enum into a shared Swift module or
framework that both the main app and notification service extension can depend
on, OR if that is not feasible, add a clear comment in both the current location
and in AppDelegate.swift that notes this duplication exists and that any future
changes to one must be synchronized with the other. The goal is to prevent the
two implementations from drifting out of sync over time.
In `@lib/hooks/use_relay_resolution.dart`:
- Line 45: The new error mapping for ApiError_LoginInvalidKeyFormat() to
'loginErrorInvalidKey' in the useRelayResolution hook lacks test coverage. Add a
hook-level test that verifies ApiError_LoginInvalidKeyFormat() correctly maps to
the 'loginErrorInvalidKey' error message, and include a matching screen
assertion to validate the complete error handling path. This will ensure the new
mapping is protected from regressions and aligns with the test coverage
requirements for this PR.
🪄 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: 3a072930-432c-4423-b704-a4f1575a77e1
📒 Files selected for processing (8)
ios/Runner/AppDelegate.swiftios/WhiteNoiseNotificationServiceExtension/NotificationService.swiftlib/hooks/use_relay_resolution.dartlib/main.dartlib/screens/relay_resolution_screen.darttest/hooks/use_relay_resolution_test.darttest/main_test.darttest/screens/relay_resolution_screen_test.dart
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
lib/**/*.dart
📄 CodeRabbit inference engine (AGENTS.md)
lib/**/*.dart: Use single quotes for strings in Dart/Flutter
Enable and followprefer_const_constructorslinting rule in Dart
Enable and followprefer_final_localslinting rule in Dart
Maintain line width of 100 characters in Dart/Flutter code
Preserve trailing commas in Dart/Flutter code
Do not add comments except for code that is really complex or hard to understand
Useflutter_screenutilfor all size values to ensure responsive layouts: use.wfor width,.hfor height,.spfor font size/letter spacing,.rfor radius
Avoid StatefulWidget; prefer providers (shared app-wide state) or hooks (widget-local state) instead
Avoid using// coverage:ignore,// coverage:ignore-line,// coverage:ignore-start, or// coverage:ignore-endto bypass coverage requirements; write tests instead
Files:
lib/hooks/use_relay_resolution.dartlib/main.dartlib/screens/relay_resolution_screen.dart
lib/hooks/use_*.dart
📄 CodeRabbit inference engine (AGENTS.md)
Hook files should be prefixed with
use_(e.g.,use_chat_list.dart) and hook functions should start withuse(e.g.,useChatList())
Files:
lib/hooks/use_relay_resolution.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_relay_resolution.dart
test/**/*_test.dart
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*_test.dart: Test files must mirror source structure with_test.dartsuffix and maintain minimum 99% code coverage
UsesetUpTestView(tester)test helper to configure test view dimensions
UsemountTestApp(tester, overrides)test helper to mount full app with provider overrides
UsemountHook(tester, useHook)test helper to test individual hooks
UsemountWidget(child, tester)test helper to mount single widget
UsemountStackedWidget(child, tester)test helper to mount widget in Stack
Mock Rust API usingRustLib.initMock(api: mockApi)in tests
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 (seetest_helpers.dartfor examples), not dummy values like'abc'or'test-pubkey'
Files:
test/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_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/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dart
lib/screens/**
⚙️ CodeRabbit configuration file
lib/screens/**: This is a Flutter screen (full-page widget).
Architecture rules:
- Screens should WATCH Riverpod providers for shared state
- Use flutter_hooks for ephemeral/local state (NOT StatefulWidget)
- Pass data to hooks, not refs
- Use flutter_screenutil (.w, .h, .sp, .r) for all size values
- Widgets should use const constructors where possible
- No comments except for truly complex logic
- When a widget is extracted from a screen and only used in that one
screen, it should be prefixed with the screen name (e.g.
ChatListTile for a widget only used in the chat list screen).
These are screen-scoped widgets and do NOT use the Wn prefix.
Files:
lib/screens/relay_resolution_screen.dart
🧠 Learnings (33)
📚 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_relay_resolution.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.darttest/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.dartlib/main.dartlib/screens/relay_resolution_screen.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_relay_resolution.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_relay_resolution.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_relay_resolution.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_relay_resolution.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_relay_resolution.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_relay_resolution.dart
📚 Learning: 2025-12-23T14:37:11.241Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 9
File: test/screens/error_screen_test.dart:60-71
Timestamp: 2025-12-23T14:37:11.241Z
Learning: In tests for screen widgets that are not wired through the app's Routes.build (e.g., ErrorScreen under lib/screens/error_screen.dart), avoid mounting the full app with mountTestApp. instead, configure a minimal test harness with a custom router or navigator setup (e.g., a MaterialApp with a mock router) to render the screen in isolation. This ensures the test focuses on the screen's behavior without depending on the app's global routing configuration.
Applied to files:
test/screens/relay_resolution_screen_test.dart
📚 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/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_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/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_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/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_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/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_test.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/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_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/screens/relay_resolution_screen_test.darttest/hooks/use_relay_resolution_test.darttest/main_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_relay_resolution_test.dart
📚 Learning: 2026-01-15T15:00:17.356Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 39
File: lib/screens/user_search_screen.dart:50-75
Timestamp: 2026-01-15T15:00:17.356Z
Learning: In lib/screens/user_search_screen.dart and similar screen widgets in the sloth repository, prefer using inline ternary expressions for conditional rendering (e.g., isLoading → empty state → list) over extracting into separate widget classes when the conditional UI is simple. This improves readability by keeping the render logic in a single place. Reserve extracting to a separate widget only when the conditional UI becomes sufficiently complex or is reused across multiple screens.
Applied to files:
lib/screens/relay_resolution_screen.dart
📚 Learning: 2026-02-10T04:57:31.475Z
Learnt from: josefinalliende
Repo: marmot-protocol/sloth PR: 208
File: lib/screens/signup_screen.dart:66-70
Timestamp: 2026-02-10T04:57:31.475Z
Learning: In Dart/Flutter using flutter_hooks, a ValueNotifier (from useState) is disposed when the widget unmounts. Async callbacks (e.g., .then() on animations) may still run after disposal and throw 'A ValueNotifier was used after being disposed'. Reviewers should ensure that state updates in async callbacks are guarded by a mounted check (e.g., if (mounted) { notifier.value = ... } or cancel/avoid updating after disposal). This guideline applies to screen widgets under lib/screens and similar Dart files using hooks that manage widget lifecycle. Update code patterns to prevent post-disposal state updates, and document the rationale in comments where necessary.
Applied to files:
lib/screens/relay_resolution_screen.dart
📚 Learning: 2026-02-19T10:33:16.889Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 315
File: lib/screens/group_info_screen.dart:146-149
Timestamp: 2026-02-19T10:33:16.889Z
Learning: Pubkey validation (length/format) is performed by the Rust crate before data reaches Dart. Do not add defensive substring guards in Dart for pubkeys in lib/screens/**/*.dart or other Flutter code consuming group member pubkeys, as this would duplicate validation and be unnecessary. If any validation is needed in Dart, keep it to lightweight sanity checks only after data enters Dart and ensure it aligns with the Rust validation rules.
Applied to files:
lib/screens/relay_resolution_screen.dart
📚 Learning: 2026-04-06T09:40:51.965Z
Learnt from: untreu2
Repo: marmot-protocol/whitenoise PR: 552
File: lib/screens/sign_out_screen.dart:86-86
Timestamp: 2026-04-06T09:40:51.965Z
Learning: When reviewing Flutter/Dart code that uses ScreenUtil (e.g., ScreenUtil `.h`/`.w`/`.sp`), do not require converting a literal `0` used for padding/margin/sizing to a ScreenUtil expression. In sizing contexts like `EdgeInsets` (or other `EdgeInsets`/layout parameters), raw `0` is mathematically invariant (`0 * scale = 0`) and should not be flagged as a ScreenUtil violation. Only non-zero literal size values should be checked for ScreenUtil usage.
Applied to files:
lib/screens/relay_resolution_screen.dart
🔇 Additional comments (10)
lib/screens/relay_resolution_screen.dart (1)
21-24: LGTM!test/hooks/use_relay_resolution_test.dart (1)
1093-1125: LGTM!test/screens/relay_resolution_screen_test.dart (1)
453-463: LGTM!ios/Runner/AppDelegate.swift (2)
95-96: LGTM!Also applies to: 194-202
316-328: LGTM!ios/WhiteNoiseNotificationServiceExtension/NotificationService.swift (1)
182-183: LGTM!lib/main.dart (2)
116-116: LGTM!
134-157: LGTM!test/main_test.dart (2)
12-12: LGTM!
936-1000: LGTM!
Comment on lines
+284
to
+314
| enum WhiteNoiseDataProtection { | ||
| private static let markerName = ".wn_data_protection_v1" | ||
|
|
||
| @discardableResult | ||
| static func apply(atPath path: String) -> Bool { | ||
| let fileManager = FileManager.default | ||
| let attributes: [FileAttributeKey: Any] = [ | ||
| .protectionKey: FileProtectionType.completeUntilFirstUserAuthentication | ||
| ] | ||
| // Always set the class on the directory itself; new files (db, -wal, -shm, | ||
| // media cache) inherit it, so this is the only step needed on most launches. | ||
| var success = setAttributes(attributes, atPath: path, using: fileManager) | ||
|
|
||
| // Downgrade files that predate this fix exactly once. Recursing the whole | ||
| // data dir (which holds the media cache) on every launch would be unbounded | ||
| // work, so a marker gates it after the first successful pass. | ||
| let markerPath = (path as NSString).appendingPathComponent(markerName) | ||
| guard !fileManager.fileExists(atPath: markerPath) else { | ||
| return success | ||
| } | ||
| if let enumerator = fileManager.enumerator(atPath: path) { | ||
| for case let relativePath as String in enumerator { | ||
| let itemPath = (path as NSString).appendingPathComponent(relativePath) | ||
| success = setAttributes(attributes, atPath: itemPath, using: fileManager) && success | ||
| } | ||
| } | ||
| if success { | ||
| fileManager.createFile(atPath: markerPath, contents: nil, attributes: attributes) | ||
| } | ||
| return success | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider writing the marker file even on partial success.
If setAttributes fails for some files (e.g., a locked file) but succeeds for others, the current logic skips creating the marker. On the next launch the code re-enumerates and re-attempts the entire directory. This is resilient for transient failures but means every launch incurs the full recursive walk until 100% success.
Given that the PR describes this as a one-time migration for files that "predate this fix" and the NSE operates under tight time/memory constraints, consider creating the marker unconditionally (or on first invocation regardless of outcome) so the recursive pass is truly bounded to once. Persistent failures would still be logged.
🤖 Prompt for 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.
In `@ios/Runner/AppDelegate.swift` around lines 284 - 314, In the apply(atPath:)
method of the WhiteNoiseDataProtection enum, the marker file is currently
created only when all setAttributes calls succeed. To ensure the recursive
directory walk is bounded to a single launch and not repeated on subsequent
launches when transient failures occur, modify the logic so that the marker file
is created after the enumeration loop regardless of the success variable's final
value, or create it unconditionally before returning. This way, even if some
files fail to have their attributes set, the migration attempt is marked as
completed and the expensive recursive walk will not be repeated on the next
launch.
Comment on lines
+274
to
+328
| /// Applies `completeUntilFirstUserAuthentication` data protection to a directory | ||
| /// and everything inside it. | ||
| /// | ||
| /// The White Noise database lives in the shared App Group container that this | ||
| /// extension reads. Files there default to a protection class that is | ||
| /// unavailable while the device is locked; holding a SQLite/WAL lock on such a | ||
| /// file in a shared container across suspension makes iOS terminate the process | ||
| /// with 0xdead10cc. Setting the relaxed class keeps the files reachable after | ||
| /// first unlock and avoids that termination. | ||
| enum WhiteNoiseDataProtection { | ||
| private static let markerName = ".wn_data_protection_v1" | ||
|
|
||
| @discardableResult | ||
| static func apply(atPath path: String) -> Bool { | ||
| let fileManager = FileManager.default | ||
| let attributes: [FileAttributeKey: Any] = [ | ||
| .protectionKey: FileProtectionType.completeUntilFirstUserAuthentication | ||
| ] | ||
| // Always set the class on the directory itself; new files (db, -wal, -shm, | ||
| // media cache) inherit it, so this is the only step needed on most launches. | ||
| var success = setAttributes(attributes, atPath: path, using: fileManager) | ||
|
|
||
| // Downgrade files that predate this fix exactly once. The extension runs on | ||
| // a tight time/memory budget, so recursing the whole data dir on every push | ||
| // is unacceptable; a marker gates it after the first successful pass. | ||
| let markerPath = (path as NSString).appendingPathComponent(markerName) | ||
| guard !fileManager.fileExists(atPath: markerPath) else { | ||
| return success | ||
| } | ||
| if let enumerator = fileManager.enumerator(atPath: path) { | ||
| for case let relativePath as String in enumerator { | ||
| let itemPath = (path as NSString).appendingPathComponent(relativePath) | ||
| success = setAttributes(attributes, atPath: itemPath, using: fileManager) && success | ||
| } | ||
| } | ||
| if success { | ||
| fileManager.createFile(atPath: markerPath, contents: nil, attributes: attributes) | ||
| } | ||
| return success | ||
| } | ||
|
|
||
| private static func setAttributes( | ||
| _ attributes: [FileAttributeKey: Any], | ||
| atPath path: String, | ||
| using fileManager: FileManager | ||
| ) -> Bool { | ||
| do { | ||
| try fileManager.setAttributes(attributes, ofItemAtPath: path) | ||
| return true | ||
| } catch { | ||
| NSLog("White Noise NSE failed to set data protection on %@: %@", path, error.localizedDescription) | ||
| return false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Duplicated WhiteNoiseDataProtection across targets.
This enum is copied verbatim from AppDelegate.swift. While iOS app extension architecture often necessitates such duplication (the NSE and main app are separate targets), consider extracting this into a shared Swift module or framework to avoid drift and ease future maintenance.
If extraction is not feasible, add a comment in both files noting the duplication so future changes are synchronized.
🤖 Prompt for 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.
In `@ios/WhiteNoiseNotificationServiceExtension/NotificationService.swift` around
lines 274 - 328, The WhiteNoiseDataProtection enum is duplicated verbatim
between this NotificationService.swift file and AppDelegate.swift in the main
app target. To fix this, either extract the enum into a shared Swift module or
framework that both the main app and notification service extension can depend
on, OR if that is not feasible, add a clear comment in both the current location
and in AppDelegate.swift that notes this duplication exists and that any future
changes to one must be synchronized with the other. The goal is to prevent the
two implementations from drifting out of sync over time.
|
|
||
| String _relayResolutionErrorMessage(Object error) { | ||
| return switch (error) { | ||
| ApiError_LoginInvalidKeyFormat() => 'loginErrorInvalidKey', |
There was a problem hiding this comment.
Add coverage for the new invalid-key error mapping.
ApiError_LoginInvalidKeyFormat() is newly mapped here, but the updated tests in this PR only assert
loginErrorNoLoginInProgress. Please add a hook-level mapping test (and matching screen assertion) for
loginErrorInvalidKey to protect this new path from regressions.
As per coding guidelines, test changes should protect behavior and CI enforces non-regressing coverage.
🤖 Prompt for 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.
In `@lib/hooks/use_relay_resolution.dart` at line 45, The new error mapping for
ApiError_LoginInvalidKeyFormat() to 'loginErrorInvalidKey' in the
useRelayResolution hook lacks test coverage. Add a hook-level test that verifies
ApiError_LoginInvalidKeyFormat() correctly maps to the 'loginErrorInvalidKey'
error message, and include a matching screen assertion to validate the complete
error handling path. This will ensure the new mapping is protected from
regressions and aligns with the test coverage requirements for this PR.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two linked iOS fixes for the "can't create the account / White Noise Crashed" report.
0xdead10ccSIGKILL that crashes the app (and the NSE) during account creation/login.Why this change?
The crash (
0xdead10cc)Pulled the real crash reports off-device (
idevicecrashreport, no reinstall). Both processes were terminated by RUNNINGBOARD:0xdead10cc("deadlock") is iOS killing a process that holds a SQLite/file lock on a file in a shared container across suspension. The chain here:group…/whitenoise/data) so the NSE can read it.-wal/-shmlocks.NSFileProtectionComplete, which is unavailable while the device is locked.0xdead10cc.Fix: apply
NSFileProtectionCompleteUntilFirstUserAuthenticationto thedata/logsdirs (and, once, to files that predate the fix) in bothRunnerand the NSE, before the DB is opened. New files inherit the directory's class, so the recursive downgrade is gated by a marker (.wn_data_protection_v1) to avoid walking the media cache on every launch / every push. This also lets the NSE read the DB while the device is locked (which it must, to render push content).The login banner (image with "An error occurred during login")
This is the same incident's second symptom. The pending login is an in-memory
DashMap(pending_logins,AccountManager); it is wiped when the process is killed (the crash above) or the core reinitializes. After that, the relay-publish step returnsLoginError::NoLoginInProgress.The Relay Setup screen's error map (
use_relay_resolution.dart) only handledNoRelayConnections/Timeout/Internal, soNoLoginInProgressfell through to the generic retryable message — but retrying there can never succeed; the user must restart from key entry. Tracing the core confirmsNoLoginInProgressis the only path to the generic message at that screen (keychain and relay failures all map to specific, already-handled errors).Fix: mirror the login screen's map on the relay screen (
NoLoginInProgress,InvalidKeyFormat) using existing l10n strings — no new translation keys.NoLoginInProgressnow renders "No login in progress. Please start over."Test plan
flutter analyze— clean.flutter test— full suite green (4736 tests), including new cases:main_test.dart:applyIosDataProtectioninvokes the channel on iOS, is a no-op elsewhere, swallows platform exceptions.use_relay_resolution_test.dart+relay_resolution_screen_test.dart:NoLoginInProgressmaps to the "start over" message.Verification note
The data-protection change can't be fully verified at runtime in this environment: file data protection is a no-op on the Simulator, and reinstalling on a device wipes the App Group DB. Recommend on-device verification via
xcrun devicectl device installover the existing install (do notflutter install, which may uninstall first). The root robustness of the in-memorypending_loginslives inwhitenoise-rs, out of scope for this app-layer PR; this PR makes the symptom accurate and removes the crash that wipes the session.Summary by CodeRabbit
Release Notes
dataandlogs, including a one-time safeguard for existing files.