Perf fixes #633
Perf fixes #633
Conversation
WalkthroughThis PR normalizes pubkey comparisons to hex, makes group member/admin flows cache-first and domain_user-based, turns WnImage stateful and adds FallbackAvatar, restructures multiple settings and network UI components (relay sections, pull-to-refresh), updates tests, and bumps a Rust crate revision. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI
participant GroupsNotifier
participant FollowsProvider
participant API as Metadata API
note over UI,GroupsNotifier: Load group members/admins
UI->>GroupsNotifier: loadGroupMembers(groupId)
loop for each member pubkey
GroupsNotifier->>FollowsProvider: findFollowByPubkey(pubkey)
alt Found in cache
FollowsProvider-->>GroupsNotifier: Follow metadata
GroupsNotifier->>GroupsNotifier: domain_user.User.fromMetadata(...)
else Not found
GroupsNotifier->>API: fetchMetadata(pubkey)
alt Success
API-->>GroupsNotifier: metadata
GroupsNotifier->>GroupsNotifier: domain_user.User.fromMetadata(...)
else Failure
GroupsNotifier->>GroupsNotifier: create minimal domain_user.User(pubkey)
end
end
end
GroupsNotifier-->>UI: List<domain_user.User>
sequenceDiagram
autonumber
actor WidgetTree
participant WnImage
participant State as _WnImageState
note over WnImage,State: Stateful rendering decision flow
WidgetTree->>WnImage: build(src, size, ...)
WnImage->>State: createState/initState
State->>State: _computeImageProperties(src)
alt src changed later
WidgetTree->>WnImage: didUpdateWidget(new src)
State->>State: recompute + async local-file check
end
alt Network SVG
State-->>WidgetTree: SvgPicture.network(...)
else Network raster
State-->>WidgetTree: CachedNetworkImage(... DPR-based mem cache ...)
else Local file / Asset
State-->>WidgetTree: SvgPicture.file/Image.file or SvgPicture.asset/Image.asset
end
sequenceDiagram
autonumber
actor User
participant NetworkScreen
participant OptimizedScroll as _OptimizedScrollView
participant RelaySection
participant Notifier as RelaysNotifier
note over User,NetworkScreen: Pull-to-refresh relays
User->>OptimizedScroll: drag down
OptimizedScroll->>NetworkScreen: throttled onPull(progress)
alt threshold reached
NetworkScreen->>NetworkScreen: _refreshData()
NetworkScreen->>Notifier: refreshStatuses() then fetchAll()
Notifier-->>NetworkScreen: updated RelayState
NetworkScreen-->>RelaySection: relayState updated
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
lib/utils/message_converter.dart (1)
529-541: Normalize pubkeys before fetching to avoid cache misses and “isMe” mismatchesIf upstream can emit npub or hex, compare and fetch using a normalized form. Also avoid scheduling with microtask unless you’re inside a build; here it’s safe to call directly.
Apply this diff to normalize and use the normalized key consistently:
- // If not found in contacts, fetch directly from API with build-safe scheduling - return await Future.microtask(() async { - final userProfileNotifier = ref.read(userProfileDataProvider.notifier); - final contactModel = await userProfileNotifier.getUserProfileData(pubkey); + // If not found in contacts, fetch from profile data provider + final userProfileNotifier = ref.read(userProfileDataProvider.notifier); + final normalized = pubkey; // replace with extension when available + final contactModel = await userProfileNotifier.getUserProfileData(normalized); - return User( - id: pubkey, - displayName: contactModel.displayName, - nip05: contactModel.nip05 ?? '', - publicKey: pubkey, - imagePath: contactModel.imagePath, - ); - }); + return User( + id: normalized, + displayName: contactModel.displayName, + nip05: contactModel.nip05 ?? '', + publicKey: normalized, + imagePath: contactModel.imagePath, + );Outside this hunk, import and use your pubkey normalization extension (see next comment).
lib/ui/settings/general_settings_screen.dart (1)
209-215: Logic bug: multiple accounts thresholdhasMultipleAccounts should be > 1, not > 2. As written, two accounts won’t trigger the multiple-account flow.
- final hasMultipleAccounts = _accounts.length > 2; + final hasMultipleAccounts = _accounts.length > 1;lib/ui/settings/developer/developer_settings_screen.dart (3)
114-123: Potential setState after dispose in async fetch.
setStateis called after anawaitwithout a mounted check; can throw if the screen is popped mid-request.try { final keyPackages = await accounts_api.accountKeyPackages( accountPubkey: activePubkey, ); + if (!mounted) return; setState(() { _keyPackages = keyPackages; _showKeyPackages = true; });
84-96: Same mounted guard needed after delete-all before setState.
Avoids setState on a disposed State when user leaves during the call.try { final deletedCount = await accounts_api.deleteAccountKeyPackages( accountPubkey: activePubkey, ); + if (!mounted) return; ref.showSuccessToast('Deleted $deletedCount key packages successfully'); // Clear the displayed key packages if they were being shown if (_showKeyPackages) { setState(() { _keyPackages = []; _showKeyPackages = false; }); }
168-201: Destructive dialogs should not be barrier-dismissible.
Accidental outside taps will dismiss the confirmation; setbarrierDismissible: false.- final confirmed = await showDialog<bool>( + final confirmed = await showDialog<bool>( context: context, barrierColor: Colors.transparent, + barrierDismissible: false, builder: (dialogContext) => WnDialog(Apply the same to the “Delete All Key Packages” dialog above.
lib/config/providers/group_provider.dart (4)
263-265: Missing await when extracting ApiError message.
e.messageText()returns a Future; logging will print a Future instead of the actual message.- final errorDetails = e.messageText(); + final errorDetails = await e.messageText();
919-927: Filter invalid hex conversions before adding members.Empty strings from
toHex() ?? ''will be sent to the API, likely causing failures or silent no-ops.- final usersPubkeyHex = - membersNpubs.map((userNpub) { - return PubkeyFormatter(pubkey: userNpub).toHex() ?? ''; - }).toList(); + final usersPubkeyHex = membersNpubs + .map((npub) => PubkeyFormatter(pubkey: npub).toHex()) + .whereType<String>() + .toList(); + if (usersPubkeyHex.isEmpty) { + state = state.copyWith(error: 'No valid member pubkeys to add.'); + return; + }
973-977: Apply the same conversion filtering when removing members.Mirror the add flow to avoid passing invalid pubkeys.
- final usersPubkeyHex = - membersNpubs.map((userNpub) { - return PubkeyFormatter(pubkey: userNpub).toHex() ?? ''; - }).toList(); + final usersPubkeyHex = membersNpubs + .map((npub) => PubkeyFormatter(pubkey: npub).toHex()) + .whereType<String>() + .toList(); + if (usersPubkeyHex.isEmpty) { + state = state.copyWith(error: 'No valid member pubkeys to remove.'); + return; + }
1063-1087: Handle null conversions in DM “other member” selection.If
toHex()returns null for the active or member keys, current logic can falsely include/exclude members.domain_user.User? getOtherGroupMember(String? groupId) { if (groupId == null) return null; final activePubkey = ref.read(activePubkeyProvider); if (activePubkey == null || activePubkey.isEmpty) return null; final members = getGroupMembers(groupId); if (members == null || members.isEmpty) return null; - final hexActivePubkey = PubkeyFormatter(pubkey: activePubkey).toHex(); - final otherMembers = - members - .where( - (member) => PubkeyFormatter(pubkey: member.publicKey).toHex() != hexActivePubkey, - ) - .toList(); + final hexActivePubkey = PubkeyFormatter(pubkey: activePubkey).toHex(); + if (hexActivePubkey == null) { + _logger.warning('GroupsProvider: Active pubkey failed to normalize for group $groupId.'); + return null; + } + final otherMembers = members + .where((member) { + final mHex = PubkeyFormatter(pubkey: member.publicKey).toHex(); + return mHex != null && mHex != hexActivePubkey; + }) + .toList();
🧹 Nitpick comments (21)
rust/Cargo.toml (1)
26-26: Avoid mixingversionwith agitdependency; removeversionto reduce confusion.For git-sourced dependencies, Cargo ignores
version. Keeping it can mislead readers. Prefer onlygit+rev(ortagif available).-whitenoise = { version = "0.1.0", git = "https://github.com/parres-hq/whitenoise", rev = "0590dc1f1e3a7d288413781390ad789f93c54f24" } +whitenoise = { git = "https://github.com/parres-hq/whitenoise", rev = "0590dc1f1e3a7d288413781390ad789f93c54f24" }test/ui/settings/profile/share_profile_qr_scan_screen_test.dart (3)
144-158: Back button tap: prefer deterministic settle and assert effectUsing pump() right after tap may be flaky; also the test doesn’t assert that a pop actually occurred. Consider settling frames or pumping with a small duration and asserting navigation state.
Apply this diff to make the interaction deterministic and verify the screen was popped:
await tester.pumpWidget( const MaterialApp( - home: ShareProfileQrScanScreen(), + home: Navigator( + onGenerateRoute: (_) => MaterialPageRoute( + builder: (_) => const ShareProfileQrScanScreen(), + ), + ), ), ); @@ await tester.tap(backButton); - await tester.pump(); + await tester.pumpAndSettle(); + expect(find.byType(ShareProfileQrScanScreen), findsNothing);
189-202: Lifecycle observer test: settle frames to avoid flakinessA single pump() may not process queued microtasks/animations. Use pumpAndSettle() (or pump with a short duration) to stabilize.
- await tester.pump(); + await tester.pumpAndSettle();
312-319: Accessibility tap: settle after interactionSame as above—prefer pumpAndSettle() for deterministic behavior.
- await tester.pump(); + await tester.pumpAndSettle();lib/ui/settings/network/widgets/relay_tile.dart (1)
119-131: Truncated URL: add semantics label for screen readersEllipsized text may be ambiguous to AT. Provide a semanticsLabel with the full sanitized URL.
- child: RepaintBoundary( - child: Text( - widget.relayInfo.url.sanitizedUrl, + child: RepaintBoundary( + child: Text( + widget.relayInfo.url.sanitizedUrl, style: TextStyle( color: context.colors.primary, fontWeight: FontWeight.w600, fontSize: 12.sp, ), overflow: TextOverflow.ellipsis, + semanticsLabel: widget.relayInfo.url.sanitizedUrl, ), ),lib/ui/core/ui/wn_image.dart (1)
158-170: Double-apply color on CachedNetworkImageYou already tint in imageBuilder via Image(... color: ...). Passing color/colorBlendMode to CachedNetworkImage may be unsupported by some versions and is redundant.
- color: widget.color, - colorBlendMode: widget.color == null ? null : BlendMode.srcIn,test/ui/contact_list/widgets/user_profile_test.dart (1)
103-110: Make the copy-button finder robust (avoid byType ambiguity).
Relying on find.byType(IconButton) can break if other IconButtons are added. Prefer a Key-based finder.Apply this diff to the test, assuming the production IconButton gets a key:
- final copyButton = find.byType(IconButton); + final copyButton = find.byKey(const Key('copyPubkeyButton'));Outside this file (in lib/ui/contact_list/widgets/user_profile.dart), add a key to the copy button:
IconButton( key: const Key('copyPubkeyButton'), icon: WnImage(...), onPressed: ..., )lib/ui/core/ui/wn_avatar.dart (2)
61-69: Don’t require a nullable argument.
displayNameisString?but markedrequired. Make it optional for cleaner call sites.- const FallbackAvatar({ + const FallbackAvatar({ super.key, - required this.displayName, + this.displayName, required this.size, });
86-96: Minor: prefer ColoredBox for solid backgrounds.
Tiny perf/readability win inside the padding.- return Padding( - padding: EdgeInsets.all(size * 0.25), - child: Center( - child: WnImage( + return Padding( + padding: EdgeInsets.all(size * 0.25), + child: Center( + child: WnImage( AssetsPaths.icUser, width: size * 0.4, height: size * 0.4, color: context.colors.primary, ), ), );lib/ui/settings/developer/developer_settings_screen.dart (2)
268-273: Use .h for vertical padding.
bottom: 24.wshould be24.hto scale with height.- padding: EdgeInsets.only( + padding: EdgeInsets.only( left: 16.w, right: 16.w, - bottom: 24.w, + bottom: 24.h, ),
395-497: Good separation with _KeyPackageItem; a few polish suggestions.
- Consider a semantic label for the delete icon for accessibility.
- Courier may not exist on all platforms; prefer a bundled mono font (e.g., RobotoMono) or theme textStyle with
fontFeatures/monospace.lib/ui/settings/network/network_screen.dart (3)
151-156: Unify pull-to-refresh thresholds (px vs .h).
Indicator opacity uses60.h, trigger uses literal60.0. This can feel inconsistent across devices.Introduce configurable thresholds from the parent:
- const _OptimizedScrollView({ - required this.children, - required this.onPullToRefresh, - required this.onPullStateChanged, - }); + const _OptimizedScrollView({ + required this.children, + required this.onPullToRefresh, + required this.onPullStateChanged, + required this.refreshTrigger, + }); final void Function(bool, double) onPullStateChanged; + final double refreshTrigger; // pass 60.h from parent - if (_isPulling && _pullDistance >= 60.0) { + if (_isPulling && _pullDistance >= widget.refreshTrigger) {And pass
refreshTrigger: 60.hfrom the parent.Also applies to: 355-357
305-314: Type onPullToRefresh to Future and mark unawaited call.
Prevents analyzer “unawaited_futures” noise and documents async semantics.- final VoidCallback onPullToRefresh; + final Future<void> Function() onPullToRefresh;- if (_isPulling && _pullDistance >= 60.0) { - widget.onPullToRefresh(); + if (_isPulling && _pullDistance >= widget.refreshTrigger) { + // fire-and-forget by design + // ignore: discarded_futures + widget.onPullToRefresh(); }Alternatively, use
unawaited(widget.onPullToRefresh());if available.Also applies to: 356-358
338-345: Also handle OverscrollNotification for robustness.
Some physics report overscroll viaOverscrollNotification(not negative pixels). Consider handling it for consistent UX.lib/ui/settings/network/widgets/relay_section.dart (2)
41-56: Avoid runtime type checks; prefer a common interface.
Downcasting to specific notifiers is brittle. Define an abstract base withaddRelay/deleteRelayand type the field to that.Example:
abstract class RelayCommands { Future<void> addRelay(String url); Future<void> deleteRelay(String url); }Have your notifiers implement it and change
final RelayCommands relayNotifier;.
135-142: Surface error details to aid debugging.
Rendering a generic “Error Loading Relays” hides useful info. Consider showing a terse message or a retry.lib/config/providers/follows_provider.dart (2)
116-123: Index follows by normalized hex for O(1) lookups.
toHex()is called per user per lookup. Keep aMap<String, User>(hex->User) in state or Notifier and build it once on load. This will also speed upisFollowing.I can propose a small refactor in
loadFollows()to populate an index and then replace this lookup withreturn _followsByHex[hexPubkey];.
126-130: Unify on single source of truth for following checks.Leverage
findFollowByPubkeyto avoid duplicate conversion logic.- bool isFollowing(String pubkey) { - final hexPubkey = PubkeyFormatter(pubkey: pubkey).toHex(); - if (hexPubkey == null) return false; - return state.follows.any((user) => PubkeyFormatter(pubkey: user.pubkey).toHex() == hexPubkey); - } + bool isFollowing(String pubkey) { + return findFollowByPubkey(pubkey) != null; + }lib/config/providers/group_provider.dart (3)
404-433: Parity: leverage follows cache for admins too.Admins path doesn’t attempt cache lookup, missing a cheap win.
- for (final adminPubkey in adminPubkeys) { + for (final adminPubkey in adminPubkeys) { try { final npub = PubkeyFormatter(pubkey: adminPubkey).toNpub() ?? ''; - - try { + // Try follows cache first + final followsNotifier = ref.read(followsProvider.notifier); + final existingFollow = followsNotifier.findFollowByPubkey(adminPubkey); + if (existingFollow != null) { + _logger.info('Found admin $npub in follows cache'); + final domainUser = domain_user.User.fromMetadata(existingFollow.metadata, npub); + admins.add(domainUser); + continue; + } + try { final metadata = await rust_users.userMetadata(pubkey: adminPubkey); final user = domain_user.User.fromMetadata(metadata, npub); admins.add(user);
610-619: Avoid repeated getGroupInformation calls; use cached type when available.You already cache group types in state. Prefer that for display name branching to reduce RPCs.
- final groupInformation = await getGroupInformation( - accountPubkey: activePubkey, - groupId: group.mlsGroupId, - ); - if (groupInformation.groupType == GroupType.directMessage) { + final cachedType = getCachedGroupType(group.mlsGroupId); + final type = cachedType ?? await getGroupType(group); + if (type == GroupType.directMessage) {
231-240: Minor log clarity: log the actual combined admin keys.Message prints
adminPublicKeyHexsinstead of the combined set with the creator.- _logger.info(' - Admin pubkeys: $adminPublicKeyHexs'); + _logger.info(' - Admin pubkeys (combined): $combinedAdminKeys');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
android/app/src/main/jniLibs/arm64-v8a/librust_lib_whitenoise.sois excluded by!**/*.soandroid/app/src/main/jniLibs/armeabi-v7a/librust_lib_whitenoise.sois excluded by!**/*.soandroid/app/src/main/jniLibs/x86_64/librust_lib_whitenoise.sois excluded by!**/*.sorust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
lib/config/providers/follows_provider.dart(1 hunks)lib/config/providers/group_provider.dart(9 hunks)lib/config/providers/metadata_cache_provider.dart(0 hunks)lib/ui/core/ui/wn_avatar.dart(1 hunks)lib/ui/core/ui/wn_image.dart(3 hunks)lib/ui/settings/developer/developer_settings_screen.dart(4 hunks)lib/ui/settings/general_settings_screen.dart(8 hunks)lib/ui/settings/network/network_screen.dart(6 hunks)lib/ui/settings/network/widgets/relay_expansion_tile.dart(0 hunks)lib/ui/settings/network/widgets/relay_section.dart(1 hunks)lib/ui/settings/network/widgets/relay_tile.dart(1 hunks)lib/utils/message_converter.dart(3 hunks)rust/Cargo.toml(1 hunks)test/ui/contact_list/widgets/user_profile_test.dart(2 hunks)test/ui/settings/profile/share_profile_qr_scan_screen_test.dart(3 hunks)
💤 Files with no reviewable changes (2)
- lib/ui/settings/network/widgets/relay_expansion_tile.dart
- lib/config/providers/metadata_cache_provider.dart
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#594
File: lib/src/rust/api/utils.dart:12-16
Timestamp: 2025-09-07T02:15:31.931Z
Learning: The lingering PublicKey references in lib/config/providers/auth_provider.dart, lib/config/providers/group_provider.dart, and rust_builder/cargokit/build_tool/lib/src/options.dart identified during the utils.dart API cleanup were fixed in PR #597 in the whitenoise_flutter codebase.
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#594
File: lib/config/providers/user_profile_data_provider.dart:0-0
Timestamp: 2025-09-03T20:57:53.202Z
Learning: In the whitenoise_flutter codebase, pubkey normalization (npub/hex format handling) is implemented in a pubkey validations extension rather than being imported directly into individual providers like user_profile_data_provider.dart.
📚 Learning: 2025-09-03T20:57:53.202Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#594
File: lib/config/providers/user_profile_data_provider.dart:0-0
Timestamp: 2025-09-03T20:57:53.202Z
Learning: In the whitenoise_flutter codebase, pubkey normalization (npub/hex format handling) is implemented in a pubkey validations extension rather than being imported directly into individual providers like user_profile_data_provider.dart.
Applied to files:
lib/config/providers/follows_provider.dartlib/config/providers/group_provider.dart
📚 Learning: 2025-09-07T02:15:31.931Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#594
File: lib/src/rust/api/utils.dart:12-16
Timestamp: 2025-09-07T02:15:31.931Z
Learning: The lingering PublicKey references in lib/config/providers/auth_provider.dart, lib/config/providers/group_provider.dart, and rust_builder/cargokit/build_tool/lib/src/options.dart identified during the utils.dart API cleanup were fixed in PR #597 in the whitenoise_flutter codebase.
Applied to files:
lib/config/providers/follows_provider.dartlib/utils/message_converter.dartlib/config/providers/group_provider.dart
📚 Learning: 2025-08-12T11:21:53.640Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#455
File: lib/ui/settings/profile/switch_profile_bottom_sheet.dart:0-0
Timestamp: 2025-08-12T11:21:53.640Z
Learning: In the whitenoise_flutter codebase, ContactModel.publicKey can be stored in either npub format (from metadata cache) or hex format (from account storage). The activeAccountProvider may return either format depending on how the account was originally stored, so normalization to hex format is required when comparing with other hex-normalized keys in sorting logic.
Applied to files:
lib/config/providers/follows_provider.dart
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use Riverpod to manage state. Use StreamProviders to watch for state changes that come from the rust api. See keepAlive if you need to keep the state alive.
Applied to files:
lib/ui/settings/network/network_screen.dartlib/utils/message_converter.dart
📚 Learning: 2025-08-23T11:02:28.308Z
Learnt from: Quwaysim
PR: parres-hq/whitenoise_flutter#527
File: lib/ui/core/ui/wn_avatar.dart:1-6
Timestamp: 2025-08-23T11:02:28.308Z
Learning: In the whitenoise_flutter codebase, flutter_screenutil is re-exported through lib/ui/core/themes/src/app_theme.dart, so files that import app_theme.dart do not need to directly import flutter_screenutil to use .w, .h, .sp extensions.
Applied to files:
lib/ui/settings/network/network_screen.darttest/ui/contact_list/widgets/user_profile_test.dart
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*_test.dart : Use the standard widget testing for Flutter.
Applied to files:
test/ui/contact_list/widgets/user_profile_test.dart
📚 Learning: 2025-09-07T13:10:16.542Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#597
File: lib/config/providers/group_provider.dart:311-314
Timestamp: 2025-09-07T13:10:16.542Z
Learning: In the whitenoise_flutter codebase, the User class used in group_provider.dart (and similar contexts) is presentational only, not the actual user class from the Rust API. There are plans to remove this User class and replace it with UserProfileData, similar to the planned consolidation with ContactModel.
Applied to files:
test/ui/contact_list/widgets/user_profile_test.dartlib/utils/message_converter.dartlib/config/providers/group_provider.dart
📚 Learning: 2025-09-08T17:39:13.711Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#599
File: test/ui/contact_list/start_chat_bottom_sheet_test.dart:173-176
Timestamp: 2025-09-08T17:39:13.711Z
Learning: In the whitenoise Flutter app, the followProvider uses followsProvider behind the scenes, so mocking followsProvider with MockFollowsNotifier effectively controls what followProvider sees in tests.
Applied to files:
lib/utils/message_converter.dartlib/config/providers/group_provider.dart
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use the whitenoise rust crate (via flutter_rust_bridge) as the source of all data and only way to trigger changes in our data model.
Applied to files:
lib/utils/message_converter.dartlib/config/providers/group_provider.dart
📚 Learning: 2025-09-05T00:08:34.509Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#594
File: lib/config/providers/active_account_provider.dart:140-146
Timestamp: 2025-09-05T00:08:34.509Z
Learning: josefinalliende prefers centralizing validation logic (like trimming whitespace) in providers rather than handling it defensively at every consumer site. This approach was applied to activePubkeyProvider in the whitenoise_flutter codebase.
Applied to files:
lib/config/providers/group_provider.dart
🔇 Additional comments (15)
rust/Cargo.toml (1)
26-26: Verify the pinned commit and lockfile to ensure reproducible builds.Ran the verification script: Cargo.lock not found; git ls-remote returned no match for rev 0590dc1f1e3a7d288413781390ad789f93c54f24 — verification inconclusive.
- Confirm rev 0590dc1f1e3a7d288413781390ad789f93c54f24 exists in https://github.com/parres-hq/whitenoise and matches the intended perf fix.
- If the repository tracks Cargo.lock, update and commit Cargo.lock so the exact git rev is recorded.
- If a release tag points to that commit, prefer pinning by tag for readability.
File: rust/Cargo.toml (line 26)
lib/utils/message_converter.dart (1)
3-3: Provider migration LGTMSwitching to user_profile_data_provider is the right direction and aligns with recent refactors.
lib/ui/settings/general_settings_screen.dart (2)
55-58: Good: invalidate cache on active account updatesThis ensures fresh data after profile changes.
115-129: Package info loader: solid guard/finally resetThe idempotent guard and finally-reset pattern looks good.
lib/ui/core/ui/wn_image.dart (2)
125-137: Device pixel ratio caching: goodCaching DPR avoids repeated lookups; no concerns.
240-248: Clip only when needed: LGTMConditional ClipRRect based on borderRadius keeps it lightweight.
test/ui/contact_list/widgets/user_profile_test.dart (1)
5-5: LGTM: updated import aligns with WnImage migration.
No issues; keeps the test type-safe for isA().lib/ui/core/ui/wn_avatar.dart (2)
29-39: Border/layout refactor looks good.
Outer circular border + inner ClipOval is cleaner and avoids double clips.
44-53: Confirm WnImage sizing API usage (size vs width/height).
This usessize: sizehere, while FallbackAvatar useswidth/height. Ensure WnImage still supportssizeafter the refactor, or standardize on width/height for consistency.If needed, switch to width/height:
- child: WnImage( - imageUrl, - size: size, + child: WnImage( + imageUrl, + width: size, + height: size, fit: BoxFit.cover, fallbackWidget: ... ),lib/ui/settings/developer/developer_settings_screen.dart (1)
331-341: Alpha adjustment helper: confirm availability across channels.
.withValues(alpha: 0.3)may rely on custom extensions or newer Flutter APIs. If custom, fine; if not, preferwithOpacity(0.3)for compatibility.lib/ui/settings/network/network_screen.dart (1)
144-149: Header extraction + RepaintBoundary: good for rasterization.
Clean separation; no issues spotted.lib/ui/settings/network/widgets/relay_section.dart (1)
144-159: ListView inside parent ListView: good use of NeverScrollable + shrinkWrap.
This avoids nested scrolling issues.lib/config/providers/group_provider.dart (3)
9-12: Imports look good; domain alias improves clarity.No functional issues here. The
follows_providerimport enables the cache path below.
311-326: existingFollow.metadata is FlutterMetadata — type-safe; optional: guard empty npubexistingFollow.metadata is a FlutterMetadata (rust API User.metadata) and domain_user.User.fromMetadata accepts FlutterMetadata — no runtime type error. Optional: avoid using PubkeyFormatter(...).toNpub() ?? '' — validate/skip null-or-empty npub before constructing domain_user.User to prevent users with empty id/publicKey.
Likely an incorrect or invalid review comment.
730-736: No downstream changes required — call sites & tests already use domain_user.UserVerified: tests (test/config/providers/group_provider_test.dart — expect(members, [testUser]) / expect(admins, [testUser])) and internal usages (lib/config/providers/group_provider.dart — members.map((m) => m.publicKey)) operate on domain_user.User, so no API-update work needed.
josefinalliende
left a comment
josefinalliende
left a comment
There was a problem hiding this comment.
Lots of Loc, haven't tried it locally yet, but this comment I think it's important.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/ui/settings/network/widgets/relay_tile.dart (2)
34-35: Annotate local variable type per guidelinesExplicitly type locals in Dart per repo rules.
- final confirmed = await showDialog<bool>( + final bool? confirmed = await showDialog<bool>(
45-83: Localize user-facing strings in lib/ui/settings/network/widgets/relay_tile.dartReplace literal strings with AppLocalizations: "Remove Relay?", the confirmation body text, "Cancel", "Remove Relay", and the toast "Relay removed successfully". Locations: lines 45–83 and 93. If you share the localization keys I will propose the exact patch wiring AppLocalizations (including the toast).
♻️ Duplicate comments (3)
lib/ui/settings/network/widgets/relay_tile.dart (1)
134-152: Delete control: still-too-small hit target + missing tooltip/value; align copy with dialogGood fix gating on showOptions. Please also enlarge the tap target to ~44x44, add a Tooltip, include the URL as Semantics.value, and align copy to “Remove relay” (dialog uses “Remove Relay?”).
- if (widget.showOptions) - RepaintBoundary( - child: Semantics( - label: 'Delete relay', - button: true, - child: GestureDetector( - onTap: _removeRelay, - behavior: HitTestBehavior.opaque, - child: Padding( - padding: EdgeInsets.all(4.w), - child: WnImage( - AssetsPaths.icDelete, - color: context.colors.primary, - size: 20.w, - ), - ), - ), - ), - ), + if (widget.showOptions) + RepaintBoundary( + child: Semantics( + label: 'Remove relay', + value: widget.relayInfo.url.sanitizedUrl, + button: true, + child: Tooltip( + message: 'Remove relay', + child: GestureDetector( + onTap: _removeRelay, + behavior: HitTestBehavior.opaque, + child: SizedBox( + width: 44.w, + height: 44.w, + child: Center( + child: WnImage( + AssetsPaths.icDelete, + color: context.colors.primary, + size: 20.w, + ), + ), + ), + ), + ), + ), + ),lib/ui/settings/general_settings_screen.dart (1)
70-80: Cache guard now correctly checks profile data presence — LGTMThe early-return condition includes both accounts and profile data, addressing the prior concern.
lib/ui/core/ui/wn_image.dart (1)
96-111: Short-circuiting file checks for non-file URIs — LGTMThe heuristic avoids unnecessary
File.existsI/O for assets/virtual URIs.
🧹 Nitpick comments (11)
lib/ui/settings/network/widgets/relay_tile.dart (3)
100-116: Overuse of RepaintBoundary; keep only the outer oneNested RepaintBoundary widgets add compositing overhead and won’t further reduce repaints. Keep the outer boundary and remove the inner ones.
- return RepaintBoundary( - child: Container( + return RepaintBoundary( + child: Container( padding: EdgeInsets.symmetric(horizontal: 16.w, vertical: 12.h), decoration: BoxDecoration( color: context.colors.surface, borderRadius: BorderRadius.circular(8.r), ), child: Row( children: [ - RepaintBoundary( - child: WnImage( - widget.relayInfo.status.getIconAsset(), - color: widget.relayInfo.status.getColor(context), - size: 24.w, - ), - ), + WnImage( + widget.relayInfo.status.getIconAsset(), + color: widget.relayInfo.status.getColor(context), + size: 24.w, + ), SizedBox(width: 12.w), - Expanded( - child: RepaintBoundary( - child: Text( - widget.relayInfo.url.sanitizedUrl, - style: TextStyle( - color: context.colors.primary, - fontWeight: FontWeight.w600, - fontSize: 12.sp, - ), - overflow: TextOverflow.ellipsis, - ), - ), - ), + Expanded( + child: Text( + widget.relayInfo.url.sanitizedUrl, + style: TextStyle( + color: context.colors.primary, + fontWeight: FontWeight.w600, + fontSize: 12.sp, + ), + overflow: TextOverflow.ellipsis, + ), + ), - if (widget.showOptions) - RepaintBoundary( - child: Semantics( - label: 'Delete relay', - button: true, - child: GestureDetector( - onTap: _removeRelay, - behavior: HitTestBehavior.opaque, - child: Padding( - padding: EdgeInsets.all(4.w), - child: WnImage( - AssetsPaths.icDelete, - color: context.colors.primary, - size: 20.w, - ), - ), - ), - ), - ), + // Keep only one outer RepaintBoundary for the tile; action semantics handled elsewhere. + if (widget.showOptions) + Semantics( + label: 'Remove relay', + value: widget.relayInfo.url.sanitizedUrl, + button: true, + child: Tooltip( + message: 'Remove relay', + child: GestureDetector( + onTap: _removeRelay, + behavior: HitTestBehavior.opaque, + child: SizedBox( + width: 44.w, + height: 44.w, + child: Center( + child: WnImage( + AssetsPaths.icDelete, + color: context.colors.primary, + size: 20.w, + ), + ), + ), + ), + ), + ),Also applies to: 120-131, 135-152
53-60: Add tooltip to close button for a11yProvide long-press tooltip and better a11y.
- IconButton( + IconButton( + tooltip: 'Close', onPressed: () => Navigator.of(context).pop(false), icon: WnImage( AssetsPaths.icClose, color: context.colors.primary, size: 24.w, ), ),
16-21: Guard API: require onDelete when showOptions is trueAvoid rendering a delete UI that can no-op. Add an assert to keep contract tight in debug builds.
const RelayTile({ super.key, required this.relayInfo, this.showOptions = false, this.onDelete, - }); + }) : assert(!showOptions || onDelete != null, 'onDelete is required when showOptions is true');lib/ui/settings/general_settings_screen.dart (4)
171-176: Prevent accidental dismissal of sign-out dialogMake the confirmation modal non-dismissible by tapping outside.
- final confirmed = await showDialog<bool>( + final confirmed = await showDialog<bool>( context: context, - barrierColor: Colors.transparent, + barrierColor: Colors.transparent, + barrierDismissible: false, builder:
176-199: Localize user-facing strings and move to constantsReplace hard-coded strings with
AppLocalizationskeys and centralize any reusable labels in constants per guidelines.Example:
- title: 'Sign out', + title: AppLocalizations.of(context)!.signOut,Also applies to: 269-275, 290-299, 315-333, 349-357, 373-377, 386-393, 240-241
134-146: Minor: rename for clarity
hasMultipleAccountsactually checks for “≥3 accounts”. Consider renaming tohasMoreThanTwoAccountsorhasAnotherChoice.- final hasMultipleAccounts = _accounts.length > 2; + final hasMoreThanTwoAccounts = _accounts.length > 2; ... - if (hasMultipleAccounts) { + if (hasMoreThanTwoAccounts) {
46-132: Style: extra blank lines within functionsThere are several blank lines inside functions; the repo guidelines ask to avoid them. Consider tightening to improve consistency.
Also applies to: 148-249, 251-399
lib/ui/core/ui/wn_image.dart (4)
113-131: Avoid stale result whensrcchanges during async file checkCapture
srcbefore awaiting and only apply the result if it’s unchanged.- void _checkLocalFileAsync() async { + void _checkLocalFileAsync() async { + final currentSrc = widget.src; try { - final file = File(widget.src); + final file = platformFile(currentSrc) as File; // ignore: avoid_slow_async_io - final exists = await file.exists(); - if (mounted) { + final exists = await platformFileExists(currentSrc); + if (mounted && currentSrc == widget.src) { setState(() { _isLocalFile = exists; }); } } catch (e) { - if (mounted) { + if (mounted && currentSrc == widget.src) { setState(() { _isLocalFile = false; }); } } }
88-96: Robust SVG detectionFor URLs with query strings (e.g.,
icon.svg?v=1),endsWith('.svg')fails. Prefer checking the parsed URI path.- _isSvg = widget.src.toLowerCase().endsWith('.svg'); + final pathLower = (_parsedUri?.path ?? widget.src).toLowerCase(); + _isSvg = pathLower.endsWith('.svg');
166-191: Redundant properties when usingimageBuilderWhen
imageBuilderis supplied,fit,alignment,color, andcolorBlendModeonCachedNetworkImageare ignored. Keep them only on the builtImageto avoid confusion.- child = CachedNetworkImage( + child = CachedNetworkImage( imageUrl: widget.src, width: width, height: height, - fit: widget.fit, - alignment: widget.alignment, memCacheWidth: width == null ? null : (width * _cachedDevicePixelRatio!).round(), memCacheHeight: height == null ? null : (height * _cachedDevicePixelRatio!).round(), cacheKey: widget.cacheKey, placeholder: (context, url) => _buildPlaceholder(context), errorWidget: (context, url, error) => _buildFallback(context), - color: widget.color, - colorBlendMode: widget.color == null ? null : BlendMode.srcIn, imageBuilder: (context, provider) => Image( image: provider, width: width, height: height, fit: widget.fit, alignment: widget.alignment, color: widget.color, colorBlendMode: widget.color == null ? null : BlendMode.srcIn, semanticLabel: widget.semanticLabel, errorBuilder: (context, error, stackTrace) => _buildFallback(context), ), );
132-257: Style: extra blank lines within methodsMultiple blank lines inside methods; tighten per repo style guide.
Also applies to: 259-299
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
android/app/src/main/jniLibs/arm64-v8a/librust_lib_whitenoise.sois excluded by!**/*.soandroid/app/src/main/jniLibs/armeabi-v7a/librust_lib_whitenoise.sois excluded by!**/*.soandroid/app/src/main/jniLibs/x86_64/librust_lib_whitenoise.sois excluded by!**/*.sorust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
lib/config/providers/follows_provider.dart(2 hunks)lib/ui/core/ui/wn_image.dart(3 hunks)lib/ui/settings/general_settings_screen.dart(8 hunks)lib/ui/settings/network/widgets/relay_section.dart(1 hunks)lib/ui/settings/network/widgets/relay_tile.dart(1 hunks)lib/utils/message_converter.dart(1 hunks)rust/Cargo.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/utils/message_converter.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/config/providers/follows_provider.dart
- rust/Cargo.toml
- lib/ui/settings/network/widgets/relay_section.dart
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
lib/ui/core/ui/wn_image.dartlib/ui/settings/network/widgets/relay_tile.dartlib/ui/settings/general_settings_screen.dart
🔇 Additional comments (4)
lib/ui/settings/general_settings_screen.dart (3)
251-301: RepaintBoundary usage for hot UI paths — good perf hygieneWrapping heavy/interactive tiles and header elements with
RepaintBoundaryshould reduce unnecessary repaints.Also applies to: 310-336, 344-360, 368-381, 384-395
86-114: Account load concurrency guard and TTL update look correct
_isLoadingAccountsgating +_lastAccountsLoadTimerefresh prevent stampedes and stale data.
148-167: Prevent duplicate success toast when switching profilesSwitchProfileBottomSheet.show forwards showSuccessToast to the sheet while the onProfileSelected handler calls _switchAccount — _switchAccount also shows a success toast, which can produce duplicate feedback. Fix options:
- Add a bool showToast = true parameter to _switchAccount and call _switchAccount(..., showToast: false) from the bottom sheet; or
- Remove the bottom-sheet toast and centralize success feedback inside _switchAccount.
Verify implementation at lib/ui/settings/general_settings_screen.dart ( _switchAccount ≈ line 134; SwitchProfileBottomSheet.show call ≈ lines 158–166). Run to inspect the function: sed -n '120,180p' lib/ui/settings/general_settings_screen.dart
lib/ui/core/ui/wn_image.dart (1)
1-1: Web-compat: avoid unconditionaldart:ioin lib/ui/core/ui/wn_image.dart
- File unconditionally imports 'dart:io' and calls File(...)/exists(); this will break Flutter Web builds.
- Repo shows mixed signals (README: "Web — Coming soon", pubspec.lock contains web adapters; local note says repo currently doesn't target web) — confirm whether web is a supported target.
- If web is or may become a target: replace direct dart:io usage with conditional imports + a platform adapter (e.g., platformFile/platformFileExists) and update File(...) usages accordingly.
- Location: lib/ui/core/ui/wn_image.dart (import line, _checkLocalFileAsync, and File(...) usages).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
rust/src/api/users.rs (4)
43-45: Unify error conversion (avoid mixed?vsmap_err).
get_useruses.map_err(ApiError::from)?while this call uses bare?. IfFrom<whitenoise::Error> for ApiErrorisn’t implemented, this won’t compile; if it is, we still have inconsistency. Pick one style repo‑wide.Option A (preferred): switch
get_userto?for consistency (outside this hunk):- let user = whitenoise - .find_or_create_user_by_pubkey(&pubkey) - .await - .map_err(ApiError::from)?; + let user = whitenoise.find_or_create_user_by_pubkey(&pubkey).await?;Option B: keep
.map_err(...)here too:- let user = whitenoise.find_or_create_user_by_pubkey(&pubkey).await?; + let user = whitenoise + .find_or_create_user_by_pubkey(&pubkey) + .await + .map_err(ApiError::from)?;
43-44: Behavior change: read paths now create users — confirm this is intended and safe.
find_or_create_user_by_pubkeycan write. Ensure this is idempotent, cheap, and bounded (no untrusted spam keys creating rows). Consider metrics, rate‑limits, and a feature flag if needed.
51-53: Apply the same error‑handling policy here.Match the decision above for
.await?vs.map_err(ApiError::from)?on both thefind_or_create...call andrelays_by_type(...).await?.- let user = whitenoise.find_or_create_user_by_pubkey(&pubkey).await?; + let user = whitenoise + .find_or_create_user_by_pubkey(&pubkey) + .await + .map_err(ApiError::from)?; - let relays = user.relays_by_type(relay_type, &whitenoise).await?; + let relays = user + .relays_by_type(relay_type, &whitenoise) + .await + .map_err(ApiError::from)?;
60-62: Nit: pass&whitenoisefor consistency and align error mapping.We pass
&whitenoiseelsewhere; do the same here and align error conversion.- let user = whitenoise.find_or_create_user_by_pubkey(&pubkey).await?; - Ok(user.key_package_event(whitenoise).await?.is_some()) + let user = whitenoise + .find_or_create_user_by_pubkey(&pubkey) + .await + .map_err(ApiError::from)?; + Ok(user + .key_package_event(&whitenoise) + .await + .map_err(ApiError::from)? + .is_some())lib/domain/models/contact_model.dart (2)
39-46: Unify fallback copy and consider localization.
'Unknown User'here vs'Unknown'inUsermodel diverge. Prefer a single constant (and eventually AppLocalizations in UI).
48-48: Minor naming clarity.
finalDisplayNamereads fine, butresolvedDisplayNameoreffectiveDisplayNamebetter conveys “selected via fallback.” Optional.lib/domain/models/user_model.dart (2)
20-27: Trim before emptiness checks and reuse sanitizer for parity with ContactModel.Use
.trim().isNotEmpty(or share_sanitizeString) to avoid treating whitespace as a valid name. This keeps behavior consistent across models.- if (metadata.displayName?.isNotEmpty == true) { - finalDisplayName = metadata.displayName!; - } else if (metadata.name?.isNotEmpty == true) { - finalDisplayName = metadata.name!; + final dn = metadata.displayName?.trim(); + final nm = metadata.name?.trim(); + if ((dn?.isNotEmpty ?? false)) { + finalDisplayName = dn!; + } else if ((nm?.isNotEmpty ?? false)) { + finalDisplayName = nm!; }
30-30: Normalize pubkey in equality/hash or guarantee callers pass hex.Given broader pubkey normalization changes, either:
- Normalize
publicKeyandidin==/hashCodelikeContactModel, or- Enforce a hex‑only invariant at construction (and document it).
Happy to provide a patch once you confirm the preferred invariant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/domain/models/contact_model.dart(1 hunks)lib/domain/models/user_model.dart(1 hunks)rust/src/api/users.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
lib/domain/models/user_model.dartlib/domain/models/contact_model.dart
🧠 Learnings (2)
📓 Common learnings
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#594
File: lib/src/rust/api/utils.dart:12-16
Timestamp: 2025-09-07T02:15:31.931Z
Learning: The lingering PublicKey references in lib/config/providers/auth_provider.dart, lib/config/providers/group_provider.dart, and rust_builder/cargokit/build_tool/lib/src/options.dart identified during the utils.dart API cleanup were fixed in PR #597 in the whitenoise_flutter codebase.
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#594
File: lib/config/providers/user_profile_data_provider.dart:0-0
Timestamp: 2025-09-03T20:57:53.202Z
Learning: In the whitenoise_flutter codebase, pubkey normalization (npub/hex format handling) is implemented in a pubkey validations extension rather than being imported directly into individual providers like user_profile_data_provider.dart.
📚 Learning: 2025-09-07T13:10:16.542Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#597
File: lib/config/providers/group_provider.dart:311-314
Timestamp: 2025-09-07T13:10:16.542Z
Learning: In the whitenoise_flutter codebase, the User class used in group_provider.dart (and similar contexts) is presentational only, not the actual user class from the Rust API. There are plans to remove this User class and replace it with UserProfileData, similar to the planned consolidation with ContactModel.
Applied to files:
lib/domain/models/user_model.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Flutter CI
🔇 Additional comments (1)
lib/domain/models/contact_model.dart (1)
31-31: LGTM on sanitizingname.Adding
_sanitizeString(metadata?.name)is a good defensive step and aligns with the existing sanitizers.
| // Use display_name first, then fall back to name, then to Unknown User | ||
| String finalDisplayName = 'Unknown User'; | ||
| if (displayName.isNotEmpty) { | ||
| finalDisplayName = displayName; | ||
| } else if (name.isNotEmpty) { | ||
| finalDisplayName = name; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix equals/hashCode contract: normalize key in hashCode too.
Equality compares normalized hex pubkeys, but hashCode (Line 98) hashes the raw publicKey. Two equal objects can then yield different hashes. Normalize in hashCode (and optionally DRY up equality).
Apply outside-range change:
@override
int get hashCode {
- return Object.hash(publicKey, imagePath, displayName, about, website, nip05, lud16);
+ final normalizedHex =
+ PubkeyFormatter(pubkey: publicKey).toHex() ?? publicKey;
+ return Object.hash(
+ normalizedHex,
+ imagePath,
+ displayName,
+ about,
+ website,
+ nip05,
+ lud16,
+ );
}Optional: extract a private getter to reuse in operator ==.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use display_name first, then fall back to name, then to Unknown User | |
| String finalDisplayName = 'Unknown User'; | |
| if (displayName.isNotEmpty) { | |
| finalDisplayName = displayName; | |
| } else if (name.isNotEmpty) { | |
| finalDisplayName = name; | |
| } | |
| @override | |
| int get hashCode { | |
| final String normalizedHex = | |
| PubkeyFormatter(pubkey: publicKey).toHex() ?? publicKey; | |
| return Object.hash( | |
| normalizedHex, | |
| imagePath, | |
| displayName, | |
| about, | |
| website, | |
| nip05, | |
| lud16, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In lib/domain/models/contact_model.dart around lines 39-46 (hashCode at ~line
98), equality currently compares normalized hex public keys but hashCode uses
the raw publicKey, breaking the equals/hashCode contract; normalize the key in
hashCode as well (same normalization used in operator==, e.g. strip 0x prefix
and lowercase) and update operator== to use that same normalized value; to DRY,
add a private getter (e.g. _normalizedPublicKey) that performs the normalization
and use it in both operator== and hashCode so both use the identical canonical
key.
| import 'package:whitenoise/config/providers/follows_provider.dart'; | ||
| import 'package:whitenoise/config/states/group_state.dart'; | ||
| import 'package:whitenoise/domain/models/user_model.dart'; | ||
| import 'package:whitenoise/domain/models/user_model.dart' as domain_user; |
There was a problem hiding this comment.
💯 nice, it was easy before to get confused between the user that came from rust crate and the user from the models file.
| } | ||
|
|
||
| User? findFollowByPubkey(String pubkey) { | ||
| return state.follows.where((user) => user.pubkey == pubkey).firstOrNull; |
There was a problem hiding this comment.
good catch, thanks for fixing it
| } | ||
|
|
||
| /// Optimized key package item widget with reduced rasterization cost | ||
| class _KeyPackageItem extends StatelessWidget { |
There was a problem hiding this comment.
[sug] maybe move this widget to a separate file? For me it's easier to find widgets if they are one per file. But jus suggesting
| decoration: BoxDecoration( | ||
| border: Border.all( | ||
| color: context.colors.border.withValues(alpha: 0.3), | ||
| width: 0.5, // Thinner border for better performance |
There was a problem hiding this comment.
Had no idea that border widths could affect performance...
There was a problem hiding this comment.
This is AI - I wonder if it's actually true. Claude seems to think so...
Yes, smaller border widths can improve performance in Flutter apps, though the impact is usually minimal.
Why it helps:
- Less pixel filling work for the GPU
- Reduced overdraw when borders overlap with other elements
- Simpler rasterization calculations
When it matters:
- Lists with many bordered items
- Complex layouts with nested borders
- Lower-end devices with limited GPU power
- High frame rate animations involving bordered widgets
For most apps, you won't notice the difference unless you're dealing with performance-critical scenarios or rendering hundreds of bordered elements simultaneously.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores