Repository navigation
fix(chat): keep X-Chat-Last-Common-Read from the read marker response - #6846
ToteMeiSter wants to merge 2 commits into
Conversation
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to Check marks may briefly regress when refresh responses arrive out of order. The issue is localized and recoverable, but the ordering fix should be considered before merging. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 15.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 70 functions across 15 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly summarizes the main change: preserving the X-Chat-Last-Common-Read header from the read-marker response. Description check The description explains the problem, implementation, testing, manual validation, and AI involvement. It omits the optional screenshots and TODO sections, and the backport and milestone checklist item… 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.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
![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
🧹 Nitpick comments (1)
app/src/test/java/com/nextcloud/talk/jobs/ReadMarkerSyncWorkerTest.kt (1)
44-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a worker-level success-path test.
ReadMarkerSyncWorkerTesttests header parsing and callsConversationListUpdaterdirectly. It does not executeReadMarkerSyncWorkeror verify that a successful response callsupdateLastCommonRead("$userId@$roomToken", value). A regression that removes the worker call can therefore pass the current tests while the user's own-message check marks remain stale. Add a focused worker test, or a test seam that still exercises this worker-to-updater call.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
302bbf35-93ce-4a9a-95f6-b82e39493d93
📒 Files selected for processing (9)
app/src/main/java/com/nextcloud/talk/api/NcApi.javaapp/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.ktapp/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.ktapp/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.ktapp/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.ktapp/src/main/java/com/nextcloud/talk/jobs/ReadMarkerSyncWorker.ktapp/src/main/java/com/nextcloud/talk/receivers/MarkAsReadReceiver.ktapp/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.ktapp/src/test/java/com/nextcloud/talk/jobs/ReadMarkerSyncWorkerTest.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.
|
Review: #6846 (review) @coderabbitai Addressed in e9055e7: |
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:
b2bb1429-9bf0-4765-b1b4-506d25b2dfb5
📒 Files selected for processing (10)
app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.ktapp/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.ktapp/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.ktapp/src/main/java/com/nextcloud/talk/jobs/ReadMarkerSyncWorker.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdaterCommonReadTest.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdaterPinTest.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdaterUnreadTest.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.ktapp/src/test/java/com/nextcloud/talk/jobs/ReadMarkerSyncWorkerTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt
- app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.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.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
for file in \
app/src/test/java/com/nextcloud/talk/jobs/ReadMarkerSyncWorkerTest.kt \
app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdaterCommonReadTest.kt
do
ast-grep outline "$file" --items all
cat -n "$file"
doneLength of output: 16428
🧠 Learnings usedIf 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. |
The answer to POST chat/<token>/read carries the last message read by all participants in the X-Chat-Last-Common-Read header. The read marker worker dropped it, so the read check marks of the own messages stayed behind until the next chat sync or room list refresh. Return the whole response from setChatReadMarker, write the header value to the conversation entry and let the open chat pick it up from there. A non-2xx answer is now treated as a failed attempt explicitly, as the response is no longer thrown as an error. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
A room list response requested before the read marker answer reached the client can still carry an older X-Chat-Last-Common-Read value, and applying it after the answer lowered the read check marks again until the next sync. The read marker pending guard cannot catch this, it only covers lastReadMessage. Remember when the answer arrived and give preservePendingLocalState the start time of the request that produced the response: a response requested before that moment cannot lower the value, one requested after it applies unchanged and ends the guard. A sync holds a lock from its merge through its write and the answer takes the same lock, so an answer cannot slip in between the two and be overwritten. The value written from an answer may be lower than the stored one, as marking a chat as unread lowers it on the server. A failure while storing the value no longer leaves the worker, the marker has been delivered and must not be sent again. The worker test now runs the worker. Assisted-by: Claude-Code:claude-opus-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
e9055e7 to
9a3375a
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
The response to
POST chat/<token>/readcarries the last message read by all participants in theX-Chat-Last-Common-Readheader.ReadMarkerSyncWorkerdropped it (setChatReadMarkerreturned only the body), so the two check marks of the user's own messages stayed behind until the next chat sync or room list refresh.Change
setChatReadMarkerreturns the wholeResponse<GenericOverall>,ReadMarkerSyncWorkerreads the header (lastCommonReadOf) and stores it in the conversation entry viaConversationListUpdater.updateLastCommonRead(newConversationsDao.updateLastCommonRead); the open chat already combinesconversation.lastCommonReadMessageinto its status icons,MarkAsReadReceivercheckisSuccessfulexplicitly (a failed answer still retries/does not cancel the notification, as before).How to test
./gradlew :app:testGenericDebugUnitTest --tests "com.nextcloud.talk.jobs.ReadMarkerSyncWorkerTest"(header parsing, missing/broken header, value reaches the DAO).The worker's
doWork()itself is not unit tested (WorkManager injection).🏁 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).