Skip to content

Conversation

@nqhhdev
Copy link
Member

@nqhhdev nqhhdev commented Nov 20, 2025

Ticket

Resolved

Attach screenshots or videos demonstrating the changes

  • Web:
  • Android:
  • IOS:
Screen.Recording.2025-11-26.at.11.02.19.mov

Summary by CodeRabbit

  • New Features

    • New Contacts Visibility settings page under Privacy & Security — control who can see your phone/email (Everyone, My contacts, Nobody) and choose visibility per field.
  • Localization

    • Added English strings for contacts visibility options, field labels, and error messages.
  • Navigation / UI

    • Contacts visibility added to Security settings and navigation; improved navigation identifiers for more reliable UI flow.
  • Tests

    • Added integration tests covering the contacts visibility UI and flows.

✏️ Tip: You can customize this high-level summary in your review settings.

@github-actions
Copy link
Contributor

This PR has been deployed to https://linagora.github.io/twake-on-matrix/2711

@nqhhdev nqhhdev force-pushed the TW-2710-settings-to-display-or-hide-contact-info branch from 22d687c to 30e8df2 Compare December 10, 2025 02:31
@nqhhdev nqhhdev marked this pull request as ready for review December 10, 2025 02:32
@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

Adds a contacts visibility feature across the app: new localization keys; a SettingsContactsVisibility screen, view, controller, enums, and UI extensions; domain models UserInfoVisibility and UserInfoVisibilityRequest; repository, datasource, API, and interactor methods for getting/updating visibility; dependency injection registrations; routing for the new screen; navigation keys for testability; and new integration test robots and tests covering the settings flow.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is incomplete. It only includes ticket reference and a demo video link, missing critical sections like Root cause, Solution, Impact description, Test recommendations, and Pre-merge checklist from the template. Complete the PR description using the template: add Root cause (if applicable), Solution overview, Impact description, Test recommendations, and Pre-merge checklist sections.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding settings for contact info visibility management.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch TW-2710-settings-to-display-or-hide-contact-info

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (3)
lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_enum.dart (1)

20-29: Consider simplifying the logic.

Both public and contacts return true, which can be simplified for better maintainability.

Apply this diff to simplify:

  bool enableDivider() {
-   switch (this) {
-     case SettingsContactsVisibilityEnum.public:
-       return true;
-     case SettingsContactsVisibilityEnum.contacts:
-       return true;
-     case SettingsContactsVisibilityEnum.private:
-       return false;
-   }
+   return this != SettingsContactsVisibilityEnum.private;
  }
lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart (1)

242-242: Fix typo in method name: _buildVisibleFieldIem_buildVisibleFieldItem

