Repository navigation
fix(chat): release the active chat only if it is still ours - #1517
Merged
Merged
Conversation
Opening a chat directly from another chat left notifications firing for the chat on screen. ChatViewModel.onCleared cleared the active chat id unconditionally, and Navigation 3 keeps both entries alive across that kind of switch: the outgoing entry is disposed after the incoming one has already claimed the chat, so the outgoing screen's teardown cleared the chat the user was by then looking at. An on-device trace caught the window — `Navigating to Chat[VDbei…]` at 13:58:55.368, the outgoing `[W4rX…]` screen's dispose at 13:58:55.892. Going back to the chat list first puts the dispose ahead of the open, which is why the same chat notified correctly when reached that way and made this look chat-specific. It is not: any chat opened straight from another one hits it. Replace the unconditional clear with clearActiveChat(chatId), a compare-and- clear that releases the chat only if it is still the active one. Correct in either dispose order. RealChatCoordinator also drops the backgrounded stash on a match, so a screen destroyed while the app is away is not restored as the active chat with nothing on screen.
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.
Opening a chat directly from another chat left notifications firing for the chat on screen — tap a push for chat B while chat A is open, land in B, and every subsequent message in B notifies.
Cause
The active chat id suppresses notifications for the conversation on screen (
NotificationService.kt:261). Each chat screen claimed it on open and released it inonCleared— unconditionally, which is the bug. Navigation 3 keeps both entries alive across a chat-to-chat switch and disposes the outgoing one after the incoming one has already claimed the chat, so the outgoing screen's teardown cleared the chat the user was by then looking at.An on-device trace caught the window:
Going back to the chat list first puts the dispose ahead of the open, which is why the same chat behaved correctly when reached that way and made this look chat-specific. It is not — any chat opened straight from another one hits it, DM or group. The notification-tap route (
AppRouterresolvingTipChatto[Tips, Chat]) is the common way in.Change
setActiveChatId(null)inChatViewModel.onClearedbecomesclearActiveChat(chatId), a compare-and-clear on the coordinator that releases the chat only if it is still the active one. Correct in either dispose order, so it does not depend on the timing that varies by route.RealChatCoordinatoralso drops the backgrounded stash on a match:onStopsaves the active chat foronStartto restore, and a screen destroyed while the app is away would otherwise come back as the active chat with nothing on screen, silencing it permanently.Tests
ActiveChatHandoffTestcovers the handoff, ordinary teardown, an unresolved chat id, and the backgrounded case. The handoff test discriminates: reverting the conditional to an unconditional clear fails it and leaves the other three green.