refactor(llc)!: unify comment data model and add tests #64
Conversation
- refactor: Replaced `ThreadedCommentData` with a unified `CommentData` model to handle both flat and threaded comments. This simplifies the data model by adding a `replies` field to `CommentData`. - feat: Added `ActivityCommentList` and `CommentReplyList` for managing and displaying comments and their replies, along with comprehensive tests for API operations and real-time event handling. - feat: Implemented mutation extension methods on `ActivityData` and `CommentData` for immutably handling updates, additions, and deletions of comments, reactions, and bookmarks. - test: Added extensive test suites for `Activity`, `ActivityList`, `ActivityCommentList`, and `CommentReplyList` to ensure correctness of state management and event handling. - chore: Updated `stream_core` dependency to `^0.3.1`.
WalkthroughConsolidates threaded and flat comments by removing ThreadedCommentData and expanding CommentData; adds mutation helpers (updateWith/upsert/remove) across ActivityData, CommentData, FeedsReactionData, and PollData; updates state/event layers to pass activity/comment context for reactions/bookmarks; adds ReactionIcon and UI wiring; bumps stream_core to ^0.3.1. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sample_app/lib/screens/user_feed/comment/user_comments.dart (1)
271-277: Deletion error is silently ignored.The
deleteCommentcall is not awaited, so if deletion fails, the dialog closes without any error indication. The user would see the comment still present with no feedback about what went wrong.Consider awaiting the deletion and handling potential errors:
if (canDelete) SimpleDialogOption( child: const Text('Delete'), - onPressed: () { - activity.deleteComment(comment.id); + onPressed: () async { Navigator.pop(context); + try { + await activity.deleteComment(comment.id); + } catch (e) { + if (context.mounted) { + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar(content: Text('Failed to delete comment')), + ); + } + } }, ),Alternatively, close the dialog after successful deletion to provide visual confirmation.
🧹 Nitpick comments (11)
sample_app/lib/screens/user_feed/user_feed_screen.dart (1)
67-79: Stories feed init path is now disabled; verify behavior and consider removing dead/commented codeBy commenting out both the initial
storiesFeed.getOrCreate()+_followSelfIfNeededcall (lines 67‑69) and thefeedFromQuery(...).getOrCreate()block (lines 72‑79), you’ve changed when/how the story feeds are created and wired:
- The
storiesFeedinstance is still used throughout the UI and disposed indispose, but no longer guaranteed to exist or follow the user’s own story feed at startup.- The only remaining explicit creation is in
_showCreateActivityBottomSheet, and only when an activity is added that targetsFeedId.story(client.user.id).If the newer
stream_feeds/ backend behavior now auto‑creates story feeds and handles the self‑follow wiring, this is fine, but then this commented‑out setup becomes dead code and should either be deleted or replaced with a short comment explaining why manual initialization is no longer required.If auto‑creation/self‑follow are not guaranteed, you may want to reintroduce a minimal init path (e.g., just
storiesFeed.getOrCreate()or a one‑time follow) or move that logic to a more appropriate place (such as the first time stories are displayed).Please double‑check the current
stream_feeds0.4.0 story feed semantics (auto‑creation and follow behavior) and confirm that removing this initialization won’t regress the “first stories” experience for new users.sample_app/lib/screens/user_feed/feed/reaction_icon.dart (1)
49-64: Consider usingAppColorTokensfor consistency.The
defaultReactionslist uses hardcoded color values (0xFFE91E63,0xFFFF5722) while the rest of the app usesAppColorTokensfor theming. This could lead to inconsistency if the color palette is updated. Consider importing and usingAppColorTokensor making these colors configurable at runtime based on theme.That said, since these are semantic reaction-specific colors that may intentionally differ from the app's accent colors, keeping them hardcoded is also a valid approach.
sample_app/lib/screens/user_feed/comment/user_comments_item.dart (1)
88-111: Inconsistent fallback color between heart and fire buttons.The heart button uses
nullfor the non-highlighted state (line 96), while the fire button explicitly usescontext.appColors.textLowEmphasis(line 109). This creates a visual inconsistency where both buttons should behave the same when not selected.Consider aligning the fallback colors:
ActionButton( icon: Icon( switch (hasOwnHeart) { true => Icons.favorite_rounded, false => Icons.favorite_outline_rounded, }, ), count: heartsCount, - color: hasOwnHeart ? context.appColors.accentError : null, + color: hasOwnHeart + ? context.appColors.accentError + : context.appColors.textLowEmphasis, onTap: () => onReactionClick(comment, 'heart', !hasOwnHeart), ),Alternatively, if
nullis the intended default forActionButton, update the fire button to usenullas well.sample_app/lib/screens/user_feed/feed/user_feed_item.dart (1)
233-248: Consider removing the redundant parameter.The
onReactionClickparameter is already available as a class field on_UserActions(line 194). The method can accessthis.onReactionClickdirectly.- Iterable<Widget> _buildReactions( - BuildContext context, { - ValueSetter<ReactionIcon>? onReactionClick, - }) sync* { + Iterable<Widget> _buildReactions(BuildContext context) sync* { for (final reaction in ReactionIcon.defaultReactions) { final count = data.reactionGroups[reaction.type]?.count ?? 0; final selected = data.ownReactions.any((it) => it.type == reaction.type); yield ActionButton( icon: Icon(reaction.getIcon(selected)), count: count, color: reaction.getColor(selected), onTap: () => onReactionClick?.call(reaction), ); } }And update the call site at line 211:
- ..._buildReactions(context, onReactionClick: onReactionClick), + ..._buildReactions(context),packages/stream_feeds/lib/src/models/poll_data.dart (1)
179-212: Consider consolidatingaddOptionandupdateOption.Both methods perform an upsert by ID -
addOptionusesupsert()which already handles both add and update cases, whileupdateOptionmanually maps and replaces. These could be unified:- PollData updateOption(PollOptionData option) { - final updatedOptions = options.map((it) { - if (it.id != option.id) return it; - return option; - }).toList(); - - return copyWith(options: updatedOptions); - } + /// Alias for [addOption] - updates an existing option by ID. + PollData updateOption(PollOptionData option) => addOption(option);Alternatively, if the semantic distinction is intentional (e.g.,
updateOptionshould fail silently when option doesn't exist), document that difference explicitly.packages/stream_feeds/lib/src/state/event/feed_event_handler.dart (1)
173-183: Acknowledged: TODO comments for activity filter matching.The TODO comments on lines 175 and 181 indicate that activity filter matching for
CommentUpdatedEventandCommentDeletedEventis pending until the event payload includes the activity data. This is acceptable technical debt as long as it's tracked.Would you like me to open an issue to track implementing the activity filter check once the event includes the activity data?
packages/stream_feeds/lib/src/state/activity_state.dart (1)
202-206: Update documentation to reflect the unified model.The doc comment mentions "threaded comments" but the type is now the unified
CommentDatamodel. Consider updating the documentation for consistency./// The comments associated with this activity. /// - /// Contains a list of threaded comments related to the activity. + /// Contains a list of comments related to the activity. @override final List<CommentData> comments;packages/stream_feeds/test/state/activity_list_test.dart (1)
323-324: Minor: Remove shadowed constant.
activityIdis already defined at line 7 with the same value. This inner declaration shadows the outer one unnecessarily.group('Activity feedback', () { - const activityId = 'activity-1'; - activityListTest(packages/stream_feeds/test/test_utils/testers/activity_comment_list_tester.dart (1)
99-147: Consider renaming helper function for consistency with the CommentData migration.The
get()method usescreateDefaultThreadedCommentResponsewhich references the oldThreadedCommentDatanaming convention. Since the PR unifies the data model toCommentData, consider renaming this helper tocreateDefaultCommentResponsefor consistency.- createDefaultThreadedCommentResponse( + createDefaultCommentResponse( id: 'comment-1', objectId: query.objectId, objectType: query.objectType, text: 'First comment', userId: 'user-1', ), - createDefaultThreadedCommentResponse( + createDefaultCommentResponse( id: 'comment-2', ... ), - createDefaultThreadedCommentResponse( + createDefaultCommentResponse( id: 'comment-3', ... ),packages/stream_feeds/lib/src/state/activity_comment_list_state.dart (1)
86-98: UnusedhardDeleteparameter.The
hardDeleteparameter is declared but not used in the method body—the removal logic is identical regardless of its value. If this is intentional future-proofing, consider adding a TODO comment. Otherwise, remove the parameter to avoid confusion./// Handles the removal of a comment by ID. - void onCommentRemoved( - String commentId, { - bool hardDelete = false, - }) { + void onCommentRemoved(String commentId) { + // TODO: Handle soft-delete vs hard-delete distinction if needed final updatedComments = state.comments.removeNested(packages/stream_feeds/test/test_utils/fakes.dart (1)
326-343: Minor: Redundant null-coalescing for userId.The
userId ?? 'user-1'on line 339 is redundant sincecreateDefaultCommentResponsealready handlesnullwith the same default value. For consistency withcreateDefaultAddCommentResponseandcreateDefaultUpdateCommentResponse, you could passuserIddirectly.comment: createDefaultCommentResponse( id: commentId, objectId: objectId, objectType: objectType, - userId: userId ?? 'user-1', + userId: userId, ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (45)
melos.yaml(1 hunks)packages/stream_feeds/lib/src/models.dart(0 hunks)packages/stream_feeds/lib/src/models/activity_data.dart(1 hunks)packages/stream_feeds/lib/src/models/comment_data.dart(3 hunks)packages/stream_feeds/lib/src/models/feeds_reaction_data.dart(2 hunks)packages/stream_feeds/lib/src/models/poll_data.dart(3 hunks)packages/stream_feeds/lib/src/models/threaded_comment_data.dart(0 hunks)packages/stream_feeds/lib/src/models/threaded_comment_data.freezed.dart(0 hunks)packages/stream_feeds/lib/src/repository/comments_repository.dart(8 hunks)packages/stream_feeds/lib/src/state/activity.dart(6 hunks)packages/stream_feeds/lib/src/state/activity_comment_list.dart(3 hunks)packages/stream_feeds/lib/src/state/activity_comment_list_state.dart(6 hunks)packages/stream_feeds/lib/src/state/activity_comment_list_state.freezed.dart(3 hunks)packages/stream_feeds/lib/src/state/activity_list_state.dart(3 hunks)packages/stream_feeds/lib/src/state/activity_state.dart(2 hunks)packages/stream_feeds/lib/src/state/activity_state.freezed.dart(3 hunks)packages/stream_feeds/lib/src/state/comment_reply_list.dart(4 hunks)packages/stream_feeds/lib/src/state/comment_reply_list_state.dart(6 hunks)packages/stream_feeds/lib/src/state/comment_reply_list_state.freezed.dart(3 hunks)packages/stream_feeds/lib/src/state/event/activity_comment_list_event_handler.dart(2 hunks)packages/stream_feeds/lib/src/state/event/activity_list_event_handler.dart(4 hunks)packages/stream_feeds/lib/src/state/event/comment_reply_list_event_handler.dart(1 hunks)packages/stream_feeds/lib/src/state/event/feed_event_handler.dart(3 hunks)packages/stream_feeds/lib/src/state/feed.dart(5 hunks)packages/stream_feeds/lib/src/state/feed_state.dart(4 hunks)packages/stream_feeds/pubspec.yaml(1 hunks)packages/stream_feeds/test/state/activity_comment_list_test.dart(1 hunks)packages/stream_feeds/test/state/activity_list_test.dart(11 hunks)packages/stream_feeds/test/state/activity_test.dart(2 hunks)packages/stream_feeds/test/state/comment_reply_list_test.dart(1 hunks)packages/stream_feeds/test/state/feed_test.dart(1 hunks)packages/stream_feeds/test/test_utils.dart(1 hunks)packages/stream_feeds/test/test_utils/event_types.dart(1 hunks)packages/stream_feeds/test/test_utils/fakes.dart(8 hunks)packages/stream_feeds/test/test_utils/testers/activity_comment_list_tester.dart(1 hunks)packages/stream_feeds/test/test_utils/testers/activity_list_tester.dart(1 hunks)packages/stream_feeds/test/test_utils/testers/comment_reply_list_tester.dart(1 hunks)sample_app/lib/screens/user_feed/comment/user_comments.dart(6 hunks)sample_app/lib/screens/user_feed/comment/user_comments_item.dart(4 hunks)sample_app/lib/screens/user_feed/feed/reaction_icon.dart(1 hunks)sample_app/lib/screens/user_feed/feed/user_feed.dart(7 hunks)sample_app/lib/screens/user_feed/feed/user_feed_item.dart(8 hunks)sample_app/lib/screens/user_feed/user_feed_screen.dart(1 hunks)sample_app/lib/theme/schemes/app_color_scheme.dart(9 hunks)sample_app/lib/theme/tokens/color_tokens.dart(1 hunks)
💤 Files with no reviewable changes (3)
- packages/stream_feeds/lib/src/models.dart
- packages/stream_feeds/lib/src/models/threaded_comment_data.freezed.dart
- packages/stream_feeds/lib/src/models/threaded_comment_data.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). (3)
- GitHub Check: analyze
- GitHub Check: stream_feeds
- GitHub Check: build
🔇 Additional comments (114)
packages/stream_feeds/test/test_utils/event_types.dart (1)
22-28: Comment event constants are consistent and well‑namedThe added
commentDeletedand comment‑reaction event constants follow the existing event naming pattern and keep the API surface clear and predictable. No changes needed here.sample_app/lib/theme/tokens/color_tokens.dart (1)
39-42: LGTM!The new orange color tokens follow the established pattern for semantic colors with light/dark variants (500/600). The naming and hex values are consistent with the existing color scale conventions.
sample_app/lib/theme/schemes/app_color_scheme.dart (1)
27-27: LGTM!The
accentWarningfield is consistently integrated across all necessary locations: constructor, factory methods, field declaration,copyWith,lerp, equality operator, andhashCode. The implementation follows the established patterns for other accent colors.Also applies to: 48-48, 70-70, 122-123, 150-150, 168-168, 190-190, 213-213, 234-234
sample_app/lib/screens/user_feed/feed/reaction_icon.dart (1)
67-74: LGTM!The extension methods provide clean helper functions for icon/color selection based on highlight state. The null return for non-highlighted color allows the caller to provide a contextual fallback.
sample_app/lib/screens/user_feed/feed/user_feed.dart (4)
57-65: LGTM!The callback wiring is clean and properly separates concerns. The unified reaction pathway through
onReactionClickalongside dedicated handlers for repost and bookmark actions provides a clear and maintainable API.
118-144: LGTM!The reaction toggle logic correctly:
- Checks ownership via
ownReactions.any()to determine add vs delete- Uses
enforceUnique: trueto prevent duplicate reactions- Conditionally includes
emoji_codein custom data only when availableThe pattern is clean and follows the delete-if-exists, add-otherwise idiom.
156-164: LGTM!The bookmark toggle logic is straightforward and correct - checking
ownBookmarks.isNotEmptyto determine the appropriate action.
183-188: LGTM!The callback type signatures are clear and properly typed. Using
void Function(ActivityData, ReactionIcon)?for the reaction callback allows passing both the activity context and the reaction type, which is cleaner than multiple separate callbacks.sample_app/lib/screens/user_feed/comment/user_comments_item.dart (2)
38-66: LGTM!The Row-based layout cleanly separates the avatar from the content area, with the
GestureDetectorappropriately wrapping only the comment content for long-press actions. This provides a better UX where tapping the avatar won't accidentally trigger the long-press action.
116-125: LGTM!The recursive rendering of child replies correctly propagates
onReactionClickand other callbacks to nestedUserCommentItemwidgets, ensuring consistent behavior throughout the comment tree.sample_app/lib/screens/user_feed/feed/user_feed_item.dart (2)
13-13: LGTM! Clean migration to generalized reaction system.The refactoring from heart-specific logic to a generalized
ReactionIcon-based system is well-structured. The callback type changes (onReactionClick, simplifiedonRepostClick) are consistent throughout the widget hierarchy.Also applies to: 25-26, 38-39, 64-64
200-231: LGTM!The bookmark state check and action button layout are correctly implemented. The dynamic reaction rendering integrates cleanly with existing UI elements.
sample_app/lib/screens/user_feed/comment/user_comments.dart (7)
141-187: LGTM! Clean migration to CommentData.The list building logic correctly uses
CommentDataand properly wires up the newonReactionClickandonLongPressCommentcallbacks.
207-224: LGTM!The generalized reaction handler correctly distinguishes between adding and removing reactions, with
enforceUnique: trueensuring proper toggle behavior.
226-248: LGTM!The reply flow cleanly handles both top-level comments and nested replies, with dynamic title generation and proper
parentIdpropagation.
290-326: LGTM!The edit flow properly awaits both the dialog and the update operation. The
_displayTextInputDialogmethod cleanly encapsulates dialog presentation with optional parent comment context.
345-354: LGTM! Proper controller lifecycle management.The
TextEditingControlleris correctly initialized and disposed.
371-388: LGTM! Good use of ValueListenableBuilder for reactive button state.Using the controller's value listenable to enable/disable the submit button based on text content is a clean approach that avoids unnecessary rebuilds.
398-476: LGTM! Well-structured dialog content.The conditional parent comment preview and TextField configuration with reasonable limits (
maxLines: 5,maxLength: 300) provide a good user experience for both new comments and replies.melos.yaml (1)
51-51: Consistent with package-level dependency.The melos bootstrap dependency correctly mirrors the version in
packages/stream_feeds/pubspec.yaml.packages/stream_feeds/lib/src/state/feed.dart (2)
254-257: Documentation improvements look good.Clearer description of the bookmark folder handling behavior.
269-271: Documentation updates are consistent and helpful.The updated docs clearly explain the folder-related behaviors for
updateBookmarkanddeleteBookmark.Also applies to: 286-286, 296-299
packages/stream_feeds/lib/src/state/comment_reply_list_state.freezed.dart (1)
1-86: Generated file reflects source model changes correctly.This Freezed-generated file correctly reflects the
ThreadedCommentData→CommentDatamigration from the source file. Ensuremelos run generate:dartwas executed to regenerate this file.packages/stream_feeds/lib/src/models/poll_data.dart (2)
153-171: Well-documented mutation extension with clear immutability semantics.The
updateWithmethod correctly preservesownVotesAndAnswerswhen merging WebSocket event data, preventing loss of user-specific state.
220-234: Answer casting logic is correct.The method properly scopes
ownVotesAndAnswersupdates to the current user while universally updatinglatestAnswers.packages/stream_feeds/lib/src/state/event/feed_event_handler.dart (1)
86-101: LGTM - Reaction event handlers are well-structured.The new
ActivityReactionAddedEventandActivityReactionUpdatedEventhandlers follow a consistent pattern with the existing event handlers: fid check → filter validation → model conversion → state callback. The activity context is now properly passed alongside the reaction.packages/stream_feeds/lib/src/models/feeds_reaction_data.dart (2)
54-64: LGTM - Clean computed identity properties.The
idanduserReactionsGroupIdgetters provide a well-designed identity scheme:
userReactionsGroupIdcorrectly distinguishes between activity-level and comment-level reactionsidcombines type with the group ID for full uniquenessThe use of Dart 3's pattern matching (
if (commentId case final id?)) is idiomatic.
67-113: LGTM - Well-documented mutation extension.The
FeedsReactionListMutationextension provides clean upsert semantics with optional uniqueness enforcement. The_alwaysEqualComparatordefault maintains insertion order, which is appropriate for reactions without explicit ordering requirements. TheinsertUniqueandsortedUpsertmethods are properly available from thestream_coredependency (^0.3.1), which is imported at the top of the file.packages/stream_feeds/lib/src/models/comment_data.dart (4)
163-194: LGTM - Clean updateWith implementation.The conditional preservation logic is well-designed:
- Threaded comments (with replies loaded) only preserve
ownReactions- Non-threaded comments also preserve
metaandrepliesto avoid losing local stateThis correctly handles the WebSocket event case where user-specific data may not be reliable.
196-232: LGTM - Reply mutation methods.The
upsertReplyandremoveReplymethods correctly:
- Safely handle null
replieswith spread operator on nullable list- Recalculate
replyCountbased on actual list size changes- Use
math.max(0, ...)to prevent negative counts
261-292: Verify asymmetry between upsertReaction and removeReaction.There's an asymmetry in how these methods access
ownReactions:
upsertReaction(line 267) usesupdatedComment.ownReactionsremoveReaction(line 286) usesthis.ownReactionsIf this is intentional (upsert merges server state, remove uses local state), consider adding a brief comment explaining the design choice for future maintainers.
336-374: LGTM - ThreadedCommentResponseMapper extension.The mapper correctly handles all fields including
metaandreplies(with recursive mapping). The structure mirrorsCommentResponseMapperappropriately while adding the threaded-specific fields.packages/stream_feeds/lib/src/models/activity_data.dart (4)
319-331: LGTM - Immutable updateWith pattern.The
updateWithmethod correctly:
- Preserves user-specific data (
ownBookmarks,ownReactions) that may not be reliable from WebSocket events- Uses the null-aware
letoperator withpoll?.updateWithfor nested immutable updates
339-371: LGTM - Comment mutation methods.The
upsertCommentandremoveCommentmethods follow the same well-designed pattern asCommentData.upsertReply/removeReply:
- Calculate count difference from actual list changes
- Use
math.max(0, ...)to prevent negative counts- Return new immutable instances via
copyWith
379-407: LGTM - Bookmark mutation methods.The bookmark methods correctly:
- Guard against updating for non-current users
- Use
bookmark.activityto get updated activity data with server-side counts- Preserve computed
ownBookmarksthroughupdateWith
416-467: LGTM - Reaction mutation methods.The reaction methods follow a consistent pattern:
- Guard against updating for non-current users
- Compute updated
ownReactionsfrom current state- Use
updateWithto merge with server activity data while preserving local user dataThe
enforceUniqueoption viaupsertUniqueReactionprovides flexibility for different reaction behaviors.packages/stream_feeds/test/test_utils/testers/activity_list_tester.dart (1)
104-110: LGTM - Improved test data construction.Good refactor to use centralized
createDefaultQueryActivitiesResponseandcreateDefaultActivityResponsehelpers. This improves consistency across tests and makes the default dataset easier to maintain. Adding a third activity expands pagination/state merging test coverage.packages/stream_feeds/test/test_utils.dart (1)
4-10: LGTM!The new test utility exports for
activity_comment_list_tester.dartandcomment_reply_list_tester.dartare correctly placed in alphabetical order and follow the existing naming conventions.packages/stream_feeds/lib/src/state/activity_comment_list.dart (2)
7-7: LGTM!Import correctly updated to use the unified
CommentDatamodel.
61-62: LGTM!The return type migration from
ThreadedCommentDatatoCommentDatais consistently applied across all public and internal methods (get(),queryMoreComments(),_queryComments()). The API surface is cohesive.Also applies to: 70-70, 90-90
packages/stream_feeds/lib/src/state/activity_state.freezed.dart (1)
1-104: Generated code correctly reflects source model changes.This freezed-generated file properly reflects the
List<CommentData>type change from the sourceactivity_state.dart. No manual modifications needed.packages/stream_feeds/lib/src/state/activity_state.dart (1)
6-6: LGTM!Import correctly updated to use the unified
CommentDatamodel.packages/stream_feeds/lib/src/state/activity_comment_list_state.freezed.dart (1)
1-86: Generated code correctly reflects source model changes.This freezed-generated file properly reflects the
List<CommentData>type change from the sourceactivity_comment_list_state.dart. No manual modifications needed.packages/stream_feeds/lib/src/state/comment_reply_list.dart (3)
7-7: LGTM!Import correctly updated to use the unified
CommentDatamodel.
30-34: LGTM!The
parentCommentIdparameter is correctly passed fromquery.commentIdto the state notifier, enabling proper filtering of reply events for this specific comment.
64-64: LGTM!The return type migration from
ThreadedCommentDatatoCommentDatais consistently applied across all public and internal methods. The API surface is cohesive with the unified comment model.Also applies to: 70-70, 90-90
packages/stream_feeds/test/state/activity_list_test.dart (6)
7-11: LGTM!Good practice extracting common test constants at the top level for reuse across test cases.
401-507: LGTM!Comprehensive test coverage for query operations including initial query, pagination with
queryMoreActivities, and handling of empty results when no more data is available. The tests properly verify both the returned results and state mutations.
513-610: LGTM!Event handling tests thoroughly cover the activity lifecycle events (
ActivityAddedEvent,ActivityUpdatedEvent,ActivityDeletedEvent) with proper state verification before and after each event.
616-710: LGTM!Reaction event tests properly verify the add/remove flows for
ActivityReactionAddedEventandActivityReactionDeletedEvent, including state mutations onownReactions.
716-793: LGTM!Bookmark event tests properly verify the add/remove flows for
BookmarkAddedEventandBookmarkDeletedEvent, including state mutations onownBookmarks.
799-943: LGTM!Comment event tests comprehensively cover
CommentAddedEvent,CommentUpdatedEvent, andCommentDeletedEventwith proper verification of comment text, count, and state mutations. This aligns well with the unifiedCommentDatamodel introduced in this PR.packages/stream_feeds/test/state/activity_comment_list_test.dart (4)
1-16: Well-structured test constants and imports.The test setup is clean with meaningful constant names. The query definition is appropriately scoped at the top level for reuse across test groups.
21-156: Comprehensive query operation tests with good pagination coverage.The tests effectively validate initial fetch, pagination merging, and the empty result case. The verification callbacks ensure API interactions are correct.
162-327: Thorough event handling tests covering full comment lifecycle.The tests cover add, update, and delete events with proper state verification. The non-matching objectId test ensures events for other activities are correctly filtered out.
333-505: Solid reaction event handling tests.The tests properly verify that reactions are added to and removed from the
ownReactionslist, and that events for non-matching activities are correctly ignored.packages/stream_feeds/test/state/comment_reply_list_test.dart (4)
1-16: Well-defined test constants with clear naming.The constants clearly distinguish between
commentId,parentCommentId, andreplyId, making the nested comment structure easy to follow.
21-158: Query operations follow established patterns with proper pagination handling.The tests mirror the activity comment list structure, ensuring consistent behavior across both comment contexts.
359-408: Excellent nested reply handling test.This test validates that when a
CommentAddedEventhas aparentIdmatching an existing reply's ID, the new comment is correctly nested within that reply'sreplieslist. This is critical for proper threaded comment display.
691-756: Nested reply reaction handling is correctly tested.The test verifies that reactions can be added to deeply nested replies, with the state update correctly propagating through the reply hierarchy.
packages/stream_feeds/test/state/activity_test.dart (4)
384-433: Comprehensive addComment test with proper API verification.The test correctly mocks the API call using
any(named: 'addCommentRequest'), validates the result, and verifies state updates. The verification callback ensures the API was called correctly.
469-521: Important idempotency test for combined API and event handling.This test validates that when both an API call and a real-time event occur for the same comment, the state correctly handles deduplication and doesn't create duplicate entries. This is critical for real-world scenarios where events may arrive during or after API calls.
849-851: Proper mocktail fallback registration.The
setUpAllcorrectly registers a fallback value forAddCommentReactionRequest, which is required for mocktail'sany()matcher to work with non-primitive types.
968-1040: Good idempotency test for reaction operations.Similar to the comment tests, this validates that combined API + event scenarios don't result in duplicate reactions, ensuring state consistency.
packages/stream_feeds/lib/src/state/event/activity_list_event_handler.dart (3)
43-53: Correct implementation of ActivityAddedEvent handling.The handler correctly converts the event activity to a model, applies the query filter, updates state, and fetches updated feed capabilities. This mirrors the existing
ActivityUpdatedEventpattern.
74-105: Reaction handlers correctly pass activity context for per-activity state updates.The handlers now consistently pass both
activityandreactionto the state notifier methods, enabling accurate per-activity reaction tracking. The filter check ensures activities that no longer match are removed rather than updated.
137-140: TODO is appropriately documented for future API enhancement.The comment correctly notes the limitation that
CommentUpdatedEventdoesn't include activity data for filter matching. This is consistent with theCommentDeletedEventhandler and is a reasonable trade-off pending API changes.packages/stream_feeds/lib/src/state/activity_list_state.dart (3)
109-122: Clean delegation pattern for comment operations.The
onCommentAddeddelegation toonCommentUpdatedensures consistent upsert behavior for both add and update operations, aligning with the immutable state update pattern.
136-174: Well-designed reaction mutation methods with proper semantics.The distinction between
upsertReaction(foronReactionAdded) andupsertUniqueReaction(foronReactionUpdated) correctly handles different reaction update scenarios:
- Added: May add a new reaction alongside existing ones
- Updated: Replaces existing reaction with same identity
The methods consistently pass the activity context for accurate per-activity state mutations.
86-106: Bookmark operations correctly scoped to current user.The
upsertBookmarkandremoveBookmarkmethods properly passcurrentUserIdto enable per-user bookmark tracking, maintaining consistency with the reaction methods.packages/stream_feeds/test/state/feed_test.dart (6)
952-1013: LGTM! Well-structured bookmark API tests.The test properly validates the add bookmark flow via API, checking initial state, API response, and state updates. Good use of mock setup and verification.
1054-1105: Good deduplication test for API + event scenario.This test validates an important edge case where both API response and real-time event arrive, ensuring the bookmark isn't duplicated. This is critical for preventing state inconsistencies.
1201-1204: Good documentation of missing event handler.The comment clarifies why
BookmarkUpdatedEventisn't tested here and provides context for future implementation.
1373-1375: Fallback registration placed correctly in setUpAll.The
registerFallbackValueforAddReactionRequestis properly scoped to the Reactions group, preventing pollution of other test groups.
1611-1706: Comprehensive multi-reaction type test.This test validates an important scenario where multiple reaction types (heart, fire) coexist on the same activity. The assertions properly verify both
ownReactionsandreactionGroupsare updated correctly.
1708-1788: Good test for selective reaction removal.This test ensures removing one reaction type preserves others, validating the reaction state is properly managed per-type rather than as a single unit.
packages/stream_feeds/lib/src/state/event/comment_reply_list_event_handler.dart (1)
20-46: Clean refactor to CommentData model.The event handler now consistently extracts
CommentDataviatoModel()before passing to state methods. The pattern is uniform across all event types (add, update, reaction added/removed), making the code predictable and maintainable.packages/stream_feeds/lib/src/state/event/activity_comment_list_event_handler.dart (2)
42-49: Improved consistency in CommentDeletedEvent handling.Good refactor to extract
CommentDataviatoModel()first, then use its properties for validation. This aligns with the pattern used in other event handlers and avoids accessing raw response properties directly.
61-69: New CommentReactionUpdatedEvent handler added.This handler follows the established pattern and provides support for reaction updates on comments, which was likely missing before this refactor.
packages/stream_feeds/lib/src/repository/comments_repository.dart (2)
102-124: Well-implemented batch comment addition.The
addCommentsBatchmethod properly:
- Processes attachments for all requests via
processRequestsBatch- Maps to API request format
- Returns list of
CommentDatamodelsThe async chain with
flatMapAsynchandles the sequential async operations cleanly.
174-190: Good use of Dart 3 records for composite return types.Returning
({CommentData comment, FeedsReactionData reaction})is a clean pattern that avoids creating a dedicated class for this one-off composite return type. This provides named access to both fields while maintaining type safety.packages/stream_feeds/test/test_utils/testers/comment_reply_list_tester.dart (3)
42-66: Well-documented test helper function.The
commentReplyListTestfunction follows the established pattern from other tester utilities. Good use of@isTestannotation and comprehensive parameter documentation with example usage.
156-174: Clean tester factory with proper resource cleanup.The factory properly registers teardown via
test.addTearDown(subject.dispose)ensuring theCommentReplyListis disposed after each test, preventing resource leaks.
106-130: No action needed. The functioncreateDefaultThreadedCommentResponseis correctly named—it creates and returns aThreadedCommentResponseAPI response object. The naming reflects the response type, not the internal data model. The PR's migration fromThreadedCommentDatatoCommentDatais a separate concern and does not affect API response naming. Renaming this function would introduce ambiguity and require unnecessary updates across 40+ test usages.Likely an incorrect or invalid review comment.
packages/stream_feeds/lib/src/state/feed_state.dart (4)
122-140: LGTM! Clean update pattern for activity merging.The
updatecallback usingupdateWithproperly merges existing activity state with incoming updates, preserving local state while applying server updates. The same pattern correctly applies to pinned activities.
255-281: LGTM! User-scoped bookmark operations.The updated documentation and implementation correctly scope bookmark operations to
currentUserId, ensuring only the current user's bookmarks are managed. The symmetry betweenupsertBookmarkandremoveBookmarkis well maintained.
283-308: LGTM! Clean delegation pattern for comment handling.Delegating
onCommentAddedtoonCommentUpdatedis a good approach—both operations semantically perform an upsert on the activity's comments. This reduces code duplication while maintaining the distinct public API surface for callers.
374-414: LGTM! Context-aware reaction handling.The updated signatures correctly pass
ActivityDataalongsideFeedsReactionData, enabling the mutation methods (upsertReaction,upsertUniqueReaction,removeReaction) to properly merge reaction counts and user reactions with full context. The distinction betweenonReactionAddedandonReactionUpdated(unique vs non-unique reactions) is correctly maintained.packages/stream_feeds/test/test_utils/testers/activity_comment_list_tester.dart (2)
41-65: LGTM! Well-structured test helper with proper lifecycle management.The
activityCommentListTestfunction provides a clean abstraction for testing activity comment list operations with proper setup, body, verify, and teardown hooks. The delegation totestWithTesterkeeps the implementation DRY.
154-172: LGTM! Proper resource cleanup.The tester creation correctly registers
subject.disposeas a teardown action, ensuring the activity comment list is properly disposed after each test.packages/stream_feeds/lib/src/state/activity.dart (5)
139-152: LGTM! Clean API surface update.The return types correctly reflect the migration from
ThreadedCommentDatatoCommentData, maintaining a consistent public API surface for comment queries.
168-178: LGTM! Simplified callback.The direct method reference
_commentsList.notifier.onCommentAddedis cleaner than the previous wrapper. Good simplification.
180-193: LGTM! Idiomatic batch handling.Using
forEachwith the method reference for batch comment addition is clean and idiomatic Dart.
234-260: LGTM! Correct enforceUnique handling.The branching logic correctly distinguishes between:
enforceUnique: true→ callsonCommentReactionUpdated(replaces existing reaction of same type)enforceUnique: false(or null) → callsonCommentReactionAdded(adds new reaction)This ensures the state notifier receives the appropriate signal for how to update the reaction counts and user reactions.
265-282: LGTM! Consistent use of pair.comment.Using
pair.commentinstead of a separatecommentIdmaintains consistency with theaddCommentReactionpattern and provides the full comment context for state updates.packages/stream_feeds/lib/src/state/comment_reply_list_state.dart (5)
16-28: LGTM! parentCommentId enables correct reply hierarchy handling.Adding
parentCommentIdto the notifier allows distinguishing between top-level replies (direct children of the parent comment) and nested replies (children of other replies), which is essential for the updatedonCommentAddedlogic.
50-79: LGTM! Clear three-way branching for reply placement.The logic correctly handles:
- Comments without
parentIdare ignored (not replies)- Comments with
parentId == parentCommentIdare top-level replies—added directly to state- All other comments are nested replies—upserted within their parent reply
This properly maintains the reply hierarchy.
92-104: LGTM! Consistent update pattern.Using
updateWithfor merging comment updates maintains consistency with the broader refactoring pattern across the codebase.
106-156: LGTM! Consistent reaction handling pattern.The three reaction handlers (
onCommentReactionAdded,onCommentReactionUpdated,onCommentReactionRemoved) follow the same pattern asActivityCommentListStateNotifier, ensuring consistent behavior across different comment list contexts. The use ofcurrentUserIdfor user-scoped operations is correctly applied.
163-187: LGTM! Type migration complete.The
repliesfield correctly usesList<CommentData>instead of the previousList<ThreadedCommentData>, completing the unified data model migration for this state class.packages/stream_feeds/lib/src/state/activity_comment_list_state.dart (5)
28-46: LGTM! Consistent type migration.The
PaginationResult<CommentData>type correctly reflects the unified data model. The merge operation maintains sort order as expected.
48-72: LGTM! Proper nested comment handling.The branching logic correctly handles reply insertion:
- Replies with a
parentIdare nested within their parent usingupsertReply- Top-level comments are added directly to the comments list
The use of
sortedUpsertmaintains proper ordering.
74-84: LGTM! Consistent updateWith pattern.Using
updateWithfor comment updates aligns with the mutation extension pattern introduced in this PR.
100-147: LGTM! Complete reaction handler suite.The three reaction handlers are well-structured with consistent patterns:
onCommentReactionAddedusesupsertReactiononCommentReactionUpdatedusesupsertUniqueReactiononCommentReactionRemovedusesremoveReactionAll correctly scope operations to
currentUserId.
154-178: LGTM! Type migration complete.The
commentsfield correctly usesList<CommentData>, completing the unified data model migration for the activity comment list state.packages/stream_feeds/test/test_utils/fakes.dart (9)
5-42: LGTM!The three new factory functions follow a consistent pattern with sensible defaults. Using
const []for default list parameters is appropriate for Dart's immutable defaults.
83-132: LGTM!Good approach to derive
bookmarkCount,commentCount, andreactionCountfrom the actual collections. This ensures test data consistency and avoids mismatches between counts and collection sizes.
134-167: LGTM!The
voteCountcalculation usingsumOfis consistent with the pattern used elsewhere and ensures the count matches actual vote data.
229-255: LGTM!Adding the optional
userIdparameter with a sensible default provides flexibility for tests while maintaining backward compatibility.
257-286: LGTM!The
replyCountderivation fromreplies.lengthensures data consistency. Settingrepliestonullwhen empty correctly models API responses where empty collections may be omitted.
345-395: LGTM!Both reaction response factories follow a consistent pattern. Using
DateTime.timestamp()for test timestamps is appropriate for mock data generation.
413-427: LGTM!Adding the
activityTypeparameter provides additional flexibility for tests to customize the bookmarked activity's type.
429-472: LGTM!The three bookmark response factories follow a consistent pattern, properly delegating to
createDefaultBookmarkResponse.
474-508: LGTM!Both reaction response factories are well-structured. The naming clearly distinguishes these activity-level reactions from the comment reaction factories defined earlier.
This commit refactors the handling of comments and replies within the `ActivityCommentListState` and `CommentReplyListState`. Key changes: - Renamed methods in `CommentReplyListState` from `onComment...` to `onReply...` for clarity. - Updated `onCommentRemoved` and `onReplyRemoved` methods to accept a `CommentData` object instead of just an ID, enabling more robust removal logic for nested replies. - Added extensive tests for nested and deep-nested reply scenarios, including additions, updates, deletions, and reactions. - `CommentReplyListState` now explicitly ignores events for top-level comments (where `parentId` is null) to prevent incorrect state updates.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_feeds/lib/src/state/activity.dart (1)
200-212: Critical: Type mismatch in deleteComment callback.The
deleteCommentmethod returnsResult<void>, but line 209 passesonCommentRemovedtoonSuccess. However,onCommentRemovedexpects aCommentDataparameter (as seen inactivity_comment_list_state.dartline 91), notvoid. This will cause a compilation error.You'll need to either:
- Capture the comment before deletion and pass it in the closure
- Change
onCommentRemovedto accept just the comment ID- Update the repository to return the deleted comment
Apply this diff to capture the comment before deletion:
Future<Result<void>> deleteComment( String commentId, { bool? hardDelete, }) async { + // Capture the comment before deletion for state updates + final commentToDelete = _commentsList.state.comments + .firstWhereOrNull((c) => c.id == commentId); + final result = await commentsRepository.deleteComment( commentId, hardDelete: hardDelete, ); - result.onSuccess(_commentsList.notifier.onCommentRemoved); + if (commentToDelete != null) { + result.onSuccess((_) => _commentsList.notifier.onCommentRemoved(commentToDelete)); + } return result; }Note: You'll need to add the
collectionpackage import forfirstWhereOrNullor implement an alternative lookup.
🧹 Nitpick comments (1)
packages/stream_feeds/lib/src/state/activity.dart (1)
188-190: Consider simplifying the callback.The nested closure with
forEachworks but could be more concise.Apply this diff for a cleaner implementation:
- result.onSuccess( - (comments) => comments.forEach(_commentsList.notifier.onCommentAdded), - ); + result.onSuccess((comments) { + for (final comment in comments) { + _commentsList.notifier.onCommentAdded(comment); + } + });Or even simpler if you prefer functional style:
- result.onSuccess( - (comments) => comments.forEach(_commentsList.notifier.onCommentAdded), - ); + result.onSuccess((comments) { + comments.forEach(_commentsList.notifier.onCommentAdded); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
packages/stream_feeds/lib/src/models/comment_data.dart(3 hunks)packages/stream_feeds/lib/src/models/feeds_reaction_data.dart(2 hunks)packages/stream_feeds/lib/src/repository/comments_repository.dart(9 hunks)packages/stream_feeds/lib/src/state/activity.dart(7 hunks)packages/stream_feeds/lib/src/state/activity_comment_list_state.dart(5 hunks)packages/stream_feeds/lib/src/state/comment_reply_list_state.dart(5 hunks)packages/stream_feeds/lib/src/state/event/activity_comment_list_event_handler.dart(1 hunks)packages/stream_feeds/lib/src/state/event/comment_reply_list_event_handler.dart(1 hunks)packages/stream_feeds/test/state/activity_comment_list_test.dart(1 hunks)packages/stream_feeds/test/state/comment_reply_list_test.dart(1 hunks)packages/stream_feeds/test/test_utils/fakes.dart(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_feeds/test/state/activity_comment_list_test.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). (2)
- GitHub Check: stream_feeds
- GitHub Check: build
🔇 Additional comments (30)
packages/stream_feeds/test/state/comment_reply_list_test.dart (3)
1-16: Well-structured test suite with comprehensive coverage.The test file is well-organized with clear separation of concerns across three groups: Query Operations, Event Handling, and Reply Reactions. The constants and query setup at the top provide good reusability.
164-199: Good event handling test coverage.The event handling tests thoroughly cover CommentAddedEvent scenarios including parent ID matching, nested replies, and filtering of top-level comments. The state assertions properly verify the expected behavior.
889-942: Thorough reaction event handling tests.The reaction tests comprehensively cover addition, deletion, and update scenarios for reactions on replies at various nesting levels. The filtering logic for top-level comments is also well-tested.
packages/stream_feeds/lib/src/models/feeds_reaction_data.dart (2)
54-63: Well-designed reaction identity and grouping logic.The
idanduserReactionsGroupIdproperties provide clear separation of concerns:iduniquely identifies a reaction (type + user context), whileuserReactionsGroupIdenables grouping for unique enforcement. The conditional inclusion ofcommentIdcorrectly differentiates activity-level vs comment-level reactions.
66-95: Clean implementation of reaction upsert with unique enforcement.The extension correctly implements both unique (one reaction per user) and non-unique (multiple reactions allowed) modes. The
_alwaysEqualComparatordefault places new items at the beginning, which is appropriate for showing the most recent reactions first.packages/stream_feeds/lib/src/models/comment_data.dart (4)
173-194: Core fix for the race condition issue.The
updateWithmethod correctly preservesownReactionsfrom the current instance when not explicitly provided, addressing the PR's objective to prevent WebSocket events from overwriting user-specific fields with stale data. The distinction between threaded and non-threaded comments ensures appropriate field preservation.
202-221: Correct immutable reply upsert with automatic count management.The method properly uses
sortedUpsertwith an update callback that preserves existing reply data, and correctly recalculatesreplyCountwith a floor of 0.
229-240: LGTM!The reply removal logic is straightforward and correctly handles count recalculation with appropriate bounds checking.
344-381: Complete ThreadedCommentResponse mapper with recursive reply handling.The mapper correctly converts all fields and handles nested replies through recursive mapping. This enables the unified
CommentDatamodel to represent both flat and threaded comment structures.packages/stream_feeds/test/test_utils/fakes.dart (4)
5-42: Well-parameterized response factories for flexible test setup.The refactored factories with optional parameters for pagination cursors and collections provide good flexibility for test scenarios while maintaining sensible defaults.
83-132: Consistent derived counts in ActivityResponse factory.The factory correctly derives
bookmarkCount,commentCount, andreactionCountfrom the provided collections, ensuring test data consistency.
259-288: ThreadedCommentResponse factory with proper reply handling.The factory correctly derives
replyCountfrom the replies list and uses the null-if-empty pattern for replies, which aligns with theisThreadedproperty logic inCommentData.
290-510: Comprehensive set of response factories for testing CRUD operations.The factories for add/update/delete operations on comments, reactions, and bookmarks follow consistent patterns and provide good coverage for test scenarios.
packages/stream_feeds/lib/src/repository/comments_repository.dart (4)
58-81: API returns unified CommentData model.The
getCommentsmethod correctly returnsPaginationResult<CommentData>, aligning with the PR's objective to unify the comment data model.
102-124: Clean batch comment addition implementation.The
addCommentsBatchmethod follows established patterns with proper error handling viaflatMapAsyncand consistent response mapping.
133-143: Improved deleteComment return type.Returning
CommentDatainstead ofvoidenables callers to properly update state with the deleted comment's information, which is essential for real-time UI updates.
177-216: Record return types enable proper state merging.The reaction methods now return both the updated comment and the reaction data as a record, enabling callers to properly merge state and avoid the race condition described in the PR objectives.
packages/stream_feeds/lib/src/state/activity.dart (2)
141-142: LGTM: Return type updated consistently.The return type correctly reflects the migration from
ThreadedCommentDatatoCommentData.
244-256: LGTM: Proper handling of unique reactions.The conditional logic correctly uses
onCommentReactionUpdatedfor unique reactions andonCommentReactionAddedfor non-unique reactions. This aligns with the PR objective of preventing race conditions when multiple reactions are added.packages/stream_feeds/lib/src/state/event/activity_comment_list_event_handler.dart (2)
26-33: LGTM: Consistent event handling pattern.The handler correctly extracts the comment model, validates the scope (objectId/objectType), and passes the full
CommentDatato the state notifier. This pattern supports better merge logic and helps prevent race conditions.
63-71: LGTM: New reaction updated event handler.The
CommentReactionUpdatedEventhandler properly supports the unique reaction update flow, which aligns with the PR objective of handling sequential WebSocket events correctly.packages/stream_feeds/lib/src/state/event/comment_reply_list_event_handler.dart (2)
22-27: LGTM: Proper reply filtering.The
parentId != nullcheck correctly ensures that only replies (not top-level comments) are processed by this handler. The renamed methodonReplyAddedmakes the intent clearer.
51-57: LGTM: Reaction updated handler for replies.The new
onReplyReactionUpdatedhandler properly supports unique reaction updates for nested replies, consistent with the broader refactoring.packages/stream_feeds/lib/src/state/comment_reply_list_state.dart (4)
21-25: LGTM: Required field for reply context.The
parentCommentIdfield is essential for correctly identifying top-level replies vs nested replies in the mutation logic.
51-79: LGTM: Robust reply insertion logic.The method correctly distinguishes between:
- Top-level replies (direct children of the parent comment) - lines 57-66
- Nested replies (children of other replies) - lines 69-78
The use of
updateWithfor merging existing data helps preserve user-specific fields (ownReactions, ownBookmarks) when handling out-of-order events, addressing the race condition mentioned in issue #57.
96-118: LGTM: Complete removal logic.The method correctly handles removal from both top-level replies and nested replies, with proper fallback logic.
121-153: LGTM: Proper reaction state management.The distinction between
upsertReaction(line 129) andupsertUniqueReaction(line 147) correctly handles:
- Non-unique reactions: adds to the list
- Unique reactions: replaces existing reaction of same type
This prevents race conditions when rapid sequential reactions are added, directly addressing issue #57.
packages/stream_feeds/lib/src/state/activity_comment_list_state.dart (3)
29-46: LGTM: Consistent merge logic.The query result merging properly maintains sort order and uses identity-based deduplication via
key: (it) => it.id.
49-75: LGTM: Correct comment vs reply distinction.The logic correctly handles:
- Top-level comments (parentId == null) - lines 53-62
- Replies to comments (parentId != null) - lines 65-74
The use of
updateWithfor merging helps preserve user-specific fields when processing out-of-order events.
116-146: LGTM: Consistent reaction handling.The reaction mutation logic mirrors the reply list implementation:
upsertReactionfor non-unique reactions (line 123)upsertUniqueReactionfor unique reactions (line 140)This consistency across comment and reply lists ensures predictable behavior when handling reaction events.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/stream_feeds/test/state/comment_reply_list_test.dart (1)
104-120: Verify block uses stale pagination state.This issue was previously flagged. The
verifycallback runs afterbody, sotester.commentReplyListState.pagination?.nextisnullat this point (as asserted on line 98). ThenextPageQuery.nextwill benull, which doesn't match the actual API call made duringbodywhere it was'next-cursor'.Consider capturing the expected query values before the call or hardcoding the expected cursor value in the verification:
verify: (tester) { - final nextPageQuery = tester.commentReplyList.query.copyWith( - next: tester.commentReplyListState.pagination?.next, - ); - tester.verifyApi( (api) => api.getCommentReplies( - id: nextPageQuery.commentId, - depth: nextPageQuery.depth, - limit: nextPageQuery.limit, - next: nextPageQuery.next, - prev: nextPageQuery.previous, - repliesLimit: nextPageQuery.repliesLimit, - sort: nextPageQuery.sort, + id: query.commentId, + depth: query.depth, + limit: query.limit, + next: 'next-cursor', + prev: query.previous, + repliesLimit: query.repliesLimit, + sort: query.sort, ), ); },
🧹 Nitpick comments (1)
packages/stream_feeds/test/state/comment_reply_list_test.dart (1)
6-16: Consider adding error scenario and concurrency tests.The test suite is comprehensive for happy-path scenarios. Given the PR's objective to fix race conditions with
ownReactions, consider adding tests for:
- Error handling: What happens when
queryMoreRepliesAPI call fails?- Concurrent events: Simulating rapid sequential events (e.g., multiple
CommentReactionAddedEvents) to verify the race condition fix from issue #57.These would strengthen confidence that the race condition is resolved.
Would you like me to draft example test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/stream_feeds/test/state/activity_comment_list_test.dart(1 hunks)packages/stream_feeds/test/state/comment_reply_list_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_feeds/test/state/activity_comment_list_test.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). (2)
- GitHub Check: build
- GitHub Check: stream_feeds
🔇 Additional comments (3)
packages/stream_feeds/test/state/comment_reply_list_test.dart (3)
1-16: Well-structured test setup.The constants and query are appropriately scoped at the top level for reuse across all test groups. The naming is clear and follows conventions.
164-884: Comprehensive event handling test coverage.The tests thoroughly cover:
- Direct reply add/update/delete operations
- Nested and deep-nested reply handling
- Edge cases for non-matching parentIds and top-level comment filtering
The test structure is consistent and assertions are clear.
890-1480: Thorough reaction handling tests that support the PR objective.The tests comprehensively cover reaction add/update/delete scenarios at all nesting levels and correctly verify that
ownReactionsstate is properly maintained. This test coverage directly supports the PR's goal of fixing the race condition whereownReactionscould be reset when multiple reactions are added quickly.
xsahil03x
changed the title
The `ReactionIcon` widget and its related logic have been moved from the `feed` directory to the parent `user_feed` directory. This allows it to be reused across different features, such as `user_comments`. Imports and related code have been updated in `UserFeed`, `UserFeedItem`, `UserComments`, and `UserCommentsItem` to reflect this new, shared location. The reaction handling logic in `UserCommentsItem` has been refactored to use the shared `ReactionIcon` implementation, aligning it with `UserFeedItem`.
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)
sample_app/lib/screens/user_feed/comment/user_comments.dart (1)
278-285: Delete operation executes before dialog closes without awaiting.The
deleteCommentcall is fire-and-forget, andNavigator.popis called immediately after. If the delete fails, the user won't receive feedback. Consider awaiting the operation or using a loading indicator.if (canDelete) SimpleDialogOption( child: const Text('Delete'), - onPressed: () { - activity.deleteComment(comment.id); + onPressed: () async { Navigator.pop(context); + try { + await activity.deleteComment(comment.id); + } catch (e) { + // Show error feedback if mounted + } }, ),
🧹 Nitpick comments (6)
sample_app/lib/screens/user_feed/comment/user_comments_item.dart (1)
92-101: Consider potential performance impact with deeply nested replies.The recursive rendering of replies could cause performance issues with deeply nested comment trees, as each level creates new widget instances with the same callbacks. For typical use cases this is fine, but consider adding a depth limit or lazy loading for very deep threads if this becomes a concern.
sample_app/lib/screens/user_feed/feed/user_feed_item.dart (1)
233-251: Duplicate_buildReactionsimplementation.This generator is nearly identical to
_buildReactionsinuser_comments_item.dart. Consider extracting this into a shared utility function or widget to reduce duplication and ensure consistent behavior.Example shared helper:
// In a shared file, e.g., reaction_icon.dart or a new reactions_builder.dart Iterable<Widget> buildReactionButtons({ required Map<String, ReactionGroup> groups, required List<FeedsReactionData> ownReactions, required ValueSetter<ReactionIcon>? onReactionClick, }) sync* { for (final reaction in ReactionIcon.defaultReactions) { final count = groups[reaction.type]?.count ?? 0; final selected = ownReactions.any((it) => it.type == reaction.type); yield ActionButton( icon: Icon(reaction.getIcon(selected)), count: count, color: reaction.getColor(selected), onTap: () => onReactionClick?.call(reaction), ); } }sample_app/lib/screens/user_feed/feed/user_feed.dart (2)
118-144: Reaction toggle logic is correct but lacks error handling.The implementation correctly determines whether to delete or add a reaction based on
ownReactions. However, the returnedFutureis not awaited and errors are silently ignored. Consider adding error handling to provide user feedback on failure.- Future<void> _onReactionClick( + Future<void> _onReactionClick( ActivityData activity, ReactionIcon reaction, - ) { + ) async { final ownReactions = [...activity.ownReactions]; final shouldDelete = ownReactions.any((it) => it.type == reaction.type); - if (shouldDelete) { - return timelineFeed.deleteActivityReaction( - type: reaction.type, - activityId: activity.id, - ); + try { + if (shouldDelete) { + await timelineFeed.deleteActivityReaction( + type: reaction.type, + activityId: activity.id, + ); + } else { + await timelineFeed.addActivityReaction( + activityId: activity.id, + request: AddReactionRequest( + type: reaction.type, + enforceUnique: true, + createNotificationActivity: true, + custom: { + if (reaction.emojiCode case final code?) 'emoji_code': code, + }, + ), + ); + } + } catch (e) { + // Handle error (e.g., show snackbar) + debugPrint('Reaction operation failed: $e'); } - - return timelineFeed.addActivityReaction( - activityId: activity.id, - request: AddReactionRequest( - type: reaction.type, - enforceUnique: true, - createNotificationActivity: true, - custom: { - // Add emoji code only if available - if (reaction.emojiCode case final code?) 'emoji_code': code, - }, - ), - ); }
146-164: Bookmark and repost handlers follow the same fire-and-forget pattern.Same observation as with
_onReactionClick- consider adding error handling for a better user experience when operations fail.packages/stream_feeds/CHANGELOG.md (1)
1-4: Clarify whetherenforceUniqueis a breaking change in the changelogThe first bullet is tagged
[BREAKING], but theenforceUniqueline isn’t. If addingenforceUniquerequired a breaking API change (e.g., changing an existing method signature), consider tagging that line as[BREAKING]and briefly hinting at the API change; if it’s purely additive/optional, you might call that out explicitly so users know it’s backward-compatible.packages/stream_feeds/lib/src/models/comment_data.dart (1)
242-300: Reaction upsert/remove correctly preserve current user state across WS updatesThe reaction helpers use
this.ownReactionsas the base, restrict changes tocurrentUserId, and then feed the result intoupdateWith(updatedComment, ownReactions: ...). That pattern prevents incoming WS payloads from overwriting fresher localownReactions, which is exactly what’s needed to avoid the race/reset behavior described in the issue. The overall design looks consistent and safe.Optionally, you might tweak the
upsertUniqueReactiondoc to say it always enforces uniqueness (since this wrapper always passesenforceUnique: true), but that’s purely cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
packages/stream_feeds/CHANGELOG.md(1 hunks)packages/stream_feeds/lib/src/models/comment_data.dart(3 hunks)packages/stream_feeds/test/state/comment_reply_list_test.dart(1 hunks)sample_app/lib/screens/user_feed/comment/user_comments.dart(7 hunks)sample_app/lib/screens/user_feed/comment/user_comments_item.dart(4 hunks)sample_app/lib/screens/user_feed/feed/user_feed.dart(7 hunks)sample_app/lib/screens/user_feed/feed/user_feed_item.dart(8 hunks)sample_app/lib/screens/user_feed/reaction_icon.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_feeds/test/state/comment_reply_list_test.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). (2)
- GitHub Check: build
- GitHub Check: stream_feeds
🔇 Additional comments (13)
sample_app/lib/screens/user_feed/reaction_icon.dart (2)
8-65: Well-structured immutable data model.The
ReactionIconclass is well-designed with immutable fields, a const constructor, and a sensible default set of reactions. The use ofstatic constfordefaultReactionsensures compile-time efficiency.
67-74: Extension methods are clean and concise.The helper methods provide a clear abstraction for determining icon/color based on highlighted state.
sample_app/lib/screens/user_feed/comment/user_comments_item.dart (2)
21-24: API migration to generalized reaction system looks correct.The signature change from
onHeartClicktoonReactionClick(CommentData, ReactionIcon)properly supports multiple reaction types while maintaining type safety.
106-124: Clean generator function for building reactions.The
sync*generator efficiently yields reaction widgets without allocating an intermediate list. The logic correctly derivescountfromreactionGroupsandselectedstate fromownReactions.sample_app/lib/screens/user_feed/feed/user_feed_item.dart (1)
198-231: Reaction and bookmark UI logic is well-structured.The
_UserActionswidget cleanly separates concerns and the bookmark toggle logic using pattern matching is readable.sample_app/lib/screens/user_feed/feed/user_feed.dart (1)
57-65: Callback wiring is clean and consistent.The callbacks are properly wired to pass the activity context along with the reaction/action type.
sample_app/lib/screens/user_feed/comment/user_comments.dart (5)
208-231: Reaction toggle logic is consistent with the feed implementation.The pattern correctly handles adding/deleting reactions based on
ownReactionsstate. Same error handling considerations apply as noted foruser_feed.dart.
335-350: Well-designed input dialog widget.The
_CommentInputDialogis a clean, focused widget with proper separation of concerns. The StatefulWidget correctly manages theTextEditingControllerlifecycle.
378-394: Good use of ValueListenableBuilder for reactive button state.The positive action button reactively enables/disables based on input text, providing good UX feedback. The use of pattern matching for the
onPressedcallback is elegant.
455-481: TextField configuration is appropriate.The input field has sensible defaults:
maxLines: 5,maxLength: 300,autofocus: true, and dynamic hint text based on reply context. The styling is consistent with the app theme.
233-255: Reply handling with optional parentComment is well-implemented.The title dynamically reflects whether the user is adding a new comment or replying to an existing one. The
parentIdis correctly passed to theActivityAddCommentRequest.packages/stream_feeds/lib/src/models/comment_data.dart (2)
196-240: Reply upsert/remove helpers correctly maintain reply list and count
upsertReply/removeReplyusesortedUpsertplus a length-diff–based adjustment forreplyCount, clamped withmath.max(0, ...). Combined withexisting.updateWith(updated)for updates, this keeps reply ordering, avoids negative counts, and ensures nested comments benefit from the same merge semantics as roots. Looks solid.
344-382: ThreadedCommentResponseMapper mapping looks consistent with CommentDataThe threaded mapper cleanly mirrors
CommentResponseMapperwhile preservingmetaand mapping nestedrepliesintoList<CommentData>. Field coverage and null-handling look consistent with the unifiedCommentDatamodel and should work well for threaded trees.
| /// Extension functions for [CommentData] to handle common operations. | ||
| extension CommentDataMutations on CommentData { | ||
| /// Updates this comment with new data while preserving own reactions. | ||
| /// | ||
| /// Merges [updated] comment data with this instance, preserving [ownReactions] from | ||
| /// this instance when not provided. For threaded comments, also preserves meta and | ||
| /// replies data. This ensures that user-specific data is not lost when updating | ||
| /// from WebSocket events. | ||
| /// | ||
| /// Returns a new [CommentData] instance with the merged data. | ||
| CommentData updateWith( | ||
| CommentData updated, { | ||
| List<FeedsReactionData>? ownReactions, | ||
| }) { | ||
| // If the comment is threaded, only preserve own reactions. | ||
| if (updated.isThreaded) { | ||
| return updated.copyWith( | ||
| // Preserve own reactions from the current instance if not provided | ||
| // as they may not be reliable from WS events. | ||
| ownReactions: ownReactions ?? this.ownReactions, | ||
| ); | ||
| } | ||
|
|
||
| // For non-threaded comments, preserve meta and replies as well. | ||
| return updated.copyWith( | ||
| meta: meta, | ||
| replies: replies, | ||
| // Preserve own reactions from the current instance if not provided | ||
| // as they may not be reliable from WS events. | ||
| ownReactions: ownReactions ?? this.ownReactions, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
updateWith logic matches race-condition goal, but doc text is inverted
The implementation correctly uses the existing instance as source of truth for ownReactions (and for meta/replies in the non-threaded branch), which should prevent stale WS payloads from resetting user-specific state. However, the doc says “For threaded comments, also preserves meta and replies,” while the code preserves meta/replies only in the non-threaded branch and preserves just ownReactions when updated.isThreaded.
Consider aligning the comment with the actual behavior (non-threaded vs threaded) to avoid confusion.
🤖 Prompt for AI Agents
In packages/stream_feeds/lib/src/models/comment_data.dart around lines 163 to
195, the doc comment text is inverted: it currently states that meta and replies
are preserved for threaded comments while the code preserves meta/replies only
for non-threaded comments; update the documentation to accurately reflect the
implementation by changing the sentence to indicate that meta and replies are
preserved for non-threaded comments and that for threaded comments only
ownReactions are preserved (or otherwise rephrase to match the exact branch
behavior).
Submit a pull request
Fixes: #57
Description of the pull request
ThreadedCommentDatawith a unifiedCommentDatamodel to handle both flat and threaded comments. This simplifies the data model by adding arepliesfield toCommentData.ActivityCommentListandCommentReplyListfor managing and displaying comments and their replies, along with comprehensive tests for API operations and real-time event handling.ActivityDataandCommentDatafor immutably handling updates, additions, and deletions of comments, reactions, and bookmarks.Activity,ActivityList,ActivityCommentList, andCommentReplyListto ensure correctness of state management and event handling.stream_coredependency to^0.3.1.Summary by CodeRabbit
New Features
Breaking Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.