feat(ui): add optionsBuilder to custom attachment options #2363
Conversation
WalkthroughAdds an optionsBuilder API for attachment picker (mobile and web/desktop), wires it through the modal bottom sheet, introduces two typedefs, enforces exclusivity with customOptions, refactors default/final options computation, updates builder signatures, adds tests, and documents the change in the CHANGELOG. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Modal as showStreamAttachmentPickerModalBottomSheet
participant MobileBuilder as mobileAttachmentPickerBuilder
participant WebBuilder as webOrDesktopAttachmentPickerBuilder
participant Sheet as AttachmentPickerBottomSheet
App->>Modal: open(..., customOptions?, optionsBuilder?)
Note right of Modal: assert(!(customOptions && optionsBuilder))
alt optionsBuilder provided
Modal->>MobileBuilder: call(..., optionsBuilder: optionsBuilder)
Modal->>WebBuilder: call(..., optionsBuilder: wrapper(mapping default→webOption))
Note right of MobileBuilder: build defaultOptions\nfinalOptions = optionsBuilder(context, defaultOptions)
Note right of WebBuilder: build defaultOptions\nmapped = defaultOptions→webOptions\nfinalOptions = optionsBuilder(context, mapped)
else legacy path
Modal->>MobileBuilder: call(..., customOptions?)
Modal->>WebBuilder: call(..., customOptions?)
Note over MobileBuilder,WebBuilder: finalOptions = (customOptions? + defaultOptions)
end
MobileBuilder->>Sheet: present(finalOptions)
WebBuilder->>Sheet: present(finalOptions)
Sheet-->>App: selection or dismiss
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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.
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)
packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker.dart (1)
279-282: Null key will crash Set/hash operations
AttachmentPickerOption.hashCodecallskey.hashCodeunconditionally, butkeyis nullable. Several code paths convert lists toSet<AttachmentPickerOption>; if a developer returns an option without a key viaoptionsBuilder, this will throw.Make
hashCodenull-safe. This is critical given the new extension points.- @override - int get hashCode => - key.hashCode ^ const IterableEquality().hash(supportedTypes); + @override + int get hashCode => + (key?.hashCode ?? 0) ^ const IterableEquality().hash(supportedTypes);
🧹 Nitpick comments (10)
packages/stream_chat_flutter/CHANGELOG.md (1)
5-7: Clarify mutual exclusivity and tighten wordingPlease call out that
optionsBuilderandcustomOptionsare mutually exclusive (asserted in code) and thatoptionsBuildertakes precedence if both are provided in release mode. Also prefer “ordering and display” over the slashed phrasing.Proposed edit:
- - Added `optionsBuilder` to `showStreamAttachmentPickerModalBottomSheet`, - `mobileAttachmentPickerBuilder`, and `webOrDesktopAttachmentPickerBuilder` - to allow full control over the attachment picker options ordering / display. + - Added `optionsBuilder` to `showStreamAttachmentPickerModalBottomSheet`, + `mobileAttachmentPickerBuilder`, and `webOrDesktopAttachmentPickerBuilder`, + allowing full control over the attachment picker options ordering and display. + Note: `optionsBuilder` and `customOptions` are mutually exclusive. If both + are supplied (e.g., in release where asserts are disabled), `optionsBuilder` + takes precedence and `customOptions` are ignored.packages/stream_chat_flutter/test/src/message_input/attachment_picker/options_builder_test.dart (4)
65-89: Assert coverage on reordering is solid; consider adding a guard on key assumptionsThe test hard-codes
'gallery-picker'. That's fine for stability, but if keys ever change this test will fail for unrelated reasons. Consider verifying by type (supportedTypes) as a fallback in an additional assertion.
97-114: Typedef smoke test is fine; prefer a compile-time use site in real widget contextThis ensures the typedef compiles. If you want stronger guarantees, add a tiny widget that accepts
AttachmentPickerOptionsBuilderand invokes it once. Not required here.
116-136: Web/Desktop typedef test exists; add an integration test for the web/desktop builder pathYou validate the typedef, but there’s no integration case that exercises
webOrDesktopAttachmentPickerBuilder(optionsBuilder: ...). Adding one will guard the mapping logic introduced in the modal and the builder.Happy to draft a test that pumps a minimal app, calls
webOrDesktopAttachmentPickerBuilderwith anoptionsBuilderthat filters to a single option, and asserts the bottom sheet options and titles.
174-200: Assertion test is good; consider documenting release-mode behaviorThis asserts the mutual exclusivity in debug. In release, both can be passed and
optionsBuilderwins silently. Consider a doc comment or an error log to make this explicit.packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker.dart (5)
223-231: Docstring nit: spacing and clarityMinor grammar/punctuation issues in the typedef docs.
-/// The [defaultOptions] parameter contains the standard attachment picker -/// options(gallery, file, image, video, poll pickers). Developers can use -/// these as-is, reorder them, or combine them with custom options. +/// The [defaultOptions] parameter contains the standard attachment picker +/// options (gallery, file, image, video, poll pickers). Developers can use +/// these as-is, reorder them, or combine them with custom options.
236-244: Docstring nit: spacingMinor spacing fix.
-/// picker options (image, video, file, poll pickers). Developers can use these -/// as-is,reorder them, or combine them with custom options. +/// picker options (image, video, file, poll pickers). Developers can use these +/// as-is, reorder them, or combine them with custom options.
915-921: Consider reapplying allowedTypes after optionsBuilderA user-provided
optionsBuildercan introduce options whosesupportedTypesare not inallowedTypes. IfallowedTypesshould be authoritative, re-filterfinalOptionspost-build. If it’s purely an advisory default and customization should override it, leave as-is, but document the precedence.- final finalOptions = optionsBuilder != null - ? optionsBuilder(context, defaultOptions).toSet() - : { - if (customOptions != null) ...customOptions, - ...defaultOptions, - }; + final built = optionsBuilder != null + ? optionsBuilder(context, defaultOptions) + : [ + if (customOptions != null) ...customOptions, + ...defaultOptions, + ]; + final finalOptions = built + .where((o) => o.supportedTypes.every(allowedTypes.contains)) + .toSet();
982-991: Optional: enforce allowedTypes after custom builder on web/desktopSame consideration as mobile: if
allowedTypesshould remain authoritative even with a custom builder, re-filter after building the final list.- if (optionsBuilder != null) { - finalOptions = optionsBuilder(context, defaultOptions).toSet(); - } else { + if (optionsBuilder != null) { + finalOptions = optionsBuilder(context, defaultOptions) + .where((o) => o.supportedTypes.every(allowedTypes.contains)) + .toSet(); + } else { // Legacy behavior: combine custom and default options finalOptions = { if (customOptions != null) ...customOptions, ...defaultOptions, }; }
604-606: Typo in doc commentStray character in the docstring.
- /// The callback that is called when the end of the frame is reached.x + /// The callback that is called when the end of the frame is reached.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/stream_chat_flutter/CHANGELOG.md(1 hunks)packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker.dart(3 hunks)packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker_bottom_sheet.dart(4 hunks)packages/stream_chat_flutter/test/src/message_input/attachment_picker/options_builder_test.dart(1 hunks)
⏰ 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). (9)
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_persistence
- GitHub Check: stream_chat_flutter
- GitHub Check: analyze_legacy_versions
- GitHub Check: build (ios)
- GitHub Check: test
- GitHub Check: build (android)
- GitHub Check: analyze
🔇 Additional comments (10)
packages/stream_chat_flutter/test/src/message_input/attachment_picker/options_builder_test.dart (1)
22-33: Good coverage for mobile builder happy pathThese tests validate ordering and inclusion semantics when
optionsBuilderis provided and ensure the returned widget type. Nice.Also applies to: 34-48
packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker_bottom_sheet.dart (2)
95-101: Assert for mutual exclusivity: good guardThe assert preventing simultaneous
optionsBuilderandcustomOptionsis appropriate and consistent with the builders.
171-185: ForwardingoptionsBuilderto the mobile path is correctPassing through
optionsBuilderkeeps behavior consistent across platforms.packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker.dart (7)
911-913: Confirm intended filtering semantics for compound options (gallery)
gallery-pickersupports both images and videos. Withevery(allowedTypes.contains), it is included only when both are allowed. If you wantgalleryshown when either images or videos are allowed (and internally restrict selection), changeeverytoanyand ensure the view enforces filtering. If the current behavior is intentional to avoid exposing disallowed media, keep as-is.Would you like me to draft a small test to lock the intended behavior and avoid regressions?
951-979: Web/desktop default options look good and respect allowedTypesThe web/desktop defaults are localized and filtered.
996-1025: Error handling is consistent and user-friendlyCatching errors, popping the sheet, and delegating to
onErrormaintains UX and developer control.
793-910: Mobile default option implementations: solid, with consistent error handlingThe option view builders are cohesive and consistently guard errors. Nice work.
496-575: Options row UX logic is cleanEnablement based on
filterEnabledTypes, selected color, and send button gating onisValueChangedreads well.
314-358: Equality and enablement helpers are helpful; just fix the null key hash noted aboveThe helpers reduce duplication and centralize semantics.
369-413: Web/desktop bottom sheet list rendering is correctUsing
ListTileand enablement by type works well with the proposed title assert.
...t_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker_bottom_sheet.dart
Show resolved
Hide resolved
|
@xsahil03x @renefloor - unsure if anything needs to be fixed here - the only CodeRabbit comment is outside of my diff and not a part of these changes. |
dballance
force-pushed
the
feat/enable-ordering-custom-attachment-handlers
branch
from
c022b22 to
1ef2cb5
Compare
|
Rebased on the tip and fixed analyze nit. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2363 +/- ##
==========================================
+ Coverage 63.92% 64.01% +0.08%
==========================================
Files 413 413
Lines 25892 25905 +13
==========================================
+ Hits 16552 16582 +30
+ Misses 9340 9323 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hey @dballance, I will review this PR this week. Can you also allow edits from maintainers if I had to do any change before merging it. |
|
@xsahil03x - I do not see that option in my UI for this PR. Happy to turn it on - but I just don't see it anywhere. I think it's because of this: https://github.com/orgs/community/discussions/5634 Our fork lives within our organization, so it isn't available. Best bet may be to simply copy these commits over into another branch you own. Edit: Which, to be clear, would be totally fine with me. I've signed the CLA and certainly don't require that I'm the author of these commits if the overall approach works! |
|
@xsahil03x Anything needed from me here? |
dballance
force-pushed
the
feat/enable-ordering-custom-attachment-handlers
branch
from
6451b0f to
ded8149
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker_bottom_sheet.dart (1)
146-153: Fix type mismatch and add null-title guard for web/desktop wrapper
- You’re passing defaultOptions (List) into AttachmentPickerOptionsBuilder (expects List) without a cast. This is unsafe and can break type checks.
- Also reintroduce a debug assert to ensure titles are non-null before mapping, to prevent runtime crashes when
option.titleis null on web/desktop.Apply:
- optionsBuilder: optionsBuilder == null - ? null - : (context, defaultOptions) { - return optionsBuilder(context, defaultOptions) - .map(WebOrDesktopAttachmentPickerOption - .fromAttachmentPickerOption) - .toList(); - }, + optionsBuilder: optionsBuilder == null + ? null + : (context, defaultOptions) { + final built = optionsBuilder( + context, + defaultOptions.cast<AttachmentPickerOption>(), + ); + assert( + built.every((o) => o.title != null), + 'Each option must provide a non-null title when using ' + 'optionsBuilder in web/desktop or with ' + 'useSystemAttachmentPicker=true.', + ); + return built + .map(WebOrDesktopAttachmentPickerOption + .fromAttachmentPickerOption) + .toList(); + },Note: Consider the same assert when mapping
customOptionsto web/desktop options.
🧹 Nitpick comments (3)
packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker.dart (3)
223-244: Typedefs LGTM; tiny doc nitAdd a space after the comma in “as-is,reorder” -> “as-is, reorder”.
915-921: Enforce allowedTypes on optionsBuilder results (mobile)As-is, a custom optionsBuilder can reintroduce disallowed types. Filter the builder’s output to keep semantics aligned with allowedTypes.
- final finalOptions = optionsBuilder != null - ? optionsBuilder(context, defaultOptions).toSet() + final finalOptions = optionsBuilder != null + ? optionsBuilder(context, defaultOptions) + .where((o) => o.supportedTypes.every(allowedTypes.contains)) + .toSet() : { if (customOptions != null) ...customOptions, ...defaultOptions, };
981-991: Enforce allowedTypes on optionsBuilder results (web/desktop)Keep options in sync with allowedTypes when optionsBuilder is used.
- if (optionsBuilder != null) { - finalOptions = optionsBuilder(context, defaultOptions).toSet(); - } else { + if (optionsBuilder != null) { + finalOptions = optionsBuilder(context, defaultOptions) + .where((o) => o.supportedTypes.every(allowedTypes.contains)) + .toSet(); + } else { // Legacy behavior: combine custom and default options finalOptions = { if (customOptions != null) ...customOptions, ...defaultOptions, }; }
📜 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 (4)
packages/stream_chat_flutter/CHANGELOG.md(1 hunks)packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker.dart(3 hunks)packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker_bottom_sheet.dart(4 hunks)packages/stream_chat_flutter/test/src/message_input/attachment_picker/options_builder_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/stream_chat_flutter/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_chat_flutter/test/src/message_input/attachment_picker/options_builder_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). (7)
- GitHub Check: analyze_legacy_versions
- GitHub Check: build (android)
- GitHub Check: test
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat_persistence
🔇 Additional comments (4)
packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker_bottom_sheet.dart (2)
95-100: Good guard: optionsBuilder vs customOptions are mutually exclusiveAssert prevents ambiguous configuration. LGTM.
169-169: Mobile wiring looks correctPassing optionsBuilder through to the mobile builder keeps behavior consistent.
packages/stream_chat_flutter/lib/src/message_input/attachment_picker/stream_attachment_picker.dart (2)
784-790: Good guardMutual exclusivity assert is appropriate.
944-950: Good guardSame assert here is sound.
CLA
Description of the pull request
Adds
optionsBuildertomobileAttachmentPickerBuilder,webOrDesktopAttachmentPickerBuilder, andshowStreamAttachmentPickerModalBottomSheetso that the order of the options can be fully controlled.Today, options are strictly prepended, with no option to control ordering custom options after the default options.
Screenshots
Would also allow interspersing, or putting an exact picker in an exact index location, or reversing the list, etc. Below, the red indicate dummy options interspersed with
defaultOptionsSummary by CodeRabbit
New Features
Documentation
Tests