TW-2928 Reorder contacts list#2929
Open
tddang-linagora wants to merge 1 commit intosprint/march26from
Open
Conversation
tddang-linagora
requested review from
9clg6,
dab246,
hoangdat and
nqhhdev
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
Contributor
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2929 |
![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 (1)
lib/pages/contacts_tab/widgets/sliver_contacts_with_matrix_id.dart (1)
67-119: Consider unifying duplicated contact-tile implementations.
_ExternalContactTileand_ContactTileduplicate the same tile configuration; only sliver wrapping differs. Consolidating this reduces maintenance drift.♻️ Proposed refactor
-class _ExternalContactTile extends StatelessWidget { - const _ExternalContactTile({required this.controller, required this.contact}); +class _ContactTile extends StatelessWidget { + const _ContactTile({ + required this.contact, + required this.controller, + this.wrapWithSliver = false, + }); - final ContactsTabController controller; final PresentationContact contact; + final ContactsTabController controller; + final bool wrapWithSliver; `@override` Widget build(BuildContext context) { - return SliverToBoxAdapter( - child: Padding( - padding: const EdgeInsets.symmetric( - horizontal: ContactsTabViewStyle.padding, - ), - child: ExpansionContactListTile( - contact: contact, - highlightKeyword: controller.textEditingController.text, - enableInvitation: controller.supportInvitation(), - onContactTap: () => controller.onContactTap( - context: context, - path: 'rooms', - contact: contact, - ), - ), - ), - ); + final tile = Padding( + padding: const EdgeInsets.symmetric(horizontal: ContactsTabViewStyle.padding), + child: ExpansionContactListTile( + contact: contact, + highlightKeyword: controller.textEditingController.text, + enableInvitation: controller.supportInvitation(), + onContactTap: () => controller.onContactTap( + context: context, + path: 'rooms', + contact: contact, + ), + ), + ); + return wrapWithSliver ? SliverToBoxAdapter(child: tile) : tile; } }- return _ExternalContactTile( + return _ContactTile( controller: controller, contact: success.contact, + wrapWithSliver: true, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/contacts_tab/widgets/sliver_contacts_with_matrix_id.dart` around lines 67 - 119, Both _ExternalContactTile and _ContactTile duplicate the same ExpansionContactListTile configuration; refactor by creating a single reusable widget (e.g., ContactTile or ContactTileWrapper) that accepts the same PresentationContact and ContactsTabController props plus a flag or enum (e.g., bool asSliver) to decide whether to wrap the tile in SliverToBoxAdapter. Move the shared ExpansionContactListTile construction (including highlightKeyword, enableInvitation, and onContactTap usage) into that single widget and replace both _ExternalContactTile and _ContactTile usages with the new widget, passing asSliver=true for the sliver case and false for the regular case so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/pages/contacts_tab/widgets/sliver_empty_contacts.dart`:
- Around line 14-17: Update the _isLoading function so it also returns true
while the phonebook data is loading to avoid showing
EmptyContactBody/NoContactsFound prematurely; specifically, inside _isLoading
(which currently checks controller.presentationContactNotifier for
ContactsLoading), also check the controller's phonebook loading notifier/state
(e.g. controller.phonebookNotifier or controller.phonebookState) and return true
if that notifier indicates a loading state, combining the two checks so either
presentationContactNotifier is ContactsLoading OR the phonebook notifier is
loading.
---
Nitpick comments:
In `@lib/pages/contacts_tab/widgets/sliver_contacts_with_matrix_id.dart`:
- Around line 67-119: Both _ExternalContactTile and _ContactTile duplicate the
same ExpansionContactListTile configuration; refactor by creating a single
reusable widget (e.g., ContactTile or ContactTileWrapper) that accepts the same
PresentationContact and ContactsTabController props plus a flag or enum (e.g.,
bool asSliver) to decide whether to wrap the tile in SliverToBoxAdapter. Move
the shared ExpansionContactListTile construction (including highlightKeyword,
enableInvitation, and onContactTap usage) into that single widget and replace
both _ExternalContactTile and _ContactTile usages with the new widget, passing
asSliver=true for the sliver case and false for the regular case so behavior
remains identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: edd66a26-a6bd-4395-8411-4bc18dbab4ca
📒 Files selected for processing (9)
lib/pages/contacts_tab/contacts_tab_body_view.dartlib/pages/contacts_tab/widgets/sliver_contacts_with_matrix_id.dartlib/pages/contacts_tab/widgets/sliver_contacts_without_matrix_id.dartlib/pages/contacts_tab/widgets/sliver_empty_contacts.dartlib/pages/contacts_tab/widgets/sliver_loading_contacts.dartlib/pages/contacts_tab/widgets/sliver_phonebook_contacts_with_matrix_id.dartlib/pages/contacts_tab/widgets/sliver_phonebook_contacts_without_matrix_id.dartlib/pages/contacts_tab/widgets/sliver_recent_contacts.dartlib/pages/contacts_tab/widgets/sliver_warning_banner.dart
Comment on lines
+14
to
+17
| bool _isLoading() => controller.presentationContactNotifier.value.fold( | ||
| (_) => false, | ||
| (s) => s is ContactsLoading, | ||
| ); |
There was a problem hiding this comment.
Include phonebook loading in _isLoading to prevent premature empty-state rendering.
Right now, loading is inferred only from presentationContactNotifier. When phonebook data is still loading, the widget can briefly show EmptyContactBody / NoContactsFound before data arrives.
🐛 Proposed fix
- bool _isLoading() => controller.presentationContactNotifier.value.fold(
- (_) => false,
- (s) => s is ContactsLoading,
- );
+ bool _isLoading() {
+ final contactsLoading = controller.presentationContactNotifier.value.fold(
+ (_) => false,
+ (s) => s is ContactsLoading,
+ );
+ final phonebookLoading =
+ controller.presentationPhonebookContactNotifier.value.fold(
+ (_) => false,
+ (s) => s is ContactsLoading,
+ );
+ return contactsLoading || phonebookLoading;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/pages/contacts_tab/widgets/sliver_empty_contacts.dart` around lines 14 -
17, Update the _isLoading function so it also returns true while the phonebook
data is loading to avoid showing EmptyContactBody/NoContactsFound prematurely;
specifically, inside _isLoading (which currently checks
controller.presentationContactNotifier for ContactsLoading), also check the
controller's phonebook loading notifier/state (e.g. controller.phonebookNotifier
or controller.phonebookState) and return true if that notifier indicates a
loading state, combining the two checks so either presentationContactNotifier is
ContactsLoading OR the phonebook notifier is loading.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
Resolved
contact.mp4
Summary by CodeRabbit