Conversation
9clg6
requested review from
dab246,
hoangdat,
nqhhdev and
tddang-linagora
as code owners
WalkthroughSearch-related widgets were converted from StatelessWidget to StatefulWidget and now fetch external Matrix profile information asynchronously via a Matrix client (injectable via a test-only parameter). Lifecycle methods (didChangeDependencies, didUpdateWidget) trigger profile fetches when inputs change. UI uses FutureBuilder to render loading, error/empty, or populated contact views, builds PresentationContact from fetched profile data (falling back to keyword-derived display name), and forwards the resolved contact on taps. New widget and integration tests cover loading, not-found, and success scenarios. Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2918 |
9clg6
force-pushed
the
feature/TW-2474-improve-searching-non-exist-matrix-address
branch
from
577e116 to
adcde8a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/pages/search/search_external_contact.dart (1)
60-72: FutureBuilder implementation correctly handles all connection states.The three-state pattern is sound. Consider adding error logging to help debug issues in production:
📝 Optional: Add error logging for debugging
if (snapshot.hasError || !snapshot.hasData) { + if (snapshot.hasError) { + Logs().v( + 'SearchExternalContactWidget: Failed to fetch profile for ${widget.keyword}', + snapshot.error, + ); + } return const EmptySearchWidget(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/search/search_external_contact.dart` around lines 60 - 72, The FutureBuilder in SearchExternalContact (using _profileFuture and the snapshot in the builder) currently returns EmptySearchWidget when snapshot.hasError or !snapshot.hasData; add an error logging call inside that error branch to record snapshot.error and snapshot.stackTrace (or snapshot.error.toString()) with a clear context message before returning the EmptySearchWidget so production logs capture the failure details for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/pages/search/search_external_contact.dart`:
- Around line 60-72: The FutureBuilder in SearchExternalContact (using
_profileFuture and the snapshot in the builder) currently returns
EmptySearchWidget when snapshot.hasError or !snapshot.hasData; add an error
logging call inside that error branch to record snapshot.error and
snapshot.stackTrace (or snapshot.error.toString()) with a clear context message
before returning the EmptySearchWidget so production logs capture the failure
details for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e424ddb7-847d-433b-8a12-e94d305f6289
📒 Files selected for processing (1)
lib/pages/search/search_external_contact.dart
Collaborator
This is your test recommendations in the PR description, but the current demo is no where near any of them. Please update the demo accordingly. |
hoangdat
reviewed
Mar 17, 2026
hoangdat
reviewed
Mar 17, 2026
9clg6
force-pushed
the
feature/TW-2474-improve-searching-non-exist-matrix-address
branch
from
adcde8a to
1b4e02b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/pages/search/search_external_contact_test.dart (1)
149-217: Comprehensive test coverage for all three states.The tests correctly validate:
- Loading: Uses a never-completing
Completerto keep the future pending- Error: Simulates
M_NOT_FOUNDviaMatrixException.fromJson- Success: Verifies profile displayName is rendered
Missing test case: The implementation at line 80 uses
profile.displayname ?? widget.keyword.substring(1)as a fallback when displayname isnull. Add a test to verify this fallback behavior—whengetUserProfilereturns a valid profile withdisplayname: null, the tile should display the keyword (minus the "@" prefix).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/pages/search/search_external_contact_test.dart` around lines 149 - 217, Add a test that stubs mockClient.getUserProfile (used in buildWidget) to return a CachedProfileInformation.fromProfile where ProfileInformation has displayname: null; call buildWidget with keyword '@validuser:server.com', pumpAndSettle, then assert ExpansionContactListTile is shown and that the rendered text contains the fallback keyword without the leading '@' (i.e., widget.keyword.substring(1) -> 'validuser:server.com'), verifying the profile.displayname ?? widget.keyword.substring(1) fallback path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/pages/search/search_external_contact_test.dart`:
- Around line 149-217: Add a test that stubs mockClient.getUserProfile (used in
buildWidget) to return a CachedProfileInformation.fromProfile where
ProfileInformation has displayname: null; call buildWidget with keyword
'@validuser:server.com', pumpAndSettle, then assert ExpansionContactListTile is
shown and that the rendered text contains the fallback keyword without the
leading '@' (i.e., widget.keyword.substring(1) -> 'validuser:server.com'),
verifying the profile.displayname ?? widget.keyword.substring(1) fallback path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a6d19ba-2fd1-4a6f-b9d7-39eb2bda9803
📒 Files selected for processing (4)
integration_test/tests/chat/search_external_mxid_test.dartlib/pages/contacts_tab/contacts_tab_body_view.dartlib/pages/search/search_external_contact.darttest/pages/search/search_external_contact_test.dart
9clg6
force-pushed
the
feature/TW-2474-improve-searching-non-exist-matrix-address
branch
from
1b4e02b to
92e6932
Compare
9clg6
force-pushed
the
feature/TW-2474-improve-searching-non-exist-matrix-address
branch
from
92e6932 to
4732672
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/pages/search/search_external_contact_test.dart (1)
64-109: Stabilize GetIt state per test case.New mocks are created in every
setUp, but singleton registration is skipped after first test dueisRegistered, which can leak DI state and stubs across cases. Prefer resetting GetIt and re-registering dependencies per test.Suggested fix
- setUp(() { + setUp(() async { final getIt = GetIt.instance; + await getIt.reset(); @@ - if (!getIt.isRegistered<HiveGetInvitationStatusInteractor>()) { - getIt.registerSingleton<HiveGetInvitationStatusInteractor>( - MockHiveGetInvitationStatusInteractor(), - ); - } - if (!getIt.isRegistered<GetInvitationStatusInteractor>()) { - getIt.registerSingleton<GetInvitationStatusInteractor>( - MockGetInvitationStatusInteractor(), - ); - } - if (!getIt.isRegistered<PostAddressBookInteractor>()) { - getIt.registerSingleton<PostAddressBookInteractor>( - MockPostAddressBookInteractor(), - ); - } - if (!getIt.isRegistered<HiveDeleteInvitationStatusInteractor>()) { - getIt.registerSingleton<HiveDeleteInvitationStatusInteractor>( - MockHiveDeleteInvitationStatusInteractor(), - ); - } - if (!getIt.isRegistered<DeleteThirdPartyContactBoxInteractor>()) { - getIt.registerSingleton<DeleteThirdPartyContactBoxInteractor>( - MockDeleteThirdPartyContactBoxInteractor(), - ); - } + getIt.registerSingleton<HiveGetInvitationStatusInteractor>( + MockHiveGetInvitationStatusInteractor(), + ); + getIt.registerSingleton<GetInvitationStatusInteractor>( + MockGetInvitationStatusInteractor(), + ); + getIt.registerSingleton<PostAddressBookInteractor>( + MockPostAddressBookInteractor(), + ); + getIt.registerSingleton<HiveDeleteInvitationStatusInteractor>( + MockHiveDeleteInvitationStatusInteractor(), + ); + getIt.registerSingleton<DeleteThirdPartyContactBoxInteractor>( + MockDeleteThirdPartyContactBoxInteractor(), + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/pages/search/search_external_contact_test.dart` around lines 64 - 109, Reset the GetIt container at the start of each setUp and register all required singletons unconditionally there (instead of using setUpAll and checking isRegistered), e.g., call GetIt.instance.reset() (or GetIt.I.reset()) at the top of setUp, then register ResponsiveUtils, HiveGetInvitationStatusInteractor, GetInvitationStatusInteractor, PostAddressBookInteractor, HiveDeleteInvitationStatusInteractor, DeleteThirdPartyContactBoxInteractor and any other mocks via registerSingleton so each test starts with a fresh DI state and no leaked stubs from previous tests.
🤖 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/contacts_tab_body_view.dart`:
- Around line 303-311: When widget.externalContact.matrixId is null, the method
_fetchProfileIfNeeded currently leaves previous values intact causing stale UI;
update _fetchProfileIfNeeded to handle the null branch by clearing
_currentMatrixId and _profileFuture (set them to null) so any previous profile
future is discarded; reference the symbols _fetchProfileIfNeeded,
_currentMatrixId, _profileFuture and widget.externalContact.matrixId and ensure
the clearing happens in the else/null case so the view will stop rendering old
profile data.
In `@lib/pages/search/search_external_contact.dart`:
- Around line 49-57: The _fetchProfileIfNeeded method only compares
_currentKeyword to widget.keyword, so it fails to refetch when the active client
changes; update the invalidation logic to also track and compare the current
client (use widget.clientForTesting ?? Matrix.of(context).client) against a
stored field (e.g., _currentClient) and when either the keyword or client
identity differs, set _currentKeyword and _currentClient and call
client.getUserProfile(...) to assign _profileFuture (preserving maxCacheAge:
Duration.zero and existing behavior in _fetchProfileIfNeeded).
---
Nitpick comments:
In `@test/pages/search/search_external_contact_test.dart`:
- Around line 64-109: Reset the GetIt container at the start of each setUp and
register all required singletons unconditionally there (instead of using
setUpAll and checking isRegistered), e.g., call GetIt.instance.reset() (or
GetIt.I.reset()) at the top of setUp, then register ResponsiveUtils,
HiveGetInvitationStatusInteractor, GetInvitationStatusInteractor,
PostAddressBookInteractor, HiveDeleteInvitationStatusInteractor,
DeleteThirdPartyContactBoxInteractor and any other mocks via registerSingleton
so each test starts with a fresh DI state and no leaked stubs from previous
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9718904a-55eb-4adf-8578-00daac1436aa
📒 Files selected for processing (4)
integration_test/tests/chat/search_external_mxid_test.dartlib/pages/contacts_tab/contacts_tab_body_view.dartlib/pages/search/search_external_contact.darttest/pages/search/search_external_contact_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_test/tests/chat/search_external_mxid_test.dart
9clg6
force-pushed
the
feature/TW-2474-improve-searching-non-exist-matrix-address
branch
from
4732672 to
6e4e557
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/pages/search/search_external_contact.dart (1)
62-74: Consider handling the null_profileFuturestate more explicitly.When
_profileFutureis null (e.g., before the firstdidChangeDependenciescall or if no keyword is provided),FutureBuildertreats it as having no data, which will showEmptySearchWidget. This may cause a brief flash of "No Results" before the actual loading state appears.If this is intentional, it's fine. Otherwise, consider adding an explicit null check:
💡 Optional: Handle null future explicitly
Widget build(BuildContext context) { + if (_profileFuture == null) { + return const SizedBox.shrink(); + } return FutureBuilder<CachedProfileInformation>( future: _profileFuture,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/search/search_external_contact.dart` around lines 62 - 74, The FutureBuilder is fed a possibly null _profileFuture which causes a brief flash of EmptySearchWidget; update the widget to explicitly handle null futures by checking _profileFuture before returning FutureBuilder (or inside the builder check for snapshot.connectionState == ConnectionState.none or _profileFuture == null) and return an appropriate initial placeholder (e.g., SizedBox/empty container or a different idle widget) instead of EmptySearchWidget; look for _profileFuture, FutureBuilder<CachedProfileInformation>, EmptySearchWidget and didChangeDependencies to locate where to add the null check and return the idle placeholder until a real future is assigned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/pages/search/search_external_contact.dart`:
- Around line 62-74: The FutureBuilder is fed a possibly null _profileFuture
which causes a brief flash of EmptySearchWidget; update the widget to explicitly
handle null futures by checking _profileFuture before returning FutureBuilder
(or inside the builder check for snapshot.connectionState ==
ConnectionState.none or _profileFuture == null) and return an appropriate
initial placeholder (e.g., SizedBox/empty container or a different idle widget)
instead of EmptySearchWidget; look for _profileFuture,
FutureBuilder<CachedProfileInformation>, EmptySearchWidget and
didChangeDependencies to locate where to add the null check and return the idle
placeholder until a real future is assigned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45f3ae3c-68a3-4884-905a-a7837a263eb4
📒 Files selected for processing (4)
integration_test/tests/chat/search_external_mxid_test.dartlib/pages/contacts_tab/contacts_tab_body_view.dartlib/pages/search/search_external_contact.darttest/pages/search/search_external_contact_test.dart
hoangdat
reviewed
Mar 23, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/pages/search/search_external_contact.dart (1)
32-57:⚠️ Potential issue | 🟠 MajorInvalidate the cached lookup when the client changes.
didUpdateWidget()only reruns the fetch whenkeywordchanges, and_fetchProfileIfNeeded()keys_profileFutureonly bykeyword. Rebuilding this widget with the same MXID but a differentclientForTesting/active Matrix client will keep the previous lookup alive.Suggested fix
class _SearchExternalContactWidgetState extends State<SearchExternalContactWidget> { Future<CachedProfileInformation>? _profileFuture; String? _currentKeyword; + Client? _currentClient; @@ `@override` void didUpdateWidget(covariant SearchExternalContactWidget oldWidget) { super.didUpdateWidget(oldWidget); - if (oldWidget.keyword != widget.keyword) { - _fetchProfileIfNeeded(); - } + _fetchProfileIfNeeded(); } void _fetchProfileIfNeeded() { - if (_currentKeyword != widget.keyword) { - _currentKeyword = widget.keyword; - final client = widget.clientForTesting ?? Matrix.of(context).client; + final client = widget.clientForTesting ?? Matrix.of(context).client; + if (_currentKeyword != widget.keyword || + !identical(_currentClient, client)) { + _currentKeyword = widget.keyword; + _currentClient = client; _profileFuture = client.getUserProfile( widget.keyword, maxCacheAge: Duration.zero, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/search/search_external_contact.dart` around lines 32 - 57, The widget currently only invalidates cached lookups by keyword; update didUpdateWidget and _fetchProfileIfNeeded so the cache key includes the client instance: when didUpdateWidget detects a different client (compare oldWidget.clientForTesting ?? Matrix.of(context).client to widget.clientForTesting ?? Matrix.of(context).client) force a refetch by clearing/updating _currentKeyword or resetting _profileFuture, and in _fetchProfileIfNeeded ensure you consider both widget.keyword and the resolved client identity before returning the cached _profileFuture so a new client triggers client.getUserProfile(...) even if the MXID didn't change.
🤖 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/contacts_tab_body_view.dart`:
- Around line 285-316: The profile fetch logic only keys off matrixId, so add
tracking of the Matrix client to force refetch when the controller's client
changes: add a field like _currentClient and in _fetchProfileIfNeeded (and/or in
didUpdateWidget) compare widget.controller.client to _currentClient; if they
differ, set _currentClient = widget.controller.client, reset _profileFuture (and
_currentMatrixId if needed) and call
widget.controller.client.getUserProfile(...) so the cached profile always
corresponds to the current controller.client used elsewhere (e.g.
onContactTap()).
- Around line 342-347: The code assigns profile.displayname directly to
PresentationContact.displayName which allows empty or whitespace names to
override the fallback; update the logic in the snapshot handling so you treat
blank/whitespace as missing: use profile.displayname trimmed and check for
null/empty (e.g. profile.displayname?.trim().isEmpty) and only use
profile.displayname (preferably trimmed) when non-empty, otherwise fall back to
widget.externalContact.displayName so PresentationContact (and downstream
onContactTap()) never gets a blank name.
---
Duplicate comments:
In `@lib/pages/search/search_external_contact.dart`:
- Around line 32-57: The widget currently only invalidates cached lookups by
keyword; update didUpdateWidget and _fetchProfileIfNeeded so the cache key
includes the client instance: when didUpdateWidget detects a different client
(compare oldWidget.clientForTesting ?? Matrix.of(context).client to
widget.clientForTesting ?? Matrix.of(context).client) force a refetch by
clearing/updating _currentKeyword or resetting _profileFuture, and in
_fetchProfileIfNeeded ensure you consider both widget.keyword and the resolved
client identity before returning the cached _profileFuture so a new client
triggers client.getUserProfile(...) even if the MXID didn't change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9756aaa-6696-44a1-a920-4608b7fc0920
📒 Files selected for processing (4)
integration_test/tests/chat/search_external_mxid_test.dartlib/pages/contacts_tab/contacts_tab_body_view.dartlib/pages/search/search_external_contact.darttest/pages/search/search_external_contact_test.dart
✅ Files skipped from review due to trivial changes (2)
- test/pages/search/search_external_contact_test.dart
- integration_test/tests/chat/search_external_mxid_test.dart
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/pages/contacts_tab/contacts_tab_body_view.dart (2)
345-346:⚠️ Potential issue | 🟡 MinorIgnore blank/whitespace
profile.displaynamewhen resolving display name.A blank server display name currently overrides the fallback and can produce an empty label.
Suggested fix
- final validatedContact = PresentationContact( + final resolvedDisplayName = profile.displayname?.trim(); + final fallbackDisplayName = widget.externalContact.displayName?.trim(); + + final validatedContact = PresentationContact( matrixId: widget.externalContact.matrixId, - displayName: - profile.displayname ?? widget.externalContact.displayName, + displayName: + resolvedDisplayName?.isNotEmpty == true + ? resolvedDisplayName + : (fallbackDisplayName?.isNotEmpty == true + ? fallbackDisplayName + : widget.externalContact.matrixId), type: widget.externalContact.type, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/contacts_tab/contacts_tab_body_view.dart` around lines 345 - 346, The displayName assignment currently uses profile.displayname even when it's blank/whitespace; update the resolution logic at the displayName field (where profile.displayname and widget.externalContact.displayName are used) to treat blank/whitespace as absent — e.g., check profile.displayname?.trim().isNotEmpty and only use profile.displayname when that is true, otherwise fall back to widget.externalContact.displayName; adjust the expression in the same assignment so empty or whitespace server names do not override the fallback.
296-316:⚠️ Potential issue | 🟠 MajorProfile future invalidation should also track
widget.controller.client.Current logic only keys on
matrixId, so account/client swaps can reuse a future from a previous client.Suggested fix
class _SilverExternalContactState extends State<_SilverExternalContact> { Future<CachedProfileInformation>? _profileFuture; String? _currentMatrixId; + Client? _currentClient; @@ void didUpdateWidget(covariant _SilverExternalContact oldWidget) { super.didUpdateWidget(oldWidget); - if (oldWidget.externalContact.matrixId != widget.externalContact.matrixId) { - _fetchProfileIfNeeded(); - } + _fetchProfileIfNeeded(); } void _fetchProfileIfNeeded() { final matrixId = widget.externalContact.matrixId; + final client = widget.controller.client; if (matrixId == null) { _currentMatrixId = null; + _currentClient = null; _profileFuture = null; return; } - if (_currentMatrixId != matrixId) { + if (_currentMatrixId != matrixId || !identical(_currentClient, client)) { _currentMatrixId = matrixId; - _profileFuture = widget.controller.client.getUserProfile( + _currentClient = client; + _profileFuture = client.getUserProfile( matrixId, maxCacheAge: Duration.zero, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/contacts_tab/contacts_tab_body_view.dart` around lines 296 - 316, The profile future invalidation only compares matrixId, so when the account/client changes we may reuse a stale future; update the logic in didUpdateWidget and/or _fetchProfileIfNeeded to also detect changes to widget.controller.client (or its identity) and invalidate/reset _currentMatrixId/_profileFuture when the client reference changes; specifically, compare a stored client reference (e.g., _currentClient) against widget.controller.client and if different set _currentMatrixId = null and _profileFuture = null before calling widget.controller.client.getUserProfile to ensure _profileFuture always corresponds to the current client.lib/pages/search/search_external_contact.dart (1)
49-57:⚠️ Potential issue | 🟠 MajorRefetch key should include active Matrix client, not keyword only.
The future cache can go stale when client identity changes and
widget.keywordstays unchanged, because invalidation currently keys only on_currentKeyword.Suggested fix
class _SearchExternalContactWidgetState extends State<SearchExternalContactWidget> { Future<CachedProfileInformation>? _profileFuture; String? _currentKeyword; + String? _currentClientUserId; @@ void _fetchProfileIfNeeded() { - if (_currentKeyword != widget.keyword) { - _currentKeyword = widget.keyword; - final client = widget.clientForTesting ?? Matrix.of(context).client; + final client = widget.clientForTesting ?? Matrix.of(context).client; + final clientUserId = client.userID; + if (_currentKeyword != widget.keyword || + _currentClientUserId != clientUserId) { + _currentKeyword = widget.keyword; + _currentClientUserId = clientUserId; _profileFuture = client.getUserProfile( widget.keyword, maxCacheAge: Duration.zero, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/search/search_external_contact.dart` around lines 49 - 57, The refetch logic in _fetchProfileIfNeeded currently only compares _currentKeyword to widget.keyword, so changes in the active Matrix client (widget.clientForTesting or Matrix.of(context).client) won't trigger a refresh; add a stored client identity (e.g. a private field _currentClient) and include it in the invalidation check: if either _currentKeyword != widget.keyword OR _currentClient != client then update _currentKeyword and _currentClient and set _profileFuture by calling client.getUserProfile(...). Use the same client variable (widget.clientForTesting ?? Matrix.of(context).client) for comparison and assignment so client swaps correctly trigger refetches.
🤖 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/search/search_external_contact.dart`:
- Line 79: The displayName assignment currently uses the null-coalescing
operator which treats empty or whitespace-only profile.displayname as present;
update the expression used where displayName is set (the displayName:
profile.displayname ?? widget.keyword.substring(1) line) to treat
empty/whitespace as missing by checking trimmed emptiness first — e.g. compute a
safeName = (profile.displayname ?? '').trim(); then use displayName:
safeName.isEmpty ? widget.keyword.substring(1) : profile.displayname (or
safeName) so blank names fall back to the keyword.
---
Duplicate comments:
In `@lib/pages/contacts_tab/contacts_tab_body_view.dart`:
- Around line 345-346: The displayName assignment currently uses
profile.displayname even when it's blank/whitespace; update the resolution logic
at the displayName field (where profile.displayname and
widget.externalContact.displayName are used) to treat blank/whitespace as absent
— e.g., check profile.displayname?.trim().isNotEmpty and only use
profile.displayname when that is true, otherwise fall back to
widget.externalContact.displayName; adjust the expression in the same assignment
so empty or whitespace server names do not override the fallback.
- Around line 296-316: The profile future invalidation only compares matrixId,
so when the account/client changes we may reuse a stale future; update the logic
in didUpdateWidget and/or _fetchProfileIfNeeded to also detect changes to
widget.controller.client (or its identity) and invalidate/reset
_currentMatrixId/_profileFuture when the client reference changes; specifically,
compare a stored client reference (e.g., _currentClient) against
widget.controller.client and if different set _currentMatrixId = null and
_profileFuture = null before calling widget.controller.client.getUserProfile to
ensure _profileFuture always corresponds to the current client.
In `@lib/pages/search/search_external_contact.dart`:
- Around line 49-57: The refetch logic in _fetchProfileIfNeeded currently only
compares _currentKeyword to widget.keyword, so changes in the active Matrix
client (widget.clientForTesting or Matrix.of(context).client) won't trigger a
refresh; add a stored client identity (e.g. a private field _currentClient) and
include it in the invalidation check: if either _currentKeyword !=
widget.keyword OR _currentClient != client then update _currentKeyword and
_currentClient and set _profileFuture by calling client.getUserProfile(...). Use
the same client variable (widget.clientForTesting ?? Matrix.of(context).client)
for comparison and assignment so client swaps correctly trigger refetches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a168800-ce2a-4162-92ed-e07a22112b44
📒 Files selected for processing (4)
integration_test/tests/chat/search_external_mxid_test.dartlib/pages/contacts_tab/contacts_tab_body_view.dartlib/pages/search/search_external_contact.darttest/pages/search/search_external_contact_test.dart
✅ Files skipped from review due to trivial changes (1)
- test/pages/search/search_external_contact_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- integration_test/tests/chat/search_external_mxid_test.dart
Collaborator
Author
✅ |
tddang-linagora
approved these changes
Mar 27, 2026
hoangdat
deleted the
feature/TW-2474-improve-searching-non-exist-matrix-address
branch
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
3 participants
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
TID-2474 - Improve searching by the non-exist matrix address
Root cause
SearchExternalContactWidgetdisplayed any syntactically valid mxid as a contact result without verifying it exists on the server. The check was purely format-based (isValidMatrixId), so typing@noexist:server.comshowed a fake profile instead of "No Results".Solution
Converted
SearchExternalContactWidgetfromStatelessWidgettoStatefulWidgetwith aFutureBuilderthat callsclient.getProfileFromUserId()before rendering:EmptySearchWidget("No Results")Impact description
N/A (bug fix)
Test recommendations
@validuser:your-server.com→ should show profile info@nonexistent:your-server.com→ should show "No Results" mascotPre-merge
No
Resolved
test.mp4
Summary by CodeRabbit