fix(ui): pass the correct BuildContext to onReactionPicked (#1234)#2823
fix(ui): pass the correct BuildContext to onReactionPicked (#1234)#2823w830207 wants to merge 8 commits into
onReactionPicked (#1234)#2823Conversation
…am#1234) * fix(ui): pass the correct BuildContext to `onReactionPicked` Pass the appropriate `BuildContext` to the `onReactionPicked` callback so navigation uses the correct context. * Update tests and usages to match the new signature.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe reaction picker adds a context-aware callback while retaining the deprecated callback. Modal integrations use the provided context when dismissing, and picker tests and the changelog reflect the updated behavior. ChangesReaction picker context callback
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker.dart (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify the
voidreturn type for strict typing.The previous
ValueSetter<Reaction>typedef inherently enforced avoidreturn type. Changing the field toFunction(Reaction, BuildContext)?makes the return typedynamic. It is recommended to explicitly declare thevoidreturn type for stricter type safety.♻️ Proposed refactor
- final Function(Reaction, BuildContext)? onReactionPicked; + final void Function(Reaction, BuildContext)? onReactionPicked;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker.dart` at line 35, Update the onReactionPicked callback type in ReactionPicker to explicitly return void while retaining its Reaction and BuildContext parameters and optionality, restoring the strict return-type contract previously provided by ValueSetter<Reaction>.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker.dart`:
- Line 35: Update the onReactionPicked callback type in ReactionPicker to
explicitly return void while retaining its Reaction and BuildContext parameters
and optionality, restoring the strict return-type contract previously provided
by ValueSetter<Reaction>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4648df1-b6f5-4a14-82cd-81da836ee0ab
📒 Files selected for processing (4)
packages/stream_chat_flutter/lib/src/message_modal/message_actions_modal.dartpackages/stream_chat_flutter/lib/src/message_widget/stream_message_item.dartpackages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker.dartpackages/stream_chat_flutter/test/src/reactions/picker/reaction_picker_test.dart
| final Message message; | ||
|
|
||
| /// {@macro onReactionPicked} | ||
| final OnReactionPicked? onReactionPicked; |
There was a problem hiding this comment.
This is a breaking change for anybody using the ReactionPicker directly. If the context is really needed we should make a new method and deprecate the old one.
There was a problem hiding this comment.
Thanks for the feedback. That's a great suggestion.
I can make this change. Do you have any recommendations for the new method name?
…etStream#1234)" This reverts commit 0f57a73.
* Deprecate `onReactionPicked` in favor of `onReactionSelected` to provide `BuildContext` for better navigation handling. * Update `StreamMessageItem` and `MessageActionsModal` to use the new `onReactionSelected` callback. * Add unit tests to verify `onReactionSelected` behavior and ensure backward compatibility with `onReactionPicked`.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker.dart (1)
6-9: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the legacy callback signature.
OnReactionPickedpreviously accepted(BuildContext, Reaction), so changing it toValueSetter<Reaction>makes existing direct picker consumers fail to compile. Keep the deprecated typedef and its invocations source-compatible; useonReactionSelectedas the new context-aware API. This is the breaking API concern previously raised.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker.dart` around lines 6 - 9, Restore OnReactionPicked to the legacy callback signature accepting BuildContext and Reaction, and retain it as a deprecated typedef for source compatibility. Update direct picker invocations to pass both arguments, while keeping onReactionSelected as the new context-aware API.
🧹 Nitpick comments (1)
packages/stream_chat_flutter/test/src/reactions/picker/reaction_picker_test.dart (1)
91-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the nested-navigator behavior.
This test ignores the supplied
BuildContext, so it passes even if the picker forwards a context from the wrong navigator. Add a nested modal/navigator case that callsNavigator.pop(callbackContext)and asserts the intended picker modal is dismissed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat_flutter/test/src/reactions/picker/reaction_picker_test.dart` around lines 91 - 113, Extend the reaction picker test around StreamMessageReactionPicker to use a nested navigator/modal and capture the callback context supplied to onReactionSelected. Invoke Navigator.pop with that callback context, then assert the intended picker modal is dismissed, ensuring the picker forwards the correct nested-navigator context rather than ignoring it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker.dart`:
- Around line 6-9: Restore OnReactionPicked to the legacy callback signature
accepting BuildContext and Reaction, and retain it as a deprecated typedef for
source compatibility. Update direct picker invocations to pass both arguments,
while keeping onReactionSelected as the new context-aware API.
---
Nitpick comments:
In
`@packages/stream_chat_flutter/test/src/reactions/picker/reaction_picker_test.dart`:
- Around line 91-113: Extend the reaction picker test around
StreamMessageReactionPicker to use a nested navigator/modal and capture the
callback context supplied to onReactionSelected. Invoke Navigator.pop with that
callback context, then assert the intended picker modal is dismissed, ensuring
the picker forwards the correct nested-navigator context rather than ignoring
it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c7ffde6-e8d8-4c0d-9c24-e942fa09036c
📒 Files selected for processing (4)
packages/stream_chat_flutter/lib/src/message_modal/message_actions_modal.dartpackages/stream_chat_flutter/lib/src/message_widget/stream_message_item.dartpackages/stream_chat_flutter/lib/src/reactions/picker/reaction_picker.dartpackages/stream_chat_flutter/test/src/reactions/picker/reaction_picker_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/stream_chat_flutter/lib/src/message_widget/stream_message_item.dart
- packages/stream_chat_flutter/lib/src/message_modal/message_actions_modal.dart
2 participants
onReactionPickedPass the appropriate
BuildContextto theonReactionPickedcallback so navigation uses the correct context.Submit a pull request
Linear: FLU-
Github Issue: #
CLA
Description of the pull request
This PR fixes an issue where
onReactionPickedcould receive an incorrectBuildContext.When the reaction picker is used within nested navigators, using the wrong
BuildContextmay cause navigation operations (such asNavigator.pop) to target the wrong navigator, resulting in the parent page being closed unexpectedly.This change passes the correct
BuildContexttoonReactionPicked, ensuring navigation is performed on the intended navigator.Changes included:
BuildContexttoonReactionPicked.BuildContext.Screenshots / Videos
Summary by CodeRabbit
onReactionSelected(BuildContext, Reaction)to the reaction picker for context-aware handling.onReactionPicked(Reaction)in favor ofonReactionSelected.