refactor(ui)!: improve reaction bubble implementation #2277
Conversation
WalkthroughThis update introduces a comprehensive refactor and enhancement of the chat message reaction system. It adds new overlay widgets for reaction indicators and pickers, customizes layout and alignment logic, and introduces utility widgets for flexible sizing and size change detection. Legacy reaction indicator and alignment files are removed, and several callback and layout APIs are simplified or unified. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MessageWidget
participant ReactionIndicatorBubbleOverlay
participant ReactionPickerBubbleOverlay
participant ReactionBubbleOverlay
User->>MessageWidget: Tap on message/reaction
MessageWidget->>ReactionIndicatorBubbleOverlay: Display reaction indicator
MessageWidget->>ReactionPickerBubbleOverlay: Display reaction picker overlay (on action)
ReactionIndicatorBubbleOverlay->>ReactionBubbleOverlay: Position and render indicator bubble
ReactionPickerBubbleOverlay->>ReactionBubbleOverlay: Position and render picker bubble
ReactionPickerBubbleOverlay-->>User: User picks a reaction
ReactionPickerBubbleOverlay->>MessageWidget: Notify reaction picked
MessageWidget->>MessageWidget: Update message reactions
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 10
🔭 Outside diff range comments (2)
packages/stream_chat_flutter/lib/src/theme/stream_chat_theme.dart (1)
675-682: 💡 Verification agent❓ Verification inconclusive
getMessageThemeJavadoc contradicts implementation (logic inversion)The new implementation now returns
ownMessageThemewhenreverse == true, but the comment still claims the opposite, which will mislead maintainers and 3rd-party integrators.- /// If [reverse] is true, it returns the [otherMessageTheme], otherwise it - /// returns the [ownMessageTheme]. + /// If [reverse] is true, it returns the [ownMessageTheme], otherwise it + /// returns the [otherMessageTheme].Please update either the documentation or the code so they agree, and double-check all callers (e.g. tests in
message_reactions_modal_test.dartandmessage_actions_modal_test.dart) for unintended behavioural changes.
Update Javadoc in getMessageTheme to match implementation
The current documentation is inverted relative to the code, which returns
ownMessageThemewhenreverse == true. Please update the comment so it accurately reflects the logic:• File: packages/stream_chat_flutter/lib/src/theme/stream_chat_theme.dart (lines 675–682)
- /// If [reverse] is true, it returns the [otherMessageTheme], otherwise it - /// returns the [ownMessageTheme]. + /// If [reverse] is true, it returns the [ownMessageTheme], otherwise it + /// returns the [otherMessageTheme].• Double-check any callers (e.g. in
message_reactions_modal_test.dartandmessage_actions_modal_test.dart) to ensure they still behave as expected after this documentation update.packages/stream_chat_flutter/lib/src/message_modal/message_actions_modal.dart (1)
70-97:⚠️ Potential issue
showReactionPickerflag is ignored &themeis unused – functional regression.The previous implementation respected
showReactionPicker.
After refactor the picker overlay is always built, so callers cannot hide it.
Additionally,themeis now an unused local producing a linter warning.- final theme = StreamChatTheme.of(context); ... - return StreamMessageModal( - spacing: 4, - alignment: alignment, - headerBuilder: (context) => ReactionPickerBubbleOverlay( - message: message, - reverse: reverse, - anchorOffset: const Offset(0, -8), - onReactionPicked: onReactionPicked, - reactionPickerBuilder: reactionPickerBuilder, - child: IgnorePointer(child: messageWidget), - ), + if (!showReactionPicker) { + return StreamMessageModal( + spacing: 4, + alignment: alignment, + headerBuilder: (_) => IgnorePointer(child: messageWidget), + contentBuilder: _buildContent, + ); + } + + return StreamMessageModal( + spacing: 4, + alignment: alignment, + headerBuilder: (context) => ReactionPickerBubbleOverlay( + message: message, + reverse: reverse, + anchorOffset: const Offset(0, -8), + onReactionPicked: onReactionPicked, + reactionPickerBuilder: reactionPickerBuilder, + child: IgnorePointer(child: messageWidget), + ), contentBuilder: _buildContent, );(Extract
_buildContentto keep the diff small.)Fixing this restores expected behaviour and removes the dead
themevariable.
🧹 Nitpick comments (11)
packages/stream_chat_flutter/test/src/message_modal/message_actions_modal_test.dart (1)
234-257: NestedPortalroots – harmless but redundant
_wrapWithMaterialAppintroduces aPortalas the root, while widgets under test (StreamMessageActionsModal,…ReactionsModal,…BubbleOverlay) also create their ownPortalinstances.
flutter_portalsupports nesting, yet extra roots marginally affect layout passes.Not blocking, but you could drop the outer
Portaland rely on the widget-under-test to host overlays, simplifying the test tree.packages/stream_chat_flutter/test/src/message_modal/message_reactions_modal_test.dart (1)
247-276: Same observation as above – consider removing the top-levelPortalwrapper to keep the test harness minimal.packages/stream_chat/lib/src/core/models/reaction_group.dart (1)
37-41: Typo in documentation reference.
/// Creates a copy of [Reaction] with specified attributes overridden.→[ReactionGroup].
A small fix prevents IDE mis-references.packages/stream_chat_flutter/lib/src/message_widget/message_card.dart (1)
165-170: Avatar-overlap regression risk.Previous logic added start/end margin when an avatar was shown; the new logic gates margin only on
isFailedState. If the avatar is visible for successful messages the bubble may now clash with the avatar.Please verify layout for:
•showUserAvatar == DisplayWidget.gonevsDisplayWidget.show
•isFailedState == falseYou might want to re-introduce avatar-based spacing or make
12.0derived frommessageTheme.avatarSize.packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker.dart (1)
111-116: Padding additive logic may double custom horizontal padding.
padding.add(extraPadding)unconditionally addsextraPaddingwhen there is more than one icon.
If callers already pass horizontal padding (e.g.EdgeInsets.symmetric(horizontal: 8)), the result becomes 8 + 4 = 12 which could break pixel-perfect designs.Consider:
- padding: padding.add(extraPadding), + padding: EdgeInsets.only( + top: padding.top, + bottom: padding.bottom, + left: padding.left + (isSinglePickerIcon ? 0 : 4), + right: padding.right + (isSinglePickerIcon ? 0 : 4), + ),Or document that the widget adds 4 px horizontally when multiple icons are present.
packages/stream_chat_flutter/lib/src/reactions/indicator/reaction_indicator_bubble_overlay.dart (1)
47-50: Target alignment hard-codes1 / -1; considertextDirection.
AlignmentDirectional(reverse ? -1 : 1, -1)assumes LTR whenreverseis false. In RTL locales the bubble tail may attach to the wrong side. Derive alignment fromDirectionality.of(context)instead ofreverse.packages/stream_chat_flutter/lib/src/reactions/user_reactions.dart (1)
106-109: Lookup of reaction icons is O(N) on every build – consider caching or aMap.
firstWherescans the fullreactionIconslist each time_UserReactionItem.buildis executed.
With large custom icon sets or fast-scrolling reaction lists this quickly adds up.-final reactionIcon = reactionIcons.firstWhere( - (it) => it.type == reaction.type, - orElse: () => const StreamReactionIcon.unknown(), -); +// Pre–compute a `{type: icon}` map once in `StreamChatConfiguration` +// or at least convert locally before the loop: +final iconByType = { + for (final i in reactionIcons) i.type: i, +}; +final reactionIcon = iconByType[reaction.type] ?? + const StreamReactionIcon.unknown();packages/stream_chat_flutter/lib/src/reactions/indicator/reaction_indicator.dart (1)
70-78:isSingleIndicatorIconrecomputeslength– minor micro-optimisation.
indicatorIconsis anIterable;lengthtraverses it. Convert to aListonce and reuse.- final isSingleIndicatorIcon = indicatorIcons?.length == 1; - final extraPadding = switch (isSingleIndicatorIcon) { + final indicatorIconList = [...indicatorIcons]; + final isSingleIndicatorIcon = indicatorIconList.length == 1; + final extraPadding = switch (isSingleIndicatorIcon) { ... - final indicator = ReactionIndicatorIconList( - indicatorIcons: [...?indicatorIcons], + final indicator = ReactionIndicatorIconList( + indicatorIcons: indicatorIconList, );packages/stream_chat_flutter/lib/src/message_modal/message_reactions_modal.dart (1)
109-115: Minor – extract constant0.78or document its rationale.Hard-coded width factors sprinkled across modals make later tuning hard. Consider a named constant or exposing it via the theme/config.
packages/stream_chat_flutter/lib/src/message_widget/message_widget_content.dart (1)
426-438: Consider making the extension more specific to avoid conflicts.The extension on generic
Iterable<T>with a generic method nameaddConditionallycould potentially conflict with other extensions in the codebase or third-party packages.Consider one of these approaches:
- Make it a private extension specific to this file:
-extension<T> on Iterable<T> { +extension _MessageWidgetIterableExtension<T> on Iterable<T> {
- Or move it to a utility class:
class WidgetListUtils { static Iterable<T> addConditionally<T>( Iterable<T> items, T? item, { required bool Function(T element) condition, bool reverse = false, }) sync* { for (final element in items) { if (item != null && !reverse && condition(element)) yield item; yield element; if (item != null && reverse && condition(element)) yield item; } } }packages/stream_chat_flutter/lib/src/reactions/reaction_bubble_overlay.dart (1)
1-2: Consider removing unnecessary ignore comment.The
cascade_invocationslint ignore might not be necessary. If there are no cascade invocation warnings in this file, this comment should be removed.If the ignore is not needed, remove it:
-// ignore_for_file: cascade_invocations -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
packages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_dark.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_light.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_reversed_dark.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_reversed_light.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_reversed_with_reactions_dark.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_reversed_with_reactions_light.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_with_reactions_dark.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_with_reactions_light.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_reactions_modal_dark.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_reactions_modal_light.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_reactions_modal_reversed_dark.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_reactions_modal_reversed_light.pngis excluded by!**/*.png
📒 Files selected for processing (23)
packages/stream_chat/lib/src/core/models/reaction_group.dart(1 hunks)packages/stream_chat_flutter/lib/src/message_modal/message_actions_modal.dart(2 hunks)packages/stream_chat_flutter/lib/src/message_modal/message_reactions_modal.dart(2 hunks)packages/stream_chat_flutter/lib/src/message_widget/message_card.dart(1 hunks)packages/stream_chat_flutter/lib/src/message_widget/message_widget.dart(2 hunks)packages/stream_chat_flutter/lib/src/message_widget/message_widget_content.dart(4 hunks)packages/stream_chat_flutter/lib/src/misc/flexible_fractionally_sized_box.dart(1 hunks)packages/stream_chat_flutter/lib/src/misc/size_change_listener.dart(1 hunks)packages/stream_chat_flutter/lib/src/reactions/indicator/reaction_indicator.dart(1 hunks)packages/stream_chat_flutter/lib/src/reactions/indicator/reaction_indicator_bubble_overlay.dart(1 hunks)packages/stream_chat_flutter/lib/src/reactions/indicator/reaction_indicator_icon_list.dart(1 hunks)packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker.dart(3 hunks)packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker_bubble_overlay.dart(1 hunks)packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker_icon_list.dart(7 hunks)packages/stream_chat_flutter/lib/src/reactions/reaction_bubble_overlay.dart(1 hunks)packages/stream_chat_flutter/lib/src/reactions/reaction_indicator.dart(0 hunks)packages/stream_chat_flutter/lib/src/reactions/reactions_align.dart(0 hunks)packages/stream_chat_flutter/lib/src/reactions/user_reactions.dart(3 hunks)packages/stream_chat_flutter/lib/src/theme/stream_chat_theme.dart(1 hunks)packages/stream_chat_flutter/lib/stream_chat_flutter.dart(0 hunks)packages/stream_chat_flutter/test/src/message_modal/message_actions_modal_test.dart(3 hunks)packages/stream_chat_flutter/test/src/message_modal/message_reactions_modal_test.dart(3 hunks)packages/stream_chat_flutter/test/src/reactions/reaction_picker_icon_list_test.dart(4 hunks)
💤 Files with no reviewable changes (3)
- packages/stream_chat_flutter/lib/stream_chat_flutter.dart
- packages/stream_chat_flutter/lib/src/reactions/reaction_indicator.dart
- packages/stream_chat_flutter/lib/src/reactions/reactions_align.dart
🔇 Additional comments (5)
packages/stream_chat_flutter/test/src/reactions/reaction_picker_icon_list_test.dart (1)
62-66: Callback signature change – lost reaction context?
onPressedis nowVoidCallback; the reaction type is no longer passed to callers.
That’s fine for tests, but production code may still need the selected type. EnsureReactionIconButton/ReactionPickerIconListderive the type from internal state (e.g. theReactionPickerIcon) and surface it through higher-level callbacks (onReactionPicked).If that hand-off was removed unintentionally, re-introduce a typed callback to avoid extra look-ups.
packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker_bubble_overlay.dart (1)
1-61: Well-implemented reaction picker bubble overlay.The implementation is clean and follows Flutter best practices with proper documentation, const constructors, and appropriate theme usage.
packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker_icon_list.dart (1)
10-10: Excellent API simplification and improvements.The changes improve the API design:
- Using
ValueSetter<Reaction>aligns with Flutter conventions- Simplified
VoidCallbackremoves unnecessary parameters- Private
_builderwith publicbuild()method provides better encapsulation- Icon size property allows flexible sizing
Also applies to: 28-32, 185-191, 225-243, 263-279
packages/stream_chat_flutter/lib/src/reactions/reaction_bubble_overlay.dart (1)
279-371: Well-implemented custom painter with good performance considerations.The
ReactionBubblePainterimplementation is excellent:
- Efficient path combination for bubble and tail circles
- Proper use of
RepaintBoundaryin the default builder- Clean separation of mask, border, and fill painting
packages/stream_chat_flutter/lib/src/message_widget/message_widget.dart (1)
710-713: 👍 Direction-aware alignment is a solid improvementSwitching from
Alignment.centerRight/LefttoAlignmentDirectional.centerEnd/Startautomatically respects the currentTextDirection, giving correct RTL support without additional code.
Looks good as-is.
packages/stream_chat_flutter/lib/src/reactions/indicator/reaction_indicator_bubble_overlay.dart
Show resolved
Hide resolved
packages/stream_chat_flutter/lib/src/reactions/indicator/reaction_indicator.dart
Show resolved
Hide resolved
packages/stream_chat_flutter/lib/src/message_modal/message_reactions_modal.dart
Show resolved
Hide resolved
packages/stream_chat_flutter/lib/src/misc/flexible_fractionally_sized_box.dart
Show resolved
Hide resolved
packages/stream_chat_flutter/lib/src/reactions/indicator/reaction_indicator_icon_list.dart
Outdated
Show resolved
Hide resolved
packages/stream_chat_flutter/lib/src/reactions/indicator/reaction_indicator_icon_list.dart
Outdated
Show resolved
Hide resolved
packages/stream_chat_flutter/lib/src/message_widget/message_widget_content.dart
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v10.0.0 #2277 +/- ##
==========================================
Coverage ? 63.57%
==========================================
Files ? 404
Lines ? 25274
Branches ? 0
==========================================
Hits ? 16067
Misses ? 9207
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/stream_chat_flutter/lib/src/message_widget/message_widget_content.dart (1)
264-266:Rowstill usesCrossAxisAlignment.end; previous feedback unresolved.
endbreaks the expected baseline alignment for incoming messages – please revert tostart.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_dark.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_light.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_reversed_dark.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_actions_modal_reversed_light.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_reactions_modal_dark.pngis excluded by!**/*.pngpackages/stream_chat_flutter/test/src/message_modal/goldens/ci/stream_message_reactions_modal_light.pngis excluded by!**/*.png
📒 Files selected for processing (3)
packages/stream_chat_flutter/CHANGELOG.md(1 hunks)packages/stream_chat_flutter/lib/src/message_widget/message_widget.dart(2 hunks)packages/stream_chat_flutter/lib/src/message_widget/message_widget_content.dart(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/stream_chat_flutter/CHANGELOG.md
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/stream_chat_flutter/lib/src/message_widget/message_widget_content.dart
[warning] 309-313: packages/stream_chat_flutter/lib/src/message_widget/message_widget_content.dart#L309-L313
Added lines #L309 - L313 were not covered by tests
[warning] 350-354: packages/stream_chat_flutter/lib/src/message_widget/message_widget_content.dart#L350-L354
Added lines #L350 - L354 were not covered by tests
[warning] 431-431: packages/stream_chat_flutter/lib/src/message_widget/message_widget_content.dart#L431
Added line #L431 was not covered by tests
packages/stream_chat_flutter/lib/src/message_widget/message_widget.dart
[warning] 742-746: packages/stream_chat_flutter/lib/src/message_widget/message_widget.dart#L742-L746
Added lines #L742 - L746 were not covered by tests
🔇 Additional comments (1)
packages/stream_chat_flutter/lib/src/message_widget/message_widget.dart (1)
699-707: ValidatewidthFactorbefore passing it toFlexibleFractionallySizedBox.
widthFactoris forwarded directly without guarding against> 1,< 0, orNaN.
IfFlexibleFractionallySizedBoxhasn’t yet implemented the tighter assertions/clamping requested in previous reviews, an out-of-range value will overflow the layout in release builds.child: FlexibleFractionallySizedBox( - widthFactor: widget.widthFactor, + // Ensure safe range [0, 1]. + widthFactor: widget.widthFactor.clamp(0.0, 1.0),Alternatively, add a runtime assert in the widget’s constructor.
packages/stream_chat_flutter/lib/src/message_widget/message_widget_content.dart
Show resolved
Hide resolved
.../src/message_modal/goldens/ci/stream_message_actions_modal_reversed_with_reactions_light.png
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Tests