Share Information with deep linking and Join Bottom Sheet.#167
Share Information with deep linking and Join Bottom Sheet.#167anisha-e10 merged 21 commits intomainfrom
Conversation
…anisha/share-user-info
WalkthroughExtracts Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ShareItemsBottomSheet
participant Preview as SheetSharePreviewWidget
participant Provider as SheetList
participant OS as OS_Share
participant DeepLink as DeepLinkInitializer
participant Nav as Navigator
User->>UI: open share sheet
UI->>Preview: render preview + message input
User->>Preview: type message
Preview-->>UI: onMessageChanged(message)
UI->>Provider: updateSheetShareInfo(sheetId, sharedBy?, message?)
Provider-->>UI: sheet state updated
UI->>OS: trigger share with link?sharedBy=...&message=...
Note over User,DeepLink: Recipient opens link later
Recipient->>DeepLink: open link (sheetId + query params)
DeepLink->>Provider: updateSheetShareInfo(sheetId, sharedBy, message)
DeepLink->>Nav: navigate to join-sheet or sheet view
Nav-->>Recipient: show sheet with shared info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
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
🧹 Nitpick comments (13)
lib/features/share/widgets/sheet_join_preview_widget.dart (2)
83-84: Consider adding parentheses for clarity.While the operator precedence is correct, the condition would be more readable with explicit parentheses to make the grouping immediately clear.
- if (sharedBy != null && sharedBy.isNotEmpty || - (message != null && message.trim().isNotEmpty)) ...[ + if ((sharedBy != null && sharedBy.isNotEmpty) || + (message != null && message.trim().isNotEmpty)) ...[
83-84: Inconsistent trim() usage for sharedBy checks.Line 83 checks
sharedBy.isNotEmptywithout trim, while line 107 checkssharedBy.trim().isNotEmpty. This inconsistency could lead to showing an empty card if sharedBy contains only whitespace.Apply this diff to use consistent trimming:
- if (sharedBy != null && sharedBy.isNotEmpty || + if (sharedBy != null && sharedBy.trim().isNotEmpty || (message != null && message.trim().isNotEmpty)) ...[Also applies to: 107-107
test/features/share/widgets/sheet_join_preview_widget_test.dart (1)
360-645: Shared Info Card tests are solid; consider scoping the findersThe new tests cover all important sharedBy/message combinations (including trimming and empty/whitespace cases) and verify styling, which is great. To make them more robust if additional
GlassyContainerorZoeUserChipWidgetinstances are added elsewhere in the tree, consider scoping the matchers, e.g. usingfind.descendant(of: find.byType(SheetJoinPreviewWidget), matching: find.byType(GlassyContainer)), rather than a globalfind.byType.lib/features/share/utils/share_utils.dart (2)
17-27: getLinkPrefixUrl query param handling looks correct; small ergonomics tweak possibleMerging existing
uri.queryParameterswith the providedqueryParamsand returning the same🔗-prefixed format preserves behavior for existing callers while enabling deep-link metadata. You could optionally move theisNotEmptyguard into this method (treat empty maps likenull) so callers don’t need to computequeryParams.isNotEmpty ? queryParams : null, but that’s purely an ergonomics/refactor point.
30-65: Sheet share message logic is sound; confirm desired UX for user message visibilityThe additions here look good: emoji-only when
AvatarType.emoji, safesheetnull-check, and query param construction that trimsuserMessageand avoids sending whitespace-only values while omitting query params entirely when there’s nothing to add.One thing to double-check from a product perspective: right now the user-entered
userMessageonly appears URL-encoded in the link (?message=...), not as a separate human-readable paragraph in the share text body. If users are expected to see the message directly in the shared text (even if the deep link isn’t opened), you may want to also appenduserMessage.trim()before the link. You might also consider trimminguserNamefor consistency.lib/features/share/widgets/sheet_share_preview_widget.dart (1)
52-145:contentTextis no longer used in the build; consider removing or rendering itThe widget still requires a
contentTextargument but it isn’t referenced anywhere inbuild()now that the UI shows only the message input and stats. That can surprise callers and future readers. Either render the content preview (e.g., above the message field) or, if that responsibility has moved into the bottom sheet, consider droppingcontentTextfrom the API to avoid dead parameters.test/features/share/widgets/share_items_bottom_sheet_test.dart (1)
78-415: Sheet share tests give strong coverage of sharedBy/message flows; one possible enhancementThe new sheet-share tests do a nice job of covering: basic UI (preview +
SheetSharePreviewWidget), message typing viaAnimatedTextField, persistence ofsharedBy/messageon theSheetModel(including trimming and empty-message cases), and the null-currentUserpath where sharing still works but metadata isn’t updated. One gap is thatincludes user message in share content when providedcurrently only validates the text field/controller state; if the requirement is that the actual outbound share payload includes the message (not just as a query param), you could extend the platform share stub to capture the shared text and assert on it.test/features/share/utils/share_utils_test.dart (2)
77-128: Query-param tests are thorough but tightly coupled to encoding formatThe new
getLinkPrefixUrltests cover presence, empties, nulls, and special characters well. They do, however, hard-code expectations likesharedBy=John+Doeand specific%encodings, which tightly couples the tests to the exact encoding strategy (+for spaces, order, etc.). If you ever change implementation details (e.g., switch to%20for spaces or reorder params), these will fail even though behavior is still correct.Consider parsing the link with
Uri.parseand asserting onuri.queryParametersinstead of raw string fragments to keep the tests focused on semantics rather than encoding representation and ordering.
139-155: Sheet share message tests nicely cover userName/userMessage combinationsThe changes to
pumpSheetShareUtilsWidgetand the added tests arounduserName/userMessage(presence, null, empty, whitespace-only, trimming) give very solid coverage of the new behavior and edge cases, including the URL encoding of the message.One minor improvement would be to also assert that, when query params are present, the link line itself is structurally correct (e.g., parse the last line as a
Uriand inspectqueryParameters) rather than only looking forcontains('sharedBy=...')/contains('message=...'). That would catch regressions where the query string is malformed while still keeping tests resilient to parameter ordering.Also applies to: 265-419
lib/features/share/widgets/share_items_bottom_sheet.dart (4)
38-71: Stateful conversion and contentText computation are functionally sound, but trim logic can be centralizedConverting
ShareItemsBottomSheetto aConsumerStatefulWidgetwith_userMessagestate is appropriate for driving the live preview and the final shared text. The sheet branch ofcontentTextcorrectly pullscurrentUserfromcurrentUserProviderand threadsuserName/userMessageintoShareUtils.getSheetShareMessage.You currently pre-filter
userNameanduserMessagewith:userName: userName.isNotEmpty ? userName : null, userMessage: _userMessage.trim().isNotEmpty ? _userMessage : null,Given the ShareUtils/tests already handle trimming and empty/whitespace-only messages, you could simplify this by passing
_userMessage(and evenuserName) as-is and letting ShareUtils centralize the trimming/emptiness rules. That avoids duplication of logic and keeps all normalization in one place.Functionally this is fine as-is; this is just a simplification/maintainability suggestion.
73-107: Be mindful of nested scroll views when not sharing a sheetThe outer
SingleChildScrollViewaround the entire column is helpful for making the bottom sheet scrollable on smaller screens or when the keyboard is open. For non-sheet content,_buildContentPreviewalso wraps the text in its ownSingleChildScrollView, which can lead to nested vertical scroll views.This usually works but can result in slightly odd scrolling behavior or unnecessary complexity. If you run into UX issues, a small refactor (e.g., let the outer
SingleChildScrollViewbe the only one and make the inner preview a simpleText/SelectableText) would simplify the hierarchy.
112-123: Sheet header helper is straightforward; consider handling null sheet more explicitly
_getSheetInfonicely reuses providers to render the sheet avatar and title forisSheetflows. Whensheetisnull, you currently returnSizedBox.shrink()but still allow the rest of the bottom sheet (including share) to render.If a missing sheet is considered an error state, you might want to also disable the share button or show an error/placeholder instead of silently hiding the header. Not a blocker, just something to align with expected UX.
197-220: Sheet share metadata update matches link behavior; minor cleanup possibleThe updated
_buildShareButtoncorrectly:
- Uses localized
l10n.sharefor the subject and button text.- When
isSheetis true and acurrentUserexists, callsupdateSheetShareInfowithsheetId,sharedBy, and a trimmedmessage(ornullwhen empty/whitespace), then sharescontentText.This aligns with the tests that ensure
sharedBy/messageare only propagated when non-empty and thatuserMessageis trimmed.Two small, non-blocking refinements you might consider:
- Guard against an empty
userNamewhen updatingsharedBy(similar to how you guard incontentText) to avoid persistingsharedBy: ''if that’s ever possible.- Avoid trimming
_userMessagetwice by storingfinal trimmed = _userMessage.trim();and reusing it for both theisNotEmptycheck and the value.Behavior is otherwise solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
lib/core/deeplink/deep_link_initializer.dart(3 hunks)lib/features/share/utils/share_utils.dart(4 hunks)lib/features/share/widgets/share_items_bottom_sheet.dart(4 hunks)lib/features/share/widgets/sheet_join_preview_widget.dart(4 hunks)lib/features/share/widgets/sheet_share_preview_widget.dart(3 hunks)lib/features/sheet/models/sheet_model.dart(5 hunks)lib/features/sheet/providers/sheet_providers.dart(1 hunks)lib/l10n/app_en.arb(1 hunks)lib/l10n/app_gu.arb(1 hunks)lib/l10n/app_hi.arb(1 hunks)test/features/share/utils/share_utils_test.dart(3 hunks)test/features/share/widgets/share_items_bottom_sheet_test.dart(13 hunks)test/features/share/widgets/sheet_join_preview_widget_test.dart(2 hunks)test/features/share/widgets/sheet_share_preview_widget_test.dart(4 hunks)test/features/sheet/providers/sheet_providers_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). (7)
- GitHub Check: Test zoe_native plugin
- GitHub Check: test
- GitHub Check: windows
- GitHub Check: macos
- GitHub Check: linux
- GitHub Check: android
- GitHub Check: ios
🔇 Additional comments (6)
lib/features/share/widgets/sheet_share_preview_widget.dart (1)
15-50: Stateful conversion and message controller wiring look correctUsing
ConsumerStatefulWidgetwith a lazily createdTextEditingController, a single listener that proxies toonMessageChanged, and proper disposal indispose()is a clean way to surface the free-form message while keeping Riverpodref.watchusage inbuild(). No lifecycle or reactivity issues stand out here.test/features/share/widgets/share_items_bottom_sheet_test.dart (2)
35-69: Test container setup and L10n helper improve clarity; note container reassignmentsInitializing an
initialContainerto fetch a test user id and then creating the sharedcontainerwith aloggedInUserProvideroverride keeps the sheet-share tests realistic. Centralizing localization viagetL10nalso removes duplication. Just be aware that in some sub-groups (e.g., Event/Poll share) you later reassigncontainer = ProviderContainer.test(...)with different overrides, which drops the logged-in-user override; that’s fine for the current assertions but is a subtle behavior to keep in mind if future tests in those groups start depending on the current user.
708-757: Non-sheet share button behavior test is straightforward and valuableThe
share button calls share for non-sheet contenttest cleanly verifies that the same share callback is triggered for non-sheet items, without entangling it with sheet-specific state likeupdateSheetShareInfo. This helps guard against regressions where future refactors inadvertently gate sharing logic onisSheet.test/features/share/widgets/sheet_share_preview_widget_test.dart (1)
40-228: Message input tests thoroughly validate the new SheetSharePreviewWidget contractExtending the pump helper to pass
onMessageChangedand adding theMessage Input Fieldgroup gives very good coverage of the new stateful behavior: AnimatedTextField configuration, callback invocation (including multiple updates), null-callback safety, and controller text lifecycle (typing and clearing). This should make future refactors of the message field or controller wiring much safer.lib/features/share/widgets/share_items_bottom_sheet.dart (2)
29-36: Bottom inset padding integration looks goodWrapping the bottom sheet content in a
Paddingthat usesMediaQuery.of(context).viewInsets.bottomis a good way to keep the sheet above the keyboard while retainingisScrollControlled: trueand the existing shape/drag handle. No functional issues spotted here.
125-179: Title/message dispatch by content type is correct and consistentUpdating
_getShareTitleand_getContentShareMessageto usewidget.parentIdand the richerContentType/listType switching keeps the bottom sheet’s title and message aligned with the existing share utils for text, event, list, task, bullet, and poll.This logic is clean and matches the behavior exercised in the tests for each content type. No issues from a correctness standpoint.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/features/share/widgets/sheet_join_preview_widget.dart (1)
72-82: Consider simplifying the null handling for better readability.While the null check on lines 72-73 prevents issues, the double force unwrap on lines 77-78 is not idiomatic. Consider extracting to a local variable for cleaner code.
Apply this diff to improve readability:
- if (sheet.description?.plainText != null && - sheet.description!.plainText!.isNotEmpty) + if (sheet.description?.plainText?.isNotEmpty ?? false) Padding( padding: const EdgeInsets.only(top: 8), child: Text( - sheet.description!.plainText!, + sheet.description!.plainText!, style: theme.textTheme.bodySmall, maxLines: 2, overflow: TextOverflow.ellipsis, ), ),Or use a local variable:
+ final descriptionText = sheet.description?.plainText; - if (sheet.description?.plainText != null && - sheet.description!.plainText!.isNotEmpty) + if (descriptionText != null && descriptionText.isNotEmpty) Padding( padding: const EdgeInsets.only(top: 8), child: Text( - sheet.description!.plainText!, + descriptionText, style: theme.textTheme.bodySmall, maxLines: 2, overflow: TextOverflow.ellipsis, ), ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/share/widgets/sheet_join_preview_widget.dart(4 hunks)
🔇 Additional comments (4)
lib/features/share/widgets/sheet_join_preview_widget.dart (4)
4-5: LGTM! New imports properly support the shared info UI.The imports for
ZoeUserChipWidget,GlassyContainer, andZoeUserChipTypeare correctly added to support the new shared information card functionality.Also applies to: 8-8
38-38: LGTM! Localization and data extraction are correctly implemented.The localization setup and extraction of
sharedByandmessagefrom the sheet model follow standard patterns.Also applies to: 42-43
83-87: LGTM! Conditional rendering logic is correct.The condition properly checks for the presence of
sharedByor a non-emptymessagebefore rendering the shared info card.
96-162: Review comment contains incorrect assumption about data model; only address efficiency and UX concerns.The review's core suggestion to change
u.id == sharedByis incorrect. The code intentionally stores user names (not IDs) insharedBy, as confirmed by SheetModel definition and test data using'John Doe','Jane Smith'. The current name-based lookup is by design.However, two valid concerns remain:
Inefficiency: Replace
where().firstOrNullwithfirstWhereOrNullfor early exit- final sharedByUser = userList.where((u) => u.name == sharedBy).firstOrNull; + final sharedByUser = userList.firstWhereOrNull((u) => u.name == sharedBy);Silent UX failure: When no user is found, the "shared by" section disappears entirely. Show the name as fallback text:
- if (hasSharedBy && sharedByUser != null) ...[ + if (hasSharedBy) ...[ ... - child: ZoeUserChipWidget( - user: sharedByUser, + child: sharedByUser != null + ? ZoeUserChipWidget( + user: sharedByUser, + ) + : Text(sharedBy, style: theme.textTheme.bodyMedium),Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/features/share/widgets/share_items_bottom_sheet.dart (1)
101-112: Consider showing a loading state for better UX.When the sheet is
null(e.g., during initial load), the method returns an empty widget. Consider showing a loading indicator to provide feedback to the user while the sheet data is being fetched.For example:
Widget _getSheetInfo(BuildContext context, WidgetRef ref) { final sheet = ref.watch(sheetProvider(widget.parentId)); - if (sheet == null) return const SizedBox.shrink(); + if (sheet == null) { + return const Center( + child: Padding( + padding: EdgeInsets.all(8.0), + child: CircularProgressIndicator(), + ), + ); + } return Column( crossAxisAlignment: CrossAxisAlignment.center, children: [ SheetAvatarWidget(sheetId: sheet.id, padding: const EdgeInsets.all(8)), const SizedBox(height: 8), Text(sheet.title, style: Theme.of(context).textTheme.bodyMedium), ], ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/features/share/widgets/share_items_bottom_sheet.dart(5 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). (8)
- GitHub Check: Test zoe_native plugin
- GitHub Check: format
- GitHub Check: test
- GitHub Check: windows
- GitHub Check: linux
- GitHub Check: macos
- GitHub Check: android
- GitHub Check: ios
🔇 Additional comments (5)
lib/features/share/widgets/share_items_bottom_sheet.dart (5)
12-14: LGTM!The new imports are necessary for the sheet sharing functionality and are properly utilized throughout the file.
34-51: LGTM!The conversion to
ConsumerStatefulWidgetis appropriate for managing the user's custom share message. The implementation follows Flutter and Riverpod conventions correctly.
53-99: LGTM!The build method updates correctly handle keyboard insets, enable scrolling for better UX, and properly wire up the state management for the user message. The conditional rendering based on
isSheetis clean and appropriate.
114-133: LGTM!The localization handling is clean, and all content types are properly mapped to their corresponding localized share titles.
147-185: LGTM!The method correctly references
widget.parentIdthroughout and properly handles all content types with their respective share message generation logic.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/features/share/widgets/sheet_join_preview_widget.dart (1)
83-87: Guard sharedBy/message sections to avoid null crashes and partial data casesRight now
_buildSharedInfoCard:
- Computes
hasMessage/hasSharedBy, but still always renders:
ZoeUserChipWidget(user: sharedByUser!)Text(message!)This will crash when:
- Only
messageis present (nosharedBy), orsharedByis set but no matchingsharedByUserexists.It also forces both sections even if only one has data.
Recommend:
- Use
hasSharedByto gate the "shared by" row.- Use
hasMessageto gate the message section.- Only use
!where the correspondinghas*is true.Example refactor:
final hasMessage = (message != null && message.trim().isNotEmpty); final hasSharedBy = (sharedBy != null && sharedBy.trim().isNotEmpty && sharedByUser != null); // If nothing to show, no need to build card if (!hasMessage && !hasSharedBy) return const SizedBox.shrink(); return GlassyContainer( @@ - child: Column( - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - Row( - children: [ - Text( - '${l10n.sharedBy}: ', - style: theme.textTheme.labelSmall?.copyWith( - color: theme.colorScheme.onSurface.withValues(alpha: 0.6), - fontWeight: FontWeight.w500, - ), - ), - const SizedBox(width: 4), - Flexible( - child: ZoeUserChipWidget( - user: sharedByUser!, - type: ZoeUserChipType.userNameWithAvatarChip, - ), - ), - ], - ), - - const SizedBox(height: 12), - Text( - '${l10n.message}:', - style: theme.textTheme.labelSmall?.copyWith( - color: theme.colorScheme.onSurface.withValues(alpha: 0.6), - ), - ), - const SizedBox(height: 6), - Text( - message!, - style: theme.textTheme.bodyMedium?.copyWith( - color: theme.colorScheme.onSurface.withValues(alpha: 0.9), - ), - ), - ], - ), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + if (hasSharedBy) ...[ + Row( + children: [ + Text( + '${l10n.sharedBy}: ', + style: theme.textTheme.labelSmall?.copyWith( + color: theme.colorScheme.onSurface.withValues(alpha: 0.6), + fontWeight: FontWeight.w500, + ), + ), + const SizedBox(width: 4), + Flexible( + child: ZoeUserChipWidget( + user: sharedByUser!, + type: ZoeUserChipType.userNameWithAvatarChip, + ), + ), + ], + ), + ], + if (hasMessage) ...[ + if (hasSharedBy) const SizedBox(height: 12), + Text( + '${l10n.message}:', + style: theme.textTheme.labelSmall?.copyWith( + color: theme.colorScheme.onSurface.withValues(alpha: 0.6), + ), + ), + const SizedBox(height: 6), + Text( + message!.trim(), + style: theme.textTheme.bodyMedium?.copyWith( + color: theme.colorScheme.onSurface.withValues(alpha: 0.9), + ), + ), + ], + ], + ),This avoids null crashes and handles "only message" / "only sharedBy" cleanly.
Also applies to: 96-160
🧹 Nitpick comments (1)
lib/features/share/widgets/share_items_bottom_sheet.dart (1)
70-90: Sheet share message relies on upstream callback wiring; consider trimming consistencyThe overall sheet-sharing flow here looks good:
_userMessageis fed into both:
_getSheetShareMessage(for the preview text), andupdateSheetShareInfoin_buildShareButton(to persistsharedBy/message).Two points to be aware of:
Dependency on message callback wiring
_userMessageonly updates viaonMessageChangedfromSheetSharePreviewWidget. With the current callback implementation there (and inAnimatedTextField), this is never invoked, so messages will always be empty. Once that callback wiring is fixed (see comments insheet_share_preview_widget.dartandanimated_textfield_widget.dart), this code path should work as intended.Minor trim inconsistency (optional)
_getSheetShareMessagechecksuserMessage: _userMessage.trim().isNotEmpty ? _userMessage : null,_buildShareButtonusesmessage: _userMessage.trim().isNotEmpty ? _userMessage.trim() : null,For consistent behavior and to avoid persisting trailing spaces while sharing a trimmed preview (or vice versa), you may want to use the trimmed value in both places:
String _getSheetShareMessage(WidgetRef ref) {@@
- return ShareUtils.getSheetShareMessage(
ref: ref,parentId: widget.parentId,userName: userName.isNotEmpty ? userName : null,userMessage: _userMessage.trim().isNotEmpty ? _userMessage : null,- );
- final trimmedMessage = _userMessage.trim();
- return ShareUtils.getSheetShareMessage(
ref: ref,parentId: widget.parentId,userName: userName.isNotEmpty ? userName : null,userMessage: trimmedMessage.isNotEmpty ? trimmedMessage : null,- );
}(Button handler is already using `trim()` consistently.) Also applies to: 135-145, 198-218 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between bd583a857f73347a354508dfef0a708b8caee549 and 013fa1d27f7dba8f770f83e2311428bf48193355. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `lib/common/widgets/animated_textfield_widget.dart` (4 hunks) * `lib/features/share/widgets/share_items_bottom_sheet.dart` (5 hunks) * `lib/features/share/widgets/sheet_join_preview_widget.dart` (4 hunks) * `lib/features/share/widgets/sheet_share_preview_widget.dart` (3 hunks) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/features/share/widgets/sheet_share_preview_widget.dart (1)
32-46: Consider initializing the controller with existing sheet message.The
TextEditingControlleris initialized empty, but according to the PR context, the sheet model now contains amessagefield (extracted from deep links or previous input). If a message already exists on the sheet, it won't be displayed in the text field.Consider initializing the controller with the existing message:
@override void initState() { super.initState(); - _messageController = TextEditingController(); + final sheet = ref.read(sheetProvider(widget.parentId)); + _messageController = TextEditingController(text: sheet?.message ?? ''); }This ensures that any existing message (e.g., from a deep link) is displayed when the widget is created.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/common/widgets/animated_textfield_widget.dart(4 hunks)lib/features/share/widgets/sheet_share_preview_widget.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/common/widgets/animated_textfield_widget.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). (8)
- GitHub Check: Test zoe_native plugin
- GitHub Check: test
- GitHub Check: format
- GitHub Check: macos
- GitHub Check: windows
- GitHub Check: linux
- GitHub Check: android
- GitHub Check: ios
🔇 Additional comments (5)
lib/features/share/widgets/sheet_share_preview_widget.dart (5)
1-13: LGTM! Imports are appropriate.The imports correctly include the new dependencies needed for the refactored widget:
AnimatedTextFieldand localization support.
15-30: LGTM! Proper StatefulWidget conversion.The refactoring from
ConsumerWidgettoConsumerStatefulWidgetis correctly implemented to support theTextEditingControllerlifecycle and the newonMessageChangedcallback enables parent widgets to receive message updates.
48-77: LGTM! Data filtering and setup are correct.The filtering logic consistently uses
widget.parentIdto isolate sheet-specific data, and the early return for null sheet ensures safe operation. Localization is properly initialized.
88-102: ✅ Previous callback issue has been fixed!The
onTextChangehandler now correctly invokeswidget.onMessageChangedwith the current text value from_messageController.text(lines 93-98). This addresses the previous review comment where the callback was only referenced but never called.
105-201: LGTM! Statistics rendering is well-structured.The conditional rendering ensures statistics are only displayed when relevant data exists, providing good UX. The
_buildStatCardhelper method is reusable and properly styled with theme-aware colors and localized labels.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/features/share/widgets/sheet_join_preview_widget_test.dart (4)
342-380: Consider consistent provider overrides for user resolution.Test 2 (lines 393-395) explicitly overrides
getUserByNameProviderwhen creating a custom user, while this test usesgetUserByIndex(container)without the override. While this may work due to test utilities automatically configuring providers, the inconsistency could lead to confusion or brittleness if test utilities change.Consider adding the provider override for consistency:
final testContainer = ProviderContainer.test( overrides: [ loggedInUserProvider.overrideWithValue(AsyncValue.data(testUserId)), + getUserByNameProvider(sharingUser.name).overrideWith((ref) => sharingUser), sheetListProvider.overrideWith( () => SheetList()..state = [sheetWithBoth], ), sheetProvider(testSheetId).overrideWith((ref) => sheetWithBoth), ], );
382-413: Rename variable and simplify test scope.Two issues in this test:
Misleading variable name (line 385): The variable
sheetWithTrimmedMessageis misleading—the message is not trimmed and contains whitespace. Consider renaming tosheetWithWhitespaceMessagefor clarity.Mixed concerns: The test creates a new
UserModeland overridesgetUserByNameProvider(lines 384, 393-395), when the test is focused on whitespace preservation, not user resolution. This adds unnecessary complexity. Consider usinggetUserByIndex(container)like Test 1 to keep the test focused on its stated purpose.Apply this diff to rename the variable:
-final sheetWithTrimmedMessage = testSheet.copyWith( +final sheetWithWhitespaceMessage = testSheet.copyWith( message: testMessage, sharedBy: sharingUser.name, ); final testContainer = ProviderContainer.test( overrides: [ loggedInUserProvider.overrideWithValue(AsyncValue.data(testUserId)), getUserByNameProvider( sharingUser.name, ).overrideWith((ref) => sharingUser), sheetListProvider.overrideWith( - () => SheetList()..state = [sheetWithTrimmedMessage], + () => SheetList()..state = [sheetWithWhitespaceMessage], ), sheetProvider( testSheetId, - ).overrideWith((ref) => sheetWithTrimmedMessage), + ).overrideWith((ref) => sheetWithWhitespaceMessage), ], );
544-577: Consider consistent provider overrides for user resolution.Similar to the first test in this group (lines 342-380), this test uses
getUserByIndex(container)without explicitly overridinggetUserByNameProvider, creating inconsistency with Test 2. While this likely works due to test utilities, consider adding the override for consistency and clarity.
341-578: Excellent test coverage with minor gaps.The "Shared Info Card" test group provides comprehensive coverage of the shared info functionality. The tests are well-structured and cover both positive and negative scenarios effectively.
Strengths:
- Thorough edge case coverage (null, empty, whitespace-only)
- Clear test names that describe expected behavior
- Separate tests for each concern
- Styling verification included
Minor gaps:
Consider adding tests for:
sharedBywith only whitespace (you test this formessagebut notsharedBy)- Only
sharedBypresent (message null/empty)- Only
messagepresent (sharedBy null/empty)These scenarios would ensure the card displays correctly when only one field is populated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/features/share/widgets/sheet_join_preview_widget_test.dart(10 hunks)
🔇 Additional comments (2)
test/features/share/widgets/sheet_join_preview_widget_test.dart (2)
5-5: LGTM: Import additions support new test coverage.The new imports for
GlassyContainerWidget,ZoeUserChipWidget, andUserModelare appropriate and necessary for the expanded "Shared Info Card" test group.Also applies to: 8-8, 15-15
66-66: LGTM: Formatting consolidation improves readability.The consolidation of multi-line function calls to single lines when they fit comfortably improves code compactness without affecting functionality.
Also applies to: 109-109, 196-196, 214-214, 232-232, 246-246, 275-275, 314-316
kumarpalsinh25
left a comment
kumarpalsinh25
left a comment
There was a problem hiding this comment.
Looks good overall, just minor feedback which need to address before merge.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/features/share/widgets/sheet_share_preview_widget.dart (1)
32-46: User message isn’t reliably propagated to parent; onMessageChanged only fires on submit
SheetSharePreviewWidgetowns_messageController, and the only placeonMessageChangedis called is insideonSubmitted. If the user types a message and taps the Share button without hitting the keyboard’s submit action, the parent never sees the text, so the sheet’s sharedmessagewill be empty even though the UI shows it.Hook the controller to the callback instead of relying solely on submit:
class _SheetSharePreviewWidgetState extends ConsumerState<SheetSharePreviewWidget> { - late TextEditingController _messageController; + late final TextEditingController _messageController; @override void initState() { super.initState(); _messageController = TextEditingController(); + _messageController.addListener(_handleMessageChanged); } + void _handleMessageChanged() { + final callback = widget.onMessageChanged; + if (callback != null) { + callback(_messageController.text); + } + } + @override void dispose() { + _messageController.removeListener(_handleMessageChanged); _messageController.dispose(); super.dispose(); } @@ - AnimatedTextField( + AnimatedTextField( controller: _messageController, hintText: l10n.addAMessage, onErrorChanged: (error) {}, - onSubmitted: () { - final callback = widget.onMessageChanged; - if (callback != null) { - callback(_messageController.text); - } - }, + onSubmitted: _handleMessageChanged, maxLines: 3, keyboardType: TextInputType.multiline, autofocus: false, ),This ensures
onMessageChangedreflects the current message as the user types, and still fires on submit.Also applies to: 88-101
🧹 Nitpick comments (2)
lib/features/share/utils/share_utils.dart (2)
15-40: Avoid emitting empty sharedBy/message query params in deep links
getSheetDeepLinkUrlalways addssharedBy/messagekeys, even when null/empty, leading to URLs like...?sharedBy=&message=. If consumers distinguish between “absent” and “empty” or you want cleaner links, build the map only for non-empty values and passnullwhen it’s empty.static String getSheetDeepLinkUrl( String sheetId, String? sharedBy, String? message, ) { - final queryParams = <String, String>{ - 'sharedBy': sharedBy ?? '', - 'message': message ?? '', - }; - return getLinkPrefixUrl('sheet/$sheetId', queryParams: queryParams); + final queryParams = <String, String>{ + if (sharedBy != null && sharedBy.isNotEmpty) 'sharedBy': sharedBy, + if (message != null && message.isNotEmpty) 'message': message, + }; + + return getLinkPrefixUrl( + 'sheet/$sheetId', + queryParams: queryParams.isEmpty ? null : queryParams, + ); }
47-61: Confirm whether userMessage should also appear in the visible share textRight now
userMessageis only threaded into the deep link (messagequery param) and never written into the visible share body built bygetSheetShareMessage. That’s fine if the intent is to show the message only inside the app on the join flow, but if you expect the outbound share (e.g., WhatsApp/iMessage) to visibly include the typed message too, you’ll need to prepend/append it tobufferbefore the link.Also applies to: 71-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/common/widgets/animated_textfield_widget.dart(1 hunks)lib/features/share/utils/share_utils.dart(2 hunks)lib/features/share/widgets/sheet_share_preview_widget.dart(3 hunks)
🔇 Additional comments (2)
lib/common/widgets/animated_textfield_widget.dart (1)
98-119: Border, fill, and padding tweaks look safeThe adjustments to
focusedBorder/errorBorder/focusedErrorBorder,fillColor, andcontentPaddingare purely stylistic; they don’t affect controller behavior, shake animation, or callbacks. No functional regressions spotted.lib/features/share/widgets/sheet_share_preview_widget.dart (1)
53-71: Sheet-scoped filtering and localized stats preview look consistentFiltering events/tasks/documents/polls by
widget.parentIdplus theisTodaychecks, and using localized labels/subtitles in_buildStatCard, all look consistent with the new sheet-centric share preview. Always showing the cards with a count (including zero) also aligns with prior feedback.Also applies to: 77-137
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/features/share/widgets/share_items_bottom_sheet.dart (1)
197-213: Add error handling for sheet share info update.The
updateSheetShareInfocall lacks error handling. If it fails, the user won't receive feedback, and the share proceeds with potentially incomplete metadata. While the share functionality itself still works, error handling would improve the user experience.Consider wrapping the update in try-catch:
if (widget.isSheet) { final currentUser = await ref.read(currentUserProvider.future); if (currentUser != null) { try { final trimmedMessage = _userMessage.trim(); ref.read(sheetListProvider.notifier).updateSheetShareInfo( sheetId: widget.parentId, sharedBy: currentUser.name, message: trimmedMessage.isNotEmpty ? trimmedMessage : null, ); } catch (e) { // Log error or show user feedback if (context.mounted) { ScaffoldMessenger.of(context).showSnackBar( SnackBar(content: Text('Failed to update share info: $e')), ); } } } }lib/features/share/utils/share_utils.dart (1)
66-66: Consider handling AsyncValue states explicitly.Accessing
.valuedirectly oncurrentUserProviderdoesn't distinguish between loading, error, and actual null states. While the current code handles null gracefully by omitting thesharedByparameter, explicitly handling AsyncValue states would be more robust.Consider using
whenormaybeWhenfor clearer state handling:final userName = ref.watch(currentUserProvider).when( data: (user) => user?.name, loading: () => null, error: (_, __) => null, );This makes the intent clearer and ensures consistent behavior across all AsyncValue states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/share/utils/share_utils.dart(8 hunks)lib/features/share/widgets/share_items_bottom_sheet.dart(5 hunks)test/features/share/utils/share_utils_test.dart(11 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). (7)
- GitHub Check: macos
- GitHub Check: Test zoe_native plugin
- GitHub Check: linux
- GitHub Check: android
- GitHub Check: test
- GitHub Check: windows
- GitHub Check: ios
🔇 Additional comments (14)
test/features/share/utils/share_utils_test.dart (5)
36-44: LGTM! Clean test setup.The test scaffolding properly overrides
currentUserProviderto simulate a logged-in user, which is essential for testing the newsharedByquery parameter functionality.
84-150: Excellent test coverage for query parameter handling.The tests thoroughly cover edge cases including null/empty query params, proper URL encoding (spaces as
+, special characters like&and!), and multiple parameters. This ensures the link generation is robust.
177-201: Well-designed test helper.The
getSheetShareLinkhelper function mirrors the implementation's trimming and encoding logic, making tests more maintainable and readable. The conditional query parameter inclusion matches the expected behavior.
293-400: Comprehensive test coverage for share message query parameters.The tests thoroughly validate all scenarios: both parameters present, only
sharedBy, trimming behavior, and exclusion of empty/whitespace messages. This ensures the sharing feature works correctly across all user input variations.
403-1671: Consistent test coverage across all content types.All content type tests (text, event, list, task, bullet, poll) have been properly updated to use the new
baseUrlandgetLinkPostfixUrlAPI. The test patterns are consistent and comprehensive.lib/features/share/widgets/share_items_bottom_sheet.dart (4)
34-51: Clean migration to stateful widget.The refactoring from
ConsumerWidgettoConsumerStatefulWidgetis correct and necessary to track the user's message input (_userMessage) locally. The state management is straightforward and appropriate for this use case.
85-89: Proper state update callback.The
onMessageChangedcallback correctly usessetStateto update_userMessage, ensuring the widget rebuilds when the user modifies their share message. This provides a good reactive flow from input to preview to sharing.
101-112: Clean sheet info display.The
_getSheetInfomethod properly handles the null case and displays the sheet avatar and title in a clear layout. The use ofSizedBox.shrink()for missing sheets is appropriate.
135-141: Message construction delegated correctly.The method properly delegates to
ShareUtils.getSheetShareMessagewith conditionaluserMessageinclusion. The trimming logic ensures empty messages aren't passed.lib/features/share/utils/share_utils.dart (5)
15-16: LGTM! Clean constant definition.The
baseUrlconstant is appropriately defined and replaces the previouslinkPrefixUrl. The hardcoded URL is acceptable for a known application domain.
17-40: Well-implemented deep link URL generation.The method correctly trims and validates query parameters before including them, preventing empty or whitespace-only values from appearing in the URL. The use of conditional map population is clean and efficient.
42-54: Robust URL construction with proper encoding.The method correctly uses
Uri.parseanduri.replaceto build URLs with query parameters. The automatic URL encoding byUriensures special characters are handled properly, and the conditional logic handles null/empty params correctly.
72-74: Clean conditional emoji handling.The code properly checks the avatar type before including the emoji in the share message. This ensures that non-emoji avatars (like images) don't incorrectly appear in the text.
115-115: Consistent link generation updates across all content types.All content share methods have been uniformly updated to use
getLinkPostfixUrl. The consistent pattern makes the code maintainable and easy to understand.Also applies to: 155-155, 204-204, 246-246, 273-273, 300-300
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
test/features/share/widgets/share_items_bottom_sheet_test.dart (5)
13-25: Good centralization of localization access viagetL10nExtracting
getL10nand reusing it across all content-type tests removes duplication and keeps thebyType: ShareItemsBottomSheetdetail in one place. The updated call sites for titles and labels all line up with the expected l10n keys and improve readability.If you find yourself needing
getL10nin other share-related test files too, consider moving it into a shared test helper to avoid re-defining it in multiple places.Also applies to: 66-68, 427-429, 452-453, 482-483, 520-521, 558-559, 604-605, 646-647
37-53: Container setup works but could be made more robust and reusableThe new global
setUpcorrectly wiresloggedInUserProviderandcurrentUserProviderusing a test user, which is what the new sheet-sharing tests rely on. A couple of small robustness tweaks are worth considering:
- You’re now creating an extra
ProviderContainer.test()(initialContainer) purely to fetch a user, but not disposing it. You can dispose it after extractingtestUserId/user to avoid lingering containers in long-running test suites.- Several inner
setUpblocks (events, tasks, polls) replacecontainerwith a freshProviderContainer.test(overrides: [...contentProvider...])that drops the logged-in overrides from the globalsetUp. That’s fine for the current tests (they don’t rely on user info there), but it’s easy to forget if future tests around non-sheet sharing start depending on current user context.You could factor out a small helper that builds a “base test container with logged-in user” and then layer content-specific overrides on top of that to keep behavior consistent across groups.
Also applies to: 439-446, 547-551, 635-640
145-198: Message-handling tests are thorough but a bit redundant; tighten or rename one testThe group of tests around message entry and
sharedBy/messagepersistence is comprehensive:
- Verifies text entry and controller state.
- Persists
sharedByand message (including trimming and empty-message behavior).- Confirms
updateSheetShareInfosemantics whencurrentUseris present.Two minor improvements:
The test
'includes user message in share content when provided'currently only checks that theAnimatedTextField’s controller has the entered text, which is already covered by'updates message state when user types'. Either:
- Rename it to reflect what it actually asserts (e.g. “keeps user message in text field”), or
- Extend it to assert on the generated share payload (if you have a hook to inspect what’s sent to the share layer), so the name matches the behavior.
The “saves X...” tests and the “calls updateSheetShareInfo...” tests validate essentially the same state on
sheetProvider(testSheet.id). You might collapse them into fewer tests with parameterized inputs (message vs empty) to reduce duplication while still covering trimming and null-handling.None of this affects correctness, but trimming the overlap will keep the suite faster and easier to maintain.
Also applies to: 200-229, 231-266, 268-289, 291-364
366-413: Null-currentUser test is valuable; consider seeding the sheet state explicitlyThe
'does not call updateSheetShareInfo when currentUser is null'test nicely covers the negative path: it forces bothloggedInUserProviderandcurrentUserProvidertonull, then ensures the sheet’ssharedBy/messagefields stay unchanged while the platform share still fires.To make this test more explicit and less dependent on whatever default state
sheetProvider(testSheet.id)returns in a freshProviderContainer.test():
- Consider overriding the underlying sheet state/provider in
testContainerwith a knownSheetModel(e.g., thetestSheetfrom the outer container) before pumping the bottom sheet.- That way, you’re asserting that a known concrete sheet instance is not modified, rather than relying on whatever
sheetProviderhappens to produce by default for that id.This would make the test more self-contained and resilient to future changes in how sheets are seeded in tests.
725-729: Share button interaction tests look good; small consistency tweak possibleThe new tests confirming that:
- The primary button has the correct icon/text (
share), and- Tapping it triggers the share platform handler for both sheet and non-sheet content
are aligned with the intended behavior and give good confidence in the wiring.
For consistency with most Flutter testing patterns (and with your own non-sheet test here), you might also add an
await tester.pump();after the tap in'share button has correct properties', even though the boolean flag is set synchronously today. It keeps the pattern uniform if the handler grows more asynchronous later.Also applies to: 731-756
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/features/share/widgets/share_items_bottom_sheet_test.dart(13 hunks)test/features/share/widgets/sheet_share_preview_widget_test.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/features/share/widgets/sheet_share_preview_widget_test.dart
🔇 Additional comments (1)
test/features/share/widgets/share_items_bottom_sheet_test.dart (1)
78-99: Sheet share UI tests cover the new preview and message input wellThe added expectations for
SheetSharePreviewWidget,GlassyContainer, the share button, and theAnimatedTextField(including checking theaddAMessagehint) give good coverage of the new sheet-specific share UI. The tests are focused, and the structure makes it clear which part of the UI each test is validating.Also applies to: 112-144
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/features/share/widgets/share_items_bottom_sheet.dart (1)
207-224: AsyncValue handling improved, but error handling is still missing.The previous review flagged improper
AsyncValuehandling. This has been addressed by usingawait ref.read(currentUserProvider.future), which correctly awaits the async value.However, error handling for the
updateSheetShareInfocall is still missing. If the update fails, the share proceeds silently with potentially stale data, and the user receives no feedback.Consider wrapping in try-catch:
if (widget.isSheet) { - final currentUser = await ref.read(currentUserProvider.future); - if (currentUser != null) { - final userName = currentUser.name; - ref - .read(sheetListProvider.notifier) - .updateSheetShareInfo( - sheetId: widget.parentId, - sharedBy: userName, - message: _messageController.text.trim().isNotEmpty - ? _messageController.text.trim() - : null, - ); + try { + final currentUser = await ref.read(currentUserProvider.future); + if (currentUser != null) { + final userName = currentUser.name; + ref.read(sheetListProvider.notifier).updateSheetShareInfo( + sheetId: widget.parentId, + sharedBy: userName, + message: _messageController.text.trim().isNotEmpty + ? _messageController.text.trim() + : null, + ); + } + } catch (e) { + // Log error or show feedback - share still proceeds } }
🧹 Nitpick comments (2)
lib/features/share/widgets/sheet_share_preview_widget.dart (2)
15-30: UnusedcontentTextparameter.The
contentTextparameter is declared and required in the constructor but is never used within the widget. This appears to be dead code.Consider removing it if it's no longer needed:
class SheetSharePreviewWidget extends ConsumerStatefulWidget { final String parentId; - final String contentText; final TextEditingController messageController; const SheetSharePreviewWidget({ super.key, required this.parentId, - required this.contentText, required this.messageController, });Note: This would require updating all call sites that pass
contentText.
73-74: Duplicate comment.The comment
// Message Text Fieldis repeated twice on consecutive lines.- // Message Text Field // Message Text Field AnimatedTextField(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/share/widgets/share_items_bottom_sheet.dart(5 hunks)lib/features/share/widgets/sheet_share_preview_widget.dart(3 hunks)test/features/share/widgets/sheet_share_preview_widget_test.dart(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/features/share/widgets/sheet_share_preview_widget_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). (8)
- GitHub Check: test
- GitHub Check: format
- GitHub Check: windows
- GitHub Check: Test zoe_native plugin
- GitHub Check: macos
- GitHub Check: ios
- GitHub Check: android
- GitHub Check: linux
🔇 Additional comments (6)
lib/features/share/widgets/sheet_share_preview_widget.dart (2)
75-83: Controller-based approach resolves the previous callback issue.The previous review flagged that
onMessageChangedcallback was never properly invoked. This has been correctly addressed by switching to a controller-based approach where the parent owns_messageControllerand passes it directly. This is a cleaner pattern as the parent can read_messageController.textat any time without needing callback synchronization.
46-61: Filtering logic is correct.The filtering by
widget.parentIdfor events, tasks, documents, and polls is implemented correctly, and theisTodaychecks for deriving today's counts are appropriate.lib/features/share/widgets/share_items_bottom_sheet.dart (4)
49-62: Good lifecycle management for TextEditingController.The
_messageControlleris properly initialized ininitStateand disposed indispose, preventing memory leaks.
143-151: Previous review concern resolved.The previous review flagged improper
AsyncValuehandling in_getSheetShareMessage. This has been addressed by removing thecurrentUserProviderdependency entirely from this method - the user name is now only used in_buildShareButtonwhere it's properly awaited.
109-120: Sheet info widget renders correctly.The
_getSheetInfomethod properly watches the sheet provider, handles the null case, and displays the avatar and title appropriately.
93-98: Verify contentText is reactive to message changes.
contentTextis computed once during build using_getSheetShareMessage(ref), which reads_messageController.text. However, since_messageControlleris not listened to with asetStatetrigger, thecontentTextwon't update as the user types.If the share message should reflect the latest typed message, consider adding a listener:
@override void initState() { super.initState(); _messageController = TextEditingController(); + _messageController.addListener(() => setState(() {})); }Alternatively, if the current text is only needed at share-time (which appears to be the case given line 217-218 reads from the controller directly), this may be intentional and the preview text is expected to remain static. Please verify the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/features/share/widgets/sheet_share_preview_widget_test.dart (1)
40-55: Use theonMessageChangedparameter instead of hardcoded callback.The helper function accepts
onMessageChangedas a parameter (line 45) but ignores it and always passes a hardcoded empty callback(message) {}on line 52. This prevents tests from verifying that the callback is invoked with the correct values.Apply this diff to use the parameter:
Future<void> pumpSheetSharePreviewWidget( WidgetTester tester, { required String parentId, required String contentText, ProviderContainer? testContainer, ValueChanged<String>? onMessageChanged, }) async { await tester.pumpMaterialWidgetWithProviderScope( container: testContainer ?? container, child: SheetSharePreviewWidget( parentId: parentId, contentText: contentText, - onMessageChanged: (message) {}, + onMessageChanged: onMessageChanged ?? (message) {}, ), ); }
♻️ Duplicate comments (1)
lib/features/share/widgets/share_items_bottom_sheet.dart (1)
197-213: Add error handling forupdateSheetShareInfo.The call to
updateSheetShareInfo(lines 203-211) lacks error handling. If it fails, the share action will still proceed with potentially stale metadata, and the user won't be notified of the failure.Apply this diff to add error handling:
onPressed: () async { if (widget.isSheet) { // Try to get the current user; if not available, fallback to logged in user ID final currentUser = await ref.read(currentUserProvider.future); if (currentUser != null) { final userName = currentUser.name; - ref - .read(sheetListProvider.notifier) - .updateSheetShareInfo( - sheetId: widget.parentId, - sharedBy: userName, - message: _userMessage.trim().isNotEmpty - ? _userMessage.trim() - : null, - ); + try { + ref + .read(sheetListProvider.notifier) + .updateSheetShareInfo( + sheetId: widget.parentId, + sharedBy: userName, + message: _userMessage.trim().isNotEmpty + ? _userMessage.trim() + : null, + ); + } catch (e) { + // Log error or show user feedback + debugPrint('Failed to update sheet share info: $e'); + } } } CommonUtils.shareText(contentText, subject: l10n.share);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/share/widgets/share_items_bottom_sheet.dart(5 hunks)lib/features/share/widgets/sheet_share_preview_widget.dart(3 hunks)test/features/share/widgets/sheet_share_preview_widget_test.dart(5 hunks)
🔇 Additional comments (8)
test/features/share/widgets/sheet_share_preview_widget_test.dart (2)
99-123: LGTM!The test correctly verifies all AnimatedTextField properties including localized hint text, input configuration, and focus behavior.
141-162: LGTM!The test correctly verifies that clearing the text field updates the controller state appropriately.
lib/features/share/widgets/sheet_share_preview_widget.dart (2)
36-49: LGTM!The lifecycle management is correct. The controller listener properly invokes the callback with the current text, which addresses the previous review comment about the callback not being called.
63-74: LGTM!The filtering logic correctly uses
widget.parentIdconsistently across all entity types (events, tasks, documents, polls).lib/features/share/widgets/share_items_bottom_sheet.dart (4)
34-50: LGTM!The migration to a stateful widget is appropriate for managing the user message state, and the implementation is correct.
101-112: LGTM!The sheet info rendering correctly displays the avatar and title with appropriate null handling.
197-217: Good improvement on AsyncValue handling.Using
await ref.read(currentUserProvider.future)(line 200) properly handles the AsyncValue states, which addresses the previous review comment. Thecontext.mountedcheck (line 215) is also good practice.
135-141: The review comment is incorrect and should be dismissed.The method signature at
lib/features/share/utils/share_utils.dart:57shows thatgetSheetShareMessageaccepts only three parameters:ref,parentId, and optionaluserMessage. There is nouserNameparameter. The method internally derives the username viaref.watch(currentUserProvider).value?.namewithin its implementation.The current code at lines 135-141 correctly passes all required parameters (
ref,parentId) and optionally passesuserMessage. This aligns with the actual API signature and matches the pattern used in other call sites (e.g.,lib/features/sheet/actions/sheet_actions.dart:111and the test case).Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/features/share/widgets/sheet_share_preview_widget_test.dart (1)
40-55: WireonMessageChangedthrough the test helper.
pumpSheetSharePreviewWidgetaccepts anonMessageChangedparameter (Line 45) but always passes a no-op closure toSheetSharePreviewWidget(Line 52). This makes the parameter misleading and prevents tests from asserting callback behavior.Consider forwarding the parameter:
Future<void> pumpSheetSharePreviewWidget( WidgetTester tester, { required String parentId, required String contentText, ProviderContainer? testContainer, - ValueChanged<String>? onMessageChanged, + ValueChanged<String>? onMessageChanged, }) async { await tester.pumpMaterialWidgetWithProviderScope( container: testContainer ?? container, child: SheetSharePreviewWidget( parentId: parentId, contentText: contentText, - onMessageChanged: (message) {}, + onMessageChanged: onMessageChanged, ), ); }This keeps the helper honest and enables future tests to verify
onMessageChangedwiring if needed.Please re-run the widget tests (e.g.,
flutter test test/features/share/widgets/sheet_share_preview_widget_test.dart) after this change to confirm behavior is unchanged.lib/features/share/widgets/sheet_share_preview_widget.dart (2)
48-51: Reconsider whenonMessageChangedis fired (only on submit today).Right now
onMessageChangedis invoked only inonSubmitted(Lines 92–97). If the parent is expected to always have the latest message (e.g., when the user taps a separate "Share" button without submitting from the keyboard), this can lead to stale or empty values unless the user explicitly submits the field.Two possible improvements:
- Attach a listener to
_messageControllerininitStatethat callswidget.onMessageChangedon every change, and droponSubmitted, or- If
AnimatedTextFieldexposes anonChangedcallback, map that directly toonMessageChanged.If the intended behavior is truly "only fire when submitted", consider documenting that in the widget’s doc comment so the contract is clear to callers.
Please verify against your share flow (and its tests) whether you rely on live updates or only on submit, and adjust wiring accordingly.
Also applies to: 60-71, 77-101
88-101: Optional: remove no-oponErrorChangedif not required.
onErrorChanged: (error) {},is currently a no-op. If validation errors are not used for this field andonErrorChangedis optional onAnimatedTextField, you can omit this parameter entirely to reduce noise.Check the
AnimatedTextFieldAPI: ifonErrorChangedis non-nullable/required, keeping the no-op is fine; otherwise consider removing it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/share/widgets/share_items_bottom_sheet.dart(5 hunks)lib/features/share/widgets/sheet_share_preview_widget.dart(3 hunks)test/features/share/widgets/sheet_share_preview_widget_test.dart(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/share/widgets/share_items_bottom_sheet.dart
🔇 Additional comments (3)
test/features/share/widgets/sheet_share_preview_widget_test.dart (1)
69-71: AnimatedTextField tests now assert real behavior and props.The new "Message Input Field" tests correctly:
- Assert that
AnimatedTextFieldis rendered.- Verify key properties (hint text from
l10n.addAMessage,maxLines,keyboardType,autofocus).- Check that
tester.enterTextupdates the underlying controller text, and that clearing input resets it.This replaces the previous trivial assertion and gives good coverage of the input field’s behavior.
Please keep these tests in sync with any future API changes to
AnimatedTextField(e.g., renaminghintTextor changing defaultmaxLines).Also applies to: 98-123, 125-165
lib/features/share/widgets/sheet_share_preview_widget.dart (2)
15-47: Stateful conversion and controller lifecycle look correct.Switching to
ConsumerStatefulWidgetand managing_messageControllerininitState/disposeis done properly and keeps the text field’s state local to the widget. This is a clean way to support richer message input while preserving provider usage viaref.After any further tweaks to this widget, rerun the share-related widget tests to ensure there are no regressions in the preview UI.
103-137: Localized stat cards and sheet-based filtering are consistent.Using
widget.parentIdto filter events/tasks/documents/polls and driving labels/subtitles froml10n(events,tasks,documents,polls,today,dueToday) keeps the stats aligned with sheet context and localization. The_buildStatCardusage is straightforward and matches expectations from the tests.If you later add more sheet-scoped entities, following this same pattern (filtered lists + localized labels) will keep the UI consistent.
2 participants
Summary by CodeRabbit
New Features
Accessibility / UI
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.