TW-2837: Add /match endpoint to combine data in ContactManager#2838
TW-2837: Add /match endpoint to combine data in ContactManager#2838
/match endpoint to combine data in ContactManager#2838Conversation
…teractor to unify contact retrieval logic and update related tests and dependencies.
nqhhdev
requested review from
dab246,
hoangdat and
tddang-linagora
as code owners
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a new GetCombinedContactsInteractor that concurrently consumes GetTomContactsInteractor and LookupMatchContactInteractor, aggregates their results, and deduplicates contacts by ID with Tom contacts taking precedence. Updates LookupMatchContactInteractor to accept a nullable query and return multiple contacts. Replaces GetTomContactsInteractor usages with GetCombinedContactsInteractor across ContactsManager, DI, and tests. Adjusts AddressBookExtension.toContact() id resolution to prefer mxid, and adds unit tests for GetCombinedContactsInteractor covering deduplication and failure/empty-source scenarios. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/domain/usecase/contacts/lookup_match_contact_interactor.dart (1)
16-36: Normalize nullablevalbefore building the lookup request.
ContactQueryusesval ?? '', butLookupMxidRequest.valreceives the nullablevaldirectly. If the backend expects a non-null keyword, this can yield inconsistent filtering or 400s whenvalis null. Consider normalizing both to the same value or short‑circuiting on empty input.
🤖 Fix all issues with AI agents
In `@lib/domain/contact_manager/contacts_manager.dart`:
- Around line 202-208: The searchContacts method ignores the incoming query —
update it so the query is used: either call
getCombinedContactsInteractor.execute with the query (e.g.,
getCombinedContactsInteractor.execute(query: query)) and update the interactor
signature (GetCombinedContactsInteractor.execute) to accept an optional String
query and forward it to downstream interactors, or if you prefer not to change
the interactor, filter the stream payload inside the listener before assigning
_contactsNotifier.value (in searchContacts) to only include contacts matching
the query; ensure tomContactsSubscription cancellation and reassignment behavior
in searchContacts remains correct.
In `@lib/domain/usecase/contacts/get_combined_contacts_interactor.dart`:
- Line 22: Fix the typo in the inline comment that reads "// Execute both
interactors concurrentlyz" by changing "concurrentlyz" to "concurrently"
wherever that comment appears (e.g., in get_combined_contacts_interactor.dart
near the code that executes both interactors concurrently).
- Around line 51-66: The failure branch in lookupResult.fold contains a dead
check against LookupContactsEmpty (a Success subtype); remove the "if (failure
is! LookupContactsEmpty)" guard and simplify the failure handler to just assign
failureState = failure when appropriate (e.g., if failureState != null then keep
existing logic), so the fold only treats actual failures here; verify the
success branch still handles LookupMatchContactSuccess and other Success
variants (e.g., LookupContactsEmpty) as intended.
In `@test/domain/contacts/contacts_manager_test.dart`:
- Around line 5512-5529: The test currently only verifies execute() was called
twice but not that the first subscription was cancelled; modify the test for
searchContacts on contactsManager to use controllable streams (e.g.,
StreamController) for mockGetCombinedContactsInteractor.execute(val: query1) and
execute(val: query2), have the first controller emit a delayed value after the
second search is invoked, and assert the contacts notifier (or update callback)
only reflects values from the second stream and never receives the late value
from the first stream; ensure you close controllers and verify execute() calls
still occur as before while asserting the notifier/update was not updated by the
first stream’s late emission.
In `@test/domain/usecase/contacts/get_combined_contacts_interactor_test.dart`:
- Line 74: Rename the test description string in the test named "should deduct
duplicates preferring Tom contacts" to correct the typo—replace "deduct" with
"deduplicate" (or "remove") so it reads e.g. "should deduplicate duplicates
preferring Tom contacts" (or "should remove duplicates preferring Tom
contacts"); update the test declaration that contains this exact text to the
corrected phrasing.
lib/domain/usecase/contacts/get_combined_contacts_interactor.dart
Outdated
Show resolved
Hide resolved
test/domain/usecase/contacts/get_combined_contacts_interactor_test.dart
Outdated
Show resolved
Hide resolved
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2838 |
…tactsInteractor to unify contact retrieval logic and update related tests and dependencies.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/domain/usecase/contacts/get_combined_contacts_interactor.dart`:
- Around line 67-80: The current decision logic in
get_combined_contacts_interactor.dart treats any single source failure as
"critical" when both contact lists are empty; update the branch where
!hasAnyContacts to only surface a failure when both sources actually failed
(tomFailure != null && lookupFailure != null). Specifically, change the
condition that yields tomFailure ?? lookupFailure to require both tomFailure and
lookupFailure, and otherwise yield const Left(GetContactsIsEmpty()); reference
tomContacts, lookupContacts, tomFailure, lookupFailure, and GetContactsIsEmpty
when making the change.
…inedContactsInteractor to unify contact retrieval logic and update related tests and dependencies.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/domain/usecase/contacts/get_combined_contacts_interactor_test.dart`:
- Around line 55-71: The test closure for the case that asserts combined
contacts should be made async and the expectLater call awaited to ensure stream
assertions complete; update the test function containing calls to
mockGetTomContactsInteractor.execute(),
mockLookupMatchContactInteractor.execute(), and the interactor.execute()
assertion by marking the test as async and changing expectLater(...) to await
expectLater(...), and apply the same pattern to other tests in this file that
use expectLater so the stream matchers run to completion.
🧹 Nitpick comments (1)
lib/domain/usecase/contacts/get_combined_contacts_interactor.dart (1)
23-31: Prevent hangs/unhandled errors when awaiting.lastresults.Using
.last(Line 24–25) assumes each interactor stream emits at least one element and then closes. If a stream stays open (e.g., continuous updates) or completes without events, this will hang or throw, andFuture.waitwill terminate the generator without emitting aLeft. Please confirm these streams always close after a terminal result, or add a guarded path (take(1)/firstWhere+ timeout) with a try/catch to map errors into a Failure state.
test/domain/usecase/contacts/get_combined_contacts_interactor_test.dart
Outdated
Show resolved
Hide resolved
…ly to message with media or file
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/domain/usecase/contacts/lookup_match_contact_interactor.dart`:
- Around line 44-46: The log call in LookupMatchContactInteractor uses
jsonEncode(contactMatched) but Contact lacks JSON serialization; remove
jsonEncode and log contactMatched directly (e.g.,
Logs().i('LookupMatchContactInteractor:: contactMatched $contactMatched')), or
add JSON support on the Contact model by implementing a toJson() method (or
annotating Contact with `@JsonSerializable` and adding the generated part) so
jsonEncode can serialize it; update imports and run code-gen if using
json_serializable.
… to reply to message with media or file
…ability to reply to message with media or file
…7: Add ability to reply to message with media or file
|
Postponse this fix, because the change from BE. Keep it here until release well. |
3 participants
Ticket
Related issue
Resolved
Attach screenshots or videos demonstrating the changes
1.mov
2.mov
Simulator.Screen.Recording.-.iPhone.17.-.2026-01-20.at.17.46.22.mov
Screen.Recording.2026-01-21.at.10.05.26.mov
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.