-  Widget _buildVisibleFieldIem({
+  Widget _buildVisibleFieldItem({

Also update the call site at line 152:

-                                        return _buildVisibleFieldIem(
+                                        return _buildVisibleFieldItem(
integration_test/robots/setting/settings_contacts_visibility_robot.dart (1)

11-13: Fragile finder for back button.

$(IconButton).containing($(Icon)) could match any IconButton containing an Icon in the widget tree, leading to flaky tests if multiple such buttons exist. Consider using a more specific key-based finder.

If a key is added to the back button in the view (e.g., Key('contacts_visibility_back_button')), update the finder:

  PatrolFinder backButton() {
-   return $(IconButton).containing($(Icon));
+   return $(Key('contacts_visibility_back_button'));
  }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2027982 and 30e8df2.

📒 Files selected for processing (27)
  • assets/l10n/intl_en.arb (1 hunks)
  • integration_test/robots/home_robot.dart (3 hunks)
  • integration_test/robots/setting/setting_robot.dart (3 hunks)
  • integration_test/robots/setting/settings_contacts_visibility_robot.dart (1 hunks)
  • integration_test/robots/setting/settings_privacy_and_security_robot.dart (1 hunks)
  • integration_test/tests/setting/settings_contacts_visibility_test.dart (1 hunks)
  • lib/config/go_routes/go_router.dart (2 hunks)
  • lib/data/datasource/user_info/user_info_datasource.dart (1 hunks)
  • lib/data/datasource_impl/user_info/user_info_datasource_impl.dart (2 hunks)
  • lib/data/network/user_info/user_info_api.dart (2 hunks)
  • lib/data/repository/user_info_repository_impl.dart (2 hunks)
  • lib/di/global/get_it_initializer.dart (2 hunks)
  • lib/domain/app_state/user_info/get_user_info_visibility_state.dart (1 hunks)
  • lib/domain/app_state/user_info/update_user_info_visibility_state.dart (1 hunks)
  • lib/domain/model/user_info/user_info_visibility.dart (1 hunks)
  • lib/domain/model/user_info/user_info_visibility_request.dart (1 hunks)
  • lib/domain/repository/user_info/user_info_repository.dart (1 hunks)
  • lib/domain/usecase/user_info/get_user_info_visibility_interactor.dart (1 hunks)
  • lib/domain/usecase/user_info/update_user_info_visibility_interactor.dart (1 hunks)
  • lib/pages/settings_dashboard/settings/settings_item_builder.dart (1 hunks)
  • lib/pages/settings_dashboard/settings/settings_view.dart (1 hunks)
  • lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility.dart (1 hunks)
  • lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_enum.dart (1 hunks)
  • lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart (1 hunks)
  • lib/pages/settings_dashboard/settings_security/settings_security_view.dart (1 hunks)
  • lib/widgets/avatar/bottom_navigation_avatar.dart (1 hunks)
  • lib/widgets/layouts/enum/adaptive_destinations_enum.dart (4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-09T06:19:59.615Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2761
File: lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart:49-57
Timestamp: 2025-12-09T06:19:59.615Z
Learning: For the Twake on Matrix Flutter app (fluffychat), desktop platforms (macOS, Linux, Windows) are not supported. During reviews, assume only web and mobile targets and avoid introducing desktop-specific builds, dependencies, or platform checks. This guideline applies across Dart source files in the repository.

Applied to files:

  • lib/pages/settings_dashboard/settings_security/settings_security_view.dart
  • lib/domain/usecase/user_info/get_user_info_visibility_interactor.dart
  • lib/data/datasource/user_info/user_info_datasource.dart
  • lib/domain/model/user_info/user_info_visibility_request.dart
  • lib/domain/usecase/user_info/update_user_info_visibility_interactor.dart
  • lib/domain/repository/user_info/user_info_repository.dart
  • lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_enum.dart
  • lib/data/datasource_impl/user_info/user_info_datasource_impl.dart
  • lib/domain/app_state/user_info/update_user_info_visibility_state.dart
  • lib/di/global/get_it_initializer.dart
  • integration_test/robots/setting/setting_robot.dart
  • lib/config/go_routes/go_router.dart
  • integration_test/robots/home_robot.dart
  • lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility.dart
  • lib/data/repository/user_info_repository_impl.dart
  • lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart
  • lib/domain/app_state/user_info/get_user_info_visibility_state.dart
  • integration_test/robots/setting/settings_privacy_and_security_robot.dart
  • lib/data/network/user_info/user_info_api.dart
  • lib/domain/model/user_info/user_info_visibility.dart
  • lib/widgets/layouts/enum/adaptive_destinations_enum.dart
  • integration_test/robots/setting/settings_contacts_visibility_robot.dart
  • lib/widgets/avatar/bottom_navigation_avatar.dart
  • lib/pages/settings_dashboard/settings/settings_item_builder.dart
  • integration_test/tests/setting/settings_contacts_visibility_test.dart
  • lib/pages/settings_dashboard/settings/settings_view.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). (3)
  • GitHub Check: Analyze code
  • GitHub Check: Widget test
  • GitHub Check: Deploy preview versions on pull requests
🔇 Additional comments (27)
lib/pages/settings_dashboard/settings/settings_item_builder.dart (1)

42-42: LGTM!

The key forwarding to TwakeInkWell is correct and will enable proper widget identification for testing purposes.

lib/config/go_routes/go_router.dart (1)

452-459: LGTM!

The route configuration follows the established patterns in the codebase, is properly nested under the security settings group, and includes appropriate redirect logic.

lib/pages/settings_dashboard/settings/settings_view.dart (1)

184-184: LGTM!

Using Key(item.name) provides stable, unique keys for each settings item, which is beneficial for testing and widget identification.

lib/pages/settings_dashboard/settings_security/settings_security_view.dart (1)

63-77: LGTM!

The contacts visibility settings item is properly implemented following the established patterns in the security settings view. The inclusion of a test key and proper navigation setup is good practice.

lib/widgets/layouts/enum/adaptive_destinations_enum.dart (1)

22-22: LGTM!

Adding keys to navigation destinations improves testability without affecting functionality. The key naming convention is consistent and follows Flutter best practices.

Also applies to: 35-35, 48-48, 94-94, 99-99

assets/l10n/intl_en.arb (1)

3493-3494: Fix grammatical errors in user-facing strings.

The localization keys contain typos:

  • Line 3493: "youNumberIsVisibleAccordingToTheSettingAbove" → should be "Your number" or "Your phone number"
  • Line 3494: "youEmailIsVisibleAccordingToTheSettingAbove" → should be "Your email"

Apply this diff to fix the strings:

-  "youNumberIsVisibleAccordingToTheSettingAbove": "Your number is visible according to the setting above",
+  "youNumberIsVisibleAccordingToTheSettingAbove": "Your phone number is visible according to the setting above",
   "@youNumberIsVisibleAccordingToTheSettingAbove": {},
-  "youEmailIsVisibleAccordingToTheSettingAbove": "Your email is visible according to the setting above",
+  "youEmailIsVisibleAccordingToTheSettingAbove": "Your email address is visible according to the setting above",

Likely an incorrect or invalid review comment.

lib/data/repository/user_info_repository_impl.dart (1)

16-30: No action needed—error handling is properly implemented across the data and domain layers.

The datasource and use-case layers already handle exceptions correctly. The API layer (UserInfoApi) uses .onError() to re-throw errors with proper type preservation, and both interactors (GetUserInfoVisibilityInteractor and UpdateUserInfoVisibilityInteractor) wrap repository calls in try-catch blocks, converting exceptions to Either<Failure, Success> using the dartz pattern. This is a standard clean architecture error handling approach where errors propagate from the API through the datasource and repository, then are caught and transformed at the use-case level.

Likely an incorrect or invalid review comment.

integration_test/robots/setting/setting_robot.dart (1)

6-6: LGTM! Improved test robustness with Key-based finder.

The rename from "privateAndSecuritySetting" to "privacyAndSecuritySetting" fixes the typo, and switching from a text-based finder to a Key-based finder makes the test more robust against localization changes.

Also applies to: 24-26, 58-58

integration_test/robots/home_robot.dart (1)

2-2: LGTM! Key-based finder improves test maintainability.

The switch to a Key-based finder for the settings tab makes the test more resilient to UI text changes and localization.

Also applies to: 21-21, 40-41

lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_enum.dart (1)

9-18: LGTM! Localized title mapping is correct.

The title method properly maps each enum value to its localized string.

lib/domain/repository/user_info/user_info_repository.dart (1)

2-3: LGTM! Clean interface extension.

The new methods for user visibility management are well-defined and follow the existing repository pattern.

Also applies to: 8-13

lib/data/datasource_impl/user_info/user_info_datasource_impl.dart (1)

5-6: LGTM! Consistent datasource implementation.

The new methods follow the existing delegation pattern to the API layer.

Also applies to: 17-30

lib/data/network/user_info/user_info_api.dart (2)

46-69: Verify whether visibility data should be cached.

The getUserInfo method uses MatrixDioCacheInterceptor to cache responses, but getUserVisibility does not. If visibility settings can change frequently and should always reflect the latest state, omitting the cache is correct. However, if caching would improve performance without staleness concerns, consider adding it.


71-98: LGTM! Update method follows the established pattern.

The implementation correctly uses POST for updating visibility settings and maintains consistent error handling with other API methods.

integration_test/tests/setting/settings_contacts_visibility_test.dart (1)

6-66: LGTM! Well-structured integration tests.

The tests clearly verify the presence of each visibility option and related UI elements. The separation into distinct tests makes it easy to identify which specific feature fails if a test breaks.

lib/domain/usecase/user_info/update_user_info_visibility_interactor.dart (1)

12-28: LGTM! Standard interactor pattern correctly implemented.

The interactor follows the established pattern in the codebase with appropriate state emissions and error handling.

lib/di/global/get_it_initializer.dart (1)

138-139: DI wiring for visibility interactors is consistent

The new imports and registerFactory bindings for GetUserInfoVisibilityInteractor and UpdateUserInfoVisibilityInteractor follow the existing interactor pattern and look correct.

Also applies to: 539-541

integration_test/robots/setting/settings_privacy_and_security_robot.dart (1)

1-18: Robot for navigating to Contacts Visibility screen looks good

The finder key and the openContactVisibilitySetting() flow (tap + waitUntilVisible on SettingsContactsVisibilityView) align with existing robot patterns and should be reliable for integration tests.

lib/domain/usecase/user_info/get_user_info_visibility_interactor.dart (1)

1-24: GetUserInfoVisibility interactor follows existing pattern

The interactor correctly emits GettingUserInfoVisibility followed by either GetUserInfoVisibilitySuccess or GetUserInfoVisibilityFailure, and uses UserInfoRepository.getUserVisibility via DI. This is consistent with other interactors and looks fine.

lib/data/datasource/user_info/user_info_datasource.dart (1)

2-13: UserInfoDatasource visibility API shape is appropriate

Extending the contract with getUserVisibility and updateUserInfoVisibility using UserInfoVisibility / UserInfoVisibilityRequest matches the new use cases and keeps the abstraction clean.

lib/domain/model/user_info/user_info_visibility_request.dart (1)

1-29: UserInfoVisibilityRequest model and JSON mapping look solid

The model cleanly represents the payload with optional visibility and visible_fields, uses json_serializable correctly, and defines value equality via EquatableMixin. No issues from a domain or serialization perspective.

lib/domain/app_state/user_info/update_user_info_visibility_state.dart (1)

1-26: Update visibility state classes are consistent with existing patterns

UpdatingUserInfoVisibility, UpdateUserInfoVisibilitySuccess, and UpdateUserInfoVisibilityFailure line up with the get-visibility states and the interactor flow; equality via props is correctly wired.

lib/domain/app_state/user_info/get_user_info_visibility_state.dart (1)

1-26: Get visibility state classes are well-aligned with the interactor

GettingUserInfoVisibility, GetUserInfoVisibilitySuccess, and GetUserInfoVisibilityFailure provide the expected loading/success/failure trio and match how the interactor emits states. No changes needed here.

lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart (2)

76-99: Outer ValueListenableBuilder appears unused.

The state from getUserInfoVisibilityNotifier is captured but never referenced in the inner builder. If this notifier is only needed for initial data loading, consider whether this builder is necessary here or if the logic should be restructured.

Please verify if getUserInfoVisibilityNotifier is intended to trigger rebuilds here. If not, consider removing the outer builder to simplify the widget tree.


11-14: LGTM on the overall structure and widget key usage.

Good practice using Key for visibility options and fields—this aids testability and integration test automation. The controller-view separation is clean.

Also applies to: 182-240, 242-316

integration_test/robots/setting/settings_contacts_visibility_robot.dart (2)

16-79: LGTM on finder organization and naming.

The finders are well-organized with clear naming conventions, and the key-based approach using enum names ensures consistency with the view's widget keys.


82-105: LGTM on action methods.

The action methods properly await taps and use waitUntilVisible or pumpAndSettle to ensure UI stability before proceeding.

Also applies to: 158-160

@chibenwa
Copy link
Member

Looks like the rabbit had very valuable comments

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility.dart (1)

214-218: Consider calling super.initState() first.

Flutter convention is to call super.initState() before performing initialization logic. While this works, reordering improves consistency with Flutter patterns.

  @override
  void initState() {
+   super.initState();
    initialGetUserInfoVisibility();
-   super.initState();
  }
integration_test/robots/setting/settings_contacts_visibility_robot.dart (1)

11-13: Consider using a more specific finder for backButton().

$(IconButton).containing($(Icon)) is generic and could match multiple widgets if the screen has more than one IconButton. Consider using a Key or semantic identifier for more reliable test targeting.

  PatrolFinder backButton() {
-   return $(IconButton).containing($(Icon));
+   return $(Key('settings_contacts_visibility_back_button'));
  }

This requires adding a corresponding Key to the back button widget in the view.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 30e8df2 and 4e828bd.

📒 Files selected for processing (6)
  • integration_test/robots/setting/settings_contacts_visibility_robot.dart (1 hunks)
  • lib/domain/model/user_info/user_info_visibility.dart (1 hunks)
  • lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility.dart (1 hunks)
  • lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart (1 hunks)
  • lib/presentation/extensions/settings/user_info_visibility_extension.dart (1 hunks)
  • lib/widgets/avatar/bottom_navigation_avatar.dart (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility_view.dart
  • lib/domain/model/user_info/user_info_visibility.dart
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-10T04:59:56.814Z
Learnt from: chibenwa
Repo: linagora/twake-on-matrix PR: 2711
File: lib/domain/model/user_info/user_info_visibility.dart:8-38
Timestamp: 2025-12-10T04:59:56.814Z
Learning: In the fluffychat codebase, domain models (in lib/domain/model/) should remain UI-agnostic and not contain UI-related methods that depend on BuildContext or L10n. UI presentation logic for domain enums should be implemented as extensions in the presentation layer (e.g., lib/presentation/extensions/).

Applied to files:

  • lib/presentation/extensions/settings/user_info_visibility_extension.dart
📚 Learning: 2025-12-09T06:19:59.615Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2761
File: lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart:49-57
Timestamp: 2025-12-09T06:19:59.615Z
Learning: For the Twake on Matrix Flutter app (fluffychat), desktop platforms (macOS, Linux, Windows) are not supported. During reviews, assume only web and mobile targets and avoid introducing desktop-specific builds, dependencies, or platform checks. This guideline applies across Dart source files in the repository.

Applied to files:

  • lib/presentation/extensions/settings/user_info_visibility_extension.dart
  • lib/widgets/avatar/bottom_navigation_avatar.dart
  • lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility.dart
  • integration_test/robots/setting/settings_contacts_visibility_robot.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). (3)
  • GitHub Check: Deploy preview versions on pull requests
  • GitHub Check: Widget test
  • GitHub Check: Analyze code
🔇 Additional comments (8)
lib/widgets/avatar/bottom_navigation_avatar.dart (1)

16-16: LGTM!

The blank line improves readability by providing visual separation between the constructor and the build method.

lib/presentation/extensions/settings/user_info_visibility_extension.dart (1)

1-27: LGTM! Proper separation of UI logic from domain model.

The extension correctly places UI-related methods (localized titles, subtitles, divider logic) in the presentation layer rather than in the domain model, following the established pattern. Based on learnings, this aligns with the codebase convention that domain models in lib/domain/model/ remain UI-agnostic.

lib/pages/settings_dashboard/settings_contacts_visibility/settings_contacts_visibility.dart (4)

220-228: Past review issues addressed: dispose properly cleans up resources.

The dispose method now correctly:

  • Cancels both getUserInfoVisibilityStreamSub and updateUserInfoVisibilityStreamSub
  • Disposes all four ValueNotifier instances

This prevents memory leaks and dangling subscriptions that could fire after widget disposal.


126-135: Past review issue addressed: null check guards against uninitialized client.

The null check for client.userID is now consistent between updateUserInfoVisibility() and initialGetUserInfoVisibility(), preventing potential null pointer exceptions if the client becomes uninitialized.


155-165: Past review issue addressed: selectedVisibleFieldNotifier now stays in sync.

The success handler correctly updates both selectedVisibilityOptionNotifier and selectedVisibleFieldNotifier with backend response data, ensuring subsequent toggles compute from current state.


136-137: Good practice: cancelling previous subscription before starting new one.

This prevents overlapping stream listeners and ensures only one update operation is in progress at a time.

integration_test/robots/setting/settings_contacts_visibility_robot.dart (2)

113-131: Past review issue addressed: state-check methods now work correctly.

The helper methods now directly return the .exists boolean value instead of wrapping in try/catch blocks that always returned true.


81-110: LGTM! Actions follow proper async testing patterns.

The action methods correctly use waitUntilVisible after selection taps to ensure state transitions complete, and pumpAndSettle for toggle operations. This approach prevents flaky tests from timing issues.

);
}

Widget _buildVisibleFieldIem({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Widget _buildVisibleFieldIem({
Widget _buildVisibleFieldItem({

Comment on lines +301 to +318
Padding(
padding: const EdgeInsets.only(
left: 8,
right: 16,
),
child: SizedBox(
width: 24,
height: 24,
child: isSelected
? Icon(
key: Key('visible_field_option_selected_${option.name}'),
Icons.check,
color: LinagoraSysColors.material().primary,
size: 24,
)
: null,
),
),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Padding(
padding: const EdgeInsets.only(
left: 8,
right: 16,
),
child: SizedBox(
width: 24,
height: 24,
child: isSelected
? Icon(
key: Key('visible_field_option_selected_${option.name}'),
Icons.check,
color: LinagoraSysColors.material().primary,
size: 24,
)
: null,
),
),
Container(
padding: const EdgeInsets.only(
left: 8,
right: 16,
),
width: 24,
height: 24,
child: isSelected
? Icon(
key: Key('visible_field_option_selected_${option.name}'),
Icons.check,
color: LinagoraSysColors.material().primary,
size: 24,
)
: null,
),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For lists or frequently rebuilt widgets, the performance difference compounds. SizedBox is specifically optimized for size constraints, while Container is a convenience wrapper that does more work under the hood.

https://claude.ai/share/bedde466-c0a5-48b3-b2e5-3497dfe19b5b

Comment on lines +225 to +242
Padding(
padding: const EdgeInsets.only(
left: 8,
right: 16,
),
child: SizedBox(
width: 24,
height: 24,
child: isSelected
? Icon(
key: Key('visibility_option_selected_${option.name}'),
Icons.check,
color: LinagoraSysColors.material().primary,
size: 24,
)
: null,
),
),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple ValueListenableBuilder with controller.selectedVisibleFieldNotifier. Should they be combined?

if (option == selectedVisibilityOptionNotifier.value) {
return;
}
switch (option) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why visibleFields: for each option is not consistent?

@tddang-linagora
Copy link
Collaborator

  • Update PR with working demo for "Everyone" option
  • Update PR with working demo for "My contacts" option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants