[#2758-Part-1] TW-2759 Invite when app installed #2761
Conversation
tddang-linagora
requested review from
dab246,
hoangdat and
nqhhdev
as code owners
tddang-linagora
temporarily deployed
to
PR-2761
GitHub Actions
Inactive
WalkthroughAdds an "Invite friend" UI and English i18n keys, updates URL launching to optionally open/create direct chats via Matrix IDs, defers sharing-intent initialization until after first sync, cleans up intent subscriptions, and removes two controller methods. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Contacts UI
participant Widget as SliverInviteFriendButton
participant Platform as Platform API
participant Clipboard as Clipboard/Share
UI->>Widget: Tap Invite
Widget->>Platform: detect platform
alt Mobile
Widget->>Clipboard: Share.share(url)
else Web
Widget->>Clipboard: writeToClipboard(url)
Clipboard-->>Widget: success
Widget->>UI: show snackbar "linkCopiedToClipboard"
end
sequenceDiagram
participant External as Incoming Universal Link
participant UrlLauncher as UrlLauncher.launchUrl(client)
participant Client as Matrix Client
participant Router as Router/Rooms
External->>UrlLauncher: open link (matrix ID path)
UrlLauncher->>Client: find or create direct chat for matrixId
alt room exists
Client-->>UrlLauncher: roomId
UrlLauncher->>Router: navigate to /rooms/{roomId}
else no room
Client-->>UrlLauncher: newRoomId
UrlLauncher->>Router: navigate to /rooms/{newRoomId}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
tddang-linagora
temporarily deployed
to
PR-2761
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
lib/utils/url_launcher.dart (1)
48-54: Minor: RedundantreplaceFirstcalls.The path manipulation calls
replaceFirst('/', '')twice. Consider extracting to a local variable for clarity:if (uri.host == AppConstants.appLinkUniversalLinkDomain) { final pathWithoutChatPrefix = uri.path.replaceFirst('/chat', ''); - if (pathWithoutChatPrefix.startsWith('/') && - pathWithoutChatPrefix.replaceFirst('/', '').isValidMatrixId) { - return _openChatWithUser( - pathWithoutChatPrefix.replaceFirst('/', ''), - client: client, - ); - } + final matrixId = pathWithoutChatPrefix.replaceFirst('/', ''); + if (pathWithoutChatPrefix.startsWith('/') && matrixId.isValidMatrixId) { + return _openChatWithUser(matrixId, client: client); + } context.go(pathWithoutChatPrefix);lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart (1)
50-58: Desktop platforms not handled - tap does nothing silently.The
onTaphandler only covers mobile (Share.share) and web (Clipboard). On desktop (Linux, macOS, Windows), tapping the button has no effect. Consider adding clipboard support for desktop:try { if (PlatformInfos.isMobile) { await Share.share(url); - } else if (PlatformInfos.isWeb) { + } else { + // Web and Desktop await Clipboard.setData(ClipboardData(text: url)); TwakeSnackBar.show( context, L10n.of(context)!.linkCopiedToClipboard, ); }lib/widgets/matrix.dart (1)
379-388: Consider adding error handling for sync stream.If
onSync.stream.firstnever completes (e.g., network issues during initial sync), sharing intent won't be initialized. Consider adding a timeout or error handler:void _initializeSharingIntentAfterFirstSync(Client client) { if (_hasInitializedSharingIntent) return; - client.onSync.stream.first.then((_) { + client.onSync.stream.first.timeout( + const Duration(minutes: 2), + onTimeout: () => SyncUpdate(nextBatch: ''), + ).then((_) { Logs().d( 'MatrixState::_initializeSharingIntentAfterFirstSync: First sync completed, initializing sharing intent', ); _initializeSharingIntentOnce(); + }).catchError((e) { + Logs().e('MatrixState::_initializeSharingIntentAfterFirstSync: Error waiting for first sync: $e'); + _initializeSharingIntentOnce(); // Initialize anyway to not block sharing }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
assets/l10n/intl_en.arb(1 hunks)lib/pages/chat_draft/draft_chat.dart(0 hunks)lib/pages/chat_list/chat_list.dart(0 hunks)lib/pages/chat_list/receive_sharing_intent_mixin.dart(3 hunks)lib/pages/contacts_tab/contacts_appbar.dart(2 hunks)lib/pages/contacts_tab/contacts_tab_body_view.dart(2 hunks)lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart(1 hunks)lib/utils/url_launcher.dart(3 hunks)lib/widgets/matrix.dart(4 hunks)
💤 Files with no reviewable changes (2)
- lib/pages/chat_draft/draft_chat.dart
- lib/pages/chat_list/chat_list.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: Analyze code
- GitHub Check: Widget test
🔇 Additional comments (7)
lib/pages/contacts_tab/contacts_appbar.dart (1)
29-70: LGTM! Layout simplification.Adding
mainAxisSize: MainAxisSize.minprevents unnecessary vertical expansion, and removing the redundantRow/Expandedwrapper aroundSearchTextFieldsimplifies the widget tree without changing visual behavior.assets/l10n/intl_en.arb (1)
3488-3491: LGTM! Localization entries added correctly.The new
inviteFriendandlinkCopiedToClipboardkeys follow the existing ARB format and support the new invite friend feature.lib/pages/chat_list/receive_sharing_intent_mixin.dart (2)
99-101: LGTM! Client parameter correctly passed.Passing
matrixState.clientenables the new Matrix ID navigation flow inUrlLauncher.
113-126: Good defensive programming - preventing duplicate subscriptions.Cancelling existing subscriptions before re-subscribing prevents memory leaks and duplicate event handling if
initReceiveSharingIntent()is called multiple times.lib/pages/contacts_tab/contacts_tab_body_view.dart (1)
35-36: LGTM! Conditional rendering with proper null check.The
SliverInviteFriendButtonis correctly guarded by theuserID != nullcheck, ensuring it only appears for authenticated users. The force unwrap is safe given the preceding condition.lib/widgets/matrix.dart (2)
372-388: LGTM! Deferred sharing intent initialization improves reliability.Initializing sharing intent after the first sync ensures the client has room data before processing deep links. The guard flag prevents duplicate initialization.
661-665: LGTM! Immediate initialization for already logged-in clients.For returning users with persisted sessions, initializing sharing intent immediately in
initMatrixensures deep links work without waiting for a redundant sync trigger.
lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart
Outdated
Show resolved
Hide resolved
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/utils/url_launcher.dart (1)
33-39: Fix return type mismatch: cannot returnvoidfromFuture<void>At line 38,
return openMatrixToUrl();attempts to return the result of avoidasync function. SinceopenMatrixToUrl()is declared asvoid openMatrixToUrl() async, the expression has typevoid, which cannot be returned from a function declared to returnFuture<void>. This is a type error that the analyzer will flag.Call
openMatrixToUrl()as a statement and then return explicitly:return openMatrixToUrl(); + openMatrixToUrl(); + return;Alternatively, if you want
launchUrl'sFutureto complete only after matrix navigation logic finishes, changeopenMatrixToUrlto returnFuture<void>and await it (this only affects the single call site at line 38).
♻️ Duplicate comments (1)
lib/utils/url_launcher.dart (1)
252-273: Guard_openChatWithUseragainst nullclient/roomIdand always resetisCreatingChatFromUrlAs written:
- If
clientis null,roomsis null andstartDirectChatis never called, soroomIdwill be null and you still navigate to/rooms/null.- If
client.startDirectChatthrows,isCreatingChatFromUrlis never reset tofalse, so this instance will silently ignore all future URL‑driven chat opens (if (isCreatingChatFromUrl) return;).- Even if
startDirectChatreturns a nullable ID, you unconditionally callcontext.go('/rooms/$roomId').A safer structure:
- Early‑return (with optional user feedback/log) when
clientis null.- Wrap the body in
try/finallysoisCreatingChatFromUrlis always reset.- Only navigate when you have a non‑null
roomId.For example:
Future<void> _openChatWithUser( String matrixId, { Client? client, }) async { - if (isCreatingChatFromUrl) return; - isCreatingChatFromUrl = true; - final rooms = client?.rooms.where((room) => room.isDirectChat).toList(); + if (isCreatingChatFromUrl) return; + isCreatingChatFromUrl = true; + + if (client == null) { + // Optionally show a snackbar or log here to indicate failure. + isCreatingChatFromUrl = false; + return; + } + + final rooms = client.rooms.where((room) => room.isDirectChat).toList(); @@ - if (availableRoom != null) { - isCreatingChatFromUrl = false; - context.go('/rooms/${availableRoom.id}'); - return; - } - - final roomId = await client?.startDirectChat(matrixId); - isCreatingChatFromUrl = false; - context.go('/rooms/$roomId'); + if (availableRoom != null) { + isCreatingChatFromUrl = false; + context.go('/rooms/${availableRoom.id}'); + return; + } + + try { + final roomId = await client.startDirectChat(matrixId); + if (roomId == null) { + // Optionally show error/snackbar here. + return; + } + context.go('/rooms/$roomId'); + } finally { + isCreatingChatFromUrl = false; + } }This addresses the previously‑raised
/rooms/nullnavigation risk and makes the re‑entrancy guard robust even when the SDK throws.
🧹 Nitpick comments (1)
lib/utils/url_launcher.dart (1)
48-54: Minor readability tweak when extracting Matrix ID from universal‑link pathThe logic is correct, but
pathWithoutChatPrefix.replaceFirst('/', '')is computed three times, which hurts readability slightly and risks subtle inconsistencies if it’s later changed.You can factor out the matrix ID once and reuse it:
final pathWithoutChatPrefix = uri.path.replaceFirst('/chat', ''); - if (pathWithoutChatPrefix.startsWith('/') && - pathWithoutChatPrefix.replaceFirst('/', '').isValidMatrixId) { - return _openChatWithUser( - pathWithoutChatPrefix.replaceFirst('/', ''), - client: client, - ); - } + if (pathWithoutChatPrefix.startsWith('/')) { + final matrixId = pathWithoutChatPrefix.substring(1); + if (matrixId.isValidMatrixId) { + return _openChatWithUser( + matrixId, + client: client, + ); + } + }Behavior stays the same but the intent (“take the path after
/chat/and treat it as a Matrix ID”) is clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/utils/url_launcher.dart(3 hunks)
⏰ 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). (2)
- GitHub Check: Widget test
- GitHub Check: Analyze code
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2761 |
tddang-linagora
temporarily deployed
to
PR-2761
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart (2)
45-45: Address the TODO comment for the URL format.The TODO indicates the URL structure may still be a placeholder.
Do you want me to help finalize the URL format or open a new issue to track this task?
58-60: Consider showing user feedback when sharing/copying fails.The error is logged but the user receives no indication that the operation failed. This may lead to confusion if they expect the link to be shared or copied.
Consider showing a snackbar with an error message:
} catch (e) { Logs().e('InviteFriendButtonDelegate::onTap():', e); + if (context.mounted) { + TwakeSnackBar.show( + context, + L10n.of(context)!.anErrorOccurred, // or a more specific message + ); + } }lib/utils/url_launcher.dart (2)
33-55: Clarify behavior for matrix‑ID universal links whenclientis nullIn the universal-link branch, if
pathWithoutChatPrefixis a Matrix ID you now always go through_openChatWithUser. But_openChatWithUserimmediately returns whenclient == null, so in that caselaunchUrlcompletes without any navigation. Previously this path would have fallen through tocontext.go(pathWithoutChatPrefix).If
launchUrlcan be called before the Matrix client is ready (e.g. app opened from an invite while logged out or before first sync), this can make invite links appear to “do nothing”.Consider keeping the previous routing behavior as a fallback when no client is available:
- if (pathWithoutChatPrefix.startsWith('/') && - pathWithoutChatPrefix.replaceFirst('/', '').isValidMatrixId) { - return _openChatWithUser( - pathWithoutChatPrefix.replaceFirst('/', ''), - client: client, - ); - } - context.go(pathWithoutChatPrefix); - return; + if (pathWithoutChatPrefix.startsWith('/') && + pathWithoutChatPrefix.replaceFirst('/', '').isValidMatrixId) { + if (client != null) { + return _openChatWithUser( + pathWithoutChatPrefix.replaceFirst('/', ''), + client: client, + ); + } + // No client yet (e.g. logged out) – fall back to plain navigation. + } + context.go(pathWithoutChatPrefix); + return;This keeps the new “open or create DM” behavior for logged‑in cases while preserving a sane default when the client isn’t available.
252-274: MakeisCreatingChatFromUrlrobust withtry/finally
isCreatingChatFromUrlis set back tofalseonly on the happy paths. Ifclient.startDirectChat(or the room iteration) throws, the flag staystrueand future calls on the sameUrlLauncherinstance will be ignored.Wrapping the body in
try/finallyavoids this sticky state and simplifies the resets:Future<void> _openChatWithUser( String matrixId, { Client? client, }) async { - if (client == null || isCreatingChatFromUrl) return; - isCreatingChatFromUrl = true; - final rooms = client.rooms.where((room) => room.isDirectChat).toList(); - final availableRoom = rooms.firstWhereOrNull((room) { - return room.getParticipants().any((user) => user.id == matrixId); - }); - - if (availableRoom != null) { - isCreatingChatFromUrl = false; - context.go('/rooms/${availableRoom.id}'); - return; - } - - final roomId = await client.startDirectChat(matrixId); - isCreatingChatFromUrl = false; - context.go('/rooms/$roomId'); + if (client == null || isCreatingChatFromUrl) return; + isCreatingChatFromUrl = true; + try { + final rooms = client.rooms.where((room) => room.isDirectChat).toList(); + final availableRoom = rooms.firstWhereOrNull((room) { + return room.getParticipants().any((user) => user.id == matrixId); + }); + + if (availableRoom != null) { + context.go('/rooms/${availableRoom.id}'); + return; + } + + final roomId = await client.startDirectChat(matrixId); + context.go('/rooms/$roomId'); + } finally { + isCreatingChatFromUrl = false; + } }This keeps the behavior the same on success while ensuring the reentrancy guard doesn’t get stuck after an exception.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart(1 hunks)lib/utils/url_launcher.dart(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-09T05:40:46.528Z
Learnt from: nqhhdev
Repo: linagora/twake-on-matrix PR: 2751
File: lib/pages/chat_draft/draft_chat.dart:608-611
Timestamp: 2025-12-09T05:40:46.528Z
Learning: In draft_chat.dart, when sending media with caption through sendImagesWithCaption, the dialog returns SendMediaWithCaptionStatus.emptyRoom (not done) when the user confirms the send, because no room exists yet. The emptyRoom status triggers room creation and media upload, making it effectively the "success" case. Therefore, text should only be restored when status != emptyRoom.
Applied to files:
lib/utils/url_launcher.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 (1)
lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart (1)
109-110: Good fix for shouldRebuild!The
shouldRebuildmethod now correctly comparesuserIdto determine when the delegate needs to rebuild, addressing the previous review concern. This ensures the invite link updates when the user switches accounts.
lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart
Outdated
Show resolved
Hide resolved
| pathWithoutChatPrefix.replaceFirst('/', ''), | ||
| client: client, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Is it safer to add a subpath in the universal link that manages invite ?
Like links.twake.app/chat/user/@aaa:linagora.com instead of links.twake.app/chat/@aaa:linagora.com to keep other routing possibilities for the future? Maybe one day links.twake.app/chat/group/ or links.twake.app/chat/settings/ that opens settings or I don't know
There was a problem hiding this comment.
okie, so please keep as the matrix scheme, I propose links.twake.app/chat/user/#/@aaa:linagora.com
There was a problem hiding this comment.
There was a problem hiding this comment.
https://matrix.to/#/@tddang:linagora.com
Twake chat is not listed in the supported apps.
There was a problem hiding this comment.
I did not know that Matrix scheme already manage rooms, users, messages, etc so maybe my comment is less relevant
Ticket
Resolved
Android
Invite link
android-invite-link.webm
Logged out
android-logged-out.webm
Logged in
android-logged-in.webm
iOS
Invite link
ios-invite-link.mp4
Logged out
ios-logged-out.mp4
Logged in
ios-logged-in.mp4
Summary by CodeRabbit
New Features
Improvements
Localization
✏️ Tip: You can customize this high-level summary in your review settings.