Repository navigation
Conversation
📝 WalkthroughWalkthroughHTTP 429 login responses now map to a dedicated state and message. Chat polling applies bounded backoff after failures. Unauthorized-response handling uses a credential-tracking interceptor and a separate remote-wipe handler. Conversation sync errors carry account IDs. Direct Share shortcut avatars use conversation-list avatar content. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Fix the sync error path before merging: a cache lookup failure can interrupt recovery from an already failed sync. The previously identified wipe-handling and shortcut-request risks also remain unresolved in the supplied evidence. 🚥 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: 4
🧹 Nitpick comments (1)
app/src/test/java/com/nextcloud/talk/utils/RemoteWipeHandlerTest.kt (1)
39-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive-path handler test.
No existing test exercises a successful
wipe = trueresponse. Add a focused test that assertsscheduleUserForDeletionWithId(...)is called andwipeIfRequested(...)returnstrue.Robolectric is already available. WorkManager testing is configured only for instrumented tests, so the unit test also needs WorkManager test support, initialization, and a real application context. This is a localized coverage improvement, not a current production failure. The positive path schedules account deletion, so the coverage benefit justifies the setup cost.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
71c8dba5-9432-445c-8bf3-b095787e9b79
📒 Files selected for processing (25)
app/src/main/java/com/nextcloud/talk/account/BrowserLoginActivity.ktapp/src/main/java/com/nextcloud/talk/account/data/LoginRepository.ktapp/src/main/java/com/nextcloud/talk/account/data/network/NetworkLoginDataSource.ktapp/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.ktapp/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.ktapp/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.ktapp/src/main/java/com/nextcloud/talk/conversationlist/DirectShareHelper.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/ui/AvatarContent.ktapp/src/main/java/com/nextcloud/talk/conversationlist/ui/ConversationListItem.ktapp/src/main/java/com/nextcloud/talk/conversationlist/viewmodels/ConversationsListViewModel.ktapp/src/main/java/com/nextcloud/talk/dagger/modules/RestModule.javaapp/src/main/java/com/nextcloud/talk/utils/RejectedCredentialsInterceptor.ktapp/src/main/java/com/nextcloud/talk/utils/RemoteWipeHandler.ktapp/src/main/java/com/nextcloud/talk/utils/RemoteWipeInterceptor.ktapp/src/main/java/com/nextcloud/talk/utils/bundle/BundleKeys.ktapp/src/main/res/values/strings.xmlapp/src/test/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModelTest.ktapp/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryTest.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.ktapp/src/test/java/com/nextcloud/talk/login/data/network/NetworkLoginDataSourceTest.ktapp/src/test/java/com/nextcloud/talk/utils/RejectedCredentialsInterceptorTest.ktapp/src/test/java/com/nextcloud/talk/utils/RemoteWipeHandlerTest.ktapp/src/test/java/com/nextcloud/talk/utils/RemoteWipeInterceptorTest.kt
💤 Files with no reviewable changes (3)
- app/src/test/java/com/nextcloud/talk/utils/RemoteWipeInterceptorTest.kt
- app/src/main/java/com/nextcloud/talk/utils/bundle/BundleKeys.kt
- app/src/main/java/com/nextcloud/talk/utils/RemoteWipeInterceptor.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.
Contributor
📱 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 |
mahibi
marked this pull request as draft
October 7, 2026 07:55
mahibi
force-pushed
the
checkIfServerRequestedWipe
branch
2 times, most recently
from
October 7, 2026 08:46
d8238dc to
1f00543
Compare
mahibi
marked this pull request as ready for review
October 7, 2026 10:35
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryTest.kt (1)
116-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSynchronize the first test with the first request.
The 200 ms observation starts at launch, but the failed-request delay starts only after the request fails and is 1,000 ms. If
Dispatchers.Defaultstarts polling late, the test can see one request and pass before a retry is due. If polling never makes the request,times(1)fails; it does not pass. Awaiting the first request before starting the observation window closes the late-start coverage gap.Suggested synchronization
- wheneverBlocking { network.pullChatMessages(any(), any(), any()) } - .thenReturn(Response.error(HTTP_UNAUTHORIZED, "".toResponseBody("text/plain".toMediaType()))) + val firstRequestSent = CompletableDeferred<Unit>() + wheneverBlocking { network.pullChatMessages(any(), any(), any()) } doSuspendableAnswer { + firstRequestSent.complete(Unit) + Response.error(HTTP_UNAUTHORIZED, "".toResponseBody("text/plain".toMediaType())) + } val polling = launch(Dispatchers.Default) { repository.initLongPolling() } + withTimeout(AWAIT_TIMEOUT_MILLIS) { firstRequestSent.await() } // shorter than the wait after a failed request, so only the first request may have been sent delay(LONG_POLLING_OBSERVATION_MILLIS)
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
76b8397e-c998-436a-bc16-ec065445c486
📒 Files selected for processing (11)
app/src/main/java/com/nextcloud/talk/activities/BaseActivity.ktapp/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.ktapp/src/main/java/com/nextcloud/talk/conversationlist/DirectShareHelper.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/events/RemoteWipeEvent.ktapp/src/main/java/com/nextcloud/talk/jobs/RemoteWipeSuccessWorker.ktapp/src/main/java/com/nextcloud/talk/utils/RemoteWipeHandler.ktapp/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryTest.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.ktapp/src/test/java/com/nextcloud/talk/utils/RemoteWipeHandlerTest.kt
💤 Files with no reviewable changes (2)
- app/src/main/java/com/nextcloud/talk/events/RemoteWipeEvent.kt
- app/src/main/java/com/nextcloud/talk/activities/BaseActivity.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
marked this pull request as draft
October 7, 2026 15:56
…re rejected A failed room sync only reached the UI when no conversations were cached, so with a cached list a 401 was only logged and the dialog to reauthorize or remove the account was never shown, e.g. after the app password was revoked. The sync also retried a 401 three times. Every attempt is a failed login for the server, so its brute force protection soon answered all requests with 429. Now a 401 is not retried and always reaches the UI. As rooms are fetched on every resume, the dialog is only shown when it is not already showing. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
RemoteWipeInterceptor removed an account for any 401 that carried its credentials. It asked the server via /core/wipe/check, but the answer only decided whether a wipe success was reported afterwards. So a 401 caused by e.g. a password change in an external user backend, or one that was temporarily unreachable, removed the account. Now the server is asked when the room sync of the conversation list gets a 401, before the dialog to reauthorize or remove the account is shown. Only when it answers that it requested a wipe, the account is removed, the same way as with "remove account", and the wipe is reported to the server afterwards. Otherwise the dialog is shown. The list syncs again on every resume, and the server answers at most 10 wipe checks per 5 minutes from an IP, so each conversation list asks once. The interceptor is removed, so a wipe is not noticed by background requests any more, but once the conversation list is opened. The Files app also only checks for a wipe when it asks for new credentials. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
The long polling sent the next request right away when one failed. A request with rejected credentials fails at once, so an open chat sent requests as fast as the server answered. The server counts each of them as a failed login, so its brute force protection soon answered all requests from that IP with 429, which also returned at once. Now the long polling waits after a failed request, starting with one second and doubling up to a minute, until a request succeeds again. The field map is passed to the syncer directly instead of through a Bundle, which only the long polling used. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Once the server rejected the credentials of an account, e.g. after its app password was revoked or its password changed in an external user backend, every screen and worker kept sending them. The server counts each of those requests as a failed login, so its brute force protection soon answered all requests from that IP with 429, also those of other accounts and devices. Now a request with credentials that got a 401 is answered with a 401 without sending it. Other credentials, e.g. after reauthorizing the account, are sent as usual. As the rejection might be temporary, one request with the rejected credentials is sent again every five minutes, and once it is not rejected they are sent as usual again. A 401 after a redirect that dropped the credentials, and a 429, say nothing about the credentials, so they do not change whether they count as rejected. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…gins After too many failed logins from an IP, the server answers every login from it with 429 for a while, without checking the credentials. A login with a one-time QR code then only showed "Sorry, something went wrong", so it was not clear that waiting or another network would help. Now a 429 for the one-time login shows that there were too many failed login attempts from this network. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…e list The direct share shortcuts are published again with every change of the conversation list, also right when it is opened, and each time the avatars of all shortcut conversations were loaded with the credentials of the account. They used another URL and cache key than the list, so they never found the avatars the list had loaded, and with rejected credentials nothing was cached at all. So opening the list sent up to the maximum number of shortcuts of avatar requests. The server counts each of them as a failed login, which together with the avatars of the list itself was enough for its brute force protection to answer all requests from that IP with 429. Now the shortcuts load their avatars with the same URL, cache key and theme as the list, so the avatars the list loaded come from the cache, and the others are loaded from the server and then cached for the list as well. They are loaded one after another, and only one publication runs at a time, so while the server rejects the credentials only the first request reaches it, and RejectedCredentialsInterceptor does not send the following ones. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
The conversation repository is shared by all accounts, and a sync runs on in its own scope after the screen that started it is gone. Its errors did not say which account they belong to, so every conversation list showed them. Since a rejected login is shown even with cached conversations, this also includes the dialog to reauthorize or remove the account, and the remote wipe check before it. When the account was switched while the sync of the previous one was running, its 401 reached the conversation list of the new account. That list checked for a requested wipe with the token of the new account and offered to reauthorize or remove it, so removing it removed an account whose credentials were fine. Now each sync error carries its account, and a conversation list only shows the errors of its own account. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
The test of the early reconnect started the long polling on another dispatcher and changed the connectivity after fixed delays. If the polling started late, the connectivity could change before its first request failed, and the test saw fewer requests although the long polling worked. Now the test only goes offline once the first request was sent, and waits for the second request with a timeout that is still shorter than the wait after a failed request. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
… again When the room sync of the conversation list gets a 401, the list either removes the account because its server requested a wipe, or shows the dialog to reauthorize or remove it. The removal runs on in its own coroutine and worker until the account is gone, which can take a while, e.g. to unregister push notifications. A 401 of another sync meanwhile, e.g. after a resume, was handled again: the server was not asked about a wipe a second time, so the dialog was shown for the account that was being removed, and "remove account" there started another removal. Now further 401s are ignored from starting to remove the account until the removal succeeded or failed, for a wipe and for "remove account". Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
… later After the account the server requested a wipe of was removed, RemoteWipeSuccessWorker reports that to the server. It ran without waiting for a network connection, and it gave up for good when sending the report failed, e.g. when the device had gone offline meanwhile. It also counted any answer of the server as success, e.g. a 429 of its rate limit or a server error. Then the server kept waiting for the wipe to be done, and the admin never saw it confirmed. Now the report waits for a network connection. A failed request, a 429 or a server error is tried again later with an increasing delay, up to ten times. Other answers, e.g. a 404 when the server does not know the wipe of the token any more, end it, as trying again would not help. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
mahibi
force-pushed
the
checkIfServerRequestedWipe
branch
from
October 7, 2026 15:58
4f0ebaf to
a10cbf1
Compare
mahibi
marked this pull request as ready for review
October 7, 2026 15:59
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:
7f524db7-aeb1-4143-949f-50d4ab8605a7
📒 Files selected for processing (4)
app/src/main/java/com/nextcloud/talk/conversationlist/ConversationsListActivity.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/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.
rapterjet2004
approved these changes
Oct 7, 2026
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wipe handling (7cefae42d)
Why the reauthorize dialog (c165284c1)
Why the 429 work (e0665f3ea, 7c7015fd2, adc751ea5, a8da135a7)
Why the sync errors carry the account (d0bb25ca6)
How to test
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)