From 1b6923f1b3624ea09b01d1caed2253f1fedc1544 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Thu, 3 Sep 2026 10:53:07 +0200 Subject: [PATCH 1/5] fix(login): unconditionally proceed with login after account verification proceedWithLogin() skipped setting the account active and navigating onward whenever the newly stored account already looked like the active user in the DB. Since storeProfile() always inserts new accounts with current=true and the current-user query picks the row with the highest id, this was true for any 2nd+ account added, leaving the verification screen stuck forever. Why the check existed: It was added in ac5061d8a (2023) to stop proceedWithLogin() firing more than once for the same account and launching ConversationsListActivity twice. SignalingSettingsWorker loops over every logged-in user and posts one SIGNALING_SETTINGS event per account, and it is triggered both from this verification flow and, separately, from NextcloudTalkApplication.initWorkers() on every app start - so two matching events for the same account were possible while this activity was still alive and subscribed. The guard relied on currentUser still pointing at the previously-active account until setUserAsActive() had genuinely run once; that held back then because the underlying query had no ORDER BY and an unordered scan happened to return the older row first. f4157de71 (2026), for an unrelated reason (deterministic duplicate-account cleanup), made that query ORDER BY id DESC LIMIT 1, which flipped the tie-break to always favor the newest row and silently invalidated this guard's assumption. Possible follow-ups to restore the intent safely: - Guard against a genuine duplicate SIGNALING_SETTINGS event with a local instance flag in the activity instead of a DB query, so it no longer depends on current-user tie-break timing. - Give ConversationsListActivity a launchMode (singleTask/singleTop) or launch it with FLAG_ACTIVITY_SINGLE_TOP/CLEAR_TOP, since it has none today and any duplicate startActivity() call stacks a second instance. - Scope SignalingSettingsWorker's event posting to the account being verified instead of looping over and emitting for every user, to remove the cross-trigger duplicate risk at the source. - Audit other reads of getActiveUser()/getActiveUserObservable()/ getActiveUserSynchronously() for the same class of bug, since any code written before f4157de71 may have relied on the old (effectively oldest-row) tie-break behavior. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe --- .../account/AccountVerificationActivity.kt | 33 +++++++------------ 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt b/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt index a26b37fa4b..75b6e27eca 100644 --- a/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt @@ -519,32 +519,21 @@ class AccountVerificationActivity : BaseActivity() { private fun proceedWithLogin() { cookieManager.cookieStore.removeAll() - if (userManager.users.blockingGet().size == 1 || - currentUserProviderOld.currentUser.blockingGet().id != internalAccountId - ) { - val userToSetAsActive = userManager.getUserWithId(internalAccountId).blockingGet() - Log.d(TAG, "userToSetAsActive: " + userToSetAsActive.username) + val userToSetAsActive = userManager.getUserWithId(internalAccountId).blockingGet() + Log.d(TAG, "userToSetAsActive: " + userToSetAsActive.username) - if (userManager.setUserAsActive(userToSetAsActive).blockingGet()) { - runOnUiThread { - if (userManager.users.blockingGet().size == 1) { - val intent = Intent(context, ConversationsListActivity::class.java) - startActivity(intent) - } else { - if (isAccountImport) { - ApplicationWideMessageHolder.getInstance().messageType = - ApplicationWideMessageHolder.MessageType.ACCOUNT_WAS_IMPORTED - } - val intent = Intent(context, ConversationsListActivity::class.java) - startActivity(intent) - } + if (userManager.setUserAsActive(userToSetAsActive).blockingGet()) { + runOnUiThread { + if (userManager.users.blockingGet().size > 1 && isAccountImport) { + ApplicationWideMessageHolder.getInstance().messageType = + ApplicationWideMessageHolder.MessageType.ACCOUNT_WAS_IMPORTED } - } else { - Log.e(TAG, "failed to set active user") - Snackbar.make(binding.root, R.string.nc_common_error_sorry, Snackbar.LENGTH_LONG).show() + val intent = Intent(context, ConversationsListActivity::class.java) + startActivity(intent) } } else { - Log.d(TAG, "continuing proceedWithLogin was skipped for this user") + Log.e(TAG, "failed to set active user") + Snackbar.make(binding.root, R.string.nc_common_error_sorry, Snackbar.LENGTH_LONG).show() } } From 074316878452479299d077c960511886251c7c6b Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Thu, 3 Sep 2026 12:02:58 +0200 Subject: [PATCH 2/5] fix(login): don't mark a new account current before it's verified storeProfile() inserted a brand-new account with current=true right away, before capabilities/push/signaling verification had even run. Nothing in that verification pipeline needs the row to be current - CapabilitiesWorker, SignalingSettingsWorker and setupPushNotifications() all operate on the explicit internal user id, and push registration iterates every account regardless of its current flag. Marking it current here only created a window where two rows were current=1 at once (the original source of the stuck-login bug), and some consumers that read the raw per-row current flag instead of the resolved active user (e.g. the account switcher/import lists filtering on "!user.current") can misbehave while that window is open. setUserAsActive(), called from proceedWithLogin() once verification succeeds, is the only place in the app that flips a row's current flag via the atomic single-owner update - so let it stay the only one. ELI5: The app keeps a notebook with a checkmark next to whichever account is "currently logged in". Adding a second account put a checkmark next to the new account immediately, before even checking that it worked, and never erased the old account's checkmark - so two names ended up checked at once. A separate, unrelated rule said "if I'm done setting up an account and it's already checked, I must have handled this already - skip it". With two checkmarks, that rule fired immediately on the very first (and only) real completion, so the app skipped the one step that opens your chats and just froze. Fixing this means the new account only gets checked off once it's actually confirmed to work, so there's never a two-checkmark mix-up to trip that rule. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe --- .../com/nextcloud/talk/account/AccountVerificationActivity.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt b/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt index 75b6e27eca..6e10764a27 100644 --- a/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt @@ -240,7 +240,7 @@ class AccountVerificationActivity : BaseActivity() { UserManager.UserAttributes( id = null, serverUrl = baseUrl, - currentUser = true, + currentUser = false, userId = userId, token = token, displayName = displayName, From 57a0d6cfb5b3bfc3fdf18ea4f90a7fefd8b5486c Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Thu, 3 Sep 2026 12:33:58 +0200 Subject: [PATCH 3/5] fix(account): update the shared active-user cache synchronously After switching the active account, ConversationsListActivity/ ConversationsListViewModel and the account-chooser dialog could show two different accounts as "current" at the same time. Both read CurrentUserProviderOld's app-wide singleton cache, which only ever refreshed asynchronously via Room's InvalidationTracker noticing the `current` column changed. AccountVerificationActivity.proceedWithLogin() flips that column and, almost immediately after, starts a new ConversationsListActivity - fast enough to usually win the race against the async cache refresh, so the freshly-launched screen (and its ViewModel, which drives which account's rooms/credentials get loaded, not just the toolbar avatar) captured the stale, previous account. The account-chooser dialog re-reads the same singleton fresh on every recomposition, so by the time it was opened later the async refresh had usually caught up - producing two screens that disagreed. UserManager.setUserAsActive() is the sole place in the app that flips which account is active (see the two preceding commits), so it's the right place to make that change observable immediately: it now pushes the newly-active user into a serialized BehaviorSubject synchronously, in addition to Room's own reactive query still feeding that subject as a backstop for any change to the `current` flag that doesn't go through setUserAsActive(). CurrentUserProviderOld/CurrentUserProvider need no changes themselves, since they already just observe UserManager.currentUserObservable - they benefit automatically. This only works if every consumer shares the same UserManager instance, which it turns out they didn't: provideUserManager() in UserModule had no scope, so Dagger handed out a fresh UserManager (and thus a disconnected subject) per injection site despite the app having a single @Singleton component. Scoped it @Singleton to match how every dependent class already assumed it behaved. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe --- .../com/nextcloud/talk/users/UserManager.kt | 27 ++++++++++++++++--- .../talk/utils/database/user/UserModule.kt | 2 ++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/users/UserManager.kt b/app/src/main/java/com/nextcloud/talk/users/UserManager.kt index 7ec21adbbf..42aa26ff1a 100644 --- a/app/src/main/java/com/nextcloud/talk/users/UserManager.kt +++ b/app/src/main/java/com/nextcloud/talk/users/UserManager.kt @@ -19,6 +19,8 @@ import com.nextcloud.talk.models.json.push.PushConfigurationState import io.reactivex.Maybe import io.reactivex.Observable import io.reactivex.Single +import io.reactivex.subjects.BehaviorSubject +import io.reactivex.subjects.Subject @Suppress("TooManyFunctions") class UserManager internal constructor(private val userRepository: UsersRepository) { @@ -34,10 +36,24 @@ class UserManager internal constructor(private val userRepository: UsersReposito .switchIfEmpty(Maybe.defer { getAnyUserAndSetAsActive() }) } + /** + * Backed by [activeUserSubject] rather than [UsersRepository.getActiveUserObservable] directly, so that + * [setUserAsActive] can push the newly-active user out synchronously the moment it succeeds, instead of + * consumers having to wait for Room's invalidation-tracker round trip to notice the DB write and re-query. + * That round trip is asynchronous and was racing against code (e.g. AccountVerificationActivity. + * proceedWithLogin()) that both changes the active user and immediately acts as if every observer already + * knows about it - e.g. launching a screen for the new user before its avatar/data had actually updated. + * Room's own observable is still relied on underneath to seed this and to catch any change to the `current` + * flag that doesn't go through [setUserAsActive]. + */ val currentUserObservable: Observable - get() { - return userRepository.getActiveUserObservable() - } + get() = activeUserSubject + + private val activeUserSubject: Subject by lazy { + val subject = BehaviorSubject.create().toSerialized() + userRepository.getActiveUserObservable().subscribe(subject::onNext) { } + subject + } fun deleteUser(internalId: Long): Int = userRepository.deleteUser(userRepository.getUserWithId(internalId).blockingGet()) @@ -156,6 +172,11 @@ class UserManager internal constructor(private val userRepository: UsersReposito fun setUserAsActive(user: User): Single { Log.d(TAG, "setUserAsActive:" + user.id!!) return userRepository.setUserAsActiveWithId(user.id!!) + .doOnSuccess { success -> + if (success) { + activeUserSubject.onNext(user) + } + } } fun storeProfile(username: String?, userAttributes: UserAttributes): Maybe = diff --git a/app/src/main/java/com/nextcloud/talk/utils/database/user/UserModule.kt b/app/src/main/java/com/nextcloud/talk/utils/database/user/UserModule.kt index 4d42d4e041..07036cc858 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/database/user/UserModule.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/database/user/UserModule.kt @@ -14,6 +14,7 @@ import com.nextcloud.talk.users.UserManager import dagger.Binds import dagger.Module import dagger.Provides +import javax.inject.Singleton @Module(includes = [DatabaseModule::class]) abstract class UserModule { @@ -28,6 +29,7 @@ abstract class UserModule { companion object { @Provides + @Singleton fun provideUserManager(userRepository: UsersRepository): UserManager = UserManager(userRepository) } } From 91d7d173ef4b29d7011f972a5c3669a397f54b3e Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Thu, 3 Sep 2026 12:49:22 +0200 Subject: [PATCH 4/5] refactor(account): expose the active-user signal natively as a StateFlow CurrentUserProviderImpl is coroutine-based but had to bridge into Flow via userManager.currentUserObservable.asFlow().stateIn(...) on Dispatchers.IO - an unnecessary RxJava round trip and an extra coroutine dispatch hop for a value UserManager already updates synchronously in memory. UserManager now also pushes into a MutableStateFlow at the same point it pushes into the existing RxJava subject (inside setUserAsActive()), so CurrentUserProviderImpl can read it directly, with no scope of its own to manage and no thread hop between the write and the value being observable. CurrentUserProviderOld keeps using the RxJava-based currentUserObservable unchanged, since it's a deprecated, non-coroutine class not worth converting further. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe --- .../com/nextcloud/talk/users/UserManager.kt | 20 +++++++++++++++++++ .../database/user/CurrentUserProviderImpl.kt | 19 +----------------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/users/UserManager.kt b/app/src/main/java/com/nextcloud/talk/users/UserManager.kt index 42aa26ff1a..8a785130a1 100644 --- a/app/src/main/java/com/nextcloud/talk/users/UserManager.kt +++ b/app/src/main/java/com/nextcloud/talk/users/UserManager.kt @@ -21,6 +21,8 @@ import io.reactivex.Observable import io.reactivex.Single import io.reactivex.subjects.BehaviorSubject import io.reactivex.subjects.Subject +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow @Suppress("TooManyFunctions") class UserManager internal constructor(private val userRepository: UsersRepository) { @@ -45,16 +47,33 @@ class UserManager internal constructor(private val userRepository: UsersReposito * knows about it - e.g. launching a screen for the new user before its avatar/data had actually updated. * Room's own observable is still relied on underneath to seed this and to catch any change to the `current` * flag that doesn't go through [setUserAsActive]. + * + * RxJava-based for CurrentUserProviderOld, the still-used but deprecated consumer. Coroutine-based code + * should prefer [currentUserFlow] instead, which is updated at the exact same point and needs no RxJava + * bridging on the consuming side. */ val currentUserObservable: Observable get() = activeUserSubject + /** + * Coroutine-native counterpart to [currentUserObservable] - see its doc for why this exists. Both are + * updated synchronously, at the same point in [setUserAsActive], from Room's same underlying query. + */ + val currentUserFlow: StateFlow + get() = activeUserStateFlow + private val activeUserSubject: Subject by lazy { val subject = BehaviorSubject.create().toSerialized() userRepository.getActiveUserObservable().subscribe(subject::onNext) { } subject } + private val activeUserStateFlow: MutableStateFlow by lazy { + val flow = MutableStateFlow(null) + userRepository.getActiveUserObservable().subscribe({ flow.value = it }) { } + flow + } + fun deleteUser(internalId: Long): Int = userRepository.deleteUser(userRepository.getUserWithId(internalId).blockingGet()) @@ -175,6 +194,7 @@ class UserManager internal constructor(private val userRepository: UsersReposito .doOnSuccess { success -> if (success) { activeUserSubject.onNext(user) + activeUserStateFlow.value = user } } } diff --git a/app/src/main/java/com/nextcloud/talk/utils/database/user/CurrentUserProviderImpl.kt b/app/src/main/java/com/nextcloud/talk/utils/database/user/CurrentUserProviderImpl.kt index 25bfdbe0f6..abc2ee7820 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/database/user/CurrentUserProviderImpl.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/database/user/CurrentUserProviderImpl.kt @@ -9,16 +9,9 @@ package com.nextcloud.talk.utils.database.user import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.users.UserManager -import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.SharingStarted -import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.first -import kotlinx.coroutines.flow.stateIn -import kotlinx.coroutines.rx2.asFlow import kotlinx.coroutines.withTimeoutOrNull import javax.inject.Inject import javax.inject.Singleton @@ -26,18 +19,8 @@ import javax.inject.Singleton @Singleton class CurrentUserProviderImpl @Inject constructor(private val userManager: UserManager) : CurrentUserProvider { - private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO) - - private val currentUser: StateFlow = userManager.currentUserObservable - .asFlow() - .stateIn( - scope = scope, - started = SharingStarted.Eagerly, - initialValue = null - ) - // only emit non-null users - override val currentUserFlow: Flow = currentUser.filterNotNull() + override val currentUserFlow: Flow = userManager.currentUserFlow.filterNotNull() // function for safe one-shot access override suspend fun getCurrentUser(timeout: Long): Result { From df373b778d065d2359e151a9987ce30da5cd55d4 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Thu, 3 Sep 2026 13:10:11 +0200 Subject: [PATCH 5/5] fix(account): clear stale ConversationsListActivity instance after adding an account proceedWithLogin() started a new ConversationsListActivity with a plain Intent and no flags. If an instance of it was already resident in the back stack - e.g. the conversation list was open for the current account when the user added a second one from within it - that old instance was left behind rather than replaced, reachable via back navigation and showing stale data (its own account, credentials, avatar) forever, since it's a different Activity instance that never gets recreated. Add FLAG_ACTIVITY_CLEAR_TOP, matching the pattern already used by ChooseAccountDialogCompose's own manual "switch to this account" row. ConversationsListActivity uses the default 'standard' launch mode, so this finishes that stale instance and starts a genuinely fresh one via onCreate() instead of leaving it in place - which, combined with the preceding commit making UserManager's active-user signal update synchronously, means the fresh instance reads the correct, currently active user. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe --- .../com/nextcloud/talk/account/AccountVerificationActivity.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt b/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt index 6e10764a27..92d72e2668 100644 --- a/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt @@ -529,6 +529,7 @@ class AccountVerificationActivity : BaseActivity() { ApplicationWideMessageHolder.MessageType.ACCOUNT_WAS_IMPORTED } val intent = Intent(context, ConversationsListActivity::class.java) + intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP) startActivity(intent) } } else {