Repository navigation
fix(chat): show typing only from participants of the current conversation - #6905
ToteMeiSter wants to merge 1 commit into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
1c7b5586-699b-423c-b95c-264054a80196
📒 Files selected for processing: 10
app/src/main/java/com/nextcloud/talk/chat/ChatActivity.ktapp/src/main/java/com/nextcloud/talk/chat/MessageInputFragment.ktapp/src/main/java/com/nextcloud/talk/chat/TypingParticipant.ktapp/src/main/java/com/nextcloud/talk/signaling/ConversationMessageNotifier.ktapp/src/main/java/com/nextcloud/talk/signaling/SignalingMessageReceiver.ktapp/src/main/java/com/nextcloud/talk/webrtc/RoomSessionRegistry.ktapp/src/main/java/com/nextcloud/talk/webrtc/WebSocketInstance.ktapp/src/test/java/com/nextcloud/talk/chat/TypingParticipantTest.ktapp/src/test/java/com/nextcloud/talk/signaling/SignalingMessageReceiverSessionsLeftTest.ktapp/src/test/java/com/nextcloud/talk/webrtc/RoomSessionRegistryTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
da5969f1-13d9-4629-8c8f-b3b1801fc10f
📒 Files selected for processing: 5
app/src/main/java/com/nextcloud/talk/chat/ChatActivity.ktapp/src/main/java/com/nextcloud/talk/chat/TypingParticipant.ktapp/src/main/java/com/nextcloud/talk/webrtc/RoomSessionRegistry.ktapp/src/test/java/com/nextcloud/talk/chat/TypingParticipantTest.ktapp/src/test/java/com/nextcloud/talk/webrtc/RoomSessionRegistryTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
|
I added one more commit: 1517c84 "fix(chat): stop and expire typing per session, not per participant". de29297 keeps all sessions of a typing user, but only the leave event used them. A Now every session has its own 15 s timer, and Tests: How to test: log in with the same user on two devices, open the conversation on a third device. Type on both devices, then stop typing on one of them. "… is typing" stays visible. Stop on the second device: the indicator disappears. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2320d593-5685-463c-9573-ca56421955b7
📒 Files selected for processing: 2
app/src/main/java/com/nextcloud/talk/webrtc/RoomSessionRegistry.ktapp/src/test/java/com/nextcloud/talk/webrtc/RoomSessionRegistryTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
rapterjet2004
left a comment
There was a problem hiding this comment.
Thanks, the room scoping looks right: clearing the registry on a room change before currentRoomToken is updated, ignoring joins while no room is known, the per-session typing timers and keeping the typing map on the main thread all look good.
Change requested: the new test doesn't compile against master. WebSocketInstanceRoomSessionsTest.kt imports okhttp3.mockwebserver.*, but master replaced that dependency with mockwebserver3. CI tests the merge ref, so compileGplayDebugUnitTestKotlin fails with "Unresolved reference 'mockwebserver'" and no unit tests run at all. A rebase and a port to the mockwebserver3 API (as in #6828) should fix it.
Non-blocking:
- The test seams add three test-only constructor parameters (
testOkHttpClient,testEventBus,testConnectionHelper,@JvmOverloads) plus aninitbranch that skips injection toWebSocketInstance, and a test-only constructor toWebSocketConnectionHelper. Behaviour is unchanged with thenulldefaults, but it would be nicer to keep test scaffolding out of the production classes if there's a reasonable way. - Typing is now only accepted from sessions in the registry. Could you confirm with one manual test that typing from a federated participant still shows? I couldn't verify that federated sessions arrive as
joinevents on this websocket. - Typing that arrives between joining a room and receiving its join list is dropped. That's fine, the next keystroke re-sends it.
…tion The signaling connection of an account is shared by all conversations, but the session map of WebSocketInstance was filled by join events and cleared only by leave events. It kept the sessions of earlier rooms, so typing messages went to sessions of other rooms and typing from such sessions was shown as "Guest". - RoomSessionRegistry replaces the session map. It holds only the sessions of the current room and is cleared when the server answers with another room, when the room is left, after a hello without resume and on no_such_session / no_such_room. A resumed connection keeps it. - Join events of a left room are dropped: after a leave the room is unknown, so the answer to the leave clears the sessions as well. - Typing messages are sent only to the sessions of the current room and accepted only from them. - The leave event reaches the chat listener (onSessionsLeft), so the typing indicator of a session which left the room is hidden at once. - Typing is stopped and expired per session, not per participant. The tests of WebSocketInstance use mockwebserver3 and hand the client and the event bus to the production code through a mocked application component, so the production classes need no test seam. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
6eb2b01 to
8b914cb
Compare
|
Thanks for the review. All four points are handled in one commit, rebased on current Blocker: 1. Test seams in production code — removed 2. Typing from a federated participant
What is not verified (needs a manual test, I have no federated setup): that a typing message addressed to a federated session id is routed by the HPBs in both directions, and which display name HPB sends in 3. Typing between room join and the |
Problem
A conversation shows "Guest is typing…". The person who types is not a guest. They type in another conversation.
Cause
WebSocketInstance(usersHashMap) was filled by join events and cleared only by leave events. It was not cleared when the room changed, when the room was left, after a hello without resume, or onno_such_session/no_such_room. So it kept the sessions of earlier rooms.MessageInputFragmentsentstartedTyping/stoppedTypingto every session in the map.sessionto that session in any room. In nextcloud-spreed-signaling,server/hub.goprocessMessageMsgchecks only "same backend" and "not to self". It does not check the room of the recipient.src/utils/SignalingTypingHandler.jsin spreed sends typing only to the participants of the current room and ignores typing from sessions which are not in that list.Changes
RoomSessionRegistryreplacesusersHashMap. It is thread safe. It is cleared when the server answers with another room, when the room is left, after a hello without resume, and onno_such_session/no_such_room. A resumed connection keeps it.WebSocketInstancegives a snapshot of the room sessions without the own session, and checks if a session is in the current room.MessageInputFragmentsends typing only to the sessions of the current room.ChatActivityaccepts typing only from a session of the current room. A real guest still joins without a user id, stays in the registry and is still shown as "Guest".SignalingMessageReceiverandConversationMessageNotifierpass the leave event to the chat listener (newonSessionsLeft, empty by default).TypingParticipantstores the session and can remove the participants of left sessions.ChatActivitychanges the typing participants on the UI thread only, also for a stop message.stoppedTypingand the timeout forget only that session; the participant is removed with its last session.Tests
RoomSessionRegistryTestTypingParticipantTestSignalingMessageReceiverSessionsLeftTestHow to test
Tested on a Huawei DEL-LX9 and a Samsung SM-S926B (Android 17): "Guest is typing…" from another conversation no longer appears.
🖼️ Screenshots
No screenshots. The UI change is the absence of a wrong typing line.
🚧 TODO
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)
The code and the tests were written by an AI agent (Claude Code, model claude-sonnet-5-5). I tested the change on a device and I am responsible for it. The commits carry the trailer
Assisted-by: Claude-Code:claude-sonnet-5-5. This description was also prepared with AI help and proofread by me.