Messages improvements #186
Conversation
WalkthroughThis pull request introduces comprehensive changes to the application's error handling, messaging system, and data models. The modifications span multiple files across the project, focusing on enhancing user experience, improving type safety, and streamlining data management. Key areas of improvement include localization of error messages, refactoring of chat and thread-related components, and introducing more robust error handling mechanisms. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI
participant ErrorHandler
participant APIService
participant Localization
User->>UI: Performs action
UI->>APIService: Makes API call
alt Network Error
APIService-->>ErrorHandler: Generates error
ErrorHandler->>Localization: Retrieve localized message
ErrorHandler->>UI: Display error message
UI->>User: Show "No internet connection" or generic error
else Successful
APIService->>UI: Return data
UI->>User: Display data
end
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@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: 3
🧹 Nitpick comments (16)
app/lib/ui/flow/message/thread_list_view_model.dart (1)
80-91: Check for multiple rebuilds
_getUsersupdates the state’susersmap, which triggers a rebuild. If_getUsersis called frequently or on a large number of member IDs, multiple rebuilds may impact performance. Consider batching or caching requests if callers invoke_getUsersoften in quick succession.app/lib/ui/flow/message/thread_list_screen.dart (1)
422-423: Edge case with formatting names
getFormattedNames()handles small lists carefully, but if the first or second item is an empty string, the UI might show extra punctuation. If that’s unlikely, this is fine. Otherwise, consider trimming or validating user names before merging.app/lib/ui/flow/message/chat/chat_view_model.dart (2)
171-182: Revisit the 2-second delay inmarkMessageAsSeen
Introducing aFuture.delayed(const Duration(seconds: 2))before marking messages as seen might cause delayed read receipts or confusion for users. If it’s intentional (e.g. to batch operations), clearly document the rationale. Otherwise, removing or lowering this delay could improve responsiveness.
396-397: Graceful handling of nullmemberId
toggleMemberSelectionimmediately returns ifmemberIdis null. This is correct for safety. If null is truly an unexpected case, consider throwing an assertion or logging a warning for debugging.app/lib/ui/components/on_visible_callback.dart (1)
3-15: OnVisibleCallback widget definition
This design is straightforward: it requires anonVisiblecallback and achild. EnsureonVisibleis idempotent or safe to call once if this widget is potentially re-initialized.data/lib/network/error.dart (1)
5-31: ApiError base class & factory
This is a good approach for normalizing errors. ThefromErrorfactory accurately distinguishes known error classes. Keep an eye on type coverage to ensure newly introduced error types are added here as well.data/lib/service/message_service.dart (2)
48-50: Ensure error handling for empty streams
You're retrieving the first event from the stream. If no data is emitted or there's an error, this could throw. Consider gracefully handling an empty or error response before returning.
68-69: Await the API call
SinceaddThreadSeenByreturns aFuture, consider usingawaitto ensure that any downstream logic executes after the service call completes.- messageService.addThreadSeenBy(threadId, userId); + await messageService.addThreadSeenBy(threadId, userId);data/lib/api/auth/api_user_service.dart (1)
124-139: Consider partial failures or error handling in batched queries.The batched approach is excellent for handling chunks of up to 10 userIds, and using
Future.waitis efficient. However, if one query fails, the entire method throws an exception. If partial results are acceptable (e.g., skipping failed queries), you could add error handling to keep partial successes.app/assets/locales/app_en.arb (1)
41-45: New error messages added successfully!Including
errorNoConnectionanderrorGenericis a neat way to localize error handling. If you plan to expand your error catalog, consider grouping them under a dedicated JSON section for better organization.app/lib/ui/flow/home/map/map_view_model.freezed.dart (1)
427-428: Ensure consistent hashing logic for spatial fields.
When switching to direct checks, confirm that hash collisions are minimized and that the new approach consistently reflects object equality.data/lib/api/message/message_models.freezed.dart (5)
103-106: Check null-safe copy logic.
Here,seen_by_idsgets copied withnull == seen_by_ids ? _value.seen_by_ids : seen_by_ids. Ensure that null inputs are handled as intended.
187-190: Manage empty list references.
Usingconst []as a default can help avoid null pointer issues. Confirm you do not inadvertently mutate the default list.
219-219: New default list assignment forseen_by_ids.
Coordinate with the front-end or other consumers to ensure an empty list has the correct semantics for “no viewers.”
275-275:toStringmodifications
Includingseen_by_idsin thetoStringhelps debugging, but avoid logging sensitive user data in production.
290-291: Check deep equality usage for_seen_by_ids.
Ensure that you truly need a deep equality check here, as repeated creation of the same list object may affect performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/ios/Podfile.lockis excluded by!**/*.lockapp/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
app/assets/locales/app_en.arb(1 hunks)app/lib/domain/extenstions/api_error_extension.dart(1 hunks)app/lib/ui/app_route.dart(1 hunks)app/lib/ui/components/error_snakebar.dart(1 hunks)app/lib/ui/components/on_visible_callback.dart(1 hunks)app/lib/ui/flow/geofence/add/locate/locate_on_map_view_model.freezed.dart(2 hunks)app/lib/ui/flow/home/map/map_view_model.freezed.dart(2 hunks)app/lib/ui/flow/message/chat/chat_screen.dart(15 hunks)app/lib/ui/flow/message/chat/chat_view_model.dart(9 hunks)app/lib/ui/flow/message/chat/chat_view_model.freezed.dart(19 hunks)app/lib/ui/flow/message/thread_list_screen.dart(7 hunks)app/lib/ui/flow/message/thread_list_view_model.dart(5 hunks)app/lib/ui/flow/message/thread_list_view_model.freezed.dart(12 hunks)app/pubspec.yaml(1 hunks)data/lib/api/auth/api_user_service.dart(2 hunks)data/lib/api/message/api_message_service.dart(5 hunks)data/lib/api/message/message_models.dart(1 hunks)data/lib/api/message/message_models.freezed.dart(14 hunks)data/lib/api/message/message_models.g.dart(2 hunks)data/lib/network/error.dart(1 hunks)data/lib/service/message_service.dart(2 hunks)
🔇 Additional comments (67)
app/lib/ui/flow/message/thread_list_view_model.dart (3)
52-77: Consider concurrency when calling listenThreads multiple times
If listenThreads can be invoked repeatedly (e.g. after setSpace updates or other triggers), ensure only one active subscription is listening at a time. This code cancels the previous subscription, which is good. However, confirm in the broader codebase that there are no rapid consecutive calls creating overlapping subscriptions.
94-110: Validate archived-thread filtering logic
The filter excludes threads where archiveTimestamp >= lastMessageTimestamp. Verify that this matches the intended behavior, especially if the user archives the thread after the last message was posted—such a thread is removed from the list. Ensure this is the desired user experience.
157-161: Properly disposing subscriptions
Disposing of _streamSubscription in dispose() is crucial. This prevents memory leaks and ensures listeners do not linger after the notifier is destroyed. This is an excellent practice.
app/lib/ui/flow/message/thread_list_view_model.freezed.dart (1)
26-27: Auto-generated code
This file is generated by Freezed and should not be manually modified. The new threads and users fields are properly represented with unmodifiable collections, ensuring immutability in the state. All changes here look appropriate.
Also applies to: 42-50, 76-80, 109-130, 259-274
app/lib/ui/flow/message/chat/chat_view_model.dart (1)
415-419: Subscriptions fully cleaned up on dispose
Both _threadSubscription and _messageSubscription are canceled. This is best practice for preventing memory leaks in a StateNotifier.
app/lib/ui/flow/message/chat/chat_screen.dart (1)
612-632: Fallback chat title logic
_formattedChatTitle has a sensible fallback to context.l10n.chat_start_new_chat_title or space.name, returning an empty string otherwise. This ensures a graceful user experience if member data or space info is incomplete.
app/lib/ui/flow/message/chat/chat_view_model.freezed.dart (13)
21-21: Updated properties for ChatViewState
These newly introduced or renamed properties (loadingMoreMessages, threadId, actionError, space, thread, members, currentUser, and threads) clarify responsibilities within the state and align well with typical naming. However, consider ensuring that threadId is consistently treated as nullable throughout the rest of the code (e.g., usage sites, constructors, or conditionals) to avoid potential null pointer issues.
Also applies to: 27-27, 32-37
53-53: Expanded copyWith arguments
The copyWith factory now accommodates the new fields. No issues identified, but verify that all references to these properties in the codebase do not assume non-null or a previous type signature.
Also applies to: 59-59, 64-72
91-91: Handling optional fields in copyWith
The logic correctly accounts for threadId, error, actionError, etc. Ensure the rest of the app checks for null values where appropriate, especially for error fields typed as Object?. A more specialized error type might help if consistently used throughout.
Also applies to: 97-97, 102-107, 118-120
Line range hint 142-179: Continued copyWith expansion
These lines mirror the prior logic and appear consistent. The transitions from loadingMoreMessages, threadId, and newly introduced fields look correct.
185-215: Nested copy for nested objects
It’s good that space, thread, and currentUser each have their own copy logic. This pattern ensures immutability across nested objects. No changes needed.
231-247: Second copyWith interface
Same general comment as above: code is consistent, likely auto-generated by Freezed. Just ensure usage sites rely on these new fields consistently.
Line range hint 270-299: Refining the constructor parameters
Reiterates the same pattern for the _$$ChatViewStateImplCopyWithImpl. This looks fine and is standard Freezed boilerplate.
321-321: Object creation logic
The logic to create a new _ChatViewStateImpl with updated fields is consistent. No issues detected.
Also applies to: 324-324, 338-358
369-389: _ChatViewStateImpl constructor
Both optional and default parameters look fine. The usage of lists/maps returned as unmodifiable wrappers is a best practice to ensure immutability in state.
Line range hint 399-462: Getters for the new properties
The built-in immutability checks with EqualUnmodifiableListView and EqualUnmodifiableMapView are well-handled. Nothing to change here.
Line range hint 467-505: Equality checks
The equality logic now includes the newly introduced fields. This is consistent with typical Freezed expansions.
Line range hint 513-559: Hash code updates
The newly introduced fields are included in the hash computation, which is correct to maintain consistency.
Line range hint 566-598: Interface definitions
Defines the updated shape of _ChatViewState, including the new or modified fields. Ensure external references are updated accordingly.
app/lib/ui/components/on_visible_callback.dart (2)
1-2: Import statement
The import is minimal, referencing only package:flutter/cupertino.dart. This is fine as a dependency for your StatefulWidget.
17-28: _OnCreateState lifecycle approach
Calling onVisible() in initState() is clear. Just be mindful if any asynchronous logic or side effects might cause issues if this state is disposed early or re-created. Overall, good approach.
app/lib/domain/extenstions/api_error_extension.dart (2)
1-3: Imports and extension definition
Bringing in context_extenstions.dart and network/error.dart clarifies usage of the extension for localized error messages. No issues here.
6-23: AppErrorExtensions on Object
This approach allows flexible handling of various error types, returning a user-friendly message. Just confirm each case (NoInternetConnectionError, GenericError, etc.) matches an actual variant from the error classes.
app/lib/ui/components/error_snakebar.dart (2)
3-3: Importing new error utilities
Importing api_error_extension.dart is crucial for the new localized message support. This is an improvement for user feedback.
Also applies to: 7-7
11-11: Using l10nMessage for error
Replacing direct string conversion with ApiError.fromError(error).l10nMessage(context) is a solid way to unify error messages across the app. No issues here.
data/lib/network/error.dart (3)
1-1: Import for async
Using dart:async is standard for TimeoutException.
34-62: Specific error classes
Each subclass clarifies a distinct error scenario. The messages are direct and user-friendly. This is well-structured for typical usage.
64-81: GenericError and StringError
Capturing unknown errors or raw string errors in distinct classes is helpful. The toString override and message getter are well-defined.
data/lib/service/message_service.dart (1)
52-53: Naming alignment
The streamThread method name is consistent with its purpose, returning a stream of ApiThread objects. No issues found here.
data/lib/api/message/message_models.dart (1)
17-17: Default list usage
Using @Default([]) for seen_by_ids is a clean way to avoid null checks. This is a good practice for safer data handling.
data/lib/api/message/message_models.g.dart (2)
17-20: Null safety for seen_by_ids
Great use of ?? const [], which prevents null pointer issues when seen_by_ids is missing in the JSON. This is well-implemented.
36-36: Consistent serialization
Your JSON serialization for seen_by_ids matches the new field in the model. This ensures consistent read/write operations.
data/lib/api/message/api_message_service.dart (20)
10-12: Provider injection
The constructor parameters are neatly injected here. The usage of Riverpod’s ref.read maintains clarity. No issues found.
22-25: Type conversion
The Firestore converter is correctly set up for ApiThread. No concerns with the usage of fromFirestore and toFirestore.
27-35: Typed reference
Returning CollectionReference<ApiThreadMessage> ensures strong typing. This helps avoid serialization mistakes.
38-41: Constructor usage
The createThread method looks clear and straightforward. The fields are properly provided, and the method returns the newly created threadId.
69-70: Maintain sorting consistency
Using orderBy("created_at", descending: true) is consistent with other message retrieval methods, ensuring the stream is reversed chronologically.
75-75: Mapping snapshot
The .map((doc) => doc.data()) pattern is concise and clear for streaming message data in real-time.
78-79: Limit usage
The limit parameter is a common design for pagination. Maintains consistent design with the streamed fetch.
84-84: Return of mapped data
Your approach of returning snapshot.docs.map((doc) => doc.data()).toList() is direct and keeps the logic simple.
91-94: Optional chaining consideration
Your fallback to null for member and subsequent creation of a nullable ApiUserInfo is properly handled. No problems found.
97-100: Filtering null members
This code effectively removes any null ApiUserInfo. The final cast to a non-null list is a good practice.
105-106: Parameter usage
from is used to limit messages by time. Good design for pagination or caching logic.
117-117: Query building
Chaining .where('created_at', isLessThan: ...) is correct for a descending fetch. Logic is consistent.
119-119: Return statement
Mapping over snapshot.docs is succinct, returning a list of typed messages.
122-125: Constructor clarity
Constructing ApiThreadMessage inline with optional fields is readable, ensuring the message is ready for immediate use.
139-139: Simplified message sending
Using .set(message) directly is elegant; no extraneous steps are performed.
142-145: Shift from markMessagesAsSeen to addThreadSeenBy
This new approach simplifies marking multiple messages as seen by focusing on the thread. The use of arrayUnion ensures idempotent handling.
148-149: Method naming
getThreadsWithMembers is nicely descriptive. This helps maintain clarity when merging the thread data and user info.
157-160: Optional user handling
Fallback to null when no user is found is correct. The additional check avoids potential usage of incomplete user data.
164-167: Collecting valid members
Same pattern of filtering out unreachable or invalid user references to maintain data quality.
191-191: End of class
All changes appear consistent and comply with the updated data structure requirements.
data/lib/api/auth/api_user_service.dart (1)
46-46: Enhanced type safety looks great!
Changing _userRef to CollectionReference<ApiUser> improves the clarity and safety of Firestore operations by linking the collection to the correct data model.
app/lib/ui/flow/geofence/add/locate/locate_on_map_view_model.freezed.dart (2)
235-241: Verify whether reference equality suffices for your use case.
Replacing deep equality checks with a direct == or identical(...) is more performant but might overlook changes in nested data if those objects are mutable. Confirm that these fields are immutable or that reference equality is enough for your app logic.
253-254: Ensure consistency between equality and hashCode.
Your hash code now references the fields directly instead of relying on a deep collection hash. This must be kept in sync with your equality checks. Currently, the code is consistent, but any future changes to equality logic should also update the hash code.
Also applies to: 256-256
app/lib/ui/app_route.dart (2)
265-267: Updated parameters for chat route.
Switching from spaceId/spaceName to ApiSpace? space and from threadMessage to List<ApiThread>? threads aligns well with your new data model. Make sure any existing code references are updated to prevent breakage.
272-274: Validate new ChatScreen instantiation logic.
The ChatScreen now receives space, threadId, and threadInfoList, which should reflect your new chat flow. Double-check whether any old references (like spaceId) remain in ChatScreen or related view models.
app/lib/ui/flow/home/map/map_view_model.freezed.dart (1)
401-404: Consider implications of replacing DeepCollectionEquality with direct reference checks.
If LatLng and CameraPosition instances are frequently reconstructed (e.g., new objects with the same data), direct reference checks might fail. Ensure this change won’t cause unexpected mismatches.
data/lib/api/message/message_models.freezed.dart (9)
27-27: New seen_by_ids property introduced.
Ensure that you handle empty or null lists consistently. Also, consider if you need a default value to avoid runtime exceptions.
54-54: Keep seen_by_ids references consistent in copyWith.
Verify that setting seen_by_ids through copyWith results in expected updates.
80-80: Confirm usage with large lists.
If member_ids or seen_by_ids can contain a lot of data, verify that performance and memory usage remain acceptable.
164-164: Validate new property usage.
At a glance, the property is consistent, but confirm it’s not overshadowing any existing fields in the JSON.
225-225: Cascading changes with _archived_for.
Be cautious about combining new and existing optional fields; ensure backward compatibility with older data.
246-253: Review immutability of _seen_by_ids.
You’re returning an EqualUnmodifiableListView. Confirm that callers are aware they cannot mutate this list.
310-310: Hash code generation
Updates to the hash code are consistent with the new field. Confirm no collisions are introduced by large lists.
338-338: JSON constructor reference
Double-check that any migration logic accounts for introducing seen_by_ids.
358-359: Ensure consistent copyWith usage for seen_by_ids.
Check if a null seen_by_ids in the JSON would cause any unexpected behavior during deserialization.
| final hasUnreadMessage = threads.any((message) => | ||
| !thread.seen_by_ids.contains(notifier.currentUser?.id)); |
There was a problem hiding this comment.
Likely incorrect usage of threads.any(...)
final hasUnreadMessage = threads.any(
(message) => !thread.seen_by_ids.contains(notifier.currentUser?.id)
);Here, the parameter message is actually an ApiThread, but you are checking thread.seen_by_ids for the current thread. This suggests a mix-up of variables. If you want to check whether the current thread has unread messages, just test !thread.seen_by_ids.contains(currentUserId).
- final hasUnreadMessage = threads.any((message) => !thread.seen_by_ids.contains(notifier.currentUser?.id));
+ final hasUnreadMessage = !thread.seen_by_ids.contains(notifier.currentUser?.id);|
|
||
| final sender = members.isNotEmpty && members.length > 2 | ||
| ? members.firstWhereOrNull((m) => m.id == message.sender_id) | ||
| : null; | ||
| final isSender = message.sender_id != state.currentUser?.id; | ||
|
|
||
| final seenBy = threadInfo?.members | ||
| .where((member) => | ||
| message.seen_by.contains(member.user.id) && | ||
| member.user.id != currentUserId) | ||
| final seenBy = (state.thread?.seen_by_ids ?? []) | ||
| .map((id) => members.firstWhereOrNull((m) => m.id == id)) | ||
| .whereType<ApiUser>() | ||
| .toList(); | ||
|
|
||
| final showSeenText = notifier.isSender(message) && | ||
| (seenBy?.isNotEmpty ?? false) && | ||
| message.id == messages.first.id | ||
| ? true | ||
| : false; | ||
|
|
||
| if (messages.isEmpty) { | ||
| return const AppProgressIndicator(); | ||
| } | ||
|
|
||
| return Column( | ||
| crossAxisAlignment: notifier.isSender(message) | ||
| ? showSeenText | ||
| ? CrossAxisAlignment.end | ||
| : CrossAxisAlignment.start | ||
| : CrossAxisAlignment.end, | ||
| children: [ | ||
| if (showDateHeader) | ||
| _dateHeader(message.created_at! | ||
| .format(context, DateFormatType.relativeDate)), | ||
| _chatItem( | ||
| context: context, | ||
| message: message, | ||
| sender: senderInfo, | ||
| isSender: notifier.isSender(message), | ||
| showTimeHeader: notifier.showTimeHeader(index, message), | ||
| isFirstInGroup: notifier.isFirstInGroupAtIndex(index), | ||
| isLastInGroup: notifier.isLastInGroupAtIndex(index), | ||
| memberCount: sender.length, | ||
| isDifferentSender: isDifferentSender, | ||
| ), | ||
| if (showSeenText) ...[ | ||
| const SizedBox(height: 4), | ||
| Text( | ||
| sender.length > 2 | ||
| ? context.l10n.chat_seen_by_message_text( | ||
| seenBy!.map((e) => e.user.first_name).join(', ')) | ||
| : context.l10n.chat_seen_message_text, | ||
| style: AppTextStyle.caption.copyWith( | ||
| color: context.colorScheme.textDisabled, | ||
| //print('seenBy: ${seenBy.length} seen_by_ids ${state.thread?.seen_by_ids} currentUser ${state.currentUser?.id}'); | ||
| final isSenderLatestMsg = message.sender_id == state.currentUser?.id && | ||
| message.id == messages.first.id; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential naming confusion in isSender
final isSender = message.sender_id != state.currentUser?.id;Semantically, “isSender” often implies the current user is the sender, but here it’s set to true if the message.sender_id differs from the current user. This might cause confusion. Rename it to isOtherSender or invert the logic for clarity.
- final isSender = message.sender_id != state.currentUser?.id;
+ final isSender = message.sender_id == state.currentUser?.id;| @@ -65,6 +65,7 @@ dependencies: | |||
| flutter_background_service: ^5.0.6 | |||
| url_launcher: ^6.3.0 | |||
| battery_plus: ^6.2.0 | |||
| collection: any | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin your dependencies to a stable version.
Currently, collection: any may unintentionally pull in breaking changes. Consider pinning to a specific major version.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Performance Improvements
Documentation
Chores