feat #142: create button builder callback to replace the submit button#174
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds ChangesbuttonBuilder feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BottomPicker
participant BottomPickerState
participant Navigator
User->>BottomPicker: buttonBuilder(instance, context)
BottomPicker->>BottomPicker: dismiss()
BottomPicker->>Navigator: pop() if mounted and canPop
BottomPickerState->>BottomPicker: dispose() sets disposed = true
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 5
🤖 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.
Inline comments:
In `@lib/bottom_picker.dart`:
- Around line 798-799: Consolidate the picker lifecycle into a single disposed
flag because BottomPicker and BottomPickerState currently maintain separate
disposed states that can drift apart. Update the dismissal and disposal flow in
BottomPickerState, including dismiss(), dispose(), and _onKeyPressed, to read
and write one source of truth instead of mixing widget.disposed and the State’s
own disposed field. Remove the redundant flag or delegate all checks to the same
field so escape-key dismissal, manual dismissals, and State.dispose() all
observe the same lifecycle state.
- Around line 827-834: The dismiss() method in BottomPicker is using a stored
_context without verifying it is still mounted before calling
Navigator.of(_context!).canPop(), which can crash if the context is stale.
Update dismiss() to first guard against _context being null and unmounted by
checking _context!.mounted before any Navigator lookup, then keep the existing
canPop() and disposed checks in place. Use the dismiss() method and the _context
field in BottomPicker to locate the fix.
- Around line 366-373: Update BottomPicker.range() so the submit validation no
longer always requires onRangeDateSubmitPressed when buttonBuilder is supplied;
adjust the assert in the BottomPicker constructor/initializer path to allow a
custom button to replace the default submit button, matching the existing
buttonBuilder callback flow. Use the BottomPicker.range() setup and the
assert(buttonBuilder != null || onRangeDateSubmitPressed != null) behavior as
the reference point, and keep the deprecated onRangeDateSubmitPressed
requirement only when no custom button is provided.
- Around line 798-804: The `_context` field in `BottomPicker` should not be a
late stored caller `BuildContext`, because `dismiss()` may access it before
`show()` initializes it and a retained external context can become invalid.
Update the `show()`/`dismiss()` flow to avoid holding `_context` directly in
`BottomPicker`; instead keep a navigator/route reference or add
mounted/unmounted guards around the `Navigator.of(...)` usage so dismissal does
not depend on a stale context.
In `@test/button_builder_test.dart`:
- Around line 1-117: Add coverage for the new BottomPicker lifecycle behavior in
the button builder test suite by creating tests around the
BottomPicker.dismiss() and disposed flow. Verify that calling dismiss() on an
active BottomPicker pops the route, and that calling it again after the picker
has already been dismissed does nothing/error-free. Use the existing
BottomPicker test setup as the entry point and reference the
BottomPicker.dismiss method and disposed state to locate the new assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 445c476e-ddb5-45da-8413-d73873a8eada
📒 Files selected for processing (4)
example/lib/main.dartlib/bottom_picker.dartlib/widgets/bottom_picker_button.darttest/button_builder_test.dart
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 (2)
lib/bottom_picker.dart (2)
738-744: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
buttonBuilderloses generic type info for consumers.The field is typed
Widget Function(BottomPicker instance, BuildContext context)?; bareBottomPickerresolves toBottomPicker<dynamic>, so callback consumers get an untypedinstanceinstead ofBottomPicker<T>.♻️ Proposed fix
- final Widget Function(BottomPicker instance, BuildContext context)? + final Widget Function(BottomPicker<T> instance, BuildContext context)? buttonBuilder;🤖 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 `@lib/bottom_picker.dart` around lines 738 - 744, The buttonBuilder callback is losing the picker’s generic type because its first parameter is declared as a bare BottomPicker, which erases T for consumers. Update the buttonBuilder field type so the instance parameter preserves the generic form used throughout BottomPicker<T>, and ensure any related callback signatures or uses of buttonBuilder reference the generic BottomPicker<T> consistently.
426-483: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame assert issue in
BottomPicker.rangeTime().
assert(onRangeTimeSubmitPressed != null);at line 475 has the identical problem flagged forBottomPicker.range(): it forces the deprecatedonRangeTimeSubmitPressedcallback to be non-null even whenbuttonBuilderis provided to fully replace the submit button.🐛 Proposed fix
- assert(onRangeTimeSubmitPressed != null); + assert(buttonBuilder != null || onRangeTimeSubmitPressed != null);🤖 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 `@lib/bottom_picker.dart` around lines 426 - 483, The same deprecated-callback assertion issue exists in BottomPicker.rangeTime(): the constructor currently requires onRangeTimeSubmitPressed even when buttonBuilder is used to fully handle submit actions. Update BottomPicker.rangeTime so the validation only enforces the deprecated callback when buttonBuilder is not provided, matching the behavior of the submit button replacement path and keeping the range-time API consistent with BottomPicker.range.
🤖 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 `@lib/bottom_picker.dart`:
- Around line 738-744: The buttonBuilder callback is losing the picker’s generic
type because its first parameter is declared as a bare BottomPicker, which
erases T for consumers. Update the buttonBuilder field type so the instance
parameter preserves the generic form used throughout BottomPicker<T>, and ensure
any related callback signatures or uses of buttonBuilder reference the generic
BottomPicker<T> consistently.
- Around line 426-483: The same deprecated-callback assertion issue exists in
BottomPicker.rangeTime(): the constructor currently requires
onRangeTimeSubmitPressed even when buttonBuilder is used to fully handle submit
actions. Update BottomPicker.rangeTime so the validation only enforces the
deprecated callback when buttonBuilder is not provided, matching the behavior of
the submit button replacement path and keeping the range-time API consistent
with BottomPicker.range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c619cb9-179c-409a-a17b-5de9428c3d98
📒 Files selected for processing (2)
analysis_options.yamllib/bottom_picker.dart
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
buttonBuilderoption to fully customize the submit button in bottom pickers.Changes
buttonBuilder.dismiss()method and safer lifecycle tracking.Tests