refactor: remove metadata cache provider #654
Conversation
WalkthroughReplaces in-provider message loading with a new groupMessagesProvider and fetchMessages API, adds optimistic send/update/failure flows and MessageMergerService, refactors MessageConverter and reaction/pubkey utilities, removes metadata_cache_provider and UI message extensions, and adds comprehensive unit tests across providers, converters, and merger logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Chat UI
participant CP as ChatProvider
participant GMP as groupMessagesProvider(groupId)
participant GMN as GroupMessagesNotifier
participant API as Messages API
participant Merge as MessageMergerService
UI->>CP: loadMessagesForGroup(groupId)
CP->>GMP: .notifier
GMP->>GMN: fetchMessages()
GMN->>API: fetchAggregatedMessages(pubkey, groupId)
API-->>GMN: List<ChatMessage>
GMN->>GMN: Convert via MessageConverter + profiles
GMN-->>CP: List<MessageModel> (dbMessages)
CP->>CP: Collect state messages (incl. optimistic)
CP->>Merge: merge(stateMessages, dbMessages)
Merge-->>CP: mergedMessages
CP-->>UI: mergedMessages (sorted)
sequenceDiagram
autonumber
participant UI as Chat UI
participant CP as ChatProvider
participant API as Send Message API
participant State as Local State
UI->>CP: sendMessage(groupId, content, replyTo?)
CP->>State: create & append optimistic MessageModel (status=sending)
CP->>API: send(content, replyToId?)
alt success
API-->>CP: serverMessage(id, createdAt, status)
CP->>State: update optimistic entry with id/createdAt/status
else error
API-->>CP: error
CP->>State: mark optimistic entry as failed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.md📄 CodeRabbit inference engine (.cursor/rules/whitenoise.mdc)
Files:
🧠 Learnings (2)📚 Learning: 2025-09-07T02:15:31.931ZApplied to files:
📚 Learning: 2025-09-16T07:24:07.489ZApplied to files:
⏰ 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)
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 review |
✅ Actions performedReview 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: 2
🧹 Nitpick comments (10)
lib/utils/message_converter.dart (4)
32-33: Use cache-aware reaction conversion to populate reaction users.Single-message path uses
_convertReactions, which hardcodes “Unknown User” and references the old cache. Switch to the cache‑aware helper.- final reactions = _convertReactions(messageData.reactions); + final reactions = _convertReactionsWithCache(messageData.reactions, usersMap);Also consider removing
_convertReactionsentirely if no longer needed, or clearly mark it as a fallback-only path.Also applies to: 155-156, 304-308
256-267: Remove unnecessary Future.microtask wrapping.
Future.microtaskadds scheduling overhead without benefit here; the function is alreadyasync.- return await Future.microtask(() async { - final userProfileDataNotifier = ref.read(userProfileDataProvider.notifier); - final userProfileData = await userProfileDataNotifier.getUserProfileData(pubkey); - - return domain_user.User( - id: pubkey, - displayName: userProfileData.displayName, - nip05: userProfileData.nip05 ?? '', - publicKey: pubkey, - imagePath: userProfileData.imagePath, - ); - }); + final userProfileDataNotifier = ref.read(userProfileDataProvider.notifier); + final userProfileData = await userProfileDataNotifier.getUserProfileData(pubkey); + return domain_user.User( + id: pubkey, + displayName: userProfileData.displayName, + nip05: userProfileData.nip05 ?? '', + publicKey: pubkey, + imagePath: userProfileData.imagePath, + );
97-104: Avoid async when not awaiting (micro-optimization and clarity).
fromChatMessageListuses noawait. Return a completed future without marking the functionasync.- static Future<List<MessageModel>> fromChatMessageList( + static Future<List<MessageModel>> fromChatMessageList( List<ChatMessage> chatMessages, { required String? currentUserPublicKey, String? groupId, required Ref ref, required Map<String, domain_user.User> usersMap, - }) async { + }) { ... - return messages; + return Future.value(messages);Also applies to: 128-129
285-291: Remove obsolete comment referencing the deleted metadata cache.This comment is stale post‑refactor.
- displayName: 'Unknown User', // Will be resolved by metadata cache later + displayName: 'Unknown User',lib/config/providers/chat_provider.dart (3)
182-199: Deduplicate chatMessagesMap construction (two identical blocks).The MessageModel → ChatMessage conversion is duplicated in send and reply flows. Extract a small helper to avoid drift.
- final chatMessagesMap = <String, ChatMessage>{}; - for (final msg in currentMessages) { - // Convert existing MessageModel back to ChatMessage for cache - final chatMessage = ChatMessage( - id: msg.id, - pubkey: msg.sender.publicKey, - content: msg.content ?? '', - createdAt: msg.createdAt, - tags: const [], - isReply: msg.replyTo != null, - replyToId: msg.replyTo?.id, - isDeleted: false, - contentTokens: const [], - reactions: const ReactionSummary(byEmoji: [], userReactions: []), - kind: msg.kind, // Use the actual message kind - ); - chatMessagesMap[msg.id] = chatMessage; - } + final chatMessagesMap = _buildChatMessagesMap(currentMessages);Add this helper to ChatNotifier:
Map<String, ChatMessage> _buildChatMessagesMap(List<MessageModel> models) { return { for (final m in models) m.id: ChatMessage( id: m.id, pubkey: m.sender.publicKey, content: m.content ?? '', createdAt: m.createdAt, tags: const [], isReply: m.replyTo != null, replyToId: m.replyTo?.id, isDeleted: false, contentTokens: const [], reactions: const ReactionSummary(byEmoji: [], userReactions: []), kind: m.kind, ) }; }Also applies to: 629-646
125-128: Replace magic Nostr kind numbers with named constants.Improves readability and prevents accidental mismatches.
- int kind = 9, // Default to text message + int kind = NostrKind.text, ... - kind: 7, // Nostr kind 7 = reaction + kind: NostrKind.reaction, ... - kind: 5, // Nostr kind 5 = deletion + kind: NostrKind.deletion,Add somewhere central (e.g., a constants file) and import:
class NostrKind { static const int text = 9; static const int reaction = 7; static const int deletion = 5; }Also applies to: 548-555, 713-715
429-444: Avoid using??=on a parameter for clarity.Slightly confusing to reassign the param. Prefer a local final.
- final gId = groupId ??= state.selectedGroupId; + final gId = groupId ?? state.selectedGroupId;lib/config/services/group_messages_service.dart (3)
21-36: Thread chatMessagesMap and activePubkey through helpers (no hidden reads).Avoid re‑reading providers inside helpers; use the already available inputs and include reaction authors in the prefetch to populate reaction avatars.
Future<MessageModel> toMessageModel({ required ChatMessage chatMessage, required String activePubkey, required Map<String, ChatMessage> chatMessagesMap, }) async { - final usersMap = await _fetchMessageUsersMap(chatMessage: chatMessage); + final usersMap = await _fetchMessageUsersMap( + chatMessage: chatMessage, + activePubkey: activePubkey, + chatMessagesMap: chatMessagesMap, + );- Future<Map<String, domain_user.User>> _fetchMessageUsersMap({ - required ChatMessage chatMessage, - }) async { - return await Future.microtask(() async { - final activePubkey = ref.read(activePubkeyProvider); - if (activePubkey == null || activePubkey.isEmpty) { - return {}; - } - final messagePubkeys = [chatMessage.pubkey]; - if (chatMessage.replyToId?.isNotEmpty ?? false) { - messagePubkeys.add(chatMessage.replyToId!); - } - final contacts = await _fetchUsersProfileData(messagePubkeys); - final domainUsersMap = _mapContactModelsToDomainUsers( - activePubkey: activePubkey, - contacts: contacts, - ); - return domainUsersMap; - }); - } + Future<Map<String, domain_user.User>> _fetchMessageUsersMap({ + required ChatMessage chatMessage, + required String activePubkey, + required Map<String, ChatMessage> chatMessagesMap, + }) async { + final pubkeys = <String>{chatMessage.pubkey}; + // reply author + if (chatMessage.isReply && chatMessage.replyToId != null) { + final original = chatMessagesMap[chatMessage.replyToId!]; + if (original != null && original.pubkey.isNotEmpty) { + pubkeys.add(original.pubkey); + } + } + // reaction authors + for (final ur in chatMessage.reactions.userReactions) { + if (ur.user.isNotEmpty) pubkeys.add(ur.user); + } + final contacts = await _fetchUsersProfileData(pubkeys.toList()); + return _mapContactModelsToDomainUsers( + activePubkey: activePubkey, + contacts: contacts, + ); + }Also applies to: 53-56
42-49: Avoid re-reading activePubkey inside _fetchGroupUsersMap and drop microtasks.Pass
activePubkeyexplicitly and removeFuture.microtask.Future<List<MessageModel>> toMessageModels({ required List<ChatMessage> chatMessages, required String activePubkey, }) async { - final usersMap = await _fetchGroupUsersMap(); + final usersMap = await _fetchGroupUsersMap(activePubkey: activePubkey);- Future<Map<String, domain_user.User>> _fetchGroupUsersMap() async { - return await Future.microtask(() async { - final activePubkey = ref.read(activePubkeyProvider); - if (activePubkey == null || activePubkey.isEmpty) { - return {}; - } - final groupContacts = await _fetchGroupUsersProfileData(activePubkey); - final domainUsersMap = _mapContactModelsToDomainUsers( - activePubkey: activePubkey, - contacts: groupContacts, - ); - return domainUsersMap; - }); - } + Future<Map<String, domain_user.User>> _fetchGroupUsersMap({ + required String activePubkey, + }) async { + if (activePubkey.isEmpty) return {}; + final groupContacts = await _fetchGroupUsersProfileData(activePubkey); + return _mapContactModelsToDomainUsers( + activePubkey: activePubkey, + contacts: groupContacts, + ); + }Also applies to: 74-87
89-100: De-duplicate pubkeys before fetching profiles.Prevents redundant requests and wasted allocations.
- Future<List<MapEntry<String, ContactModel>>> _fetchUsersProfileData( - List<String> pubkeys, - ) async { + Future<List<MapEntry<String, ContactModel>>> _fetchUsersProfileData( + List<String> pubkeys, + ) async { + pubkeys = pubkeys.toSet().toList(); final userProfileDataNotifier = ref.read(userProfileDataProvider.notifier); final userFutures = pubkeys.map( (pubkey) => userProfileDataNotifier .getUserProfileData(pubkey) .then((contact) => MapEntry(pubkey, contact)), ); final usersProfileData = await Future.wait(userFutures); return usersProfileData; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
lib/config/providers/chat_provider.dart(7 hunks)lib/config/providers/metadata_cache_provider.dart(0 hunks)lib/config/services/group_messages_service.dart(1 hunks)lib/utils/message_converter.dart(14 hunks)
💤 Files with no reviewable changes (1)
- lib/config/providers/metadata_cache_provider.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/config/services/group_messages_service.dartlib/config/providers/chat_provider.dartlib/utils/message_converter.dart
🧠 Learnings (4)
📓 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.
📚 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/config/services/group_messages_service.dartlib/utils/message_converter.dart
📚 Learning: 2025-09-14T21:22:00.962Z
Learnt from: Quwaysim
PR: parres-hq/whitenoise_flutter#634
File: lib/config/providers/group_provider.dart:1130-1132
Timestamp: 2025-09-14T21:22:00.962Z
Learning: In the whitenoise_flutter codebase, the _updateGroupInfo method in GroupsNotifier performs optimistic updates that directly modify Group objects in the provider state (groups list and groupsMap). For non-DM groups, the display name comes from group.name, so updating the Group object directly is sufficient to reflect name changes in the UI without needing to refresh the separate display name cache.
Applied to files:
lib/config/services/group_messages_service.dartlib/config/providers/chat_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/chat_provider.dart
josefinalliende
force-pushed
the
refactor/last-step-to-remove-metadata-cache
branch
5 times, most recently
from
6b3bb51 to
2d99577
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (21)
lib/utils/reaction_converter.dart (2)
5-34: Normalize pubkeys during lookup; simplify loop; extract constant.Without key normalization, npub/hex mismatches can incorrectly fall back to "Unknown User". Also, the loop can be expressed functionally and the magic string should be a constant.
@@ -import 'package:whitenoise/domain/models/user_model.dart' as domain_user; +import 'package:whitenoise/domain/models/user_model.dart' as domain_user; +import 'package:whitenoise/utils/pubkey_formatter.dart'; @@ class ReactionConverter { static List<Reaction> fromReactionSummary({ required ReactionSummary reactionSummary, required Map<String, domain_user.User> usersMap, }) { - final List<Reaction> convertedReactions = []; - - for (final userReaction in reactionSummary.userReactions) { - final user = - usersMap[userReaction.user] ?? - domain_user.User( - id: userReaction.user, - displayName: 'Unknown User', - nip05: '', - publicKey: userReaction.user, - ); - - final reaction = Reaction( - emoji: userReaction.emoji, - user: user, - createdAt: userReaction.createdAt, - ); - - convertedReactions.add(reaction); - } - - return convertedReactions; + const String unknownUserName = 'Unknown User'; + if (reactionSummary.userReactions.isEmpty) return const <Reaction>[]; + return reactionSummary.userReactions.map((userReaction) { + final String key = userReaction.user; + final String? hexKey = PubkeyFormatter(pubkey: key).toHex(); + final domain_user.User user = + usersMap[key] ?? + (hexKey != null ? usersMap[hexKey] : null) ?? + domain_user.User( + id: key, + displayName: unknownUserName, + nip05: '', + publicKey: key, + ); + return Reaction( + emoji: userReaction.emoji, + user: user, + createdAt: userReaction.createdAt, + ); + }).toList(growable: false); } }
13-21: Add explicit type annotation for localuser.Aligns with the repo rule to declare types for variables.
- final user = + final domain_user.User user = usersMap[userReaction.user] ??lib/utils/pubkey_utils.dart (1)
3-15: LGTM; small style nits.
- Remove the blank line inside the function to match the "no blank lines within a function" guideline.
- Consider a brief doc comment.
class PubkeyUtils { static bool isMe({required String myPubkey, required String otherPubkey}) { final myHexPubkey = PubkeyFormatter(pubkey: myPubkey).toHex(); final otherHexPubkey = PubkeyFormatter(pubkey: otherPubkey).toHex(); if (myHexPubkey == null || myHexPubkey.isEmpty || otherHexPubkey == null || otherHexPubkey.isEmpty) { return false; } - return myHexPubkey == otherHexPubkey; } }Optionally:
/// Returns true when both pubkeys refer to the same identity (hex-normalized).test/utils/message_converter_test.dart (1)
36-175: Comprehensive coverage; consider factoring builders.Repeated ChatMessage construction can be centralized in small helpers to keep tests focused and shorter. Also add a whitespace-only content case to ensure filtering treats ' ' as empty.
lib/config/providers/chat_provider.dart (6)
76-76: Type annotation for messages.Prefer explicit typing per repo rules.
- final messages = await ref.read(groupMessagesProvider(groupId).notifier).fetchMessages(); + final List<MessageModel> messages = + await ref.read(groupMessagesProvider(groupId).notifier).fetchMessages();
161-178: Duplicate cache building logic — extract a helper.This block is repeated below for replies. Extract a private helper to reduce duplication and mistakes.
- // Build message cache from current messages for consistency - final chatMessagesMap = <String, ChatMessage>{}; - for (final msg in currentMessages) { - // Convert existing MessageModel back to ChatMessage for cache - final chatMessage = ChatMessage( - id: msg.id, - pubkey: msg.sender.publicKey, - content: msg.content ?? '', - createdAt: msg.createdAt, - tags: const [], - isReply: msg.replyTo != null, - replyToId: msg.replyTo?.id, - isDeleted: false, - contentTokens: const [], - reactions: const ReactionSummary(byEmoji: [], userReactions: []), - kind: msg.kind, // Use the actual message kind - ); - chatMessagesMap[msg.id] = chatMessage; - } + // Build message cache from current messages for consistency + final Map<String, ChatMessage> chatMessagesMap = + _buildChatMessagesMapFromMessageModels(currentMessages);Add this helper inside ChatNotifier:
Map<String, ChatMessage> _buildChatMessagesMapFromMessageModels(List<MessageModel> models) { final map = <String, ChatMessage>{}; for (final msg in models) { map[msg.id] = ChatMessage( id: msg.id, pubkey: msg.sender.publicKey, content: msg.content ?? '', createdAt: msg.createdAt, tags: const [], isReply: msg.replyTo != null, replyToId: msg.replyTo?.id, isDeleted: false, contentTokens: const [], reactions: const ReactionSummary(byEmoji: [], userReactions: []), kind: msg.kind, ); } return map; }
180-185: Type annotation forsentMessageModel.Keep local types explicit.
- final sentMessageModel = await ref + final MessageModel sentMessageModel = await ref .read(groupMessagesProvider(groupId).notifier) .toMessageModel( chatMessage: sentChatMessage, chatMessagesMap: chatMessagesMap, );
272-272: Annotate type and use ID-based delta instead of index/length.Index-based skip assumes strict append-only and identical ordering. Switch to ID diff to be robust against backfill/reordering.
- final newMessages = await ref.read(groupMessagesProvider(groupId).notifier).fetchMessages(); + final List<MessageModel> newMessages = + await ref.read(groupMessagesProvider(groupId).notifier).fetchMessages(); @@ - if (newMessages.length > currentMessages.length) { - // Add only new messages to preserve performance - final newMessagesOnly = newMessages.skip(currentMessages.length).toList(); + if (newMessages.length > currentMessages.length) { + // Add only messages with unseen IDs + final existingIds = {for (final m in currentMessages) m.id}; + final newMessagesOnly = + newMessages.where((m) => !existingIds.contains(m.id)).toList(growable: false);
592-609: Reuse the extracted helper here as well.Same cache-build duplication as above.
- // Build message cache from current messages for reply lookup - final chatMessagesMap = <String, ChatMessage>{}; - for (final msg in currentMessages) { - // Convert existing MessageModel back to ChatMessage for cache - final chatMessage = ChatMessage( - id: msg.id, - pubkey: msg.sender.publicKey, - content: msg.content ?? '', - createdAt: msg.createdAt, - tags: const [], - isReply: msg.replyTo != null, - replyToId: msg.replyTo?.id, - isDeleted: false, - contentTokens: const [], - reactions: const ReactionSummary(byEmoji: [], userReactions: []), - kind: msg.kind, // Use the actual message kind - ); - chatMessagesMap[msg.id] = chatMessage; - } + // Build message cache from current messages for reply lookup + final Map<String, ChatMessage> chatMessagesMap = + _buildChatMessagesMapFromMessageModels(currentMessages);
611-616: Type annotation forsentMessageModel.Same nit as sendMessage().
- final sentMessageModel = await ref + final MessageModel sentMessageModel = await ref .read(groupMessagesProvider(groupId).notifier) .toMessageModel( chatMessage: sentChatMessage, chatMessagesMap: chatMessagesMap, );test/config/providers/group_messages_provider_test.dart (4)
36-52: Remove unused MockPubkeyFormatter and factory.These mocks aren’t used anywhere in this test file; drop them to reduce noise.
class MockPubkeyFormatter implements PubkeyFormatter { final String _pubkey; MockPubkeyFormatter(this._pubkey); @override String? toHex() { // Simple mock: return the pubkey as-is for comparison return _pubkey; } @override String? toNpub() => _pubkey; @override dynamic noSuchMethod(Invocation invocation) => null; } - -PubkeyFormatter Function({String? pubkey}) mockPubkeyFormatter() { - return ({String? pubkey}) => MockPubkeyFormatter(pubkey ?? ''); -}Also applies to: 69-71
84-104: Add explicit type annotations for test data.Project guidelines require explicit types in Dart files. Annotate maps and lists.
- final testContacts = { + final Map<String, ContactModel> testContacts = { 'npub1testkey12345678901234567890': ContactModel( publicKey: 'npub1testkey12345678901234567890', displayName: 'Alice', imagePath: '/path/to/alice.jpg', nip05: 'alice@example.com', ), ... }; - final mockMessages = [ + final List<ChatMessage> mockMessages = [ ChatMessage( id: 'message_1', pubkey: 'npub1testkey12345678901234567890', content: 'Hello world!', createdAt: DateTime.fromMillisecondsSinceEpoch(1234567890000), tags: [], isReply: false, isDeleted: false, contentTokens: [], reactions: const ReactionSummary(byEmoji: [], userReactions: []), kind: 9, ), ... ];Also applies to: 105-142
221-224: Type inference → explicit types in assertions.Make result types explicit per repo guidelines.
- final messages = await notifier.fetchMessages(); + final List<MessageModel> messages = await notifier.fetchMessages();Apply similarly at Lines 227, 234.
Also applies to: 227-231, 234-238
266-266: Unify group title wording.For consistency with other groups, rename to “with Carl as active pubkey”.
- group('with Carl active pubkey', () { + group('with Carl as active pubkey', () {lib/config/providers/group_messages_provider.dart (4)
113-132: Remove unnecessary Future.microtask wrapper in _fetchMessageUsersMap (simplify + avoid nested futures).Microtask + async adds complexity without benefit. Inline the logic; also dedupe pubkeys.
- Future<Map<String, domain_user.User>> _fetchMessageUsersMap({ + Future<Map<String, domain_user.User>> _fetchMessageUsersMap({ required ChatMessage chatMessage, required Map<String, ChatMessage> chatMessagesMap, }) async { - return await Future.microtask(() async { - final activePubkey = ref.read(activePubkeyProvider); - if (activePubkey == null || activePubkey.isEmpty) { - return {}; - } - final messagePubkeys = [chatMessage.pubkey]; - if (chatMessage.isReply && chatMessage.replyToId != null) { - final original = chatMessagesMap[chatMessage.replyToId!]; - if (original != null && original.pubkey.isNotEmpty) { - messagePubkeys.add(original.pubkey); - } - } - final contacts = await _fetchUsersProfileData(messagePubkeys); - final domainUsersMap = _mapContactModelsToDomainUsers( - activePubkey: activePubkey, - contacts: contacts, - ); - return domainUsersMap; - }); + final String? activePubkey = ref.read(activePubkeyProvider); + if (activePubkey == null || activePubkey.isEmpty) { + return {}; + } + final Set<String> pubkeys = <String>{chatMessage.pubkey}; + if (chatMessage.isReply && chatMessage.replyToId != null) { + final ChatMessage? original = chatMessagesMap[chatMessage.replyToId!]; + if (original != null && original.pubkey.isNotEmpty) { + pubkeys.add(original.pubkey); + } + } + final List<MapEntry<String, ContactModel>> contacts = + await _fetchUsersProfileData(pubkeys.toList()); + return _mapContactModelsToDomainUsers( + activePubkey: activePubkey, + contacts: contacts, + ); }
134-147: Same simplification for _fetchGroupUsersMap.- Future<Map<String, domain_user.User>> _fetchGroupUsersMap() async { - return await Future.microtask(() async { - final activePubkey = ref.read(activePubkeyProvider); - if (activePubkey == null || activePubkey.isEmpty) { - return {}; - } - final groupContacts = await _fetchGroupUsersProfileData(activePubkey); - final domainUsersMap = _mapContactModelsToDomainUsers( - activePubkey: activePubkey, - contacts: groupContacts, - ); - return domainUsersMap; - }); - } + Future<Map<String, domain_user.User>> _fetchGroupUsersMap() async { + final String? activePubkey = ref.read(activePubkeyProvider); + if (activePubkey == null || activePubkey.isEmpty) { + return {}; + } + final List<MapEntry<String, ContactModel>> groupContacts = + await _fetchGroupUsersProfileData(activePubkey); + return _mapContactModelsToDomainUsers( + activePubkey: activePubkey, + contacts: groupContacts, + ); + }
149-160: Deduplicate pubkeys before fetching profiles.Prevents redundant lookups when the same pubkey appears multiple times.
- Future<List<MapEntry<String, ContactModel>>> _fetchUsersProfileData( - List<String> pubkeys, - ) async { + Future<List<MapEntry<String, ContactModel>>> _fetchUsersProfileData( + List<String> pubkeys, + ) async { final userProfileDataNotifier = ref.read(userProfileDataProvider.notifier); - final userFutures = pubkeys.map( + final List<String> uniquePubkeys = pubkeys.toSet().toList(); + final Iterable<Future<MapEntry<String, ContactModel>>> userFutures = uniquePubkeys.map( (pubkey) => userProfileDataNotifier .getUserProfileData(pubkey) .then((contact) => MapEntry(pubkey, contact)), ); final usersProfileData = await Future.wait(userFutures); return usersProfileData; }
187-196: Avoid magic strings for display names (“You”).Extract to a constant to standardize across modules and ease i18n later.
- if (_isMe(myPubkey: activePubkey, otherPubkey: contact.publicKey)) { - return 'You'; + if (_isMe(myPubkey: activePubkey, otherPubkey: contact.publicKey)) { + return kYouDisplayName;Add near the top of this file:
+const String kYouDisplayName = 'You';lib/utils/message_converter.dart (3)
60-66: Make fromChatMessageList synchronous (it’s purely sync).Removes unnecessary Future/async overhead and simplifies callers.
- static Future<List<MessageModel>> fromChatMessageList( + static List<MessageModel> fromChatMessageList( List<ChatMessage> chatMessages, { required String currentUserPublicKey, required String groupId, required Map<String, domain_user.User> usersMap, bool Function({required String myPubkey, required String otherPubkey})? isMeFn, - }) async { + }) { // Filter valid messages first - final validMessages = + final List<ChatMessage> validMessages = chatMessages.where((msg) => !msg.isDeleted && msg.content.isNotEmpty).toList(); // Build messages map for reply lookups - final chatMessagesMap = <String, ChatMessage>{}; + final Map<String, ChatMessage> chatMessagesMap = <String, ChatMessage>{}; for (final msg in validMessages) { chatMessagesMap[msg.id] = msg; } - final messages = + final List<MessageModel> messages = validMessages .map( (messageData) => fromChatMessage( messageData, currentUserPublicKey: currentUserPublicKey, groupId: groupId, chatMessagesMap: chatMessagesMap, usersMap: usersMap, isMeFn: isMeFn, ), ) .toList(); return messages; }Update caller in lib/config/providers/group_messages_provider.dart Lines 100‑106:
- final messages = MessageConverter.fromChatMessageList( + final List<MessageModel> messages = MessageConverter.fromChatMessageList( chatMessages, currentUserPublicKey: activePubkey, groupId: state.groupId, usersMap: usersMap, ); - return messages; + return messages;Also applies to: 77-92
127-141: Deterministic fallback timestamp for “message not found”.Using DateTime.now() makes tests flaky. Prefer a fixed epoch.
- createdAt: DateTime.now(), + createdAt: DateTime.fromMillisecondsSinceEpoch(0),
146-153: Avoid magic string: “Unknown User”.Extract a constant for reuse and future i18n.
- static domain_user.User _unknownUser({required String pubkey}) { - return domain_user.User( - id: pubkey, - displayName: 'Unknown User', + static const String kUnknownUserDisplayName = 'Unknown User'; + + static domain_user.User _unknownUser({required String pubkey}) { + return domain_user.User( + id: pubkey, + displayName: kUnknownUserDisplayName, nip05: '', publicKey: pubkey, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)lib/config/providers/chat_provider.dart(7 hunks)lib/config/providers/group_messages_provider.dart(1 hunks)lib/config/providers/metadata_cache_provider.dart(0 hunks)lib/utils/message_converter.dart(2 hunks)lib/utils/pubkey_utils.dart(1 hunks)lib/utils/reaction_converter.dart(1 hunks)test/config/providers/group_messages_provider_test.dart(1 hunks)test/utils/message_converter_test.dart(1 hunks)test/utils/reaction_converter_test.dart(1 hunks)
💤 Files with no reviewable changes (1)
- lib/config/providers/metadata_cache_provider.dart
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/utils/pubkey_utils.darttest/utils/message_converter_test.darttest/config/providers/group_messages_provider_test.darttest/utils/reaction_converter_test.dartlib/utils/reaction_converter.dartlib/config/providers/group_messages_provider.dartlib/config/providers/chat_provider.dartlib/utils/message_converter.dart
**/*_test.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*_test.dart: Follow the Arrange-Act-Assert convention for tests.
Name test variables clearly. Follow the convention: inputX, mockX, actualX, expectedX, etc.
Write unit tests for each public function. Use test doubles to simulate dependencies, except for third-party dependencies that are not expensive to execute.
Write acceptance tests for each module. Follow the Given-When-Then convention.
Use the standard widget testing for Flutter.
Files:
test/utils/message_converter_test.darttest/config/providers/group_messages_provider_test.darttest/utils/reaction_converter_test.dart
🧠 Learnings (10)
📓 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.
📚 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/utils/pubkey_utils.dartlib/utils/message_converter.dart
📚 Learning: 2025-09-04T17:54:09.162Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#594
File: lib/ui/chat/chat_info/dm_chat_info.dart:0-0
Timestamp: 2025-09-04T17:54:09.162Z
Learning: In the whitenoise_flutter codebase, pubkey conversion from hex to npub format is handled using a .toNpub() extension method on String, which eliminates the need for manual format checking or calling npubFromHexPubkey() function directly.
Applied to files:
lib/utils/pubkey_utils.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/utils/pubkey_utils.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/utils/pubkey_utils.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 : Write unit tests for each public function. Use test doubles to simulate dependencies, except for third-party dependencies that are not expensive to execute.
Applied to files:
test/utils/message_converter_test.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:
test/config/providers/group_messages_provider_test.dart
📚 Learning: 2025-09-14T21:22:00.962Z
Learnt from: Quwaysim
PR: parres-hq/whitenoise_flutter#634
File: lib/config/providers/group_provider.dart:1130-1132
Timestamp: 2025-09-14T21:22:00.962Z
Learning: In the whitenoise_flutter codebase, the _updateGroupInfo method in GroupsNotifier performs optimistic updates that directly modify Group objects in the provider state (groups list and groupsMap). For non-DM groups, the display name comes from group.name, so updating the Group object directly is sufficient to reflect name changes in the UI without needing to refresh the separate display name cache.
Applied to files:
lib/config/providers/group_messages_provider.dartlib/config/providers/chat_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/config/providers/group_messages_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/utils/message_converter.dart
🔇 Additional comments (4)
test/utils/reaction_converter_test.dart (1)
1-124: Tests are solid — add a format‑mismatch case (hex ↔ npub)Add a test where usersMap keys are hex while ReactionSummary.user values are bech32 (npub...) and the inverse to prevent known users being treated as "Unknown User" if formats differ.
Check usersMap construction at lib/config/providers/group_messages_provider.dart (_fetchGroupUsersMap, _mapContactModelsToDomainUsers) and the converter at lib/utils/reaction_converter.dart (fromReactionSummary); also review usages in lib/utils/message_converter.dart.
test/config/providers/group_messages_provider_test.dart (1)
149-165: Good test harness (overrides + teardown).ProviderContainer overrides are clean, and teardown disposes correctly.
Also applies to: 167-169
lib/config/providers/group_messages_provider.dart (1)
62-74: LGTM: early return on missing activePubkey and stable ascending sort.The flow is clear and side‑effect free.
lib/utils/message_converter.dart (1)
9-32: Solid, side‑effect‑free conversion path.Clean separation of concerns; reply handling and reaction mapping are straightforward.
Also applies to: 45-58
josefinalliende
force-pushed
the
refactor/last-step-to-remove-metadata-cache
branch
from
2d99577 to
f1ee229
Compare
josefinalliende
force-pushed
the
refactor/last-step-to-remove-metadata-cache
branch
from
f1ee229 to
9ef3e09
Compare
josefinalliende
force-pushed
the
refactor/last-step-to-remove-metadata-cache
branch
2 times, most recently
from
1317a0d to
1504a15
Compare
|
This PR is already too big, so fix for #644 will not be here :) |
josefinalliende
force-pushed
the
refactor/last-step-to-remove-metadata-cache
branch
from
44bd366 to
9b8a376
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/config/providers/chat_provider.dart (1)
266-321: Fragile positional diffing; prefer ID‑based merge and preserve optimistic “sending.”Comparing by index and using
skip(currentMessages.length)assumes identical ordering and no gaps. Use ID sets to append truly new messages and detect edits/reactions, then re‑append recent “sending” messages from MessageFilterService.Illustrative replacement:
- if (newMessages.length != currentMessages.length) { - // New or deleted messages - hasChanges = true; - } else if (newMessages.isNotEmpty && currentMessages.isNotEmpty) { - // Check if any message content or reactions have changed - for (int i = 0; i < newMessages.length; i++) { - final newMsg = newMessages[i]; - final currentMsg = currentMessages[i]; - if (newMsg.content != currentMsg.content || - newMsg.reactions.length != currentMsg.reactions.length || - !_areReactionsEqual(newMsg.reactions, currentMsg.reactions)) { - hasChanges = true; - break; - } - } - } + final currentById = {for (final m in currentMessages) m.id: m}; + final newById = {for (final m in newMessages) m.id: m}; + final currentIds = currentById.keys.toSet(); + final newIds = newById.keys.toSet(); + + final addedIds = newIds.difference(currentIds); + final removedIds = currentIds.difference(newIds); + final commonIds = newIds.intersection(currentIds); + + final bool contentChanged = commonIds.any((id) { + final a = newById[id]!, b = currentById[id]!; + return a.content != b.content || + a.reactions.length != b.reactions.length || + !_areReactionsEqual(a.reactions, b.reactions); + }); + + final bool hasChanges = addedIds.isNotEmpty || removedIds.isNotEmpty || contentChanged; @@ - if (hasChanges) { - if (newMessages.length > currentMessages.length) { - // Add only new messages to preserve performance - final newMessagesOnly = newMessages.skip(currentMessages.length).toList(); - state = state.copyWith( - groupMessages: { - ...state.groupMessages, - groupId: [...currentMessages, ...newMessagesOnly], - }, - ); - _logger.info( - 'ChatProvider: Added ${newMessagesOnly.length} new messages for group $groupId', - ); - } else { - // Replace all messages when there are content changes (reactions, edits, etc.) - // But keep recent messages in sending state if they don't match the last server message - final messagesToKeep = MessageFilterService.getRecentSendingMessages( - currentMessages: currentMessages, - newMessages: newMessages, - ); - state = state.copyWith( - groupMessages: { - ...state.groupMessages, - groupId: [...newMessages, ...messagesToKeep], - }, - ); - _logger.info( - 'ChatProvider: Updated messages with content changes for group $groupId', - ); - } - // Update group order when messages are updated - _updateGroupOrderForNewMessage(groupId); - } + if (hasChanges) { + final List<MessageModel> messagesToKeep = MessageFilterService.getRecentSendingMessages( + currentMessages: currentMessages, + newMessages: newMessages, + ); + final List<MessageModel> merged = [ + ...newMessages, + ...messagesToKeep, + ]; + state = state.copyWith( + groupMessages: { + ...state.groupMessages, + groupId: merged, + }, + ); + _logger.info('ChatProvider: Merged ${addedIds.length} added, ' + '${removedIds.length} removed, contentChanged=$contentChanged for group $groupId'); + await _updateGroupOrderForNewMessage(groupId); + }This aligns with the recent “keep recent sending messages” approach. Based on learnings.
🧹 Nitpick comments (9)
CHANGELOG.md (2)
14-14: Changelog entry now correctly under Unreleased.Looks good. Consider cross‑linking the underlying issue for traceability: append a link to issue #541 since this PR “completes the fix.”
Example:
- - Removes metadata cache provider [#654](https://github.com/parres-hq/whitenoise_flutter/pull/654) + - Removes metadata cache provider [#654](https://github.com/parres-hq/whitenoise_flutter/pull/654) ([#541](https://github.com/parres-hq/whitenoise_flutter/issues/541))
21-21: Optional: reference the user‑visible bug it addresses.Add a link to issue #612 for “double rendering” to help readers connect the fix.
- - Fixed double rendering issue for some messages [#654](https://github.com/parres-hq/whitenoise_flutter/pull/654) + - Fixed double rendering issue for some messages [#654](https://github.com/parres-hq/whitenoise_flutter/pull/654) ([#612](https://github.com/parres-hq/whitenoise_flutter/issues/612))test/utils/message_converter_test.dart (1)
13-35: Declare explicit types for test constants and fixtures (per guidelines).Our Dart guidelines require explicit types for variables. Please annotate these with String/User/Map types.
Apply pattern like:
- final currentUserPublicKey = 'npub1zg69v7ys40x77y352eufp27daufrg4ncjz4ummcjx3t83y9tehhsqepuh0'; - final otherUserPublicKey = 'npub140x77y352eufp27daufrg4ncjz4ummcjx3t83y9tehh3ydzk0zgqwxq8j0'; - final groupId = 'group_123'; + final String currentUserPublicKey = 'npub1zg69v7ys40x77y352eufp27daufrg4ncjz4ummcjx3t83y9tehhsqepuh0'; + final String otherUserPublicKey = 'npub140x77y352eufp27daufrg4ncjz4ummcjx3t83y9tehh3ydzk0zgqwxq8j0'; + final String groupId = 'group_123'; @@ - final currentUser = domain_user.User( + final domain_user.User currentUser = domain_user.User( @@ - final otherUser = domain_user.User( + final domain_user.User otherUser = domain_user.User( @@ - final usersMap = <String, domain_user.User>{ + final Map<String, domain_user.User> usersMap = <String, domain_user.User>{Please propagate the same change to other locals (e.g., ChatMessage, List) throughout this file.
As per coding guidelines.lib/config/providers/chat_provider.dart (5)
392-403: Avoid mutating parameters with??=; use a local derived value.
final gId = groupId ??= state.selectedGroupId;mutates the parameter and may violate lints (parameter_assignments). Use a pure expression.- final gId = groupId ??= state.selectedGroupId; + final String? gId = groupId ?? state.selectedGroupId;
405-416: Same parameter-mutation issue in isNextSameSender.Mirror the fix to avoid
??=on the parameter.- final gId = groupId ??= state.selectedGroupId; + final String? gId = groupId ?? state.selectedGroupId;
106-109: Avoid magic number for kind=9.Extract a named constant (e.g., kNostrKindText = 9) or use an enum from the API if available, and reuse in send/reply.
- int kind = 9, // Default to text message + int kind = kNostrKindText, // Default to text messageand declare:
const int kNostrKindText = 9;
327-329: Await group order update to avoid dropped errors.checkForNewMessages doesn’t await the async update; prefer awaiting to ensure ordering and error surfacing.
- _updateGroupOrderForNewMessage(groupId); + await _updateGroupOrderForNewMessage(groupId);
586-597: Include NIP-10 reply tags
Per NIP-10, reply events should use:
- an e-tag with
[eventId, relayURL, "reply", authorPubkey]- a p-tag for the replied-to event’s author
Fallback to a minimal e-tag whenreplyToMessageis null.// Create tags for reply per NIP-10 final List<Tag> replyTags = [ if (replyToMessage != null) ...[ await tagFromVec(vec: [ 'e', replyToMessage.id, '', // relay URL (empty if unknown) 'reply', // marker replyToMessage.sender.publicKey, ]), await tagFromVec(vec: ['p', replyToMessage.sender.publicKey]), ] else await tagFromVec(vec: ['e', replyToMessageId]), ];k-tags aren’t defined by NIP-10/NIP-18 and should be omitted.
lib/config/providers/group_messages_provider.dart (1)
90-103: Use Future.value() or remove unnecessary Future.microtask wrapper.The
Future.microtask(() async { ... })pattern is unusual here. Since the entire body is already async and you're returning aFuture<Map<...>>, you can simplify this to just execute the async block directly.Apply this diff:
- Future<Map<String, domain_user.User>> _fetchGroupUsersMap() async { - return await Future.microtask(() async { - final activePubkey = ref.read(activePubkeyProvider); - if (activePubkey == null || activePubkey.isEmpty) { - return {}; - } - final groupContacts = await _fetchGroupUsersProfileData(activePubkey); - final domainUsersMap = _mapContactModelsToDomainUsers( - activePubkey: activePubkey, - contacts: groupContacts, - ); - return domainUsersMap; - }); + Future<Map<String, domain_user.User>> _fetchGroupUsersMap() async { + final activePubkey = ref.read(activePubkeyProvider); + if (activePubkey == null || activePubkey.isEmpty) { + return {}; + } + final groupContacts = await _fetchGroupUsersProfileData(activePubkey); + final domainUsersMap = _mapContactModelsToDomainUsers( + activePubkey: activePubkey, + contacts: groupContacts, + ); + return domainUsersMap; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(1 hunks)lib/config/providers/chat_provider.dart(9 hunks)lib/config/providers/group_messages_provider.dart(1 hunks)lib/config/providers/metadata_cache_provider.dart(0 hunks)lib/domain/services/message_filter_service.dart(1 hunks)lib/ui/chat/utils/message_extensions.dart(0 hunks)lib/utils/message_converter.dart(2 hunks)lib/utils/pubkey_utils.dart(1 hunks)lib/utils/reaction_converter.dart(1 hunks)test/config/providers/group_messages_provider_test.dart(1 hunks)test/domain/services/message_filter_service_test.dart(1 hunks)test/utils/message_converter_test.dart(1 hunks)test/utils/reaction_converter_test.dart(1 hunks)
💤 Files with no reviewable changes (2)
- lib/config/providers/metadata_cache_provider.dart
- lib/ui/chat/utils/message_extensions.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- test/utils/reaction_converter_test.dart
- lib/utils/reaction_converter.dart
- lib/domain/services/message_filter_service.dart
- lib/utils/pubkey_utils.dart
- test/domain/services/message_filter_service_test.dart
🧰 Additional context used
📓 Path-based instructions (3)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using '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:
test/utils/message_converter_test.dartlib/utils/message_converter.darttest/config/providers/group_messages_provider_test.dartlib/config/providers/chat_provider.dartlib/config/providers/group_messages_provider.dart
**/*_test.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*_test.dart: Follow the Arrange-Act-Assert convention for tests.
Name test variables clearly. Follow the convention: inputX, mockX, actualX, expectedX, etc.
Write unit tests for each public function. Use test doubles to simulate dependencies, except for third-party dependencies that are not expensive to execute.
Write acceptance tests for each module. Follow the Given-When-Then convention.
Use the standard widget testing for Flutter.
Files:
test/utils/message_converter_test.darttest/config/providers/group_messages_provider_test.dart
**/*.md
📄 CodeRabbit inference engine (.cursor/rules/whitenoise.mdc)
**/*.md: NIPs (Nostr Implementation Possibilities) are numbered likeNIP-XXwhereXXare two capitalized hexadecimal digits, e.g.,NIP-01andNIP-C7.
To read a specific NIP, construct the NIP URL following this template:https://raw.githubusercontent.com/nostr-protocol/nips/refs/heads/master/{nip}.md(replace{nip}in the URL template with the relevant NIP name, e.g.,07for NIP-07, orC7for NIP-C7).
To read the definition of a specific kind, construct a URL following this template:https://nostrbook.dev/kinds/{kind}.md(replace{kind}in the template with the kind number, e.g.,https://nostrbook.dev/kinds/0.mdfor kind 0).
Files:
CHANGELOG.md
🧠 Learnings (3)
📚 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/chat_provider.dart
📚 Learning: 2025-09-29T20:12:50.785Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#654
File: lib/domain/services/message_filter_service.dart:0-0
Timestamp: 2025-09-29T20:12:50.785Z
Learning: In the MessageFilterService.getRecentSendingMessages method in lib/domain/services/message_filter_service.dart, the implemented solution checks the last 10 messages within a 1-minute timeframe when comparing optimistic "sending" messages against server confirmations to prevent double-rendering. This balanced approach avoids performance issues while handling typical server response times (usually < 2 seconds) with a generous buffer.
Applied to files:
lib/config/providers/chat_provider.dart
📚 Learning: 2025-09-14T21:22:00.962Z
Learnt from: Quwaysim
PR: parres-hq/whitenoise_flutter#634
File: lib/config/providers/group_provider.dart:1130-1132
Timestamp: 2025-09-14T21:22:00.962Z
Learning: In the whitenoise_flutter codebase, the _updateGroupInfo method in GroupsNotifier performs optimistic updates that directly modify Group objects in the provider state (groups list and groupsMap). For non-DM groups, the display name comes from group.name, so updating the Group object directly is sufficient to reflect name changes in the UI without needing to refresh the separate display name cache.
Applied to files:
lib/config/providers/group_messages_provider.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 (31)
test/config/providers/group_messages_provider_test.dart (12)
10-17: LGTM - Mock follows constructor injection pattern.The mock correctly inherits from
ActivePubkeyNotifierand uses constructor parameters to inject test behavior.
19-33: LGTM - Mock provides appropriate test data.The
MockUserProfileDataNotifiercorrectly simulates the profile data provider with a fallback for unknown users.
35-51: LGTM - Mock implements required interface.The
MockPubkeyFormatterprovides necessary methods for test scenarios.
53-76: LGTM - Helper factories simplify test setup.These factory functions effectively create mock implementations for dependency injection in tests.
143-164: LGTM - Test container factory provides flexible setup.The
createContainerhelper allows easy customization of test scenarios with sensible defaults.
170-178: LGTM - Verifies state initialization.Confirms the provider correctly stores the groupId in its state.
182-189: LGTM - Tests null pubkey edge case.Validates that the provider returns an empty list when no active user is present.
193-201: LGTM - Tests empty pubkey edge case.Validates that the provider returns an empty list for empty pubkey strings.
220-230: LGTM - Verifies message count and ordering.Tests confirm correct message retrieval and chronological ordering by
createdAt.
232-237: LGTM - Validates "You" label for active user.Confirms that messages from the active user display "You" while others show their actual names.
257-262: LGTM - Tests display name mapping for different active users.Validates the "You" labeling changes correctly based on the active user context.
282-287: LGTM - Completes display name test coverage.Tests the third user scenario to ensure consistent "You" labeling behavior.
lib/config/providers/group_messages_provider.dart (11)
13-19: LGTM - Simple state with required groupId.The state class correctly encapsulates the groupId parameter.
21-53: LGTM - Constructor enables dependency injection for testing.The notifier correctly implements constructor injection with sensible defaults, allowing test doubles to be provided.
55-60: LGTM - Initializes state with groupId.The build method correctly creates the initial state.
62-74: LGTM - Fetches and converts messages correctly.The method guards against null/empty pubkeys, fetches messages, sorts chronologically, and delegates conversion appropriately.
76-88: LGTM - Delegates to MessageConverter with enriched user data.The conversion method correctly fetches the users map and passes it to the converter.
105-116: LGTM - Concurrent profile fetches improve performance.Using
Future.waitto fetch multiple user profiles concurrently is an efficient approach.
118-127: LGTM - Fetches group member profiles.Correctly retrieves member pubkeys and delegates to profile fetching.
129-141: LGTM - Maps contacts to domain users.The transformation from
MapEntry<String, ContactModel>toMap<String, domain_user.User>is clean and functional.
143-152: LGTM - Implements "You" labeling for active user.The display name logic correctly distinguishes the active user from others.
154-165: LGTM - Converts ContactModel to domain User.The conversion correctly maps all necessary fields from contact to domain user.
168-171: LGTM - Declares family provider correctly.The provider export follows the Riverpod family pattern appropriately.
lib/utils/message_converter.dart (8)
9-57: LGTM - Synchronous conversion with efficient user lookup.The refactored
fromChatMessagecorrectly:
- Uses the provided
usersMapfor sender lookup (addressing past review feedback)- Falls back to
_unknownUserfor missing entries- Handles reactions conditionally via
ReactionConverter- Creates reply placeholders when needed
59-99: LGTM - Two-pass approach resolves out-of-order replies.The refactored
fromChatMessageListcorrectly:
- Builds
chatMessagesMapfor reply lookups (addressing past review feedback about out-of-order messages)- Creates all MessageModels first, then enriches with reply references
- Uses separate maps to track messages and reply relationships
This approach successfully handles the Nostr scenario where reply timestamps may precede parent timestamps.
101-133: LGTM - Creates optimistic UI updates for sending state.The
createOptimisticMessagemethod correctly generates temporary messages with "You" as display name andsendingstatus.
135-142: LGTM - Provides fallback for missing user data.The helper creates a reasonable placeholder for unknown users.
144-163: LGTM - Iterates messages to add reply references.The helper correctly maps over messages and delegates to
_addReply.
165-190: LGTM - Conditionally adds reply to message.The method correctly checks if a message is a reply and updates it with the reply reference via
_findReply.
192-208: LGTM - Looks up reply from messageModelsMap.The helper correctly retrieves the reply message from the map built during the first pass, falling back to a placeholder when not found. This addresses the out-of-order reply scenario discussed in past reviews.
210-229: LGTM - Builds placeholder for missing replies.The helper creates a sensible placeholder message when the parent message cannot be found.
josefinalliende
force-pushed
the
refactor/last-step-to-remove-metadata-cache
branch
from
9b8a376 to
a19b091
Compare
josefinalliende
force-pushed
the
refactor/last-step-to-remove-metadata-cache
branch
from
a19b091 to
7d92120
Compare
This is fixed, now replies work as normal |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/config/providers/group_messages_provider.dart (1)
72-73: Consider performance impact of in-place sort on large message lists.The code sorts
chatMessagesin-place before conversion. For groups with thousands of messages, this could be noticeable. Consider whether the Rust API can return pre-sorted messages or if sorting should happen at the database level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)lib/config/providers/chat_provider.dart(9 hunks)lib/config/providers/group_messages_provider.dart(1 hunks)lib/config/providers/metadata_cache_provider.dart(0 hunks)lib/domain/services/message_filter_service.dart(1 hunks)lib/ui/chat/utils/message_extensions.dart(0 hunks)test/config/providers/group_messages_provider_test.dart(1 hunks)test/domain/services/message_filter_service_test.dart(1 hunks)
💤 Files with no reviewable changes (2)
- lib/ui/chat/utils/message_extensions.dart
- lib/config/providers/metadata_cache_provider.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/domain/services/message_filter_service.dart
🧰 Additional context used
📓 Path-based instructions (3)
**/*.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using '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:
test/domain/services/message_filter_service_test.darttest/config/providers/group_messages_provider_test.dartlib/config/providers/group_messages_provider.dartlib/config/providers/chat_provider.dart
**/*_test.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*_test.dart: Follow the Arrange-Act-Assert convention for tests.
Name test variables clearly. Follow the convention: inputX, mockX, actualX, expectedX, etc.
Write unit tests for each public function. Use test doubles to simulate dependencies, except for third-party dependencies that are not expensive to execute.
Write acceptance tests for each module. Follow the Given-When-Then convention.
Use the standard widget testing for Flutter.
Files:
test/domain/services/message_filter_service_test.darttest/config/providers/group_messages_provider_test.dart
**/*.md
📄 CodeRabbit inference engine (.cursor/rules/whitenoise.mdc)
**/*.md: NIPs (Nostr Implementation Possibilities) are numbered likeNIP-XXwhereXXare two capitalized hexadecimal digits, e.g.,NIP-01andNIP-C7.
To read a specific NIP, construct the NIP URL following this template:https://raw.githubusercontent.com/nostr-protocol/nips/refs/heads/master/{nip}.md(replace{nip}in the URL template with the relevant NIP name, e.g.,07for NIP-07, orC7for NIP-C7).
To read the definition of a specific kind, construct a URL following this template:https://nostrbook.dev/kinds/{kind}.md(replace{kind}in the template with the kind number, e.g.,https://nostrbook.dev/kinds/0.mdfor kind 0).
Files:
CHANGELOG.md
🧠 Learnings (3)
📚 Learning: 2025-09-29T20:12:50.785Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#654
File: lib/domain/services/message_filter_service.dart:0-0
Timestamp: 2025-09-29T20:12:50.785Z
Learning: In the MessageFilterService.getRecentSendingMessages method in lib/domain/services/message_filter_service.dart, the implemented solution checks the last 10 messages within a 1-minute timeframe when comparing optimistic "sending" messages against server confirmations to prevent double-rendering. This balanced approach avoids performance issues while handling typical server response times (usually < 2 seconds) with a generous buffer.
Applied to files:
test/domain/services/message_filter_service_test.dartlib/config/providers/chat_provider.dart
📚 Learning: 2025-09-14T21:22:00.962Z
Learnt from: Quwaysim
PR: parres-hq/whitenoise_flutter#634
File: lib/config/providers/group_provider.dart:1130-1132
Timestamp: 2025-09-14T21:22:00.962Z
Learning: In the whitenoise_flutter codebase, the _updateGroupInfo method in GroupsNotifier performs optimistic updates that directly modify Group objects in the provider state (groups list and groupsMap). For non-DM groups, the display name comes from group.name, so updating the Group object directly is sufficient to reflect name changes in the UI without needing to refresh the separate display name cache.
Applied to files:
lib/config/providers/group_messages_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:
CHANGELOG.md
⏰ 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 (26)
lib/config/providers/chat_provider.dart (13)
3-3: LGTM - Import added for collection package.The
collectionimport is needed forfirstWhereOrNullused in the reply handling fix.
9-9: LGTM - Import added for group messages provider.This import supports the provider-driven message fetching approach that reduces responsibilities in chat_provider.
13-13: LGTM - Import added for message filter service.This import is needed for the
getRecentSendingMessagesmethod that prevents double-rendering of optimistic messages.
79-79: LGTM - Proper delegation to group messages provider.The refactor correctly delegates message fetching to
groupMessagesProvider, reducing duplication and centralizing message conversion logic.
122-146: LGTM - Clean optimistic message creation and state update.The optimistic message flow is well-structured:
- Creates optimistic message immediately
- Appends to current messages
- Updates state with sending flag
This provides responsive UX while the server request is in flight.
160-170: LGTM - Proper optimistic message update on success.The success path correctly updates the optimistic message with the server-assigned id, timestamp, and sent status.
197-217: LGTM - Proper error handling for optimistic messages.The error path correctly marks the optimistic message as failed and preserves it in the list, allowing the UI to show the failure state.
310-319: LGTM - Proper integration with MessageFilterService.The code correctly uses
MessageFilterService.getRecentSendingMessagesto merge recent optimistic messages with new server messages, preventing double-rendering while preserving unsent messages.Based on learnings.
551-558: LGTM - Reply crash fix applied correctly.The fix uses
firstWhereOrNullinstead offirstWhere, preventing crashes when the reply target isn't in memory. This addresses the serious issue noted in the PR objectives.
560-581: LGTM - Optimistic reply message handling.The optimistic reply flow mirrors the regular send flow, creating an immediate UI response with the optional
replyToMessagereference.
600-620: LGTM - Proper success handling for reply messages.The success path correctly updates the optimistic reply message with server-assigned values and clears the sending state.
635-653: LGTM - Proper error handling for reply messages.The error path correctly marks the optimistic reply as failed and preserves it in the message list.
116-120: No dangling optimistic state on early exit. The early-return guard runs before any optimistic message is created, and_setGroupErrorresetssendingStatestofalse, so there’s no leftover optimistic state to clear.Likely an incorrect or invalid review comment.
lib/config/providers/group_messages_provider.dart (13)
1-12: LGTM - Proper imports and aliasing.The imports are well-organized, and the aliasing of
domain_userandgroups_apiprevents naming conflicts while maintaining clarity.
13-19: LGTM - Clean state class.The
GroupMessagesStateclass is minimal and focused, containing only the requiredgroupId.
21-54: LGTM - Excellent testability through dependency injection.The constructor accepts optional function overrides for
fetchAggregatedMessagesForGroupFn,groupMembersFn, andisMeFn, enabling easy mocking in tests while defaulting to production implementations.
55-60: LGTM - Simple build initialization.The
buildmethod correctly initializes the state with the providedgroupId.
76-88: LGTM - Clean message conversion flow.The
_toMessageModelsmethod properly fetches the users map and delegates toMessageConverter.fromChatMessageListfor conversion.
90-101: LGTM - Proper user map construction.The method fetches group member profiles and maps them to domain user objects with proper error handling for missing active pubkey.
103-114: LGTM - Efficient concurrent profile fetching.The method correctly uses
Future.waitto fetch multiple user profiles concurrently, improving performance for groups with many members.
116-125: LGTM - Clean composition of profile fetching.The method properly composes
_groupMembersand_fetchUsersProfileDatato fetch profiles for all group members.
127-139: LGTM - Proper mapping from contacts to domain users.The method correctly transforms the list of contact entries into a map of domain user objects.
141-150: LGTM - Clean display name logic.The method properly uses
_isMe(viaPubkeyUtils.isMe) to determine whether to show "You" or the contact's display name.
152-163: LGTM - Clean domain user construction.The method properly constructs a
domain_user.Userfrom aContactModelwith appropriate field mappings and null coalescing fornip05.
166-169: LGTM - Proper family provider declaration.The provider is correctly declared as a family provider that creates a new
GroupMessagesNotifierfor each uniquegroupId.
62-74: Verify concurrency of fetchMessages
Ensure that concurrent invocations offetchMessages(e.g. rapid or overlapping refreshes) won’t trigger duplicate_fetchAggregatedMessagesForGroupcalls or lead to out-of-order results under race conditions.
josefinalliende
force-pushed
the
refactor/last-step-to-remove-metadata-cache
branch
from
7d92120 to
8bfc674
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/config/providers/chat_provider.dart (1)
79-90: Do not drop local optimistic bubbles on load
Line [81] writes the raw DB list back into state, so any in-flight or failed message you were showing disappears the moment a refresh completes (and the send success handler can no longer locate the optimistic entry by id). Please merge the freshly fetched records with the current state viaMessageMergerService.mergebefore updatinggroupMessages.- final messages = await ref.read(groupMessagesProvider(groupId).notifier).fetchMessages(); - - state = state.copyWith( + final messages = await ref.read(groupMessagesProvider(groupId).notifier).fetchMessages(); + final currentStateMessages = state.groupMessages[groupId] ?? <MessageModel>[]; + final mergedMessages = MessageMergerService.merge( + stateMessages: currentStateMessages, + dbMessages: messages, + ); + + state = state.copyWith( groupMessages: { ...state.groupMessages, - groupId: messages, + groupId: mergedMessages, },That keeps optimistic UI consistent across reloads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)lib/config/providers/chat_provider.dart(9 hunks)lib/config/providers/metadata_cache_provider.dart(0 hunks)lib/domain/services/message_merger_service.dart(1 hunks)lib/ui/chat/utils/message_extensions.dart(0 hunks)test/domain/services/message_merger_service_test.dart(1 hunks)
💤 Files with no reviewable changes (2)
- lib/config/providers/metadata_cache_provider.dart
- lib/ui/chat/utils/message_extensions.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/services/message_merger_service.dartlib/config/providers/chat_provider.darttest/domain/services/message_merger_service_test.dart
**/*_test.dart
📄 CodeRabbit inference engine (.cursor/rules/flutter.mdc)
**/*_test.dart: Follow the Arrange-Act-Assert convention for tests.
Name test variables clearly. Follow the convention: inputX, mockX, actualX, expectedX, etc.
Write unit tests for each public function. Use test doubles to simulate dependencies, except for third-party dependencies that are not expensive to execute.
Write acceptance tests for each module. Follow the Given-When-Then convention.
Use the standard widget testing for Flutter.
Files:
test/domain/services/message_merger_service_test.dart
🧠 Learnings (1)
📚 Learning: 2025-09-29T20:12:50.785Z
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#654
File: lib/domain/services/message_filter_service.dart:0-0
Timestamp: 2025-09-29T20:12:50.785Z
Learning: In the MessageFilterService.getRecentSendingMessages method in lib/domain/services/message_filter_service.dart, the implemented solution checks the last 10 messages within a 1-minute timeframe when comparing optimistic "sending" messages against server confirmations to prevent double-rendering. This balanced approach avoids performance issues while handling typical server response times (usually < 2 seconds) with a generous buffer.
Applied to files:
lib/domain/services/message_merger_service.dart
🔇 Additional comments (1)
test/domain/services/message_merger_service_test.dart (1)
38-206: Add a regression for failed optimistic messages
Please add a test case wherestateMessagescontains aMessageStatus.failedentry whiledbMessageslacks it. After fixing the merger, the failed bubble should still be present in the result, ensuring we don’t regress again.
josefinalliende
requested review from
Quwaysim,
codeswot,
erskingardner and
untreu2
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
@josefinalliende I won't have time today to review this super carefully. I have had a quick look through everything though and didn't notice anything glaring. I trust you've checked it super thoroughly. I say 🚢 when you're ready.
josefinalliende
deleted the
refactor/last-step-to-remove-metadata-cache
branch
Description
Finally ends fixing #541 ✅ , by removing the last use of metadata cache provider from message converter. Also fixes issue #612 ✅ related to double rendering some messages
I couldn't just replace directly in message converter the metadata cache provider with user profile data provider because this was used at message level. The user profile data provider instead of caching metadata, queries metadata via rust crate to the db. Which means that if had replaced directly the first provider with the second one, if I sent 100 messages in the same chat, it would have triggered 100 queries db (for the same user metadata).
So it was necessary to move the metadata queries one level above, not to message converter but where it was used, to chat provider. The problem is that the chat provider is already huge. So I added a new provider called group messages provider where that has the responsibility of: getting the group messages and the group members metadata and then converting the chat messages to message model objects using that metadata.
Also removes some unused methods from message converter file and moved reaction conversion logic to its own class.
Commit by commit
MessageConverterclass). Adds tests to this classisMelogic (just comparing pubkeys) that I realized I was repeating in message converter and the new provider while refactoring.MessageConverterby removing metadata cache provider and removing unused or duplicated logic that now it's going to be handled one level above (in a provider).groupMessagesProvider, which is a family provider that receives a group id and then, fetches the messages, the group members and converts the messages to message objects. This way the chat provider will have less responsibility (that one was too long)MessageMergerServicewith the responsibility of merging messages from db and the ones in the provider state. It decides which optimistic messages in sending state should be kept along with new messages (to help to fix double rendering message issue)Type of Change
Checklist
just precommitto ensure that formatting and linting are correctjust check-flutter-coverageto ensure that flutter coverage rules are passingCHANGELOG.mdfile with your changes (if they affect the user experience)Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests