feat(llc, core): message reminders #2269
Conversation
WalkthroughThis update introduces full support for message reminders, allowing users to bookmark or schedule reminders for messages. It includes backend API integration, new models, event types, response classes, and UI components for creating, updating, deleting, and listing reminders. The sample app is updated with a Reminders page and reminder management actions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Channel
participant Client
participant RemindersApi
participant Server
User->>UI: Selects "Remind me" on a message
UI->>Channel: createReminder(messageId, remindAt)
Channel->>Client: createReminder(messageId, remindAt)
Client->>RemindersApi: createReminder(messageId, remindAt)
RemindersApi->>Server: POST /messages/{messageId}/reminders
Server-->>RemindersApi: CreateReminderResponse
RemindersApi-->>Client: CreateReminderResponse
Client-->>Channel: CreateReminderResponse
Channel-->>UI: Update message with reminder
Note over UI,Channel: UI updates message display to show reminder/bookmark
sequenceDiagram
participant Client
participant RemindersApi
participant Server
participant Controller
participant UI
Client->>RemindersApi: queryReminders(filter, sort, pagination)
RemindersApi->>Server: POST /reminders/query
Server-->>RemindersApi: QueryRemindersResponse
RemindersApi-->>Client: QueryRemindersResponse
Client-->>Controller: Reminders list
Controller-->>UI: Display reminders list
Assessment against linked issues
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 8
🧹 Nitpick comments (8)
packages/stream_chat/lib/src/core/api/responses.dart (1)
769-773: Fix the comment to accurately describe the reminder field.The comment incorrectly refers to "Draft returned by the api call" but should reference reminder since this is the base class for reminder responses.
/// Base Model response for draft based api calls. class MessageReminderResponse extends _BaseResponse { - /// Draft returned by the api call + /// Reminder returned by the api call late MessageReminder reminder; }packages/stream_chat_flutter_core/lib/src/stream_message_reminder_list_event_handler.dart (1)
22-24: Fix documentation inconsistency in onMessageReminderCreated.The documentation comment incorrectly refers to
messageReminderUpdatedevent type but should refer to the creation event./// Function which gets called for the event - /// [EventType.messageReminderUpdated]. + /// [EventType.messageReminderCreated]. /// - /// This event is fired when a message reminder is updated. + /// This event is fired when a message reminder is created. /// - /// By default, this updates the reminder in the list. + /// By default, this adds the reminder to the list.Also applies to: 28-36
packages/stream_chat/lib/src/client/channel.dart (1)
1311-1319: updateReminder(): documentation/argument mismatchThe doc-string mentions
reminderIdbut the parameter ismessageId.
If the endpoint expects a message id (which matches the usage), please update the comment; otherwise the implementation is wrong.packages/stream_chat_flutter_core/lib/src/stream_message_reminder_list_controller.dart (5)
96-107:valueoverride re-sorts on every assignment – avoid redundant workEvery time
valueis set, the list is cloned and resorted, even when the incomingPagedValueis already ordered or contains only the delta fromloadMore().
For large lists this can be expensive.
- Short-circuit when the incoming list is already sorted:
- items: success.items.sorted(reminderSort.compare), + items: success.items.isSorted(reminderSort.compare) + ? success.items + : success.items.sorted(reminderSort.compare),
isSortedcan be a tiny extension that performs an O(n) check.
- Consider sorting only once right before exposing items to the UI rather than on every assignment.
[performance]
176-199: Filter the event stream to save bandwidth & CPU
client.on().listendelivers all WS events, most of which are irrelevant here.
You can substantially reduce GC pressure and wake-ups by pre-filtering:- _reminderEventSubscription = client.on().listen((event) { + _reminderEventSubscription = client + .on( + EventType.reminderCreated, + EventType.reminderUpdated, + EventType.reminderDeleted, + EventType.notificationReminderDue, + EventType.connectionRecovered, + ) + .listen((event) {This also makes intent explicit.
[performance]
221-235:mergeextension is non-standard – ensure null-safety & O(n²) avoidanceNice use of
merge, but:
If
messageIdoruserIdcan benull,(reminder.messageId, reminder.userId)forms a tuple containingnull, causingMapKeycollisions. Guard with??fallbacks or assert non-null.
mergemust be O(n) internally; otherwise repeated updates could degrade to O(n²). Verify the extension’s implementation or switch to aMap<(String,String),MessageReminder>internally.
240-255:deleteRemindercould use the key-based approach used inupdateReminderYou already compute a composite key in
updateReminder; replicating that logic here would simplify the code and avoid an O(n) scan:- final removeIndex = currentReminders.indexWhere( - (it) { - var predicate = it.userId == reminder.userId; - predicate &= it.messageId == reminder.messageId; - return predicate; - }, - ); + final removeIndex = currentReminders.indexWhere( + (it) => it.userId == reminder.userId && + it.messageId == reminder.messageId, + );(Or keep a
Set/Mapkeyed by the tuple for O(1) removals.)
275-279: Race-free disposal
_unsubscribeFromReminderListEvents()cancels the subscription but does not set a disposed flag.
If a pendingloadMore()future completes after disposal, it can re-establish state.
Set a_isDisposedboolean and early-return inloadMore/doInitialLoadwhen disposed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/stream_chat/lib/src/client/channel.dart(3 hunks)packages/stream_chat/lib/src/client/client.dart(2 hunks)packages/stream_chat/lib/src/core/api/reminders_api.dart(1 hunks)packages/stream_chat/lib/src/core/api/responses.dart(2 hunks)packages/stream_chat/lib/src/core/api/responses.g.dart(1 hunks)packages/stream_chat/lib/src/core/api/stream_chat_api.dart(2 hunks)packages/stream_chat/lib/src/core/models/channel_config.dart(2 hunks)packages/stream_chat/lib/src/core/models/channel_config.g.dart(2 hunks)packages/stream_chat/lib/src/core/models/event.dart(8 hunks)packages/stream_chat/lib/src/core/models/event.g.dart(3 hunks)packages/stream_chat/lib/src/core/models/message.dart(8 hunks)packages/stream_chat/lib/src/core/models/message.g.dart(2 hunks)packages/stream_chat/lib/src/core/models/message_reminder.dart(1 hunks)packages/stream_chat/lib/src/core/models/message_reminder.g.dart(1 hunks)packages/stream_chat/lib/src/event_type.dart(1 hunks)packages/stream_chat/lib/stream_chat.dart(1 hunks)packages/stream_chat_flutter_core/lib/src/stream_message_reminder_list_controller.dart(1 hunks)packages/stream_chat_flutter_core/lib/src/stream_message_reminder_list_event_handler.dart(1 hunks)packages/stream_chat_flutter_core/lib/stream_chat_flutter_core.dart(1 hunks)sample_app/lib/pages/channel_list_page.dart(3 hunks)sample_app/lib/pages/channel_page.dart(5 hunks)sample_app/lib/pages/reminders_page.dart(1 hunks)sample_app/lib/widgets/reminder_dialog.dart(1 hunks)sample_app/pubspec.yaml(1 hunks)
🔇 Additional comments (46)
sample_app/pubspec.yaml (1)
34-34:✅ Verification successful
Verify the jiffy package version for currency and security.
The
jiffy ^6.2.1dependency addition is appropriate for the message reminders feature, as it provides excellent DateTime manipulation and formatting capabilities. However, ensure this version is current and free from security vulnerabilities.
🌐 Web query:
What is the latest stable version of the jiffy Dart package and are there any known security vulnerabilities in version 6.2.1?💡 Result:
Latest Stable Version of the Jiffy Dart Package
The latest stable version of the Jiffy package for Dart and Flutter is 6.4.3 as of May 2025[1][3]. This version is compatible with Dart 3, supports all major platforms, and includes several improvements over previous versions.
Known Security Vulnerabilities in Jiffy 6.2.1
No security advisories or known vulnerabilities have been published for Jiffy version 6.2.1. Searches of CVE databases and Dart's official security advisory listings return no results for vulnerabilities associated specifically with the
jiffypackage, including version 6.2.1[6][7][9]. The Dart security advisories are integrated with GitHub's advisory database, and any major or recent issues would be surfaced via tools likedart pub get; as of now, there are no alerts or advisories listed for this version.Summary Table
Package Latest Stable Version Security Issues in 6.2.1 Jiffy (Dart/Flutter) 6.4.3 None reported as of May 2025 If you are starting a new project or maintaining an existing one, upgrading to the latest version (6.4.3) is recommended to benefit from continued bug fixes, improvements, and any future security patches[1][3][2].
Citations:
- 1: https://pub.dev/packages/jiffy
- 2: https://pub.dev/packages/jiffy/versions
- 3: https://fluttergems.dev/packages/jiffy/
- 4: https://codewithandrea.com/newsletter/may-2025/
- 5: https://github.com/jama5262/jiffy/blob/master/doc/README.md
- 6: https://dart.dev/tools/pub/security-advisories
- 7: https://cve.mitre.org/cgi-bin/cvekey.cgi
- 8: https://blog.stackademic.com/the-essential-flutter-dart-package-guide-for-2025-10-most-downloaded-libraries-every-developer-fd7ccfe5dc02
- 9: https://www.cvedetails.com/vendor/12360/Dart.html
- 10: https://www.scribd.com/document/687489337/Locks-Safes-and-Security-an-International-Police-Reference-eBook-Version-by-Marc-Weber-Tobias-Z-lib-org
Audit of jiffy version – no security risks; consider upgrade
- Verified that Jiffy 6.2.1 has no known security advisories or vulnerabilities.
- The latest stable release is 6.4.3 (May 2025), which includes additional bug fixes and improvements.
- You may continue with 6.2.1 safely, but consider updating to
^6.4.3to benefit from the latest enhancements.packages/stream_chat/lib/stream_chat.dart (1)
47-47: LGTM! Export follows consistent patterns.The export for
message_reminder.dartis correctly placed in alphabetical order and follows the established pattern for model exports in this library file.packages/stream_chat/lib/src/core/api/stream_chat_api.dart (2)
12-12: LGTM! Import follows consistent patterns.The import for
RemindersApiis correctly placed in alphabetical order with other API imports.
91-93: LGTM! API integration follows established patterns.The
RemindersApiintegration perfectly follows the established pattern used by other APIs in this class:
- Proper documentation comment
- Lazy initialization using the null-aware assignment operator
- Private nullable field for caching
- Consistent naming convention
packages/stream_chat_flutter_core/lib/stream_chat_flutter_core.dart (1)
25-26: LGTM! Exports follow consistent architectural patterns.The two new exports for
stream_message_reminder_list_controller.dartandstream_message_reminder_list_event_handler.dartare:
- Correctly placed in alphabetical order
- Following the established naming convention
- Consistent with the controller + event handler pattern used by other list management features (channels, drafts, threads)
packages/stream_chat/lib/src/core/models/channel_config.dart (1)
28-28: LGTM! Clean implementation following established patterns.The
userMessageRemindersfield addition follows the existing code patterns perfectly:
- Appropriate default value of
falsefor new feature- Consistent constructor parameter ordering
- Clear documentation explaining the field's purpose
- Follows naming conventions and field declaration patterns
Also applies to: 91-92
packages/stream_chat/lib/src/core/models/channel_config.g.dart (1)
36-36: Auto-generated serialization code looks correct.The JSON serialization/deserialization logic properly handles the new
userMessageRemindersfield:
- Correct snake_case JSON key mapping (
user_message_reminders)- Appropriate null safety with
?? falsedefault- Follows consistent patterns with other boolean configuration fields
Also applies to: 59-59
sample_app/lib/pages/channel_list_page.dart (3)
13-13: Clean import addition.The import statement follows the existing pattern and is properly organized with other page imports.
96-104: Excellent tab implementation following established patterns.The new "Reminders" tab is well-implemented:
- Semantic bookmark icon choice is appropriate for reminders functionality
- Consistent color theming using
_isSelected(4)logic- Follows the exact same structure as existing navigation items
- Proper index handling for the 5th tab position
149-149: Proper IndexedStack integration.The
RemindersPageis correctly added to the IndexedStack at the appropriate index position matching the bottom navigation tab order.packages/stream_chat/lib/src/core/models/message.g.dart (1)
95-97: Auto-generated serialization correctly handles the new reminder field.The serialization logic properly implements the optional
reminderfield:
- Appropriate null checking during deserialization
- Correct type mapping to
MessageReminder.fromJson- Follows the same pattern as other optional complex fields (poll, draft)
- Proper null-safe serialization with
?.toJson()Also applies to: 116-116
packages/stream_chat/lib/src/event_type.dart (1)
162-172: LGTM! Reminder event types follow established conventions.The new reminder-related event types are properly implemented with consistent naming conventions and appropriate documentation. They follow the existing pattern of camelCase constants with dot-separated lowercase string values.
packages/stream_chat/lib/src/core/models/message_reminder.g.dart (1)
39-41:❌ Incorrect review comment
Verify null safety consistency for DateTime fields.
The
toJsonmethod callstoIso8601String()oncreatedAtandupdatedAtwithout null checks, but thefromJsonmethod treats these fields as nullable. This suggests potential inconsistency in theMessageRemindermodel definition.
🏁 Script executed:
#!/bin/bash # Check the MessageReminder model definition to verify DateTime field nullability cat packages/stream_chat/lib/src/core/models/message_reminder.dart | grep -A 10 -B 5 "createdAt\|updatedAt"Length of output: 3336
Ignore null-safety concerns for
createdAt/updatedAtThe
MessageReminderconstructor acceptsDateTime? createdAtandupdatedAtbut immediately defaults them toDateTime.now(), making the final fields non-nullable. Therefore, callingtoIso8601String()intoJsonis safe and consistent with the model.No changes are required.
Likely an incorrect or invalid review comment.
packages/stream_chat/lib/src/core/models/event.g.dart (1)
11-11: LGTM! Event JSON serialization properly handles new fields.The additions for
userIdandreminderfields are correctly implemented with:
- Proper null safety handling in both serialization and deserialization
- Appropriate JSON key mapping (
user_idfor userId)- Consistent use of conditional serialization patterns
Also applies to: 68-70, 77-77, 114-114
packages/stream_chat/lib/src/core/models/event.dart (4)
13-13: LGTM! Constructor properly includes new parameters.The
userIdandreminderparameters are correctly added to the constructor following the existing parameter order and nullable patterns.Also applies to: 43-43
59-60: LGTM! Field declarations are well-documented and consistent.The new fields follow the established documentation pattern and proper type declarations. The
userIdfield provides clear context about event ownership, and thereminderfield enables events to carry reminder data.Also applies to: 154-155
164-164: LGTM! Serialization fields properly registered.The
user_idandreminderkeys are correctly added totopLevelFields, ensuring proper serialization handling by theSerializerutility.Also applies to: 195-195
206-206: LGTM! copyWith method comprehensively updated.The
copyWithmethod properly includes the new fields in both the parameter list and the constructor call, maintaining consistency with the existing implementation pattern.Also applies to: 236-236, 241-241, 271-271
packages/stream_chat/lib/src/core/api/responses.g.dart (1)
447-469: LGTM! Generated code follows established patterns.The new JSON deserialization functions for reminder responses are correctly generated and follow the same patterns as existing response deserializers. The handling of nullable fields, default values, and nested object deserialization is consistent with the rest of the codebase.
packages/stream_chat/lib/src/core/models/message.dart (7)
7-7: LGTM! Proper import added for MessageReminder.The import statement correctly adds the MessageReminder model which is needed for the new reminder field.
71-71: LGTM! Reminder field properly added to constructor.The reminder parameter is correctly added to the Message constructor with the appropriate nullable type.
312-315: LGTM! Reminder field properly declared with good documentation.The reminder field is correctly declared as optional with clear documentation explaining its purpose.
364-364: LGTM! Reminder field added to topLevelFields.The reminder field is correctly included in the topLevelFields array, which is essential for proper serialization handling.
426-426: LGTM! Reminder field properly integrated into copyWith method.The reminder parameter is correctly added to the copyWith method signature and implementation using the null-const sentinel pattern, consistent with the existing draft field handling.
Also applies to: 507-508
553-553: LGTM! Reminder field included in merge method.The reminder field is properly included in the merge method to ensure it's copied when combining messages.
618-618: LGTM! Reminder field added to props for equality comparison.The reminder field is correctly included in the props getter, ensuring it's considered in equality comparisons and hash code generation.
packages/stream_chat/lib/src/core/api/responses.dart (2)
14-14: LGTM! Proper import added for MessageReminder.The import statement correctly adds the MessageReminder model which is needed for the new response classes.
775-804: LGTM! Reminder response classes follow established patterns.The new response classes (
CreateReminderResponse,UpdateReminderResponse, andQueryRemindersResponse) are well-structured and follow the same patterns as existing response classes in the codebase. They properly:
- Use
@JsonSerializable(createToJson: false)annotation- Extend appropriate base classes
- Include static
fromJsonfactory methods- Handle lists with default values for
QueryRemindersResponse- Include pagination support with the
nextfieldpackages/stream_chat_flutter_core/lib/src/stream_message_reminder_list_event_handler.dart (5)
8-8: LGTM! Well-designed mixin class for event handling.The
mixin classapproach allows flexible composition and provides good default implementations for reminder event handling.
15-20: LGTM! Proper no-op implementation for connection recovery.The connection recovery handler is appropriately implemented as a no-op with clear documentation explaining its purpose and potential use cases.
44-52: LGTM! Proper implementation for reminder updates.The update handler correctly extracts the reminder from the event and calls the controller's update method.
60-68: LGTM! Proper implementation for reminder deletion.The deletion handler correctly extracts the reminder from the event and calls the controller's delete method.
76-81: LGTM! Logical delegation for reminder due events.The reminder due handler appropriately delegates to the update handler, as a due reminder should be updated in the list to reflect its new status.
packages/stream_chat/lib/src/core/api/reminders_api.dart (1)
10-91: LGTM! Well-structured API class.The
RemindersApiclass follows consistent patterns with other API classes in the codebase. The use of POST for querying is appropriate given the complexity of filters and pagination parameters that may be included.packages/stream_chat/lib/src/client/client.dart (2)
29-29: LGTM! Correct import addition.The import for the new MessageReminder model is properly added.
1958-2003: LGTM! Clean integration of reminder functionality.The reminder methods are well-implemented with:
- Consistent delegation to the API layer
- Clear documentation
- Appropriate parameter handling
- Follows established patterns in the codebase
sample_app/lib/widgets/reminder_dialog.dart (3)
4-15: LGTM! Excellent use of sealed classes for type safety.The
ReminderOptionhierarchy withScheduledReminderandBookmarkReminderprovides clear type safety and makes the intent explicit. This is a well-designed pattern for distinguishing between scheduled reminders and bookmarks.
17-53: LGTM! Well-implemented create reminder dialog.The dialog provides a clean user experience with:
- Predefined durations that cover common use cases
- Human-readable relative time display using Jiffy
- Consistent Cupertino theming
- Proper navigation handling
55-102: LGTM! Edit dialog with appropriate functionality.The edit dialog appropriately extends the create dialog with the ability to clear due dates for non-bookmark reminders. The conditional logic for showing the "Clear due date" option is correct.
packages/stream_chat/lib/src/core/models/message_reminder.dart (5)
10-14: LGTM! Excellent use of sentinel pattern.The
_NullConstpattern is a sophisticated approach to distinguish between explicitly setting a nullable field to null versus leaving it unchanged in thecopyWithmethod. This is a best practice for optional nullable parameters.
28-42: LGTM! Well-designed constructor with sensible defaults.The constructor properly handles:
- Required core identifiers
- Optional related objects that may be populated later
- Automatic timestamp initialization
- Clear parameter organization
48-78: LGTM! Appropriate JSON serialization configuration.The use of
@JsonKey(includeToJson: false)for related objects (channel, message, user) is correct since these are typically populated from separate API calls and shouldn't be serialized back to the server.
84-106: LGTM! Correct implementation of copyWith with sentinel pattern.The
copyWithmethod correctly uses the sentinel value to distinguish between:
- Not changing the
remindAtfield (whenremindAt == _nullConst)- Explicitly setting it to null (when
remindAtisnull)- Setting it to a specific DateTime value
This allows for precise control over nullable field updates.
151-164: LGTM! Type-safe sort keys with extension types.The
MessageReminderSortKeyextension type provides compile-time safety for sort field names while maintaining string compatibility with the API. This is a modern Dart pattern that prevents typos in sort field names.sample_app/lib/pages/channel_page.dart (1)
220-224:client.createReminder(messageId)call no longer matches the new signatureAfter making
remindAtoptional (see previous comment) the call is fine; if you keep itrequired, this line won’t compile. Keep the two in sync.packages/stream_chat_flutter_core/lib/src/stream_message_reminder_list_controller.dart (1)
202-208:refresh(resetValue: true)unexpectedly reverts active filters/sortsBecause
resetValue: trueassigns_activeFilter = filterandfilteris the constructor value (see first comment), callingrefresh()after dynamically changing filters reverts the controller to its initial query, surprising callers.Consider either:
- Removing the automatic reset when
resetValueistrue, or- Passing explicit parameters to
refresh().At minimum, document this gotcha in the dart-doc.
packages/stream_chat_flutter_core/lib/src/stream_message_reminder_list_controller.dart
Show resolved
Hide resolved
packages/stream_chat_flutter_core/lib/src/stream_message_reminder_list_controller.dart
Show resolved
Hide resolved
xsahil03x
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/stream_chat/lib/src/core/api/sort_order.dart (1)
113-136:⚠️ Potential issueNull ordering is inverted for DESC sorts – run-time bug
comparemultiplies the result of_compareNullableFieldsbydirection.
Because_compareNullableFieldscurrently returns-1/1without takingdirectioninto account, the sign is flipped forDESCsorts, pushingnullvalues to the end even whennullOrderingisnullsFirst(and vice-versa).// Example: desc + nullsFirst a == null, b == 5 ➜ _compareNullableFields ⇒ -1 compare ⇒ -1 * -1 = +1 // null ends up *after* 5Fix by making the helper aware of
directionso that its result is already in final orientation:-int _compareNullableFields(ComparableField? a, ComparableField? b) { - if (a == null && b == null) return 0; - if (a == null) return nullOrdering == NullOrdering.nullsFirst ? -1 : 1; - if (b == null) return nullOrdering == NullOrdering.nullsFirst ? 1 : -1; - - return a.compareTo(b); -} +int _compareNullableFields(ComparableField? a, ComparableField? b) { + if (a == null && b == null) return 0; + + // Respect desired null placement irrespective of sort direction. + if (a == null) { + return nullOrdering == NullOrdering.nullsFirst ? -direction : direction; + } + if (b == null) { + return nullOrdering == NullOrdering.nullsFirst ? direction : -direction; + } + + // Natural comparison; will be flipped by `direction` in the public `compare`. + return a.compareTo(b); +}This keeps
nullelements consistently at the requested side for both ASC and DESC sorts.
Please add a unit test covering all four combinations (ASC/DESC × nullsFirst/nullsLast).
🧹 Nitpick comments (2)
packages/stream_chat/lib/src/core/api/sort_order.dart (2)
56-62: Allow callers to overridenullOrderingin convenience constructors
SortOption.ascandSortOption.deschard-code their null-ordering defaults and
don’t expose a parameter to change them, unlike the primary constructor.
Consider adding the same named parameter:-const SortOption.desc( - this.field, { - Comparator<T>? comparator, -}) : direction = SortOption.DESC, - nullOrdering = NullOrdering.nullsFirst, - _comparator = comparator; +const SortOption.desc( + this.field, { + this.nullOrdering = NullOrdering.nullsFirst, + Comparator<T>? comparator, +}) : direction = SortOption.DESC, + _comparator = comparator;(and similarly for
asc).
It keeps the API symmetrical and avoids forcing callers to fall back to the verbose constructor when only the null placement differs.Also applies to: 70-76
16-23: Minor: document enum values with lower-case namesDart style encourages sentence-style doc comments; also consider lower-case enum value docs for consistency with surrounding comments.
No functional change needed – purely stylistic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/stream_chat/CHANGELOG.md(1 hunks)packages/stream_chat/lib/src/client/channel.dart(3 hunks)packages/stream_chat/lib/src/core/api/reminders_api.dart(1 hunks)packages/stream_chat/lib/src/core/api/sort_order.dart(6 hunks)packages/stream_chat_flutter_core/CHANGELOG.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/stream_chat/CHANGELOG.md
- packages/stream_chat_flutter_core/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/stream_chat/lib/src/core/api/reminders_api.dart
- packages/stream_chat/lib/src/client/channel.dart
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test
- GitHub Check: build (android)
- GitHub Check: build (ios)
- GitHub Check: analyze
- GitHub Check: stream_chat_persistence
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter_core
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/stream_chat_flutter_core/test/stream_message_reminder_list_controller_test.dart (3)
25-33: Use deterministicDateTimevalues to prevent flakey tests
DateTime.now()makes the generated reminder depend on wall-clock time.
If tests and assertions run fast enough, equality checks may accidentally succeed; if they span even a few ms they can start failing intermittently (CI, slower machines).- remindAt: remindAt ?? DateTime.now().add(const Duration(hours: 1)), - createdAt: createdAt ?? DateTime.now(), + // Use a fixed instant when none is provided to keep tests repeatable + remindAt: remindAt ?? DateTime.utc(2020, 1, 1, 1), + createdAt: createdAt ?? DateTime.utc(2020, 1, 1),A single, hard-coded instant (or a fake clock injected via a helper) removes timing-related flakes without touching production code.
44-69:generateMessageRemindersstill relies onDateTime.now()Even if the individual helper uses a fixed instant, this bulk generator re-introduces non-determinism through
final now = DateTime.now();.Consider passing an optional
baseTimeparameter (defaulting to a constant) and re-using it for bothremindAt&createdAtcalculations. This keeps all produced reminders aligned to a predictable timeline and avoids follow-up assertions that silently depend on execution speed.
243-245: PreferisAtSameMomentAsover millisecond comparisonexpect( controller.value.asSuccess.items.first.remindAt?.millisecondsSinceEpoch, equals(newRemindAt.millisecondsSinceEpoch), );Using
DateTime.isAtSameMomentAs(newRemindAt)orexpect(remindAt, equals(newRemindAt))(if equality is overridden) reads clearer and avoids manual epoch math.packages/stream_chat/test/src/client/channel_test.dart (1)
4423-4431: Use a deterministic pump instead ofFuture.delayed(Duration.zero)Relying on
await Future.delayed(Duration.zero)to flush the micro-task queue is brittle and can introduce flaky tests under heavy load.
Preferawait pumpEventQueue();(frompackage:async/async.dart) orawait Future.microtask(() {})which is explicit and instantaneous.No functional breakage – just a robustness improvement.
Also applies to: 4529-4533, 4581-4584, 4639-4644, 4691-4694
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/stream_chat/lib/src/core/api/sort_order.dart(6 hunks)packages/stream_chat/test/fixtures/message_to_json.json(1 hunks)packages/stream_chat/test/src/client/channel_test.dart(2 hunks)packages/stream_chat/test/src/core/api/reminders_api_test.dart(1 hunks)packages/stream_chat_flutter_core/test/stream_message_reminder_list_controller_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/stream_chat/test/fixtures/message_to_json.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_chat/lib/src/core/api/sort_order.dart
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: analyze_legacy_versions
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter_core
- GitHub Check: build (android)
- GitHub Check: stream_chat_persistence
- GitHub Check: build (ios)
- GitHub Check: analyze
- GitHub Check: test
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat
🔇 Additional comments (9)
packages/stream_chat_flutter_core/test/stream_message_reminder_list_controller_test.dart (1)
218-221: Loading failure stores error inside aSuccessstate – confirm intent
loadMorepropagates aStreamChatError, but the test assertsexpect(controller.value.isSuccess, isTrue); expect(controller.value.asSuccess.error, equals(chatError));If the controller records failures as
Success + error, consumers must always inspect bothvalue.isSuccessandvalue.asSuccess.error, which is easy to miss.
Is this design deliberate? If an error occurs, returning anErrorstate (or a distinctPartialSuccess) would be clearer.Please double-check the underlying implementation to ensure this behaviour is intentional.
packages/stream_chat/test/src/core/api/reminders_api_test.dart (7)
23-28: Avoid the redundantlatekeyword and recreate the mock per test
late final client = MockHttpClient();creates a single mock instance shared by all tests and thelatemodifier is unnecessary when the variable is initialised immediately.
Sharing the mock across groups makes stubbing / interaction history bleed between tests and can lead to brittle failures.- late final client = MockHttpClient(); + late MockHttpClient client; ... setUp(() { - remindersApi = RemindersApi(client); + client = MockHttpClient(); // fresh mock – no residual stubbing + remindersApi = RemindersApi(client); });[ suggest_essential_refactor ]
53-66: Stubbing / verifying with rawjsonEncode({})is too strictThe production code only needs to send an empty body – it might pass
{},null, or already-encoded data.
Stubbing & verification that demands the exact encoded string ("{}") will break as soon as the implementation changes the encoding strategy while still being semantically correct.- when(() => client.post(path, data: jsonEncode({}))) + when(() => client.post(path, data: any(named: 'data'))) .thenAnswer((_) async => successResponse(path, data: { 'reminders': reminders.map((r) => r.toJson()).toList(), })); ... - verify(() => client.post(path, data: jsonEncode({}))).called(1); + verify(() => client.post(path, data: any(named: 'data'))).called(1);Apply the same pattern to the other tests that encode
{}.
This keeps the tests focused on behaviour rather than wire-format minutiae.
[ suggest_essential_refactor ]
92-108: Payload equality check couples the test to the exact serialization
expectedPayloadis built withjsonEncode({...})and then used for both stubbing and verification.
Any legitimate re-ordering of map keys or a future change to send the raw map (letting Dio encode) would break the test even though the request is still correct.Consider matching the Map instead of its encoded string or at least relaxing the verification to
any(named: 'data')and asserting the decoded map:late Map capturedPayload; when(() => client.post(path, data: captureAny(named: 'data'))) .thenAnswer((_) async => successResponse(path, data: {...})); // after the call expect(jsonDecode(capturedPayload), { 'filter': filter.toJson(), 'sort': sort.map((s) => s.toJson()).toList(), ...pagination.toJson(), });This keeps the test resilient while still validating the payload semantics.
[ suggest_optional_refactor ]
125-137: Missing assertion on theremindAtdefaultThe test verifies only
messageId. Given that the API should return aremindAt(even when the field isn’t provided by the client), add an expectation to ensure defaults are honoured:expect(res.reminder.remindAt, isNotNull, reason: 'API should set default remindAt');This guards against server/client regressions where
remindAtmay accidentally become nullable.
[ suggest_nitpick ]
174-200:updateReminderwithoutremindAt– same strict JSON issueAs with
createReminder, usingjsonEncode({})in the stub/verify is fragile. Replace withany(named: 'data')or capture & validate as a map to decouple the test from the exact encoding.
[ duplicate_comment ]
216-233: Repeated payload-encoding couplingThe encoded-string coupling happens here as well (update with
remindAt). Apply the same relaxed stubbing/verification strategy to keep the suite maintainable.
[ duplicate_comment ]
237-251: Consider verifying server response handling fordeleteReminderCurrently the test only asserts that the response is non-null.
Add an expectation that the SDK correctly interprets the empty body (or success flag) to make the test meaningful:expect(res.statusCode, 200);or whatever shape
DeleteReminderResponseexposes.
[ suggest_nitpick ]packages/stream_chat/test/src/client/channel_test.dart (1)
4375-4702: Event-driven reminder tests look comprehensive – nice coverage!
The added cases exercise creation, update and deletion for both channel and thread messages and validate state mutation – solid addition. 👍
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/stream_chat/test/src/core/models/message_reminder_test.dart (2)
83-97: Strengthen JSON-serialization assertionsThe current check only verifies the presence + type of the ISO strings. It would be safer to assert the exact value to catch accidental timezone / truncation regressions:
- expect(json['remind_at'], isA<String>()); - expect(json['created_at'], isA<String>()); - expect(json['updated_at'], isA<String>()); + expect(json['remind_at'], equals(remindAt.toIso8601String())); + expect(json['created_at'], equals(now.toIso8601String())); + expect(json['updated_at'], equals(now.toIso8601String()));This adds negligible maintenance cost while increasing confidence in the serializer.
268-293: Add negative-case coverage fortoJson()whenremindAt == null
getComparableFieldcases look good. A similar negative-case fortoJson()(bookmark reminder) would guaranteeremind_atis omitted whennull:final bookmarkJson = bookmarkReminder.toJson(); expect(bookmarkJson.containsKey('remind_at'), isFalse);Helps prevent accidental breaking API contracts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/stream_chat/test/fixtures/channel_state_to_json.json(25 hunks)packages/stream_chat/test/src/core/models/message_reminder_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/stream_chat/test/fixtures/channel_state_to_json.json
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: analyze
- GitHub Check: build (ios)
- GitHub Check: build (android)
- GitHub Check: test
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_persistence
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat
- GitHub Check: analyze_legacy_versions
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2269 +/- ##
==========================================
+ Coverage 63.29% 63.48% +0.19%
==========================================
Files 404 408 +4
Lines 25274 25510 +236
==========================================
+ Hits 15998 16196 +198
- Misses 9276 9314 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/stream_chat/test/src/core/api/sort_order_test.dart (1)
130-140: Custom comparator should honourdirectioninternallyThe custom comparator fully replaces the built-in direction handling.
If another developer passesSortOption.asc('name', comparator: …)expecting the comparator to be automatically reversed, they’ll get surprising results.Consider documenting this caveat in
SortOption’s API doc or enforcing that a custom comparator always receivesdirectionso callers can write:comparator: (a, b, dir) => dir * a.name!.length.compareTo(b.name!.length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/stream_chat/lib/src/core/api/sort_order.dart(6 hunks)packages/stream_chat/test/src/core/api/requests_test.dart(0 hunks)packages/stream_chat/test/src/core/api/sort_order_test.dart(1 hunks)sample_app/lib/widgets/channel_list.dart(1 hunks)
💤 Files with no reviewable changes (1)
- packages/stream_chat/test/src/core/api/requests_test.dart
✅ Files skipped from review due to trivial changes (1)
- sample_app/lib/widgets/channel_list.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_chat/lib/src/core/api/sort_order.dart
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: stream_chat_flutter
- GitHub Check: test
- GitHub Check: stream_chat_flutter_core
- GitHub Check: build (ios)
- GitHub Check: stream_chat
- GitHub Check: stream_chat_localizations
- GitHub Check: analyze
- GitHub Check: build (android)
- GitHub Check: format
- GitHub Check: stream_chat_persistence
- GitHub Check: analyze_legacy_versions
🔇 Additional comments (2)
packages/stream_chat/test/src/core/api/sort_order_test.dart (2)
56-60: Double-check defaultdirectionconstantThe test assumes that omitting the
directionargument defaults toSortOption.DESC.
If the underlying implementation ever flips the default (e.g. toASC, which is more common in public APIs), the test – and, more importantly, production code that relies on this behaviour – will silently become incorrect.Confirm in
sort_order.dartthat the unnamed constructor indeed setsdirection = DESC, and, if not, update either the tests or the implementation accordingly.
146-162: Nice explicitnullOrderingcoverageGreat to see dedicated tests ensuring
nullsLast/nullsFirstbehaviour – they guard against future regressions in a tricky corner-case.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/stream_chat_flutter_core/test/stream_message_reminder_list_controller_test.dart (2)
72-80: Share-nothing test isolation – create a fresh mock per test
final client = MockClient();is declared once at file scope and then reset intearDown().
While it works in serial execution, a shared mock can easily leak stubs or listeners if more tests are added or when tests start running in parallel isolates. A safer pattern is:late MockClient client; setUp(() { client = MockClient(); when(client.on).thenAnswer((_) => const Stream.empty()); });This guarantees a pristine mock for every test case.
88-104: Dispose the controller after each test to avoid dangling listeners
StreamMessageReminderListControllersubscribes to the client’s event stream; not disposing it leaves the subscription open and can leak memory or trigger unexpected callbacks in subsequent tests.
Wrap controller creation withaddTearDown:final controller = StreamMessageReminderListController(client: client); addTearDown(controller.dispose);Applying this to every test that instantiates the controller removes the need for manual cleanup and keeps the test suite hermetic.
Also applies to: 119-132, 153-165, 224-248, 278-301, 348-386, 415-447
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/stream_chat_flutter_core/test/stream_message_reminder_list_controller_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build (android)
- GitHub Check: test
- GitHub Check: analyze
- GitHub Check: format
- GitHub Check: analyze_legacy_versions
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_persistence
- GitHub Check: stream_chat
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter
packages/stream_chat_flutter_core/test/stream_message_reminder_list_controller_test.dart
Show resolved
Hide resolved
Submit a pull request
Linear: FLU-143
Docs PR
https://github.com/GetStream/docs-content/pull/380
Screenshots / Videos
Screen.Recording.2025-06-03.at.13.21.29.mov
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Documentation