[DO NOT MERGE] TW-2834 Fix missing messages when open chat room by notification#2835
Open
tddang-linagora wants to merge 1 commit intomainfrom
Open
[DO NOT MERGE] TW-2834 Fix missing messages when open chat room by notification#2835tddang-linagora wants to merge 1 commit intomainfrom
tddang-linagora wants to merge 1 commit intomainfrom
Conversation
tddang-linagora
requested review from
dab246,
hoangdat and
nqhhdev
as code owners
tddang-linagora
temporarily deployed
to
PR-2835
GitHub Actions
Inactive
WalkthroughMoves room initialization out of ChatView.build into ChatController.initState via a new _initRoom(), adds _resetState() to cancel subscriptions and clear room-specific state, and updates didUpdateWidget to detect widget.roomId changes and reinitialize state. _tryLoadTimeline now returns Future and includes guards to avoid operating on stale or unmounted state; timeline and history loading routines include mounted and roomId-change checks. ChatView.build no longer performs Matrix/context-based initialization. PinnedEventsController gains reset() and cancels its subscription on dispose. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/adr/0034-fix-notification-redirection-race-condition.md`:
- Around line 33-37: The ADR's description of didUpdateWidget is incomplete:
besides calling _initRoom() and _tryLoadTimeline(), the implementation must also
cancel room-specific subscriptions and reinitialize all room-dependent state;
update section 2 to state that didUpdateWidget should unsubscribe existing room
event listeners (cancel timers/streams), clear and re-fetch pinned events, reset
cached presence and participants, re-subscribe room listeners, and ensure any
room-scoped resources are re-initialized so pinned events, presence,
participants, and room event listeners are fully refreshed on room switch.
In `@lib/pages/chat/chat.dart`:
- Around line 3225-3228: When roomId changes, re-run all room-specific
initialization and clean up existing subscriptions: call
_initializePinnedEvents(), initCachedPresence(), _requestParticipants(), and
listenIgnoredUser() after _initRoom(); before re-subscribing cancel and null-out
existing cachedPresenceStreamSubscription and onUpdateEventStreamSubcription to
avoid duplicate listeners and memory leaks (mirror the same teardown logic used
in dispose or initState). Ensure subscriptions are recreated only after
cancellation so presence, participants, pinned events and ignored-user state
reflect the new room.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/adr/0034-fix-notification-redirection-race-condition.mdlib/pages/chat/chat.dartlib/pages/chat/chat_view.dart
💤 Files with no reviewable changes (1)
- lib/pages/chat/chat_view.dart
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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:51.607Z
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.
📚 Learning: 2025-12-09T05:40:51.607Z
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:51.607Z
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:
docs/adr/0034-fix-notification-redirection-race-condition.mdlib/pages/chat/chat.dart
📚 Learning: 2025-12-12T07:13:07.752Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2770
File: lib/pages/chat/chat.dart:623-670
Timestamp: 2025-12-12T07:13:07.752Z
Learning: In `lib/pages/chat/chat.dart`, when `_processReadMarker()` encounters a null or empty timeline, the design choice is to drop the pending read marker and wait for the next visibility event to trigger a fresh `setReadMarker()` call, rather than retrying. This is intentional behavior.
Applied to files:
lib/pages/chat/chat.dart
📚 Learning: 2025-12-09T06:19:59.615Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2761
File: lib/pages/contacts_tab/widgets/sliver_invite_friend_button.dart:49-57
Timestamp: 2025-12-09T06:19:59.615Z
Learning: For the Twake on Matrix Flutter app (fluffychat), desktop platforms (macOS, Linux, Windows) are not supported. During reviews, assume only web and mobile targets and avoid introducing desktop-specific builds, dependencies, or platform checks. This guideline applies across Dart source files in the repository.
Applied to files:
lib/pages/chat/chat.dart
📚 Learning: 2025-12-22T06:44:09.742Z
Learnt from: tddang-linagora
Repo: linagora/twake-on-matrix PR: 2644
File: lib/pages/chat/events/audio_message/audio_player_widget.dart:134-134
Timestamp: 2025-12-22T06:44:09.742Z
Learning: When using MatrixFile from matrix-dart-sdk (v4.1.0+), MatrixFile.bytes is non-nullable (Uint8List). Ensure code handles non-null bytes and rely on methods like downloadAndDecryptAttachment() that guarantee non-null returned bytes. Do not assume MatrixFile.bytes may be null; add null-safety checks only where the API guarantees non-null yield, and avoid nullable casts unless upgrading the SDK version. This guideline applies to Dart files across the codebase that interact with MatrixFile.
Applied to files:
lib/pages/chat/chat.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 (3)
lib/pages/chat/chat.dart (2)
3209-3214: LGTM!The
_initRoom()method correctly usesMatrix.read(context)for synchronous initialization without establishing an inherited widget dependency. The null room case is properly guarded ininitStateat line 3193-3194.
3178-3181: LGTM!Calling
_initRoom()at the start ofinitStateensures the room is initialized before any room-dependent operations like_initializePinnedEvents()and_tryLoadTimeline().docs/adr/0034-fix-notification-redirection-race-condition.md (1)
1-57: Well-documented ADR.The ADR clearly explains the race condition root cause, the lifecycle changes, and the trade-offs. Good practice documenting this architectural decision.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Contributor
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2835 |
tddang-linagora
temporarily deployed
to
PR-2835
GitHub Actions
Inactive
tddang-linagora
temporarily deployed
to
PR-2835
GitHub Actions
Inactive
tddang-linagora
force-pushed
the
TW-2834/Missing-message-on-notification-click
branch
from
13a02c3 to
ba3890f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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/chat.dart (1)
3688-3702:⚠️ Potential issue | 🔴 CriticalRoom-switch logic is mobile-only — the bug persists for chat navigation on mobile.
The
roomIdchange detection (oldWidget.roomId != widget.roomId) and subsequent initialization calls are nested inside theelsebranch (i.e.,!PlatformInfos.isMobile), meaning this logic never executes on mobile. When theChatwidget is reused during room navigation on mobile (e.g., tapping a notification for a different room),didUpdateWidgetfires but the critical room reset and reload is silently skipped, leaving the UI showing the previous room's state.The main route
/rooms/:roomidinstantiatesChatAdaptiveScaffoldwithout a key, causing the widget and itsChatchild to be reused on navigation rather than recreated.Move the
roomId-change guard before the platform check so it applies to both mobile and web:Proposed fix
`@override` void didUpdateWidget(covariant Chat oldWidget) { super.didUpdateWidget(oldWidget); + if (oldWidget.roomId != widget.roomId) { + _resetState(); + _initRoom(); + _tryLoadTimeline(); + _initializePinnedEvents(); + _listenRoomUpdateEvent(); + initCachedPresence(); + _requestParticipants(); + listenIgnoredUser(); + } + // Parse event ID from URL (support both mobile and web) String? highlightEventId; if (PlatformInfos.isMobile) { highlightEventId = GoRouterState.of(context).uri.queryParameters['event']; } else { - if (oldWidget.roomId != widget.roomId) { - _resetState(); - _initRoom(); - _tryLoadTimeline(); - _initializePinnedEvents(); - _listenRoomUpdateEvent(); - // Re-initialize other room-specific items - initCachedPresence(); - _requestParticipants(); - listenIgnoredUser(); - } final currentLocation = html.window.location.href; highlightEventId = Uri.tryParse( Uri.tryParse(currentLocation)?.fragment ?? '', )?.queryParameters['event']; }
🤖 Fix all issues with AI agents
In `@docs/adr/0034-fix-notification-redirection-race-condition.md`:
- Around line 61-64: The ADR's "Files Modified" list is missing the change to
the pinned events controller; update the list to include
lib/pages/chat/chat_pinned_events/pinned_events_controller.dart and mention the
added reset() method and the updated dispose() behavior so the documentation
matches the code changes to PinnedEventsController.reset and
PinnedEventsController.dispose.
In `@lib/pages/chat/chat.dart`:
- Around line 2983-3006: The _resetState method currently leaves several
room-scoped fields intact; update _resetState to also clear the read-marker and
draft timers and reset the FAB notifier by setting _markerReadLocation = null
and unreadReceivedMessageLocation = null, cancelling and nulling
_storeInputTimeoutTimer (call .cancel() if non-null then set to null) and
resetting showScrollDownButtonNotifier.value to its default (e.g. false) so no
state from the previous room bleeds into the newly loaded room; place these
operations alongside the other nullifications and cancellations in _resetState.
Comment on lines
+61
to
+64
| ### Files Modified | ||
|
|
||
| 1. `lib/pages/chat/chat.dart` - `initState`, `_initRoom`, `didUpdateWidget`. | ||
| 2. `lib/pages/chat/chat_view.dart` - Removed initialization logic from `build`. |
There was a problem hiding this comment.
Files Modified section is incomplete.
The ADR lists only chat.dart and chat_view.dart, but this PR also modifies lib/pages/chat/chat_pinned_events/pinned_events_controller.dart (added reset() method and updated dispose()). Consider adding it.
🤖 Prompt for AI Agents
In `@docs/adr/0034-fix-notification-redirection-race-condition.md` around lines 61
- 64, The ADR's "Files Modified" list is missing the change to the pinned events
controller; update the list to include
lib/pages/chat/chat_pinned_events/pinned_events_controller.dart and mention the
added reset() method and the updated dispose() behavior so the documentation
matches the code changes to PinnedEventsController.reset and
PinnedEventsController.dispose.
Comment on lines
+2983
to
+3006
| void _resetState() { | ||
| // Cancel any pending operations or subscriptions for the old room | ||
| onUpdateEventStreamSubcription?.cancel(); | ||
| cachedPresenceStreamSubscription?.cancel(); | ||
| _jumpToEventIdSubscription?.cancel(); | ||
| ignoredUsersStreamSub?.cancel(); | ||
| _timestampTimer?.cancel(); | ||
|
|
||
| // Clear notifiers that might hold old room state | ||
| replyEventNotifier.value = null; | ||
| editEventNotifier.value = null; | ||
| stickyTimestampNotifier.value = null; | ||
| selectedEvents.clear(); | ||
| pinnedEventsController.reset(); | ||
|
|
||
| // Reset timeline and futures | ||
| timeline?.cancelSubscriptions(); | ||
| timeline = null; | ||
| loadTimelineFuture = null; | ||
|
|
||
| // Reset other state variables | ||
| _pendingReadMarkerEventId = null; | ||
| _currentChatScrollState = ChatScrollState.endScroll; | ||
| } |
There was a problem hiding this comment.
_resetState doesn't clear read-marker and draft state for the old room.
A few room-scoped fields survive the reset and will bleed into the new room:
| Field | Risk |
|---|---|
_markerReadLocation / unreadReceivedMessageLocation |
Unread indicator may point at an event from the old room |
_storeInputTimeoutTimer |
Could fire after the switch and persist the old draft under the new room's key |
showScrollDownButtonNotifier |
May show/hide the FAB based on old scroll position |
Proposed additions to `_resetState()`
void _resetState() {
// Cancel any pending operations or subscriptions for the old room
onUpdateEventStreamSubcription?.cancel();
cachedPresenceStreamSubscription?.cancel();
_jumpToEventIdSubscription?.cancel();
ignoredUsersStreamSub?.cancel();
_timestampTimer?.cancel();
+ _storeInputTimeoutTimer?.cancel();
// Clear notifiers that might hold old room state
replyEventNotifier.value = null;
editEventNotifier.value = null;
stickyTimestampNotifier.value = null;
selectedEvents.clear();
pinnedEventsController.reset();
+ showScrollDownButtonNotifier.value = false;
// Reset timeline and futures
timeline?.cancelSubscriptions();
timeline = null;
loadTimelineFuture = null;
// Reset other state variables
_pendingReadMarkerEventId = null;
_currentChatScrollState = ChatScrollState.endScroll;
+ _markerReadLocation = '';
+ unreadReceivedMessageLocation = null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void _resetState() { | |
| // Cancel any pending operations or subscriptions for the old room | |
| onUpdateEventStreamSubcription?.cancel(); | |
| cachedPresenceStreamSubscription?.cancel(); | |
| _jumpToEventIdSubscription?.cancel(); | |
| ignoredUsersStreamSub?.cancel(); | |
| _timestampTimer?.cancel(); | |
| // Clear notifiers that might hold old room state | |
| replyEventNotifier.value = null; | |
| editEventNotifier.value = null; | |
| stickyTimestampNotifier.value = null; | |
| selectedEvents.clear(); | |
| pinnedEventsController.reset(); | |
| // Reset timeline and futures | |
| timeline?.cancelSubscriptions(); | |
| timeline = null; | |
| loadTimelineFuture = null; | |
| // Reset other state variables | |
| _pendingReadMarkerEventId = null; | |
| _currentChatScrollState = ChatScrollState.endScroll; | |
| } | |
| void _resetState() { | |
| // Cancel any pending operations or subscriptions for the old room | |
| onUpdateEventStreamSubcription?.cancel(); | |
| cachedPresenceStreamSubscription?.cancel(); | |
| _jumpToEventIdSubscription?.cancel(); | |
| ignoredUsersStreamSub?.cancel(); | |
| _timestampTimer?.cancel(); | |
| _storeInputTimeoutTimer?.cancel(); | |
| // Clear notifiers that might hold old room state | |
| replyEventNotifier.value = null; | |
| editEventNotifier.value = null; | |
| stickyTimestampNotifier.value = null; | |
| selectedEvents.clear(); | |
| pinnedEventsController.reset(); | |
| showScrollDownButtonNotifier.value = false; | |
| // Reset timeline and futures | |
| timeline?.cancelSubscriptions(); | |
| timeline = null; | |
| loadTimelineFuture = null; | |
| // Reset other state variables | |
| _pendingReadMarkerEventId = null; | |
| _currentChatScrollState = ChatScrollState.endScroll; | |
| _markerReadLocation = ''; | |
| unreadReceivedMessageLocation = null; | |
| } |
🤖 Prompt for AI Agents
In `@lib/pages/chat/chat.dart` around lines 2983 - 3006, The _resetState method
currently leaves several room-scoped fields intact; update _resetState to also
clear the read-marker and draft timers and reset the FAB notifier by setting
_markerReadLocation = null and unreadReceivedMessageLocation = null, cancelling
and nulling _storeInputTimeoutTimer (call .cancel() if non-null then set to
null) and resetting showScrollDownButtonNotifier.value to its default (e.g.
false) so no state from the previous room bleeds into the newly loaded room;
place these operations alongside the other nullifications and cancellations in
_resetState.
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
Root cause
In the ADR
Summary by CodeRabbit
Bug Fixes
Stability
Documentation