Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds leave-chat localization keys and a NoPermissionForCreateChat failure type; updates CreateDirectChatInteractor to pre-check profiles, invite at creation, optionally enable encryption, wait for sync, and clean up on failure; implements a large chat profile/details UI refactor (new ChatProfileInfoAppBar, ChatProfileInfoAppBarView, ChatProfileInfoDetails, header stack, group info/actions, navigators, avatar widgets); wires a leave-chat confirmation flow and integration tests; and adjusts various style constants and AppBar action visibility. 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. ✨ 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 |
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2868 |
nqhhdev
requested review from
dab246,
hoangdat and
tddang-linagora
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pages/chat_profile_info/chat_profile_info_view.dart (1)
522-533:⚠️ Potential issue | 🔴 CriticalMissing return statement — avatar widget is discarded.
The
GetUserInfoSuccessbranch creates anAvatarwidget but doesn't return it. Execution falls through to the defaultreturn Avatar(...)below, ignoring the user info avatar URL.🐛 Proposed fix to return the constructed Avatar
if (success is GetUserInfoSuccess) { - Avatar( + return Avatar( mxContent: userInfoModel?.avatarUrl != null ? Uri.parse(userInfoModel?.avatarUrl ?? '') : avatarUri, name: userInfoModel?.displayName ?? displayName, sizeWidth: double.infinity, size: ChatProfileInfoStyle.avatarHeight, isCircle: false, fontSize: ChatProfileInfoStyle.avatarFontSize, ); }
🧹 Nitpick comments (3)
lib/pages/contacts_tab/contacts_tab.dart (1)
83-85: LGTM! Consistent abandoned DM handling.The logic mirrors the pattern in
GoToDraftChatMixin, ensuring consistent behavior across contact navigation flows.Consider extracting the abandoned DM check pattern into a shared utility method to reduce duplication:
// In a shared utility or extension static bool shouldUseDraftChat(Client client, String? roomId) { if (roomId == null) return true; final room = client.getRoomById(roomId); return room?.isAbandonedDMRoom == true; }This would centralize the logic and make future changes easier to maintain.
lib/widgets/avatar/avatar.dart (1)
13-13: LGTM with one observation about fallback consistency.The new
sizeWidthandisCircleparameters are well-implemented with sensible defaults that maintain backward compatibility. The conditional rendering logic is correct.Note: The
_fallbackAvatar()method (lines 71-84) usesRoundAvatarunconditionally, which may still render as circular even whenisCircleisfalse. If non-circular fallback avatars are needed, consider passingisCircleto the fallback logic:Widget _fallbackAvatar() { final fallbackLetters = name?.getShortcutNameForAvatar() ?? '@'; + if (!isCircle) { + // Handle non-circular fallback if needed + } return RoundAvatar( size: size, text: fallbackLetters,This may not be an issue if non-circular avatars are only used where images are guaranteed to be present.
Also applies to: 21-21, 47-56
lib/pages/chat_details/chat_details_view_style.dart (1)
18-20: Consider testing toolbar heights on smaller devices, particularly when max height is used.The
maxToolbarHeightSliverAppBarvalue of 668.0 pixels is quite large. While theNestedScrollViewstructure is designed to handle scrollable content, on smaller mobile devices (e.g., iPhone SE with ~667px height), using the maximum toolbar height would consume the entire viewport, requiring users to scroll before accessing body content. This scenario occurs when users have both emails and phones in their contact info and are in a room together. Although not a functional issue, testing on smaller devices would help validate the UX impact.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pages/chat_profile_info/chat_profile_info_view.dart (1)
271-322:⚠️ Potential issue | 🟠 MajorAdd width constraint and overflow handling for display name to prevent clipping.
The Positioned widget lacks a
rightconstraint, allowing long display names to overflow. More critically, the Text widgets in_buildDisplayNameWidgetare missingoverflow: TextOverflow.ellipsis, which prevents ellipsis from rendering even with a width constraint.Both issues need addressing:
- Add
right: 16to the Positioned widget to constrain width- Add
overflow: TextOverflow.ellipsisto all Text returns in_buildDisplayNameWidget(currently onlymaxLines: 1is set)
nqhhdev
changed the title
| // start. A separate inviteUser call after creation can cause some | ||
| // homeservers to treat the room as a group rather than a DM. | ||
| roomId = await client.createRoom( | ||
| invite: [contactMxId], |
There was a problem hiding this comment.
Replaced
// Now invite the user to the direct chat
wait client.inviteUser(roomId, contactMxId);
| if (roomId == null) { | ||
| final room = | ||
| roomId != null ? Matrix.of(context).client.getRoomById(roomId) : null; | ||
| if (roomId == null || room?.isAbandonedDMRoom == true) { |
There was a problem hiding this comment.
is it new change in new SDK? what is it?
|
|
nqhhdev
force-pushed
the
TW-2867-Add-ability-to-leave-a-DM
branch
from
8c653cd to
64da5ce
Compare
nqhhdev
changed the title
|
tddang-linagora
force-pushed
the
TW-2869/Group-details-new-ui
branch
from
9279ac3 to
69dc40e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@integration_test/tests/chat/chat_dm_leave_test.dart`:
- Around line 62-64: The current assertion uses $(TwakeListItem).exists which
only verifies any chat exists; change the check to assert the specific DM by
locating the list item by the DM's unique title/timestamp (the same selector
approach used later around line 113) instead of the generic TwakeListItem.
Modify the code that builds chatListRobot / chatExists to query for the exact
list item by text (timestampAccountTitle) or a selector that includes that title
and then assert its existence (replace the use of $(TwakeListItem).exists and
the chatExists variable with a targeted lookup for the created DM).
In `@lib/domain/usecase/create_direct_chat_interactor.dart`:
- Around line 36-47: The getUserProfile call can both throw non-MatrixExceptions
(which currently escape the outer catch) and after yielding
Left(NoPermissionForCreateChat()) the code falls through into room creation; fix
by moving the await client.getUserProfile(contactMxId) call inside the existing
outer try-catch (the one that handles (e, s)) and, in the MatrixException catch
where you yield Left(NoPermissionForCreateChat()), return immediately after
yielding to stop further execution; reference getUserProfile(contactMxId) and
the Left(NoPermissionForCreateChat()) yield to locate the changes.
- Around line 43-47: The MatrixException catch in
create_direct_chat_interactor.dart currently swallows all MatrixException cases;
update the catch in the block that handles getUserProfile (the "on
MatrixException catch (e)" handling MatrixError.M_FORBIDDEN and yielding
Left(NoPermissionForCreateChat())) so that it yields
Left(NoPermissionForCreateChat()) only when e.error == MatrixError.M_FORBIDDEN
and otherwise re-throws the MatrixException (or propagate it) so non-forbidden
errors are handled by the outer error handler.
🧹 Nitpick comments (5)
integration_test/robots/chat_list_robot.dart (1)
93-98: MissingpumpAndSettle()after tap — inconsistent withopenSearchScreen().The existing
openSearchScreen()(line 85) callsawait $.pumpAndSettle()between the tap andwaitUntilVisible. This method omits it. WhilewaitUntilVisiblewill pump frames internally, addingpumpAndSettle()keeps the pattern consistent and avoids potential flakiness if the search screen transition has animations.Suggested fix
Future<void> openSearchScreenWithoutAcceptPermission() async { // Tap on the search TextField in the chat list to open search screen await $(TextField).tap(); + await $.pumpAndSettle(); // Verify SearchView is opened await $.waitUntilVisible($(SearchView)); }integration_test/tests/chat/chat_dm_leave_test.dart (2)
17-27: Consider extracting the timestamp formatting for readability.Line 26 is a very long single-line expression that's hard to parse. Breaking it into intermediate variables would improve maintainability.
♻️ Suggested refactor
String generateTimestampAccount() { final now = DateTime.now(); const serverUrl = String.fromEnvironment('SERVER_URL'); if (serverUrl.isEmpty) { throw StateError( 'SERVER_URL environment variable is not set. ' 'Please provide it via --dart-define=SERVER_URL=your-server', ); } - return '@user${now.year}${now.month.toString().padLeft(2, '0')}${now.day.toString().padLeft(2, '0')}.${now.hour.toString().padLeft(2, '0')}${now.minute.toString().padLeft(2, '0')}${now.second.toString().padLeft(2, '0')}:$serverUrl'; + final date = '${now.year}' + '${now.month.toString().padLeft(2, '0')}' + '${now.day.toString().padLeft(2, '0')}'; + final time = '${now.hour.toString().padLeft(2, '0')}' + '${now.minute.toString().padLeft(2, '0')}' + '${now.second.toString().padLeft(2, '0')}'; + return '@user$date.$time:$serverUrl'; }
108-116: Potential flakiness:pumpAndSettleafter leave may not be sufficient.After confirming leave, the app navigates back to the chat list. If the room removal involves an async server call,
pumpAndSettlealone may not wait for the list to update. Consider adding awaitUntilVisiblefor the chat list screen or a brief delay/retry before asserting removal at line 113, to reduce flakiness.integration_test/robots/chat/chat_profile_info_robot.dart (2)
128-138: Redundant existence check afterwaitUntilVisible.
waitUntilVisible(line 130) will already throw if the widget never appears, making theexpecton line 131-134 redundant. Not harmful, but unnecessary noise.
153-184: Hardcoded English'Leave'text and fragile positional fallback create test brittleness.Two concerns:
Line 160: The test uses hardcoded
'Leave'in English, but the actual app usesL10n.of(context)!.leavefrom localization. If the app's locale changes during tests or the translation key is updated, this will silently fall through to the positional fallback without signaling a failure.Lines 177–179: The fallback assumes the confirm button is always the second button (index 1). This is fragile; if the dialog layout changes (e.g., button order or count), the test may tap the wrong button without error.
Both work in the current state, but the test could be more resilient by using the dialog's localization directly instead of hardcoding English text, and by avoiding positional assumptions. The external
ConfirmationDialogBuilderdoes not expose button-level keys, so improving this would require either changes to that dependency or a different approach (e.g., detecting the button by its style/role rather than position).
|
…ability to leave a DM
There was a problem hiding this comment.
Actionable comments posted: 1
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/create_direct_chat_interactor.dart (1)
57-70:⚠️ Potential issue | 🟡 MinorIf invite-join doesn't achieve
Membership.join, the code silently falls through to create a second room.When
room.join()succeeds but the post-sync membership check on line 66-67 fails (e.g., transient sync issue), execution falls through to create a brand-new room for the same contact, potentially leaving a ghost room behind. Consider either yielding a failure or retrying before falling through.
🤖 Fix all issues with AI agents
In `@lib/domain/usecase/create_direct_chat_interactor.dart`:
- Line 52: The code uses a missing Room property isAbandonedDMRoom which causes
compilation errors; add a Room extension or getter named isAbandonedDMRoom that
returns a bool (e.g., extension on Room { bool get isAbandonedDMRoom => /*
implement check */; }) and implement the abandonment check logic you need, then
import that extension where used (references: isAbandonedDMRoom, Room,
create_direct_chat_interactor.dart usages such as the if (room != null &&
!room.isAbandonedDMRoom) check). Ensure the extension is defined in a common
utilities file and exported so the five files using isAbandonedDMRoom compile
cleanly.
🧹 Nitpick comments (2)
integration_test/tests/chat/chat_dm_leave_test.dart (2)
17-35: EnsureSERVER_URLis a bare homeserver domain, not a full URL.The generated Matrix ID (
@user...:$serverUrl) requires a domain (e.g.,matrix.example.com), not a URL with a scheme. If someone passeshttps://matrix.example.com, the resulting ID will be malformed. Consider either documenting this expectation more explicitly in the error message, or stripping a potential scheme prefix defensively.🛡️ Defensive stripping suggestion
final date = '${now.year}' '${now.month.toString().padLeft(2, '0')}' '${now.day.toString().padLeft(2, '0')}'; final time = '${now.hour.toString().padLeft(2, '0')}' '${now.minute.toString().padLeft(2, '0')}' '${now.second.toString().padLeft(2, '0')}'; - return '@user$date.$time:$serverUrl'; + final homeserver = serverUrl.replaceFirst(RegExp(r'^https?://'), ''); + return '@user$date.$time:$homeserver';
89-96: Search result assertions could target the specific DM title.Both
waitUntilVisibleand the soft assertion use the generic$(TwakeListItem)finder. Since the search is filtered bytimestampAccount, this is unlikely to cause a false positive, but for consistency with the stronger assertion pattern used at lines 71-74, consider matching by title.Additionally, the soft assertion at line 93 is redundant — if
waitUntilVisible(line 89) succeeds,.existsis guaranteed to betrue; if it times out, it throws before reaching the assertion.
|
@coderabbitai Ensure SERVER_URL is a bare homeserver domain, not a full URL. Yes, our SERVER_URL=stg.lin-saas.com |
|
Tip For best results, initiate chat on the files or code changes.
The test will use this to construct the full homeserver URL when creating the DM chat with the timestamp account. Your setup is good to go! ✅ |
| future: Matrix.of(context).client | ||
| .getProfileFromUserId( | ||
| contact!.matrixId!, | ||
| getFromRooms: false, | ||
| ), |
There was a problem hiding this comment.
this was call multiple time in animation, we should reduce it?
| setState(() { | ||
| isExpandedAvatar.value = false; | ||
| }); |
There was a problem hiding this comment.
still use setState and value notifier? can we use 1
lib/pages/chat_profile_info/chat_profile_info_app_bar_view.dart
Outdated
Show resolved
Hide resolved
lib/pages/chat_profile_info/chat_profile_info_app_bar_view.dart
Outdated
Show resolved
Hide resolved
…7: Add ability to leave a DM
… TW-2867: Add ability to leave a DM
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/domain/usecase/create_direct_chat_interactor.dart (1)
36-45:⚠️ Potential issue | 🔴 CriticalTwo control-flow bugs remain in the
getUserProfileguard block.Bug 1 (critical) — missing
returninelsebranch (line 43).
Afteryield Left(CreateDirectChatFailed(exception: e))there is noreturn, so execution falls through into the outertryblock at line 47. The caller receives aCreateDirectChatFailedevent and then the interactor proceeds to check for, and potentially create, a new direct-chat room — emitting a secondCreateDirectChatLoading/CreateDirectChatSuccess/CreateDirectChatFailedevent on the same stream.Bug 2 (major) — non-
MatrixExceptionexceptions escape the stream.
ThegetUserProfilecall sits outside the outertry-catch (e, s)at line 47. ASocketException,TimeoutException, or any other non-MatrixExceptionthrown bygetUserProfileis not caught by eithercatchblock and propagates as an unhandled exception from theasync*generator, causing the stream to error rather than emitting aCreateDirectChatFailed.Both issues are fixed by moving
getUserProfileinside the outertryblock:🐛 Proposed fix
- try { - await client.getUserProfile(contactMxId); - } on MatrixException catch (e) { - if (e.error == MatrixError.M_FORBIDDEN) { - yield const Left(NoPermissionForCreateChat()); - return; - } else { - yield Left(CreateDirectChatFailed(exception: e)); - } - } String? roomId; try { + try { + await client.getUserProfile(contactMxId); + } on MatrixException catch (e) { + if (e.error == MatrixError.M_FORBIDDEN) { + yield const Left(NoPermissionForCreateChat()); + return; + } + rethrow; // non-forbidden errors bubble to the outer catch + } // Check if a direct chat already exists with this contact final directChatRoomId = client.getDirectChatFromUserId(contactMxId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/domain/usecase/create_direct_chat_interactor.dart` around lines 36 - 45, The guard around client.getUserProfile(contactMxId) has two control-flow bugs: non-MatrixException errors escape the stream because the call is outside the outer try/catch, and the MatrixException else branch yields CreateDirectChatFailed but does not return so execution falls through; fix by moving the client.getUserProfile(contactMxId) call inside the outer try block (the same try that wraps the create-direct-chat flow) so all exceptions are caught by the outer catch, and ensure after yielding Left(CreateDirectChatFailed(exception: e)) in the MatrixException else branch you immediately return to stop further processing in the createDirectChat interactor method.lib/presentation/mixins/leave_chat_mixin.dart (1)
12-14:⚠️ Potential issue | 🔴 CriticalFix inverted mounted guard to avoid skipping the leave flow.
Line 12-14 returns early when the context is mounted, so
leaveChatnever runs under normal conditions. This should be negated.🛠️ Proposed fix
- if (context.mounted) { - return; - } + if (!context.mounted) { + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/presentation/mixins/leave_chat_mixin.dart` around lines 12 - 14, The mounted guard in leave_chat_mixin.dart is inverted so leaveChat is skipped when the widget is mounted; update the condition in the mixin's leave flow (look for the guard inside the leaveChat or LeaveChatMixin method) to return early only when the context is NOT mounted (i.e., negate the current check) so the leaveChat logic executes under normal mounted conditions.
🤖 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/chat_profile_info/chat_profile_info_app_bar.dart`:
- Around line 122-137: In _handleGroupInfoTap, the delayed .then callbacks
mutate isExpandedAvatar and call animationController.forward()/reverse() without
checking widget lifecycle; add a guard at the start of each delayed callback
closure (e.g. if (!mounted) return;) before touching isExpandedAvatar or
animationController to avoid operating on disposed resources, ensuring callbacks
safely bail out when the widget is no longer mounted.
- Around line 98-112: The async method _loadProfileIfNeeded attempts to set
_profileNotifier.value after awaiting
Matrix.of(context).client.getProfileFromUserId(...), which can happen after the
widget is disposed; to fix, add mounted checks immediately after each await (and
before any use of context or updating _profileNotifier) in _loadProfileIfNeeded:
after the getProfileFromUserId await, verify mounted and only then assign
_profileNotifier.value = profile; in the catch block also check mounted before
setting _profileNotifier.value = null; avoid using context after disposal by
moving Matrix.of(context) usage or re-checking mounted before calling it.
---
Duplicate comments:
In `@lib/domain/usecase/create_direct_chat_interactor.dart`:
- Around line 36-45: The guard around client.getUserProfile(contactMxId) has two
control-flow bugs: non-MatrixException errors escape the stream because the call
is outside the outer try/catch, and the MatrixException else branch yields
CreateDirectChatFailed but does not return so execution falls through; fix by
moving the client.getUserProfile(contactMxId) call inside the outer try block
(the same try that wraps the create-direct-chat flow) so all exceptions are
caught by the outer catch, and ensure after yielding
Left(CreateDirectChatFailed(exception: e)) in the MatrixException else branch
you immediately return to stop further processing in the createDirectChat
interactor method.
In `@lib/presentation/mixins/leave_chat_mixin.dart`:
- Around line 12-14: The mounted guard in leave_chat_mixin.dart is inverted so
leaveChat is skipped when the widget is mounted; update the condition in the
mixin's leave flow (look for the guard inside the leaveChat or LeaveChatMixin
method) to return early only when the context is NOT mounted (i.e., negate the
current check) so the leaveChat logic executes under normal mounted conditions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/domain/usecase/create_direct_chat_interactor.dartlib/pages/chat_details/chat_details_view_style.dartlib/pages/chat_profile_info/chat_profile_info_app_bar.dartlib/pages/chat_profile_info/chat_profile_info_app_bar_view.dartlib/pages/chat_profile_info/chat_profile_info_details.dartlib/presentation/mixins/leave_chat_mixin.dart
… fixup! TW-2867: Add ability to leave a DM
… fixup! fixup! TW-2867: Add ability to leave a DM
2 participants
Ticket
Related issue
Resolved
Attach screenshots or videos demonstrating the changes
Simulator.Screen.Recording.-.iPhone.17.-.2026-02-05.at.14.55.31.mov
Simulator.Screen.Recording.-.iPhone.17.-.2026-02-05.at.15.00.20.mov
Screen.Recording.2026-02-05.at.16.22.26.mov
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Localization