Repository navigation
Refresh the conversation list while it is open - #6746
Conversation
abc0d72 to
7d19a5c
Compare
008bbaf to
e931686
Compare
7d19a5c to
f92c673
Compare
e931686 to
cc193bf
Compare
f92c673 to
523a0fa
Compare
cc193bf to
4916c3c
Compare
523a0fa to
6621cf4
Compare
4916c3c to
8fd1b66
Compare
6621cf4 to
76d0e52
Compare
8fd1b66 to
1c54f14
Compare
76d0e52 to
fbd79d1
Compare
1c54f14 to
d1cf526
Compare
fbd79d1 to
9b7b1a6
Compare
d1cf526 to
61d24d9
Compare
9b7b1a6 to
d9a67e2
Compare
61d24d9 to
6cbc0f4
Compare
d9a67e2 to
ae967b3
Compare
6cbc0f4 to
e314853
Compare
ae967b3 to
3da4c9f
Compare
e314853 to
79bbdf7
Compare
3da4c9f to
9596d4e
Compare
79bbdf7 to
d6f1aeb
Compare
9596d4e to
b9aa2b8
Compare
d6f1aeb to
f7eb4c3
Compare
95d81d0 to
5c93aee
Compare
f4d924e to
137d96b
Compare
5c93aee to
0b0389f
Compare
137d96b to
05f03bd
Compare
0b0389f to
d190020
Compare
05f03bd to
35d92e1
Compare
d190020 to
e74c3a1
Compare
35d92e1 to
66b34e9
Compare
e74c3a1 to
81d9079
Compare
66b34e9 to
4ac0f98
Compare
81d9079 to
40eb091
Compare
4ac0f98 to
dd6512e
Compare
40eb091 to
911db4c
Compare
dd6512e to
cce7913
Compare
911db4c to
ffc197a
Compare
cce7913 to
f880b1f
Compare
ffc197a to
731fcc1
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe conversation repository adds a periodic-sync eligibility check and serializes room-list syncs by account. While the activity is resumed, it requests a refresh every 30 seconds. The view model skips refreshes during an active sync, while search is active, or when periodic synchronization is not due. New tests cover these conditions and verify that foreground refreshes do not request a full sync. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The refresh loop follows the documented sync cadence and stops when the list is no longer resumed. No actionable merge-blocking risk remains after normal checks. 🚥 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: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2d765032-196b-4842-9f4c-5e625259780f
📒 Files selected for processing (5)
app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.ktapp/src/main/java/com/nextcloud/talk/conversationlist/data/OfflineConversationsRepository.ktapp/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.ktapp/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.ktapp/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.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.
|
@mahibi one thing from #6745 I would rather you decide than decide for you, since it is your call on master code rather than something this PR introduces. In #6745 you removed the connectivity pre-check from the background sync ( override fun getRooms(user: User, forceFullSync: Boolean): Job =
scope.launch {
if (networkMonitor.isOnline.value) {
The same reasoning you applied to the worker would say to attempt the request and report the failure, which also gets a sync error onto I have left it alone because it predates this PR and because you made the opposite choice for the foreground path deliberately. Happy to change it here, in a separate PR, or not at all — whichever you prefer. |
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:
ff4db2f9-5980-4ac4-90e4-5de13690736d
📒 Files selected for processing (2)
app/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.ktapp/src/test/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModelForegroundRefreshTest.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.
📱 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 |
With the list on screen, a message sent from another device shows up only when a push arrives or the user leaves the screen and comes back. That gap matters most for the people who have no working push at all - no Play Services, no UnifiedPush distributor - for whom nothing else refreshes the list while they are looking at it. Re-sync it every thirty seconds instead, matching the iOS timer. The loop is bound to the resumed lifecycle, so it cannot run behind the screen or outlive it, and it waits before the first sync rather than after: onResume already fetches, and syncing on entering the loop would fetch twice every time the screen is resumed. A tick is dropped while a sync is already in flight, so the loop can never queue behind itself, and while a search is open, where swapping the list out would move what the user is reading. Being offline and battery saver are deliberately not checked here: the first is already handled where the sync would make its request, and the second already stops the expensive half, the message catch-up it triggers. Every tick asks only for what changed, which is what makes a timer this short affordable at all. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
The loop fires whatever else is going on, so what decides whether the list stays usable is when a tick is dropped: one that is not dropped while a sync is running queues requests behind each other, and one that is not dropped during a search swaps the list out from under what the user is reading. The two cases pin each other down. A tick during an in-flight sync must leave the call count at one, and a tick after the previous sync finished must take it to two, so neither dropping every tick nor dropping none of them passes both. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
|
getRooms cleared the loading flag when its own job finished, even if another sync started meanwhile was still running. With resume, pull to refresh and the foreground refresh tick all starting syncs, they now overlap more often, and two things went wrong when they did: - pull to refresh stopped spinning when the older sync finished, while the forced full sync it started was still running, so a conversation left elsewhere could stay visible after the indicator was gone - the next refresh tick saw no sync in flight and started another one alongside the one still running Count the syncs in flight and clear the flag only when the last one completes. getRooms is also called from a background event subscriber, so the count and the flag update are guarded by a lock. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Each room list sync reads the stored conversations, merges its response into them and writes the result, and a full sync deletes what its response no longer has. Nothing kept two syncs of the same account from running at once, and they are started from several places: the screen, pull to refresh, the foreground refresh tick and the background worker. Interleaved, they undo each other. A delta sync that is still running when a pull to refresh starts a full sync writes its response after the full sync has deleted a conversation the user left elsewhere, and puts it back until the next full sync. An older response can likewise overwrite newer state. The refresh tick only avoids starting a sync while another one runs; the other callers do not check. Hold a per-account lock for the whole sync, including the choice of modifiedSince, so a waiting sync builds on what the previous one stored. Accounts do not wait for each other. Waiting for the lock counts against the worker's time budget, and a failed read of the stored sync state is now handled as a failed sync instead of escaping the sync's coroutine. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…nning sync refreshRoomsIfIdle checked for a running room list sync and an open search first, and only then read from storage whether a sync was due. That read suspends, so a sync or a search started while it ran went unnoticed and the tick refreshed anyway: a redundant request, or a list refreshed under a search the skip was meant to protect. Read the cadence first, so nothing suspends between the checks and the call to getRooms. The read now also runs on ticks that are then skipped, which is one small storage read every thirty seconds. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
2 participants
With the list open, a message sent from another device only shows up when a push arrives or the user leaves and comes back. This adds the 30-second refresh timer iOS has, which matters most for the people who have no working push at all — no Play Services, no UnifiedPush distributor — for whom this becomes the only freshness mechanism the list has.
What it does
onResumealready fetches, and syncing on entry would fetch twice on every resume.What a reviewer should push back on
includeStatus, because with it the server returns every one-to-one room regardless ofmodifiedSinceand most of the saving disappears. iOS keeps status fresher here; this trades that for the request volume.Not in this PR
delete/disinvitesignaling messages, which would let the five-minute rule relax.🚧 TODO
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)