Repository navigation
fix(chat): show typing only from participants of the current conversation - #6905
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
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 16.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 74 functions across 10 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: restricting typing indicators to participants in the current conversation. Description check The description is complete and follows the repository template. It explains the problem, cause, changes, tests, manual verification, TODO status, and checklist. The backport and milestone items remai… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is complete and follows the repository template. It explains the problem, cause, changes, tests, manual verification, TODO status, and checklist. The backport and milestone items remain unchecked, but these are non-critical metadata items.
-
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![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
ℹ️ 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
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.
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 |
|
Thank you for you contributions. We will try to review this soon! |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
…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>
When the connection switches rooms without leaving first (e.g. during a call), the sessions of the previous room stay in the registry until the server answers the join of the new room. A typing message from such a session was shown in the conversation of the new room. The chat now checks the session together with its own room token, so a session counts only for the room the connection is in. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
… room in tests - isSessionInCurrentRoom() was only used by tests after the room token check, so it is removed and the tests use isSessionInRoom(), the check the chat makes. - removeSessions() of TypingParticipant forgets the sessions in a plain loop instead of inside a filter predicate. - RoomSessionRegistry documents why a resumed session keeps the sessions: the signaling server stores the messages of a disconnected session, including join and leave events, and sends them on resume. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
16c4fa6 to
585854b
Compare
|
/backport to stable-25.1.x |
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.