Don't delete conversations that arrive while a list sync is in flight - #6749
AndyScherzinger wants to merge 1 commit into
Conversation
📱 QA build
The QA build installs alongside a released Nextcloud app, so you can keep Downloading the file requires a GitHub account, so open this link on the |
cd90e54 to
88d6996
Compare
d94e69b to
554f690
Compare
AndyScherzinger
added
3. to review
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe repository now captures an account’s conversation IDs before requesting rooms from the server. It limits sync deletion candidates to conversations in that snapshot. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Sync protects conversations added during an in-flight room request, and the regression test exercises deletion reconciliation. No identified issue blocks merging. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change narrows which conversations a sync may delete, protecting conversations that arrive during the request. Account isolation and atomic database updates remain intact. No material security risk introduced or worsened by this change was identified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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[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: f13dcaf9-a12c-479b-b7ad-5e14bac0aa90
📒 Files selected for processing (4)
app/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/utils/preview/ComposePreviewUtilsDaos.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.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.
A conversation list sync reads the locally known conversations after the server has answered, and treats every one the response does not mention as a conversation the user left. A conversation that reached the database while the request was in flight - a single-room fetch, a room joined on another device, one the user just created - was never in that response and could not have been, so it was deleted, taking its cached messages and chat blocks with it through the foreign key cascade. Take the known conversation ids before the request goes out and reconcile removals only against those. Anything that appeared afterwards is left alone until the next full sync, which is the first one whose response can speak about it at all. Reading the conversations themselves earlier would not do: that same read feeds the pending local state guard, which needs the freshest state to protect a favourite or read marker that is still on its way to the server. Only the removal reconcile wants the older snapshot, so only it gets one, as an id query that does not load the rows. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
554f690 to
54a04a5
Compare
1 participant
A conversation list sync reads the locally known conversations after the server has answered, and treats every one the response does not mention as a conversation the user left. A conversation that reached the database while the request was in flight — a single-room fetch, a room joined on another device, one the user just created — was never in that response and could not have been, so it was deleted, taking its cached messages and chat blocks with it through the foreign key cascade.
What it does
Takes the set of known conversation ids before the request goes out, and reconciles removals only against those. A conversation that appeared afterwards is left alone until the next full sync, which is the first one whose response could actually speak about it.
Why not simply read the conversations earlier
The same read feeds
preservePendingLocalState, which needs the freshest local state to protect an in-flight favourite or read marker. Moving it earlier would trade this bug for that one. Only the removal reconcile wants the older snapshot, so only it gets one — as an id-only query, so the extra read does not load every conversation row.Note for reviewers
detektis red on this branch. It is red onmastertoo, at the same count: 111 weighted issues against amaxIssuesof 110, measured onmasterwith this branch stashed. This change adds none.🚧 TODO
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)