Repository navigation
fix(chat): do not send a read marker below the user's own message - #6844
ToteMeiSter wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 24.19% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 62 functions across 9 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 fix: preventing read markers from being sent below the user's own message. Description check The description explains the problem, cause, implementation, testing approach, limitations, checklist status, and AI assistance. The optional Screenshots and TODO sections are absent, but the descript… 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.
- Fix all pre-merge checks with AI
- 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: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
1df25bda-fe1e-409c-b2fa-a4411fa8bd4c
📒 Files selected for processing (2)
app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktapp/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
rapterjet2004
left a comment
rapterjet2004
left a comment
There was a problem hiding this comment.
LGTM, thanks for the server-side analysis. Taking the max with the newest accepted own message is safe with respect to the earlier "marker too high" issue (#6330): newestOwnMessageId filters on jsonMessageId > 0, and both temporary text ids and upload placeholder ids are ≤ 0. The tests cover the relevant sequences well.
One suggestion (non-blocking): SendStatus.PENDING also covers messages queued while offline, waiting for the network. While such a message exists, readMarkerToSend returns null and setChatReadMessage isn't called at all, which also skips the local read state update. Example: offline, open a chat with 5 unread messages, read them, write a reply and leave: the conversation list keeps showing 5 unread until the message is sent and the next sync corrects it. Still updating the local read state in that case and only skipping the server request would avoid it.
Also: check-ai-trailers fails, most likely because of the trailer format. Assisted-by: Claude Code (claude-sonnet-5-5) should pass the allowed pattern.
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 GitHub limitations.
🟠 Major · Keep marker decisions on the latest message window. · ChatViewModel.kt:1305
app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt:1305
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep marker decisions on the latest message window.
In
SEARCH_MODEorOLD_CHATBLOCK_MODE,messagesFlowcan observe messages around an older anchor. This cache can therefore omit a newly accepted own message. If the local marker is 2623 and that own message is 2624, leaving the anchored view can send 2623 and move the server marker below the own message. Observe the latest window separately for read-marker decisions; keep the anchored window for rendering.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
91343e03-eaa2-4ef9-bf8b-3fb801da44f9
📒 Files selected for processing: 7
app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.ktapp/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.ktapp/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktapp/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.ktapp/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.ktapp/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.
|
@coderabbitai Re: the outside-diff comment "Keep marker decisions on the latest message window" (ChatViewModel.kt:1305): Confirmed (the code is from 8b79419, not from the latest push). In search / old chat block mode |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -t f '^(ChatViewModel|OfflineFirstChatRepository|ChatViewModelReadMarkerTest)\.kt$'
ast-grep outline app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt --match 'onMessagesChanged|setChatReadMessage' --view expanded
ast-grep outline app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt --match 'observeMessagesForAnchor|messagesFlow' --view expanded
ast-grep outline app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.kt --items all --view expanded
rg -n -C 12 'latestMessages|fun onMessagesChanged|observeMessagesForAnchor' app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt
rg -n -C 15 'a window around an old message|simulateOld|oldMessage|anchor' app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.ktLength of output: 22359
I checked the current code. I inspected the tests but did not run them. ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
Thanks for the review. Both points are below. (1) Local read state. Confirmed: the offline-queued message (
Why not the full local update you suggested: I tried it in a3f4b1c. If the local Side effect: an online room list sync while the message is still unsent restores the server count until the message goes out. Offline there is no sync, so your scenario keeps the lowered count. Tests:
(2) check-ai-trailers. The format is not the cause. In run https://github.com/nextcloud/talk-android/actions/runs/38146825477 the search step found both trailers ( |
The server moves the read marker of the sender to a message when it is sent (ChatManager::addChatMessage) but accepts any later POST chat/<token>/read, including a lower value. The chat sent its local marker on leaving the chat while that marker still pointed below the own, just sent message. lastCommonRead is the minimum over the participants, so the recipient kept seeing a single check mark after a reply. Advance the local marker to the newest accepted own message, never send a marker below it, and skip the request while the sent message is not yet confirmed by the server (its temporary copy is still shown). Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
The check that keeps the read marker from going below the user's own message looked at the rendered items. They lag behind the database (throttled rebuild), so a message the server had already accepted could be missing there when the chat was left right after sending, and the older local marker was sent over the marker the server had moved to that message. Take the decision from the message list of the database instead, updated before the throttle: a stored temporary copy of the user's text message that did not fail is treated as possibly accepted, whether the request is still in flight or only its server copy is missing. A file placeholder counts only after the file was shared, as the server has no message for it before that. Assisted-by: Claude-Code:claude-opus-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
… queued A text message queued offline (SendStatus.PENDING) made readMarkerToSend return null, so setChatReadMessage was skipped as a whole, including the local read state update. The conversation list kept showing the messages as unread until the message was sent and the next sync. Write the local read state whenever the conversation lacks it and skip only the server request while an own message is on its way, so the marker is still never sent below the own message. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
The previous commit wrote the full local read state while an own message was queued. The stored marker then equalled the held-back one, so the "newer than the conversation marker" check suppressed the send for good: after the message failed, the server stayed unread and the pending marker guarded the local state. While the marker has to wait, lower only the unread count of the conversation entry. The stored marker stays the one the server knows and nothing is registered as pending, so the marker is decided again by the next call and sent once the message is confirmed or failed. Add a test on the real ChatViewModel for the path updateRemoteLastReadMessageIfNeeded -> repository -> ReadMarkerSyncWorker. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
…e window is shown Review findings on the held-back read marker: - updateLocalUnreadCountOnly read the conversation, suspended on the count and then wrote the stored marker back with updateReadState, so a marker written meanwhile was overwritten with the old one. Write only the unread count (ConversationsDao.updateUnreadMessages). - The decision on the read marker used the last list the view model received. In search and old chat block mode that is a window around a quoted message, which can miss the own message just sent: leaving the chat from there sent a marker below it. Keep the list of the newest messages and ignore anchored windows. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
b789097 to
cc98e55
Compare
|
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.) |
Problem
After the user sends a message, the chat can send a stale
POST chat/<token>/readthat carries a value below the message the user has just sent. The server accepts it and moves the user'slastReadMessagebackwards,lastCommonRead(the minimum over the participants) follows, and the other participant keeps seeing a single check mark for a conversation the sender is actively replying in.Server side confirmation (binlog of
oc_talk_attendees.last_read_messageof the sender; two out of two occurrences in a 10 minute window):POST …/readlast_read_messageChatManager::addChatMessagesets the sender's marker to the new message,ChatController::setReadMarkerwrites whatever the client sends (updateLastReadMessage()has no "only forward" check, unlike the automatic marker).Cause
ChatViewModel.updateRemoteLastReadMessageIfNeeded()(called fromChatActivity.onPause) sendslocalLastReadMessagewhen it is greater than the conversation'slastReadMessage.localLastReadMessageonly follows the newest message on screen / the conversation entry, so after a reply it can still point to the message before the own one, while the server already moved the marker to the own message.Change
newestOwnMessageId),readMarkerToSend()never returns a value below it,hasUnconfirmedOwnMessage): the server has already moved the marker.The change is inside the existing
advanceLocalLastReadMessageIfNeeded/updateRemoteLastReadMessageIfNeededpath, no new request and no new mechanism.How to test
./gradlew :app:testGenericDebugUnitTest --tests "com.nextcloud.talk.chat.viewmodels.ChatViewModelTest"(the sequence 2623 → own 2624, conversation marker 2620 now yields 2624; with an unconfirmed own message nothing is sent; incoming only → old behaviour).Not covered by unit tests: the
onPausewiring itself (no instrumented test infrastructure in this change).🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)
The change and the tests were prepared with AI assistance (Claude Code,
claude-sonnet-5-5).